Re: [PATCH] fuse: Fix deadlock on open(O_TRUNC)

[Yongji Xie <xieyongji@xxxxxxxxxxxxx>]

On Mon, Aug 16, 2021 at 10:35 AM Peng Tao <bergwolf@xxxxxxxxx> wrote:
>
> On Fri, Aug 13, 2021 at 5:57 PM Xie Yongji <xieyongji@xxxxxxxxxxxxx> wrote:
> >
> > The invalidate_inode_pages2() might be called with FUSE_NOWRITE
> > set in fuse_finish_open(), which can lead to deadlock in
> > fuse_launder_page().
> >
> > To fix it, this tries to delay calling invalidate_inode_pages2()
> > until FUSE_NOWRITE is removed.
> >
> > Fixes: e4648309b85a ("fuse: truncate pending writes on O_TRUNC")
> > Signed-off-by: Xie Yongji <xieyongji@xxxxxxxxxxxxx>
> > ---
> >  fs/fuse/dir.c    |  2 +-
> >  fs/fuse/file.c   | 19 +++++++++++++++----
> >  fs/fuse/fuse_i.h |  2 +-
> >  3 files changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index eade6f965b2e..d919c3e89cb0 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -548,7 +548,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >                 fuse_sync_release(fi, ff, flags);
> >         } else {
> >                 file->private_data = ff;
> > -               fuse_finish_open(inode, file);
> > +               fuse_finish_open(inode, file, false);
> >         }
> >         return err;
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 97f860cfc195..035af9c88eaf 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -193,12 +193,12 @@ static void fuse_link_write_file(struct file *file)
> >         spin_unlock(&fi->lock);
> >  }
> >
> > -void fuse_finish_open(struct inode *inode, struct file *file)
> > +void fuse_finish_open(struct inode *inode, struct file *file, bool no_write)
> >  {
> >         struct fuse_file *ff = file->private_data;
> >         struct fuse_conn *fc = get_fuse_conn(inode);
> >
> > -       if (!(ff->open_flags & FOPEN_KEEP_CACHE))
> > +       if (!(ff->open_flags & FOPEN_KEEP_CACHE) && !no_write)
> >                 invalidate_inode_pages2(inode->i_mapping);
> It would break !FOPEN_KEEP_CACHE semantics, right? Fuse server asks
> the kernel not to keep cache across open but kernel still keeps it?
>

The caller should call invalidate_inode_pages2() in another place, for
example, in our case:

@@ -259,6 +264,12 @@ int fuse_open_common(struct inode *inode, struct
file *file, bool isdir)

        if (is_wb_truncate | dax_truncate) {
                fuse_release_nowrite(inode);
+               /*
+                * Only call invalidate_inode_pages2() after removing
+                * FUSE_NOWRITE, otherwise fuse_launder_page() would deadlock.
+                */
+               if (!keep_cache)
+                       invalidate_inode_pages2(inode->i_mapping);
                inode_unlock(inode);
        }

Thanks,
Yongji