[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH] blockdev-backup: don't check aio_context too ea

From: John Snow
Subject: Re: [Qemu-block] [PATCH] blockdev-backup: don't check aio_context too early
Date: Tue, 7 May 2019 10:19:04 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1

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 <address@hidden>
>> 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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]