On Mon, Oct 09, 2023 at 02:52:42PM +0200, Bernd Schubert wrote:
>
>
> On 10/7/23 02:41, Krister Johansen wrote:
> > On Fri, Oct 06, 2023 at 07:13:06PM +0200, Bernd Schubert wrote:
> > >
> > >
> > > On 10/2/23 17:24, Krister Johansen wrote:
> > > > The submount code uses the parent nodeid passed into the function in
> > > > order to create the root dentry for the new submount. This nodeid does
> > > > not get its remote reference count incremented by a lookup option.
> > > >
> > > > If the parent inode is evicted from its superblock, due to memory
> > > > pressure for example, it can result in a forget opertation being sent to
> > > > the server. Should this nodeid be forgotten while it is still in use in
> > > > a submount, users of the submount get an error from the server on any
> > > > subsequent access. In the author's case, this was an EBADF on all
> > > > subsequent operations that needed to reference the root.
> > > >
> > > > Debugging the problem revealed that the dentry shrinker triggered a forget
> > > > after killing the dentry with the last reference, despite the root
> > > > dentry in another superblock still using the nodeid.
> > > >
> > > > As a result, a container that was also using this submount failed to
> > > > access its filesystem because it had borrowed the reference instead of
> > > > taking its own when setting up its superblock for the submount.
> > > >
> > > > This commit fixes the problem by having the new submount trigger a
> > > > lookup for the parent as part of creating a new root dentry for the
> > > > virtiofsd submount superblock. This allows each superblock to have its
> > > > inodes removed by the shrinker when unreferenced, while keeping the
> > > > nodeid reference count accurate and active with the server.
> > > >
> > > > Signed-off-by: Krister Johansen <kjlx@xxxxxxxxxxxxxxxxxx>
> > > > ---
> > > > fs/fuse/dir.c | 10 +++++-----
> > > > fs/fuse/fuse_i.h | 6 ++++++
> > > > fs/fuse/inode.c | 43 +++++++++++++++++++++++++++++++++++++------
> > > > 3 files changed, 48 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > > index 5e01946d7531..333730c74619 100644
> > > > --- a/fs/fuse/dir.c
> > > > +++ b/fs/fuse/dir.c
> > > > @@ -183,11 +183,11 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args,
> > > > args->out_args[0].value = outarg;
> > > > }
> > > > -static int fuse_dentry_revalidate_lookup(struct fuse_mount *fm,
> > > > - struct dentry *entry,
> > > > - struct inode *inode,
> > > > - struct fuse_entry_out *outarg,
> > > > - bool *lookedup)
> > > > +int fuse_dentry_revalidate_lookup(struct fuse_mount *fm,
> > > > + struct dentry *entry,
> > > > + struct inode *inode,
> > > > + struct fuse_entry_out *outarg,
> > > > + bool *lookedup)
> > > > {
> > > > struct dentry *parent;
> > > > struct fuse_forget_link *forget;
> > > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > > > index 405252bb51f2..a66fcf50a4cc 100644
> > > > --- a/fs/fuse/fuse_i.h
> > > > +++ b/fs/fuse/fuse_i.h
> > > > @@ -1325,6 +1325,12 @@ void fuse_dax_dontcache(struct inode *inode, unsigned int flags);
> > > > bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int map_alignment);
> > > > void fuse_dax_cancel_work(struct fuse_conn *fc);
> > > > +/* dir.c */
> > > > +int fuse_dentry_revalidate_lookup(struct fuse_mount *fm, struct dentry *entry,
> > > > + struct inode *inode,
> > > > + struct fuse_entry_out *outarg,
> > > > + bool *lookedup);
> > > > +
> > > > /* ioctl.c */
> > > > long fuse_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
> > > > long fuse_file_compat_ioctl(struct file *file, unsigned int cmd,
> > > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > > index 444418e240c8..79a31cb55512 100644
> > > > --- a/fs/fuse/inode.c
> > > > +++ b/fs/fuse/inode.c
> > > > @@ -1464,7 +1464,13 @@ static int fuse_fill_super_submount(struct super_block *sb,
> > > > struct fuse_mount *fm = get_fuse_mount_super(sb);
> > > > struct super_block *parent_sb = parent_fi->inode.i_sb;
> > > > struct fuse_attr root_attr;
> > > > + struct fuse_inode *fi;
> > > > struct inode *root;
> > > > + struct inode *parent;
> > > > + struct dentry *pdent;
> > > > + struct fuse_entry_out outarg;
> > > > + bool lookedup = false;
> > > > + int ret;
> > > > fuse_sb_defaults(sb);
> > > > fm->sb = sb;
> > > > @@ -1480,14 +1486,39 @@ static int fuse_fill_super_submount(struct super_block *sb,
> > > > if (parent_sb->s_subtype && !sb->s_subtype)
> > > > return -ENOMEM;
> > > > - fuse_fill_attr_from_inode(&root_attr, parent_fi);
> > > > - root = fuse_iget(sb, parent_fi->nodeid, 0, &root_attr, 0, 0);
> > > > /*
> > > > - * This inode is just a duplicate, so it is not looked up and
> > > > - * its nlookup should not be incremented. fuse_iget() does
> > > > - * that, though, so undo it here.
> > > > + * It is necessary to lookup the parent_if->nodeid in case the dentry
> > > > + * that triggered the automount of the submount is later evicted.
> > > > + * If this dentry is evicted without the lookup count getting increased
> > > > + * on the submount root, then the server can subsequently forget this
> > > > + * nodeid which leads to errors when trying to access the root of the
> > > > + * submount.
> > > > */
> > > > - get_fuse_inode(root)->nlookup--;
> > > > + parent = &parent_fi->inode;
> > > > + pdent = d_find_alias(parent);
> > > > + if (!pdent)
> > > > + return -EINVAL;
> > > > +
> > > > + ret = fuse_dentry_revalidate_lookup(fm, pdent, parent, &outarg,
> > > > + &lookedup);
> > > > + dput(pdent);
> > > > + /*
> > > > + * The new root owns this nlookup on success, and it is incremented by
> > > > + * fuse_iget(). In the case the lookup succeeded but revalidate fails,
> > > > + * ensure that the lookup count is tracked by the parent.
> > > > + */
> > > > + if (ret <= 0) {
> > > > + if (lookedup) {
> > > > + fi = get_fuse_inode(parent);
> > > > + spin_lock(&fi->lock);
> > > > + fi->nlookup++;
> > > > + spin_unlock(&fi->lock);
> > > > + }
> > >
> > > I might be wrong, but doesn't that mean that
> > > "get_fuse_inode(root)->nlookup--" needs to be called?
> >
> > In the case where ret > 0, the nlookup on get_fuse_inode(root) is set to
> > 1 by fuse_iget(). That ensures that the root is forgotten when later
> > unmounted. The code that handles the forget uses the count of nlookup
> > to tell the server-side how many references to forget. (That's in
> > fuse_evict_inode()).
> >
> > However, if the fuse_dentry_revalidate_lookup() call performs a valid
> > lookup but returns an error, this function will return before it fills
> > out s_root in the superblock or calls fuse_iget(). If the superblock
> > doesn't have a s_root set, then the code in generic_kill_super() won't
> > dput() the root dentry and trigger the forget.
> >
> > The intention of this code was to handle the case where the lookup had
> > succeeded, but the code determined it was still necessary to return an
> > error. In that situation, the reference taken by the lookup has to be
> > accounted somewhere, and the parent seemed like a plausible candidate.
>
> Yeah sorry, I had just missed that fuse_iget() also moved and then thought
> it would have increased fi->nlookup already.
No worries; I'd much rather get feedback if something doesn't look
right, even if it turns out okay in the end.
> > However, after writing up this response, I can see that there's still a
> > problem here if d_make_root(root) returns NULL, because we'll also lose
> > track of the nlookup in that case.
> >
> > If you agree that charging this to the parent on error makes sense, I'll
> > re-work the error handling here so that the right thing happens when
> > either fuse_dentry_revalidate_lookup() or d_make_root() encounter an
> > error.
>
> Oh yeah, I also missed that. Although, iput() calls iput_final, which then
> calls evict and sends the fuse forget - isn't that the right action already?
Thanks, I had forgotten that d_make_root() would call iput() for me if
d_alloc_anon() fails. Let me restate this to suggest that I account the
nlookup to the parent if fuse_dentry_revalidate_lookup() or fuse_iget()
fail instead. Does that sound right?
> > Thanks for the feedback.
>
> Well, false alarm from my side, sorry again!
No apology necessary; I appreciate you spending the time to look and ask
questions.
-K
