[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test
From: |
Thomas Huth |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command |
Date: |
Fri, 25 Jan 2019 13:48:26 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
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
- [Qemu-block] [PATCH RFC 0/2] virtio-blk: add DISCARD and WRITE ZEROES features, Stefano Garzarella, 2019/01/24
- [Qemu-block] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Stefano Garzarella, 2019/01/24
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Thomas Huth, 2019/01/25
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Thomas Huth, 2019/01/25
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Thomas Huth, 2019/01/25
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Stefano Garzarella, 2019/01/25
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Thomas Huth, 2019/01/25
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Liu, Changpeng, 2019/01/25
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command,
Thomas Huth <=
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Michael S. Tsirkin, 2019/01/25
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Stefan Hajnoczi, 2019/01/25
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Michael S. Tsirkin, 2019/01/25
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Stefan Hajnoczi, 2019/01/27
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Michael S. Tsirkin, 2019/01/27
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Stefan Hajnoczi, 2019/01/30
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Liu, Changpeng, 2019/01/30
- Re: [Qemu-block] [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command, Michael S. Tsirkin, 2019/01/25
[Qemu-block] [PATCH RFC 1/2] virtio-blk: add DISCARD and WRITE ZEROES features, Stefano Garzarella, 2019/01/24