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 <ebl...@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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
