diff mbox series

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

Message ID abccfcad6764986c8442f2163de618af11232475.1595166227.git.qemu_oss@crudebyte.com (mailing list archive)
State New, archived
Headers show
Series 9pfs: readdir optimization | expand

Commit Message

Christian Schoenebeck July 19, 2020, 1:20 p.m. UTC
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(-)

Comments

Christian Schoenebeck July 28, 2020, 9:46 a.m. UTC | #1
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
diff mbox series

Patch

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);
+    }
 }
 
 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);
+    }
 }
 
 /**