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

[Dave Chinner <david@fromorbit.com>]

On Thu, Nov 05, 2020 at 06:19:51PM -0800, Darrick J. Wong wrote:
> On Fri, Nov 06, 2020 at 08:34:15AM +1100, Dave Chinner wrote:
> > 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....
> 
> Yeah.  Would you be willing to RVB this, or are you all waiting for a v2
> series?

Send out a v2 - you probably need to include some of the above
information in the change log removing the lockdep stuff so it's
preserved this time...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com