Re: [Qemu-devel] [PATCHv5 3/6] block/iscsi: set max_transfer_length

[Max Reitz <mreitz@redhat.com>]

On 2014-10-27 at 09:36, Peter Lieven wrote:
> On 27.10.2014 09:28, Max Reitz wrote:
>> On 2014-10-25 at 18:18, Peter Lieven wrote:
>>> Copy the max_xfer_len from the BlockLimits VPD or use the
>>> maximum value fitting in the CDB.
>>>
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>>   block/iscsi.c |   17 +++++++++++++++--
>>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>> index 233f462..1ae4add 100644
>>> --- a/block/iscsi.c
>>> +++ b/block/iscsi.c
>>> @@ -297,6 +297,11 @@ static int64_t sector_lun2qemu(int64_t sector, 
>>> IscsiLun *iscsilun)
>>>       return sector * iscsilun->block_size / BDRV_SECTOR_SIZE;
>>>   }
>>>   +static int nb_sectors_lun2qemu(int64_t sector, IscsiLun *iscsilun)
>>> +{
>>> +    return MIN(sector_lun2qemu(sector, iscsilun), INT_MAX);
>>> +}
>>> +
>>>   static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
>>>   {
>>>       return sector * BDRV_SECTOR_SIZE / iscsilun->block_size;
>>> @@ -1449,10 +1454,18 @@ static void iscsi_close(BlockDriverState *bs)
>>>     static void iscsi_refresh_limits(BlockDriverState *bs, Error 
>>> **errp)
>>>   {
>>> -    IscsiLun *iscsilun = bs->opaque;
>>> -
>>>       /* We don't actually refresh here, but just return data 
>>> queried in
>>>        * iscsi_open(): iscsi targets don't change their limits. */
>>> +
>>> +    IscsiLun *iscsilun = bs->opaque;
>>> +    uint32_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 
>>> 0xffff;
>>> +
>>> +    if (iscsilun->bl.max_xfer_len) {
>>> +        max_xfer_len = MIN(max_xfer_len, iscsilun->bl.max_xfer_len);
>>> +    }
>>> +
>>> +    bs->bl.max_transfer_length = nb_sectors_lun2qemu(max_xfer_len, 
>>> iscsilun);
>>> +
>>>       if (iscsilun->lbp.lbpu) {
>>>           if (iscsilun->bl.max_unmap < 0xffffffff) {
>>>               bs->bl.max_discard = 
>>> sector_lun2qemu(iscsilun->bl.max_unmap,
>>
>> Hm, seems strange to have a function called nb_sectors_lun2qemu() not 
>> only convert values, but also cap the result. But anyway:
>
> Would you give it another name?

Aaah, looking for names... It's always the same horror...

Hm, I just realized why it's nb_sectors, it just wasn't obvious to me 
that nb_sectors is always an int (and I think it might not be to others 
either).

So maybe int_sector_lun2qemu() or even better sector_lun2qemu_int()? I 
always consider everything on the left of the "2" to be the original 
unit and everything on the right to be the result unit, maybe that 
confused me further (so sector_lun2qemu_nb_sectors() is what I'd expect, 
although it's rather long).

But I now see why you called the function that way and it seems fine to 
me. Just leaving it is fine, too. ;-)

Max