Message ID | 1464128732-12667-6-git-send-email-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Am 25.05.2016 um 00:25 hat Eric Blake geschrieben: > Another step on our continuing quest to switch to byte-based > interfaces. > > As this is the first byte-based iscsi interface, convert > is_request_lun_aligned() into two versions, one for sectors > and one for bytes. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > block/iscsi.c | 53 +++++++++++++++++++++++++++++++---------------------- > 1 file changed, 31 insertions(+), 22 deletions(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index 0acc3dc..3dbfd57 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -401,18 +401,25 @@ static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun) > return sector * BDRV_SECTOR_SIZE / iscsilun->block_size; > } > > -static bool is_request_lun_aligned(int64_t sector_num, int nb_sectors, > - IscsiLun *iscsilun) > +static bool is_byte_request_lun_aligned(int64_t offset, int count, > + IscsiLun *iscsilun) > { > - if ((sector_num * BDRV_SECTOR_SIZE) % iscsilun->block_size || > - (nb_sectors * BDRV_SECTOR_SIZE) % iscsilun->block_size) { > - error_report("iSCSI misaligned request: " > - "iscsilun->block_size %u, sector_num %" PRIi64 > - ", nb_sectors %d", > - iscsilun->block_size, sector_num, nb_sectors); > - return 0; > + if (offset % iscsilun->block_size || count % iscsilun->block_size) { > + error_report("iSCSI misaligned request: " > + "iscsilun->block_size %u, offset %" PRIi64 > + ", count %d", > + iscsilun->block_size, offset, count); > + return false; > } > - return 1; > + return true; > +} > + > +static bool is_sector_request_lun_aligned(int64_t sector_num, int nb_sectors, > + IscsiLun *iscsilun) > +{ > + return is_byte_request_lun_aligned(sector_num << BDRV_SECTOR_BITS, > + nb_sectors << BDRV_SECTOR_BITS, > + iscsilun); > } You're switching from (nb_sectors * BDRV_SECTOR_SIZE) to (nb_sectors << BDRV_SECTOR_BITS). The difference is that the former is a 64 bit calculation because BDRV_SECTOR_BITS is unsigned long long, whereas the latter is a 32 bit calculation. Fortunately, it seems to me that all input values come directly from the block layer which already limits requests to BDRV_REQUEST_MAX_SECTORS. So we should be safe from overflows here. > static int > -coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, > - int nb_sectors, BdrvRequestFlags flags) > +coroutine_fn iscsi_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, > + int count, BdrvRequestFlags flags) > { > IscsiLun *iscsilun = bs->opaque; > struct IscsiTask iTask; > @@ -978,7 +985,7 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, > uint32_t nb_blocks; > bool use_16_for_ws = iscsilun->use_16_for_rw; > > - if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { > + if (!is_byte_request_lun_aligned(offset, count, iscsilun)) { > return -EINVAL; > } Should this become -ENOTSUP so that emulation can take over rather than failing the request? We should probably also always set bs->bl.pwrite_zeroes_alignment, with a fallback to iscsilun->block_size if we don't have iscsilun->lbp.lbpws. But that's a separate patch. Kevin
On 05/25/2016 07:34 AM, Kevin Wolf wrote: > Am 25.05.2016 um 00:25 hat Eric Blake geschrieben: >> Another step on our continuing quest to switch to byte-based >> interfaces. >> >> As this is the first byte-based iscsi interface, convert >> is_request_lun_aligned() into two versions, one for sectors >> and one for bytes. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> block/iscsi.c | 53 +++++++++++++++++++++++++++++++---------------------- >> 1 file changed, 31 insertions(+), 22 deletions(-) >> >> +static bool is_sector_request_lun_aligned(int64_t sector_num, int nb_sectors, >> + IscsiLun *iscsilun) >> +{ >> + return is_byte_request_lun_aligned(sector_num << BDRV_SECTOR_BITS, >> + nb_sectors << BDRV_SECTOR_BITS, >> + iscsilun); >> } > > You're switching from (nb_sectors * BDRV_SECTOR_SIZE) to (nb_sectors << > BDRV_SECTOR_BITS). The difference is that the former is a 64 bit > calculation because BDRV_SECTOR_BITS is unsigned long long, whereas the > latter is a 32 bit calculation. > > Fortunately, it seems to me that all input values come directly from the > block layer which already limits requests to BDRV_REQUEST_MAX_SECTORS. > So we should be safe from overflows here. Still, it won't hurt to add an assert. >> @@ -978,7 +985,7 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, >> uint32_t nb_blocks; >> bool use_16_for_ws = iscsilun->use_16_for_rw; >> >> - if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { >> + if (!is_byte_request_lun_aligned(offset, count, iscsilun)) { >> return -EINVAL; >> } > > Should this become -ENOTSUP so that emulation can take over rather than > failing the request? It's still -EINVAL on unaligned write requests; then again, the block layer guarantees that it will honor bs->request_alignment for write requests, even on RMW for write-zeroes fallbacks. So switching to -ENOTSUP makes sense. > > We should probably also always set bs->bl.pwrite_zeroes_alignment, with > a fallback to iscsilun->block_size if we don't have iscsilun->lbp.lbpws. > But that's a separate patch. Yes, added as a separate patch.
diff --git a/block/iscsi.c b/block/iscsi.c index 0acc3dc..3dbfd57 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -401,18 +401,25 @@ static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun) return sector * BDRV_SECTOR_SIZE / iscsilun->block_size; } -static bool is_request_lun_aligned(int64_t sector_num, int nb_sectors, - IscsiLun *iscsilun) +static bool is_byte_request_lun_aligned(int64_t offset, int count, + IscsiLun *iscsilun) { - if ((sector_num * BDRV_SECTOR_SIZE) % iscsilun->block_size || - (nb_sectors * BDRV_SECTOR_SIZE) % iscsilun->block_size) { - error_report("iSCSI misaligned request: " - "iscsilun->block_size %u, sector_num %" PRIi64 - ", nb_sectors %d", - iscsilun->block_size, sector_num, nb_sectors); - return 0; + if (offset % iscsilun->block_size || count % iscsilun->block_size) { + error_report("iSCSI misaligned request: " + "iscsilun->block_size %u, offset %" PRIi64 + ", count %d", + iscsilun->block_size, offset, count); + return false; } - return 1; + return true; +} + +static bool is_sector_request_lun_aligned(int64_t sector_num, int nb_sectors, + IscsiLun *iscsilun) +{ + return is_byte_request_lun_aligned(sector_num << BDRV_SECTOR_BITS, + nb_sectors << BDRV_SECTOR_BITS, + iscsilun); } static unsigned long *iscsi_allocationmap_init(IscsiLun *iscsilun) @@ -461,7 +468,7 @@ iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors, if (fua) { assert(iscsilun->dpofua); } - if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { + if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { return -EINVAL; } @@ -541,7 +548,7 @@ static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs, iscsi_co_init_iscsitask(iscsilun, &iTask); - if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { + if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { ret = -EINVAL; goto out; } @@ -638,7 +645,7 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs, uint64_t lba; uint32_t num_sectors; - if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { + if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { return -EINVAL; } @@ -918,7 +925,7 @@ coroutine_fn iscsi_co_discard(BlockDriverState *bs, int64_t sector_num, struct IscsiTask iTask; struct unmap_list list; - if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { + if (!is_sector_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { return -EINVAL; } @@ -969,8 +976,8 @@ retry: } static int -coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, - int nb_sectors, BdrvRequestFlags flags) +coroutine_fn iscsi_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, + int count, BdrvRequestFlags flags) { IscsiLun *iscsilun = bs->opaque; struct IscsiTask iTask; @@ -978,7 +985,7 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, uint32_t nb_blocks; bool use_16_for_ws = iscsilun->use_16_for_rw; - if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) { + if (!is_byte_request_lun_aligned(offset, count, iscsilun)) { return -EINVAL; } @@ -1000,8 +1007,8 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, return -ENOTSUP; } - lba = sector_qemu2lun(sector_num, iscsilun); - nb_blocks = sector_qemu2lun(nb_sectors, iscsilun); + lba = offset / iscsilun->block_size; + nb_blocks = count / iscsilun->block_size; if (iscsilun->zeroblock == NULL) { iscsilun->zeroblock = g_try_malloc0(iscsilun->block_size); @@ -1057,9 +1064,11 @@ retry: } if (flags & BDRV_REQ_MAY_UNMAP) { - iscsi_allocationmap_clear(iscsilun, sector_num, nb_sectors); + iscsi_allocationmap_clear(iscsilun, offset >> BDRV_SECTOR_BITS, + count >> BDRV_SECTOR_BITS); } else { - iscsi_allocationmap_set(iscsilun, sector_num, nb_sectors); + iscsi_allocationmap_set(iscsilun, offset >> BDRV_SECTOR_BITS, + count >> BDRV_SECTOR_BITS); } return 0; @@ -1842,7 +1851,7 @@ static BlockDriver bdrv_iscsi = { .bdrv_co_get_block_status = iscsi_co_get_block_status, .bdrv_co_discard = iscsi_co_discard, - .bdrv_co_write_zeroes = iscsi_co_write_zeroes, + .bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes, .bdrv_co_readv = iscsi_co_readv, .bdrv_co_writev_flags = iscsi_co_writev_flags, .bdrv_co_flush_to_disk = iscsi_co_flush,
Another step on our continuing quest to switch to byte-based interfaces. As this is the first byte-based iscsi interface, convert is_request_lun_aligned() into two versions, one for sectors and one for bytes. Signed-off-by: Eric Blake <eblake@redhat.com> --- block/iscsi.c | 53 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 22 deletions(-)