| Message ID | 20190913152507.56197-2-slp@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | blockdev: avoid acquiring AioContext lock twice at do_drive_backup() | expand |
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 <slp@redhat.com> > --- > 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); > - > /* Paired with .clean() */ > bdrv_drained_begin(bs); Do we need to make this change to blockdev_backup_prepare as well? > + aio_context_release(aio_context); > > state->bs = bs; > > state->job = do_drive_backup(backup, common->block_job_txn, &local_err); > if (local_err) { > error_propagate(errp, local_err); > - goto out; > } > - > -out: > - aio_context_release(aio_context); > } > > static void drive_backup_commit(BlkActionState *common) >
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 <slp@redhat.com> > > --- > > 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? > > /* 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? Kevin
Kevin Wolf <kwolf@redhat.com> 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 <slp@redhat.com> >> > --- >> > 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. Sergio.
John Snow <jsnow@redhat.com> writes: > 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 <slp@redhat.com> >> --- >> 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); >> - >> /* Paired with .clean() */ >> bdrv_drained_begin(bs); > > Do we need to make this change to blockdev_backup_prepare as well? Yes, we do. Thanks. >> + aio_context_release(aio_context); >> >> state->bs = bs; >> >> state->job = do_drive_backup(backup, common->block_job_txn, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> - goto out; >> } >> - >> -out: >> - aio_context_release(aio_context); >> } >> >> static void drive_backup_commit(BlkActionState *common) >>
Sergio Lopez <slp@redhat.com> writes: > Kevin Wolf <kwolf@redhat.com> 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 <slp@redhat.com> >>> > --- >>> > 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? Cheers, Sergio.
Am 03.10.2019 um 11:33 hat Sergio Lopez geschrieben: > > Sergio Lopez <slp@redhat.com> writes: > > > Kevin Wolf <kwolf@redhat.com> 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 <slp@redhat.com> > >>> > --- > >>> > 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
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); - /* Paired with .clean() */ bdrv_drained_begin(bs); + aio_context_release(aio_context); state->bs = bs; state->job = do_drive_backup(backup, common->block_job_txn, &local_err); if (local_err) { error_propagate(errp, local_err); - goto out; } - -out: - aio_context_release(aio_context); } static void drive_backup_commit(BlkActionState *common)
do_drive_backup() already acquires the AioContext, so release it before the call. Signed-off-by: Sergio Lopez <slp@redhat.com> --- blockdev.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)