| Message ID | 1457970292-12291-3-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/14/2016 09:44 AM, Kevin Wolf wrote: > This patch changes dirty bitmaps from following a BlockBackend in graph > changes to sticking with the node they were created at. For the full > discussion, read the following mailing list thread: > > [Qemu-block] block: Dirty bitmaps and COR in bdrv_move_feature_fields() > https://lists.nongnu.org/archive/html/qemu-block/2016-02/msg00745.html > > In summary, the justification for this change is: [snip nice list] > > At the end of the day it seems to be very clear that it was a mistake to > include dirty bitmaps in bdrv_move_feature_fields(). The functionality > of moving bitmaps and/or attaching them to a BlockBackend instead will > probably be needed, but it should be done with a new explicit QMP > command or option. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 3 --- > 1 file changed, 3 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> [Always interesting when a patch is way shorter than the justification for why we need the patch] > > diff --git a/block.c b/block.c > index a75c4b3..698e2c7 100644 > --- a/block.c > +++ b/block.c > @@ -2284,9 +2284,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, > > /* dev info */ > bs_dest->enable_write_cache = bs_src->enable_write_cache; > - > - /* dirty bitmap */ > - bs_dest->dirty_bitmaps = bs_src->dirty_bitmaps; > } > > static void change_parent_backing_link(BlockDriverState *from, >
diff --git a/block.c b/block.c index a75c4b3..698e2c7 100644 --- a/block.c +++ b/block.c @@ -2284,9 +2284,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, /* dev info */ bs_dest->enable_write_cache = bs_src->enable_write_cache; - - /* dirty bitmap */ - bs_dest->dirty_bitmaps = bs_src->dirty_bitmaps; } static void change_parent_backing_link(BlockDriverState *from,
This patch changes dirty bitmaps from following a BlockBackend in graph changes to sticking with the node they were created at. For the full discussion, read the following mailing list thread: [Qemu-block] block: Dirty bitmaps and COR in bdrv_move_feature_fields() https://lists.nongnu.org/archive/html/qemu-block/2016-02/msg00745.html In summary, the justification for this change is: * When moving the dirty bitmap to the top of the tree was introduced in bdrv_append() in commit a9fc4408, it didn't actually have any effect because there could never be a bitmap in use when bdrv_append() was called (op blockers would prevent this). This is still true today for all internal uses of dirty bitmaps. * Support for user-defined dirty bitmaps was introduced in 2.4, but we discouraged users from using it because we didn't consider it ready yet. Moreover, in 2.5, the bdrv_swap() removal introduced a bug that left dangling pointers if a dirty bitmap was present (the anchors of the dirty bitmap were swapped, but the back link in the first element wasn't updated), so it didn't even work correctly. * block-dirty-bitmap-add takes an arbitrary node name, even if no BlockBackend is attached. This suggests that it is a node level operation and not a BlockBackend one. Consequently, there is no reason for dirty bitmaps to stay with a BlockBackend that was attached to the node they were created for. * It was suggested that block-dirty-bitmap-add could track the node if a node name was specified, and track the BlockBackend if the device name was specified. This would however be inconsistent with other QMP commands. Commands that accept both device and node names currently interpret the device name just as an alias for the current root node of that BlockBackend. * Dirty bitmaps have a name that is only unique amongst the bitmaps in a specific node. Moving bitmaps could lead to name clashes. Automatic renaming would involve too much magic. * Persistent bitmaps are stored in a specific node. Moving them around automatically might be at least surprising, but it would probably also become a real problem because that would have to happen atomically without the management tool knowing of the operation. At the end of the day it seems to be very clear that it was a mistake to include dirty bitmaps in bdrv_move_feature_fields(). The functionality of moving bitmaps and/or attaching them to a BlockBackend instead will probably be needed, but it should be done with a new explicit QMP command or option. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 3 --- 1 file changed, 3 deletions(-)