Re: [PATCH 07/13] xfs: document the invalidate_bdev call in invalidate_bdev

On Thu, Aug 10, 2023 at 05:52:25PM +0200, Christoph Hellwig wrote:

On Thu, Aug 10, 2023 at 04:22:15PM +0100, Matthew Wilcox wrote:

quoted

quoted

 		blkdev_issue_flush(mp->m_logdev_targp->bt_bdev);
 		invalidate_bdev(mp->m_logdev_targp->bt_bdev);

While I have no complaints with this as a commit message, it's just too
verbose for an inline comment, IMO.  Something pithier and more generic
would seem appropriate.  How about:

	/*
	 * Prevent userspace (eg blkid or xfs_db) from seeing stale data.
	 * XFS is not coherent with the bdev's page cache.
	 */

Well, this completely misses the point.  The point is that XFS should
never have to invalidate the page cache because it's not using it,
but it has to due to weird races.  I tried to condese the message but
I could not come up with a good one that's not losing information.

Agreed -- it took me a while to set up an arm64 box with just the right
debugging info to figure out why certain fstests were flaky.  I do think
it's useful (despite my other reply to willy) to retain the defect
details for hard-to-reproduce errors, and the only way to do that
without encountering the dead url problem is to dump it in a huge
commit message or a comment.

(Too bad there's no way to have a commit whose code comments reference
the commit id of that commit to say "Hey, you need to read this commit
before you touch this line"...)

--D