Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
On Tue, Jun 29, 2021 at 5:40 PM Kevin Wolf wrote: > > Am 29.06.2021 um 09:23 hat Vladimir Sementsov-Ogievskiy geschrieben: > > 28.06.2021 20:42, Eric Blake wrote: > > > On Wed, Jun 23, 2021 at 06:04:19PM +0200, Kevin Wolf wrote: > > > > > This is fine, but it means that this flag will present in all ranges, > > > > > instead of only in unallocated ranges (what this patch is doing). > > > > > > > > An argument for always having the flag would be that it's probably > > > > useful for a tool to know whether a given block is actually absent or > > > > whether it's just running an old qemu-img. > > > > > > > > If we didn't care about this, I would still define the actual value, but > > > > also document a default. > > > > > > So to summarize, it looks like my v3 will have the best chance of > > > approval if I go with always outputting the new field (instead of only > > > on one of its two boolean values), and put it at the end of the JSON > > > output. Since the "present" key is always present, it does not need to be at the end. > > > It also looks like we have consensus on spelling the new > > > field "present":true for data found in the backing chain, and > > > "present":false for places where we would defer to another file if a > > > backing file is later added. > > > > > > > I didn't follow the discussion carefully, but that sounds good to me. > > To me, too. > > > What's the decision about patch 1? > > I think we won't need patch 1 (and the potential backwards compatibility > problems it would introduce) when we have this one. Yes, looks good and patch 1 is not needed. Nir
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
Am 29.06.2021 um 09:23 hat Vladimir Sementsov-Ogievskiy geschrieben: > 28.06.2021 20:42, Eric Blake wrote: > > On Wed, Jun 23, 2021 at 06:04:19PM +0200, Kevin Wolf wrote: > > > > This is fine, but it means that this flag will present in all ranges, > > > > instead of only in unallocated ranges (what this patch is doing). > > > > > > An argument for always having the flag would be that it's probably > > > useful for a tool to know whether a given block is actually absent or > > > whether it's just running an old qemu-img. > > > > > > If we didn't care about this, I would still define the actual value, but > > > also document a default. > > > > So to summarize, it looks like my v3 will have the best chance of > > approval if I go with always outputting the new field (instead of only > > on one of its two boolean values), and put it at the end of the JSON > > output. It also looks like we have consensus on spelling the new > > field "present":true for data found in the backing chain, and > > "present":false for places where we would defer to another file if a > > backing file is later added. > > > > I didn't follow the discussion carefully, but that sounds good to me. To me, too. > What's the decision about patch 1? I think we won't need patch 1 (and the potential backwards compatibility problems it would introduce) when we have this one. Kevin
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
28.06.2021 20:42, Eric Blake wrote: On Wed, Jun 23, 2021 at 06:04:19PM +0200, Kevin Wolf wrote: This is fine, but it means that this flag will present in all ranges, instead of only in unallocated ranges (what this patch is doing). An argument for always having the flag would be that it's probably useful for a tool to know whether a given block is actually absent or whether it's just running an old qemu-img. If we didn't care about this, I would still define the actual value, but also document a default. So to summarize, it looks like my v3 will have the best chance of approval if I go with always outputting the new field (instead of only on one of its two boolean values), and put it at the end of the JSON output. It also looks like we have consensus on spelling the new field "present":true for data found in the backing chain, and "present":false for places where we would defer to another file if a backing file is later added. I didn't follow the discussion carefully, but that sounds good to me. What's the decision about patch 1? -- Best regards, Vladimir
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
On Wed, Jun 23, 2021 at 06:04:19PM +0200, Kevin Wolf wrote: > > This is fine, but it means that this flag will present in all ranges, > > instead of only in unallocated ranges (what this patch is doing). > > An argument for always having the flag would be that it's probably > useful for a tool to know whether a given block is actually absent or > whether it's just running an old qemu-img. > > If we didn't care about this, I would still define the actual value, but > also document a default. So to summarize, it looks like my v3 will have the best chance of approval if I go with always outputting the new field (instead of only on one of its two boolean values), and put it at the end of the JSON output. It also looks like we have consensus on spelling the new field "present":true for data found in the backing chain, and "present":false for places where we would defer to another file if a backing file is later added. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
On Wed, Jun 23, 2021 at 7:04 PM Kevin Wolf wrote: > > Am 23.06.2021 um 15:58 hat Nir Soffer geschrieben: > > On Wed, Jun 23, 2021 at 11:58 AM Kevin Wolf wrote: > > > > > > Am 22.06.2021 um 18:56 hat Nir Soffer geschrieben: > > > > On Tue, Jun 22, 2021 at 6:38 PM Kevin Wolf wrote: > > > > > > > > > > Am 11.06.2021 um 21:03 hat Eric Blake geschrieben: > > > > > > To save the user from having to check 'qemu-img info > > > > > > --backing-chain' > > > > > > or other followup command to determine which "depth":n goes beyond > > > > > > the > > > > > > chain, add a boolean field "backing" that is set only for > > > > > > unallocated > > > > > > portions of the disk. > > > > > > > > > > > > Signed-off-by: Eric Blake > > > > > > --- > > > > > > > > > > > > Touches the same iotest output as 1/1. If we decide that switching > > > > > > to > > > > > > "depth":n+1 is too risky, and that the mere addition of > > > > > > "backing":true > > > > > > while keeping "depth":n is good enough, then we'd have just one > > > > > > patch, > > > > > > instead of this double churn. Preferences? > > > > > > > > > > I think the additional flag is better because it's guaranteed to be > > > > > backwards compatible, and because you don't need to know the number of > > > > > layers to infer whether a cluster was allocated in the whole backing > > > > > chain. And by exposing ALLOCATED we definitely give access to the > > > > > whole > > > > > information that exists in QEMU. > > > > > > > > > > However, to continue with the bike shedding: I won't insist on > > > > > "allocated" even if that is what the flag is called internally and > > > > > consistency is usually helpful, but "backing" is misleading, too, > > > > > because intuitively it doesn't cover the top layer or standalone > > > > > images > > > > > without a backing file. How about something like "present"? > > > > > > > > Looks hard to document: > > > > > > > > # @present: if present and false, the range is not allocated within the > > > > # backing chain (since 6.1) > > > > > > I'm not sure why you would document it with a double negative. > > > > > > > And is not consistent with "offset". It would work better as: > > > > > > > > # @present: if present, the range is allocated within the backing > > > > # chain (since 6.1) > > > > > > Completely ignoring the value? I would have documented it like this, but > > > with "if true..." instead of "if present...". > > > > This is fine, but it means that this flag will present in all ranges, > > instead of only in unallocated ranges (what this patch is doing). > > An argument for always having the flag would be that it's probably > useful for a tool to know whether a given block is actually absent or > whether it's just running an old qemu-img. Good point, this is the best option. The disadvantage is a bigger output but if you use json you don't care about the size of the output. > If we didn't care about this, I would still define the actual value, but > also document a default. > > Kevin >
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
Am 23.06.2021 um 15:58 hat Nir Soffer geschrieben: > On Wed, Jun 23, 2021 at 11:58 AM Kevin Wolf wrote: > > > > Am 22.06.2021 um 18:56 hat Nir Soffer geschrieben: > > > On Tue, Jun 22, 2021 at 6:38 PM Kevin Wolf wrote: > > > > > > > > Am 11.06.2021 um 21:03 hat Eric Blake geschrieben: > > > > > To save the user from having to check 'qemu-img info --backing-chain' > > > > > or other followup command to determine which "depth":n goes beyond the > > > > > chain, add a boolean field "backing" that is set only for unallocated > > > > > portions of the disk. > > > > > > > > > > Signed-off-by: Eric Blake > > > > > --- > > > > > > > > > > Touches the same iotest output as 1/1. If we decide that switching to > > > > > "depth":n+1 is too risky, and that the mere addition of "backing":true > > > > > while keeping "depth":n is good enough, then we'd have just one patch, > > > > > instead of this double churn. Preferences? > > > > > > > > I think the additional flag is better because it's guaranteed to be > > > > backwards compatible, and because you don't need to know the number of > > > > layers to infer whether a cluster was allocated in the whole backing > > > > chain. And by exposing ALLOCATED we definitely give access to the whole > > > > information that exists in QEMU. > > > > > > > > However, to continue with the bike shedding: I won't insist on > > > > "allocated" even if that is what the flag is called internally and > > > > consistency is usually helpful, but "backing" is misleading, too, > > > > because intuitively it doesn't cover the top layer or standalone images > > > > without a backing file. How about something like "present"? > > > > > > Looks hard to document: > > > > > > # @present: if present and false, the range is not allocated within the > > > # backing chain (since 6.1) > > > > I'm not sure why you would document it with a double negative. > > > > > And is not consistent with "offset". It would work better as: > > > > > > # @present: if present, the range is allocated within the backing > > > # chain (since 6.1) > > > > Completely ignoring the value? I would have documented it like this, but > > with "if true..." instead of "if present...". > > This is fine, but it means that this flag will present in all ranges, > instead of only in unallocated ranges (what this patch is doing). An argument for always having the flag would be that it's probably useful for a tool to know whether a given block is actually absent or whether it's just running an old qemu-img. If we didn't care about this, I would still define the actual value, but also document a default. Kevin
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
On Wed, Jun 23, 2021 at 11:58 AM Kevin Wolf wrote: > > Am 22.06.2021 um 18:56 hat Nir Soffer geschrieben: > > On Tue, Jun 22, 2021 at 6:38 PM Kevin Wolf wrote: > > > > > > Am 11.06.2021 um 21:03 hat Eric Blake geschrieben: > > > > To save the user from having to check 'qemu-img info --backing-chain' > > > > or other followup command to determine which "depth":n goes beyond the > > > > chain, add a boolean field "backing" that is set only for unallocated > > > > portions of the disk. > > > > > > > > Signed-off-by: Eric Blake > > > > --- > > > > > > > > Touches the same iotest output as 1/1. If we decide that switching to > > > > "depth":n+1 is too risky, and that the mere addition of "backing":true > > > > while keeping "depth":n is good enough, then we'd have just one patch, > > > > instead of this double churn. Preferences? > > > > > > I think the additional flag is better because it's guaranteed to be > > > backwards compatible, and because you don't need to know the number of > > > layers to infer whether a cluster was allocated in the whole backing > > > chain. And by exposing ALLOCATED we definitely give access to the whole > > > information that exists in QEMU. > > > > > > However, to continue with the bike shedding: I won't insist on > > > "allocated" even if that is what the flag is called internally and > > > consistency is usually helpful, but "backing" is misleading, too, > > > because intuitively it doesn't cover the top layer or standalone images > > > without a backing file. How about something like "present"? > > > > Looks hard to document: > > > > # @present: if present and false, the range is not allocated within the > > # backing chain (since 6.1) > > I'm not sure why you would document it with a double negative. > > > And is not consistent with "offset". It would work better as: > > > > # @present: if present, the range is allocated within the backing > > # chain (since 6.1) > > Completely ignoring the value? I would have documented it like this, but > with "if true..." instead of "if present...". This is fine, but it means that this flag will present in all ranges, instead of only in unallocated ranges (what this patch is doing). > > > Or: > > > > # @absent: if present, the range is not allocated within the backing > > # chain (since 6.1) > > This is possible, too, but generally positive flags are preferable to > negative ones, and the internal one is already positive. > > > This is used by libnbd now: > > https://github.com/libguestfs/libnbd/commit/1d01d2ac4f6443b160b7d81119d555e1aaedb56d > > > > But I'm fine with "backing", It is consistent with BLK_BACKING_FILE, > > meaning this area exposes data from a backing file (if one exists). > > > > We use "backing" internally to be consistent with future qemu-img. > > I just realised that I actually misunderstood "backing" to mean the > opposite of what it is in this patch! > > It really means "the data comes from some imaginary additional backing > file that doesn't exist in the backing chain", while I understood it as > "something in the (real) backing chain contains the data". > > "present" or "absent" should be much less prone to such > misunderstandings. > > Kevin >
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
Am 22.06.2021 um 18:56 hat Nir Soffer geschrieben: > On Tue, Jun 22, 2021 at 6:38 PM Kevin Wolf wrote: > > > > Am 11.06.2021 um 21:03 hat Eric Blake geschrieben: > > > To save the user from having to check 'qemu-img info --backing-chain' > > > or other followup command to determine which "depth":n goes beyond the > > > chain, add a boolean field "backing" that is set only for unallocated > > > portions of the disk. > > > > > > Signed-off-by: Eric Blake > > > --- > > > > > > Touches the same iotest output as 1/1. If we decide that switching to > > > "depth":n+1 is too risky, and that the mere addition of "backing":true > > > while keeping "depth":n is good enough, then we'd have just one patch, > > > instead of this double churn. Preferences? > > > > I think the additional flag is better because it's guaranteed to be > > backwards compatible, and because you don't need to know the number of > > layers to infer whether a cluster was allocated in the whole backing > > chain. And by exposing ALLOCATED we definitely give access to the whole > > information that exists in QEMU. > > > > However, to continue with the bike shedding: I won't insist on > > "allocated" even if that is what the flag is called internally and > > consistency is usually helpful, but "backing" is misleading, too, > > because intuitively it doesn't cover the top layer or standalone images > > without a backing file. How about something like "present"? > > Looks hard to document: > > # @present: if present and false, the range is not allocated within the > # backing chain (since 6.1) I'm not sure why you would document it with a double negative. > And is not consistent with "offset". It would work better as: > > # @present: if present, the range is allocated within the backing > # chain (since 6.1) Completely ignoring the value? I would have documented it like this, but with "if true..." instead of "if present...". > Or: > > # @absent: if present, the range is not allocated within the backing > # chain (since 6.1) This is possible, too, but generally positive flags are preferable to negative ones, and the internal one is already positive. > This is used by libnbd now: > https://github.com/libguestfs/libnbd/commit/1d01d2ac4f6443b160b7d81119d555e1aaedb56d > > But I'm fine with "backing", It is consistent with BLK_BACKING_FILE, > meaning this area exposes data from a backing file (if one exists). > > We use "backing" internally to be consistent with future qemu-img. I just realised that I actually misunderstood "backing" to mean the opposite of what it is in this patch! It really means "the data comes from some imaginary additional backing file that doesn't exist in the backing chain", while I understood it as "something in the (real) backing chain contains the data". "present" or "absent" should be much less prone to such misunderstandings. Kevin
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
22.06.2021 18:38, Kevin Wolf wrote: Am 11.06.2021 um 21:03 hat Eric Blake geschrieben: To save the user from having to check 'qemu-img info --backing-chain' or other followup command to determine which "depth":n goes beyond the chain, add a boolean field "backing" that is set only for unallocated portions of the disk. Signed-off-by: Eric Blake --- Touches the same iotest output as 1/1. If we decide that switching to "depth":n+1 is too risky, and that the mere addition of "backing":true while keeping "depth":n is good enough, then we'd have just one patch, instead of this double churn. Preferences? I think the additional flag is better because it's guaranteed to be backwards compatible, and because you don't need to know the number of layers to infer whether a cluster was allocated in the whole backing chain. And by exposing ALLOCATED we definitely give access to the whole information that exists in QEMU. However, to continue with the bike shedding: I won't insist on "allocated" even if that is what the flag is called internally and consistency is usually helpful, but "backing" is misleading, too, because intuitively it doesn't cover the top layer or standalone images without a backing file. How about something like "present"? IMHO, it does cover. If we have qcow2 image with unallocated clusters, but unspecified backing file it's good to know that these unallocated clusters are not simply ZERO, but actually point to backing file, which is just absent now (and therefore read returns zeros). User may start qemu and specify backing file by options, or set backing file in the image, etc. So, the information does make sense anyway. I think it would be good to start saying about backing chains explicitly, and not hide them under "allocated" concept which has different meanings. -- Best regards, Vladimir
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
On Fri, Jun 11, 2021 at 10:03 PM Eric Blake wrote:
>
> To save the user from having to check 'qemu-img info --backing-chain'
> or other followup command to determine which "depth":n goes beyond the
> chain, add a boolean field "backing" that is set only for unallocated
> portions of the disk.
>
> Signed-off-by: Eric Blake
> ---
>
> Touches the same iotest output as 1/1. If we decide that switching to
> "depth":n+1 is too risky, and that the mere addition of "backing":true
> while keeping "depth":n is good enough, then we'd have just one patch,
> instead of this double churn. Preferences?
>
> docs/tools/qemu-img.rst| 3 ++
> qapi/block-core.json | 7 ++-
> qemu-img.c | 15 +-
> tests/qemu-iotests/122.out | 34 +++---
> tests/qemu-iotests/154.out | 96 +++---
> tests/qemu-iotests/179.out | 66 +-
> tests/qemu-iotests/223.out | 24 +-
> tests/qemu-iotests/244.out | 6 +--
> tests/qemu-iotests/252.out | 4 +-
> tests/qemu-iotests/274.out | 16 +++
> tests/qemu-iotests/291.out | 8 ++--
> tests/qemu-iotests/309.out | 4 +-
> 12 files changed, 150 insertions(+), 133 deletions(-)
>
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index c155b1bf3cc8..fbc623b645c3 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -601,6 +601,9 @@ Command description:
> a ``depth``; for example, a depth of 2 refers to the backing file
> of the backing file of *FILENAME*. Depth will be one larger than
> the chain length if no file in the chain provides the data.
> + - an optional ``backing`` field is present with value true if no
> +file in the backing chain provides the data (making it easier to
> +identify when ``depth`` exceeds the chain length).
>
>In JSON format, the ``offset`` field is optional; it is absent in
>cases where ``human`` format would omit the entry or exit with an error.
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 2ea294129e08..cebe12ba16a0 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -264,6 +264,9 @@
> # @offset: if present, the image file stores the data for this range
> # in raw format at the given (host) offset
> #
> +# @backing: if present, the range is not allocated within the backing
> +# chain (since 6.1)
> +#
> # @filename: filename that is referred to by @offset
> #
> # Since: 2.6
> @@ -271,8 +274,8 @@
> ##
> { 'struct': 'MapEntry',
>'data': {'start': 'int', 'length': 'int', 'data': 'bool',
> - 'zero': 'bool', 'depth': 'int', '*offset': 'int',
> - '*filename': 'str' } }
> + 'zero': 'bool', 'depth': 'int', '*backing': 'bool',
> + '*offset': 'int', '*filename': 'str' } }
>
> ##
> # @BlockdevCacheInfo:
> diff --git a/qemu-img.c b/qemu-img.c
> index 33a5cd012b8b..4d357f534803 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2977,8 +2977,13 @@ static int dump_map_entry(OutputFormat output_format,
> MapEntry *e,
> break;
> case OFORMAT_JSON:
> printf("{ \"start\": %"PRId64", \"length\": %"PRId64","
> - " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
> - e->start, e->length, e->depth,
> + " \"depth\": %"PRId64, e->start, e->length, e->depth);
> +if (e->has_backing) {
> +/* Backing should only be set at the end of the chain */
> +assert(e->backing && e->depth > 0);
> +printf(", \"backing\": true");
> +}
It will be easier to inspect the output if common fields come before
optional fields.
> +printf(", \"zero\": %s, \"data\": %s",
> e->zero ? "true" : "false",
> e->data ? "true" : "false");
> if (e->has_offset) {
...
> diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
> index 779dab4847f0..c5aa2c9866f1 100644
> --- a/tests/qemu-iotests/122.out
> +++ b/tests/qemu-iotests/122.out
> @@ -68,11 +68,11 @@ read 65536/65536 bytes at offset 4194304
> read 65536/65536 bytes at offset 8388608
> 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> [{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": true},
> -{ "start": 65536, "length": 4128768, "depth": 1, "zero": true, "data":
> false},
> +{ "start": 65536, "length": 4128768, "depth": 1, "backing": true, "zero":
> true, "data": false},
So this output would be:
[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": true},
{ "start": 65536, "length": 4128768, "depth": 1, "zero": true,
"data": false, "backing": true},
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
On Tue, Jun 22, 2021 at 6:38 PM Kevin Wolf wrote: > > Am 11.06.2021 um 21:03 hat Eric Blake geschrieben: > > To save the user from having to check 'qemu-img info --backing-chain' > > or other followup command to determine which "depth":n goes beyond the > > chain, add a boolean field "backing" that is set only for unallocated > > portions of the disk. > > > > Signed-off-by: Eric Blake > > --- > > > > Touches the same iotest output as 1/1. If we decide that switching to > > "depth":n+1 is too risky, and that the mere addition of "backing":true > > while keeping "depth":n is good enough, then we'd have just one patch, > > instead of this double churn. Preferences? > > I think the additional flag is better because it's guaranteed to be > backwards compatible, and because you don't need to know the number of > layers to infer whether a cluster was allocated in the whole backing > chain. And by exposing ALLOCATED we definitely give access to the whole > information that exists in QEMU. > > However, to continue with the bike shedding: I won't insist on > "allocated" even if that is what the flag is called internally and > consistency is usually helpful, but "backing" is misleading, too, > because intuitively it doesn't cover the top layer or standalone images > without a backing file. How about something like "present"? Looks hard to document: # @present: if present and false, the range is not allocated within the # backing chain (since 6.1) And is not consistent with "offset". It would work better as: # @present: if present, the range is allocated within the backing # chain (since 6.1) Or: # @absent: if present, the range is not allocated within the backing # chain (since 6.1) This is used by libnbd now: https://github.com/libguestfs/libnbd/commit/1d01d2ac4f6443b160b7d81119d555e1aaedb56d But I'm fine with "backing", It is consistent with BLK_BACKING_FILE, meaning this area exposes data from a backing file (if one exists). We use "backing" internally to be consistent with future qemu-img.
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
Am 11.06.2021 um 21:03 hat Eric Blake geschrieben: > To save the user from having to check 'qemu-img info --backing-chain' > or other followup command to determine which "depth":n goes beyond the > chain, add a boolean field "backing" that is set only for unallocated > portions of the disk. > > Signed-off-by: Eric Blake > --- > > Touches the same iotest output as 1/1. If we decide that switching to > "depth":n+1 is too risky, and that the mere addition of "backing":true > while keeping "depth":n is good enough, then we'd have just one patch, > instead of this double churn. Preferences? I think the additional flag is better because it's guaranteed to be backwards compatible, and because you don't need to know the number of layers to infer whether a cluster was allocated in the whole backing chain. And by exposing ALLOCATED we definitely give access to the whole information that exists in QEMU. However, to continue with the bike shedding: I won't insist on "allocated" even if that is what the flag is called internally and consistency is usually helpful, but "backing" is misleading, too, because intuitively it doesn't cover the top layer or standalone images without a backing file. How about something like "present"? Kevin
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
On Tue, Jun 15, 2021 at 11:54 AM Vladimir Sementsov-Ogievskiy wrote: > > 11.06.2021 22:03, Eric Blake wrote: > > To save the user from having to check 'qemu-img info --backing-chain' > > or other followup command to determine which "depth":n goes beyond the > > chain, add a boolean field "backing" that is set only for unallocated > > portions of the disk. > > > > Signed-off-by: Eric Blake > > --- > > > > Touches the same iotest output as 1/1. If we decide that switching to > > "depth":n+1 is too risky, and that the mere addition of "backing":true > > while keeping "depth":n is good enough, then we'd have just one patch, > > instead of this double churn. Preferences? > > If change something, this one patch seems safer. Still, Nir said he don't use > qemu-img map, so probably we don't need to modify qemu-img at all? Even our > iotests change shows that this change may be incompatible with at least > tests.. > > I'm not against the patch and don't have strict opinion. > > And what I really think, is that qemu-img is outdated thing and we'd better > develop QMP interface, which can be used with qemu binary or with > qemu-storage-daemon. I don't think qemu-storage-daemon can replace qemu-img. Having an easy to use command line tool is important. Using qmp with qemu-storage-daemon sounds like a better option for programs that want ultimate control. Adding only "backing: true" seems a safe change that should not break existing users and make qemu-img map better. The tests are broken because they compare strings instead of parsing the json. A program parsing qemu-img json output will not be broken by adding a new key. Nir
Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
11.06.2021 22:03, Eric Blake wrote: To save the user from having to check 'qemu-img info --backing-chain' or other followup command to determine which "depth":n goes beyond the chain, add a boolean field "backing" that is set only for unallocated portions of the disk. Signed-off-by: Eric Blake --- Touches the same iotest output as 1/1. If we decide that switching to "depth":n+1 is too risky, and that the mere addition of "backing":true while keeping "depth":n is good enough, then we'd have just one patch, instead of this double churn. Preferences? If change something, this one patch seems safer. Still, Nir said he don't use qemu-img map, so probably we don't need to modify qemu-img at all? Even our iotests change shows that this change may be incompatible with at least tests.. I'm not against the patch and don't have strict opinion. And what I really think, is that qemu-img is outdated thing and we'd better develop QMP interface, which can be used with qemu binary or with qemu-storage-daemon. -- Best regards, Vladimir
[PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments
To save the user from having to check 'qemu-img info --backing-chain'
or other followup command to determine which "depth":n goes beyond the
chain, add a boolean field "backing" that is set only for unallocated
portions of the disk.
Signed-off-by: Eric Blake
---
Touches the same iotest output as 1/1. If we decide that switching to
"depth":n+1 is too risky, and that the mere addition of "backing":true
while keeping "depth":n is good enough, then we'd have just one patch,
instead of this double churn. Preferences?
docs/tools/qemu-img.rst| 3 ++
qapi/block-core.json | 7 ++-
qemu-img.c | 15 +-
tests/qemu-iotests/122.out | 34 +++---
tests/qemu-iotests/154.out | 96 +++---
tests/qemu-iotests/179.out | 66 +-
tests/qemu-iotests/223.out | 24 +-
tests/qemu-iotests/244.out | 6 +--
tests/qemu-iotests/252.out | 4 +-
tests/qemu-iotests/274.out | 16 +++
tests/qemu-iotests/291.out | 8 ++--
tests/qemu-iotests/309.out | 4 +-
12 files changed, 150 insertions(+), 133 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index c155b1bf3cc8..fbc623b645c3 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -601,6 +601,9 @@ Command description:
a ``depth``; for example, a depth of 2 refers to the backing file
of the backing file of *FILENAME*. Depth will be one larger than
the chain length if no file in the chain provides the data.
+ - an optional ``backing`` field is present with value true if no
+file in the backing chain provides the data (making it easier to
+identify when ``depth`` exceeds the chain length).
In JSON format, the ``offset`` field is optional; it is absent in
cases where ``human`` format would omit the entry or exit with an error.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2ea294129e08..cebe12ba16a0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -264,6 +264,9 @@
# @offset: if present, the image file stores the data for this range
# in raw format at the given (host) offset
#
+# @backing: if present, the range is not allocated within the backing
+# chain (since 6.1)
+#
# @filename: filename that is referred to by @offset
#
# Since: 2.6
@@ -271,8 +274,8 @@
##
{ 'struct': 'MapEntry',
'data': {'start': 'int', 'length': 'int', 'data': 'bool',
- 'zero': 'bool', 'depth': 'int', '*offset': 'int',
- '*filename': 'str' } }
+ 'zero': 'bool', 'depth': 'int', '*backing': 'bool',
+ '*offset': 'int', '*filename': 'str' } }
##
# @BlockdevCacheInfo:
diff --git a/qemu-img.c b/qemu-img.c
index 33a5cd012b8b..4d357f534803 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2977,8 +2977,13 @@ static int dump_map_entry(OutputFormat output_format,
MapEntry *e,
break;
case OFORMAT_JSON:
printf("{ \"start\": %"PRId64", \"length\": %"PRId64","
- " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
- e->start, e->length, e->depth,
+ " \"depth\": %"PRId64, e->start, e->length, e->depth);
+if (e->has_backing) {
+/* Backing should only be set at the end of the chain */
+assert(e->backing && e->depth > 0);
+printf(", \"backing\": true");
+}
+printf(", \"zero\": %s, \"data\": %s",
e->zero ? "true" : "false",
e->data ? "true" : "false");
if (e->has_offset) {
@@ -2999,6 +3004,7 @@ static int get_block_status(BlockDriverState *bs, int64_t
offset,
{
int ret;
int depth;
+bool backing = false;
BlockDriverState *file;
bool has_offset;
int64_t map;
@@ -3037,6 +3043,7 @@ static int get_block_status(BlockDriverState *bs, int64_t
offset,
}
if (!(ret & BDRV_BLOCK_ALLOCATED)) {
depth++;
+backing = true;
}
*e = (MapEntry) {
@@ -3047,6 +3054,8 @@ static int get_block_status(BlockDriverState *bs, int64_t
offset,
.offset = map,
.has_offset = has_offset,
.depth = depth,
+.has_backing = backing,
+.backing = backing,
.has_filename = filename,
.filename = filename,
};
@@ -3072,6 +3081,8 @@ static inline bool entry_mergeable(const MapEntry *curr,
const MapEntry *next)
if (curr->has_offset && curr->offset + curr->length != next->offset) {
return false;
}
+/* backing should only ever be set for identical depth */
+assert(curr->backing == next->backing);
return true;
}
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 779dab4847f0..c5aa2c9866f1 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -68,11 +68,11 @@ read 65536/65536 bytes at offset 4194304
read 65536/65536 bytes at offset 8388608
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
[{ "start": 0, "length": 65536,
