Re: [Qemu-devel] [PATCH] blockdev-backup: don't check aio_context too early

[John Snow <jsnow@redhat.com>]

On 5/7/19 4:50 AM, Kevin Wolf wrote:
> Am 06.05.2019 um 22:33 hat John Snow geschrieben:
>> in blockdev_backup_prepare, we check to make sure that the target is
>> associated with a compatible aio context. However, do_blockdev_backup is
>> called later and has some logic to move the target to a compatible
>> aio_context. The transaction version will fail certain commands
>> needlessly early as a result.
>>
>> Allow blockdev_backup_prepare to simply call do_blockdev_backup, which
>> will ultimately decide if the contexts are compatible or not.
>>
>> Note: the transaction version has always disallowed this operation since
>> its initial commit bd8baecd (2014), whereas the version of
>> qmp_blockdev_backup at the time, from commit c29c1dd312f, tried to
>> enforce the aio_context switch instead. It's not clear, and I can't see
>> from the mailing list archives at the time, why the two functions take a
>> different approach. It wasn't until later in efd7556708b (2016) that the
>> standalone version tried to determine if it could set the context or
>> not.
>>
>> Reported-by: aihua liang <aliang@redhat.com>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1683498
> 
> Signed-off-by is missing, and a testcase, too. :-)
> 

I am apparently exceedingly good at breaking git-publish, sorry about that.

>> diff --git a/blockdev.c b/blockdev.c
>> index 79fbac8450..a81d88980c 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1872,10 +1872,6 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
>>      }
>>  
>>      aio_context = bdrv_get_aio_context(bs);
>> -    if (aio_context != bdrv_get_aio_context(target)) {
>> -        error_setg(errp, "Backup between two IO threads is not implemented");
>> -        return;
>> -    }
>>      aio_context_acquire(aio_context);
>>      state->bs = bs;
> 
> The actual change looks good to me.
> 
> Kevin
> 

As far as the test case goes, I'll see if I can reproduce it without a
real guest. If you test it with no guest and empty drives with the VM
paused, the error never gets thrown. Maybe it's enough to start an empty
guest and do some junk writes to make the bitmap non-empty.

Thanks,

--js