Re: [Qemu-devel] [PATCH] vhost-user-blk: enable discard/write zeroes features

[Liu, Changpeng]

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:address@hidden
> Sent: Monday, January 14, 2019 6:42 PM
> To: Liu, Changpeng <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden
> Subject: Re: [Qemu-devel] [PATCH] vhost-user-blk: enable discard/write zeroes
> features
> 
> On Mon, Jan 14, 2019 at 03:35:17PM +0800, Changpeng Liu wrote:
> > Linux commit 1f23816b8 "virtio_blk: add discard and write zeroes support"
> > added the support in the Guest kernel, while here enable the feature bits
> > support with vhost-user-blk driver.  Also enable the test example utility
> > with DISCARD command support.
> 
> The commit message mentions write zeroes but this patch only covers
> discard.  Will you send a separate patch for write zeros?
Not really, I don't have such plan at first, I can add it in next version.
> 
> > Signed-off-by: Changpeng Liu <address@hidden>
> 
> CCed Stefano, who is working on hw/block/virtio-blk.c emulation support.
> 
> > @@ -157,6 +161,29 @@ vub_writev(VubReq *req, struct iovec *iov, uint32_t
> iovcnt)
> >      return rc;
> >  }
> >
> > +static int
> > +vub_discard(VubReq *req, struct iovec *iov, uint32_t iovcnt)
> > +{
> > +    if (iovcnt != 1) {
> 
> This is a virtio specification violation.  The iovec layout is
> intentionally not part of the specification, leaving the guest driver
> free to choose its preferred layout.
> 
> The device backend must accept any layout, including splitting a struct
> across iovecs or even many small iovecs of just 1 byte!
I see, the original intention here is just using 1 descriptor, which is 16 
bytes.
> 
> > +        fprintf(stderr, "Invalid Discard IOV count\n");
> > +        return -1;
> > +    }
> > +
> > +    #if defined(__linux__) && defined(BLKDISCARD)
> > +    VubDev *vdev_blk = req->vdev_blk;
> > +    struct virtio_blk_discard_write_zeroes *desc =
> > +                       (struct virtio_blk_discard_write_zeroes 
> > *)iov->iov_base;
> 
> Missing input size check.  Even if this example isn't used in
> production, it's important to validate inputs since other people will
> implement their vhost-user backend based on this example.
> 
> Please check that sizeof(*desc) bytes are really available before
> accessing it.
Ok, will fix it.
> 
> > +    case VIRTIO_BLK_T_DISCARD: {
> > +        int rc;
> > +        rc = vub_discard(req, &elem->out_sg[1], out_num);
> > +        if (rc == 0) {
> >              req->in->status = VIRTIO_BLK_S_OK;
> > -            req->size = elem->in_sg[0].iov_len;
> > -            vub_req_complete(req);
> > -            break;
> > -        }
> > -        default: {
> > +        } else {
> >              req->in->status = VIRTIO_BLK_S_UNSUPP;
> 
> Is there no IOERR case?  BLKDISCARD can probably fail due to an I/O
> error and that shouldn't be reported as UNSUPP.
Yes, should include both UNSUPP and ERR status here, will fix it.
> 
> > @@ -454,7 +492,7 @@ vub_get_blocksize(int fd)
> >
> >  #if defined(__linux__) && defined(BLKSSZGET)
> >      if (ioctl(fd, BLKSSZGET, &blocksize) == 0) {
> > -        return blocklen;
> > +        return blocksize;
> >      }
> >  #endif
> 
> Unrelated bug fix?  Please submit a separate patch.
Ok.
> 
> Do you know why the patchew, Travis, etc continuous integration systems
> didn't detect the compile error?  Please ensure that
> contrib/vhost-user-blk/ is covered by CI.
I don't include the header file before, so it will return hardcoded 512 bytes, 
which works for most devices.