Re: [PATCH v2] xfs: take the ILOCK when accessing the inode core

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Jan 06, 2022 at 12:47:12PM +1100, Dave Chinner wrote:
> On Wed, Jan 05, 2022 at 11:52:26AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > I was poking around in the directory code while diagnosing online fsck
> > bugs, and noticed that xfs_readdir doesn't actually take the directory
> > ILOCK when it calls xfs_dir2_isblock.  xfs_dir_open most probably loaded
> > the data fork mappings and the VFS took i_rwsem (aka IOLOCK_SHARED) so
> > we're protected against writer threads, but we really need to follow the
> > locking model like we do in other places.
> > 
> > To avoid unnecessarily cycling the ILOCK for fairly small directories,
> > change the block/leaf _getdents functions to consume the ILOCK hold that
> > the parent readdir function took to decide on a _getdents implementation.
> > 
> > It is ok to cycle the ILOCK in readdir because the VFS takes the IOLOCK
> > in the appropriate mode during lookups and writes, and we don't want to
> > be holding the ILOCK when we copy directory entries to userspace in case
> > there's a page fault.  We really only need it to protect against data
> > fork lookups, like we do for other files.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > v2: reduce the scope of the locked region, and reduce lock cycling
> 
> Looks good, one minor thing: can you add a comment to xfs_readdir()
> that callers/VFS needs to hold the i_rwsem to ensure that the
> directory is not being concurrently modified? Maybe even add a
> ASSERT(rwsem_is_locked(VFS_I(ip)->i_rwsem)) to catch cases where
> this gets broken?

The documentation already says the caller has to hold the inode lock,
but I will change it to say the IOLOCK specifically.  And add the
ASSERT.

--D

> 
> Other than than it looks good.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com