Re: [Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status()
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 BlakeReviewed-by: Fam Zheng /* 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, , -); +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, , 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
Re: [Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status()
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. > > We can also make the simplification of asserting that the block > layer passed in aligned values. > > Signed-off-by: Eric Blake> Reviewed-by: Fam Zheng > > --- > v8: rebase to master > v7: rebase to master > v6: no change > v5: assert rather than check for alignment > v4: rebase to interface tweaks > v3: no change > v2: rebase to mapping parameter > --- > block/iscsi.c | 67 > --- > 1 file changed, 32 insertions(+), 35 deletions(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index d2b0466775c..4842519fdad 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -653,36 +653,36 @@ out_unlock: > > > > -static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs, > - int64_t sector_num, > - int nb_sectors, int *pnum, > - BlockDriverState **file) > +static int coroutine_fn iscsi_co_block_status(BlockDriverState *bs, > + bool want_zero, int64_t offset, > + int64_t bytes, int64_t *pnum, > + int64_t *map, > + BlockDriverState **file) > { > IscsiLun *iscsilun = bs->opaque; > struct scsi_get_lba_status *lbas = NULL; > struct scsi_lba_status_descriptor *lbasd = NULL; > struct IscsiTask iTask; > uint64_t lba; > -int64_t ret; > +int ret; > > iscsi_co_init_iscsitask(iscsilun, ); > > -if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { > -ret = -EINVAL; > -goto out; > -} > +assert(QEMU_IS_ALIGNED(offset | bytes, iscsilun->block_size)); > > /* 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. > +*pnum = bytes; > > /* LUN does not support logical block provisioning */ > if (!iscsilun->lbpme) { > goto out; > } > > -lba = sector_qemu2lun(sector_num, iscsilun); > +lba = offset / iscsilun->block_size; > > qemu_mutex_lock(>mutex); > retry: > @@ -727,12 +727,12 @@ retry: > > lbasd = >descriptors[0]; > > -if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) { > +if (lba != lbasd->lba) { > ret = -EIO; > goto out_unlock; > } > > -*pnum = sector_lun2qemu(lbasd->num_blocks, iscsilun); > +*pnum = lbasd->num_blocks * iscsilun->block_size; > > if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED || > lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) { > @@ -743,15 +743,13 @@ retry: > } > > if (ret & BDRV_BLOCK_ZERO) { > -iscsi_allocmap_set_unallocated(iscsilun, sector_num * > BDRV_SECTOR_SIZE, > - *pnum * BDRV_SECTOR_SIZE); > +iscsi_allocmap_set_unallocated(iscsilun, offset, *pnum); > } else { > -iscsi_allocmap_set_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE, > - *pnum * BDRV_SECTOR_SIZE); > +iscsi_allocmap_set_allocated(iscsilun, offset, *pnum); > } > > -if (*pnum > nb_sectors) { > -*pnum = nb_sectors; > +if (*pnum > bytes) { > +*pnum = bytes; > } > out_unlock: > qemu_mutex_unlock(>mutex); > @@ -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? > 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... :-( > !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
[Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status()
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. We can also make the simplification of asserting that the block layer passed in aligned values. Signed-off-by: Eric BlakeReviewed-by: Fam Zheng --- v8: rebase to master v7: rebase to master v6: no change v5: assert rather than check for alignment v4: rebase to interface tweaks v3: no change v2: rebase to mapping parameter --- block/iscsi.c | 67 --- 1 file changed, 32 insertions(+), 35 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index d2b0466775c..4842519fdad 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -653,36 +653,36 @@ out_unlock: -static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, - int nb_sectors, int *pnum, - BlockDriverState **file) +static int coroutine_fn iscsi_co_block_status(BlockDriverState *bs, + bool want_zero, int64_t offset, + int64_t bytes, int64_t *pnum, + int64_t *map, + BlockDriverState **file) { IscsiLun *iscsilun = bs->opaque; struct scsi_get_lba_status *lbas = NULL; struct scsi_lba_status_descriptor *lbasd = NULL; struct IscsiTask iTask; uint64_t lba; -int64_t ret; +int ret; iscsi_co_init_iscsitask(iscsilun, ); -if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { -ret = -EINVAL; -goto out; -} +assert(QEMU_IS_ALIGNED(offset | bytes, iscsilun->block_size)); /* 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; +} +*pnum = bytes; /* LUN does not support logical block provisioning */ if (!iscsilun->lbpme) { goto out; } -lba = sector_qemu2lun(sector_num, iscsilun); +lba = offset / iscsilun->block_size; qemu_mutex_lock(>mutex); retry: @@ -727,12 +727,12 @@ retry: lbasd = >descriptors[0]; -if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) { +if (lba != lbasd->lba) { ret = -EIO; goto out_unlock; } -*pnum = sector_lun2qemu(lbasd->num_blocks, iscsilun); +*pnum = lbasd->num_blocks * iscsilun->block_size; if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED || lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) { @@ -743,15 +743,13 @@ retry: } if (ret & BDRV_BLOCK_ZERO) { -iscsi_allocmap_set_unallocated(iscsilun, sector_num * BDRV_SECTOR_SIZE, - *pnum * BDRV_SECTOR_SIZE); +iscsi_allocmap_set_unallocated(iscsilun, offset, *pnum); } else { -iscsi_allocmap_set_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE, - *pnum * BDRV_SECTOR_SIZE); +iscsi_allocmap_set_allocated(iscsilun, offset, *pnum); } -if (*pnum > nb_sectors) { -*pnum = nb_sectors; +if (*pnum > bytes) { +*pnum = bytes; } out_unlock: qemu_mutex_unlock(>mutex); @@ -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; } return ret; @@ -800,25 +798,24 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs, nb_sectors * BDRV_SECTOR_SIZE) && !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, , -