Re: [PATCH v7 5/6] 9pfs: differentiate readdir lock between 9P2000.u vs. 9P2000.L

[Christian Schoenebeck]

On Sonntag, 19. Juli 2020 15:20:11 CEST Christian Schoenebeck wrote:
> Previous patch suggests that it might make sense to use a different mutex
> type now while handling readdir requests, depending on the precise
> protocol variant, as v9fs_do_readdir_with_stat() (used by 9P2000.u) uses
> a CoMutex to avoid deadlocks that might happen with QemuMutex otherwise,
> whereas do_readdir_many() (used by 9P2000.L) should better use a
> QemuMutex, as the precise behaviour of a failed CoMutex lock on fs driver
> side would not be clear.
> 
> This patch is just intended as transitional measure, as currently 9P2000.u
> vs. 9P2000.L implementations currently differ where the main logic of
> fetching directory entries is located at (9P2000.u still being more top
> half focused, while 9P2000.L already being bottom half focused in regards
> to fetching directory entries that is).
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p.c |  4 ++--
>  hw/9pfs/9p.h | 27 ++++++++++++++++++++++-----
>  2 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index cc4094b971..a0881ddc88 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -314,8 +314,8 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> fid) f->next = s->fid_list;
>      s->fid_list = f;
> 
> -    v9fs_readdir_init(&f->fs.dir);
> -    v9fs_readdir_init(&f->fs_reclaim.dir);
> +    v9fs_readdir_init(s->proto_version, &f->fs.dir);
> +    v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir);
> 
>      return f;
>  }
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 93b7030edf..3dd1b50b1a 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -197,22 +197,39 @@ typedef struct V9fsXattr
> 
>  typedef struct V9fsDir {
>      DIR *stream;
> -    CoMutex readdir_mutex;
> +    P9ProtoVersion proto_version;
> +    /* readdir mutex type used for 9P2000.u protocol variant */
> +    CoMutex readdir_mutex_u;
> +    /* readdir mutex type used for 9P2000.L protocol variant */
> +    QemuMutex readdir_mutex_L;
>  } V9fsDir;
> 
>  static inline void v9fs_readdir_lock(V9fsDir *dir)
>  {
> -    qemu_co_mutex_lock(&dir->readdir_mutex);
> +    if (dir->proto_version == V9FS_PROTO_2000U) {
> +        qemu_co_mutex_lock(&dir->readdir_mutex_u);
> +    } else {
> +        qemu_mutex_lock(&dir->readdir_mutex_L);
> +    }
>  }

I wonder whether I should make a minor addition to this patch: returnig an 
error to client if client sends T_readdir while not having opened the session 
as 9P2000.L, and likewise returning an error if client sends T_read on a 
directory while not using a 9P2000.u session. That would prevent the 
thoretical issue of a broken client using the wrong mutex type.

It's maybe overkill, since the plan was actually to bury this patch in git 
history by subsequently removing the lock entirely, but as I am preparing a v8 
anyway, and this is like 2 lines more, so probably not a big deal to add it.

>  static inline void v9fs_readdir_unlock(V9fsDir *dir)
>  {
> -    qemu_co_mutex_unlock(&dir->readdir_mutex);
> +    if (dir->proto_version == V9FS_PROTO_2000U) {
> +        qemu_co_mutex_unlock(&dir->readdir_mutex_u);
> +    } else {
> +        qemu_mutex_unlock(&dir->readdir_mutex_L);
> +    }
>  }
> 
> -static inline void v9fs_readdir_init(V9fsDir *dir)
> +static inline void v9fs_readdir_init(P9ProtoVersion proto_version, V9fsDir
> *dir) {
> -    qemu_co_mutex_init(&dir->readdir_mutex);
> +    dir->proto_version = proto_version;
> +    if (proto_version == V9FS_PROTO_2000U) {
> +        qemu_co_mutex_init(&dir->readdir_mutex_u);
> +    } else {
> +        qemu_mutex_init(&dir->readdir_mutex_L);
> +    }
>  }
> 
>  /**

Best regards,
Christian Schoenebeck