From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VuylO-0005Ri-I8 for qemu-devel@nongnu.org; Mon, 23 Dec 2013 01:13:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VuylF-0006If-F6 for qemu-devel@nongnu.org; Mon, 23 Dec 2013 01:13:14 -0500 Received: from e23smtp03.au.ibm.com ([202.81.31.145]:45303) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VuylE-0006Hz-Os for qemu-devel@nongnu.org; Mon, 23 Dec 2013 01:13:05 -0500 Received: from /spool/local by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 23 Dec 2013 16:12:55 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 616003578052 for ; Mon, 23 Dec 2013 17:12:53 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id rBN6CeqI7995822 for ; Mon, 23 Dec 2013 17:12:40 +1100 Received: from d23av03.au.ibm.com (localhost [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id rBN6Cq81003976 for ; Mon, 23 Dec 2013 17:12:52 +1100 Message-ID: <52B7D468.4060307@linux.vnet.ibm.com> Date: Mon, 23 Dec 2013 14:12:56 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1386244972-528-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1386244972-528-5-git-send-email-xiawenc@linux.vnet.ibm.com> <20131220142034.GB9296@stefanha-thinkpad.redhat.com> <52B7A697.40302@linux.vnet.ibm.com> In-Reply-To: <52B7A697.40302@linux.vnet.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V7 4/6] qcow2: cancel the modification on fail in qcow2_snapshot_create() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mreitz@redhat.com 于 2013/12/23 10:57, Wenchao Xia 写道: > 于 2013/12/20 22:20, Stefan Hajnoczi 写道: >> On Thu, Dec 05, 2013 at 08:02:50PM +0800, Wenchao Xia wrote: >>> +restore_refcount: >>> + if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset, >>> s->l1_size, -1) >>> + < 0 && errp) { >>> + /* Nothing can be done now, need image check later */ >>> + error_setg(&err, "%s\nqcow2: Error in restoring refcount in >>> snapshot", >>> + error_get_pretty(*errp)); >>> + error_free(*errp); >>> + *errp = NULL; >>> + error_propagate(errp, err); >>> + } >> >> We get here if writing the new snapshot list failed. If >> qcow2_update_snapshot_refcount(..., -1) also fails I think we should >> skip qcow2_free_clusters() below. We don't know the exact state of the >> disk image anymore - better to leak clusters than to have a dangling >> reference. >> > > Make sense, dangling point should be avoid in any case, will fix, > Thanks for reviewing! > >>> +dealloc_cluster: >>> + qcow2_free_clusters(bs, sn->l1_table_offset, >>> + sn->l1_size * sizeof(uint64_t), >>> + QCOW2_DISCARD_ALWAYS); >>> + >>> fail: >>> g_free(sn->id_str); >>> g_free(sn->name); >>> -- >>> 1.7.1 >>> >>> >> > > Hi, Stefan I have reconsidered the roll back process, there is many case we should take care, so it is better to summarize a general rule to do such cancel operations. I suggest: do a series of roll back operations, when one fail, skip following roll back operation. For snapshot create, the create action is: allocate new L1 -> refcount+1 -> allocate sn_list -> update header The mirrored rollback action can be: deallocate L1 <- refcount-1 <- deallocate sn_list <- restore header When fail happens in rollback action, simply stop following ones. If you agree, I'd like to reorganize the patch as above.