| Message ID | 20180919195847.7816-1-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dirty-bitmaps: allow merging to disabled bitmaps | expand |
On 9/19/18 2:58 PM, John Snow wrote: > We wish to prohibit merging to read-only bitmaps and frozen bitmaps, > but "disabled" bitmaps only preclude their recording of live, new > information. It does not prohibit them from manual writes at the behest > of the user, as is the case for merge operations. In particular, if I have a linear sequence of bitmaps, bitmap1 (disabled) tracking sectors touched during times T1-T2 bitmap2 (disabled) tracking sectors touched during T2-T3 bitmap3 (enabled) tracking sectors touched during T3-present and later decide that I no longer care about time T2, but may still want to create a differential backup against time T1, then the logical action is to merge bitmap1 and bitmap2 into a single bitmap tracking T1-T3. Whether I keep the name bitmap1 (b1 as dst, b2 as src, delete b2 on completion), bitmap2 (b1 as src, b2 as dst, delete b1 on completion) or pick yet another name (create new disabled map b12, merge b1 into b12, merge b2 into b12, delete b1 and b2) is less important - the point remains that I am trying to merge into a disabled bitmap. If a bitmap has to be enabled to perform the merge, then any guest I/O during the window in time where it is temporarily enabled will perhaps spuriously set bits corresponding to sectors not actually touched during T1-T3. Perhaps it can be worked around with a transaction that does bitmap-enable, bitmap-merge, bitmap-disable - but that seems like a lot of overhead (and does it actually prevent guest I/O from happening during the transaction?), compared to just allowing the merge. > > Reported-by: Eric Blake <eblake@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/dirty-bitmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index c9b8a6fd52..fa7e75e0af 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -798,7 +798,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, > > qemu_mutex_lock(dest->mutex); > > - assert(bdrv_dirty_bitmap_enabled(dest)); > + assert(!bdrv_dirty_bitmap_frozen(dest)); > assert(!bdrv_dirty_bitmap_readonly(dest)); At any rate, the fact that the deleted assertion was triggerable by QMP actions is a good reason to change it. Here's how I triggered it, if you want to beef up the commit message: $ qemu-img create -f qcow2 file 64k $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio {'execute':'qmp_capabilities'} {'execute':'blockdev-add', 'arguments':{'driver':'qcow2', 'node-name':'tmp', 'file':{'driver':'file', 'filename':'file'}}} {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp', 'name':'b0'}} {'execute':'transaction', 'arguments':{'actions':[ {'type':'x-block-dirty-bitmap-disable', 'data':{'node':'tmp', 'name':'b0'}}, {'type':'block-dirty-bitmap-add', 'data':{'node':'tmp', 'name':'b1'}}]}} {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp', 'name':'tmp'}} {'execute':'x-block-dirty-bitmap-disable', 'arguments':{'node':'tmp', 'name':'tmp'}} {'execute':'x-block-dirty-bitmap-merge', 'arguments':{'node':'tmp', 'src_name':'b0', 'dst_name':'tmp'}} qemu-system-x86_64: block/dirty-bitmap.c:801: bdrv_merge_dirty_bitmap: Assertion `bdrv_dirty_bitmap_enabled(dest)' failed. However, are we sure that the remaining assertions are properly flagged by callers, and that we aren't leaving any other lingering QMP crashers? Let's see if I can modify my example $ qemu-img create -f qcow2 -b file -F qcow2 file2 $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio {'execute':'qmp_capabilities'} {'execute':'blockdev-add', 'arguments':{'driver':'qcow2', 'node-name':'tmp', 'file':{'driver':'file', 'filename':'file'}}} {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp', 'name':'b0'}} {'execute':'nbd-server-start', 'arguments':{'addr':{'type':'inet', 'data':{'host':'localhost', 'port':'10809'}}}} {'execute':'blockdev-add', 'arguments':{'driver':'qcow2', 'node-name':'fleece', 'file':{'driver':'file', 'filename':'file2'}, 'backing':'tmp'}} {'execute':'transaction', 'arguments':{'actions':[ {'type':'blockdev-backup', 'data':{'job-id':'backup', 'device':'tmp', 'target':'fleece', 'sync':'none'}}, {'type':'block-dirty-bitmap-add', 'data':{'node':'tmp', 'name':'b1'}}, {'type':'x-block-dirty-bitmap-disable', 'data':{'node':'tmp', 'name':'b0'}} ]}} {'execute':'nbd-server-add', 'arguments':{'device':'fleece'}} {'execute':'x-nbd-server-add-bitmap', 'arguments':{'name':'fleece', 'bitmap':'b0'}} {'execute':'x-block-dirty-bitmap-merge', 'arguments':{'node':'tmp', 'src_name':'b1', 'dst_name':'b0'}} Prior to your patch, that asserts; but after your patch, it silently succeeds. Shouldn't a bitmap be marked frozen while it is advertised over an NBD export? We already require such a bitmap to be disabled before you can attach it, and you REALLY don't want a read-only export to be seeing changes to block status information due to merges. And then we need to make sure that we give a proper error message rather than an assertion when using QMP to attempt to merge into a frozen bitmap. But that's probably an independent patch, so: Reviewed-by: Eric Blake <eblake@redhat.com>
On 9/19/18 2:58 PM, John Snow wrote: > We wish to prohibit merging to read-only bitmaps and frozen bitmaps, > but "disabled" bitmaps only preclude their recording of live, new > information. It does not prohibit them from manual writes at the behest > of the user, as is the case for merge operations. > > Reported-by: Eric Blake <eblake@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/dirty-bitmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) I can add this to my NBD queue, if desired. > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index c9b8a6fd52..fa7e75e0af 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -798,7 +798,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, > > qemu_mutex_lock(dest->mutex); > > - assert(bdrv_dirty_bitmap_enabled(dest)); > + assert(!bdrv_dirty_bitmap_frozen(dest)); > assert(!bdrv_dirty_bitmap_readonly(dest)); > > if (!hbitmap_merge(dest->bitmap, src->bitmap)) { >
On 09/19/2018 05:08 PM, Eric Blake wrote: > On 9/19/18 2:58 PM, John Snow wrote: >> We wish to prohibit merging to read-only bitmaps and frozen bitmaps, >> but "disabled" bitmaps only preclude their recording of live, new >> information. It does not prohibit them from manual writes at the behest >> of the user, as is the case for merge operations. >> >> Reported-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/dirty-bitmap.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > I can add this to my NBD queue, if desired. > If that makes life easier for you, then please be my guest -- otherwise there's no conflict in taking it through the bitmaps tree I've got. Just let me know either way.
On 09/19/2018 04:58 PM, Eric Blake wrote: > On 9/19/18 2:58 PM, John Snow wrote: >> We wish to prohibit merging to read-only bitmaps and frozen bitmaps, >> but "disabled" bitmaps only preclude their recording of live, new >> information. It does not prohibit them from manual writes at the behest >> of the user, as is the case for merge operations. > > In particular, if I have a linear sequence of bitmaps, > bitmap1 (disabled) tracking sectors touched during times T1-T2 > bitmap2 (disabled) tracking sectors touched during T2-T3 > bitmap3 (enabled) tracking sectors touched during T3-present > > and later decide that I no longer care about time T2, but may still want > to create a differential backup against time T1, then the logical action > is to merge bitmap1 and bitmap2 into a single bitmap tracking T1-T3. > Whether I keep the name bitmap1 (b1 as dst, b2 as src, delete b2 on > completion), bitmap2 (b1 as src, b2 as dst, delete b1 on completion) > or pick yet another name (create new disabled map b12, merge b1 into > b12, merge b2 into b12, delete b1 and b2) is less important - the point > remains that I am trying to merge into a disabled bitmap. If a bitmap > has to be enabled to perform the merge, then any guest I/O during the > window in time where it is temporarily enabled will perhaps spuriously > set bits corresponding to sectors not actually touched during T1-T3. > > Perhaps it can be worked around with a transaction that does > bitmap-enable, bitmap-merge, bitmap-disable - but that seems like a lot > of overhead (and does it actually prevent guest I/O from happening > during the transaction?), compared to just allowing the merge. > >> >> Reported-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/dirty-bitmap.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index c9b8a6fd52..fa7e75e0af 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -798,7 +798,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap >> *dest, const BdrvDirtyBitmap *src, >> qemu_mutex_lock(dest->mutex); >> - assert(bdrv_dirty_bitmap_enabled(dest)); >> + assert(!bdrv_dirty_bitmap_frozen(dest)); >> assert(!bdrv_dirty_bitmap_readonly(dest)); > > At any rate, the fact that the deleted assertion was triggerable by QMP > actions is a good reason to change it. Here's how I triggered it, if > you want to beef up the commit message: > If you stage it, please feel free to amend it to add relevant details. I was relying maybe too heavily on inference. > $ qemu-img create -f qcow2 file 64k > $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio > {'execute':'qmp_capabilities'} > {'execute':'blockdev-add', 'arguments':{'driver':'qcow2', > 'node-name':'tmp', 'file':{'driver':'file', 'filename':'file'}}} > {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp', > 'name':'b0'}} > {'execute':'transaction', 'arguments':{'actions':[ > {'type':'x-block-dirty-bitmap-disable', 'data':{'node':'tmp', > 'name':'b0'}}, > {'type':'block-dirty-bitmap-add', 'data':{'node':'tmp', > 'name':'b1'}}]}} > {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp', > 'name':'tmp'}} > {'execute':'x-block-dirty-bitmap-disable', 'arguments':{'node':'tmp', > 'name':'tmp'}} > {'execute':'x-block-dirty-bitmap-merge', 'arguments':{'node':'tmp', > 'src_name':'b0', 'dst_name':'tmp'}} > qemu-system-x86_64: block/dirty-bitmap.c:801: bdrv_merge_dirty_bitmap: > Assertion `bdrv_dirty_bitmap_enabled(dest)' failed. > > However, are we sure that the remaining assertions are properly flagged > by callers, and that we aren't leaving any other lingering QMP crashers? There's only one caller of merge, we should be OK. (Am I misunderstanding your question?) > Let's see if I can modify my example > > $ qemu-img create -f qcow2 -b file -F qcow2 file2 > $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio > {'execute':'qmp_capabilities'} > {'execute':'blockdev-add', 'arguments':{'driver':'qcow2', > 'node-name':'tmp', 'file':{'driver':'file', 'filename':'file'}}} > {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp', > 'name':'b0'}} > {'execute':'nbd-server-start', 'arguments':{'addr':{'type':'inet', > 'data':{'host':'localhost', 'port':'10809'}}}} > {'execute':'blockdev-add', 'arguments':{'driver':'qcow2', > 'node-name':'fleece', 'file':{'driver':'file', 'filename':'file2'}, > 'backing':'tmp'}} > {'execute':'transaction', 'arguments':{'actions':[ > {'type':'blockdev-backup', 'data':{'job-id':'backup', 'device':'tmp', > 'target':'fleece', 'sync':'none'}}, > {'type':'block-dirty-bitmap-add', 'data':{'node':'tmp', 'name':'b1'}}, > {'type':'x-block-dirty-bitmap-disable', 'data':{'node':'tmp', > 'name':'b0'}} > ]}} > {'execute':'nbd-server-add', 'arguments':{'device':'fleece'}} > {'execute':'x-nbd-server-add-bitmap', 'arguments':{'name':'fleece', > 'bitmap':'b0'}} > {'execute':'x-block-dirty-bitmap-merge', 'arguments':{'node':'tmp', > 'src_name':'b1', 'dst_name':'b0'}} > > Prior to your patch, that asserts; but after your patch, it silently > succeeds. Shouldn't a bitmap be marked frozen while it is advertised > over an NBD export? We already require such a bitmap to be disabled > before you can attach it, and you REALLY don't want a read-only export > to be seeing changes to block status information due to merges. And then > we need to make sure that we give a proper error message rather than an > assertion when using QMP to attempt to merge into a frozen bitmap. But > that's probably an independent patch, so: > > Reviewed-by: Eric Blake <eblake@redhat.com> > So, it's not other callers we're worried about but new contexts that were previously forbidden. "Freezing" a bitmap has traditionally had somewhat of a specific connotation in the code, which is that: 1. Users are prohibited from interacting with it at all through QMP, generally. There might be exceptions, for things like query. 2. That bitmap is invisibly split into two bitmaps for the purposes of transactional guarantees -- this means it acts as both an "enabled" bitmap and a "disabled" bitmap simultaneously (it actually has one of each under the hood!) 3. The bitmap is in use by a block job operation. I'm not sure if we need that invisible-split mechanism for NBD pulls (which are generally happening on fleeced nodes?) but it would certainly be good to prohibit users from touching it in nearly any way until the operation ends. .... hmm, in this case it looks as if perhaps that merge ought to respect that "QMP Locked" behavior. We can do a V2 here, I think. --js
On 09/19/2018 04:58 PM, Eric Blake wrote: > On 9/19/18 2:58 PM, John Snow wrote: >> We wish to prohibit merging to read-only bitmaps and frozen bitmaps, >> but "disabled" bitmaps only preclude their recording of live, new >> information. It does not prohibit them from manual writes at the behest >> of the user, as is the case for merge operations. > > In particular, if I have a linear sequence of bitmaps, > bitmap1 (disabled) tracking sectors touched during times T1-T2 > bitmap2 (disabled) tracking sectors touched during T2-T3 > bitmap3 (enabled) tracking sectors touched during T3-present > > and later decide that I no longer care about time T2, but may still want > to create a differential backup against time T1, then the logical action > is to merge bitmap1 and bitmap2 into a single bitmap tracking T1-T3. > Whether I keep the name bitmap1 (b1 as dst, b2 as src, delete b2 on > completion), bitmap2 (b1 as src, b2 as dst, delete b1 on completion) > or pick yet another name (create new disabled map b12, merge b1 into > b12, merge b2 into b12, delete b1 and b2) is less important - the point > remains that I am trying to merge into a disabled bitmap. If a bitmap > has to be enabled to perform the merge, then any guest I/O during the > window in time where it is temporarily enabled will perhaps spuriously > set bits corresponding to sectors not actually touched during T1-T3. > > Perhaps it can be worked around with a transaction that does > bitmap-enable, bitmap-merge, bitmap-disable - but that seems like a lot > of overhead (and does it actually prevent guest I/O from happening > during the transaction?), compared to just allowing the merge. > >> >> Reported-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/dirty-bitmap.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index c9b8a6fd52..fa7e75e0af 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -798,7 +798,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap >> *dest, const BdrvDirtyBitmap *src, >> qemu_mutex_lock(dest->mutex); >> - assert(bdrv_dirty_bitmap_enabled(dest)); >> + assert(!bdrv_dirty_bitmap_frozen(dest)); >> assert(!bdrv_dirty_bitmap_readonly(dest)); > > At any rate, the fact that the deleted assertion was triggerable by QMP > actions is a good reason to change it. Here's how I triggered it, if > you want to beef up the commit message: > > $ qemu-img create -f qcow2 file 64k > $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio > {'execute':'qmp_capabilities'} > {'execute':'blockdev-add', 'arguments':{'driver':'qcow2', > 'node-name':'tmp', 'file':{'driver':'file', 'filename':'file'}}} > {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp', > 'name':'b0'}} > {'execute':'transaction', 'arguments':{'actions':[ > {'type':'x-block-dirty-bitmap-disable', 'data':{'node':'tmp', > 'name':'b0'}}, > {'type':'block-dirty-bitmap-add', 'data':{'node':'tmp', > 'name':'b1'}}]}} > {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp', > 'name':'tmp'}} > {'execute':'x-block-dirty-bitmap-disable', 'arguments':{'node':'tmp', > 'name':'tmp'}} > {'execute':'x-block-dirty-bitmap-merge', 'arguments':{'node':'tmp', > 'src_name':'b0', 'dst_name':'tmp'}} > qemu-system-x86_64: block/dirty-bitmap.c:801: bdrv_merge_dirty_bitmap: > Assertion `bdrv_dirty_bitmap_enabled(dest)' failed. > > However, are we sure that the remaining assertions are properly flagged > by callers, and that we aren't leaving any other lingering QMP crashers? > Let's see if I can modify my example > > $ qemu-img create -f qcow2 -b file -F qcow2 file2 > $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio > {'execute':'qmp_capabilities'} > {'execute':'blockdev-add', 'arguments':{'driver':'qcow2', > 'node-name':'tmp', 'file':{'driver':'file', 'filename':'file'}}} > {'execute':'block-dirty-bitmap-add', 'arguments':{'node':'tmp', > 'name':'b0'}} > {'execute':'nbd-server-start', 'arguments':{'addr':{'type':'inet', > 'data':{'host':'localhost', 'port':'10809'}}}} > {'execute':'blockdev-add', 'arguments':{'driver':'qcow2', > 'node-name':'fleece', 'file':{'driver':'file', 'filename':'file2'}, > 'backing':'tmp'}} > {'execute':'transaction', 'arguments':{'actions':[ > {'type':'blockdev-backup', 'data':{'job-id':'backup', 'device':'tmp', > 'target':'fleece', 'sync':'none'}}, > {'type':'block-dirty-bitmap-add', 'data':{'node':'tmp', 'name':'b1'}}, > {'type':'x-block-dirty-bitmap-disable', 'data':{'node':'tmp', > 'name':'b0'}} > ]}} > {'execute':'nbd-server-add', 'arguments':{'device':'fleece'}} > {'execute':'x-nbd-server-add-bitmap', 'arguments':{'name':'fleece', > 'bitmap':'b0'}} > {'execute':'x-block-dirty-bitmap-merge', 'arguments':{'node':'tmp', > 'src_name':'b1', 'dst_name':'b0'}} > > Prior to your patch, that asserts; but after your patch, it silently > succeeds. Shouldn't a bitmap be marked frozen while it is advertised > over an NBD export? We already require such a bitmap to be disabled > before you can attach it, and you REALLY don't want a read-only export > to be seeing changes to block status information due to merges. And then > we need to make sure that we give a proper error message rather than an > assertion when using QMP to attempt to merge into a frozen bitmap. But > that's probably an independent patch, so: > > Reviewed-by: Eric Blake <eblake@redhat.com> > It looks like in addition to this we need to also check the locked property and disallow those, so, NACK. --js
19.09.2018 22:58, John Snow wrote: > We wish to prohibit merging to read-only bitmaps and frozen bitmaps, > but "disabled" bitmaps only preclude their recording of live, new > information. It does not prohibit them from manual writes at the behest > of the user, as is the case for merge operations. > > Reported-by: Eric Blake <eblake@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> No doubts: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/dirty-bitmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index c9b8a6fd52..fa7e75e0af 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -798,7 +798,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, > > qemu_mutex_lock(dest->mutex); > > - assert(bdrv_dirty_bitmap_enabled(dest)); > + assert(!bdrv_dirty_bitmap_frozen(dest)); > assert(!bdrv_dirty_bitmap_readonly(dest)); > > if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
On 9/19/18 4:16 PM, John Snow wrote: > > > On 09/19/2018 05:08 PM, Eric Blake wrote: >> On 9/19/18 2:58 PM, John Snow wrote: >>> We wish to prohibit merging to read-only bitmaps and frozen bitmaps, >>> but "disabled" bitmaps only preclude their recording of live, new >>> information. It does not prohibit them from manual writes at the behest >>> of the user, as is the case for merge operations. >>> >>> Reported-by: Eric Blake <eblake@redhat.com> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> block/dirty-bitmap.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> I can add this to my NBD queue, if desired. >> > > If that makes life easier for you, then please be my guest -- otherwise > there's no conflict in taking it through the bitmaps tree I've got. At this point, I'm not queuing this through NBD, as I think it is better served by just folding this directly into your v3 permissions series (or does that need to go up to v4?) > > Just let me know either way. >
On 09/26/2018 10:25 PM, Eric Blake wrote: > On 9/19/18 4:16 PM, John Snow wrote: >> >> >> On 09/19/2018 05:08 PM, Eric Blake wrote: >>> On 9/19/18 2:58 PM, John Snow wrote: >>>> We wish to prohibit merging to read-only bitmaps and frozen bitmaps, >>>> but "disabled" bitmaps only preclude their recording of live, new >>>> information. It does not prohibit them from manual writes at the behest >>>> of the user, as is the case for merge operations. >>>> >>>> Reported-by: Eric Blake <eblake@redhat.com> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> block/dirty-bitmap.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> I can add this to my NBD queue, if desired. >>> >> >> If that makes life easier for you, then please be my guest -- otherwise >> there's no conflict in taking it through the bitmaps tree I've got. > > At this point, I'm not queuing this through NBD, as I think it is better > served by just folding this directly into your v3 permissions series (or > does that need to go up to v4?) > Ah, NACK. This is superseded by the permissions changeset. --js >> >> Just let me know either way. >> >
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index c9b8a6fd52..fa7e75e0af 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -798,7 +798,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, qemu_mutex_lock(dest->mutex); - assert(bdrv_dirty_bitmap_enabled(dest)); + assert(!bdrv_dirty_bitmap_frozen(dest)); assert(!bdrv_dirty_bitmap_readonly(dest)); if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
We wish to prohibit merging to read-only bitmaps and frozen bitmaps, but "disabled" bitmaps only preclude their recording of live, new information. It does not prohibit them from manual writes at the behest of the user, as is the case for merge operations. Reported-by: Eric Blake <eblake@redhat.com> Signed-off-by: John Snow <jsnow@redhat.com> --- block/dirty-bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)