Re: [Qemu-devel] [PATCH 05/13] iscsi: Convert to bdrv_co_pwrite_zeroes()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2446 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]