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

[Thomas Huth]

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?

 Thomas


> [1](https://github.com/oasis-tcs/virtio-spec/blob/master/content.tex#L3944)
> 
> 
> Thanks,
> Stefano
>