| Message ID | 20200914191009.644842-1-eblake@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] qemu-img: Support bitmap --merge into backing image | expand |
On 14.09.20 21:10, Eric Blake wrote: > If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a > bitmap from top into base, qemu-img was failing with: > > qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to get shared "write" lock > Is another process using the image [base.qcow2]? > > The easiest fix is to not open the entire backing chain of either > image (source or destination); after all, the point of 'qemu-img > bitmap' is solely to manipulate bitmaps directly within a single qcow2 > image, and this is made more precise if we don't pay attention to > other images in the chain that may happen to have a bitmap by the same > name. > > However, note that during normal usage, it is a feature that qemu will > allow a bitmap from a backing image to be exposed by an overlay BDS; > doing so makes it easier to perform incremental backup, where we have: > > Base <- Active <- temporrary *temporary (Also it’s a bit strange that “Base” and “Active” are capitalized, but “temporary” isn’t) > \--block job ->/ > > with temporary being fed by a block-copy 'sync' job; when exposing s/block-copy 'sync'/backup 'sync=none'/? > temporary over NBD, referring to a bitmap that lives only in Active is > less effort than having to copy a bitmap into temporary [1]. So the > testsuite additions in this patch check both where bitmaps get > allocated (the qemu-img info output), and, when NOT using 'qemu-img > bitmap', that bitmaps are indeed visible through a backing chain. Well. It is useful over NBD but I would doubt that it isn’t useful in general. For example, the QMP commands that refer to bitmaps always do so through a node-name + bitmap-name combination, and they require that the given bitmap is exactly on the given node. So I think this is a very much a case-by-case question. (And in practice, NBD seems to be the outlier, not qemu-img bitmap.) > [1] Full disclosure: prior to the recent commit 374eedd1c4 and > friends, we were NOT able to see bitmaps through filters, which meant > that we actually did not have nice clean semantics for uniformly being > able to pick up bitmaps from anywhere in the backing chain (seen as a > change in behavior between qemu 4.1 and 4.2 at commit 00e30f05de, when > block-copy swapped from a one-off to a filter). Which means libvirt > was already coded to copy bitmaps around for the sake of older qemu, > even though modern qemu no longer needs it. Oh well. > > Fixes: http://bugzilla.redhat.com/1877209 > Reported-by: Eyal Shenitzky <eshenitz@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > In v2: > - also use BDRV_O_NO_BACKING on source [Max] > - improved commit message [Max] > > qemu-img.c | 11 ++++++-- > tests/qemu-iotests/291 | 12 ++++++++ > tests/qemu-iotests/291.out | 56 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 76 insertions(+), 3 deletions(-) The code looks good to me, but I wonder whether in the commit message it should be noted that we don’t want to let bitmaps from deeper nodes shine through by default everywhere, but just in specific cases where that’s useful (i.e. only NBD so far AFAIA). Max
On 9/15/20 3:57 AM, Max Reitz wrote: > On 14.09.20 21:10, Eric Blake wrote: >> If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a >> bitmap from top into base, qemu-img was failing with: >> >> qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to get shared "write" lock >> Is another process using the image [base.qcow2]? >> >> The easiest fix is to not open the entire backing chain of either >> image (source or destination); after all, the point of 'qemu-img >> bitmap' is solely to manipulate bitmaps directly within a single qcow2 >> image, and this is made more precise if we don't pay attention to >> other images in the chain that may happen to have a bitmap by the same >> name. >> >> However, note that during normal usage, it is a feature that qemu will >> allow a bitmap from a backing image to be exposed by an overlay BDS; >> doing so makes it easier to perform incremental backup, where we have: >> >> Base <- Active <- temporrary > > *temporary > > (Also it’s a bit strange that “Base” and “Active” are capitalized, but > “temporary” isn’t) > >> \--block job ->/ >> >> with temporary being fed by a block-copy 'sync' job; when exposing > > s/block-copy 'sync'/backup 'sync=none'/? Will fix both. > >> temporary over NBD, referring to a bitmap that lives only in Active is >> less effort than having to copy a bitmap into temporary [1]. So the >> testsuite additions in this patch check both where bitmaps get >> allocated (the qemu-img info output), and, when NOT using 'qemu-img >> bitmap', that bitmaps are indeed visible through a backing chain. > > Well. It is useful over NBD but I would doubt that it isn’t useful in > general. For example, the QMP commands that refer to bitmaps always do > so through a node-name + bitmap-name combination, and they require that > the given bitmap is exactly on the given node. > > So I think this is a very much a case-by-case question. (And in > practice, NBD seems to be the outlier, not qemu-img bitmap.) > I'm happy to reword slightly to give that caveat. >> [1] Full disclosure: prior to the recent commit 374eedd1c4 and >> friends, we were NOT able to see bitmaps through filters, which meant >> that we actually did not have nice clean semantics for uniformly being >> able to pick up bitmaps from anywhere in the backing chain (seen as a >> change in behavior between qemu 4.1 and 4.2 at commit 00e30f05de, when >> block-copy swapped from a one-off to a filter). Which means libvirt >> was already coded to copy bitmaps around for the sake of older qemu, >> even though modern qemu no longer needs it. Oh well. >> >> Fixes: http://bugzilla.redhat.com/1877209 >> Reported-by: Eyal Shenitzky <eshenitz@redhat.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> >> In v2: >> - also use BDRV_O_NO_BACKING on source [Max] >> - improved commit message [Max] >> >> qemu-img.c | 11 ++++++-- >> tests/qemu-iotests/291 | 12 ++++++++ >> tests/qemu-iotests/291.out | 56 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 76 insertions(+), 3 deletions(-) > > The code looks good to me, but I wonder whether in the commit message it > should be noted that we don’t want to let bitmaps from deeper nodes > shine through by default everywhere, but just in specific cases where > that’s useful (i.e. only NBD so far AFAIA). So is this a Reviewed-by? I'm happy to queue it through my bitmaps tree, if so. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
14.09.2020 22:10, Eric Blake wrote: > If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a > bitmap from top into base, qemu-img was failing with: > > qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to get shared "write" lock > Is another process using the image [base.qcow2]? > > The easiest fix is to not open the entire backing chain of either > image (source or destination); after all, the point of 'qemu-img > bitmap' is solely to manipulate bitmaps directly within a single qcow2 > image, and this is made more precise if we don't pay attention to > other images in the chain that may happen to have a bitmap by the same > name. > > However, note that during normal usage, it is a feature that qemu will > allow a bitmap from a backing image to be exposed by an overlay BDS; > doing so makes it easier to perform incremental backup, where we have: > > Base <- Active <- temporrary > \--block job ->/ > > with temporary being fed by a block-copy 'sync' job; when exposing > temporary over NBD, referring to a bitmap that lives only in Active is > less effort than having to copy a bitmap into temporary [1]. So the > testsuite additions in this patch check both where bitmaps get > allocated (the qemu-img info output), and, when NOT using 'qemu-img > bitmap', that bitmaps are indeed visible through a backing chain. > > [1] Full disclosure: prior to the recent commit 374eedd1c4 and > friends, we were NOT able to see bitmaps through filters, which meant > that we actually did not have nice clean semantics for uniformly being > able to pick up bitmaps from anywhere in the backing chain (seen as a > change in behavior between qemu 4.1 and 4.2 at commit 00e30f05de, when > block-copy swapped from a one-off to a filter). Which means libvirt > was already coded to copy bitmaps around for the sake of older qemu, > even though modern qemu no longer needs it. Oh well. > > Fixes: http://bugzilla.redhat.com/1877209 > Reported-by: Eyal Shenitzky <eshenitz@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- Honestly I don't want to bother with carefully checking new test output, at least I see that it doesn't produce errors:) Code change seems obvious. Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir
On 15.09.20 15:31, Eric Blake wrote: > On 9/15/20 3:57 AM, Max Reitz wrote: >> On 14.09.20 21:10, Eric Blake wrote: >>> If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a >>> bitmap from top into base, qemu-img was failing with: >>> >>> qemu-img: Could not open 'top.qcow2': Could not open backing file: >>> Failed to get shared "write" lock >>> Is another process using the image [base.qcow2]? >>> >>> The easiest fix is to not open the entire backing chain of either >>> image (source or destination); after all, the point of 'qemu-img >>> bitmap' is solely to manipulate bitmaps directly within a single qcow2 >>> image, and this is made more precise if we don't pay attention to >>> other images in the chain that may happen to have a bitmap by the same >>> name. >>> >>> However, note that during normal usage, it is a feature that qemu will >>> allow a bitmap from a backing image to be exposed by an overlay BDS; >>> doing so makes it easier to perform incremental backup, where we have: >>> >>> Base <- Active <- temporrary >> >> *temporary >> >> (Also it’s a bit strange that “Base” and “Active” are capitalized, but >> “temporary” isn’t) >> >>> \--block job ->/ >>> >>> with temporary being fed by a block-copy 'sync' job; when exposing >> >> s/block-copy 'sync'/backup 'sync=none'/? > > Will fix both. > >> >>> temporary over NBD, referring to a bitmap that lives only in Active is >>> less effort than having to copy a bitmap into temporary [1]. So the >>> testsuite additions in this patch check both where bitmaps get >>> allocated (the qemu-img info output), and, when NOT using 'qemu-img >>> bitmap', that bitmaps are indeed visible through a backing chain. >> >> Well. It is useful over NBD but I would doubt that it isn’t useful in >> general. For example, the QMP commands that refer to bitmaps always do >> so through a node-name + bitmap-name combination, and they require that >> the given bitmap is exactly on the given node. >> >> So I think this is a very much a case-by-case question. (And in >> practice, NBD seems to be the outlier, not qemu-img bitmap.) >> > > I'm happy to reword slightly to give that caveat. > >>> [1] Full disclosure: prior to the recent commit 374eedd1c4 and >>> friends, we were NOT able to see bitmaps through filters, which meant >>> that we actually did not have nice clean semantics for uniformly being >>> able to pick up bitmaps from anywhere in the backing chain (seen as a >>> change in behavior between qemu 4.1 and 4.2 at commit 00e30f05de, when >>> block-copy swapped from a one-off to a filter). Which means libvirt >>> was already coded to copy bitmaps around for the sake of older qemu, >>> even though modern qemu no longer needs it. Oh well. >>> >>> Fixes: http://bugzilla.redhat.com/1877209 >>> Reported-by: Eyal Shenitzky <eshenitz@redhat.com> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> --- >>> >>> In v2: >>> - also use BDRV_O_NO_BACKING on source [Max] >>> - improved commit message [Max] >>> >>> qemu-img.c | 11 ++++++-- >>> tests/qemu-iotests/291 | 12 ++++++++ >>> tests/qemu-iotests/291.out | 56 ++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 76 insertions(+), 3 deletions(-) >> >> The code looks good to me, but I wonder whether in the commit message it >> should be noted that we don’t want to let bitmaps from deeper nodes >> shine through by default everywhere, but just in specific cases where >> that’s useful (i.e. only NBD so far AFAIA). > > So is this a Reviewed-by? I'm happy to queue it through my bitmaps > tree, if so. It wasn’t meant as an R-b, because my R-b depends on how the commit message addresses the question of when exactly bitmaps from the backing chain should be visible on the top image. Whether qemu-img bitmap is an exception, or whether this is really a case-by-case question. (I wanted to know whether you agree on including it. Normally, I’m happy to give an R-b on the basis of “with that done”, but in this case I wasn’t entirely sure whether my request was reasonable, but I also felt that in case it was, it wasn’t optional, given that you do have an entire paragraph in the commit message dedicated to why the backing image’s bitmap is visible on an image exported over NBD.) I have to say I would like to see how you do phrase it in the end, but given that you do agree on including it, I can give a Reviewed-by: Max Reitz <mreitz@redhat.com> Now.
On 9/17/20 5:19 AM, Max Reitz wrote: >>>> temporary over NBD, referring to a bitmap that lives only in Active is >>>> less effort than having to copy a bitmap into temporary [1]. So the >>>> testsuite additions in this patch check both where bitmaps get >>>> allocated (the qemu-img info output), and, when NOT using 'qemu-img >>>> bitmap', that bitmaps are indeed visible through a backing chain. >>> >>> Well. It is useful over NBD but I would doubt that it isn’t useful in >>> general. For example, the QMP commands that refer to bitmaps always do >>> so through a node-name + bitmap-name combination, and they require that >>> the given bitmap is exactly on the given node. >>> >>> So I think this is a very much a case-by-case question. (And in >>> practice, NBD seems to be the outlier, not qemu-img bitmap.) >>> >> >> I'm happy to reword slightly to give that caveat. >> >>> The code looks good to me, but I wonder whether in the commit message it >>> should be noted that we don’t want to let bitmaps from deeper nodes >>> shine through by default everywhere, but just in specific cases where >>> that’s useful (i.e. only NBD so far AFAIA). >> >> So is this a Reviewed-by? I'm happy to queue it through my bitmaps >> tree, if so. > > It wasn’t meant as an R-b, because my R-b depends on how the commit > message addresses the question of when exactly bitmaps from the backing > chain should be visible on the top image. Whether qemu-img bitmap is an > exception, or whether this is really a case-by-case question. > > (I wanted to know whether you agree on including it. Normally, I’m > happy to give an R-b on the basis of “with that done”, but in this case > I wasn’t entirely sure whether my request was reasonable, but I also > felt that in case it was, it wasn’t optional, given that you do have an > entire paragraph in the commit message dedicated to why the backing > image’s bitmap is visible on an image exported over NBD.) Here's my rewording: However, note that on a case-by-case analysis, there _are_ times where we treat it as a feature that we can access a bitmap from a backing layer in association with an overlay BDS. A demonstration of this is using NBD to expose both an overlay BDS (for constant contents) and a bitmap (for learning which blocks are interesting) during an incremental backup: Base <- Active <- Temporary \--block job ->/ where Temporary is being fed by a backup 'sync=none' job. When exposing Temporary over NBD, referring to a bitmap that lives only in Active is less effort than having to copy a bitmap into Temporary [1]. So the testsuite additions in this patch check both where bitmaps get allocated (the qemu-img info output), and that qemu-nbd is indeed able to access a bitmap inherited from the backing chain since it is a different use case than 'qemu-img bitmap'. > > I have to say I would like to see how you do phrase it in the end, but > given that you do agree on including it, I can give a > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > Now. Okay, I think I've met your request, so I'll go ahead and send the pull request today. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
diff --git a/qemu-img.c b/qemu-img.c index d0b1c97562e8..48d35a2f23a1 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -4757,14 +4757,19 @@ static int img_bitmap(int argc, char **argv) filename = argv[optind]; bitmap = argv[optind + 1]; - blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false, - false); + /* + * No need to open backing chains; we will be manipulating bitmaps + * directly in this image without reference to image contents. + */ + blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR | BDRV_O_NO_BACKING, + false, false, false); if (!blk) { goto out; } bs = blk_bs(blk); if (src_filename) { - src = img_open(false, src_filename, src_fmt, 0, false, false, false); + src = img_open(false, src_filename, src_fmt, BDRV_O_NO_BACKING, + false, false, false); if (!src) { goto out; } diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291 index 1e0bb76959bb..4f837b205655 100755 --- a/tests/qemu-iotests/291 +++ b/tests/qemu-iotests/291 @@ -91,6 +91,15 @@ $QEMU_IMG bitmap --remove --image-opts \ driver=$IMGFMT,file.driver=file,file.filename="$TEST_IMG" tmp _img_info --format-specific +echo +echo "=== Merge from top layer into backing image ===" +echo + +$QEMU_IMG rebase -u -F qcow2 -b "$TEST_IMG.base" "$TEST_IMG" +$QEMU_IMG bitmap --add --merge b2 -b "$TEST_IMG" -F $IMGFMT \ + -f $IMGFMT "$TEST_IMG.base" b3 +_img_info --format-specific --backing-chain + echo echo "=== Check bitmap contents ===" echo @@ -107,6 +116,9 @@ $QEMU_IMG map --output=json --image-opts \ nbd_server_start_unix_socket -r -f qcow2 -B b2 "$TEST_IMG" $QEMU_IMG map --output=json --image-opts \ "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map +nbd_server_start_unix_socket -r -f qcow2 -B b3 "$TEST_IMG" +$QEMU_IMG map --output=json --image-opts \ + "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b3" | _filter_qemu_img_map # success, all done echo '*** done' diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out index ee89a728855f..3990f7aacc7b 100644 --- a/tests/qemu-iotests/291.out +++ b/tests/qemu-iotests/291.out @@ -68,6 +68,59 @@ Format specific information: corrupt: false extended l2: false +=== Merge from top layer into backing image === + +image: TEST_DIR/t.IMGFMT +file format: IMGFMT +virtual size: 10 MiB (10485760 bytes) +cluster_size: 65536 +backing file: TEST_DIR/t.IMGFMT.base +backing file format: IMGFMT +Format specific information: + compat: 1.1 + compression type: zlib + lazy refcounts: false + bitmaps: + [0]: + flags: + name: b1 + granularity: 524288 + [1]: + flags: + [0]: auto + name: b2 + granularity: 65536 + [2]: + flags: + name: b0 + granularity: 65536 + refcount bits: 16 + corrupt: false + extended l2: false + +image: TEST_DIR/t.IMGFMT.base +file format: IMGFMT +virtual size: 10 MiB (10485760 bytes) +cluster_size: 65536 +Format specific information: + compat: 1.1 + compression type: zlib + lazy refcounts: false + bitmaps: + [0]: + flags: + [0]: auto + name: b0 + granularity: 65536 + [1]: + flags: + [0]: auto + name: b3 + granularity: 65536 + refcount bits: 16 + corrupt: false + extended l2: false + === Check bitmap contents === [{ "start": 0, "length": 3145728, "depth": 0, "zero": false, "data": true, "offset": OFFSET}, @@ -79,4 +132,7 @@ Format specific information: [{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": OFFSET}, { "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": false}, { "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data": true, "offset": OFFSET}] +[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": OFFSET}, +{ "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": false}, +{ "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data": true, "offset": OFFSET}] *** done
If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a bitmap from top into base, qemu-img was failing with: qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to get shared "write" lock Is another process using the image [base.qcow2]? The easiest fix is to not open the entire backing chain of either image (source or destination); after all, the point of 'qemu-img bitmap' is solely to manipulate bitmaps directly within a single qcow2 image, and this is made more precise if we don't pay attention to other images in the chain that may happen to have a bitmap by the same name. However, note that during normal usage, it is a feature that qemu will allow a bitmap from a backing image to be exposed by an overlay BDS; doing so makes it easier to perform incremental backup, where we have: Base <- Active <- temporrary \--block job ->/ with temporary being fed by a block-copy 'sync' job; when exposing temporary over NBD, referring to a bitmap that lives only in Active is less effort than having to copy a bitmap into temporary [1]. So the testsuite additions in this patch check both where bitmaps get allocated (the qemu-img info output), and, when NOT using 'qemu-img bitmap', that bitmaps are indeed visible through a backing chain. [1] Full disclosure: prior to the recent commit 374eedd1c4 and friends, we were NOT able to see bitmaps through filters, which meant that we actually did not have nice clean semantics for uniformly being able to pick up bitmaps from anywhere in the backing chain (seen as a change in behavior between qemu 4.1 and 4.2 at commit 00e30f05de, when block-copy swapped from a one-off to a filter). Which means libvirt was already coded to copy bitmaps around for the sake of older qemu, even though modern qemu no longer needs it. Oh well. Fixes: http://bugzilla.redhat.com/1877209 Reported-by: Eyal Shenitzky <eshenitz@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- In v2: - also use BDRV_O_NO_BACKING on source [Max] - improved commit message [Max] qemu-img.c | 11 ++++++-- tests/qemu-iotests/291 | 12 ++++++++ tests/qemu-iotests/291.out | 56 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 3 deletions(-)