From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56280) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVhmc-0008MF-VM for qemu-devel@nongnu.org; Tue, 16 Feb 2016 10:43:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aVhma-0002J7-8f for qemu-devel@nongnu.org; Tue, 16 Feb 2016 10:43:22 -0500 References: <1455318392-26765-1-git-send-email-jsnow@redhat.com> <1455318392-26765-3-git-send-email-jsnow@redhat.com> <20160214064919.GB31933@ad.usersys.redhat.com> From: John Snow Message-ID: <56C3438D.3080604@redhat.com> Date: Tue, 16 Feb 2016 10:43:09 -0500 MIME-Version: 1.0 In-Reply-To: <20160214064919.GB31933@ad.usersys.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/3] block/backup: avoid copying less than full target clusters List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, jcody@redhat.com, stefanha@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org On 02/14/2016 01:49 AM, Fam Zheng wrote: > On Fri, 02/12 18:06, John Snow wrote: >> During incremental backups, if the target has a cluster size that is >> larger than the backup cluster size and we are backing up to a target >> that cannot (for whichever reason) pull clusters up from a backing ima= ge, >> we may inadvertantly create unusable incremental backup images. >> >> For example: >> >> If the bitmap tracks changes at a 64KB granularity and we transmit 64K= B >> of data at a time but the target uses a 128KB cluster size, it is >> possible that only half of a target cluster will be recognized as dirt= y >> by the backup block job. When the cluster is allocated on the target >> image but only half populated with data, we lose the ability to >> distinguish between zero padding and uninitialized data. >> >> This does not happen if the target image has a backing file that point= s >> to the last known good backup. >> >> Even if we have a backing file, though, it's likely going to be faster >> to just buffer the redundant data ourselves from the live image than >> fetching it from the backing file, so let's just always round up to th= e >> target granularity. >> >> Signed-off-by: John Snow >> --- >> block/backup.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/block/backup.c b/block/backup.c >> index fcf0043..62faf81 100644 >> --- a/block/backup.c >> +++ b/block/backup.c >> @@ -568,9 +568,16 @@ void backup_start(BlockDriverState *bs, BlockDriv= erState *target, >> job->on_target_error =3D on_target_error; >> job->target =3D target; >> job->sync_mode =3D sync_mode; >> - job->sync_bitmap =3D sync_mode =3D=3D MIRROR_SYNC_MODE_INCREMENTA= L ? >> - sync_bitmap : NULL; >> - job->cluster_size =3D BACKUP_CLUSTER_SIZE_DEFAULT; >> + if (sync_mode =3D=3D MIRROR_SYNC_MODE_INCREMENTAL) { >> + BlockDriverInfo bdi; >> + >> + bdrv_get_info(job->target, &bdi); >> + job->sync_bitmap =3D sync_bitmap; >> + job->cluster_size =3D MAX(BACKUP_CLUSTER_SIZE_DEFAULT, >> + bdi.cluster_size); >=20 > Why not just do it for all sync modes? >=20 > Fam >=20 Caught me not thinking about those. sync=3Dfull is probably OK as-is, but top and none suffer from a similar problem, you're right. Incremental is the worst offender since the bitmap used to create the bitmap will have been consumed, but I'll pay heed to the other modes in v= 2. >> + } else { >> + job->cluster_size =3D BACKUP_CLUSTER_SIZE_DEFAULT; >> + } >> job->sectors_per_cluster =3D job->cluster_size / BDRV_SECTOR_SIZE= ; >> job->common.len =3D len; >> job->common.co =3D qemu_coroutine_create(backup_run); >> --=20 >> 2.4.3 >> --=20 =97js