Re: [PATCH v10 07/12] parallels: Move check of cluster outside image to a separate function

[Hanna Czenczek]

On 10.03.23 12:08, Alexander Ivanov wrote:
> On 3/7/23 13:17, Hanna Czenczek wrote:
> > On 03.02.23 10:18, Alexander Ivanov wrote:
> > > We will add more and more checks so we need a better code structure in
> > > parallels_co_check. Let each check performs in a separate loop in a
> > > separate helper.
> > >
> > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> > > Reviewed-by: Denis V. Lunev <den@openvz.org>
> > > ---
> > >   block/parallels.c | 81 
> > > ++++++++++++++++++++++++++++++-----------------
> > >   1 file changed, 52 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/block/parallels.c b/block/parallels.c
> > > index 02fbaee1f2..f9acee1fa8 100644
> > > --- a/block/parallels.c
> > > +++ b/block/parallels.c
> > > @@ -438,14 +438,13 @@ static void 
> > > parallels_check_unclean(BlockDriverState *bs,
> > >       }
> > >   }
> > >   -static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> > > -                                           BdrvCheckResult *res,
> > > -                                           BdrvCheckMode fix)
> > > +static int parallels_check_outside_image(BlockDriverState *bs,
> > > +                                         BdrvCheckResult *res,
> > > +                                         BdrvCheckMode fix)
> >
> > I wonder, should we mark this function coroutine_fn?  And with the 
> > graph lock changes that happened in the meantime, probably also 
> > GRAPH_RDLOCK (because it’s calling bdrv_getlength()).
> >
> > Hanna
> Thank you for your review.
>
> It seems, parallels_collect_statistics(), parallels_check_unclean() 
> and parallels_set_bat_entry() also should be marked as coroutine_fn, 
> shouldn't they?

There should be no harm in doing so.  I wasn’t mentioning it just 
because those functions don’t call other coroutine functions themselves, 
so I think it doesn’t really matter.

Hanna