On 2019-01-25 12:58, Liu, Changpeng wrote: > > >> -----Original Message----- >> From: Thomas Huth [mailto:th...@redhat.com] >> Sent: Friday, January 25, 2019 4:49 PM >> To: Stefano Garzarella <sgarz...@redhat.com>; Michael S. Tsirkin >> <m...@redhat.com>; Liu, Changpeng <changpeng....@intel.com> >> Cc: qemu-devel@nongnu.org; Laurent Vivier <lviv...@redhat.com>; Kevin Wolf >> <kw...@redhat.com>; qemu-bl...@nongnu.org; Max Reitz >> <mre...@redhat.com>; Stefan Hajnoczi <stefa...@redhat.com>; Paolo Bonzini >> <pbonz...@redhat.com> >> 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 <sgarz...@redhat.com> >>>>>> --- >>>>>> 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
