linux-ext4 - Re: [PATCH v2 05/18] xfs: Add xfs_break_layouts() to the inode eviction path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 20 Sep 2022 07:29:59 +1000
From:   Dave Chinner <david@...morbit.com>
To:     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

On Mon, Sep 19, 2022 at 09:11:48AM -0700, Dan Williams wrote:
> Dave Chinner wrote:
> > On Thu, Sep 15, 2022 at 08:35:38PM -0700, Dan Williams wrote:
> > > In preparation for moving DAX pages to be 0-based rather than 1-based
> > > for the idle refcount, the fsdax core wants to have all mappings in a
> > > "zapped" state before truncate. For typical pages this happens naturally
> > > via unmap_mapping_range(), for DAX pages some help is needed to record
> > > this state in the 'struct address_space' of the inode(s) where the page
> > > is mapped.
> > > 
> > > That "zapped" state is recorded in DAX entries as a side effect of
> > > xfs_break_layouts(). Arrange for it to be called before all truncation
> > > events which already happens for truncate() and PUNCH_HOLE, but not
> > > truncate_inode_pages_final(). Arrange for xfs_break_layouts() before
> > > truncate_inode_pages_final().
....
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index 9ac59814bbb6..ebb4a6eba3fc 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -725,6 +725,27 @@ xfs_fs_drop_inode(
> > >  	return generic_drop_inode(inode);
> > >  }
> > >  
> > > +STATIC void
> > > +xfs_fs_evict_inode(
> > > +	struct inode		*inode)
> > > +{
> > > +	struct xfs_inode	*ip = XFS_I(inode);
> > > +	uint			iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
> > > +	long			error;
> > > +
> > > +	xfs_ilock(ip, iolock);
> > 
> > I'm guessing you never ran this through lockdep.
> 
> I always run with lockdep enabled in my development kernels, but maybe my
> testing was insufficient? Somewhat moot with your concerns below...

I'm guessing your testing doesn't generate inode cache pressure and
then have direct memory reclaim inodes. e.g. on a directory inode
this will trigger lockdep immediately because readdir locks with
XFS_IOLOCK_SHARED and then does GFP_KERNEL memory reclaim. If we try
to take XFS_IOLOCK_EXCL from memory reclaim of directory inodes,
lockdep will then shout from the rooftops...

> > > +
> > > +	truncate_inode_pages_final(&inode->i_data);
> > > +	clear_inode(inode);
> > > +
> > > +	xfs_iunlock(ip, iolock);
> > > +}
> > 
> > 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?

> > /me looks deeper into the dax_layout_busy_page() stuff and realises
> > that both ext4 and XFS implementations of ext4_break_layouts() and
> > xfs_break_dax_layouts() are actually identical.
> > 
> > That is, filemap_invalidate_unlock() and xfs_iunlock(ip,
> > XFS_MMAPLOCK_EXCL) operate on exactly the same
> > inode->i_mapping->invalidate_lock. Hence the implementations in ext4
> > and XFS are both functionally identical.
> 
> I assume you mean for the purposes of this "final" break since
> xfs_file_allocate() holds XFS_IOLOCK_EXCL over xfs_break_layouts().

No, I'm just looking at the two *dax* functions - we don't care what
locks xfs_break_layouts() requires - dax mapping manipulation is
covered by the mapping->invalidate_lock and not the inode->i_rwsem.
This is explicitly documented in the code by the the asserts in both
ext4_break_layouts() and xfs_break_dax_layouts().

XFS holds the inode->i_rwsem over xfs_break_layouts() because we
have to break *file layout leases* from there, too. These are
serialised by the inode->i_rwsem, not the mapping->invalidate_lock.

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com