From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O3rgv-0003ni-M2 for qemu-devel@nongnu.org; Mon, 19 Apr 2010 10:11:13 -0400 Received: from [140.186.70.92] (port=36714 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O3rgu-0003kz-53 for qemu-devel@nongnu.org; Mon, 19 Apr 2010 10:11:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O3rgn-0002QH-AC for qemu-devel@nongnu.org; Mon, 19 Apr 2010 10:11:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43272) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O3rgn-0002Pt-1c for qemu-devel@nongnu.org; Mon, 19 Apr 2010 10:11:05 -0400 Message-ID: <4BCC6453.2020605@redhat.com> Date: Mon, 19 Apr 2010 16:10:27 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1271680452-24121-1-git-send-email-stefanha@linux.vnet.ibm.com> <1271680452-24121-2-git-send-email-stefanha@linux.vnet.ibm.com> In-Reply-To: <1271680452-24121-2-git-send-email-stefanha@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 2/2] block: Cache total_sectors to reduce bdrv_getlength calls List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Jan Kiszka , qemu-devel@nongnu.org, Christoph Hellwig Am 19.04.2010 14:34, schrieb Stefan Hajnoczi: > The BlockDriver bdrv_getlength function is called from the I/O code path > when checking that the request falls within the device. Unfortunately > this involves an lseek system call in the raw protocol; every read or > write request will incur this lseek cost. > > Jan Kiszka identified this issue and its > latency overhead. This patch caches device length in the existing > total_sectors variable so lseek calls can be avoided for fixed size > devices. > > Growable devices fall back to the full bdrv_getlength code path because > I have not added logic to detect extending the size of the device in a > write. > > Signed-off-by: Stefan Hajnoczi > --- > block.c | 28 ++++++++++++++++++++++------ > 1 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/block.c b/block.c > index def3400..d5a3ba7 100644 > --- a/block.c > +++ b/block.c > @@ -363,6 +363,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, > assert(drv != NULL); > > bs->file = NULL; > + bs->total_sectors = 0; > bs->is_temporary = 0; > bs->encrypted = 0; > bs->valid_key = 0; > @@ -416,9 +417,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, > } > > bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR); > - if (drv->bdrv_getlength) { > - bs->total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS; > - } > + bs->total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS; Does this hunk make a difference? If drv->bdrv_getlength == NULL, we'll just get back the current value. But now that you sent this hunk for review, one thing about the existing code: We should probably check the return value of bdrv_getlength. Otherwise both patches look good to me. Kevin