Patchwork [2/2] block: Cache total_sectors to reduce bdrv_getlength calls

login
register
mail settings
Submitter Stefan Hajnoczi
Date April 19, 2010, 12:34 p.m.
Message ID <1271680452-24121-2-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/50464/
State New
Headers show

Comments

Stefan Hajnoczi - April 19, 2010, 12:34 p.m.
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 <jan.kiszka@siemens.com> 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 <stefanha@linux.vnet.ibm.com>
---
 block.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)
Kevin Wolf - April 19, 2010, 2:10 p.m.
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 <jan.kiszka@siemens.com> 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 <stefanha@linux.vnet.ibm.com>
> ---
>  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
Stefan Hajnoczi - April 19, 2010, 2:26 p.m.
On Mon, Apr 19, 2010 at 3:10 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> @@ -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.

The if statement could be left as is.  I removed it to reduce the
number of places where if (drv->bdrv_getlength) is explicitly checked.
 If callers don't know the internals of bdrv_getlength() then it is
easier to extend it without auditing and changing callers.

Having said that, I did add an if (drv->bdrv_getlength) check into
bdrv_truncate...

> 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.

You're right.

I'll clean this up and send a v2.

Stefan
Kevin Wolf - April 19, 2010, 2:31 p.m.
Am 19.04.2010 16:26, schrieb Stefan Hajnoczi:
> On Mon, Apr 19, 2010 at 3:10 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> @@ -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.
> 
> The if statement could be left as is.  I removed it to reduce the
> number of places where if (drv->bdrv_getlength) is explicitly checked.
>  If callers don't know the internals of bdrv_getlength() then it is
> easier to extend it without auditing and changing callers.

Makes sense, I'm not opposed to it.

> Having said that, I did add an if (drv->bdrv_getlength) check into
> bdrv_truncate...

Well, you probably can't do much about it there.

Kevin

Patch

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;
 #ifndef _WIN32
     if (bs->is_temporary) {
         unlink(filename);
@@ -959,13 +958,26 @@  int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
 int bdrv_truncate(BlockDriverState *bs, int64_t offset)
 {
     BlockDriver *drv = bs->drv;
+    int ret;
     if (!drv)
         return -ENOMEDIUM;
     if (!drv->bdrv_truncate)
         return -ENOTSUP;
     if (bs->read_only)
         return -EACCES;
-    return drv->bdrv_truncate(bs, offset);
+    ret = drv->bdrv_truncate(bs, offset);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /* refresh total sectors */
+    if (drv->bdrv_getlength) {
+        bs->total_sectors = 0; /* discard cached value */
+        bs->total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
+    } else {
+        bs->total_sectors = offset >> BDRV_SECTOR_BITS;
+    }
+    return ret;
 }
 
 /**
@@ -976,8 +988,12 @@  int64_t bdrv_getlength(BlockDriverState *bs)
     BlockDriver *drv = bs->drv;
     if (!drv)
         return -ENOMEDIUM;
-    if (!drv->bdrv_getlength) {
-        /* legacy mode */
+
+    /* Fixed size devices use the total_sectors value for speed instead of
+       issuing a length query (like lseek) on each call.  Also, legacy block
+       drivers don't provide a bdrv_getlength function and must use
+       total_sectors. */
+    if ((bs->total_sectors && !bs->growable) || !drv->bdrv_getlength) {
         return bs->total_sectors * BDRV_SECTOR_SIZE;
     }
     return drv->bdrv_getlength(bs);