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

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",
which BTW fails both with the unoptimized and with the
optimized 9p readdir code.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/virtio-9p-test.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

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 */
}

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 ?

Not sure it is worth to address this limitation though. Especially since
count=128 isn't really a recommended choice in the first place. 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).

> which BTW fails both with the unoptimized and with the
> optimized 9p readdir code.
> 

Yes, this is the client's fault.

> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  tests/qtest/virtio-9p-test.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 8b0d94546e..e47b286340 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -647,13 +647,14 @@ static void fs_readdir_split(void *obj, void *data, QGuestAllocator *t_alloc)
>      int fid;
>      uint64_t offset;
>      /* the Treaddir 'count' parameter values to be tested */
> -    const uint32_t vcount[] = { 512, 256 };
> +    const uint32_t vcount[] = { 512, 256, 128 };
>      const int nvcount = sizeof(vcount) / sizeof(uint32_t);
>  
>      fs_attach(v9p, NULL, t_alloc);
>  
>      /* iterate over all 'count' parameter values to be tested with Treaddir */
>      for (subtest = 0; subtest < nvcount; ++subtest) {
> +        printf("\nsubtest[%d] with count=%d\n", subtest, vcount[subtest]);
>          fid = subtest + 1;
>          offset = 0;
>          entries = NULL;
> @@ -674,12 +675,16 @@ static void fs_readdir_split(void *obj, void *data, QGuestAllocator *t_alloc)
>           * entries
>           */
>          while (true) {
> +            printf("\toffset=%ld\n", offset);
>              npartialentries = 0;
>              partialentries = NULL;
>  
> +            printf("Treaddir fid=%d offset=%ld count=%d\n",
> +                   fid, offset, vcount[subtest]);
>              req = v9fs_treaddir(v9p, fid, offset, vcount[subtest], 0);
>              v9fs_req_wait_for_reply(req, NULL);
>              v9fs_rreaddir(req, &count, &npartialentries, &partialentries);
> +            printf("\t\tnpartial=%d nentries=%d\n", npartialentries, nentries);
>              if (npartialentries > 0 && partialentries) {
>                  if (!entries) {
>                      entries = partialentries;
> @@ -716,6 +721,8 @@ static void fs_readdir_split(void *obj, void *data, QGuestAllocator *t_alloc)
>          }
>  
>          v9fs_free_dirents(entries);
> +
> +        printf("PASSED subtest[%d]\n", subtest);
>      }
>  
>      g_free(wnames[0]);

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?

> 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

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