From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45953) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYNXk-0007Qc-NY for qemu-devel@nongnu.org; Tue, 23 Feb 2016 19:43:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aYNXj-00034c-ED for qemu-devel@nongnu.org; Tue, 23 Feb 2016 19:43:04 -0500 References: <1456273021-17473-1-git-send-email-jsnow@redhat.com> <1456273021-17473-3-git-send-email-jsnow@redhat.com> <20160224002757.GA749@ad.usersys.redhat.com> From: John Snow Message-ID: <56CCFC8E.8050003@redhat.com> Date: Tue, 23 Feb 2016 19:42:54 -0500 MIME-Version: 1.0 In-Reply-To: <20160224002757.GA749@ad.usersys.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 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, qemu-block@nongnu.org, jcody@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, stefanha@redhat.com On 02/23/2016 07:27 PM, Fam Zheng wrote: > On Tue, 02/23 19:17, 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 image, >> we may inadvertantly create unusable incremental backup images. >> >> For example: >> >> If the bitmap tracks changes at a 64KB granularity and we transmit 64KB >> 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 dirty >> 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 points >> 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 the >> target granularity. >> >> The same logic applies to backup modes top, none, and full. Copying >> fractional clusters without the guarantee of COW is dangerous, but even >> if we can rely on COW, it's likely better to just re-copy the data. >> >> Reported-by: Fam Zheng >> Signed-off-by: John Snow >> --- >> block/backup.c | 23 ++++++++++++++++++++--- >> 1 file changed, 20 insertions(+), 3 deletions(-) >> >> diff --git a/block/backup.c b/block/backup.c >> index 76addef..6e9f53d 100644 >> --- a/block/backup.c >> +++ b/block/backup.c >> @@ -501,6 +501,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, >> BlockJobTxn *txn, Error **errp) >> { >> int64_t len; >> + BlockDriverInfo bdi; >> + int ret; >> >> assert(bs); >> assert(target); >> @@ -570,15 +572,30 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, >> goto error; >> } >> >> - bdrv_op_block_all(target, job->common.blocker); >> - >> job->on_source_error = on_source_error; >> job->on_target_error = on_target_error; >> job->target = target; >> job->sync_mode = sync_mode; >> job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ? >> sync_bitmap : NULL; >> - job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT; >> + >> + /* If there is no backing file on the target, we cannot rely on COW if our >> + * backup cluster size is smaller than the target cluster size. Even for >> + * targets with a backing file, try to avoid COW if possible. */ >> + ret = bdrv_get_info(job->target, &bdi); >> + if (ret < 0 && !target->backing) { >> + error_setg_errno(errp, -ret, >> + "Can't determine cluster size of target that has no backing file. " >> + "This may create an unusable destination image. Aborting."); > > Most error messages don't end with a period, but that's not a hard rule AFAICT. If that's the only problem, Jeff can touch it up. I want to ask, though: "Are there any cases where we will have no backing file and no bdrv_get_info implementation, but backup should still succeed?" I think the answer is /no/ since everything we claim to support for use with QEMU (instead of qemu-img only) should implement bdrv_get_info. > >> + goto error; >> + } else if (ret < 0 && target->backing) { >> + /* Not fatal; just trudge on ahead. */ >> + job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT; >> + } else { >> + job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size); >> + } >> + >> + bdrv_op_block_all(target, job->common.blocker); >> job->common.len = len; >> job->common.co = qemu_coroutine_create(backup_run); >> block_job_txn_add_job(txn, &job->common); >> -- >> 2.4.3 >> > > Reviewed-by: Fam Zheng > Thanks.