Re: [PATCH 1/4] xfs: online repair of directories

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

On Wed, Feb 28, 2024 at 09:16:38AM -0800, Christoph Hellwig wrote:
> On Mon, Feb 26, 2024 at 06:31:01PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > If a directory looks like it's in bad shape, try to sift through the
> > rubble to find whatever directory entries we can, scan the directory
> > tree for the parent (if needed), stage the new directory contents in a
> > temporary file and use the atomic extent swapping mechanism to commit
> > the results in bulk.  As a side effect of this patch, directory
> > inactivation will be able to purge any leftover dir blocks.
> 
> I would have split xfs_inactive_dir and it's caller into a separate
> prep patch, but if you want to keep that together that's probably
> fine as it's completely unrelated functionality.

Eh, I'll split it out.

> > + * Legacy Locking Issues
> > + * ---------------------
> > + *
> > + * Prior to Linux 6.5, if /a, /a/b, and /c were all directories, the VFS would
> > + * not take i_rwsem on /a/b for a "mv /a/b /c/" operation.  This meant that
> > + * only b's ILOCK protected b's dotdot update.  b's IOLOCK was not taken,
> > + * unlike every other dotdot update (link, remove, mkdir).  If the repair code
> > + * dropped the ILOCK, we it was required either to revalidate the dotdot entry
> > + * or to use dirent hooks to capture updates from other threads.
> > + */
> 
> How does this matter here?

Al's been threatening to revert brauner's change to hold i_rwsem on
child directories during renames due to deadlock potential.

OH.  He finally merged it for 6.8-rc1:
a8b0026847b8c ("rename(): avoid a deadlock in the case of parents having no common ancestor")

"Prior to Linux 6.5" and "Legacy" can be deleted now; this is the state
of the world again.  Let me correct some of the weird sentence
structure:

/*
 * Locking Issues
 * --------------
 *
 * If /a, /a/b, and /c are all directories, the VFS does not take i_rwsem on
 * /a/b for a "mv /a/b /c/" operation.  This means that only b's ILOCK protects
 * b's dotdot update.  This is in contrast to every other dotdot update (link,
 * remove, mkdir).  If the repair code drops the ILOCK, it must either
 * revalidate the dotdot entry or use dirent hooks to capture updates from
 * other threads.
 */

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

--D