| Message ID | 20220121170544.2049944-22-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block layer: split block APIs in global state and I/O | expand |
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);
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
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
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 --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;
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(-)