Re: [Qemu-devel] [PATCH v6 07/20] scsi-disk: Switch to byte-based aio block access

[Eric Blake <eblake@redhat.com>]

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

On 05/06/2016 06:50 AM, Kevin Wolf wrote:
> Am 05.05.2016 um 01:55 hat Eric Blake geschrieben:
>> Sector-based blk_aio_readv() and blk_aio_writev() should die; switch
>> to byte-based blk_aio_preadv() and blk_aio_pwritev() instead.
>>

>> @@ -343,8 +343,9 @@ static void scsi_do_read(SCSIDiskReq *r, int ret)
>>          n = scsi_init_iovec(r, SCSI_DMA_BUF_SIZE);
>>          block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
>>                           n * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
> 
> If you replace n * BDRV_SECTOR_SIZE here as well, n is unused and can go
> away (you actually did that already for writes).

Sure, I can add that in.


>> @@ -1730,13 +1730,11 @@ static void scsi_write_same_complete(void *opaque, int ret)
>>      if (data->iov.iov_len) {
>>          block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
>>                           data->iov.iov_len, BLOCK_ACCT_WRITE);
>> -        /* blk_aio_write doesn't like the qiov size being different from
>> -         * nb_sectors, make sure they match.
>> -         */
> 
> Wouldn't it be better to update the comment instead of deleting it? If I
> understand correctly, this additional qemu_iovec_init_external() is for
> the last part of an unaligned WRITE SAME request, where the qiov can
> become shorter than in the previous iterations.

The comment mattered when you were passing two sizes to blk_aio_writev
(one embedded in data->qiov, and one in terms of sectors as a direct
argument); the block layer asserted the two were equal.  But now that we
are handling bytes, we pass in a single size (that of data->qiov), so
the comment no longer makes sense.

> 
>>          qemu_iovec_init_external(&data->qiov, &data->iov, 1);
>> -        r->req.aiocb = blk_aio_writev(s->qdev.conf.blk, data->sector,
>> -                                      &data->qiov, data->iov.iov_len / 512,
>> -                                      scsi_write_same_complete, data);
>> +        r->req.aiocb = blk_aio_pwritev(s->qdev.conf.blk,
>> +                                       data->sector << BDRV_SECTOR_BITS,
>> +                                       &data->qiov, 0,

It caught me more than once while writing the patch that my new function
replaced nb_sectors (the duplicated size) by flags, rather than adding a
parameter (as it was harder to get the compiler to gripe about
incomplete conversions, such as forgetting to s/write/pwrite/).  But by
the end of the series, when the old interface is gone, everything still
compiles under the new name, so at least we're assured that the series
worked on that front.

-- 
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 --]