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

[David Sterba <dsterba@suse.cz>]

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.

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

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