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

[Yongji Xie]

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