On Wed, Jan 05, 2022 at 11:52:26AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > 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@xxxxxxxxxx> > --- > 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@xxxxxxxxxxxxx
