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@xxxxxxxxxx>
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@xxxxxxxx>
SUSE Labs, CR
