Re: [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare

[Kevin Wolf]

Am 03.10.2019 um 11:33 hat Sergio Lopez geschrieben:
> 
> Sergio Lopez <address@hidden> writes:
> 
> > Kevin Wolf <address@hidden> writes:
> >
> >> Am 13.09.2019 um 21:54 hat John Snow geschrieben:
> >>> 
> >>> 
> >>> On 9/13/19 11:25 AM, Sergio Lopez wrote:
> >>> > do_drive_backup() already acquires the AioContext, so release it
> >>> > before the call.
> >>> > 
> >>> > Signed-off-by: Sergio Lopez <address@hidden>
> >>> > ---
> >>> >  blockdev.c | 6 +-----
> >>> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >>> > 
> >>> > diff --git a/blockdev.c b/blockdev.c
> >>> > index fbef6845c8..3927fdab80 100644
> >>> > --- a/blockdev.c
> >>> > +++ b/blockdev.c
> >>> > @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState 
> >>> > *common, Error **errp)
> >>> >  
> >>> >      aio_context = bdrv_get_aio_context(bs);
> >>> >      aio_context_acquire(aio_context);
> >>> > -
> >>
> >> Are you removing this unrelated empty line intentionally?
> >
> > Yes. In the sense of that whole set of lines being a "open drained
> > section" block.
> >
> >>> >      /* Paired with .clean() */
> >>> >      bdrv_drained_begin(bs);
> >>> 
> >>> Do we need to make this change to blockdev_backup_prepare as well?
> >>
> >> Actually, the whole structure feels a bit wrong. We get the bs here and
> >> take its lock, then release the lock again and forget the reference,
> >> only to do both things again inside do_drive_backup().
> >>
> >> The way snapshots work is that the "normal" snapshot commands are
> >> wrappers that turn it into a single-entry transaction. Then you have
> >> only one code path where you can resolve the ID and take the lock just
> >> once. So maybe backup should work like this, too?
> >
> > I'm neither opposed nor in favor, but I think this is outside the scope
> > of this patch series.
> 
> Kevin, do you think we should attempt to just fix this issue (which
> would make a possible backport easier) or try to move all blockdev
> actions to be transaction-based?

Maybe fix it and then do the cleanup on top, though possibly in the same
series?

Kevin

signature.asc
Description: PGP signature