drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

[Dan Carpenter]

Hi Mike,

First bad commit (maybe != root cause):

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head:   3d5ad2d4eca337e80f38df77de89614aa5aaceb9
commit: f36b1d3ba533d21b5b793623f05761b0297d114e dm vdo: use a proper Makefile for dm-vdo
config: x86_64-randconfig-161-20241019 (https://download.01.org/0day-ci/archive/20241019/202410191209.osrd8z9H-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202410191209.osrd8z9H-lkp@intel.com/

smatch warnings:
drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

vim +976 drivers/md/dm-vdo/data-vio.c

79535a7881c0cb Matthew Sakai 2023-11-16  958  void vdo_launch_bio(struct data_vio_pool *pool, struct bio *bio)
79535a7881c0cb Matthew Sakai 2023-11-16  959  {
79535a7881c0cb Matthew Sakai 2023-11-16  960  	struct data_vio *data_vio;
79535a7881c0cb Matthew Sakai 2023-11-16  961  
79535a7881c0cb Matthew Sakai 2023-11-16  962  	ASSERT_LOG_ONLY(!vdo_is_state_quiescent(&pool->state),
79535a7881c0cb Matthew Sakai 2023-11-16  963  			"data_vio_pool not quiescent on acquire");
79535a7881c0cb Matthew Sakai 2023-11-16  964  
79535a7881c0cb Matthew Sakai 2023-11-16  965  	bio->bi_private = (void *) jiffies;
79535a7881c0cb Matthew Sakai 2023-11-16  966  	spin_lock(&pool->lock);
79535a7881c0cb Matthew Sakai 2023-11-16  967  	if ((bio_op(bio) == REQ_OP_DISCARD) &&
79535a7881c0cb Matthew Sakai 2023-11-16  968  	    !acquire_permit(&pool->discard_limiter, bio))
79535a7881c0cb Matthew Sakai 2023-11-16  969  		return;
                                                        ^^^^^^

79535a7881c0cb Matthew Sakai 2023-11-16  970  
79535a7881c0cb Matthew Sakai 2023-11-16  971  	if (!acquire_permit(&pool->limiter, bio))
79535a7881c0cb Matthew Sakai 2023-11-16  972  		return;
                                                        ^^^^^^
Still holding pool->lock on these paths.

79535a7881c0cb Matthew Sakai 2023-11-16  973  
79535a7881c0cb Matthew Sakai 2023-11-16  974  	data_vio = get_available_data_vio(pool);
79535a7881c0cb Matthew Sakai 2023-11-16  975  	spin_unlock(&pool->lock);
79535a7881c0cb Matthew Sakai 2023-11-16 @976  	launch_bio(pool->completion.vdo, data_vio, bio);
79535a7881c0cb Matthew Sakai 2023-11-16  977  }
79535a7881c0cb Matthew Sakai 2023-11-16  978  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

[Matthew Sakai]

This should be addressed upstream by commit 
872564c501b72ae0c84af51084753e8652e4a84b ("dm vdo data-vio: silence 
sparse warnings about locking context imbalances")

That commit is from February. Would it be possible for these checks to 
use a more up-to-date version of the code before warning us about things 
that have already been addressed?

Matt

On 10/21/24 3:19 AM, Dan Carpenter wrote:
> Hi Mike,
> 
> First bad commit (maybe != root cause):
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   3d5ad2d4eca337e80f38df77de89614aa5aaceb9
> commit: f36b1d3ba533d21b5b793623f05761b0297d114e dm vdo: use a proper Makefile for dm-vdo
> config: x86_64-randconfig-161-20241019 (https://download.01.org/0day-ci/archive/20241019/202410191209.osrd8z9H-lkp@intel.com/config)
> compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202410191209.osrd8z9H-lkp@intel.com/
> 
> smatch warnings:
> drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.
> 
> vim +976 drivers/md/dm-vdo/data-vio.c
> 
> 79535a7881c0cb Matthew Sakai 2023-11-16  958  void vdo_launch_bio(struct data_vio_pool *pool, struct bio *bio)
> 79535a7881c0cb Matthew Sakai 2023-11-16  959  {
> 79535a7881c0cb Matthew Sakai 2023-11-16  960  	struct data_vio *data_vio;
> 79535a7881c0cb Matthew Sakai 2023-11-16  961
> 79535a7881c0cb Matthew Sakai 2023-11-16  962  	ASSERT_LOG_ONLY(!vdo_is_state_quiescent(&pool->state),
> 79535a7881c0cb Matthew Sakai 2023-11-16  963  			"data_vio_pool not quiescent on acquire");
> 79535a7881c0cb Matthew Sakai 2023-11-16  964
> 79535a7881c0cb Matthew Sakai 2023-11-16  965  	bio->bi_private = (void *) jiffies;
> 79535a7881c0cb Matthew Sakai 2023-11-16  966  	spin_lock(&pool->lock);
> 79535a7881c0cb Matthew Sakai 2023-11-16  967  	if ((bio_op(bio) == REQ_OP_DISCARD) &&
> 79535a7881c0cb Matthew Sakai 2023-11-16  968  	    !acquire_permit(&pool->discard_limiter, bio))
> 79535a7881c0cb Matthew Sakai 2023-11-16  969  		return;
>                                                          ^^^^^^
> 
> 79535a7881c0cb Matthew Sakai 2023-11-16  970
> 79535a7881c0cb Matthew Sakai 2023-11-16  971  	if (!acquire_permit(&pool->limiter, bio))
> 79535a7881c0cb Matthew Sakai 2023-11-16  972  		return;
>                                                          ^^^^^^
> Still holding pool->lock on these paths.
> 
> 79535a7881c0cb Matthew Sakai 2023-11-16  973
> 79535a7881c0cb Matthew Sakai 2023-11-16  974  	data_vio = get_available_data_vio(pool);
> 79535a7881c0cb Matthew Sakai 2023-11-16  975  	spin_unlock(&pool->lock);
> 79535a7881c0cb Matthew Sakai 2023-11-16 @976  	launch_bio(pool->completion.vdo, data_vio, bio);
> 79535a7881c0cb Matthew Sakai 2023-11-16  977  }
> 79535a7881c0cb Matthew Sakai 2023-11-16  978
> 

Re: drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

[Philip Li]

On Mon, Oct 28, 2024 at 07:00:40PM -0400, Matthew Sakai wrote:
> This should be addressed upstream by commit
> 872564c501b72ae0c84af51084753e8652e4a84b ("dm vdo data-vio: silence sparse
> warnings about locking context imbalances")
> 
> That commit is from February. Would it be possible for these checks to use a
> more up-to-date version of the code before warning us about things that have
> already been addressed?

Sorry about this Matt, the bot side will check why this happens and fix
the issue asap to avoid meaningless report.

> 
> Matt
> 
> On 10/21/24 3:19 AM, Dan Carpenter wrote:
> > Hi Mike,
> > 
> > First bad commit (maybe != root cause):
> > 
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head:   3d5ad2d4eca337e80f38df77de89614aa5aaceb9
> > commit: f36b1d3ba533d21b5b793623f05761b0297d114e dm vdo: use a proper Makefile for dm-vdo
> > config: x86_64-randconfig-161-20241019 (https://download.01.org/0day-ci/archive/20241019/202410191209.osrd8z9H-lkp@intel.com/config)
> > compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > | Closes: https://lore.kernel.org/r/202410191209.osrd8z9H-lkp@intel.com/
> > 
> > smatch warnings:
> > drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.
> > 
> > vim +976 drivers/md/dm-vdo/data-vio.c
> > 
> > 79535a7881c0cb Matthew Sakai 2023-11-16  958  void vdo_launch_bio(struct data_vio_pool *pool, struct bio *bio)
> > 79535a7881c0cb Matthew Sakai 2023-11-16  959  {
> > 79535a7881c0cb Matthew Sakai 2023-11-16  960  	struct data_vio *data_vio;
> > 79535a7881c0cb Matthew Sakai 2023-11-16  961
> > 79535a7881c0cb Matthew Sakai 2023-11-16  962  	ASSERT_LOG_ONLY(!vdo_is_state_quiescent(&pool->state),
> > 79535a7881c0cb Matthew Sakai 2023-11-16  963  			"data_vio_pool not quiescent on acquire");
> > 79535a7881c0cb Matthew Sakai 2023-11-16  964
> > 79535a7881c0cb Matthew Sakai 2023-11-16  965  	bio->bi_private = (void *) jiffies;
> > 79535a7881c0cb Matthew Sakai 2023-11-16  966  	spin_lock(&pool->lock);
> > 79535a7881c0cb Matthew Sakai 2023-11-16  967  	if ((bio_op(bio) == REQ_OP_DISCARD) &&
> > 79535a7881c0cb Matthew Sakai 2023-11-16  968  	    !acquire_permit(&pool->discard_limiter, bio))
> > 79535a7881c0cb Matthew Sakai 2023-11-16  969  		return;
> >                                                          ^^^^^^
> > 
> > 79535a7881c0cb Matthew Sakai 2023-11-16  970
> > 79535a7881c0cb Matthew Sakai 2023-11-16  971  	if (!acquire_permit(&pool->limiter, bio))
> > 79535a7881c0cb Matthew Sakai 2023-11-16  972  		return;
> >                                                          ^^^^^^
> > Still holding pool->lock on these paths.
> > 
> > 79535a7881c0cb Matthew Sakai 2023-11-16  973
> > 79535a7881c0cb Matthew Sakai 2023-11-16  974  	data_vio = get_available_data_vio(pool);
> > 79535a7881c0cb Matthew Sakai 2023-11-16  975  	spin_unlock(&pool->lock);
> > 79535a7881c0cb Matthew Sakai 2023-11-16 @976  	launch_bio(pool->completion.vdo, data_vio, bio);
> > 79535a7881c0cb Matthew Sakai 2023-11-16  977  }
> > 79535a7881c0cb Matthew Sakai 2023-11-16  978
> > 
> 
> 

Re: drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

[Philip Li]

On Tue, Oct 29, 2024 at 11:03:07AM +0800, Philip Li wrote:
> On Mon, Oct 28, 2024 at 07:00:40PM -0400, Matthew Sakai wrote:
> > This should be addressed upstream by commit
> > 872564c501b72ae0c84af51084753e8652e4a84b ("dm vdo data-vio: silence sparse
> > warnings about locking context imbalances")
> > 
> > That commit is from February. Would it be possible for these checks to use a
> > more up-to-date version of the code before warning us about things that have
> > already been addressed?
> 
> Sorry about this Matt, the bot side will check why this happens and fix
> the issue asap to avoid meaningless report.

Hi Matt and Dan, would you mind do a further check of this, per the re-test,
smatch warns as below on v6.12-rc3

	drivers/md/dm-vdo/data-vio.c:982 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.
	  Locked on  : 972,977
	  Unlocked on: 982

The corresponding code of drivers/md/dm-vdo/data-vio.c is below

 942 static void wait_permit(struct limiter *limiter, struct bio *bio)
 943         __releases(&limiter->pool->lock)
 944 {
 945         DEFINE_WAIT(wait);
 946
 947         bio_list_add(&limiter->new_waiters, bio);
 948         prepare_to_wait_exclusive(&limiter->blocked_threads, &wait,
 949                                   TASK_UNINTERRUPTIBLE);
 950         spin_unlock(&limiter->pool->lock);
 951         io_schedule();
 952         finish_wait(&limiter->blocked_threads, &wait);
 953 }
 954
 955 /**
 956  * vdo_launch_bio() - Acquire a data_vio from the pool, assign the bio to it, and launch it.
 957  *
 958  * This will block if data_vios or discard permits are not available.
 959  */
 960 void vdo_launch_bio(struct data_vio_pool *pool, struct bio *bio)
 961 {
 962         struct data_vio *data_vio;
 963
 964         VDO_ASSERT_LOG_ONLY(!vdo_is_state_quiescent(&pool->state),
 965                             "data_vio_pool not quiescent on acquire");
 966
 967         bio->bi_private = (void *) jiffies;
 968         spin_lock(&pool->lock);
 969         if ((bio_op(bio) == REQ_OP_DISCARD) &&
 970             !acquire_permit(&pool->discard_limiter)) {
 971                 wait_permit(&pool->discard_limiter, bio);
 972                 return;
 973         }
 974
 975         if (!acquire_permit(&pool->limiter)) {
 976                 wait_permit(&pool->limiter, bio);
 977                 return;
 978         }
 979
 980         data_vio = get_available_data_vio(pool);
 981         spin_unlock(&pool->lock);
 982         launch_bio(pool->completion.vdo, data_vio, bio);
 983 }

Thanks

> 
> > 
> > Matt
> > 
> > On 10/21/24 3:19 AM, Dan Carpenter wrote:
> > > Hi Mike,
> > > 
> > > First bad commit (maybe != root cause):
> > > 
> > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > head:   3d5ad2d4eca337e80f38df77de89614aa5aaceb9
> > > commit: f36b1d3ba533d21b5b793623f05761b0297d114e dm vdo: use a proper Makefile for dm-vdo
> > > config: x86_64-randconfig-161-20241019 (https://download.01.org/0day-ci/archive/20241019/202410191209.osrd8z9H-lkp@intel.com/config)
> > > compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
> > > 
> > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > the same patch/commit), kindly add following tags
> > > | Reported-by: kernel test robot <lkp@intel.com>
> > > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > | Closes: https://lore.kernel.org/r/202410191209.osrd8z9H-lkp@intel.com/
> > > 
> > > smatch warnings:
> > > drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.
> > > 
> > > vim +976 drivers/md/dm-vdo/data-vio.c
> > > 
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  958  void vdo_launch_bio(struct data_vio_pool *pool, struct bio *bio)
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  959  {
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  960  	struct data_vio *data_vio;
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  961
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  962  	ASSERT_LOG_ONLY(!vdo_is_state_quiescent(&pool->state),
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  963  			"data_vio_pool not quiescent on acquire");
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  964
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  965  	bio->bi_private = (void *) jiffies;
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  966  	spin_lock(&pool->lock);
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  967  	if ((bio_op(bio) == REQ_OP_DISCARD) &&
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  968  	    !acquire_permit(&pool->discard_limiter, bio))
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  969  		return;
> > >                                                          ^^^^^^
> > > 
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  970
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  971  	if (!acquire_permit(&pool->limiter, bio))
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  972  		return;
> > >                                                          ^^^^^^
> > > Still holding pool->lock on these paths.
> > > 
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  973
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  974  	data_vio = get_available_data_vio(pool);
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  975  	spin_unlock(&pool->lock);
> > > 79535a7881c0cb Matthew Sakai 2023-11-16 @976  	launch_bio(pool->completion.vdo, data_vio, bio);
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  977  }
> > > 79535a7881c0cb Matthew Sakai 2023-11-16  978
> > > 
> > 
> > 

Re: drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

[Dan Carpenter]

On Tue, Oct 29, 2024 at 03:15:47PM +0800, Philip Li wrote:
> On Tue, Oct 29, 2024 at 11:03:07AM +0800, Philip Li wrote:
> > On Mon, Oct 28, 2024 at 07:00:40PM -0400, Matthew Sakai wrote:
> > > This should be addressed upstream by commit
> > > 872564c501b72ae0c84af51084753e8652e4a84b ("dm vdo data-vio: silence sparse
> > > warnings about locking context imbalances")
> > > 
> > > That commit is from February. Would it be possible for these checks to use a
> > > more up-to-date version of the code before warning us about things that have
> > > already been addressed?
> > 
> > Sorry about this Matt, the bot side will check why this happens and fix
> > the issue asap to avoid meaningless report.
> 
> Hi Matt and Dan, would you mind do a further check of this, per the re-test,
> smatch warns as below on v6.12-rc3
> 
> 	drivers/md/dm-vdo/data-vio.c:982 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.
> 	  Locked on  : 972,977
> 	  Unlocked on: 982
> 
> The corresponding code of drivers/md/dm-vdo/data-vio.c is below
> 

Ah.  Right.

The cross function DB doesn't scale well enough for the zero day bot to use so
it didn't detect the fix.  If we had the cross function DB then that silences
the warning.

1) I re-wrote the locking check so it detected this bug where before it didn't.
2) The kbuild bot was using the new check on old code because Matthew Sakai
   did a branch based on 8 month old code.
3) The kbuild bot detected the bug, but unfortunately the cross function DB
   doesn't scale well enough for the kbuild bot to use so it didn't detect the
   fix.
4) I reviewed the code based on the information in the email and determined that
   it was buggy.

All those steps had to happen for the warning  to be sent out.  In a normal
situation, I would have sent the warning out at the time when the code was
written and you wouldn't be getting warning emails eight months later.  The
kbuild-bot generally avoids sending duplicate warnings.

Sometimes the kbuild bot does send duplicate warnings, but I normally delete
those.  Perhaps some people might argue that if you do a branch from 8 month old
code, maybe you would want the warnings, but I think you should look at the
Fixes tags instead.  Not everyone gets the Fixes tags right, of course...  But
I trust kernel developers to Fix their bugs and generally they do so duplicates
are normally false positives which have been deliberately ignored.

regards,
dan carpenter

Re: drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

[Dan Carpenter]

On Tue, Oct 29, 2024 at 11:07:07AM +0300, Dan Carpenter wrote:
> On Tue, Oct 29, 2024 at 03:15:47PM +0800, Philip Li wrote:
> > On Tue, Oct 29, 2024 at 11:03:07AM +0800, Philip Li wrote:
> > > On Mon, Oct 28, 2024 at 07:00:40PM -0400, Matthew Sakai wrote:
> > > > This should be addressed upstream by commit
> > > > 872564c501b72ae0c84af51084753e8652e4a84b ("dm vdo data-vio: silence sparse
> > > > warnings about locking context imbalances")
> > > > 
> > > > That commit is from February. Would it be possible for these checks to use a
> > > > more up-to-date version of the code before warning us about things that have
> > > > already been addressed?
> > > 
> > > Sorry about this Matt, the bot side will check why this happens and fix
> > > the issue asap to avoid meaningless report.
> > 
> > Hi Matt and Dan, would you mind do a further check of this, per the re-test,
> > smatch warns as below on v6.12-rc3
> > 
> > 	drivers/md/dm-vdo/data-vio.c:982 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.
> > 	  Locked on  : 972,977
> > 	  Unlocked on: 982
> > 
> > The corresponding code of drivers/md/dm-vdo/data-vio.c is below
> > 
> 
> Ah.  Right.
> 
> The cross function DB doesn't scale well enough for the zero day bot to use so
> it didn't detect the fix.  If we had the cross function DB then that silences
> the warning.
> 
> 1) I re-wrote the locking check so it detected this bug where before it didn't.
> 2) The kbuild bot was using the new check on old code because Matthew Sakai
>    did a branch based on 8 month old code.

That's not it, is it...  The branch was based on the latest code, and the
kbuild-bot bisected it back to 8 months ago and it happened to be right.

Anyway, it's a rare coincidence.

regards,
dan carpenter

Re: drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

[Dan Carpenter]

On Tue, Oct 29, 2024 at 11:07:07AM +0300, Dan Carpenter wrote:
> 3) The kbuild bot detected the bug, but unfortunately the cross function DB
>    doesn't scale well enough for the kbuild bot to use so it didn't detect the
>    fix.

Aw crud.  It does still print a warning on linux-next actually.
Smatch says that we lock "&pool->lock" and unlocked
"&pool->discard_limiter->pool->lock".

Anyway.  Points 1 and 4 that we were running new checks on old code and that
the code in the email did have a bug are correct.

regards,
dan carpenter

Re: drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

[Matthew Sakai]

Okay. Just to restate, what I took away from this thread is
1) The current code provokes a warning.
2) When bisecting, the bot found a (different) warning from the patch 
introducing this code, and so the mail points out that warning instead 
of the warning in newer code.
3) The warning is showing up now because new checking allows the bot to 
notice problems it didn't notice before.

The commit I cited fixed warning from sparse, but I don't think we 
looked at smatch. This particular code does not have a locking problem, 
but the way it's spelled makes it difficult for static tools to 
understand that there is not a problem. I'll take a look and see if 
there's anything further I can do address the actual current warning.

Thanks for your explanation. I think I have a much better understanding 
of what this bot it doing now, I'll keep it in mind when we get warning 
in the future.

Matt

On 10/29/24 4:19 AM, Dan Carpenter wrote:
> On Tue, Oct 29, 2024 at 11:07:07AM +0300, Dan Carpenter wrote:
>> 3) The kbuild bot detected the bug, but unfortunately the cross function DB
>>     doesn't scale well enough for the kbuild bot to use so it didn't detect the
>>     fix.
> 
> Aw crud.  It does still print a warning on linux-next actually.
> Smatch says that we lock "&pool->lock" and unlocked
> "&pool->discard_limiter->pool->lock".
> 
> Anyway.  Points 1 and 4 that we were running new checks on old code and that
> the code in the email did have a bug are correct.
> 
> regards,
> dan carpenter
> 

Re: drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

[Dan Carpenter]

On Tue, Oct 29, 2024 at 01:03:35PM -0400, Matthew Sakai wrote:
> Okay. Just to restate, what I took away from this thread is
> 1) The current code provokes a warning.
> 2) When bisecting, the bot found a (different) warning from the patch
> introducing this code, and so the mail points out that warning instead of
> the warning in newer code.

Not a different warning.  It's same warning.  It's just that the code in the
original commit was buggy and Smatch kept on warning about it even when it was
fixed.

> 3) The warning is showing up now because new checking allows the bot to
> notice problems it didn't notice before.
> 
> The commit I cited fixed warning from sparse, but I don't think we looked at
> smatch. This particular code does not have a locking problem, but the way
> it's spelled makes it difficult for static tools to understand that there is
> not a problem. I'll take a look and see if there's anything further I can do
> address the actual current warning.

Don't invest a lot of time into this.  I'm re-working the locking check really
heavily right now.  I know how to silence this warning when the cross function
DB is enabled.  It's just that I'm trying to decide the best way to do it.

regards,
dan carpenter

Re: drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.

[Dan Carpenter]

On Mon, Oct 28, 2024 at 07:00:40PM -0400, Matthew Sakai wrote:
> This should be addressed upstream by commit
> 872564c501b72ae0c84af51084753e8652e4a84b ("dm vdo data-vio: silence sparse
> warnings about locking context imbalances")
> 
> That commit is from February. Would it be possible for these checks to use a
> more up-to-date version of the code before warning us about things that have
> already been addressed?
> 

Sorry, these emails are auto generated by kbuild.  I don't get a lot of
context outside of what's in the email so I didn't realize it had already
been fixed. 

I looked at today's linux-next and that doesn't trigger the warning any more.

regards,
dan carpenter

> Matt
> 
> On 10/21/24 3:19 AM, Dan Carpenter wrote:
> > Hi Mike,
> > 
> > First bad commit (maybe != root cause):
> > 
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head:   3d5ad2d4eca337e80f38df77de89614aa5aaceb9
> > commit: f36b1d3ba533d21b5b793623f05761b0297d114e dm vdo: use a proper Makefile for dm-vdo
> > config: x86_64-randconfig-161-20241019 (https://download.01.org/0day-ci/archive/20241019/202410191209.osrd8z9H-lkp@intel.com/config)
> > compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > | Closes: https://lore.kernel.org/r/202410191209.osrd8z9H-lkp@intel.com/
> > 
> > smatch warnings:
> > drivers/md/dm-vdo/data-vio.c:976 vdo_launch_bio() warn: inconsistent returns '&pool->lock'.
> > 
> > vim +976 drivers/md/dm-vdo/data-vio.c
> > 
> > 79535a7881c0cb Matthew Sakai 2023-11-16  958  void vdo_launch_bio(struct data_vio_pool *pool, struct bio *bio)
> > 79535a7881c0cb Matthew Sakai 2023-11-16  959  {
> > 79535a7881c0cb Matthew Sakai 2023-11-16  960  	struct data_vio *data_vio;
> > 79535a7881c0cb Matthew Sakai 2023-11-16  961
> > 79535a7881c0cb Matthew Sakai 2023-11-16  962  	ASSERT_LOG_ONLY(!vdo_is_state_quiescent(&pool->state),
> > 79535a7881c0cb Matthew Sakai 2023-11-16  963  			"data_vio_pool not quiescent on acquire");
> > 79535a7881c0cb Matthew Sakai 2023-11-16  964
> > 79535a7881c0cb Matthew Sakai 2023-11-16  965  	bio->bi_private = (void *) jiffies;
> > 79535a7881c0cb Matthew Sakai 2023-11-16  966  	spin_lock(&pool->lock);
> > 79535a7881c0cb Matthew Sakai 2023-11-16  967  	if ((bio_op(bio) == REQ_OP_DISCARD) &&
> > 79535a7881c0cb Matthew Sakai 2023-11-16  968  	    !acquire_permit(&pool->discard_limiter, bio))
> > 79535a7881c0cb Matthew Sakai 2023-11-16  969  		return;
> >                                                          ^^^^^^
> > 
> > 79535a7881c0cb Matthew Sakai 2023-11-16  970
> > 79535a7881c0cb Matthew Sakai 2023-11-16  971  	if (!acquire_permit(&pool->limiter, bio))
> > 79535a7881c0cb Matthew Sakai 2023-11-16  972  		return;
> >                                                          ^^^^^^
> > Still holding pool->lock on these paths.
> > 
> > 79535a7881c0cb Matthew Sakai 2023-11-16  973
> > 79535a7881c0cb Matthew Sakai 2023-11-16  974  	data_vio = get_available_data_vio(pool);
> > 79535a7881c0cb Matthew Sakai 2023-11-16  975  	spin_unlock(&pool->lock);
> > 79535a7881c0cb Matthew Sakai 2023-11-16 @976  	launch_bio(pool->completion.vdo, data_vio, bio);
> > 79535a7881c0cb Matthew Sakai 2023-11-16  977  }
> > 79535a7881c0cb Matthew Sakai 2023-11-16  978
> >