Re: [PATCH] ext2: take invalidate_lock in ext2_write_failed()

[Pedro Falcato]

On Mon, Oct 26, 2026 at 02:16:19PM +0100, Jan Kara wrote:
> On Sat 24-10-26 23:27:26, Pedro Falcato wrote:
> > Currently, ext2_write_failed() takes care of removing extra data
> > (folios & blocks) from the page cache by first truncating the page cache,
> > then truncating the actual inode's blocks.
> > 
> > Understanding that this is correct - namely, that a concurrent page cache
> > lookup can't create stale pages and stale block mappings between
> > truncate_pagecache() and ext2_truncate_blocks() - is non trivial. This is
> > correct for two non-obvious reasons:
> > 
> > 1) every caller of ext2_write_failed() holds some sort of exclusion
> > against truncation. In most cases, callers hold (in some way) the inode
> > lock. ext2_write_failed() is only reachable from buffered/direct IO write
> > paths, which hold it. page_symlink() can also call write_begin _without
> > the inode lock_, but at that point the inode is I_NEW, so there are no other
> > users.
> > 
> > 2) the page cache is generally extremely careful at never instantiating
> > pages beyond EOF.
> > 
> > This is all quite confusing. Holding the invalidate lock across both
> > operations is largely simpler, and mirrors other bits of other filesystems
> > (like ext4_truncate_failed_write). Do that.
> > 
> > This has the side effect of making it so ext2_evict_inode() doesn't take
> > the invalidate_lock when truncating blocks on inode deletion. This is safe
> > as the inode should not have any users at that point.
> > 
> > Signed-off-by: Pedro Falcato <pfalcato@xxxxxxx>
> 
> There's actually one more reason - filesystems that don't support hole
> punching are generally ok with the synchronization provided by inode lock
> and i_size checks (as within i_size allocated space can never disappear
> under you). But I agree this is all too subtle so you're change is a good
> one. Added to my tree.

Thanks!

Right. So I'm wondering if we couldn't assert some locks in truncate code.
Something like (in truncate_inode_pages_range):
	/* truncation is only safe in a couple of cases:
	 * 1) caller holds the invalidate_lock. This protects against hole
	 * punching and other truncate-like activities.
	 * 2) caller holds the inode lock. This excludes against truncate-like
	 * operations in a variety of cases.
	 * 3) inode is new or freeing - no need to synchronize anything since
	 * we should be the only user for this inode.
	 */
	lockdep_assert(lockdep_is_held(&mapping->invalidate_lock) ||
			((lockdep_is_held(&mapping->host->i_rwsem) &&
			 (lstart >= i_size_read(mapping->host) ||
			 file_does_not_support_holes) ||
			inode_new_or_freeing);

I'm sure I'm missing some silly edgecase, but it would be nice to have this
properly written down and asserted.

-- 
Pedro