Message-ID: <632ba61d7666a_34962942a@dwillia2-xfh.jf.intel.com.notmuch>
Date:   Wed, 21 Sep 2022 17:02:37 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Dave Chinner <david@...morbit.com>,
        Dan Williams <dan.j.williams@...el.com>
CC:     <akpm@...ux-foundation.org>, Matthew Wilcox <willy@...radead.org>,
        "Jan Kara" <jack@...e.cz>, "Darrick J. Wong" <djwong@...nel.org>,
        Jason Gunthorpe <jgg@...dia.com>,
        Christoph Hellwig <hch@....de>,
        John Hubbard <jhubbard@...dia.com>,
        <linux-fsdevel@...r.kernel.org>, <nvdimm@...ts.linux.dev>,
        <linux-xfs@...r.kernel.org>, <linux-mm@...ck.org>,
        <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v2 05/18] xfs: Add xfs_break_layouts() to the inode
 eviction path

Dave Chinner wrote:
> On Tue, Sep 20, 2022 at 09:44:52AM -0700, Dan Williams wrote:
> > Dave Chinner wrote:
> > > On Mon, Sep 19, 2022 at 09:11:48AM -0700, Dan Williams wrote:
> > > > Dave Chinner wrote:
> > > > > That all said, this really looks like a bit of a band-aid.
> > > > 
> > > > It definitely is since DAX is in this transitory state between doing
> > > > some activities page-less and others with page metadata. If DAX was
> > > > fully committed to behaving like a typical page then
> > > > unmap_mapping_range() would have already satisfied this reference
> > > > counting situation.
> > > > 
> > > > > I can't work out why would we we ever have an actual layout lease
> > > > > here that needs breaking given they are file based and active files
> > > > > hold a reference to the inode. If we ever break that, then I suspect
> > > > > this change will cause major problems for anyone using pNFS with XFS
> > > > > as xfs_break_layouts() can end up waiting for NFS delegation
> > > > > revocation. This is something we should never be doing in inode
> > > > > eviction/memory reclaim.
> > > > > 
> > > > > Hence I have to ask why this lease break is being done
> > > > > unconditionally for all inodes, instead of only calling
> > > > > xfs_break_dax_layouts() directly on DAX enabled regular files?  I
> > > > > also wonder what exciting new system deadlocks this will create
> > > > > because BREAK_UNMAP_FINAL can essentially block forever waiting on
> > > > > dax mappings going away. If that DAX mapping reclaim requires memory
> > > > > allocations.....
> > > > 
> > > > There should be no memory allocations in the DAX mapping reclaim path.
> > > > Also, the page pins it waits for are precluded from being GUP_LONGTERM.
> > > 
> > > So if the task that holds the pin needs memory allocation before it
> > > can unpin the page to allow direct inode reclaim to make progress?
> > 
> > No, it couldn't, and I realize now that GUP_LONGTERM has nothing to do
> > with this hang since any GFP_KERNEL in a path that took a DAX page pin
> > path could run afoul of this need to wait.
> > 
> > So, this has me looking at invalidate_inodes() and iput_final(), where I
> > did not see the reclaim entanglement, and thinking DAX has the unique
> > requirement to make sure that no access to a page outlives the hosting
> > inode.
> > 
> > Not that I need to tell you, but to get my own thinking straight,
> > compare that to typical page cache as the pinner can keep a pinned
> > page-cache page as long as it wants even after it has been truncated.
> 
> Right, because the page pin prevents the page from being freed
> after the page references the page cache keeps have been released.
> 
> But page cache page != DAX page. The DAX page is a direct reference
> to the storage media, not a generic reference counted kernel page
> that the kernel will keep alive as long as there is a reference to
> it.
> 
> Hence for a DAX page, we have to revoke all access to the page
> before the controlling owner context is torn down, otherwise we have
> a use-after-free scenario at the storage media level. For a FSDAX
> file data page, that owner context is the inode...
> 
> > DAX needs to make sure that truncate_inode_pages() ceases all access to
> > the page synchronous with the truncate.
> 
> Yes, exactly.
> 
> >
> > The typical page-cache will
> > ensure that the next mapping of the file will get a new page if the page
> > previously pinned for that offset is still in use, DAX can not offer
> > that as the same page that was previously pinned is always used.
> 
> Yes, because the new DAX ipage lookup will return the original page
> in the storage media, not a newly instantiated page cache page.
> 
> > So I think this means something like this:
> > 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 6462276dfdf0..ab16772b9a8d 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -784,6 +784,11 @@ int invalidate_inodes(struct super_block *sb, bool kill_dirty)
> >                         continue;
> >                 }
> >  
> > +               if (dax_inode_busy(inode)) {
> > +                       busy = 1;
> > +                       continue;
> > +               }
> 
> That this does more than a check (i.e. it runs whatever
> dax_zap_pages() does) means it cannot be run under the inode
> spinlock.

Here lockdep did immediately scream at me for what can be done under the
inode lock.

> As this is called from the block device code when a bdev is being
> removed (i.e. will only find a superblock and inodes to invalidate
> on hot-unplug), shouldn't this DAX mapping invalidation actually be
> handled by the pmem failure notification infrastructure we've just
> added for reflink?

Perhaps. I think the patch I have in the works now is simpler without
having to require ext4 to add notify_failure infrastructure, but that
may be where this ends up.

> 
> > +
> >                 inode->i_state |= I_FREEING;
> >                 inode_lru_list_del(inode);
> >                 spin_unlock(&inode->i_lock);
> > @@ -1733,6 +1738,8 @@ static void iput_final(struct inode *inode)
> >                 spin_unlock(&inode->i_lock);
> >  
> >                 write_inode_now(inode, 1);
> > +               if (IS_DAX(inode))
> > +                       dax_break_layouts(inode);
> >  
> >                 spin_lock(&inode->i_lock);
> >                 state = inode->i_state;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 9eced4cc286e..e4a74ab310b5 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -3028,8 +3028,20 @@ extern struct inode * igrab(struct inode *);
> >  extern ino_t iunique(struct super_block *, ino_t);
> >  extern int inode_needs_sync(struct inode *inode);
> >  extern int generic_delete_inode(struct inode *inode);
> > +
> > +static inline bool dax_inode_busy(struct inode *inode)
> > +{
> > +       if (!IS_DAX(inode))
> > +               return false;
> > +
> > +       return dax_zap_pages(inode) != NULL;
> > +}
> > +
> >  static inline int generic_drop_inode(struct inode *inode)
> >  {
> > +       if (dax_inode_busy(inode))
> > +               return 0;
> > +
> >         return !inode->i_nlink || inode_unhashed(inode);
> >  }
> 
> I don't think that's valid. This can result in unreferenced unlinked
> inodes that should be torn down immediately being placed in the LRU
> and cached in memory and potentially not processed until there is
> future memory pressure or an unmount....
> 
> i.e. dropping the final reference on an unlinked inode needs to
> reclaim the inode immediately and allow the filesystem to free the
> inode, regardless of any other factor. Nothing should have an active
> reference to the inode or inode related data/metadata at this point
> in time.
> 
> Honestly, this still seems like a band-aid because it doesn't appear
> to address that something has pinned the storage media without
> having an active reference to the object that arbitrates access to
> that storage media (i.e. the inode and, by proxy, then filesystem).
> Where are these DAX page pins that don't require the pin holder to
> also hold active references to the filesystem objects coming from?

I do not have a practical exploit for this only the observation that
iput_final() triggers truncate_inode_pages(). Then the follow-on
assumption that *if* pages are still recorded in inode->i_pages then
those pages could have an elevated reference count.

Certainly GUP_LONGTERM can set up these "memory registration" scenarios,
but those are forbidden.

The scenario I cannot convince myself is impossible is a driver that
goes into interruptible sleep while operating on a page it got from
get_user_pages(). Where the eventual driver completion path will clean
up the pinned page, but the process that launched the I/O has already
exited and dropped all the inode references it was holding. That's not
buggy on its face since the driver still cleans up everything it was
handed, but if this type of disconnect happens (closing mappings and
files while I/O is in-flight) then iput_final() needs to check.

The block-I/O submission path seems to be uninterruptible to prevent
this type of disconnect, but who knows what other drivers do with their
pages.

Powered by blists - more mailing lists