Re: [PATCH v2 4/8] block: make before-write notifiers thread-safe

[Vladimir Sementsov-Ogievskiy]

19.04.2021 11:55, Emanuele Giuseppe Esposito wrote:
> Reads access the list in RCU style, so be careful to avoid use-after-free
> scenarios in the backup block job.  Apart from this, all that's needed
> is protecting updates with a mutex.

Note that backup doesn't use write-notifiers now. Probably best thing to do is 
to update other users to use filters instead of write notifiers, and drop write 
notifiers at all. But it may require more work, so don't care.

> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c                   |  6 +++---
>   block/io.c                | 12 ++++++++++++
>   block/write-threshold.c   |  2 +-
>   include/block/block_int.h | 37 +++++++++++++++++++++++++++++++++++++
>   4 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index c5b887cec1..f40fb63c75 100644
> --- a/block.c
> +++ b/block.c
> @@ -6434,7 +6434,7 @@ static void 
> bdrv_do_remove_aio_context_notifier(BdrvAioNotifier *ban)
>       g_free(ban);
>   }
>   
> -static void bdrv_detach_aio_context(BlockDriverState *bs)
> +void bdrv_detach_aio_context(BlockDriverState *bs)

why it become public? It's not used in this commit, and this change is 
unrelated to commit message..

>   {
>       BdrvAioNotifier *baf, *baf_tmp;
>   
> @@ -6462,8 +6462,8 @@ static void bdrv_detach_aio_context(BlockDriverState *bs)
>       bs->aio_context = NULL;
>   }
>   
> -static void bdrv_attach_aio_context(BlockDriverState *bs,
> -                                    AioContext *new_context)
> +void bdrv_attach_aio_context(BlockDriverState *bs,
> +                             AioContext *new_context)
>   {
>       BdrvAioNotifier *ban, *ban_tmp;
>   
> diff --git a/block/io.c b/block/io.c
> index ca2dca3007..8f6af6077b 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3137,10 +3137,22 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, 
> QEMUIOVector *qiov)
>       return true;
>   }
>   
> +static QemuSpin notifiers_spin_lock;
> +
>   void bdrv_add_before_write_notifier(BlockDriverState *bs,
>                                       NotifierWithReturn *notifier)
>   {
> +    qemu_spin_lock(&notifiers_spin_lock);
>       notifier_with_return_list_add(&bs->before_write_notifiers, notifier);
> +    qemu_spin_unlock(&notifiers_spin_lock);
> +}
> +
> +void bdrv_remove_before_write_notifier(BlockDriverState *bs,
> +                                       NotifierWithReturn *notifier)
> +{
> +    qemu_spin_lock(&notifiers_spin_lock);
> +    notifier_with_return_remove(notifier);
> +    qemu_spin_unlock(&notifiers_spin_lock);
>   }
>   
>   void bdrv_io_plug(BlockDriverState *bs)
> diff --git a/block/write-threshold.c b/block/write-threshold.c
> index 77c74bdaa7..b87b749b02 100644
> --- a/block/write-threshold.c
> +++ b/block/write-threshold.c
> @@ -32,7 +32,7 @@ bool bdrv_write_threshold_is_set(const BlockDriverState *bs)
>   static void write_threshold_disable(BlockDriverState *bs)
>   {
>       if (bdrv_write_threshold_is_set(bs)) {
> -        notifier_with_return_remove(&bs->write_threshold_notifier);
> +        bdrv_remove_before_write_notifier(bs, &bs->write_threshold_notifier);
>           bs->write_threshold_offset = 0;
>       }
>   }
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 88e4111939..a1aad5ad2d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1089,6 +1089,8 @@ bool bdrv_backing_overridden(BlockDriverState *bs);
>   
>   /**
>    * bdrv_add_before_write_notifier:
> + * @bs: The #BlockDriverState for which to register the notifier
> + * @notifier: The notifier to add
>    *
>    * Register a callback that is invoked before write requests are processed but
>    * after any throttling or waiting for overlapping requests.
> @@ -1096,6 +1098,41 @@ bool bdrv_backing_overridden(BlockDriverState *bs);
>   void bdrv_add_before_write_notifier(BlockDriverState *bs,
>                                       NotifierWithReturn *notifier);
>   
> +/**
> + * bdrv_remove_before_write_notifier:
> + * @bs: The #BlockDriverState for which to register the notifier
> + * @notifier: The notifier to add
> + *
> + * Unregister a callback that is invoked before write requests are processed 
> but
> + * after any throttling or waiting for overlapping requests.
> + *
> + * Note that more I/O could be pending on @bs.  You need to wait for
> + * it to finish, for example with bdrv_drain(), before freeing @notifier.
> + */
> +void bdrv_remove_before_write_notifier(BlockDriverState *bs,
> +                                       NotifierWithReturn *notifier);
> +
> +/**
> + * bdrv_detach_aio_context:
> + *
> + * May be called from .bdrv_detach_aio_context() to detach children from the
> + * current #AioContext.  This is only needed by block drivers that manage their
> + * own children.  Both ->file and ->backing are automatically handled and
> + * block drivers should not call this function on them explicitly.
> + */
> +void bdrv_detach_aio_context(BlockDriverState *bs);
> +
> +/**
> + * bdrv_attach_aio_context:
> + *
> + * May be called from .bdrv_attach_aio_context() to attach children to the new
> + * #AioContext.  This is only needed by block drivers that manage their own
> + * children.  Both ->file and ->backing are automatically handled and block
> + * drivers should not call this function on them explicitly.
> + */
> +void bdrv_attach_aio_context(BlockDriverState *bs,
> +                             AioContext *new_context);
> +
>   /**
>    * bdrv_add_aio_context_notifier:
>    *


--
Best regards,
Vladimir