Re: [Qemu-devel] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status()

[Eric Blake <eblake@redhat.com>]

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 Blake <eblake@redhat.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>

>>       /* 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, &pnum,
>> -                                        &file);
>> +        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, &pnum, 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