|
From: | Eric Blake |
Subject: | Re: [Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status() |
Date: | Wed, 14 Feb 2018 08:33:30 -0600 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
On 02/14/2018 05:53 AM, Kevin Wolf wrote:
Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:We are gradually moving away from sector-based interfaces, towards byte-based. Update the iscsi driver accordingly. In this case, it is handy to teach iscsi_co_block_status() to handle a NULL map and file parameter, even though the block layer passes non-NULL values, because we also call the function directly. For now, there are no optimizations done based on the want_zero flag.
[1]
We can also make the simplification of asserting that the block layer passed in aligned values. Signed-off-by: Eric Blake <address@hidden> Reviewed-by: Fam Zheng <address@hidden>
/* default to all sectors allocated */ - ret = BDRV_BLOCK_DATA; - ret |= (sector_num << BDRV_SECTOR_BITS) | BDRV_BLOCK_OFFSET_VALID; - *pnum = nb_sectors; + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; + if (map) { + *map = offset; + }Can map ever be NULL? You didn't have that check in other drivers.
The documentation in block_int.h states that io.c never passes NULL for map. However, see my commit message [1], and the code below [2], for why THIS driver has to check for NULL.
@@ -760,7 +758,7 @@ out: if (iTask.task != NULL) { scsi_free_scsi_task(iTask.task); } - if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) { + if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID && file) { *file = bs; }Can file ever be NULL?
Ditto.
return ret; @@ -800,25 +798,24 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs, nb_sectors * BDRV_SECTOR_SIZE) &&No iscsi_co_preadv() yet... :-(
Yeah, but that's for another day.
!iscsi_allocmap_is_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE, nb_sectors * BDRV_SECTOR_SIZE)) { - int pnum; - BlockDriverState *file; + int64_t pnum; /* check the block status from the beginning of the cluster * containing the start sector */ - int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS; - int head; - int64_t ret; + int64_t head; + int ret; - assert(cluster_sectors); - head = sector_num % cluster_sectors; - ret = iscsi_co_get_block_status(bs, sector_num - head, - BDRV_REQUEST_MAX_SECTORS, &pnum, - &file); + assert(iscsilun->cluster_size); + head = (sector_num * BDRV_SECTOR_SIZE) % iscsilun->cluster_size; + ret = iscsi_co_block_status(bs, false, + sector_num * BDRV_SECTOR_SIZE - head, + BDRV_REQUEST_MAX_BYTES, &pnum, NULL, NULL);
[2] This is the reason that THIS driver has to check for NULL, even though the block layer never passes NULL.
It doesn't make a difference with your current implementation because it ignores want_zero, but consistent with your approach that want_zero=false returns just that everyhting is allocated for drivers without support for backing files, I think you want want_zero=true here.
Makes sense. If that's the only tweak, can you make it while taking the series, or will I need to respin?
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[Prev in Thread] | Current Thread | [Next in Thread] |