| Message ID | 20181002230218.13949-6-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dirty-bitmaps: fix QMP command permissions | expand |
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 <jsnow@redhat.com> > --- > 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 <eblake@redhat.com>
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 <jsnow@redhat.com> >> --- >> 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 <eblake@redhat.com> >
03.10.2018 02:02, John Snow wrote: > If the bitmap is frozen, we shouldn't touch it. > > Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > 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; > } > } > @@ -3620,10 +3620,10 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, > error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); > 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; > } > }
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 <jsnow@redhat.com> >> --- >> 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 <eblake@redhat.com> > 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
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; } } @@ -3620,10 +3620,10 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); 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; } }
If the bitmap is frozen, we shouldn't touch it. Signed-off-by: John Snow <jsnow@redhat.com> --- blockdev.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)