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

[Christian Schoenebeck]

On Mittwoch, 22. Januar 2020 23:59:54 CET Greg Kurz wrote:
> On Tue, 21 Jan 2020 01:17:35 +0100
> 
> Christian Schoenebeck <address@hidden> 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?

> 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.

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

> 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.

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.

Best regards,
Christian Schoenebeck