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

[Boris Burkov <boris@bur.io>]

On Tue, Jun 03, 2025 at 09:36:59PM +0200, David Sterba wrote:
> On Tue, Jun 03, 2025 at 11:54:42AM -0700, Boris Burkov wrote:
> > 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.
> 
> Ok, then the "-1" should be renamed to _last, so it's "last byte of the
> range", but unfortunatelly the "_end" suffix could mean the same.
> 

Agreed that it's still not supremely clear. Whatever you decide you
can add

Reviewed-by: Boris Burkov <boris@bur.io>

to both patches.

> > >  	struct extent_state *cached_state = NULL;
> > >  	struct folio *folio;
> > >  	int ret;
> > > 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
> > > @@ -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()?
> 
> We can eventually. The difference is that now it's a local convenience
> helper while in MM it would be part of the API and I haven't done the
> extensive research of it's use.
> 
> A quick grep (folio_size + folio_end) in fs/ shows
> 
>      24 btrfs
>       4 iomap
>       4 ext4
>       2 xfs
>       2 netfs
>       1 gfs2
>       1 f2fs
>       1 buffer.c
>       1 bcachefs
> 
> where bcachefs has its own helper folio_end_pos() with 19 uses. In mm/
> there are 2.
> 

OK, thanks for looking into it. It seems like it's not a crazy change
either way, without too many relevant users.

> > Too many variables out
> > there called folio_end from places that would include it?
> 
> I found only a handful of cases, so should be doable.