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:
> > --- 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()

In truncate_block_zero_beyond_eof if zero_end is folio_end() exactly
then we don't have to do "zero_end - zero_start + 1" in the length
parameter of folio_zero_range() as the -1 and +1 cancel out.

> and btrfs_writepage_fixup_worker,

Here the page_start and page_end are passed to the extent locking where
the end of the range is "-1" but from what I've seen we're using the
"end" in the variable names.

To avoid confusion I'd rename the folio_start and folio_end in
defrag_prepare_one_folio to lock_start and lock_end so the _last
semantic for ranges does exist.

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