[PATCH 0/3] 9pfs: Improve unreclaim logic

[PATCH 0/3] 9pfs: Improve unreclaim logic

[Greg Kurz]

clone of "master"

Greg Kurz (3):
  9pfs: Convert V9fsFidState::clunked to bool
  9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ
  9pfs: Improve unreclaim loop

 hw/9pfs/9p.c | 83 +++++++++++++++++++++++++++++-----------------------
 hw/9pfs/9p.h |  6 ++--
 2 files changed, 50 insertions(+), 39 deletions(-)

-- 
2.26.2

[PATCH 1/3] 9pfs: Convert V9fsFidState::clunked to bool

[Greg Kurz]

This can only be 0 or 1.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c | 4 ++--
 hw/9pfs/9p.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 6026b51a1c04..37c3379b7462 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -413,7 +413,7 @@ static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid)
     }
     fidp = *fidpp;
     *fidpp = fidp->next;
-    fidp->clunked = 1;
+    fidp->clunked = true;
     return fidp;
 }
 
@@ -544,7 +544,7 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
 
         /* Clunk fid */
         s->fid_list = fidp->next;
-        fidp->clunked = 1;
+        fidp->clunked = true;
 
         put_fid(pdu, fidp);
     }
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 32df81f360ea..93656323d1d7 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -279,7 +279,7 @@ struct V9fsFidState {
     int open_flags;
     uid_t uid;
     int ref;
-    int clunked;
+    bool clunked;
     V9fsFidState *next;
     V9fsFidState *rclm_lst;
 };
-- 
2.26.2

[PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ

[Greg Kurz]

The fid_list is currently open-coded. This doesn't seem to serve any
purpose that cannot be met with QEMU's generic lists. Let's go for a
QSIMPLEQ : this will allow to add new fids at the end of the list and
to improve the logic in v9fs_mark_fids_unreclaim().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c | 41 ++++++++++++++++++-----------------------
 hw/9pfs/9p.h |  4 ++--
 2 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 37c3379b7462..b65f320e6518 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU *pdu, int32_t fid)
     V9fsFidState *f;
     V9fsState *s = pdu->s;
 
-    for (f = s->fid_list; f; f = f->next) {
+    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
         BUG_ON(f->clunked);
         if (f->fid == fid) {
             /*
@@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
 {
     V9fsFidState *f;
 
-    for (f = s->fid_list; f; f = f->next) {
+    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
         /* If fid is already there return NULL */
         BUG_ON(f->clunked);
         if (f->fid == fid) {
@@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
      * reclaim won't close the file descriptor
      */
     f->flags |= FID_REFERENCED;
-    f->next = s->fid_list;
-    s->fid_list = f;
+    QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next);
 
     v9fs_readdir_init(s->proto_version, &f->fs.dir);
     v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir);
@@ -401,20 +400,16 @@ static int coroutine_fn put_fid(V9fsPDU *pdu, V9fsFidState *fidp)
 
 static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid)
 {
-    V9fsFidState **fidpp, *fidp;
+    V9fsFidState *fidp;
 
-    for (fidpp = &s->fid_list; *fidpp; fidpp = &(*fidpp)->next) {
-        if ((*fidpp)->fid == fid) {
-            break;
+    QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
+        if (fidp->fid == fid) {
+            QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
+            fidp->clunked = true;
+            return fidp;
         }
     }
-    if (*fidpp == NULL) {
-        return NULL;
-    }
-    fidp = *fidpp;
-    *fidpp = fidp->next;
-    fidp->clunked = true;
-    return fidp;
+    return NULL;
 }
 
 void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
@@ -423,7 +418,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
     V9fsState *s = pdu->s;
     V9fsFidState *f, *reclaim_list = NULL;
 
-    for (f = s->fid_list; f; f = f->next) {
+    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
         /*
          * Unlink fids cannot be reclaimed. Check
          * for them and skip them. Also skip fids
@@ -505,7 +500,7 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path)
     V9fsFidState *fidp;
 
 again:
-    for (fidp = s->fid_list; fidp; fidp = fidp->next) {
+    QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
         if (fidp->path.size != path->size) {
             continue;
         }
@@ -537,13 +532,13 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
     V9fsFidState *fidp;
 
     /* Free all fids */
-    while (s->fid_list) {
+    while (!QSIMPLEQ_EMPTY(&s->fid_list)) {
         /* Get fid */
-        fidp = s->fid_list;
+        fidp = QSIMPLEQ_FIRST(&s->fid_list);
         fidp->ref++;
 
         /* Clunk fid */
-        s->fid_list = fidp->next;
+        QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
         fidp->clunked = true;
 
         put_fid(pdu, fidp);
@@ -3121,7 +3116,7 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU *pdu, V9fsFidState *fidp,
      * Fixup fid's pointing to the old name to
      * start pointing to the new name
      */
-    for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) {
+    QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) {
         if (v9fs_path_is_ancestor(&fidp->path, &tfidp->path)) {
             /* replace the name */
             v9fs_fix_path(&tfidp->path, &new_path, strlen(fidp->path.data));
@@ -3215,7 +3210,7 @@ static int coroutine_fn v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir,
      * Fixup fid's pointing to the old name to
      * start pointing to the new name
      */
-    for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) {
+    QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) {
         if (v9fs_path_is_ancestor(&oldpath, &tfidp->path)) {
             /* replace the name */
             v9fs_fix_path(&tfidp->path, &newpath, strlen(oldpath.data));
@@ -4081,7 +4076,7 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
     s->ctx.fmode = fse->fmode;
     s->ctx.dmode = fse->dmode;
 
-    s->fid_list = NULL;
+    QSIMPLEQ_INIT(&s->fid_list);
     qemu_co_rwlock_init(&s->rename_lock);
 
     if (s->ops->init(&s->ctx, errp) < 0) {
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 93656323d1d7..85fb6930b0ca 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -280,7 +280,7 @@ struct V9fsFidState {
     uid_t uid;
     int ref;
     bool clunked;
-    V9fsFidState *next;
+    QSIMPLEQ_ENTRY(V9fsFidState) next;
     V9fsFidState *rclm_lst;
 };
 
@@ -339,7 +339,7 @@ typedef struct {
 struct V9fsState {
     QLIST_HEAD(, V9fsPDU) free_list;
     QLIST_HEAD(, V9fsPDU) active_list;
-    V9fsFidState *fid_list;
+    QSIMPLEQ_HEAD(, V9fsFidState) fid_list;
     FileOperations *ops;
     FsContext ctx;
     char *tag;
-- 
2.26.2

[PATCH 3/3] 9pfs: Improve unreclaim loop

[Greg Kurz]

If a fid was actually re-open by v9fs_reopen_fid(), we re-traverse the
fid list from the head in case some other request created a fid that
needs to be marked unreclaimable as well (ie. the client open a new
handle on the path that is being unlinked). This is a suboptimal since
most if not all fids that require it have likely been taken care of
already.

This is mostly the result of new fids being added to the head of the
list. Since the list is now a QSIMPLEQ, add new fids at the end instead.
Take a reference on the fid to ensure it doesn't go away during
v9fs_reopen_fid() and that it can be safely passed to QSIMPLEQ_NEXT()
afterwards. Since the associated put_fid() can also yield, same is done
with the next fid. So the logic here is to get a reference on a fid and
only put it back during the next iteration after we could get a reference
on the next fid.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index b65f320e6518..b0ab5cf61c1f 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -311,7 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
      * reclaim won't close the file descriptor
      */
     f->flags |= FID_REFERENCED;
-    QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next);
+    QSIMPLEQ_INSERT_TAIL(&s->fid_list, f, next);
 
     v9fs_readdir_init(s->proto_version, &f->fs.dir);
     v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir);
@@ -497,32 +497,48 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path)
 {
     int err;
     V9fsState *s = pdu->s;
-    V9fsFidState *fidp;
+    V9fsFidState *fidp, *fidp_next;
 
-again:
-    QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
-        if (fidp->path.size != path->size) {
-            continue;
-        }
-        if (!memcmp(fidp->path.data, path->data, path->size)) {
+    fidp = QSIMPLEQ_FIRST(&s->fid_list);
+    assert(fidp);
+
+    /*
+     * v9fs_reopen_fid() can yield : a reference on the fid must be held
+     * to ensure its pointer remains valid and we can safely pass it to
+     * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so
+     * we must keep a reference on the next fid as well. So the logic here
+     * is to get a reference on a fid and only put it back during the next
+     * iteration after we could get a reference on the next fid. Start with
+     * the first one.
+     */
+    for (fidp->ref++; fidp; fidp = fidp_next) {
+        if (fidp->path.size == path->size &&
+            !memcmp(fidp->path.data, path->data, path->size)) {
             /* Mark the fid non reclaimable. */
             fidp->flags |= FID_NON_RECLAIMABLE;
 
             /* reopen the file/dir if already closed */
             err = v9fs_reopen_fid(pdu, fidp);
             if (err < 0) {
+                put_fid(pdu, fidp);
                 return err;
             }
+        }
+
+        fidp_next = QSIMPLEQ_NEXT(fidp, next);
+
+        if (fidp_next) {
             /*
-             * Go back to head of fid list because
-             * the list could have got updated when
-             * switched to the worker thread
+             * Ensure the next fid survives a potential clunk request during
+             * put_fid() below and v9fs_reopen_fid() in the next iteration.
              */
-            if (err == 0) {
-                goto again;
-            }
+            fidp_next->ref++;
         }
+
+        /* We're done with this fid */
+        put_fid(pdu, fidp);
     }
+
     return 0;
 }
 
-- 
2.26.2

Re: [PATCH 1/3] 9pfs: Convert V9fsFidState::clunked to bool

[Christian Schoenebeck]

On Montag, 18. Januar 2021 15:22:58 CET Greg Kurz wrote:
> This can only be 0 or 1.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

I still need to go through the other 2 patches. I should be done tomorrow.

> ---
>  hw/9pfs/9p.c | 4 ++--
>  hw/9pfs/9p.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 6026b51a1c04..37c3379b7462 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -413,7 +413,7 @@ static V9fsFidState *clunk_fid(V9fsState *s, int32_t
> fid) }
>      fidp = *fidpp;
>      *fidpp = fidp->next;
> -    fidp->clunked = 1;
> +    fidp->clunked = true;
>      return fidp;
>  }
> 
> @@ -544,7 +544,7 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> 
>          /* Clunk fid */
>          s->fid_list = fidp->next;
> -        fidp->clunked = 1;
> +        fidp->clunked = true;
> 
>          put_fid(pdu, fidp);
>      }
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 32df81f360ea..93656323d1d7 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -279,7 +279,7 @@ struct V9fsFidState {
>      int open_flags;
>      uid_t uid;
>      int ref;
> -    int clunked;
> +    bool clunked;
>      V9fsFidState *next;
>      V9fsFidState *rclm_lst;
>  };

Re: [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ

[Christian Schoenebeck]

On Montag, 18. Januar 2021 15:22:59 CET Greg Kurz wrote:
> The fid_list is currently open-coded. This doesn't seem to serve any
> purpose that cannot be met with QEMU's generic lists. Let's go for a
> QSIMPLEQ : this will allow to add new fids at the end of the list and
> to improve the logic in v9fs_mark_fids_unreclaim().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

In general LGTM hence:

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

Some comments below ...

> ---
>  hw/9pfs/9p.c | 41 ++++++++++++++++++-----------------------
>  hw/9pfs/9p.h |  4 ++--
>  2 files changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 37c3379b7462..b65f320e6518 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU *pdu,
> int32_t fid) V9fsFidState *f;
>      V9fsState *s = pdu->s;
> 
> -    for (f = s->fid_list; f; f = f->next) {
> +    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
>          BUG_ON(f->clunked);
>          if (f->fid == fid) {
>              /*
> @@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> fid) {
>      V9fsFidState *f;
> 
> -    for (f = s->fid_list; f; f = f->next) {
> +    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
>          /* If fid is already there return NULL */
>          BUG_ON(f->clunked);
>          if (f->fid == fid) {
> @@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> fid) * reclaim won't close the file descriptor
>       */
>      f->flags |= FID_REFERENCED;
> -    f->next = s->fid_list;
> -    s->fid_list = f;
> +    QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next);

Not related to this series, but I wonder why queue.h wraps everything into:

do {
	...
} while (0);

No comment about it in git history. Looks like a leftover of its initial 
import from 2009.

>      v9fs_readdir_init(s->proto_version, &f->fs.dir);
>      v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir);
> @@ -401,20 +400,16 @@ static int coroutine_fn put_fid(V9fsPDU *pdu,
> V9fsFidState *fidp)
> 
>  static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid)
>  {
> -    V9fsFidState **fidpp, *fidp;
> +    V9fsFidState *fidp;
> 
> -    for (fidpp = &s->fid_list; *fidpp; fidpp = &(*fidpp)->next) {
> -        if ((*fidpp)->fid == fid) {
> -            break;
> +    QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
> +        if (fidp->fid == fid) {
> +            QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
> +            fidp->clunked = true;
> +            return fidp;
>          }
>      }
> -    if (*fidpp == NULL) {
> -        return NULL;
> -    }
> -    fidp = *fidpp;
> -    *fidpp = fidp->next;
> -    fidp->clunked = true;
> -    return fidp;
> +    return NULL;
>  }

Good cleanup there!

>  void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> @@ -423,7 +418,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>      V9fsState *s = pdu->s;
>      V9fsFidState *f, *reclaim_list = NULL;
> 
> -    for (f = s->fid_list; f; f = f->next) {
> +    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
>          /*
>           * Unlink fids cannot be reclaimed. Check
>           * for them and skip them. Also skip fids
> @@ -505,7 +500,7 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU
> *pdu, V9fsPath *path) V9fsFidState *fidp;
> 
>  again:
> -    for (fidp = s->fid_list; fidp; fidp = fidp->next) {
> +    QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
>          if (fidp->path.size != path->size) {
>              continue;
>          }
> @@ -537,13 +532,13 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
>      V9fsFidState *fidp;
> 
>      /* Free all fids */
> -    while (s->fid_list) {
> +    while (!QSIMPLEQ_EMPTY(&s->fid_list)) {
>          /* Get fid */
> -        fidp = s->fid_list;
> +        fidp = QSIMPLEQ_FIRST(&s->fid_list);
>          fidp->ref++;
> 
>          /* Clunk fid */
> -        s->fid_list = fidp->next;
> +        QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
>          fidp->clunked = true;
> 
>          put_fid(pdu, fidp);
> @@ -3121,7 +3116,7 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU
> *pdu, V9fsFidState *fidp, * Fixup fid's pointing to the old name to
>       * start pointing to the new name
>       */
> -    for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) {
> +    QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) {
>          if (v9fs_path_is_ancestor(&fidp->path, &tfidp->path)) {
>              /* replace the name */
>              v9fs_fix_path(&tfidp->path, &new_path,
> strlen(fidp->path.data)); @@ -3215,7 +3210,7 @@ static int coroutine_fn
> v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir, * Fixup fid's pointing
> to the old name to
>       * start pointing to the new name
>       */
> -    for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) {
> +    QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) {
>          if (v9fs_path_is_ancestor(&oldpath, &tfidp->path)) {
>              /* replace the name */
>              v9fs_fix_path(&tfidp->path, &newpath, strlen(oldpath.data));
> @@ -4081,7 +4076,7 @@ int v9fs_device_realize_common(V9fsState *s, const
> V9fsTransport *t, s->ctx.fmode = fse->fmode;
>      s->ctx.dmode = fse->dmode;
> 
> -    s->fid_list = NULL;
> +    QSIMPLEQ_INIT(&s->fid_list);
>      qemu_co_rwlock_init(&s->rename_lock);
> 
>      if (s->ops->init(&s->ctx, errp) < 0) {
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 93656323d1d7..85fb6930b0ca 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -280,7 +280,7 @@ struct V9fsFidState {
>      uid_t uid;
>      int ref;
>      bool clunked;
> -    V9fsFidState *next;
> +    QSIMPLEQ_ENTRY(V9fsFidState) next;
>      V9fsFidState *rclm_lst;
>  };

Is there a reason for not migrating 'rclm_lst' as well?

Anyway, if you decide to do that as well, then it would IMO be fine with a 
separate patch later on, not worth making it more complicated with a v2 just 
for that.

> @@ -339,7 +339,7 @@ typedef struct {
>  struct V9fsState {
>      QLIST_HEAD(, V9fsPDU) free_list;
>      QLIST_HEAD(, V9fsPDU) active_list;
> -    V9fsFidState *fid_list;
> +    QSIMPLEQ_HEAD(, V9fsFidState) fid_list;

Not an issue for this series, but I wonder whether that macro's first argument 
shouldn't simply be dropped in queue.h, because there are only 2 occurences in 
the entire QEMU code base using it. It looks like nobody really needs it.

>      FileOperations *ops;
>      FsContext ctx;
>      char *tag;

Best regards,
Christian Schoenebeck

Re: [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ

[Greg Kurz]

On Tue, 19 Jan 2021 14:31:26 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 18. Januar 2021 15:22:59 CET Greg Kurz wrote:
> > The fid_list is currently open-coded. This doesn't seem to serve any
> > purpose that cannot be met with QEMU's generic lists. Let's go for a
> > QSIMPLEQ : this will allow to add new fids at the end of the list and
> > to improve the logic in v9fs_mark_fids_unreclaim().
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> In general LGTM hence:
> 
> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> 
> Some comments below ...
> 
> > ---
> >  hw/9pfs/9p.c | 41 ++++++++++++++++++-----------------------
> >  hw/9pfs/9p.h |  4 ++--
> >  2 files changed, 20 insertions(+), 25 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 37c3379b7462..b65f320e6518 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU *pdu,
> > int32_t fid) V9fsFidState *f;
> >      V9fsState *s = pdu->s;
> > 
> > -    for (f = s->fid_list; f; f = f->next) {
> > +    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
> >          BUG_ON(f->clunked);
> >          if (f->fid == fid) {
> >              /*
> > @@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> > fid) {
> >      V9fsFidState *f;
> > 
> > -    for (f = s->fid_list; f; f = f->next) {
> > +    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
> >          /* If fid is already there return NULL */
> >          BUG_ON(f->clunked);
> >          if (f->fid == fid) {
> > @@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> > fid) * reclaim won't close the file descriptor
> >       */
> >      f->flags |= FID_REFERENCED;
> > -    f->next = s->fid_list;
> > -    s->fid_list = f;
> > +    QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next);
> 
> Not related to this series, but I wonder why queue.h wraps everything into:
> 
> do {
> 	...
> } while (0);
> 

Note, this is do { ... } while (0) *without* the trailing semi-colon, which
is the corner stone of this trick.

> No comment about it in git history. Looks like a leftover of its initial 
> import from 2009.
> 

This is the usual way to define multi-statement macros. More details here:

http://c-faq.com/cpp/multistmt.html

> >      v9fs_readdir_init(s->proto_version, &f->fs.dir);
> >      v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir);
> > @@ -401,20 +400,16 @@ static int coroutine_fn put_fid(V9fsPDU *pdu,
> > V9fsFidState *fidp)
> > 
> >  static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid)
> >  {
> > -    V9fsFidState **fidpp, *fidp;
> > +    V9fsFidState *fidp;
> > 
> > -    for (fidpp = &s->fid_list; *fidpp; fidpp = &(*fidpp)->next) {
> > -        if ((*fidpp)->fid == fid) {
> > -            break;
> > +    QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
> > +        if (fidp->fid == fid) {
> > +            QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
> > +            fidp->clunked = true;
> > +            return fidp;
> >          }
> >      }
> > -    if (*fidpp == NULL) {
> > -        return NULL;
> > -    }
> > -    fidp = *fidpp;
> > -    *fidpp = fidp->next;
> > -    fidp->clunked = true;
> > -    return fidp;
> > +    return NULL;
> >  }
> 
> Good cleanup there!
> 

Thanks !

> >  void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> > @@ -423,7 +418,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> >      V9fsState *s = pdu->s;
> >      V9fsFidState *f, *reclaim_list = NULL;
> > 
> > -    for (f = s->fid_list; f; f = f->next) {
> > +    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
> >          /*
> >           * Unlink fids cannot be reclaimed. Check
> >           * for them and skip them. Also skip fids
> > @@ -505,7 +500,7 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU
> > *pdu, V9fsPath *path) V9fsFidState *fidp;
> > 
> >  again:
> > -    for (fidp = s->fid_list; fidp; fidp = fidp->next) {
> > +    QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
> >          if (fidp->path.size != path->size) {
> >              continue;
> >          }
> > @@ -537,13 +532,13 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> >      V9fsFidState *fidp;
> > 
> >      /* Free all fids */
> > -    while (s->fid_list) {
> > +    while (!QSIMPLEQ_EMPTY(&s->fid_list)) {
> >          /* Get fid */
> > -        fidp = s->fid_list;
> > +        fidp = QSIMPLEQ_FIRST(&s->fid_list);
> >          fidp->ref++;
> > 
> >          /* Clunk fid */
> > -        s->fid_list = fidp->next;
> > +        QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
> >          fidp->clunked = true;
> > 
> >          put_fid(pdu, fidp);
> > @@ -3121,7 +3116,7 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU
> > *pdu, V9fsFidState *fidp, * Fixup fid's pointing to the old name to
> >       * start pointing to the new name
> >       */
> > -    for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) {
> > +    QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) {
> >          if (v9fs_path_is_ancestor(&fidp->path, &tfidp->path)) {
> >              /* replace the name */
> >              v9fs_fix_path(&tfidp->path, &new_path,
> > strlen(fidp->path.data)); @@ -3215,7 +3210,7 @@ static int coroutine_fn
> > v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir, * Fixup fid's pointing
> > to the old name to
> >       * start pointing to the new name
> >       */
> > -    for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) {
> > +    QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) {
> >          if (v9fs_path_is_ancestor(&oldpath, &tfidp->path)) {
> >              /* replace the name */
> >              v9fs_fix_path(&tfidp->path, &newpath, strlen(oldpath.data));
> > @@ -4081,7 +4076,7 @@ int v9fs_device_realize_common(V9fsState *s, const
> > V9fsTransport *t, s->ctx.fmode = fse->fmode;
> >      s->ctx.dmode = fse->dmode;
> > 
> > -    s->fid_list = NULL;
> > +    QSIMPLEQ_INIT(&s->fid_list);
> >      qemu_co_rwlock_init(&s->rename_lock);
> > 
> >      if (s->ops->init(&s->ctx, errp) < 0) {
> > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > index 93656323d1d7..85fb6930b0ca 100644
> > --- a/hw/9pfs/9p.h
> > +++ b/hw/9pfs/9p.h
> > @@ -280,7 +280,7 @@ struct V9fsFidState {
> >      uid_t uid;
> >      int ref;
> >      bool clunked;
> > -    V9fsFidState *next;
> > +    QSIMPLEQ_ENTRY(V9fsFidState) next;
> >      V9fsFidState *rclm_lst;
> >  };
> 
> Is there a reason for not migrating 'rclm_lst' as well?
> 

Yeah. I did consider doing so at first but these were
too many changes. Also, the fid list and the reclaim
list have quite different needs, that each deserve its
own patch.

> Anyway, if you decide to do that as well, then it would IMO be fine with a 
> separate patch later on, not worth making it more complicated with a v2 just 
> for that.

Yes, I already have a patch to convert the reclaim list to QSLIST.
I'll post it later as it is merely cosmetic, while this series
changes the behavior.

> > @@ -339,7 +339,7 @@ typedef struct {
> >  struct V9fsState {
> >      QLIST_HEAD(, V9fsPDU) free_list;
> >      QLIST_HEAD(, V9fsPDU) active_list;
> > -    V9fsFidState *fid_list;
> > +    QSIMPLEQ_HEAD(, V9fsFidState) fid_list;
> 
> Not an issue for this series, but I wonder whether that macro's first argument 
> shouldn't simply be dropped in queue.h, because there are only 2 occurences in 
> the entire QEMU code base using it. It looks like nobody really needs it.
> 

A similar macro exist for the other list types as well, with some more
users. If we go for such a change, I think this should stay consistent
accros list types. All users should hence be audited first.

> >      FileOperations *ops;
> >      FsContext ctx;
> >      char *tag;
> 
> Best regards,
> Christian Schoenebeck
> 
> 

Re: [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ

[Christian Schoenebeck]

On Dienstag, 19. Januar 2021 15:34:07 CET Greg Kurz wrote:
> On Tue, 19 Jan 2021 14:31:26 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Montag, 18. Januar 2021 15:22:59 CET Greg Kurz wrote:
> > > The fid_list is currently open-coded. This doesn't seem to serve any
> > > purpose that cannot be met with QEMU's generic lists. Let's go for a
> > > QSIMPLEQ : this will allow to add new fids at the end of the list and
> > > to improve the logic in v9fs_mark_fids_unreclaim().
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > 
> > In general LGTM hence:
> > 
> > Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > 
> > Some comments below ...
> > 
> > > ---
> > > 
> > >  hw/9pfs/9p.c | 41 ++++++++++++++++++-----------------------
> > >  hw/9pfs/9p.h |  4 ++--
> > >  2 files changed, 20 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 37c3379b7462..b65f320e6518 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU
> > > *pdu,
> > > int32_t fid) V9fsFidState *f;
> > > 
> > >      V9fsState *s = pdu->s;
> > > 
> > > -    for (f = s->fid_list; f; f = f->next) {
> > > +    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
> > > 
> > >          BUG_ON(f->clunked);
> > >          if (f->fid == fid) {
> > >          
> > >              /*
> > > 
> > > @@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> > > fid) {
> > > 
> > >      V9fsFidState *f;
> > > 
> > > -    for (f = s->fid_list; f; f = f->next) {
> > > +    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
> > > 
> > >          /* If fid is already there return NULL */
> > >          BUG_ON(f->clunked);
> > >          if (f->fid == fid) {
> > > 
> > > @@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> > > fid) * reclaim won't close the file descriptor
> > > 
> > >       */
> > >      
> > >      f->flags |= FID_REFERENCED;
> > > 
> > > -    f->next = s->fid_list;
> > > -    s->fid_list = f;
> > > +    QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next);
> > 
> > Not related to this series, but I wonder why queue.h wraps everything
> > into:
> > 
> > do {
> > 
> > 	...
> > 
> > } while (0);
> 
> Note, this is do { ... } while (0) *without* the trailing semi-colon, which
> is the corner stone of this trick.

Yes, I got that. What I meant was, unless I am missing something, a simple 
compound statement (without trailing semi-colon) a.k.a. a 'block' is the more 
common way to handle this, like:

#define QSIMPLEQ_INIT(head) {                                        \
    (head)->sqh_first = NULL;                                        \
    (head)->sqh_last = &(head)->sqh_first;                           \
}

Third patch tomorrow ... thanks Greg!

Best regards,
Christian Schoenebeck

Re: [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ

[Greg Kurz]

On Tue, 19 Jan 2021 16:28:01 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Dienstag, 19. Januar 2021 15:34:07 CET Greg Kurz wrote:
> > On Tue, 19 Jan 2021 14:31:26 +0100
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > On Montag, 18. Januar 2021 15:22:59 CET Greg Kurz wrote:
> > > > The fid_list is currently open-coded. This doesn't seem to serve any
> > > > purpose that cannot be met with QEMU's generic lists. Let's go for a
> > > > QSIMPLEQ : this will allow to add new fids at the end of the list and
> > > > to improve the logic in v9fs_mark_fids_unreclaim().
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > 
> > > In general LGTM hence:
> > > 
> > > Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > 
> > > Some comments below ...
> > > 
> > > > ---
> > > > 
> > > >  hw/9pfs/9p.c | 41 ++++++++++++++++++-----------------------
> > > >  hw/9pfs/9p.h |  4 ++--
> > > >  2 files changed, 20 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > index 37c3379b7462..b65f320e6518 100644
> > > > --- a/hw/9pfs/9p.c
> > > > +++ b/hw/9pfs/9p.c
> > > > @@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU
> > > > *pdu,
> > > > int32_t fid) V9fsFidState *f;
> > > > 
> > > >      V9fsState *s = pdu->s;
> > > > 
> > > > -    for (f = s->fid_list; f; f = f->next) {
> > > > +    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
> > > > 
> > > >          BUG_ON(f->clunked);
> > > >          if (f->fid == fid) {
> > > >          
> > > >              /*
> > > > 
> > > > @@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> > > > fid) {
> > > > 
> > > >      V9fsFidState *f;
> > > > 
> > > > -    for (f = s->fid_list; f; f = f->next) {
> > > > +    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
> > > > 
> > > >          /* If fid is already there return NULL */
> > > >          BUG_ON(f->clunked);
> > > >          if (f->fid == fid) {
> > > > 
> > > > @@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> > > > fid) * reclaim won't close the file descriptor
> > > > 
> > > >       */
> > > >      
> > > >      f->flags |= FID_REFERENCED;
> > > > 
> > > > -    f->next = s->fid_list;
> > > > -    s->fid_list = f;
> > > > +    QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next);
> > > 
> > > Not related to this series, but I wonder why queue.h wraps everything
> > > into:
> > > 
> > > do {
> > > 
> > > 	...
> > > 
> > > } while (0);
> > 
> > Note, this is do { ... } while (0) *without* the trailing semi-colon, which
> > is the corner stone of this trick.
> 
> Yes, I got that. What I meant was, unless I am missing something, a simple 
> compound statement (without trailing semi-colon) a.k.a. a 'block' is the more 
> common way to handle this, like:
> 
> #define QSIMPLEQ_INIT(head) {                                        \
>     (head)->sqh_first = NULL;                                        \
>     (head)->sqh_last = &(head)->sqh_first;                           \
> }
> 

No this isn't the more common way because it doesn't work with
if...else statements, i.e.

    if (some_condition)
        QSIMPLEQ_INIT(&head);
    else
	do_some_other_stuff()

has illagal syntax because of the semi-colon.

Of course, since the QEMU coding styles mandates { } in if...else
statements, your suggestion should just work, but do { } while(0)
doesn't do harm (compilers optimize that flawlessly) and it is
so commonly used elsewhere that I don't think its worth changing.

> Third patch tomorrow ... thanks Greg!
> 
> Best regards,
> Christian Schoenebeck
> 
> 

Re: [PATCH 3/3] 9pfs: Improve unreclaim loop

[Christian Schoenebeck]

On Montag, 18. Januar 2021 15:23:00 CET Greg Kurz wrote:
> If a fid was actually re-open by v9fs_reopen_fid(), we re-traverse the

"re-opened"

> fid list from the head in case some other request created a fid that
> needs to be marked unreclaimable as well (ie. the client open a new

"i.e." and either "opens" or "opened"

> handle on the path that is being unlinked). This is a suboptimal since

No "a" here: "This is suboptimal since"

> most if not all fids that require it have likely been taken care of
> already.
> 
> This is mostly the result of new fids being added to the head of the
> list. Since the list is now a QSIMPLEQ, add new fids at the end instead.
> Take a reference on the fid to ensure it doesn't go away during
> v9fs_reopen_fid() and that it can be safely passed to QSIMPLEQ_NEXT()
> afterwards. Since the associated put_fid() can also yield, same is done
> with the next fid. So the logic here is to get a reference on a fid and
> only put it back during the next iteration after we could get a reference
> on the next fid.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p.c | 44 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index b65f320e6518..b0ab5cf61c1f 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -311,7 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> fid) * reclaim won't close the file descriptor
>       */
>      f->flags |= FID_REFERENCED;
> -    QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next);
> +    QSIMPLEQ_INSERT_TAIL(&s->fid_list, f, next);

I wondered whether that behaviour change could have negative side effects, but 
I think the reason why they added it to the head of the list was simply 
because they only had a head pointer (i.e. they would have needed a loop to 
insert to tail).

So yes, I think that change makes sense now with QSIMPLEQ.

> 
>      v9fs_readdir_init(s->proto_version, &f->fs.dir);
>      v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir);
> @@ -497,32 +497,48 @@ static int coroutine_fn
> v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path) {
>      int err;
>      V9fsState *s = pdu->s;
> -    V9fsFidState *fidp;
> +    V9fsFidState *fidp, *fidp_next;
> 
> -again:
> -    QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
> -        if (fidp->path.size != path->size) {
> -            continue;
> -        }
> -        if (!memcmp(fidp->path.data, path->data, path->size)) {
> +    fidp = QSIMPLEQ_FIRST(&s->fid_list);
> +    assert(fidp);

And fidp is under regular circumstances always non-null here? The assumption 
is that there is at least the root fid in the list, which the user should not 
have permission to unlink, right?

> +
> +    /*
> +     * v9fs_reopen_fid() can yield : a reference on the fid must be held
> +     * to ensure its pointer remains valid and we can safely pass it to
> +     * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so
> +     * we must keep a reference on the next fid as well. So the logic here
> +     * is to get a reference on a fid and only put it back during the next
> +     * iteration after we could get a reference on the next fid. Start with
> +     * the first one.
> +     */
> +    for (fidp->ref++; fidp; fidp = fidp_next) {
> +        if (fidp->path.size == path->size &&
> +            !memcmp(fidp->path.data, path->data, path->size)) {
>              /* Mark the fid non reclaimable. */
>              fidp->flags |= FID_NON_RECLAIMABLE;
> 
>              /* reopen the file/dir if already closed */
>              err = v9fs_reopen_fid(pdu, fidp);
>              if (err < 0) {
> +                put_fid(pdu, fidp);
>                  return err;
>              }
> +        }
> +
> +        fidp_next = QSIMPLEQ_NEXT(fidp, next);
> +
> +        if (fidp_next) {
>              /*
> -             * Go back to head of fid list because
> -             * the list could have got updated when
> -             * switched to the worker thread
> +             * Ensure the next fid survives a potential clunk request
> during +             * put_fid() below and v9fs_reopen_fid() in the next
> iteration. */
> -            if (err == 0) {
> -                goto again;
> -            }
> +            fidp_next->ref++;

Mmm, that works as intended if fidp_next matches the requested path. However 
if it is not (like it would in the majority of cases) then the loop breaks 
next and the bumped reference count would never be reverted. Or am I missing 
something here?

>          }
> +
> +        /* We're done with this fid */
> +        put_fid(pdu, fidp);
>      }
> +
>      return 0;
>  }

Best regards,
Christian Schoenebeck

Re: [PATCH 3/3] 9pfs: Improve unreclaim loop

[Greg Kurz]

On Thu, 21 Jan 2021 13:50:37 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 18. Januar 2021 15:23:00 CET Greg Kurz wrote:
> > If a fid was actually re-open by v9fs_reopen_fid(), we re-traverse the
> 
> "re-opened"
> 
> > fid list from the head in case some other request created a fid that
> > needs to be marked unreclaimable as well (ie. the client open a new
> 
> "i.e." and either "opens" or "opened"
> 
> > handle on the path that is being unlinked). This is a suboptimal since
> 
> No "a" here: "This is suboptimal since"
> 

Thanks for the careful reading. I'll fix those :)

> > most if not all fids that require it have likely been taken care of
> > already.
> > 
> > This is mostly the result of new fids being added to the head of the
> > list. Since the list is now a QSIMPLEQ, add new fids at the end instead.
> > Take a reference on the fid to ensure it doesn't go away during
> > v9fs_reopen_fid() and that it can be safely passed to QSIMPLEQ_NEXT()
> > afterwards. Since the associated put_fid() can also yield, same is done
> > with the next fid. So the logic here is to get a reference on a fid and
> > only put it back during the next iteration after we could get a reference
> > on the next fid.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/9pfs/9p.c | 44 ++++++++++++++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index b65f320e6518..b0ab5cf61c1f 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -311,7 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t
> > fid) * reclaim won't close the file descriptor
> >       */
> >      f->flags |= FID_REFERENCED;
> > -    QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next);
> > +    QSIMPLEQ_INSERT_TAIL(&s->fid_list, f, next);
> 
> I wondered whether that behaviour change could have negative side effects, but 
> I think the reason why they added it to the head of the list was simply 
> because they only had a head pointer (i.e. they would have needed a loop to 
> insert to tail).
> 

That's my thinking as well. And the open-code fid list was there from the
start, while reclaim was only added ~1 year later. Before reclaim, adding
to the head was an obvious choice.

> So yes, I think that change makes sense now with QSIMPLEQ.
> 
> > 
> >      v9fs_readdir_init(s->proto_version, &f->fs.dir);
> >      v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir);
> > @@ -497,32 +497,48 @@ static int coroutine_fn
> > v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path) {
> >      int err;
> >      V9fsState *s = pdu->s;
> > -    V9fsFidState *fidp;
> > +    V9fsFidState *fidp, *fidp_next;
> > 
> > -again:
> > -    QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
> > -        if (fidp->path.size != path->size) {
> > -            continue;
> > -        }
> > -        if (!memcmp(fidp->path.data, path->data, path->size)) {
> > +    fidp = QSIMPLEQ_FIRST(&s->fid_list);
> > +    assert(fidp);
> 
> And fidp is under regular circumstances always non-null here? The assumption 
> is that there is at least the root fid in the list, which the user should not 
> have permission to unlink, right?
> 

Oops this is a left-over... The assumption was that the client
didn't clunk all fids at the time v9fs_mark_fids_unreclaim() is
called. This is true with v9fs_remove() which gets a fid from
the list and directly calls v9fs_mark_fids_unreclaim(). This isn't
true though for v9fs_unlinkat() which calls v9fs_co_name_to_path()
in between, and thus could potentially yield and let the client
clunk all fids.

Good catch !

> > +
> > +    /*
> > +     * v9fs_reopen_fid() can yield : a reference on the fid must be held
> > +     * to ensure its pointer remains valid and we can safely pass it to
> > +     * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so
> > +     * we must keep a reference on the next fid as well. So the logic here
> > +     * is to get a reference on a fid and only put it back during the next
> > +     * iteration after we could get a reference on the next fid. Start with
> > +     * the first one.
> > +     */
> > +    for (fidp->ref++; fidp; fidp = fidp_next) {
> > +        if (fidp->path.size == path->size &&
> > +            !memcmp(fidp->path.data, path->data, path->size)) {
> >              /* Mark the fid non reclaimable. */
> >              fidp->flags |= FID_NON_RECLAIMABLE;
> > 
> >              /* reopen the file/dir if already closed */
> >              err = v9fs_reopen_fid(pdu, fidp);
> >              if (err < 0) {
> > +                put_fid(pdu, fidp);
> >                  return err;
> >              }
> > +        }
> > +
> > +        fidp_next = QSIMPLEQ_NEXT(fidp, next);
> > +
> > +        if (fidp_next) {
> >              /*
> > -             * Go back to head of fid list because
> > -             * the list could have got updated when
> > -             * switched to the worker thread
> > +             * Ensure the next fid survives a potential clunk request
> > during +             * put_fid() below and v9fs_reopen_fid() in the next
> > iteration. */
> > -            if (err == 0) {
> > -                goto again;
> > -            }
> > +            fidp_next->ref++;
> 
> Mmm, that works as intended if fidp_next matches the requested path. However 
> if it is not (like it would in the majority of cases) then the loop breaks 
> next and the bumped reference count would never be reverted. Or am I missing 
> something here?
> 

Each iteration of the loop starts with an already referenced fidp.

The loop can only break if:

- v9fs_reopen_fid() fails, in which case put_fid(pdu, fidp) is called
  on the error path above
- end of list is reached, in which case the reference on fidp is
  dropped just like in all previous iterations...

> >          }
> > +
> > +        /* We're done with this fid */
> > +        put_fid(pdu, fidp);

... here.

> >      }
> > +
> >      return 0;
> >  }
> 
> Best regards,
> Christian Schoenebeck
> 
> 

Re: [PATCH 3/3] 9pfs: Improve unreclaim loop

[Christian Schoenebeck]

On Donnerstag, 21. Januar 2021 17:34:55 CET Greg Kurz wrote:
> > > +
> > > +    /*
> > > +     * v9fs_reopen_fid() can yield : a reference on the fid must be
> > > held
> > > +     * to ensure its pointer remains valid and we can safely pass it to
> > > +     * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so
> > > +     * we must keep a reference on the next fid as well. So the logic
> > > here
> > > +     * is to get a reference on a fid and only put it back during the
> > > next
> > > +     * iteration after we could get a reference on the next fid. Start
> > > with +     * the first one.
> > > +     */
> > > +    for (fidp->ref++; fidp; fidp = fidp_next) {
> > > +        if (fidp->path.size == path->size &&
> > > +            !memcmp(fidp->path.data, path->data, path->size)) {
> > > 
> > >              /* Mark the fid non reclaimable. */
> > >              fidp->flags |= FID_NON_RECLAIMABLE;
> > >              
> > >              /* reopen the file/dir if already closed */
> > >              err = v9fs_reopen_fid(pdu, fidp);
> > >              if (err < 0) {
> > > 
> > > +                put_fid(pdu, fidp);
> > > 
> > >                  return err;
> > >              
> > >              }
> > > 
> > > +        }
> > > +
> > > +        fidp_next = QSIMPLEQ_NEXT(fidp, next);
> > > +
> > > +        if (fidp_next) {
> > > 
> > >              /*
> > > 
> > > -             * Go back to head of fid list because
> > > -             * the list could have got updated when
> > > -             * switched to the worker thread
> > > +             * Ensure the next fid survives a potential clunk request
> > > during +             * put_fid() below and v9fs_reopen_fid() in the next
> > > iteration. */
> > > -            if (err == 0) {
> > > -                goto again;
> > > -            }
> > > +            fidp_next->ref++;
> > 
> > Mmm, that works as intended if fidp_next matches the requested path.
> > However if it is not (like it would in the majority of cases) then the
> > loop breaks next and the bumped reference count would never be reverted.
> > Or am I missing something here?
> 
> Each iteration of the loop starts with an already referenced fidp.
> 
> The loop can only break if:
> 
> - v9fs_reopen_fid() fails, in which case put_fid(pdu, fidp) is called
>   on the error path above
> - end of list is reached, in which case the reference on fidp is
>   dropped just like in all previous iterations...

Ah yes, you're right. It's fine!

So just the  assert(fidp);  change then, and the log comment fixes would be 
nice to have. ;-) That's it.

> > >          }
> > > 
> > > +
> > > +        /* We're done with this fid */
> > > +        put_fid(pdu, fidp);
> 
> ... here.
> 
> > >      }
> > > 
> > > +
> > > 
> > >      return 0;
> > >  
> > >  }
> > 
> > Best regards,
> > Christian Schoenebeck

Best regards,
Christian Schoenebeck

Re: [PATCH 0/3] 9pfs: Improve unreclaim logic

[Greg Kurz]

On Mon, 18 Jan 2021 15:22:57 +0100
Greg Kurz <groug@kaod.org> wrote:

> clone of "master"
> 

Drat... this text seems to have leaked from stgit and replaced
the one I had written in git publish:

---

We currently restart the unreclaim loop all over when we detect
that the current iteration yielded execution. This is because
a new fid might have been added to the head of the list.

This is sub-optimal : add new fids to the end of the list instead
to avoid that.

---

I'll have to sort this out...

> Greg Kurz (3):
>   9pfs: Convert V9fsFidState::clunked to bool
>   9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ
>   9pfs: Improve unreclaim loop
> 
>  hw/9pfs/9p.c | 83 +++++++++++++++++++++++++++++-----------------------
>  hw/9pfs/9p.h |  6 ++--
>  2 files changed, 50 insertions(+), 39 deletions(-)
> 

I've applied patches 1 and 2 to:

https://gitlab.com/gkurz/qemu/-/tree/9p-next