[PATCH] ext2: take invalidate_lock in ext2_write_failed()

[PATCH] ext2: take invalidate_lock in ext2_write_failed()

[Pedro Falcato]

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@suse.de>
---

This is a patch that fell out of trying to understand whether
20261021162105.62525-1-matheus.aalmeida@inf.ufrgs.br was correct. I think
it is, for the same reason ext2 is correct. I find this to be easier to reason
about if we just take the invalidate lock across both operations.

 fs/ext2/inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 12ac4cfe1500..723b31dc7dac 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -58,8 +58,10 @@ void ext2_write_failed(struct address_space *mapping, loff_t to)
 	struct inode *inode = mapping->host;
 
 	if (to > inode->i_size) {
+		filemap_invalidate_lock(mapping);
 		truncate_pagecache(inode, inode->i_size);
 		ext2_truncate_blocks(inode, inode->i_size);
+		filemap_invalidate_unlock(mapping);
 	}
 }
 
@@ -1236,9 +1238,7 @@ static void ext2_truncate_blocks(struct inode *inode, loff_t offset)
 	if (ext2_inode_is_fast_symlink(inode))
 		return;
 
-	filemap_invalidate_lock(inode->i_mapping);
 	__ext2_truncate_blocks(inode, offset);
-	filemap_invalidate_unlock(inode->i_mapping);
 }
 
 static int ext2_setsize(struct inode *inode, loff_t newsize)
-- 
2.56.0

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

[Jan Kara]

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@suse.de>

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.

								Honza

> ---
> 
> This is a patch that fell out of trying to understand whether
> 20261021162105.62525-1-matheus.aalmeida@inf.ufrgs.br was correct. I think
> it is, for the same reason ext2 is correct. I find this to be easier to reason
> about if we just take the invalidate lock across both operations.
> 
>  fs/ext2/inode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 12ac4cfe1500..723b31dc7dac 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -58,8 +58,10 @@ void ext2_write_failed(struct address_space *mapping, loff_t to)
>  	struct inode *inode = mapping->host;
>  
>  	if (to > inode->i_size) {
> +		filemap_invalidate_lock(mapping);
>  		truncate_pagecache(inode, inode->i_size);
>  		ext2_truncate_blocks(inode, inode->i_size);
> +		filemap_invalidate_unlock(mapping);
>  	}
>  }
>  
> @@ -1236,9 +1238,7 @@ static void ext2_truncate_blocks(struct inode *inode, loff_t offset)
>  	if (ext2_inode_is_fast_symlink(inode))
>  		return;
>  
> -	filemap_invalidate_lock(inode->i_mapping);
>  	__ext2_truncate_blocks(inode, offset);
> -	filemap_invalidate_unlock(inode->i_mapping);
>  }
>  
>  static int ext2_setsize(struct inode *inode, loff_t newsize)
> -- 
> 2.56.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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@suse.de>
> 
> 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