diff mbox series

[v6,21/33] block: move BQL logic of bdrv_co_invalidate_cache in bdrv_activate

Message ID 20220121170544.2049944-22-eesposit@redhat.com
State New
Headers show
Series block layer: split block APIs in global state and I/O | expand

Commit Message

Emanuele Giuseppe Esposito Jan. 21, 2022, 5:05 p.m. UTC
Split bdrv_co_invalidate cache in two: the GS code that takes
care of permissions and running GS callbacks, and leave only the
I/O code (->bdrv_co_invalidate_cache) running in the I/O coroutine.

The only side effect is that bdrv_co_invalidate_cache is not
recursive anymore, and so is every direct call to
bdrv_invalidate_cache().

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

Comments

Hanna Czenczek Jan. 26, 2022, 12:20 p.m. UTC | #1
On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote:
> Split bdrv_co_invalidate cache in two: the GS code that takes
> care of permissions and running GS callbacks, and leave only the
> I/O code (->bdrv_co_invalidate_cache) running in the I/O coroutine.
>
> The only side effect is that bdrv_co_invalidate_cache is not
> recursive anymore, and so is every direct call to
> bdrv_invalidate_cache().
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c | 36 +++++++++++++++++++++---------------
>   1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/block.c b/block.c
> index 7ab5031027..bad834c86e 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -6579,7 +6576,7 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
>        * Note that the required permissions of inactive images are always a
>        * subset of the permissions required after activating the image. This
>        * allows us to just get the permissions upfront without restricting
> -     * drv->bdrv_invalidate_cache().
> +     * drv->bdrv_co_invalidate_cache().

Perhaps just `bdrv_invalidate_cache()`, without the `drv->`?

>        *
>        * It also means that in error cases, we don't have to try and revert to
>        * the old permissions (which is an operation that could fail, too). We can
> @@ -6594,14 +6591,7 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
>               return ret;
>           }
>   
> -        if (bs->drv->bdrv_co_invalidate_cache) {
> -            bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
> -            if (local_err) {
> -                bs->open_flags |= BDRV_O_INACTIVE;
> -                error_propagate(errp, local_err);
> -                return -EINVAL;
> -            }
> -        }
> +        bdrv_invalidate_cache(bs, errp);

We should check the returned value here, and return on error.

Other than that, looks good and makes sense.

Hanna

>           FOR_EACH_DIRTY_BITMAP(bs, bm) {
>               bdrv_dirty_bitmap_skip_store(bm, false);
Kevin Wolf Jan. 27, 2022, 11:03 a.m. UTC | #2
Am 21.01.2022 um 18:05 hat Emanuele Giuseppe Esposito geschrieben:
> Split bdrv_co_invalidate cache in two: the GS code that takes
> care of permissions and running GS callbacks, and leave only the
> I/O code (->bdrv_co_invalidate_cache) running in the I/O coroutine.
> 
> The only side effect is that bdrv_co_invalidate_cache is not
> recursive anymore, and so is every direct call to
> bdrv_invalidate_cache().
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 7ab5031027..bad834c86e 100644
> --- a/block.c
> +++ b/block.c
> @@ -6550,23 +6550,20 @@ void bdrv_init_with_whitelist(void)
>  }
>  
>  int bdrv_activate(BlockDriverState *bs, Error **errp)
> -{
> -    return bdrv_invalidate_cache(bs, errp);
> -}
> -
> -int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
>  {
>      BdrvChild *child, *parent;
>      Error *local_err = NULL;
>      int ret;
>      BdrvDirtyBitmap *bm;
>  
> +    assert(qemu_in_main_thread());
> +
>      if (!bs->drv)  {
>          return -ENOMEDIUM;
>      }
>  
>      QLIST_FOREACH(child, &bs->children, next) {
> -        bdrv_co_invalidate_cache(child->bs, &local_err);
> +        bdrv_activate(child->bs, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>              return -EINVAL;
> @@ -6579,7 +6576,7 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
>       * Note that the required permissions of inactive images are always a
>       * subset of the permissions required after activating the image. This
>       * allows us to just get the permissions upfront without restricting
> -     * drv->bdrv_invalidate_cache().
> +     * drv->bdrv_co_invalidate_cache().
>       *
>       * It also means that in error cases, we don't have to try and revert to
>       * the old permissions (which is an operation that could fail, too). We can
> @@ -6594,14 +6591,7 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
>              return ret;
>          }
>  
> -        if (bs->drv->bdrv_co_invalidate_cache) {
> -            bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
> -            if (local_err) {
> -                bs->open_flags |= BDRV_O_INACTIVE;
> -                error_propagate(errp, local_err);
> -                return -EINVAL;
> -            }
> -        }
> +        bdrv_invalidate_cache(bs, errp);
>  
>          FOR_EACH_DIRTY_BITMAP(bs, bm) {
>              bdrv_dirty_bitmap_skip_store(bm, false);
> @@ -6629,6 +6619,22 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
>      return 0;
>  }
>  
> +int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
> +{
> +    Error *local_err = NULL;
> +
> +    if (bs->drv->bdrv_co_invalidate_cache) {
> +        bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
> +        if (local_err) {
> +            bs->open_flags |= BDRV_O_INACTIVE;

This doesn't feel like the right place. The flag is cleared by the
caller, so it should also be set again on failure by the caller and not
by this function.

What bdrv_co_invalidate_cache() could do is assert that BDRV_O_INACTIVE
is cleared when it's called.

> +            error_propagate(errp, local_err);
> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}

Kevin
Paolo Bonzini Feb. 2, 2022, 5:27 p.m. UTC | #3
On 1/27/22 12:03, Kevin Wolf wrote:
>> +int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +
>> +    if (bs->drv->bdrv_co_invalidate_cache) {
>> +        bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
>> +        if (local_err) {
>> +            bs->open_flags |= BDRV_O_INACTIVE;
> 
> This doesn't feel like the right place. The flag is cleared by the
> caller, so it should also be set again on failure by the caller and not
> by this function.
> 
> What bdrv_co_invalidate_cache() could do is assert that BDRV_O_INACTIVE
> is cleared when it's called.

Do you think this would be handled more easily into its own series?

In general, the work in this series is more incremental than its size 
suggests.  Perhaps it should be flushed out in smaller pieces.

Paolo
Kevin Wolf Feb. 2, 2022, 6:27 p.m. UTC | #4
Am 02.02.2022 um 18:27 hat Paolo Bonzini geschrieben:
> On 1/27/22 12:03, Kevin Wolf wrote:
> > > +int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
> > > +{
> > > +    Error *local_err = NULL;
> > > +
> > > +    if (bs->drv->bdrv_co_invalidate_cache) {
> > > +        bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
> > > +        if (local_err) {
> > > +            bs->open_flags |= BDRV_O_INACTIVE;
> > 
> > This doesn't feel like the right place. The flag is cleared by the
> > caller, so it should also be set again on failure by the caller and not
> > by this function.
> > 
> > What bdrv_co_invalidate_cache() could do is assert that BDRV_O_INACTIVE
> > is cleared when it's called.
> 
> Do you think this would be handled more easily into its own series?
> 
> In general, the work in this series is more incremental than its size
> suggests.  Perhaps it should be flushed out in smaller pieces.

Smaller pieces are always easier to handle, so if things make sense
independently, splitting them off is usually a good idea.

Kevin
diff mbox series

Patch

diff --git a/block.c b/block.c
index 7ab5031027..bad834c86e 100644
--- a/block.c
+++ b/block.c
@@ -6550,23 +6550,20 @@  void bdrv_init_with_whitelist(void)
 }
 
 int bdrv_activate(BlockDriverState *bs, Error **errp)
-{
-    return bdrv_invalidate_cache(bs, errp);
-}
-
-int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
 {
     BdrvChild *child, *parent;
     Error *local_err = NULL;
     int ret;
     BdrvDirtyBitmap *bm;
 
+    assert(qemu_in_main_thread());
+
     if (!bs->drv)  {
         return -ENOMEDIUM;
     }
 
     QLIST_FOREACH(child, &bs->children, next) {
-        bdrv_co_invalidate_cache(child->bs, &local_err);
+        bdrv_activate(child->bs, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return -EINVAL;
@@ -6579,7 +6576,7 @@  int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
      * Note that the required permissions of inactive images are always a
      * subset of the permissions required after activating the image. This
      * allows us to just get the permissions upfront without restricting
-     * drv->bdrv_invalidate_cache().
+     * drv->bdrv_co_invalidate_cache().
      *
      * It also means that in error cases, we don't have to try and revert to
      * the old permissions (which is an operation that could fail, too). We can
@@ -6594,14 +6591,7 @@  int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
             return ret;
         }
 
-        if (bs->drv->bdrv_co_invalidate_cache) {
-            bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
-            if (local_err) {
-                bs->open_flags |= BDRV_O_INACTIVE;
-                error_propagate(errp, local_err);
-                return -EINVAL;
-            }
-        }
+        bdrv_invalidate_cache(bs, errp);
 
         FOR_EACH_DIRTY_BITMAP(bs, bm) {
             bdrv_dirty_bitmap_skip_store(bm, false);
@@ -6629,6 +6619,22 @@  int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
     return 0;
 }
 
+int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
+{
+    Error *local_err = NULL;
+
+    if (bs->drv->bdrv_co_invalidate_cache) {
+        bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
+        if (local_err) {
+            bs->open_flags |= BDRV_O_INACTIVE;
+            error_propagate(errp, local_err);
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
 void bdrv_activate_all(Error **errp)
 {
     BlockDriverState *bs;