From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:55610) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QILAm-0000eh-FO for qemu-devel@nongnu.org; Fri, 06 May 2011 09:34:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QILAh-0001zD-LV for qemu-devel@nongnu.org; Fri, 06 May 2011 09:34:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36359) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QILAh-0001yL-ET for qemu-devel@nongnu.org; Fri, 06 May 2011 09:34:19 -0400 Message-ID: <4DC3F973.7020305@redhat.com> Date: Fri, 06 May 2011 15:36:51 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1303910855-28999-1-git-send-email-stefanha@linux.vnet.ibm.com> <1303910855-28999-2-git-send-email-stefanha@linux.vnet.ibm.com> <4DBAA78B.3060706@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/8] block: add bdrv_aio_stream List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Anthony Liguori , Stefan Hajnoczi , qemu-devel@nongnu.org Am 06.05.2011 15:21, schrieb Stefan Hajnoczi: > On Fri, Apr 29, 2011 at 12:56 PM, Kevin Wolf wrote: >> Am 27.04.2011 15:27, schrieb Stefan Hajnoczi: >>> +/** >>> + * Attempt to stream an image starting from sector_num. >>> + * >>> + * @sector_num - the first sector to start streaming from >>> + * @cb - block completion callback >>> + * @opaque - data to pass completion callback >>> + * >>> + * Returns NULL if the image format not support streaming, the image is >>> + * read-only, or no image is open. >>> + * >>> + * The intention of this function is for a user to execute it once with a >>> + * sector_num of 0 and then upon receiving a completion callback, to remember >>> + * the number of sectors "streamed", and then to call this function again with >>> + * an offset adjusted by the number of sectors previously streamed. >>> + * >>> + * This allows a user to progressive stream in an image at a pace that makes >>> + * sense. In general, this function tries to do the smallest amount of I/O >>> + * possible to do some useful work. >>> + * >>> + * This function only really makes sense in combination with a block format >>> + * that supports copy on read and has it enabled. If copy on read is not >>> + * enabled, a block format driver may return NULL. >>> + */ >>> +BlockDriverAIOCB *bdrv_aio_stream(BlockDriverState *bs, int64_t sector_num, >>> + BlockDriverCompletionFunc *cb, void *opaque) >> >> I think bdrv_aio_stream is a bad name for this. It only becomes image >> streaming because the caller repeatedly calls this function. What the >> function really does is copying some data from the backing file into the >> overlay image. > > That's true but bdrv_aio_copy_from_backing_file() is a bit long. bdrv_copy_backing() or something should be short enough and still describes what it's really doing. > The > special thing about this operation is that it takes a starting > sector_num but no length. The callback receives the nb_sectors. So > this operation isn't an ordinary [start, length) copy either so > bdrv_aio_stream() isn't that bad? Well, you're going to introduce nb_sectors anyway, so it's not really special any more. > I actually think the copy-on-read statement is an implementation > detail. I can imagine doing essentially the same behavior without > exposing copy on read to the user. But in QED streaming is based on > copy-on-read. Let's remove this comment. Ok. Removing the comment and calling it something with "copy" in the name should make clear what the intention is. Kevin