Re: [PATCH for-5.2 08/10] block/export: port virtio-blk discard/write zeroes input validation

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Thu, Nov 12, 2020 at 04:25:39PM +0100, Max Reitz wrote:
> On 11.11.20 13:43, Stefan Hajnoczi wrote:
> > @@ -58,30 +60,101 @@ static void vu_blk_req_complete(VuBlkReq *req)
> >       free(req);
> >   }
> > +static bool vu_blk_sect_range_ok(VuBlkExport *vexp, uint64_t sector,
> > +                                 size_t size)
> > +{
> > +    uint64_t nb_sectors = size >> BDRV_SECTOR_BITS;
> > +    uint64_t total_sectors;
> > +
> > +    if (nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
> 
> I wonder why you pass a byte length, then shift it down to sectors, when
> it’s kind of unsafe on the caller’s side to even calculate that byte length
> (because the caller has to verify that shifting the sector count up to be a
> byte length is safe).
> 
> Wouldn’t it be simpler to pass nb_sectors (perhaps even as uint64_t, because
> why not) and then compare that against BDRV_REQUEST_MAX_BYTES here?
> 
> (With how the code is written, it also has the problem of rounding down
> @size.  Which isn’t a problem in practice because the caller effectively
> guarantees that @size is aligned to sectors, but it still means that the
> code looks a bit strange.  As in, “Why is it safe to round down?  Ah,
> because the caller always produces an aligned value.  But why does the
> caller even pass a byte count, then?  Especially given that the offset is
> passed as a sector index, too.”)

Thanks for the suggestions, I'll take a look for the next revision.

> > +        return false;
> > +    }
> > +    if ((sector << BDRV_SECTOR_BITS) % vexp->blk_size) {
> 
> This made me wonder why the discard/write-zeroes sector granularity would be
> BDRV_SECTOR_BITS and not blk_size, which is used for IN/OUT (read/write)
> requests.
> 
> I still don’t know, but judging from the only reference I could find quickly
> (contrib/vhost-user-blk/vhost-user-blk.c), it seems correct.
> 
> OTOH, I wonder whether BDRV_SECTOR_BITS/BDRV_SECTOR_SIZE are the correct
> constants.  Isn’t it just 9/512 as per some specification, i.e., shouldn’t
> it be independent of qemu’s block layer’s sector size?

Yes, a new VIRTIO_BLK_SECTOR_SIZE constant would be clearer. According
to the VIRTIO 1.1 specification:

  blk_size can be read to determine the optimal sector size for the
  driver to use. This does not affect the units used in the protocol
  (always 512 bytes), but awareness of the correct value can affect
  performance.

So all virtio-blk sector fields are in 512-byte units, regardless of the
blk_size config field.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]