From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34365) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S24QH-0001BM-VF for qemu-devel@nongnu.org; Mon, 27 Feb 2012 12:31:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S24QB-0004gG-7u for qemu-devel@nongnu.org; Mon, 27 Feb 2012 12:31:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51874) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S24QA-0004fc-WA for qemu-devel@nongnu.org; Mon, 27 Feb 2012 12:31:35 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q1RHVXEE029069 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 27 Feb 2012 12:31:33 -0500 Message-ID: <4F4BBDF2.4070206@redhat.com> Date: Mon, 27 Feb 2012 12:31:30 -0500 From: Jeff Cody MIME-Version: 1.0 References: <4F4BA56B.7040608@redhat.com> <4F4BB712.8090005@redhat.com> <4F4BBBE2.1030605@redhat.com> In-Reply-To: <4F4BBBE2.1030605@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 Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: pbonzini@redhat.com, lcapitulino@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com On 02/27/2012 12:22 PM, Kevin Wolf wrote: > 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). Sorry; yes to clearing the other fields as well. I don't know if it hurts to leave them not cleared, but logically it makes sense to clear them out. > >>>> + } >>>> + >>>> + /* >>>> + * 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 >