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

[Dave Chinner <david@fromorbit.com>]

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?

Other than than it looks good.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com