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