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

[Vladimir Sementsov-Ogievskiy]

03.10.2018 15:28, 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.

hm, firstly, we can have only one job per bds (only one backup with this 
source), and now we don't search for backup bitmap anywhere else than 
source bds, so bitmap sharing is impossible. Secondly, when we support 
several jobs per bds, or allow using bitmaps of source backing chain 
(for example), I'm afraid that it is still a bad idea to share the 
bitmap, because backup should to abdicate/reclaim the bitmap at its 
finish, which definitely affect the second backup.

Maybe we'll finally come to not doing reclaim/abdicate automatically in 
backup and user will just enable/disable/merge bitmaps by hand as he 
wants. It is near to be possible now, we just need a flag for such 
behavior, as backup now use sync bitmap only on initialization phase, to 
initialize copy_bitmap. So, backup itself don't need disabling or 
locking of the bitmap and freeze/abdicate/reclaim is an additional logic.

>
> Weak Reviewed-by: Eric Blake <address@hidden>
>


-- 
Best regards,
Vladimir