On Thu, 23 Jan 2020 12:36:12 +0100
Christian Schoenebeck <qemu_...@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_...@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
>
>
