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

[Kevin Wolf]

Am 06.05.2016 um 16:18 hat Eric Blake geschrieben:
> On 05/06/2016 06:50 AM, Kevin Wolf wrote:
> > Am 05.05.2016 um 01:55 hat Eric Blake geschrieben:
> >> @@ -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.

Yes, with the current text it doesn't make sense any more. But it
explains why we have another qemu_iovec_init_external() here, and as
that call remains, it would still be good to have an explanation.

Or maybe it's obvious, don't know...

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

Yes, I was a bit worried that I might miss possible cases where you
still pass bytes instead of flags when the compiler can't complain about
them. I didn't see any, so hopefully that means it's good.

Kevin

pgp3IvDWNGgWi.pgp
Description: PGP signature