[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/5] block/vpc: simplify vpc_read
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 2/5] block/vpc: simplify vpc_read |
Date: |
Mon, 23 Feb 2015 13:29:11 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
On 2015-02-23 at 09:27, Peter Lieven wrote:
Signed-off-by: Peter Lieven <address@hidden>
---
block/vpc.c | 116 +++++++++++++++++++++++++++--------------------------------
1 file changed, 52 insertions(+), 64 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index 326c2bb..4e5ba85 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -497,40 +497,70 @@ static int vpc_get_info(BlockDriverState *bs,
BlockDriverInfo *bdi)
return 0;
}
-static int vpc_read(BlockDriverState *bs, int64_t sector_num,
- uint8_t *buf, int nb_sectors)
+static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
+ int64_t sector_num, int nb_sectors, int *pnum)
How about just putting the function header here? If you really have to
move vpc_co_get_block_status() up, I'd rather like it to be in a
separate patch.
Second, while apparently vpc_read() is actually called in a coroutine,
that is pretty hard to know. Most importantly, it's not marked as a
coroutine_fn. Therefore I don't think it's a good idea to call
vpc_co_get_block_status() directly; I'd vote for either using
bdrv_get_block_status(), or moving the content of
vpc_co_get_block_status() to a non-coroutine_fn (it doesn't contain an
coroutine-related function calls, so this is fine) and then making
vpc_co_get_block_status() a wrapper around that, or just dropping this
patch.
The latter I'm proposing because I don't really see what this patch
improves. The previous vpc_read() function was pretty straightforward,
too, and I don't think it was unbearably longer.
One could argue that the coroutine_fn stuff doesn't really matter in
this situation because it doesn't actually do anything right now and
vpc_co_get_block_status() does not call any other coroutine_fn functions
in turn; however, it is a semantic contract established by
include/block/coroutine.h and as far as I remember, Stefan did
eventually want to have something to error out on compile-time if a
non-coroutine_fn function calls a coroutine_fn. I don't like breaking
this contract even if it's not bad in this specific case.
Considering you probably think bdrv_get_block_status() to be too much
overhead (it will fall down to the protocol layer on VHD_FIXED) and you
probably find making vpc_read() shorter justified (again, which I don't
necessarily), I think moving the contents of vpc_co_get_block_status()
to a non-coroutine_fn might be the best way to go.
{
BDRVVPCState *s = bs->opaque;
- int ret;
- int64_t offset;
- int64_t sectors, sectors_per_block;
- VHDFooter *footer = (VHDFooter *) s->footer_buf;
+ VHDFooter *footer = (VHDFooter*) s->footer_buf;
+ int64_t start, offset;
+ bool allocated;
+ int n;
if (be32_to_cpu(footer->type) == VHD_FIXED) {
- return bdrv_read(bs->file, sector_num, buf, nb_sectors);
+ *pnum = nb_sectors;
+ return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+ (sector_num << BDRV_SECTOR_BITS);
}
- while (nb_sectors > 0) {
- offset = get_sector_offset(bs, sector_num, 0);
- sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
- sectors = sectors_per_block - (sector_num % sectors_per_block);
- if (sectors > nb_sectors) {
- sectors = nb_sectors;
+ offset = get_sector_offset(bs, sector_num, 0);
+ start = offset;
+ allocated = (offset != -1);
+ *pnum = 0;
+
+ do {
+ /* All sectors in a block are contiguous (without using the bitmap) */
+ n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
+ - sector_num;
+ n = MIN(n, nb_sectors);
+
+ *pnum += n;
+ sector_num += n;
+ nb_sectors -= n;
+
+ if (allocated) {
+ return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
}
+ if (nb_sectors == 0) {
+ break;
+ }
+ offset = get_sector_offset(bs, sector_num, 0);
+ } while (offset == -1);
- if (offset == -1) {
- memset(buf, 0, sectors * BDRV_SECTOR_SIZE);
- } else {
- ret = bdrv_pread(bs->file, offset, buf,
- sectors * BDRV_SECTOR_SIZE);
- if (ret != sectors * BDRV_SECTOR_SIZE) {
+ return 0;
+}
+
+static int vpc_read(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors)
+{
+ int ret, n;
+ int64_t ret2;
+
+ while (nb_sectors > 0) {
+ ret2 = vpc_co_get_block_status(bs, sector_num, nb_sectors, &n);
+
Superfluous whitespace here.
+ if (ret2 & BDRV_BLOCK_OFFSET_VALID) {
+ ret = bdrv_pread(bs->file, ret2 & BDRV_SECTOR_MASK, buf,
+ n * BDRV_SECTOR_SIZE);
+ if (ret != n * BDRV_SECTOR_SIZE) {
return -1;
Please make that "return ret" (and possibly "if (ret < 0)", if you want to).
Max
}
+ } else {
+ memset(buf, 0x00, n * BDRV_SECTOR_SIZE);
}
- nb_sectors -= sectors;
- sector_num += sectors;
- buf += sectors * BDRV_SECTOR_SIZE;
+ sector_num += n;
+ nb_sectors -= n;
+ buf += n * BDRV_SECTOR_SIZE;
}
return 0;
}
@@ -597,48 +627,6 @@ static coroutine_fn int vpc_co_write(BlockDriverState *bs,
int64_t sector_num,
return ret;
}
-static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors, int *pnum)
-{
- BDRVVPCState *s = bs->opaque;
- VHDFooter *footer = (VHDFooter*) s->footer_buf;
- int64_t start, offset;
- bool allocated;
- int n;
-
- if (be32_to_cpu(footer->type) == VHD_FIXED) {
- *pnum = nb_sectors;
- return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
- (sector_num << BDRV_SECTOR_BITS);
- }
-
- offset = get_sector_offset(bs, sector_num, 0);
- start = offset;
- allocated = (offset != -1);
- *pnum = 0;
-
- do {
- /* All sectors in a block are contiguous (without using the bitmap) */
- n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
- - sector_num;
- n = MIN(n, nb_sectors);
-
- *pnum += n;
- sector_num += n;
- nb_sectors -= n;
-
- if (allocated) {
- return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
- }
- if (nb_sectors == 0) {
- break;
- }
- offset = get_sector_offset(bs, sector_num, 0);
- } while (offset == -1);
-
- return 0;
-}
-
/*
* Calculates the number of cylinders, heads and sectors per cylinder
* based on a given number of sectors. This is the algorithm described
[Qemu-devel] [PATCH 3/5] vpc: Ignore geometry for large images, Peter Lieven, 2015/02/23