Re: [PATCH RFC 4/6] bdev: simplify waiting for concurrent claimers

[Jan Kara <jack@suse.cz>]

On Tue 24-10-23 16:53:42, Christian Brauner wrote:
> Simplify the mechanism to wait for concurrent block devices claimers
> and make it possible to introduce an additional state in the following
> patches.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

The simplification looks good but a few notes below:

> diff --git a/block/bdev.c b/block/bdev.c
> index 9deacd346192..7d19e04a8df8 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -482,6 +482,14 @@ static bool bd_may_claim(struct block_device *bdev, void *holder,
>  	return true;
>  }
>  
> +static bool wait_claimable(const struct block_device *bdev)
> +{
> +	enum bd_claim bd_claim;
> +
> +	bd_claim = smp_load_acquire(&bdev->bd_claim);
> +	return bd_claim == BD_CLAIM_DEFAULT;
> +}

Aren't you overdoing it here a bit? Given this is used only in a retry
loop and all the checks that need to be reliable are done under bdev_lock,
I'd say having:

	return READ_ONCE(bdev->bd_claim) == BD_CLAIM_DEFAULT;

shound be fine here? And probably just inline that into the
wait_var_event() call...

> @@ -511,31 +519,25 @@ int bd_prepare_to_claim(struct block_device *bdev, void *holder,
>  	}
>  
>  	/* if claiming is already in progress, wait for it to finish */
> -	if (whole->bd_claiming) {
> -		wait_queue_head_t *wq = bit_waitqueue(&whole->bd_claiming, 0);
> -		DEFINE_WAIT(wait);
> -
> -		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> +	if (whole->bd_claim) {

This test implicitely assumes that 0 is BD_CLAIM_DEFAULT. I guess that's
fine although I somewhat prefer explicit value test like:

	if (whole->bd_claim != BD_CLAIM_DEFAULT)

>  		mutex_unlock(&bdev_lock);
> -		schedule();
> -		finish_wait(wq, &wait);
> +		wait_var_event(&whole->bd_claim, wait_claimable(whole));
>  		goto retry;
>  	}
>  
>  	/* yay, all mine */
> -	whole->bd_claiming = holder;
> +	whole->bd_claim = BD_CLAIM_ACQUIRE;

Here I'd use WRITE_ONCE() to avoid KCSAN warnings and having to think
whether this can race with wait_claimable() or not.

>  	mutex_unlock(&bdev_lock);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(bd_prepare_to_claim); /* only for the loop driver */
>  
> -static void bd_clear_claiming(struct block_device *whole, void *holder)
> +static void bd_clear_claiming(struct block_device *whole)
>  {
>  	lockdep_assert_held(&bdev_lock);
> -	/* tell others that we're done */
> -	BUG_ON(whole->bd_claiming != holder);
> -	whole->bd_claiming = NULL;
> -	wake_up_bit(&whole->bd_claiming, 0);
> +	smp_store_release(&whole->bd_claim, BD_CLAIM_DEFAULT);
> +	smp_mb();
> +	wake_up_var(&whole->bd_claim);

And here since we are under bdev_lock and the waiter is going to check
under bdev_lock as well, we should be able to do:

	WRITE_ONCE(whole->bd_claim, BD_CLAIM_DEFAULT);
	/* Pairs with barrier in prepare_to_wait_event() -> set_current_state() */
	smp_mb();
	wake_up_var(&whole->bd_claim);

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR