From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Nir Soffer <nsoffer@redhat.com>
Cc: qemu-block <qemu-block@nongnu.org>,
QEMU Developers <qemu-devel@nongnu.org>,
Kevin Wolf <kwolf@redhat.com>, Max Reitz <hreitz@redhat.com>,
Eric Blake <eblake@redhat.com>, John Snow <jsnow@redhat.com>,
den@openvz.org
Subject: Re: [PATCH 3/4] qemu-img: add --shallow option for qemu-img compare --stat
Date: Wed, 29 Sep 2021 19:28:21 +0300 [thread overview]
Message-ID: <4b487f2a-947e-0c14-e7cb-992835dcdb27@virtuozzo.com> (raw)
In-Reply-To: <CAMRbyyt4fmsUni0ZxaUTG-FKYgUQeK2N+p1Le9j9tWUOBLuQiA@mail.gmail.com>
29.09.2021 19:00, Nir Soffer wrote:
> On Wed, Sep 29, 2021 at 4:37 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
>>
>> Allow compare only top images of backing chains. That's useful for
>> comparing two increments from the same chain of incremental backups.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> docs/tools/qemu-img.rst | 8 +++++++-
>> qemu-img.c | 14 ++++++++++++--
>> qemu-img-cmds.hx | 4 ++--
>> 3 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
>> index 4b382ca2b0..c8ae96be6a 100644
>> --- a/docs/tools/qemu-img.rst
>> +++ b/docs/tools/qemu-img.rst
>> @@ -176,6 +176,12 @@ Parameters to compare subcommand:
>> - If both files don't specify cluster-size, use default of 64K
>> - If only one file specify cluster-size, just use it.
>>
>> +.. option:: --shallow
>
> We use the same term in oVirt when we upload/download one layer from a chain.
>
>> + Only allowed with ``--stat``. This option prevents opening and comparing
>> + any backing files. This is useful to compare incremental images from
>> + the chain of incremental backups.
>
> This is useful also without --stat. Our current workaround in oVirt is
> to use unsafe
> rebase to disconnect the top image from the base image so we can compare
> source and destination image after backup.
>
> Here is an example of test code that could use --shallow (regardless of --stat):
> https://github.com/oVirt/ovirt-imageio/blob/master/daemon/test/backup_test.py#L114
>
> Do you have any reason to limit --shallow to --stats?
Hmm. I wrongly thought that without --stat qemu-img compare will fail on first mismatch, which will occur soon, as we don't have backing images and it's just superfluous.
But actually, qemu-img will not compare "unallocated" areas.
Ok, I agree, in v2 I'll allow --shallow without --stat.
Another question to discuss: we already have "-u" option in qemu-img create and qemu-img rebase to not open backing files. And 'u' means 'unsafe'.
I don't think that "unsafe" term is good for qemu-img compare --stat, that's why I decided to call it differently: "shallow".
Still for qemu-img compare (without --stat) "unsafe" term make sense.
So, it probably better to follow common notation, and call the option "-u".
>
>> +
>> Parameters to convert subcommand:
>>
>> .. program:: qemu-img-convert
>> @@ -395,7 +401,7 @@ Command description:
>>
>> The rate limit for the commit process is specified by ``-r``.
>>
>> -.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] FILENAME1 FILENAME2
>> +.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE] [--shallow]] FILENAME1 FILENAME2
>>
>> Check if two images have the same content. You can compare images with
>> different format or settings.
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 61e7f470bb..e8ae412c38 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -85,6 +85,7 @@ enum {
>> OPTION_SKIP_BROKEN = 277,
>> OPTION_STAT = 277,
>> OPTION_BLOCK_SIZE = 278,
>> + OPTION_SHALLOW = 279,
>> };
>>
>> typedef enum OutputFormat {
>> @@ -1482,7 +1483,7 @@ static int img_compare(int argc, char **argv)
>> int64_t block_end;
>> int ret = 0; /* return value - 0 Ident, 1 Different, >1 Error */
>> bool progress = false, quiet = false, strict = false;
>> - int flags;
>> + int flags = 0;
>> bool writethrough;
>> int64_t total_size;
>> int64_t offset = 0;
>> @@ -1504,6 +1505,7 @@ static int img_compare(int argc, char **argv)
>> {"force-share", no_argument, 0, 'U'},
>> {"stat", no_argument, 0, OPTION_STAT},
>> {"block-size", required_argument, 0, OPTION_BLOCK_SIZE},
>> + {"shallow", no_argument, 0, OPTION_SHALLOW},
>> {0, 0, 0, 0}
>> };
>> c = getopt_long(argc, argv, ":hf:F:T:pqsU",
>> @@ -1569,6 +1571,9 @@ static int img_compare(int argc, char **argv)
>> exit(EXIT_SUCCESS);
>> }
>> break;
>> + case OPTION_SHALLOW:
>> + flags |= BDRV_O_NO_BACKING;
>> + break;
>> }
>> }
>>
>> @@ -1590,10 +1595,15 @@ static int img_compare(int argc, char **argv)
>> goto out;
>> }
>>
>> + if (!do_stat && (flags & BDRV_O_NO_BACKING)) {
>> + error_report("--shallow can be used only together with --stat");
>> + ret = 1;
>> + goto out;
>> + }
>> +
>> /* Initialize before goto out */
>> qemu_progress_init(progress, 2.0);
>>
>> - flags = 0;
>> ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
>> if (ret < 0) {
>> error_report("Invalid source cache option: %s", cache);
>> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
>> index 96a193eea8..a295bc6860 100644
>> --- a/qemu-img-cmds.hx
>> +++ b/qemu-img-cmds.hx
>> @@ -40,9 +40,9 @@ SRST
>> ERST
>>
>> DEF("compare", img_compare,
>> - "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] filename1 filename2")
>> + "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE] [--shallow]] filename1 filename2")
>> SRST
>> -.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] FILENAME1 FILENAME2
>> +.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE] [--shallow]] FILENAME1 FILENAME2
>> ERST
>>
>> DEF("convert", img_convert,
>> --
>> 2.29.2
>>
>
> Looks good as is, we can remove the limit later without breaking users.
>
> Nir
>
--
Best regards,
Vladimir
next prev parent reply other threads:[~2021-09-29 16:30 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-29 13:34 [PATCH 0/4] qemu-img compare --stat Vladimir Sementsov-Ogievskiy
2021-09-29 13:34 ` [PATCH 1/4] qemu-img: implement " Vladimir Sementsov-Ogievskiy
2021-09-29 13:34 ` [PATCH 2/4] qemu-img: make --block-size optional for " Vladimir Sementsov-Ogievskiy
2021-09-29 13:34 ` [PATCH 3/4] qemu-img: add --shallow option for qemu-img " Vladimir Sementsov-Ogievskiy
2021-09-29 16:00 ` Nir Soffer
2021-09-29 16:28 ` Vladimir Sementsov-Ogievskiy [this message]
2021-09-29 16:47 ` Nir Soffer
2021-09-29 13:34 ` [PATCH 4/4] iotests: add qemu-img-compare-stat test Vladimir Sementsov-Ogievskiy
2021-09-30 0:27 ` [PATCH 0/4] qemu-img compare --stat John Snow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4b487f2a-947e-0c14-e7cb-992835dcdb27@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=den@openvz.org \
--cc=eblake@redhat.com \
--cc=hreitz@redhat.com \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=nsoffer@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).