From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34763) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TA0JP-00014g-Na for qemu-devel@nongnu.org; Fri, 07 Sep 2012 11:17:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TA0JM-0005Sf-5N for qemu-devel@nongnu.org; Fri, 07 Sep 2012 11:17:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:62344) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TA0JL-0005SY-TC for qemu-devel@nongnu.org; Fri, 07 Sep 2012 11:17:36 -0400 Message-ID: <504A1009.5030508@redhat.com> Date: Fri, 07 Sep 2012 11:17:29 -0400 From: Jeff Cody MIME-Version: 1.0 References: <5048A3C5.7000000@redhat.com> <5048BA3C.5030701@redhat.com> <5049CA14.9050902@redhat.com> In-Reply-To: <5049CA14.9050902@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v2 PATCH 1/6] block: add support functions for live commit, to find and delete images. Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: stefanha@gmail.com, supriyak@linux.vnet.ibm.com, eblake@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com On 09/07/2012 06:19 AM, Kevin Wolf wrote: > Am 06.09.2012 16:59, schrieb Jeff Cody: >> On 09/06/2012 09:23 AM, Kevin Wolf wrote: >>> Am 30.08.2012 20:47, schrieb Jeff Cody: >>>> Add bdrv_find_child(), and bdrv_delete_intermediate(). >>>> >>>> bdrv_find_child(): given 'bs' and the active (topmost) BDS of an image chain, >>>> find the image that is the immediate top of 'bs' >>>> >>>> bdrv_delete_intermediate(): >>>> Given 3 BDS (active, top, base), delete images above >>>> base up to and including top, and set base to be the >>>> parent of top's child node. >>>> >>>> E.g., this converts: >>>> >>>> bottom <- base <- intermediate <- top <- active >>>> >>>> to >>>> >>>> bottom <- base <- active >>>> >>>> where top == active is permitted, although active >>>> will not be deleted. >>>> >>>> Signed-off-by: Jeff Cody >>> >>> At first, when just reading the function name, I thought this would >>> actually delete the image file. Of course, it only removes it from the >>> backing file chain, but leaves the image file around. I don't have a >>> good suggestion, but if someone has a better name, I think we should >>> change it. >> >> Hmm, the naming seems consistent with bdrv_delete(), which does not >> actually delete the image files either (and, that is essentially what >> this does... calls bdrv_delete(), on the intermediate images). >> >> However, here are some other name proposals: >> >> * bdrv_disconnect_intermediate() >> * bdrv_drop_intermediate() >> * bdrv_shorten_chain() > > bdrv_drop_intermediate() sounds good to me. > >>> >>>> + >>>> +typedef struct BlkIntermediateStates { >>>> + BlockDriverState *bs; >>>> + QSIMPLEQ_ENTRY(BlkIntermediateStates) entry; >>>> +} BlkIntermediateStates; >>>> + >>>> + >>>> +/* deletes images above 'base' up to and including 'top', and sets the image >>>> + * above 'top' to have base as its backing file. >>>> + * >>>> + * E.g., this will convert the following chain: >>>> + * bottom <- base <- intermediate <- top <- active >>>> + * >>>> + * to >>>> + * >>>> + * bottom <- base <- active >>>> + * >>>> + * It is allowed for bottom==base, in which case it converts: >>>> + * >>>> + * base <- intermediate <- top <- active >>>> + * >>>> + * to >>>> + * >>>> + * base <- active >>>> + * >>>> + * It is also allowed for top==active, except in that case active is not >>>> + * deleted: >>> >>> Hm, makes the interface inconsistent. Shouldn't you be using top == >>> intermediate and it would work without any special casing? >>> >> >> To remain consistent, maybe we should define it as an error if >> top==active, and return error in that case? The caller can be >> responsible for checking for that - if the caller wants to merge down >> the active layer, there are additional steps to be taken anyway. > > Yes, why not. > > And we can always revisit when implementing the additional functionality. > >>>> + /* we could not find the image above 'top', this is an error */ >>>> + goto exit; >>>> + } >>>> + >>>> + /* if the active and top image passed in are the same, then we >>>> + * can't delete the active, so we start one below >>>> + */ >>>> + intermediate = (active == top) ? active->backing_hd : top; >>> >>> Aha. So intermediate is used to undo the special case. Now we're always >>> on the last image to be deleted. >>> >>> This is equivalent to an unconditional new_top_bs->backing_hd. > > How about changing this to use the simpler unconditional version? Sure - since active == top is now an error, there is no reason for the more complicated logic. And at this point, the statement (new_top_bs->backing_hd == top) should always be true. > > Kevin >