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
pgpOQsFAJ9akD.pgp
Description: PGP signature
