[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at driv
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare |
Date: |
Thu, 10 Oct 2019 17:02:32 +0200 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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