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);
+    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)
-- 
2.21.0

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.

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

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)
>>