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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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?

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com