From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45886) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQ9E2-0003IQ-0I for qemu-devel@nongnu.org; Tue, 24 Feb 2015 01:44:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQ9Dy-00050f-Oc for qemu-devel@nongnu.org; Tue, 24 Feb 2015 01:44:09 -0500 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:35072 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQ9Dy-00050Y-Dy for qemu-devel@nongnu.org; Tue, 24 Feb 2015 01:44:06 -0500 Message-ID: <54EC1DB1.6000308@kamp.de> Date: Tue, 24 Feb 2015 07:44:01 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1424701661-21241-1-git-send-email-pl@kamp.de> <1424701661-21241-3-git-send-email-pl@kamp.de> <54EB7177.9040801@redhat.com> In-Reply-To: <54EB7177.9040801@redhat.com> 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: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, carnold@suse.com, jcody@redhat.com, stefanha@redhat.com Am 23.02.2015 um 19:29 schrieb Max Reitz: > 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). I took this from the orignal vpc_read function. Maybe the author also wanted to catch short reads. I tend to drop the whole patch anyway. I was tempted to use that new vpc_co_get_block_status function somehow because I saw that part of its logic is in vpc_read as well. If it should stay maybe it would be an option to inline vpc_read in vpc_co_read (and the same for write)?