Re: [Qemu-devel] [PATCH v4 2/3] qemu-img: rebase: Reduce reads on in-chain rebase

From: Sam Eiderman <shmuel.eiderman@oracle.com>
To: Max Reitz <mreitz@redhat.com>
Cc: kwolf@redhat.com, eyal.moscovici@oracle.com,
	qemu-block@nongnu.org, arbel.moshe@oracle.com,
	QEMU <qemu-devel@nongnu.org>,
	liran.alon@oracle.com, sagi.amit@oracle.com,
	karl.heubaum@oracle.com
Subject: Re: [Qemu-devel] [PATCH v4 2/3] qemu-img: rebase: Reduce reads on in-chain rebase
Date: Thu, 23 May 2019 19:35:20 +0300	[thread overview]
Message-ID: <187CC767-4CB1-42AE-8C9C-6F0EF69F40AF@oracle.com> (raw)
In-Reply-To: <fc179916-0fb5-bb50-da41-14789b020be9@redhat.com>

Yes, my bad, resending as v5.

Sam

> On 23 May 2019, at 19:14, Max Reitz <mreitz@redhat.com> wrote:
> 
> On 23.05.19 17:44, Sam Eiderman wrote:
>> In the following case:
>> 
>> (base) A <- B <- C (tip)
>> 
>> when running:
>> 
>>    qemu-img rebase -b A C
>> 
>> QEMU would read all sectors not allocated in the file being rebased (C)
>> and compare them to the new base image (A), regardless of whether they
>> were changed or even allocated anywhere along the chain between the new
>> base and the top image (B). This causes many unneeded reads when
>> rebasing an image which represents a small diff of a large disk, as it
>> would read most of the disk's sectors.
>> 
>> Instead, use bdrv_is_allocated_above() to reduce the number of
>> unnecessary reads.
>> 
>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
>> Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com>
>> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
>> ---
>> qemu-img.c | 25 ++++++++++++++++++++++++-
>> 1 file changed, 24 insertions(+), 1 deletion(-)
>> 
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 9bd0bb1e47..e6fd8e1a98 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
> 
> [...]
> 
>> @@ -3437,6 +3443,23 @@ static int img_rebase(int argc, char **argv)
>>                 continue;
>>             }
>> 
>> +            if (prefix_chain_bs) {
>> +                /*
>> +                 * If cluster wasn't changed since prefix_chain, we don't need
>> +                 * to take action
>> +                 */
>> +                ret = bdrv_is_allocated_above(bs, prefix_chain_bs,
>> +                                              offset, n, &n);
> 
> I think you forgot to update the patch...?
> 
> Max
> 
>> +                if (ret < 0) {
>> +                    error_report("error while reading image metadata: %s",
>> +                                 strerror(-ret));
>> +                    goto out;
>> +                }
>> +                if (!ret) {
>> +                    continue;
>> +                }
>> +            }
>> +
>>             /*
>>              * Read old and new backing file and take into consideration that
>>              * backing files may be smaller than the COW image.