From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:40998) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gmxAs-0003gn-Lt for qemu-devel@nongnu.org; Fri, 25 Jan 2019 03:49:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gmxAr-0007jI-EJ for qemu-devel@nongnu.org; Fri, 25 Jan 2019 03:49:18 -0500 References: <20190124172323.230296-1-sgarzare@redhat.com> <20190124172323.230296-3-sgarzare@redhat.com> <5b3c3076-8f0e-c6a2-b6d1-707db2ae3a66@redhat.com> <962ebaff-23ce-57ff-3387-9017c2ccd14a@redhat.com> <20190125081615.2cevf7cy7bcnxqwh@steredhat> From: Thomas Huth Message-ID: Date: Fri, 25 Jan 2019 09:49:03 +0100 MIME-Version: 1.0 In-Reply-To: <20190125081615.2cevf7cy7bcnxqwh@steredhat> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH RFC 2/2] tests/virtio-blk: add test for WRITE_ZEROES command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Garzarella , "Michael S. Tsirkin" , Changpeng Liu Cc: qemu-devel@nongnu.org, Laurent Vivier , Kevin Wolf , qemu-block@nongnu.org, Max Reitz , Stefan Hajnoczi , Paolo Bonzini 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 >>>> --- >>>> 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, QGue= stAllocator *alloc, >>>> =20 >>>> guest_free(alloc, req_addr); >>>> =20 >>>> + 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 =3D VIRTIO_BLK_T_WRITE_ZEROES; >>>> + req.data =3D g_malloc0(512); >>> >>> Wouldn't it be more interesting to do a memset(req.data, 0xaa, 512) o= r >>> 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. ... an= d >> then you could also use a local "struct virtio_blk_discard_write_zeroe= s >> dwz_hdr" variable instead of a pointer, and drop the g_malloc0() compl= etely? >> >=20 > 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 require= s > 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) >=20 > Anyway I tried to allocate only the space for that struct, commented th= e > assert and the test works well. >=20 > 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 =3D (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#L3= 944) >=20 >=20 > Thanks, > Stefano >=20