Linux Virtualization - Jun 2018 - [PATCH v4] virtio_blk: add DISCARD and WRIET ZEROES commands support

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha at redhat.com]
> Sent: Thursday, May 31, 2018 6:31 PM
> To: Liu, Changpeng <changpeng.liu at intel.com>
> Cc: virtualization at lists.linux-foundation.org; cavery at redhat.com;
> jasowang at redhat.com; pbonzini at redhat.com; Wang, Wei W
> <wei.w.wang at intel.com>
> Subject: Re: [PATCH v4] virtio_blk: add DISCARD and WRIET ZEROES commands
> support
> 
> On Tue, May 29, 2018 at 09:42:24AM +0800, Changpeng Liu wrote:
> >  	num = blk_rq_map_sg(hctx->queue, req, vbr->sg);
> >  	if (num) {
> > -		if (rq_data_dir(req) == WRITE)
> > +		if (rq_data_dir(req) == WRITE || type == VIRTIO_BLK_T_DISCARD
> ||
> > +		    type == VIRTIO_BLK_T_WRITE_ZEROES)
> >  			vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev,
> VIRTIO_BLK_T_OUT);
> 
> The VIRTIO specification says:
> 
>   The type of the request is either a read (VIRTIO_BLK_T_IN), a write
>   (VIRTIO_BLK_T_OUT), a discard (VIRTIO_BLK_T_DISCARD), a write zeroes
>   (VIRTIO_BLK_T_WRITE_ZEROES) or a flush (VIRTIO_BLK_T_FLUSH).
> 
> But this patch actually uses VIRTIO_BLK_T_DISCARD | VIRTIO_BLK_T_OUT or
> VIRTIO_BLK_T_WRITE_ZEROES | VIRTIO_BLK_T_OUT.  "either A or B" is
> exclusive, it does not mean "A and B".
Hi Stefan,

For the new add DISCARD and WRITE ZEROES commands, I defined the 
type code to 11 and 13, so the last bit always is 1, there is no difference
in practice.
But I believe the specification says VIRTIO_BLK_T_OUT means direction, so
OR the two bits together should compliance with the
specification.
> 
> Can you clarify whether the spec needs to be changed or what the purpose
> of ORing in the VIRTIO_BLK_T_OUT bit is?

On Thu, May 31, 2018 at 11:53:26PM +0000, Liu, Changpeng
wrote:
> 
> 
> > -----Original Message-----
> > From: Stefan Hajnoczi [mailto:stefanha at redhat.com]
> > Sent: Thursday, May 31, 2018 6:31 PM
> > To: Liu, Changpeng <changpeng.liu at intel.com>
> > Cc: virtualization at lists.linux-foundation.org; cavery at
redhat.com;
> > jasowang at redhat.com; pbonzini at redhat.com; Wang, Wei W
> > <wei.w.wang at intel.com>
> > Subject: Re: [PATCH v4] virtio_blk: add DISCARD and WRIET ZEROES
commands
> > support
> > 
> > On Tue, May 29, 2018 at 09:42:24AM +0800, Changpeng Liu wrote:
> > >  	num = blk_rq_map_sg(hctx->queue, req, vbr->sg);
> > >  	if (num) {
> > > -		if (rq_data_dir(req) == WRITE)
> > > +		if (rq_data_dir(req) == WRITE || type == VIRTIO_BLK_T_DISCARD
> > ||
> > > +		    type == VIRTIO_BLK_T_WRITE_ZEROES)
> > >  			vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev,
> > VIRTIO_BLK_T_OUT);
> > 
> > The VIRTIO specification says:
> > 
> >   The type of the request is either a read (VIRTIO_BLK_T_IN), a write
> >   (VIRTIO_BLK_T_OUT), a discard (VIRTIO_BLK_T_DISCARD), a write zeroes
> >   (VIRTIO_BLK_T_WRITE_ZEROES) or a flush (VIRTIO_BLK_T_FLUSH).
> > 
> > But this patch actually uses VIRTIO_BLK_T_DISCARD | VIRTIO_BLK_T_OUT
or
> > VIRTIO_BLK_T_WRITE_ZEROES | VIRTIO_BLK_T_OUT.  "either A or
B" is
> > exclusive, it does not mean "A and B".
> Hi Stefan,
> 
> For the new add DISCARD and WRITE ZEROES commands, I defined the 
> type code to 11 and 13, so the last bit always is 1, there is no difference
> in practice.
Then the code is misleading.  This is clearer:

  if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES)
      /* Do nothing, type already set */
  else if (rq_data_dir(req) == WRITE)
      vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_OUT);
  ...
> But I believe the specification says VIRTIO_BLK_T_OUT means direction, so
> OR the two bits together should compliance with the specification.
I cannot find that in the specification:

 
http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-2020002

and it would contradict the "The type of the request is either ... or
..." wording that I quoted from the spec above.

If you do find something in the spec, please let me know and we can
figure out how to make the spec consistent.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20180601/1fb6754f/attachment-0001.sig>

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha at redhat.com]
> Sent: Friday, June 1, 2018 5:00 PM
> To: Liu, Changpeng <changpeng.liu at intel.com>
> Cc: virtualization at lists.linux-foundation.org; cavery at redhat.com;
> jasowang at redhat.com; pbonzini at redhat.com; Wang, Wei W
> <wei.w.wang at intel.com>
> Subject: Re: [PATCH v4] virtio_blk: add DISCARD and WRIET ZEROES commands
> support
> 
> On Thu, May 31, 2018 at 11:53:26PM +0000, Liu, Changpeng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Stefan Hajnoczi [mailto:stefanha at redhat.com]
> > > Sent: Thursday, May 31, 2018 6:31 PM
> > > To: Liu, Changpeng <changpeng.liu at intel.com>
> > > Cc: virtualization at lists.linux-foundation.org; cavery at
redhat.com;
> > > jasowang at redhat.com; pbonzini at redhat.com; Wang, Wei W
> > > <wei.w.wang at intel.com>
> > > Subject: Re: [PATCH v4] virtio_blk: add DISCARD and WRIET ZEROES
commands
> > > support
> > >
> > > On Tue, May 29, 2018 at 09:42:24AM +0800, Changpeng Liu wrote:
> > > >  	num = blk_rq_map_sg(hctx->queue, req, vbr->sg);
> > > >  	if (num) {
> > > > -		if (rq_data_dir(req) == WRITE)
> > > > +		if (rq_data_dir(req) == WRITE || type ==
VIRTIO_BLK_T_DISCARD
> > > ||
> > > > +		    type == VIRTIO_BLK_T_WRITE_ZEROES)
> > > >  			vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev,
> > > VIRTIO_BLK_T_OUT);
> > >
> > > The VIRTIO specification says:
> > >
> > >   The type of the request is either a read (VIRTIO_BLK_T_IN), a
write
> > >   (VIRTIO_BLK_T_OUT), a discard (VIRTIO_BLK_T_DISCARD), a write
zeroes
> > >   (VIRTIO_BLK_T_WRITE_ZEROES) or a flush (VIRTIO_BLK_T_FLUSH).
> > >
> > > But this patch actually uses VIRTIO_BLK_T_DISCARD |
VIRTIO_BLK_T_OUT or
> > > VIRTIO_BLK_T_WRITE_ZEROES | VIRTIO_BLK_T_OUT.  "either A or
B" is
> > > exclusive, it does not mean "A and B".
> > Hi Stefan,
> >
> > For the new add DISCARD and WRITE ZEROES commands, I defined the
> > type code to 11 and 13, so the last bit always is 1, there is no
difference
> > in practice.
> 
> Then the code is misleading.  This is clearer:
> 
>   if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES)
>       /* Do nothing, type already set */
>   else if (rq_data_dir(req) == WRITE)
>       vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev,
VIRTIO_BLK_T_OUT);
>   ...
This change sounds good to me, will change the patch
accordingly.
> 
> > But I believe the specification says VIRTIO_BLK_T_OUT means direction,
so
> > OR the two bits together should compliance with the specification.
> 
> I cannot find that in the specification:
> 
>  
http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-
> 2020002
> 
> and it would contradict the "The type of the request is either ... or
> ..." wording that I quoted from the spec above.
> 
> If you do find something in the spec, please let me know and we can
> figure out how to make the spec consistent.
I saw comments from file linux/usr/include/uapi/linux/virtio_blk.h, which says
VIRTIO_BLK_T_OUT may be combined with other commands and means direction,
the specification does not have such description.