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

[Greg Kurz <groug@kaod.org>]

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]);