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

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

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?

FWIW I couldn't find a place where we could take the freeze locks in the
wrong order (or multiple times) -- I'm pretty sure the file remap stuff
(clone/dedupe/copyrange) all grab write freeze protection; I couldn't
figure out how you could start with pagefault freeze protection and then
need write freeze protection; and AFAIK none of the filesystems that
even care about intwrite will try to grab it recursively.  Most of them
don't allow nested transactions, and the one that does (nilfs2?) appears
to be careful not to nest.

--D

[0] https://lore.kernel.org/linux-fsdevel/1334592845-22862-14-git-send-email-jack@suse.cz/
[1] https://lore.kernel.org/linux-fsdevel/1338589841-9568-14-git-send-email-jack@suse.cz/