From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.3 required=3.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_KAM_HTML_FONT_INVALID autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 054EEC2BB1D for ; Fri, 17 Apr 2020 22:32:22 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B2B05206D9 for ; Fri, 17 Apr 2020 22:32:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="rIM3DWe9" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B2B05206D9 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:52694 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jPZX2-00068W-SP for qemu-devel@archiver.kernel.org; Fri, 17 Apr 2020 18:32:20 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:37315) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jPZW4-0005ig-UJ for qemu-devel@nongnu.org; Fri, 17 Apr 2020 18:31:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1jPZW2-0000k0-Q5 for qemu-devel@nongnu.org; Fri, 17 Apr 2020 18:31:20 -0400 Received: from mail-io1-xd2f.google.com ([2607:f8b0:4864:20::d2f]:35187) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1jPZW2-0000iJ-IC; Fri, 17 Apr 2020 18:31:18 -0400 Received: by mail-io1-xd2f.google.com with SMTP id w20so4139183iob.2; Fri, 17 Apr 2020 15:31:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=us88UQFyqK65CCki2iEuIy+I+3THwFkJtAE6WzuC2fE=; b=rIM3DWe933vCYKXKllWOphiVsg+tYv/+Z6vURidroPlvHXL075z3x9GVE1hd1mFU8u 6D38Bv2AFbZdRVBDW+DNUAQXaMXl8P+LDWW9S43mCAPlEkEAFi7Q4cuDSyKly3GK7buT LMxB3bw9L7nE2q1Lirl0mVTKEg0lxQXIV7Sjlc1fsW285baHvfzSbLzRPIBCIr/E71u3 KP1ILu2nD4uIzO1bdXD/EMeU/m/ZhUtIgtoAjCRO31f1M8y8e/JxU5qC9Kzj68wcei00 uLT02deEv38+R6+nZ8PywApX8bJ0qLtRZ8Obj/+tjtrJjSjbrfdgWPpkXHz3cfjgfPNM m5jA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=us88UQFyqK65CCki2iEuIy+I+3THwFkJtAE6WzuC2fE=; b=uO3oSf10xlPriHHAk++jxoKWch+a1ru5W7n++tKk0adsgaCGoglDEQJUHjGEeY0GgF ORi6hL9HPnaOyiPUEYRdaLypiN/qNSTjXEWv9YJ63DJv/s+BPuZP8E20Exp3V8LABejy +JkhWfzIagD3Kpm040Ohsc2l7ofZTfafETEc3ZSdC/W1fv+EIlKqewgxmiVFLwXwqp+7 2i/oQAHhHfHqNCbwtAuf+/hUSzsxaTG5bYI5cChA+rD0ZWEJZFhe1Xoo2LW4C1FZ3Vtl l4Yo+2eIRtX++XS65i6AZhfsmrXHpagrcB/jtkZkl6cyNWP6WdUCUhntIfkx+SR10JFA TWkA== X-Gm-Message-State: AGi0PuZ9pVaPwnCpmygrvvE4CumKJOfuPw10kxtBU2Yf9tdGmTO+MsBM PZhvg256rp+QAkA7tZYqGVOM9TIEHT2NRBeBtfk= X-Google-Smtp-Source: APiQypIAuO50THxGjoQKdpUbh473kukUNw5LDNIu49rYTdJbAGHPKvD1MKwIEmJ2LjOEmMShj713mLmYGOioYC2Lby0= X-Received: by 2002:a5d:9c46:: with SMTP id 6mr5377897iof.146.1587162677587; Fri, 17 Apr 2020 15:31:17 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Leo Luan Date: Fri, 17 Apr 2020 15:31:06 -0700 Message-ID: Subject: Re: Avoid copying unallocated clusters during full backup To: John Snow Content-Type: multipart/alternative; boundary="00000000000099364a05a3841bb2" X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4864:20::d2f X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org, Qemu-block , Max Reitz Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --00000000000099364a05a3841bb2 Content-Type: text/plain; charset="UTF-8" On Fri, Apr 17, 2020 at 1:11 PM John Snow wrote: > > > On 4/17/20 2:33 PM, Leo Luan wrote: > > When doing a full backup from a single layer qcow2 disk file to a new > > qcow2 file, the backup_run function does not unset unallocated parts in > > the copy bit map. The subsequent backup_loop call goes through these > > unallocated clusters unnecessarily. In the case when the target and > > source reside in different file systems, an EXDEV error would cause > > zeroes to be actually copied into the target and that causes a target > > file size explosion to the full virtual disk size. > > > > I think the idea, generally, is to leave the detection of unallocated > portions to the format (qcow2) and the protocol (posix file) respectively. > > As far as I know, it is incorrect to assume that unallocated data > can/will/should always be read as zeroes; so it may not be the case that > it is "safe" to skip this data, because the target may or may not need > explicit zeroing. > Thanks for pointing this out. Would it be safe to skip unallocated clusters if both source and target's bdrv_unallocated_blocks_are_zero() returns true? > This patch aims to unset the unallocated parts in the copy bitmap when > > it is safe to do so, thereby avoid dealing with unallocated clusters in > > the backup loop to prevent significant performance or storage efficiency > > impacts when running full backup jobs. > > > > Any insights or corrections? > > > > diff --git a/block/backup.c b/block/backup.c > > index cf62b1a38c..609d551b1e 100644 > > --- a/block/backup.c > > +++ b/block/backup.c > > @@ -139,6 +139,29 @@ static void backup_clean(Job *job) > > bdrv_backup_top_drop(s->backup_top); > > } > > > > +static bool backup_ok_to_skip_unallocated(BackupBlockJob *s) > > +{ > > + /* Checks whether this backup job can avoid copying or dealing with > > + unallocated clusters in the backup loop and their associated > > + performance and storage effciency impacts. Check for the > condition > > + when it's safe to skip copying unallocated clusters that allows > the > > + corresponding bits in the copy bitmap to be unset. The > assumption > > + here is that it is ok to do so when we are doing a full backup, > > + the target file is a qcow2, and the source is single layer. > > + Do we need to add additional checks (so that it does not break > > + something) or add addtional conditions to optimize additional use > > + cases? > > + */ > > + > > + if (s->sync_mode == MIRROR_SYNC_MODE_FULL && > > + s->bcs->target->bs->drv != NULL && > > + strncmp(s->bcs->target->bs->drv->format_name, "qcow2", 5) == 0 && > > + s->bcs->source->bs->backing_file[0] == '\0') > > This isn't going to suffice upstream; the backup job can't be performing > format introspection to determine behavior on the fly. > > I think what you're really after is something like > bdrv_unallocated_blocks_are_zero(). > Thanks for this pointer. > > > + return true; > > + else > > + return false; > > +} > > + > > void backup_do_checkpoint(BlockJob *job, Error **errp) > > { > > BackupBlockJob *backup_job = container_of(job, BackupBlockJob, > common); > > @@ -248,7 +271,7 @@ static int coroutine_fn backup_run(Job *job, Error > > **errp) > > > > backup_init_copy_bitmap(s); > > > > - if (s->sync_mode == MIRROR_SYNC_MODE_TOP) { > > + if (s->sync_mode == MIRROR_SYNC_MODE_TOP || > > So the basic premise is that if you are copying a qcow2 file and the > unallocated portions as defined by the qcow2 metadata are zero, it's > safe to skip those, so you can treat it like SYNC_MODE_TOP. > In the MIRROR_SYNC_MODE_TOP case, the check for unallocated clusters does not go all the way to the base level. So it would be incorrect to treat the MIRROR_SYNC_MODE_FULL the same as MIRROR_SYNC_MODE_TOP unless the source does not have a backing and the base itself. That's why I added a check for the backing_file field of the source. I guess the code can be potentially extended with a new flag to do the block status check all the way into the base level for the case of the FULL mode? I think you *also* have to know if the *source* needs those regions > explicitly zeroed, and it's not always safe to just skip them at the > manifest level. > Do you mean some operation changing the target into non-sparse? > > I thought there was code that handled this to some extent already, but I > don't know. I think Vladimir has worked on it recently and can probably > let you know where I am mistaken :) > Thanks for the reply! > --js > > > backup_ok_to_skip_unallocated(s)) { > > int64_t offset = 0; > > int64_t count; > > > > John Snow --00000000000099364a05a3841bb2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Fri, Apr 17, 2020= at 1:11 PM John Snow <jsnow@redhat.com> wrote:


On 4/17/20 2:33 PM, Leo Luan wrote:
> When doing a full = backup from a single layer qcow2 disk file to a new
> qcow2 file, the= backup_run function does not unset unallocated parts in
> the copy b= it map.=C2=A0 The subsequent backup_loop call goes through these
> un= allocated clusters unnecessarily.=C2=A0 In the case when the target and
= > source reside in different file systems, an EXDEV error would cause> zeroes to be actually copied into the target and that causes a target=
> file size explosion to the full virtual disk size.
>

= I think the idea, generally, is to leave the detection of unallocated
po= rtions to the format (qcow2) and the protocol (posix file) respectively.
As far as I know, it is incorrect to assume that unallocated data
c= an/will/should always be read as zeroes; so it may not be the case that
= it is "safe" to skip this data, because the target may or may not= need
explicit zeroing.

Thank= s for pointing this out.=C2=A0 Would it be safe to skip unallocated cluster= s if both source and target's=C2=A0bdrv_unallocated_blocks_are_zero() r= eturns true?

=
> This patch aim= s to unset the unallocated parts in the copy bitmap when
> it is safe= to do so, thereby avoid dealing with unallocated clusters in
> the b= ackup loop to prevent significant performance or storage efficiency
>= impacts when running full backup jobs.
>
> Any insights or cor= rections?
>
> diff --git a/block/backup.c b/block/backup.c
&= gt; index cf62b1a38c..609d551b1e 100644
> --- a/block/backup.c
>= ; +++ b/block/backup.c
> @@ -139,6 +139,29 @@ static void backup_clea= n(Job *job)
> =C2=A0 =C2=A0 =C2=A0bdrv_backup_top_drop(s->backup_t= op);
> =C2=A0}
> =C2=A0
> +static bool backup_ok_to_skip_= unallocated(BackupBlockJob *s)
> +{
> + =C2=A0 =C2=A0/* Checks = whether this backup job can avoid copying or dealing with
> + =C2=A0 = =C2=A0 =C2=A0 unallocated clusters in the backup loop and their associated<= br>> + =C2=A0 =C2=A0 =C2=A0 performance and storage effciency impacts. C= heck for the condition
> + =C2=A0 =C2=A0 =C2=A0 when it's safe to= skip copying unallocated clusters that allows the
> + =C2=A0 =C2=A0 = =C2=A0 corresponding bits in the copy bitmap to be unset.=C2=A0 The assumpt= ion
> + =C2=A0 =C2=A0 =C2=A0 here is that it is ok to do so when we a= re doing a full backup,
> + =C2=A0 =C2=A0 =C2=A0 the target file is a= qcow2, and the source is single layer.
> + =C2=A0 =C2=A0 =C2=A0 Do w= e need to add additional checks (so that it does not break
> + =C2=A0= =C2=A0 =C2=A0 something) or add addtional conditions to optimize additiona= l use
> + =C2=A0 =C2=A0 =C2=A0 cases?
> + =C2=A0 =C2=A0 */
&= gt; +
> + =C2=A0 =C2=A0if (s->sync_mode =3D=3D MIRROR_SYNC_MODE_FU= LL &&
> + =C2=A0 =C2=A0 =C2=A0 s->bcs->target->bs-&g= t;drv !=3D NULL &&
> + =C2=A0 =C2=A0 =C2=A0 strncmp(s->bcs= ->target->bs->drv->format_name, "qcow2", 5) =3D=3D 0 = &&
> + =C2=A0 =C2=A0 =C2=A0 s->bcs->source->bs->b= acking_file[0] =3D=3D '\0')

This isn't going to suffice = upstream; the backup job can't be performing
format introspection to= determine behavior on the fly.

I think what you're really after= is something like
bdrv_unallocated_blocks_are_zero().
<= div>
Thanks for this pointer.
=C2=A0

> + =C2=A0 =C2=A0 =C2=A0 return true;
> + =C2=A0 =C2=A0else=
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return false;
> +}
> +> =C2=A0void backup_do_checkpoint(BlockJob *job, Error **errp)
> = =C2=A0{
> =C2=A0 =C2=A0 =C2=A0BackupBlockJob *backup_job =3D containe= r_of(job, BackupBlockJob, common);
> @@ -248,7 +271,7 @@ static int c= oroutine_fn backup_run(Job *job, Error
> **errp)
> =C2=A0
&g= t; =C2=A0 =C2=A0 =C2=A0backup_init_copy_bitmap(s);
> =C2=A0
> -= =C2=A0 =C2=A0if (s->sync_mode =3D=3D MIRROR_SYNC_MODE_TOP) {
> + = =C2=A0 =C2=A0if (s->sync_mode =3D=3D MIRROR_SYNC_MODE_TOP ||

So t= he basic premise is that if you are copying a qcow2 file and the
unalloc= ated portions as defined by the qcow2 metadata are zero, it's
safe t= o skip those, so you can treat it like SYNC_MODE_TOP.
=
In the MIRROR_SYNC_MODE_TOP case, the check for unall= ocated clusters does not go all the way to the base level.=C2=A0 So it woul= d be incorrect to treat the MIRROR_SYNC_MODE_FULL the same as MIRROR_SYNC_M= ODE_TOP unless the source does not have a backing and the base itself.=C2= =A0 That's why I added a check for the backing_file field of the source= .=C2=A0 I guess the code can be potentially extended with a new flag to do = the block status check all the way into the base level for the case of the = FULL mode?

I think you *also* have to know if the *so= urce* needs those regions
explicitly zeroed, and it's not always saf= e to just skip them at the
manifest level.

Do you mean some operation changing the target into non-spars= e?

I thought there was code that handled this to some extent alrea= dy, but I
don't know. I think Vladimir has worked on it recently and= can probably
let you know where I am mistaken :)
<= br>
Thanks for the reply!=C2=A0
=C2=A0
--js
> backup_ok_to_skip_unallocated(s)) {
> =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0int64_t offset =3D 0;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0int64_t count;
> =C2=A0

=
<= form id=3D"gmail-:ffa" action=3D"https://mail.google.com/mail/u/0/" method= =3D"POST" enctype=3D"multipart/form-data" class=3D"gmail-bAs">
3D""
John Snow

--00000000000099364a05a3841bb2--