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@xxxxxxxxxxxxx
