Re: [RFC PATCH] vfs: remove lockdep bogosity in __sb_start_write

[Dave Chinner <david@fromorbit.com>]

On Tue, Nov 03, 2020 at 11:37:50AM -0800, Darrick J. Wong wrote:
> On Tue, Nov 03, 2020 at 06:46:59PM +0000, Christoph Hellwig wrote:
> > On Tue, Nov 03, 2020 at 10:34:44AM -0800, Darrick J. Wong wrote:
> > > > Please split the function into __sb_start_write and
> > > > __sb_start_write_trylock while you're at it..
> > > 
> > > Any thoughts on this patch itself?  I don't feel like I have 100% of the
> > > context to know whether the removal is a good idea for non-xfs
> > > filesystems, though I'm fairly sure the current logic is broken.
> > 
> > The existing logic looks pretty bogus to me as well.  Did you try to find
> > the discussion that lead to it?
> 
> TBH I don't know where the discussion happened.  The "convert to
> trylock" behavior first appeared as commit 5accdf82ba25c back in 2012;
> that commit seems to have come from v6 of a patch[1] that Jan Kara sent
> to try to fix fs freeze handling back in 2012.  The behavior was not in
> the v5[0] patch, nor was there any discussion for any of the v5 patches
> that would suggest why things changed from v5 to v6.
> 
> Dave and I were talking about this on IRC yesterday, and his memory
> thought that this was lockdep trying to handle xfs taking intwrite
> protection while handling a write (or page_mkwrite) operation.  I'm not
> sure where "XFS for example gets freeze protection on internal level
> twice in some cases" would actually happen -- did xfs support nested
> transactions in the past?  We definitely don't now, so I don't think the
> comment is valid anymore.
> 
> The last commit to touch this area was f4b554af9931 (in 2015), which
> says that Dave explained that the trylock hack + comment could be
> removed, but the patch author never did that, and lore doesn't seem to
> know where or when Dave actually said that?

I'm pretty sure this "nesting internal freeze references" stems from
the fact we log and flush the superblock after fulling freezing the
filesystem to dirty the journal so recovery after a crash while
frozen handles unlinked inodes.

The high level VFS freeze annotations were not able to handle
running this transaction when transactions were supposed to already
be blocked and drained, so there was a special hack to hide it from
lockdep. Then we ended up hiding it from the VFS via
XFS_TRANS_NO_WRITECOUNT in xfs_sync_sb() because we needed it in
more places than just freeze (e.g. the log covering code
run by the background log worker). It's kinda documented here:

/*
 * xfs_sync_sb
 *
 * Sync the superblock to disk.
 *
 * Note that the caller is responsible for checking the frozen state of the
 * filesystem. This procedure uses the non-blocking transaction allocator and
 * thus will allow modifications to a frozen fs. This is required because this
 * code can be called during the process of freezing where use of the high-level
 * allocator would deadlock.
 */

So, AFAICT, the whole "XFS nests internal transactions" lockdep 
handling in __sb_start_write() has been unnecessary for quite a few
years now....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com