Re: [PATCH 1/2] btrfs: add helper folio_end()

[Boris Burkov <boris@bur.io>]

On Tue, Jun 03, 2025 at 10:16:17AM +0200, David Sterba wrote:
> There are several cases of folio_pos + folio_size, add a convenience
> helper for that. Rename local variable in defrag_prepare_one_folio() to
> avoid name clash.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/defrag.c | 8 ++++----
>  fs/btrfs/misc.h   | 7 +++++++
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
> index 6dca263b224e87..e5739835ad02f0 100644
> --- a/fs/btrfs/defrag.c
> +++ b/fs/btrfs/defrag.c
> @@ -849,7 +849,7 @@ static struct folio *defrag_prepare_one_folio(struct btrfs_inode *inode, pgoff_t
>  	struct address_space *mapping = inode->vfs_inode.i_mapping;
>  	gfp_t mask = btrfs_alloc_write_mask(mapping);
>  	u64 folio_start;
> -	u64 folio_end;
> +	u64 folio_last;

This is nitpicky, but I think introducing the new word "last" in in an
inconsistent fashion is a mistake.

In patch 2 at truncate_block_zero_beyond_eof() and
btrfs_writepage_fixup_worker, you have variables called "X_end" that get
assigned to folio_end() - 1. Either those should also get called
"X_last" or this one should have "end" in its name.

>  	struct extent_state *cached_state = NULL;
>  	struct folio *folio;
>  	int ret;
> @@ -886,14 +886,14 @@ static struct folio *defrag_prepare_one_folio(struct btrfs_inode *inode, pgoff_t
>  	}
>  
>  	folio_start = folio_pos(folio);
> -	folio_end = folio_pos(folio) + folio_size(folio) - 1;
> +	folio_last = folio_pos(folio) + folio_size(folio) - 1;
>  	/* Wait for any existing ordered extent in the range */
>  	while (1) {
>  		struct btrfs_ordered_extent *ordered;
>  
> -		btrfs_lock_extent(&inode->io_tree, folio_start, folio_end, &cached_state);
> +		btrfs_lock_extent(&inode->io_tree, folio_start, folio_last, &cached_state);
>  		ordered = btrfs_lookup_ordered_range(inode, folio_start, folio_size(folio));
> -		btrfs_unlock_extent(&inode->io_tree, folio_start, folio_end, &cached_state);
> +		btrfs_unlock_extent(&inode->io_tree, folio_start, folio_last, &cached_state);
>  		if (!ordered)
>  			break;
>  
> diff --git a/fs/btrfs/misc.h b/fs/btrfs/misc.h
> index 9cc292402696cc..ff5eac84d819d8 100644
> --- a/fs/btrfs/misc.h
> +++ b/fs/btrfs/misc.h
> @@ -7,6 +7,8 @@
>  #include <linux/bitmap.h>
>  #include <linux/sched.h>
>  #include <linux/wait.h>
> +#include <linux/mm.h>
> +#include <linux/pagemap.h>
>  #include <linux/math64.h>
>  #include <linux/rbtree.h>
>  
> @@ -158,4 +160,9 @@ static inline bool bitmap_test_range_all_zero(const unsigned long *addr,
>  	return (found_set == start + nbits);
>  }
>  
> +static inline u64 folio_end(struct folio *folio)
> +{
> +	return folio_pos(folio) + folio_size(folio);
> +}
> +

Is there a reason we can't propose this is a generic folio helper
function alongside folio_pos() and folio_size()? Too many variables out
there called folio_end from places that would include it?

>  #endif
> -- 
> 2.49.0
>