From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58985) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uuka3-00041q-V6 for qemu-devel@nongnu.org; Thu, 04 Jul 2013 10:32:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UukZz-0006UT-F1 for qemu-devel@nongnu.org; Thu, 04 Jul 2013 10:32:19 -0400 Received: from mail-ee0-x230.google.com ([2a00:1450:4013:c00::230]:59117) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UukZz-0006UI-4S for qemu-devel@nongnu.org; Thu, 04 Jul 2013 10:32:15 -0400 Received: by mail-ee0-f48.google.com with SMTP id b47so808967eek.21 for ; Thu, 04 Jul 2013 07:32:14 -0700 (PDT) Date: Thu, 4 Jul 2013 16:32:10 +0200 From: Stefan Hajnoczi Message-ID: <20130704143210.GJ4213@stefanha-thinkpad.redhat.com> References: <1372744789-997-1-git-send-email-famz@redhat.com> <1372744789-997-8-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1372744789-997-8-git-send-email-famz@redhat.com> Subject: Re: [Qemu-devel] [PATCH 7/7] block: assign backing relationship in drive-backup List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, obarenbo@redhat.com, armbru@redhat.com, roliveri@redhat.com, hbrock@redhat.com, qemu-devel@nongnu.org, rjones@redhat.com, pmyers@redhat.com, imain@redhat.com, stefanha@redhat.com, pbonzini@redhat.com On Tue, Jul 02, 2013 at 01:59:49PM +0800, Fam Zheng wrote: > Assign source image as the backing hd of target bs, so reading target bs > gets the point-in-time copy of data from source image. > > Signed-off-by: Fam Zheng > --- > block/backup.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/block/backup.c b/block/backup.c > index 4e9f927..2dd0540 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -294,6 +294,11 @@ static void coroutine_fn backup_run(void *opaque) > hbitmap_free(job->bitmap); > > bdrv_iostatus_disable(target); > + > + bdrv_put_ref(target->backing_hd); > + target->backing_hd = NULL; > + target->backing_file[0] = '\0'; > + target->backing_format[0] = '\0'; > bdrv_put_ref(target); > > block_job_completed(&job->common, ret); > @@ -332,7 +337,15 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > return; > } > > + target->backing_hd = bs; > + pstrcpy(target->backing_file, sizeof(target->backing_file), > + bs->filename); > + pstrcpy(target->backing_format, sizeof(target->backing_format), > + bs->drv->format_name); > bdrv_get_ref(target); > + /* Get another ref to source for backing_hd relationship */ > + bdrv_get_ref(bs); > + > job->on_source_error = on_source_error; > job->on_target_error = on_target_error; > job->target = target; This is a strange way of overriding the backing file. The target exists after drive-backup completes but now has a NULL backing_hd. Also, we set backing_hd even for a raw image. I thought this would be achieved as follows: 1. The management tool creates the qcow2 file. 2. drive-add if=none,id=target,file=backup.qcow2,backing_hd=drive0 3. drive-backup drive=drive0,target=target,mode=existing Something along these lines. The difference is that we do not force override the backing_hd. It is only done when the management tool/user decides explicitly to use backing files. Also, the target is left in a usable state after the blockjob completes. I know the drive-add approach has been shelved in favor of directly using drive-backup, but this patch seems a little too hacky IMO. Stefan