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

[Kevin Wolf <kwolf@redhat.com>]

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