Re: [PATCH v3] fuse: share lookup state between submount and its parent

[Krister Johansen]

Hi Miklos,
Thanks for all the feedback. I've made all the changes you requested and
was pleased to find that this reduced the overall size of the patch.

On Thu, Oct 19, 2023 at 02:39:34PM +0200, Miklos Szeredi wrote:
> On Wed, Oct 18, 2023 at 3:34 AM Krister Johansen
> <kjlx@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > Fuse submounts do not perform a lookup for the nodeid that they inherit
> > from their parent.  Instead, the code decrements the nlookup on the
> > submount's fuse_inode when it is instantiated, and no forget is
> > performed when a submount root is evicted.
> >
> > Trouble arises when the submount's parent is evicted despite the
> > submount itself being in use.  In this author's case, the submount was
> > in a container and deatched from the initial mount namespace via a
> > MNT_DEATCH operation.  When memory pressure triggered the shrinker, the
> > inode from the parent was evicted, which triggered enough forgets to
> > render the submount's nodeid invalid.
> >
> > Since submounts should still function, even if their parent goes away,
> > solve this problem by sharing refcounted state between the parent and
> > its submount.  When all of the references on this shared state reach
> > zero, it's safe to forget the final lookup of the fuse nodeid.
> >
> > Signed-off-by: Krister Johansen <kjlx@xxxxxxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 1866d779d5d2 ("fuse: Allow fuse_fill_super_common() for submounts")
> > ---
> >  fs/fuse/fuse_i.h | 20 +++++++++++
> >  fs/fuse/inode.c  | 88 ++++++++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 105 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 405252bb51f2..0d1659c5016b 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -63,6 +63,24 @@ struct fuse_forget_link {
> >         struct fuse_forget_link *next;
> >  };
> >
> > +/* Submount lookup tracking */
> > +struct fuse_submount_lookup {
> > +       /** Refcount */
> > +       refcount_t count;
> > +
> > +       /** Unique ID, which identifies the inode between userspace
> > +        * and kernel */
> > +       u64 nodeid;
> > +
> > +       /** Number of lookups on this inode */
> > +       u64 nlookup;
> 
> sl->nlookup will always be one.  So that can just be implicit and this
> field can just go away.
> 
> > +
> > +       /** The request used for sending the FORGET message */
> > +       struct fuse_forget_link *forget;
> > +
> > +       struct rcu_head rcu;
> 
> RCU would be needed if any fields could be accessed from RCU protected
> code.  But AFAICS there's no such access, so this shouldn't be needed.
>   Am I missing something?

No, you're correct and not missing anything.  I've cleaned this up.

Thanks again,

-K