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@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? 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@xxxxxxxxxxxxx
