[3/3] 9pfs: Improve unreclaim loop

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(-)

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

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
> 
>