diff mbox series

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

Message ID 20210419085541.22310-5-eesposit@redhat.com (mailing list archive)
State New, archived
Headers show
Series Block layer thread-safety, continued | expand

Commit Message

Emanuele Giuseppe Esposito April 19, 2021, 8:55 a.m. UTC 
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.

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(-)

Comments

Vladimir Sementsov-Ogievskiy April 21, 2021, 9:23 p.m. UTC | #1 
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:
>    *
>
Vladimir Sementsov-Ogievskiy April 21, 2021, 10:17 p.m. UTC | #2 
22.04.2021 00:23, Vladimir Sementsov-Ogievskiy wrote:
> 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.

But on the other hand.. Why not. We can go without filter and still drop write-notifiers. Look at my new patch "[PATCH] block: simplify write-threshold and drop write notifiers"
diff mbox series

Patch

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)
 {
     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:
  *