[v2] 9pfs: Improve unreclaim loop

If a fid was actually re-opened 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 opened a new
handle on the path that is being unlinked). 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
to avoid the need to rewind. 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>
---
v2: - fix typos in changelog
    - drop bogus assert()
---
 hw/9pfs/9p.c | 46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

On Donnerstag, 21. Januar 2021 19:15:10 CET Greg Kurz wrote:
> If a fid was actually re-opened 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 opened a new

That's "i.e.". Not a big deal so ...

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

> handle on the path that is being unlinked). 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
> to avoid the need to rewind. 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>
> ---
> v2: - fix typos in changelog
>     - drop bogus assert()
> ---
>  hw/9pfs/9p.c | 46 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index b65f320e6518..3864d014b43c 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,50 @@ 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);
> +    if (!fidp) {
> +        return 0;
> +    }
> +
> +    /*
> +     * 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;
>  }

On Fri, 22 Jan 2021 14:09:12 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Donnerstag, 21. Januar 2021 19:15:10 CET Greg Kurz wrote:
> > If a fid was actually re-opened 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 opened a new
> 
> That's "i.e.". Not a big deal so ...
> 

I'll fix this up before pushing.

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

Thanks ! I'll post the patch for the reclaim_list conversion shortly.

> > handle on the path that is being unlinked). 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
> > to avoid the need to rewind. 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>
> > ---
> > v2: - fix typos in changelog
> >     - drop bogus assert()
> > ---
> >  hw/9pfs/9p.c | 46 ++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 32 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index b65f320e6518..3864d014b43c 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,50 @@ 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);
> > +    if (!fidp) {
> > +        return 0;
> > +    }
> > +
> > +    /*
> > +     * 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;
> >  }
> 
> 
>