Re: lockdep recursive locking warning on for-next

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

On Fri, Feb 19, 2021 at 03:14:27PM +1100, Dave Chinner wrote:
> On Thu, Feb 18, 2021 at 11:31:54AM -0800, Darrick J. Wong wrote:
> > On Thu, Feb 18, 2021 at 02:12:52PM -0500, Brian Foster wrote:
> > > On Thu, Feb 18, 2021 at 10:49:26AM -0800, Darrick J. Wong wrote:
> > > > On Thu, Feb 18, 2021 at 01:14:50PM -0500, Brian Foster wrote:
> > > > > Hi Darrick,
> > > > > 
> > > > > I'm seeing the warning below via xfs/167 on a test machine. It looks
> > > > > like it's just complaining about nested freeze protection between the
> > > > > scan invocation and an underlying transaction allocation for an inode
> > > > > eofblocks trim. I suppose we could either refactor xfs_trans_alloc() to
> > > > > drop and reacquire freeze protection around the scan, or alternatively
> > > > > call __sb_writers_release() and __sb_writers_acquire() around the scan
> > > > > to retain freeze protection and quiet lockdep. Hm?
> > > > 
> > > > Erk, isn't that a potential log grant livelock too?
> > > > 
> > > > Fill up the filesystem with real data and cow blocks until it's full,
> > > > then spawn exactly enough file writer threads to eat up all the log
> > > > reservation, then each _reserve() fails, so every thread starts a scan
> > > > and tries to allocate /another/ transaction ... but there's no space
> > > > left in the log, so those scans just block indefinitely.
> > > > 
> > > > So... I think the solution here is to go back to a previous version of
> > > > what that patchset did, where we'd drop the whole transaction, run the
> > > > scan, and jump back to the top of the function to get a fresh
> > > > transaction.
> > > > 
> > > 
> > > But we don't call into the scan while holding log reservation. We hold
> > > the transaction memory and freeze protection. It's probably debatable
> > > whether we'd want to scan with freeze protection held or not, but I
> > > don't see how dropping either of those changes anything wrt to log
> > > reservation..?
> > 
> > Right, sorry about the noise.  We could just trick lockdep with
> > __sb_writers_release like you said.  Though I am a tad bit concerned
> > about the rwsem behavior -- what happens if:
> > 
> > T1 calls sb_start_intwrite (which is down_read on sb_writers), gets the
> > lock, and then hits ENOSPC and goes into our scan loop; meanwhile,
> > 
> > T2 calls sb_wait_write (which is down_write on sb_writers), and is
> > scheduled off because it was a blocking lock attempt; and then,
> > 
> > T1 finds some eofblocks to delete, and now it wants to sb_start_intwrite
> > again as part of allocating that second nested transaction.  Does that
> > actually work, or will T1 stall because we don't allow more readers once
> > something is waiting in down_write()?
> 
> The stack trace nesting inside xfs_trans_alloc() looks fundamentally
> wrong to me. It screams "warning, dragons be here" to me. We're not
> allowed to nest transactions -anywhere- so actually designing a call
> path that ends up looking like we are nesting transactions but then
> plays whacky games to avoid problems associated with nesting
> seems... poorly thought out.

The original version of the code[1] in question cancelled the
transaction before starting the scan, but the reviewers didn't think it
would be a problem to hold on to the transaction or recurse intwrite.

[1] https://lore.kernel.org/linux-xfs/20210124094816.GE670331@infradead.org/

I probably should've just kept it the way it was.  Well, maybe I'll send
in my own fixpatch and see what the rest of you think.

> > > > > BTW, the stack report also had me wondering whether we had or need any
> > > > > nesting protection in these new scan invocations. For example, if we
> > > > > have an fs with a bunch of tagged inodes and concurrent allocation
> > > > > activity, would anything prevent an in-scan transaction allocation from
> > > > > jumping back into the scan code to complete outstanding work? It looks
> > > > > like that might not be possible right now because neither scan reserves
> > > > > blocks, but they do both use transactions and that's quite a subtle
> > > > > balance..
> > > > 
> > > > Yes, that's a subtlety that screams for better documentation.
> > > > 
> > > 
> > > TBH, I'm not sure that's enough. I think we should at least have some
> > > kind of warning, even if only in DEBUG mode, that explicitly calls out
> > > if we've become susceptible to this kind of scan reentry. Otherwise I
> > > suspect that if this problem is ever truly introduced, the person who
> > > first discovers it will probably be user with a blown stack. :( Could we
> > > set a flag on the task or something that warns as such (i.e. "WARNING:
> > > attempted block reservation in block reclaim context") or perhaps just
> > > prevents scan reentry in the first place?
> > 
> > What if we implemented a XFS_TRANS_TRYRESERVE flag that would skip the
> > scanning loops?  Then it would be at least a little more obvious when
> > xfs_free_eofblocks and xfs_reflink_cancel_cow_range kick on.
> 
> Isn't detecting transaction reentry exactly what PF_FSTRANS is for?
> 
> Or have we dropped that regression fix on the ground *again*?

That series isn't making progress.

--D

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