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

[Jan Kara]

On Mon 26-10-26 16:48:03, Pedro Falcato wrote:
> 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.

The part "file_does_not_support_holes" will be somewhat difficult to assert
but honestly, I'd be willing to transition to a state where all the
filesystems in truncate code are required to hold i_rwsem *and* invalidate_lock
unless they are working beyond i_size, on new, or on freed inodes.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR