From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41702) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S24EF-00032P-Js for qemu-devel@nongnu.org; Mon, 27 Feb 2012 12:19:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S24EE-0002BW-Ce for qemu-devel@nongnu.org; Mon, 27 Feb 2012 12:19:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45693) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S24EE-0002BR-3x for qemu-devel@nongnu.org; Mon, 27 Feb 2012 12:19:14 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q1RHJDqd022768 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 27 Feb 2012 12:19:13 -0500 Message-ID: <4F4BBBE2.1030605@redhat.com> Date: Mon, 27 Feb 2012 18:22:42 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <4F4BA56B.7040608@redhat.com> <4F4BB712.8090005@redhat.com> In-Reply-To: <4F4BB712.8090005@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] qapi: Introduce blockdev-group-snapshot-sync command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: jcody@redhat.com Cc: pbonzini@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, lcapitulino@redhat.com Am 27.02.2012 18:02, schrieb Jeff Cody: >>> + >>> + /* keep the same entry in bdrv_states */ >>> + pstrcpy(tmp.device_name, sizeof(tmp.device_name), bs_top->device_name); >>> + tmp.list = bs_top->list; >>> + >>> + /* swap contents of the fixed new bs and the current top */ >>> + *bs_new = *bs_top; >>> + *bs_top = tmp; >>> + >>> + bdrv_detach_dev(bs_new, bs_new->dev); >>> +} >> >> The last line would actually deserve a comment /* clear the copied >> fields in the new backing file */, which makes clear that there are >> probably some more fields missing in this section. > > OK, added. Only the comment or also clearing other fields? For some of them it's very obvious that they need to be cleared (copy on read, I/O throttling). >>> + } >>> + >>> + /* >>> + * Now we will drain, flush, & pivot everything - we are committed at this >>> + * point. >>> + */ >>> + bdrv_drain_all(); >> >> I would feel more comfortable if we could do the bdrv_drain_all() at the >> very beginning of the function. Not that I know of a specific scenario >> that would go wrong, but you have a nested main loop here that could do >> more or less anything. > > I can move it up to the beginning if desired... My thought was that it > was best to drain closer to the point of commit. As long as we don't create new AIO requests here, drained is drained. But anyway, I'm not requesting a change here, it was just a feeling. >> >>> + QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) { >>> + bdrv_flush(states->old_bs); >> >> This can return an error which must be checked. And of course, we must >> do it before committing to the snapshot (but after bdrv_drain_all). > > Hmm. If the flush returns error, do we abandon at this point? Perhaps it > would be best to loop through and flush each device first, and if no > flushes fail, _then_ loop through and perform the bdrv_append(). Once we > are calling bdrv_append(), we want no possible failure points. Yes, this is what I meant. Sorry for the somewhat vague wording. Kevin