| Message ID | 1271680452-24121-2-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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);
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(-)