Re: [PATCH v5 3/8] block/export: Abstract out the logic of virtio-blk I/O process

From: Yongji Xie <xieyongji@bytedance.com>
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	Jason Wang <jasowang@redhat.com>,
	 Stefano Garzarella <sgarzare@redhat.com>,
	Kevin Wolf <kwolf@redhat.com>,
	mreitz@redhat.com,  mlureau@redhat.com, jsnow@redhat.com,
	Eric Blake <eblake@redhat.com>,
	 Coiby.Xu@gmail.com, hreitz@redhat.com, qemu-block@nongnu.org,
	 qemu-devel@nongnu.org
Subject: Re: [PATCH v5 3/8] block/export: Abstract out the logic of virtio-blk I/O process
Date: Thu, 19 May 2022 16:09:50 +0800	[thread overview]
Message-ID: <CACycT3supsS-KpX6VjiX4SyhV3wrOwPyK-0UX3vPLkbu_PdvWg@mail.gmail.com> (raw)
In-Reply-To: <YoTxEbiG3i0SZx7e@stefanha-x1.localdomain>

On Wed, May 18, 2022 at 9:14 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Wed, May 04, 2022 at 03:40:46PM +0800, Xie Yongji wrote:
> > -static void vu_blk_req_complete(VuBlkReq *req)
> > +static void vu_blk_req_complete(VuBlkReq *req, size_t in_len)
> >  {
> >      VuDev *vu_dev = &req->server->vu_dev;
> >
> > -    /* IO size with 1 extra status byte */
> > -    vu_queue_push(vu_dev, req->vq, &req->elem, req->size + 1);
> > +    vu_queue_push(vu_dev, req->vq, &req->elem, in_len);
>
> I think this silently fixes a bug: now the correct len value is
> calculated. Before the I/O buffer wasn't counted in read requests.

Yes.

> Please mention this in the commit description.
>

OK.

> > +static bool virtio_blk_sect_range_ok(BlockBackend *blk,
> > +                                     uint64_t sector, size_t size)
> > +{
> > +    uint64_t nb_sectors;
> > +    uint64_t total_sectors;
> > +
> > +    if (size % VIRTIO_BLK_SECTOR_SIZE) {
> > +        return false;
> > +    }
> > +
> > +    nb_sectors = size >> VIRTIO_BLK_SECTOR_BITS;
> > +
> > +    QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE != VIRTIO_BLK_SECTOR_SIZE);
> > +    if (nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
> > +        return false;
> > +    }
> > +    if ((sector << VIRTIO_BLK_SECTOR_BITS) % blk_get_guest_block_size(blk)) {
>
> Please use VirtioBlkHandler->logical_block_size instead (see below).
>
> > +int coroutine_fn virtio_blk_process_req(BlockBackend *blk, bool writable,
> > +                                        const char *serial,
>
> I suggest defining a struct instead of passing individual arguments:
>
>   typedef struct {
>       BlockBackend *blk;
>       const char *serial;
>       uint32_t logical_block_size;
>       bool writable;
>   } VirtioBlkHandler;

OK, will do it in v6.

Thanks,
Yongji