From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48696) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPxl0-00044w-Oh for qemu-devel@nongnu.org; Mon, 23 Feb 2015 13:29:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YPxkw-0001hb-Mh for qemu-devel@nongnu.org; Mon, 23 Feb 2015 13:29:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54673) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPxkw-0001hQ-EP for qemu-devel@nongnu.org; Mon, 23 Feb 2015 13:29:22 -0500 Message-ID: <54EB7177.9040801@redhat.com> Date: Mon, 23 Feb 2015 13:29:11 -0500 From: Max Reitz MIME-Version: 1.0 References: <1424701661-21241-1-git-send-email-pl@kamp.de> <1424701661-21241-3-git-send-email-pl@kamp.de> In-Reply-To: <1424701661-21241-3-git-send-email-pl@kamp.de> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/5] block/vpc: simplify vpc_read List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven , qemu-devel@nongnu.org Cc: kwolf@redhat.com, carnold@suse.com, jcody@redhat.com, stefanha@redhat.com On 2015-02-23 at 09:27, Peter Lieven wrote: > Signed-off-by: Peter Lieven > --- > 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