Re: [PATCH v2] btrfs: fix the delalloc range locking if sector size < page size

[David Sterba <dsterba@suse.cz>]

On Mon, Oct 07, 2024 at 09:51:40AM +1030, Qu Wenruo wrote:
> Inside lock_delalloc_folios(), there are several problems related to
> sector size < page size handling:
> 
> - Set the writer locks without checking if the folio is still valid
>   We call btrfs_folio_start_writer_lock() just like it's folio_lock().
>   But since the folio may not even be the folio of the current mapping,
>   we can easily screw up the folio->private.
> 
> - The range is not clampped inside the page
>   This means we can over write other bitmaps if the start/len is not
>   properly handled, and trigger the btrfs_subpage_assert().
> 
> - @processed_end is always rounded up to page end
>   If the delalloc range is not page aligned, and we need to retry
>   (returning -EAGAIN), then we will unlock to the page end.
> 
>   Thankfully this is not a huge problem, as now
>   btrfs_folio_end_writer_lock() can handle range larger than the locked
>   range, and only unlock what is already locked.
> 
> Fix all these problems by:
> 
> - Lock and check the folio first, then call
>   btrfs_folio_set_writer_lock()
>   So that if we got a folio not belonging to the inode, we won't
>   touch folio->private.
> 
> - Properly truncate the range inside the page
> 
> - Update @processed_end to the locked range end
> 
> - Remove btrfs_folio_start_writer_lock()
>   Since all callsites only utilize btrfs_folio_set_writer_lock() now,
>   remove the unnecessary btrfs_folio_start_writer_lock().
> 
> Fixes: 1e1de38792e0 ("btrfs: make process_one_page() to handle subpage locking")

This commit is in 5.14, so we'd need backport to 5.15+. The v1 would be
more suitable as it does not remove the unused
btrfs_folio_start_writer_lock(). As it's exported function there should
be no warning.

> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Changelog:
> v2:
> - Remove the unused btrfs_folio_start_writer_lock() function
> ---
>  fs/btrfs/extent_io.c | 15 +++++++-------
>  fs/btrfs/subpage.c   | 47 --------------------------------------------
>  fs/btrfs/subpage.h   |  2 --
>  3 files changed, 7 insertions(+), 57 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index e0b43640849e..0448cee2b983 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -262,22 +262,21 @@ static noinline int lock_delalloc_folios(struct inode *inode,
>  
>  		for (i = 0; i < found_folios; i++) {
>  			struct folio *folio = fbatch.folios[i];
> -			u32 len = end + 1 - start;
> +			u64 range_start = max_t(u64, folio_pos(folio), start);
> +			u32 range_len = min_t(u64, folio_pos(folio) + folio_size(folio),
> +					      end + 1) - range_start;
>  
>  			if (folio == locked_folio)
>  				continue;
>  
> -			if (btrfs_folio_start_writer_lock(fs_info, folio, start,
> -							  len))
> -				goto out;
> -
> +			folio_lock(folio);
>  			if (!folio_test_dirty(folio) || folio->mapping != mapping) {

Should the range_start and range_end be set after folio_lock? One of the
prolblems you're fixing is the folio and mapping mismatch, if I read it
correctly.