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

								Honza

> ---
> 
> This is a patch that fell out of trying to understand whether
> 20261021162105.62525-1-matheus.aalmeida@xxxxxxxxxxxx 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@xxxxxxxx>
SUSE Labs, CR