Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command

[Michael S. Tsirkin]

On Fri, Jan 25, 2019 at 01:48:26PM +0100, Thomas Huth wrote:
> On 2019-01-25 12:58, Liu, Changpeng wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: Thomas Huth [mailto:address@hidden
> >> Sent: Friday, January 25, 2019 4:49 PM
> >> To: Stefano Garzarella <address@hidden>; Michael S. Tsirkin
> >> <address@hidden>; Liu, Changpeng <address@hidden>
> >> Cc: address@hidden; Laurent Vivier <address@hidden>; Kevin Wolf
> >> <address@hidden>; address@hidden; Max Reitz
> >> <address@hidden>; Stefan Hajnoczi <address@hidden>; Paolo Bonzini
> >> <address@hidden>
> >> Subject: Re: [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for
> >> WRITE_ZEROES command
> >>
> >> On 2019-01-25 09:16, Stefano Garzarella wrote:
> >>> On Fri, Jan 25, 2019 at 07:07:35AM +0100, Thomas Huth wrote:
> >>>> On 2019-01-25 07:01, Thomas Huth wrote:
> >>>>> On 2019-01-24 18:23, Stefano Garzarella wrote:
> >>>>>> If the WRITE_ZEROES feature is enabled, we check this
> >>>>>> command in the test_basic().
> >>>>>>
> >>>>>> Signed-off-by: Stefano Garzarella <address@hidden>
> >>>>>> ---
> >>>>>>  tests/virtio-blk-test.c | 63 +++++++++++++++++++++++++++++++++++++++++
> >>>>>>  1 file changed, 63 insertions(+)
> >>>>>>
> >>>>>> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> >>>>>> index 04c608764b..8cabbcb85a 100644
> >>>>>> --- a/tests/virtio-blk-test.c
> >>>>>> +++ b/tests/virtio-blk-test.c
> >>>>>> @@ -231,6 +231,69 @@ static void test_basic(QVirtioDevice *dev,
> >> QGuestAllocator *alloc,
> >>>>>>
> >>>>>>      guest_free(alloc, req_addr);
> >>>>>>
> >>>>>> +    if (features & (1u << VIRTIO_BLK_F_WRITE_ZEROES)) {
> >>>>>> +        struct virtio_blk_discard_write_zeroes *dwz_hdr;
> >>>>>> +        void *expected;
> >>>>>> +
> >>>>>> +        /*
> >>>>>> +         * WRITE_ZEROES request on the same sector of previous test 
> >>>>>> where
> >>>>>> +         * we wrote "TEST".
> >>>>>> +         */
> >>>>>> +        req.type = VIRTIO_BLK_T_WRITE_ZEROES;
> >>>>>> +        req.data = g_malloc0(512);
> >>>>>
> >>>>> Wouldn't it be more interesting to do a memset(req.data, 0xaa, 512) or
> >>>>> something similar here, to see whether zeroes or 0xaa is written?
> >>>>
> >>>> Ah, never mind, I thought req.data would be a sector buffer here, but
> >>>> looking at the lines below, it apparently is something different.
> >>>>
> >>>> Why do you allocate 512 bytes here? I'd rather expect
> >>>> g_malloc0(sizeof(struct virtio_blk_discard_write_zeroes)) here. ... and
> >>>> then you could also use a local "struct virtio_blk_discard_write_zeroes
> >>>> dwz_hdr" variable instead of a pointer, and drop the g_malloc0() 
> >>>> completely?
> >>>>
> >>>
> >>> Hi Thomas,
> >>> it was my initial implementation, but on the first test I discovered
> >>> that virtio_blk_request() has an assert on the data_size and it requires
> >>> a multiple of 512 bytes.
> >>> Then I looked at the virtio-spec #1, and it seems that data should be
> >>> multiple of 512 bytes also if it contains the struct
> >>> virtio_blk_discard_write_zeroes. (I'm not sure)
> >>>
> >>> Anyway I tried to allocate only the space for that struct, commented the
> >>> assert and the test works well.
> >>>
> >>> How do you suggest to proceed?
> >>
> >> Wow, that's a tough question. Looking at the virtio spec, I agree with
> >> you, it looks like struct virtio_blk_discard_write_zeroes should be
> >> padded to 512 bytes here. But when I look at the Linux sources
> >> (drivers/block/virtio_blk.c), I fail to see that they are doing the
> >> padding there (but maybe I'm just too blind).
> >>
> >> Looking at the QEMU sources, it seems like it can deal with both and
> >> always sets the status right behind the last byte:
> >>
> >>     req->in = (void *)in_iov[in_num - 1].iov_base
> >>               + in_iov[in_num - 1].iov_len
> >>               - sizeof(struct virtio_blk_inhdr);
> >>
> >> Anyway, I think the virtio spec should be clearer here to avoid bad
> >> implementations in the future, so maybe Changpeng or Michael could
> >> update the spec here a little bit?
> > The data for Discard and Write Zeroes commands are struct 
> > virtio_blk_discard_write_zeroes
> > aligned, that means you can pass 16 bytes aligned data, based on the 
> > segments number supported,
> > this is also aligned with NVMe specification and  the SCSI specification.
> 
> Ok, thanks, so the "u8 data[][512];" is wrong in the virtio spec in this
> case? See:
> 
>  https://github.com/oasis-tcs/virtio-spec/blob/master/content.tex#L3944
> 
> At least this should be mentioned in the description of the data field,
> I think.
> 
>  Thomas

OK. Is it a multiple of 512 for all other operations?