Re: Re: [PATCH 3/4] btrfs: convert relocate_one_page() to relocate_one_folio()

[Goldwyn Rodrigues <rgoldwyn@suse.de>]

On 21:52 22/01, David Sterba wrote:
> On Thu, Jan 18, 2024 at 01:46:39PM -0600, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > Convert page references to folios and call the respective folio
> > functions.
> > 
> > Since find_or_create_page() takes a mask argument, call
> > __filemap_get_folio() instead of filemap_grab_folio().
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> >  fs/btrfs/relocation.c | 87 ++++++++++++++++++++++---------------------
> >  1 file changed, 44 insertions(+), 43 deletions(-)
> > 
> > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> > index c365bfc60652..f4fd4257adae 100644
> > --- a/fs/btrfs/relocation.c
> > +++ b/fs/btrfs/relocation.c
> > @@ -2849,7 +2849,7 @@ static noinline_for_stack int prealloc_file_extent_cluster(
> >  	 * btrfs_do_readpage() call of previously relocated file cluster.
> >  	 *
> >  	 * If the current cluster starts in the above range, btrfs_do_readpage()
> > -	 * will skip the read, and relocate_one_page() will later writeback
> > +	 * will skip the read, and relocate_one_folio() will later writeback
> >  	 * the padding zeros as new data, causing data corruption.
> >  	 *
> >  	 * Here we have to manually invalidate the range (i_size, PAGE_END + 1).
> > @@ -2983,68 +2983,69 @@ static u64 get_cluster_boundary_end(const struct file_extent_cluster *cluster,
> >  	return cluster->boundary[cluster_nr + 1] - 1;
> >  }
> >  
> > -static int relocate_one_page(struct inode *inode, struct file_ra_state *ra,
> > +static int relocate_one_folio(struct inode *inode, struct file_ra_state *ra,
> >  			     const struct file_extent_cluster *cluster,
> > -			     int *cluster_nr, unsigned long page_index)
> > +			     int *cluster_nr, unsigned long index)
> >  {
> >  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> >  	u64 offset = BTRFS_I(inode)->index_cnt;
> >  	const unsigned long last_index = (cluster->end - offset) >> PAGE_SHIFT;
> >  	gfp_t mask = btrfs_alloc_write_mask(inode->i_mapping);
> > -	struct page *page;
> > -	u64 page_start;
> > -	u64 page_end;
> > +	struct folio *folio;
> > +	u64 start;
> > +	u64 end;
> >  	u64 cur;
> >  	int ret;
> >  
> > -	ASSERT(page_index <= last_index);
> > -	page = find_lock_page(inode->i_mapping, page_index);
> > -	if (!page) {
> > +	ASSERT(index <= last_index);
> > +	folio = filemap_lock_folio(inode->i_mapping, index);
> > +	if (IS_ERR(folio)) {
> >  		page_cache_sync_readahead(inode->i_mapping, ra, NULL,
> 
> How do page_cache_sync_readahead and folios interact? We still have
> page == folio but the large folios are on the way, so do we need
> something to make it work? If there's an assumption about pages and
> folios this could be turned to an assertion so we don't forget about
> that later.

For now page and folio are the same and the assumption is folio size is
PAGE_SIZE. I am adding WARN_ON(folio_order(folio)) after an uptodate
folio is received to warn if the folio size changes.

-- 
Goldwyn