From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59037) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VRO0N-0000i6-NQ for qemu-devel@nongnu.org; Wed, 02 Oct 2013 11:06:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VRO0I-0004VB-3U for qemu-devel@nongnu.org; Wed, 02 Oct 2013 11:06:23 -0400 Received: from mail-bk0-x22b.google.com ([2a00:1450:4008:c01::22b]:41826) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VRO0H-0004V2-Rm for qemu-devel@nongnu.org; Wed, 02 Oct 2013 11:06:18 -0400 Received: by mail-bk0-f43.google.com with SMTP id mz13so391566bkb.2 for ; Wed, 02 Oct 2013 08:06:17 -0700 (PDT) Date: Wed, 2 Oct 2013 17:06:14 +0200 From: Stefan Hajnoczi Message-ID: <20131002150614.GA14662@stefanha-thinkpad.redhat.com> References: <1380723636-18456-1-git-send-email-pl@kamp.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1380723636-18456-1-git-send-email-pl@kamp.de> Subject: Re: [Qemu-devel] [PATCHv4] block/get_block_status: avoid redundant callouts on raw devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven Cc: kwolf@redhat.com, stefanha@redhat.com, qemu-devel@nongnu.org, anthony@codemonkey.ws, pbonzini@redhat.com, ronniesahlberg@gmail.com On Wed, Oct 02, 2013 at 04:20:36PM +0200, Peter Lieven wrote: > if a raw device like an iscsi target or host device is used > the current implementation makes a second call out to get > the block status of bs->file. however, the raw driver already > has called bdrv_get_block_status on bs->file. > > v4: use a flag to detect the raw driver instead of the strncmp > hack. > > Signed-off-by: Peter Lieven > --- > block.c | 4 ++-- > block/raw_bsd.c | 6 +++++- > include/block/block.h | 3 +++ > 3 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/block.c b/block.c > index 93e113a..7fa2e43 100644 > --- a/block.c > +++ b/block.c > @@ -3161,7 +3161,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > > if (bs->file && > (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && > - (ret & BDRV_BLOCK_OFFSET_VALID)) { > + (ret & BDRV_BLOCK_OFFSET_VALID) && !(ret & BDRV_BLOCK_RAW)) { > ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS, > *pnum, pnum); > if (ret2 >= 0) { > @@ -3172,7 +3172,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > } > } > > - return ret; > + return ret & ~BDRV_BLOCK_RAW; > } > > /* Coroutine wrapper for bdrv_get_block_status() */ > diff --git a/block/raw_bsd.c b/block/raw_bsd.c > index d4ace60..a9e0209 100644 > --- a/block/raw_bsd.c > +++ b/block/raw_bsd.c > @@ -62,7 +62,11 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, > int64_t sector_num, > int nb_sectors, int *pnum) > { > - return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum); > + int64_t ret = bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum); > + if (ret < 0) { > + return ret; > + } > + return ret | BDRV_BLOCK_RAW; > } > > static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs, > diff --git a/include/block/block.h b/include/block/block.h > index f808550..cb7019b 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -84,6 +84,8 @@ typedef struct BlockDevOps { > /* BDRV_BLOCK_DATA: data is read from bs->file or another file > * BDRV_BLOCK_ZERO: sectors read as zero > * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data > + * BDRV_BLOCK_RAW: used internally to indicate that the request > + * was piped through the raw driver Sorry I didn't review this earlier but this flag looks hacky and I'm not confident about merging the patch yet. The patch makes me wonder if the raw_bsd driver should avoid calling bs->file itself: return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); Let block.c:bdrv_co_get_block_status() call down into bs->file. The problem is then the protocol cannot report unallocated sectors with this approach. I think we want to preserve bs' offset while taking the other flags from bs->file (DATA, ZERO). Peter, Paolo: What do you think of this approach? Stefan