Re: [PATCH v4 07/11] tests/virtio-9p: failing splitted readdir test

[Greg Kurz <groug@kaod.org>]

On Thu, 23 Jan 2020 12:36:12 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Mittwoch, 22. Januar 2020 23:59:54 CET Greg Kurz wrote:
> > On Tue, 21 Jan 2020 01:17:35 +0100
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > This patch is not intended to be merged. It resembles
> > > an issue (with debug messages) where the splitted
> > > readdir test fails because server is interrupted with
> > > transport error "Failed to decode VirtFS request type 40",
> > 
> > Ok. When we send a new request, we call:
> > 
> > uint32_t qvirtqueue_add(QTestState *qts, QVirtQueue *vq, uint64_t data,
> >                         uint32_t len, bool write, bool next)
> > {
> >     uint16_t flags = 0;
> >     vq->num_free--;
> > 
> > [...]
> > 
> >     return vq->free_head++; /* Return and increase, in this order */
> > }
> 
> Ah, I see!
> 
> > where vq->num_free is the number of available buffers (aka. requests) in
> > the vq and vq->free_head the index of the next available buffer. The size
> > of the vq of the virtio-9p device is MAX_REQ (128) buffers. The driver
> > is very simple and doesn't care to handle the scenario of a full vq,
> > ie, num_free == 0 and free_head is past the vq->desc[] array. It seems
> > that count=128 generates enough extra requests to reach the end of the
> > vq. Hence the "decode" error you get. Maybe an assert(vq->num_free) in
> > qvirtqueue_add() would make that more clear ?
> 
> So just that I get it right; currently the 9pfs test suite writes to a 
> ringbuffer with every request (decreasing the free space in the ringbuffer), 
> but it never frees up that space in the ringbuffer?
> 

Correct.

> > Not sure it is worth to address this limitation though. Especially since
> > count=128 isn't really a recommended choice in the first place. 
> 
> Well, if that's what happens with the ringbuffer, it would need to be 
> addressed somehow anyway, otherwise it would be impossible to add more 9pfs 
> tests, since they would hit the ringbuffer limit as well at a certain point, 
> no matter how simple the requests are.
> 

This just means that a single test shouldn't generate more than
128 requests. I guess this is enough for a variety of tests.

> Wouldn't it make sense to reset the ringbuffer after every succesful, 
> individual 9pfs test?
> 

This is the case, hence my suggestion to pass count to fs_readdir_split()
instead of the having a vcount[] array.

> > It has
> > more chances to cause a disconnect if the server needs to return a longer
> > file name (which is expected since most fs have 255 character long file
> > names).
> 
> Well, this test is dependent on what's provided exactly by the synth driver 
> anyway. So I don't see the value 128 as a problem here. The readdir/split test 
> could even determine the max. length of a file provided by synth driver if you 
> are concerned about that, because the file name template macro 
> QTEST_V9FS_SYNTH_READDIR_FILE used by synth driver is public.
> 

It would make sense to use this knowledge and come up with
a _good_ default value for 'count'.

> And BTW it is not really this specific 'count' value (128) that triggers this 
> issue, if you just run the readdir/split test with i.e.:
> 
> 	const uint32_t vcount[] = { 128 };
> 
> then you won't trigger this test environment issue.
> 

I mean that I don't really care to check small values because
they're likely never used by real clients, and we already know
what we might get in the end: the server disconnects.

> Best regards,
> Christian Schoenebeck
> 
>