Re: [Qemu-block] [PATCH v4 5/6] block/backup: prohibit backup from using in use bitmaps

[John Snow]

On 10/03/2018 08:28 AM, Eric Blake wrote:
> On 10/2/18 6:02 PM, John Snow wrote:
>> If the bitmap is frozen, we shouldn't touch it.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>   blockdev.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index d0febfca79..098d4c337f 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3512,10 +3512,10 @@ static BlockJob *do_drive_backup(DriveBackup
>> *backup, JobTxn *txn,
>>               bdrv_unref(target_bs);
>>               goto out;
>>           }
>> -        if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
>> +        if (bdrv_dirty_bitmap_user_locked(bmap)) {
>>               error_setg(errp,
>> -                       "Bitmap '%s' is currently locked and cannot be
>> used for "
>> -                       "backup", backup->bitmap);
>> +                       "Bitmap '%s' is currently in use by another
>> operation"
>> +                       " and cannot be used for backup",
>> backup->bitmap);
>>               goto out;
> 
> Is this right?  Why can we not have two parallel backups utilizing the
> same unchanging locked bitmap as its source?  Of course, to do that,
> we'd need the condition of being locked to be a ref-counter (increment
> for each backup that reuses the bitmap, decrement when the backup
> finishes, and it is unlocked when the counter is 0) rather than a bool.
> So, without that larger refactoring, this is a conservative approach
> that is a little too strict, but allows for a simpler implementation.
> And the user can always work around the limitation by cloning the locked
> bitmap into another temporary bitmap, and starting the second parallel
> backup with the second backup instead of the original.
> 
> Weak Reviewed-by: Eric Blake <address@hidden>
> 

Vladimir gave a good recounting of the reasons. My principal
justification here is that:

- FROZEN implies that the bitmap has been split; which means there is a
pending operating to re-suture them into one bitmap which may occur at
an indeterminate time in the future that we cannot account for in the
following job code, and

- QMP_LOCKED only implies that the bitmap is in use by, say, the NBD
fleecing operation with no further pending actions.

Here, in do_drive_backup, we check only that we are not qmp_locked, but
I argue we ought to check against frozen as well. It's likely to fail
because the BDS is already in use by another job, but this check is
strictly more correct.

In the opposite case, we don't want to split a bitmap that is being used
by someone else -- we're about to fork this bitmap (which means that the
bitmap referenced by this named handle will be CLEARED) which can alter
what the NBD process is doing, which is also bad.

For now, this is correct.

--js