linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] btrfs: Use IS_ERR() instead of checking folio for NULL
       [not found] <cover.1705605787.git.rgoldwyn@suse.com>
@ 2024-01-18 19:46 ` Goldwyn Rodrigues
  2024-01-18 21:31   ` Boris Burkov
  2024-01-19 14:53   ` David Sterba
  2024-01-18 19:46 ` [PATCH 2/4] btrfs: page to folio conversion: prealloc_file_extent_cluster() Goldwyn Rodrigues
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 15+ messages in thread
From: Goldwyn Rodrigues @ 2024-01-18 19:46 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goldwyn Rodrigues

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

__filemap_get_folio() returns an error instead of a NULL pointer. Use
IS_ERR() to check if folio is not returned.

As we are fixing this, use set_folio_extent_mapped() instead of
set_page_extent_mapped().

Fixes: f8809b1f6a3e btrfs: page to folio conversion in btrfs_truncate_block()
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 7199670599d9..25090d23834b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4714,7 +4714,7 @@ int btrfs_truncate_block(struct btrfs_inode *inode, loff_t from, loff_t len,
 again:
 	folio = __filemap_get_folio(mapping, index,
 				    FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
-	if (!folio) {
+	if (IS_ERR(folio)) {
 		btrfs_delalloc_release_space(inode, data_reserved, block_start,
 					     blocksize, true);
 		btrfs_delalloc_release_extents(inode, blocksize);
@@ -4742,7 +4742,7 @@ int btrfs_truncate_block(struct btrfs_inode *inode, loff_t from, loff_t len,
 	 * folio private, but left the page in the mapping.  Set the page mapped
 	 * here to make sure it's properly set for the subpage stuff.
 	 */
-	ret = set_page_extent_mapped(&folio->page);
+	ret = set_folio_extent_mapped(folio);
 	if (ret < 0)
 		goto out_unlock;
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/4] btrfs: page to folio conversion: prealloc_file_extent_cluster()
       [not found] <cover.1705605787.git.rgoldwyn@suse.com>
  2024-01-18 19:46 ` [PATCH 1/4] btrfs: Use IS_ERR() instead of checking folio for NULL Goldwyn Rodrigues
@ 2024-01-18 19:46 ` Goldwyn Rodrigues
  2024-01-18 21:34   ` Boris Burkov
  2024-01-22 20:44   ` David Sterba
  2024-01-18 19:46 ` [PATCH 3/4] btrfs: convert relocate_one_page() to relocate_one_folio() Goldwyn Rodrigues
  2024-01-18 19:46 ` [PATCH 4/4] btrfs: page to folio conversion in put_file_data() Goldwyn Rodrigues
  3 siblings, 2 replies; 15+ messages in thread
From: Goldwyn Rodrigues @ 2024-01-18 19:46 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goldwyn Rodrigues

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Convert usage of page to folio in prealloc_file_extent_cluster(). This converts
all page-based function calls to folio-based.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/relocation.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index abe594f77f99..c365bfc60652 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2858,7 +2858,7 @@ static noinline_for_stack int prealloc_file_extent_cluster(
 		struct address_space *mapping = inode->vfs_inode.i_mapping;
 		struct btrfs_fs_info *fs_info = inode->root->fs_info;
 		const u32 sectorsize = fs_info->sectorsize;
-		struct page *page;
+		struct folio *folio;
 
 		ASSERT(sectorsize < PAGE_SIZE);
 		ASSERT(IS_ALIGNED(i_size, sectorsize));
@@ -2889,16 +2889,16 @@ static noinline_for_stack int prealloc_file_extent_cluster(
 		clear_extent_bits(&inode->io_tree, i_size,
 				  round_up(i_size, PAGE_SIZE) - 1,
 				  EXTENT_UPTODATE);
-		page = find_lock_page(mapping, i_size >> PAGE_SHIFT);
+		folio = filemap_lock_folio(mapping, i_size >> PAGE_SHIFT);
 		/*
 		 * If page is freed we don't need to do anything then, as we
 		 * will re-read the whole page anyway.
 		 */
-		if (page) {
-			btrfs_subpage_clear_uptodate(fs_info, page_folio(page), i_size,
+		if (!IS_ERR(folio)) {
+			btrfs_subpage_clear_uptodate(fs_info, folio, i_size,
 					round_up(i_size, PAGE_SIZE) - i_size);
-			unlock_page(page);
-			put_page(page);
+			folio_unlock(folio);
+			folio_put(folio);
 		}
 	}
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/4] btrfs: convert relocate_one_page() to relocate_one_folio()
       [not found] <cover.1705605787.git.rgoldwyn@suse.com>
  2024-01-18 19:46 ` [PATCH 1/4] btrfs: Use IS_ERR() instead of checking folio for NULL Goldwyn Rodrigues
  2024-01-18 19:46 ` [PATCH 2/4] btrfs: page to folio conversion: prealloc_file_extent_cluster() Goldwyn Rodrigues
@ 2024-01-18 19:46 ` Goldwyn Rodrigues
  2024-01-18 21:43   ` Boris Burkov
  2024-01-22 20:52   ` David Sterba
  2024-01-18 19:46 ` [PATCH 4/4] btrfs: page to folio conversion in put_file_data() Goldwyn Rodrigues
  3 siblings, 2 replies; 15+ messages in thread
From: Goldwyn Rodrigues @ 2024-01-18 19:46 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goldwyn Rodrigues

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,
-				page_index, last_index + 1 - page_index);
-		page = find_or_create_page(inode->i_mapping, page_index, mask);
-		if (!page)
-			return -ENOMEM;
+				index, last_index + 1 - index);
+		folio = __filemap_get_folio(inode->i_mapping, index,
+				FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
+		if (IS_ERR(folio))
+			return PTR_ERR(folio);
 	}
 
-	if (PageReadahead(page))
+	if (folio_test_readahead(folio))
 		page_cache_async_readahead(inode->i_mapping, ra, NULL,
-				page_folio(page), page_index,
-				last_index + 1 - page_index);
+				folio, index,
+				last_index + 1 - index);
 
-	if (!PageUptodate(page)) {
-		btrfs_read_folio(NULL, page_folio(page));
-		lock_page(page);
-		if (!PageUptodate(page)) {
+	if (!folio_test_uptodate(folio)) {
+		btrfs_read_folio(NULL, folio);
+		folio_lock(folio);
+		if (!folio_test_uptodate(folio)) {
 			ret = -EIO;
-			goto release_page;
+			goto release;
 		}
 	}
 
 	/*
-	 * We could have lost page private when we dropped the lock to read the
-	 * page above, make sure we set_page_extent_mapped here so we have any
+	 * We could have lost folio private when we dropped the lock to read the
+	 * folio above, make sure we set_page_extent_mapped here so we have any
 	 * of the subpage blocksize stuff we need in place.
 	 */
-	ret = set_page_extent_mapped(page);
+	ret = set_folio_extent_mapped(folio);
 	if (ret < 0)
-		goto release_page;
+		goto release;
 
-	page_start = page_offset(page);
-	page_end = page_start + PAGE_SIZE - 1;
+	start = folio_pos(folio);
+	end = start + PAGE_SIZE - 1;
 
 	/*
 	 * Start from the cluster, as for subpage case, the cluster can start
-	 * inside the page.
+	 * inside the folio.
 	 */
-	cur = max(page_start, cluster->boundary[*cluster_nr] - offset);
-	while (cur <= page_end) {
+	cur = max(start, cluster->boundary[*cluster_nr] - offset);
+	while (cur <= end) {
 		struct extent_state *cached_state = NULL;
 		u64 extent_start = cluster->boundary[*cluster_nr] - offset;
 		u64 extent_end = get_cluster_boundary_end(cluster,
 						*cluster_nr) - offset;
-		u64 clamped_start = max(page_start, extent_start);
-		u64 clamped_end = min(page_end, extent_end);
+		u64 clamped_start = max(start, extent_start);
+		u64 clamped_end = min(end, extent_end);
 		u32 clamped_len = clamped_end + 1 - clamped_start;
 
 		/* Reserve metadata for this range */
@@ -3052,7 +3053,7 @@ static int relocate_one_page(struct inode *inode, struct file_ra_state *ra,
 						      clamped_len, clamped_len,
 						      false);
 		if (ret)
-			goto release_page;
+			goto release;
 
 		/* Mark the range delalloc and dirty for later writeback */
 		lock_extent(&BTRFS_I(inode)->io_tree, clamped_start, clamped_end,
@@ -3068,20 +3069,20 @@ static int relocate_one_page(struct inode *inode, struct file_ra_state *ra,
 							clamped_len, true);
 			btrfs_delalloc_release_extents(BTRFS_I(inode),
 						       clamped_len);
-			goto release_page;
+			goto release;
 		}
-		btrfs_folio_set_dirty(fs_info, page_folio(page),
+		btrfs_folio_set_dirty(fs_info, folio,
 				      clamped_start, clamped_len);
 
 		/*
-		 * Set the boundary if it's inside the page.
+		 * Set the boundary if it's inside the folio.
 		 * Data relocation requires the destination extents to have the
 		 * same size as the source.
 		 * EXTENT_BOUNDARY bit prevents current extent from being merged
 		 * with previous extent.
 		 */
 		if (in_range(cluster->boundary[*cluster_nr] - offset,
-			     page_start, PAGE_SIZE)) {
+			     start, PAGE_SIZE)) {
 			u64 boundary_start = cluster->boundary[*cluster_nr] -
 						offset;
 			u64 boundary_end = boundary_start +
@@ -3104,8 +3105,8 @@ static int relocate_one_page(struct inode *inode, struct file_ra_state *ra,
 				break;
 		}
 	}
-	unlock_page(page);
-	put_page(page);
+	folio_unlock(folio);
+	folio_put(folio);
 
 	balance_dirty_pages_ratelimited(inode->i_mapping);
 	btrfs_throttle(fs_info);
@@ -3113,9 +3114,9 @@ static int relocate_one_page(struct inode *inode, struct file_ra_state *ra,
 		ret = -ECANCELED;
 	return ret;
 
-release_page:
-	unlock_page(page);
-	put_page(page);
+release:
+	folio_unlock(folio);
+	folio_put(folio);
 	return ret;
 }
 
@@ -3150,7 +3151,7 @@ static int relocate_file_extent_cluster(struct inode *inode,
 	last_index = (cluster->end - offset) >> PAGE_SHIFT;
 	for (index = (cluster->start - offset) >> PAGE_SHIFT;
 	     index <= last_index && !ret; index++)
-		ret = relocate_one_page(inode, ra, cluster, &cluster_nr, index);
+		ret = relocate_one_folio(inode, ra, cluster, &cluster_nr, index);
 	if (ret == 0)
 		WARN_ON(cluster_nr != cluster->nr);
 out:
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/4] btrfs: page to folio conversion in put_file_data()
       [not found] <cover.1705605787.git.rgoldwyn@suse.com>
                   ` (2 preceding siblings ...)
  2024-01-18 19:46 ` [PATCH 3/4] btrfs: convert relocate_one_page() to relocate_one_folio() Goldwyn Rodrigues
@ 2024-01-18 19:46 ` Goldwyn Rodrigues
  2024-01-18 21:48   ` Boris Burkov
  2024-01-22 20:55   ` David Sterba
  3 siblings, 2 replies; 15+ messages in thread
From: Goldwyn Rodrigues @ 2024-01-18 19:46 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goldwyn Rodrigues

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Use folio instead of page in put_file_data(). This converts usage of all
page based functions to folio-based.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/send.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 7902298c1f25..0de3d4163f6b 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -5257,10 +5257,11 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
 {
 	struct btrfs_root *root = sctx->send_root;
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct page *page;
+	struct folio *folio;
 	pgoff_t index = offset >> PAGE_SHIFT;
 	pgoff_t last_index;
 	unsigned pg_offset = offset_in_page(offset);
+	struct address_space *mapping = sctx->cur_inode->i_mapping;
 	int ret;
 
 	ret = put_data_header(sctx, len);
@@ -5273,44 +5274,43 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
 		unsigned cur_len = min_t(unsigned, len,
 					 PAGE_SIZE - pg_offset);
 
-		page = find_lock_page(sctx->cur_inode->i_mapping, index);
-		if (!page) {
-			page_cache_sync_readahead(sctx->cur_inode->i_mapping,
+		folio = filemap_lock_folio(mapping, index);
+		if (IS_ERR(folio)) {
+			page_cache_sync_readahead(mapping,
 						  &sctx->ra, NULL, index,
 						  last_index + 1 - index);
 
-			page = find_or_create_page(sctx->cur_inode->i_mapping,
-						   index, GFP_KERNEL);
-			if (!page) {
-				ret = -ENOMEM;
+	                folio = filemap_grab_folio(mapping, index);
+			if (IS_ERR(folio)) {
+				ret = PTR_ERR(folio);
 				break;
 			}
 		}
 
-		if (PageReadahead(page))
-			page_cache_async_readahead(sctx->cur_inode->i_mapping,
-						   &sctx->ra, NULL, page_folio(page),
+		if (folio_test_readahead(folio))
+			page_cache_async_readahead(mapping,
+						   &sctx->ra, NULL, folio,
 						   index, last_index + 1 - index);
 
-		if (!PageUptodate(page)) {
-			btrfs_read_folio(NULL, page_folio(page));
-			lock_page(page);
-			if (!PageUptodate(page)) {
-				unlock_page(page);
+		if (!folio_test_uptodate(folio)) {
+			btrfs_read_folio(NULL, folio);
+			folio_lock(folio);
+			if (!folio_test_uptodate(folio)) {
+				folio_unlock(folio);
 				btrfs_err(fs_info,
 			"send: IO error at offset %llu for inode %llu root %llu",
-					page_offset(page), sctx->cur_ino,
+					folio_pos(folio), sctx->cur_ino,
 					sctx->send_root->root_key.objectid);
-				put_page(page);
+				folio_put(folio);
 				ret = -EIO;
 				break;
 			}
 		}
 
-		memcpy_from_page(sctx->send_buf + sctx->send_size, page,
+		memcpy_from_folio(sctx->send_buf + sctx->send_size, folio,
 				 pg_offset, cur_len);
-		unlock_page(page);
-		put_page(page);
+		folio_unlock(folio);
+		folio_put(folio);
 		index++;
 		pg_offset = 0;
 		len -= cur_len;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] btrfs: Use IS_ERR() instead of checking folio for NULL
  2024-01-18 19:46 ` [PATCH 1/4] btrfs: Use IS_ERR() instead of checking folio for NULL Goldwyn Rodrigues
@ 2024-01-18 21:31   ` Boris Burkov
  2024-01-19 14:53   ` David Sterba
  1 sibling, 0 replies; 15+ messages in thread
From: Boris Burkov @ 2024-01-18 21:31 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues

On Thu, Jan 18, 2024 at 01:46:37PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> __filemap_get_folio() returns an error instead of a NULL pointer. Use
> IS_ERR() to check if folio is not returned.
> 
> As we are fixing this, use set_folio_extent_mapped() instead of
> set_page_extent_mapped().

nit:
I would change the commit message to something like:
"btrfs_truncate_block folio vs page fixes"

So that it is exactly what it says on the tin, and the second change
isn't a "sneaky" one.

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

> 
> Fixes: f8809b1f6a3e btrfs: page to folio conversion in btrfs_truncate_block()
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/inode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 7199670599d9..25090d23834b 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4714,7 +4714,7 @@ int btrfs_truncate_block(struct btrfs_inode *inode, loff_t from, loff_t len,
>  again:
>  	folio = __filemap_get_folio(mapping, index,
>  				    FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
> -	if (!folio) {
> +	if (IS_ERR(folio)) {
>  		btrfs_delalloc_release_space(inode, data_reserved, block_start,
>  					     blocksize, true);
>  		btrfs_delalloc_release_extents(inode, blocksize);
> @@ -4742,7 +4742,7 @@ int btrfs_truncate_block(struct btrfs_inode *inode, loff_t from, loff_t len,
>  	 * folio private, but left the page in the mapping.  Set the page mapped
>  	 * here to make sure it's properly set for the subpage stuff.
>  	 */
> -	ret = set_page_extent_mapped(&folio->page);
> +	ret = set_folio_extent_mapped(folio);
>  	if (ret < 0)
>  		goto out_unlock;
>  
> -- 
> 2.43.0
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] btrfs: page to folio conversion: prealloc_file_extent_cluster()
  2024-01-18 19:46 ` [PATCH 2/4] btrfs: page to folio conversion: prealloc_file_extent_cluster() Goldwyn Rodrigues
@ 2024-01-18 21:34   ` Boris Burkov
  2024-01-22 20:44   ` David Sterba
  1 sibling, 0 replies; 15+ messages in thread
From: Boris Burkov @ 2024-01-18 21:34 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues

On Thu, Jan 18, 2024 at 01:46:38PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Convert usage of page to folio in prealloc_file_extent_cluster(). This converts
> all page-based function calls to folio-based.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Reviewed-by: Boris Burkov <boris@bur.io>
> ---
>  fs/btrfs/relocation.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index abe594f77f99..c365bfc60652 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -2858,7 +2858,7 @@ static noinline_for_stack int prealloc_file_extent_cluster(
>  		struct address_space *mapping = inode->vfs_inode.i_mapping;
>  		struct btrfs_fs_info *fs_info = inode->root->fs_info;
>  		const u32 sectorsize = fs_info->sectorsize;
> -		struct page *page;
> +		struct folio *folio;
>  
>  		ASSERT(sectorsize < PAGE_SIZE);
>  		ASSERT(IS_ALIGNED(i_size, sectorsize));
> @@ -2889,16 +2889,16 @@ static noinline_for_stack int prealloc_file_extent_cluster(
>  		clear_extent_bits(&inode->io_tree, i_size,
>  				  round_up(i_size, PAGE_SIZE) - 1,
>  				  EXTENT_UPTODATE);
> -		page = find_lock_page(mapping, i_size >> PAGE_SHIFT);
> +		folio = filemap_lock_folio(mapping, i_size >> PAGE_SHIFT);
>  		/*
>  		 * If page is freed we don't need to do anything then, as we
>  		 * will re-read the whole page anyway.
>  		 */
> -		if (page) {
> -			btrfs_subpage_clear_uptodate(fs_info, page_folio(page), i_size,
> +		if (!IS_ERR(folio)) {
> +			btrfs_subpage_clear_uptodate(fs_info, folio, i_size,
>  					round_up(i_size, PAGE_SIZE) - i_size);
> -			unlock_page(page);
> -			put_page(page);
> +			folio_unlock(folio);
> +			folio_put(folio);
>  		}
>  	}
>  
> -- 
> 2.43.0
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] btrfs: convert relocate_one_page() to relocate_one_folio()
  2024-01-18 19:46 ` [PATCH 3/4] btrfs: convert relocate_one_page() to relocate_one_folio() Goldwyn Rodrigues
@ 2024-01-18 21:43   ` Boris Burkov
  2024-01-22 20:53     ` David Sterba
  2024-01-22 20:52   ` David Sterba
  1 sibling, 1 reply; 15+ messages in thread
From: Boris Burkov @ 2024-01-18 21:43 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues

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;

This patch throws out this function labelling the start/index/end with
'page_' which I think was pretty useful given the other starts/ends like
'extent_' and 'clamped_'. Namespacing the indices makes the code easier
to follow, IMO.

>  	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,
> -				page_index, last_index + 1 - page_index);
> -		page = find_or_create_page(inode->i_mapping, page_index, mask);
> -		if (!page)
> -			return -ENOMEM;
> +				index, last_index + 1 - index);
> +		folio = __filemap_get_folio(inode->i_mapping, index,
> +				FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
> +		if (IS_ERR(folio))
> +			return PTR_ERR(folio);
>  	}
>  
> -	if (PageReadahead(page))
> +	if (folio_test_readahead(folio))
>  		page_cache_async_readahead(inode->i_mapping, ra, NULL,
> -				page_folio(page), page_index,
> -				last_index + 1 - page_index);
> +				folio, index,
> +				last_index + 1 - index);
>  
> -	if (!PageUptodate(page)) {
> -		btrfs_read_folio(NULL, page_folio(page));
> -		lock_page(page);
> -		if (!PageUptodate(page)) {
> +	if (!folio_test_uptodate(folio)) {
> +		btrfs_read_folio(NULL, folio);
> +		folio_lock(folio);
> +		if (!folio_test_uptodate(folio)) {
>  			ret = -EIO;
> -			goto release_page;
> +			goto release;
>  		}
>  	}
>  
>  	/*
> -	 * We could have lost page private when we dropped the lock to read the
> -	 * page above, make sure we set_page_extent_mapped here so we have any
> +	 * We could have lost folio private when we dropped the lock to read the
> +	 * folio above, make sure we set_page_extent_mapped here so we have any
>  	 * of the subpage blocksize stuff we need in place.
>  	 */
> -	ret = set_page_extent_mapped(page);
> +	ret = set_folio_extent_mapped(folio);
>  	if (ret < 0)
> -		goto release_page;
> +		goto release;
>  
> -	page_start = page_offset(page);
> -	page_end = page_start + PAGE_SIZE - 1;
> +	start = folio_pos(folio);
> +	end = start + PAGE_SIZE - 1;
>  
>  	/*
>  	 * Start from the cluster, as for subpage case, the cluster can start
> -	 * inside the page.
> +	 * inside the folio.
>  	 */
> -	cur = max(page_start, cluster->boundary[*cluster_nr] - offset);
> -	while (cur <= page_end) {
> +	cur = max(start, cluster->boundary[*cluster_nr] - offset);
> +	while (cur <= end) {
>  		struct extent_state *cached_state = NULL;
>  		u64 extent_start = cluster->boundary[*cluster_nr] - offset;
>  		u64 extent_end = get_cluster_boundary_end(cluster,
>  						*cluster_nr) - offset;
> -		u64 clamped_start = max(page_start, extent_start);
> -		u64 clamped_end = min(page_end, extent_end);
> +		u64 clamped_start = max(start, extent_start);
> +		u64 clamped_end = min(end, extent_end);

e.g., I think these lines lose clarity from s/page_start/start/

>  		u32 clamped_len = clamped_end + 1 - clamped_start;
>  
>  		/* Reserve metadata for this range */
> @@ -3052,7 +3053,7 @@ static int relocate_one_page(struct inode *inode, struct file_ra_state *ra,
>  						      clamped_len, clamped_len,
>  						      false);
>  		if (ret)
> -			goto release_page;
> +			goto release;
>  
>  		/* Mark the range delalloc and dirty for later writeback */
>  		lock_extent(&BTRFS_I(inode)->io_tree, clamped_start, clamped_end,
> @@ -3068,20 +3069,20 @@ static int relocate_one_page(struct inode *inode, struct file_ra_state *ra,
>  							clamped_len, true);
>  			btrfs_delalloc_release_extents(BTRFS_I(inode),
>  						       clamped_len);
> -			goto release_page;
> +			goto release;
>  		}
> -		btrfs_folio_set_dirty(fs_info, page_folio(page),
> +		btrfs_folio_set_dirty(fs_info, folio,
>  				      clamped_start, clamped_len);

Does this fit one line now?

>  
>  		/*
> -		 * Set the boundary if it's inside the page.
> +		 * Set the boundary if it's inside the folio.
>  		 * Data relocation requires the destination extents to have the
>  		 * same size as the source.
>  		 * EXTENT_BOUNDARY bit prevents current extent from being merged
>  		 * with previous extent.
>  		 */
>  		if (in_range(cluster->boundary[*cluster_nr] - offset,
> -			     page_start, PAGE_SIZE)) {
> +			     start, PAGE_SIZE)) {
>  			u64 boundary_start = cluster->boundary[*cluster_nr] -
>  						offset;
>  			u64 boundary_end = boundary_start +
> @@ -3104,8 +3105,8 @@ static int relocate_one_page(struct inode *inode, struct file_ra_state *ra,
>  				break;
>  		}
>  	}
> -	unlock_page(page);
> -	put_page(page);
> +	folio_unlock(folio);
> +	folio_put(folio);
>  
>  	balance_dirty_pages_ratelimited(inode->i_mapping);
>  	btrfs_throttle(fs_info);
> @@ -3113,9 +3114,9 @@ static int relocate_one_page(struct inode *inode, struct file_ra_state *ra,
>  		ret = -ECANCELED;
>  	return ret;
>  
> -release_page:
> -	unlock_page(page);
> -	put_page(page);
> +release:
> +	folio_unlock(folio);
> +	folio_put(folio);
>  	return ret;
>  }
>  
> @@ -3150,7 +3151,7 @@ static int relocate_file_extent_cluster(struct inode *inode,
>  	last_index = (cluster->end - offset) >> PAGE_SHIFT;
>  	for (index = (cluster->start - offset) >> PAGE_SHIFT;
>  	     index <= last_index && !ret; index++)
> -		ret = relocate_one_page(inode, ra, cluster, &cluster_nr, index);
> +		ret = relocate_one_folio(inode, ra, cluster, &cluster_nr, index);
>  	if (ret == 0)
>  		WARN_ON(cluster_nr != cluster->nr);
>  out:
> -- 
> 2.43.0
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] btrfs: page to folio conversion in put_file_data()
  2024-01-18 19:46 ` [PATCH 4/4] btrfs: page to folio conversion in put_file_data() Goldwyn Rodrigues
@ 2024-01-18 21:48   ` Boris Burkov
  2024-01-22 20:55   ` David Sterba
  1 sibling, 0 replies; 15+ messages in thread
From: Boris Burkov @ 2024-01-18 21:48 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues

On Thu, Jan 18, 2024 at 01:46:40PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Use folio instead of page in put_file_data(). This converts usage of all
> page based functions to folio-based.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

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

> ---
>  fs/btrfs/send.c | 42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 7902298c1f25..0de3d4163f6b 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -5257,10 +5257,11 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
>  {
>  	struct btrfs_root *root = sctx->send_root;
>  	struct btrfs_fs_info *fs_info = root->fs_info;
> -	struct page *page;
> +	struct folio *folio;
>  	pgoff_t index = offset >> PAGE_SHIFT;
>  	pgoff_t last_index;
>  	unsigned pg_offset = offset_in_page(offset);
> +	struct address_space *mapping = sctx->cur_inode->i_mapping;
>  	int ret;
>  
>  	ret = put_data_header(sctx, len);
> @@ -5273,44 +5274,43 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
>  		unsigned cur_len = min_t(unsigned, len,
>  					 PAGE_SIZE - pg_offset);
>  
> -		page = find_lock_page(sctx->cur_inode->i_mapping, index);
> -		if (!page) {
> -			page_cache_sync_readahead(sctx->cur_inode->i_mapping,
> +		folio = filemap_lock_folio(mapping, index);
> +		if (IS_ERR(folio)) {
> +			page_cache_sync_readahead(mapping,
>  						  &sctx->ra, NULL, index,
>  						  last_index + 1 - index);
>  
> -			page = find_or_create_page(sctx->cur_inode->i_mapping,
> -						   index, GFP_KERNEL);
> -			if (!page) {
> -				ret = -ENOMEM;
> +	                folio = filemap_grab_folio(mapping, index);
> +			if (IS_ERR(folio)) {
> +				ret = PTR_ERR(folio);
>  				break;
>  			}
>  		}
>  
> -		if (PageReadahead(page))
> -			page_cache_async_readahead(sctx->cur_inode->i_mapping,
> -						   &sctx->ra, NULL, page_folio(page),
> +		if (folio_test_readahead(folio))
> +			page_cache_async_readahead(mapping,
> +						   &sctx->ra, NULL, folio,
>  						   index, last_index + 1 - index);
>  
> -		if (!PageUptodate(page)) {
> -			btrfs_read_folio(NULL, page_folio(page));
> -			lock_page(page);
> -			if (!PageUptodate(page)) {
> -				unlock_page(page);
> +		if (!folio_test_uptodate(folio)) {
> +			btrfs_read_folio(NULL, folio);
> +			folio_lock(folio);
> +			if (!folio_test_uptodate(folio)) {
> +				folio_unlock(folio);
>  				btrfs_err(fs_info,
>  			"send: IO error at offset %llu for inode %llu root %llu",
> -					page_offset(page), sctx->cur_ino,
> +					folio_pos(folio), sctx->cur_ino,
>  					sctx->send_root->root_key.objectid);
> -				put_page(page);
> +				folio_put(folio);
>  				ret = -EIO;
>  				break;
>  			}
>  		}
>  
> -		memcpy_from_page(sctx->send_buf + sctx->send_size, page,
> +		memcpy_from_folio(sctx->send_buf + sctx->send_size, folio,
>  				 pg_offset, cur_len);
> -		unlock_page(page);
> -		put_page(page);
> +		folio_unlock(folio);
> +		folio_put(folio);
>  		index++;
>  		pg_offset = 0;
>  		len -= cur_len;
> -- 
> 2.43.0
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] btrfs: Use IS_ERR() instead of checking folio for NULL
  2024-01-18 19:46 ` [PATCH 1/4] btrfs: Use IS_ERR() instead of checking folio for NULL Goldwyn Rodrigues
  2024-01-18 21:31   ` Boris Burkov
@ 2024-01-19 14:53   ` David Sterba
  2024-01-19 14:58     ` Goldwyn Rodrigues
  1 sibling, 1 reply; 15+ messages in thread
From: David Sterba @ 2024-01-19 14:53 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues

On Thu, Jan 18, 2024 at 01:46:37PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> __filemap_get_folio() returns an error instead of a NULL pointer. Use
> IS_ERR() to check if folio is not returned.
> 
> As we are fixing this, use set_folio_extent_mapped() instead of
> set_page_extent_mapped().
> 
> Fixes: f8809b1f6a3e btrfs: page to folio conversion in btrfs_truncate_block()

This is still in for-next so the fixup should be squashed there.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Re: [PATCH 1/4] btrfs: Use IS_ERR() instead of checking folio for NULL
  2024-01-19 14:53   ` David Sterba
@ 2024-01-19 14:58     ` Goldwyn Rodrigues
  0 siblings, 0 replies; 15+ messages in thread
From: Goldwyn Rodrigues @ 2024-01-19 14:58 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs

On 15:53 19/01, David Sterba wrote:
> On Thu, Jan 18, 2024 at 01:46:37PM -0600, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > __filemap_get_folio() returns an error instead of a NULL pointer. Use
> > IS_ERR() to check if folio is not returned.
> > 
> > As we are fixing this, use set_folio_extent_mapped() instead of
> > set_page_extent_mapped().
> > 
> > Fixes: f8809b1f6a3e btrfs: page to folio conversion in btrfs_truncate_block()
> 
> This is still in for-next so the fixup should be squashed there.

Oh cool. However, this would have to be put after Matthew's patch of
changing set_page_extent_mapped().

-- 
Goldwyn

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] btrfs: page to folio conversion: prealloc_file_extent_cluster()
  2024-01-18 19:46 ` [PATCH 2/4] btrfs: page to folio conversion: prealloc_file_extent_cluster() Goldwyn Rodrigues
  2024-01-18 21:34   ` Boris Burkov
@ 2024-01-22 20:44   ` David Sterba
  1 sibling, 0 replies; 15+ messages in thread
From: David Sterba @ 2024-01-22 20:44 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues

On Thu, Jan 18, 2024 at 01:46:38PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Convert usage of page to folio in prealloc_file_extent_cluster(). This converts
> all page-based function calls to folio-based.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] btrfs: convert relocate_one_page() to relocate_one_folio()
  2024-01-18 19:46 ` [PATCH 3/4] btrfs: convert relocate_one_page() to relocate_one_folio() Goldwyn Rodrigues
  2024-01-18 21:43   ` Boris Burkov
@ 2024-01-22 20:52   ` David Sterba
  2024-01-23 16:36     ` Goldwyn Rodrigues
  1 sibling, 1 reply; 15+ messages in thread
From: David Sterba @ 2024-01-22 20:52 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues

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.

> -				page_index, last_index + 1 - page_index);
> -		page = find_or_create_page(inode->i_mapping, page_index, mask);
> -		if (!page)
> -			return -ENOMEM;
> +				index, last_index + 1 - index);
> +		folio = __filemap_get_folio(inode->i_mapping, index,
> +				FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);

Please format the line continuation so the parameters start under (,
this has been the preferred style, although there's still a lot of the
one/two tabs as next line indentation.

> +		if (IS_ERR(folio))
> +			return PTR_ERR(folio);
>  	}
>  
> -	if (PageReadahead(page))
> +	if (folio_test_readahead(folio))
>  		page_cache_async_readahead(inode->i_mapping, ra, NULL,
> -				page_folio(page), page_index,
> -				last_index + 1 - page_index);
> +				folio, index,
> +				last_index + 1 - index);

Same here

>  
> -	if (!PageUptodate(page)) {
> -		btrfs_read_folio(NULL, page_folio(page));
> -		lock_page(page);
> -		if (!PageUptodate(page)) {
> +	if (!folio_test_uptodate(folio)) {
> +		btrfs_read_folio(NULL, folio);
> +		folio_lock(folio);
> +		if (!folio_test_uptodate(folio)) {
>  			ret = -EIO;
> -			goto release_page;
> +			goto release;
>  		}
>  	}
>  
>  	/*
> -	 * We could have lost page private when we dropped the lock to read the
> -	 * page above, make sure we set_page_extent_mapped here so we have any
> +	 * We could have lost folio private when we dropped the lock to read the
> +	 * folio above, make sure we set_page_extent_mapped here so we have any
>  	 * of the subpage blocksize stuff we need in place.
>  	 */
> -	ret = set_page_extent_mapped(page);
> +	ret = set_folio_extent_mapped(folio);
>  	if (ret < 0)
> -		goto release_page;
> +		goto release;
>  
> -	page_start = page_offset(page);
> -	page_end = page_start + PAGE_SIZE - 1;
> +	start = folio_pos(folio);
> +	end = start + PAGE_SIZE - 1;
>  
>  	/*
>  	 * Start from the cluster, as for subpage case, the cluster can start
> -	 * inside the page.
> +	 * inside the folio.
>  	 */
> -	cur = max(page_start, cluster->boundary[*cluster_nr] - offset);
> -	while (cur <= page_end) {
> +	cur = max(start, cluster->boundary[*cluster_nr] - offset);
> +	while (cur <= end) {
>  		struct extent_state *cached_state = NULL;
>  		u64 extent_start = cluster->boundary[*cluster_nr] - offset;
>  		u64 extent_end = get_cluster_boundary_end(cluster,
>  						*cluster_nr) - offset;
> -		u64 clamped_start = max(page_start, extent_start);
> -		u64 clamped_end = min(page_end, extent_end);
> +		u64 clamped_start = max(start, extent_start);
> +		u64 clamped_end = min(end, extent_end);
>  		u32 clamped_len = clamped_end + 1 - clamped_start;
>  
>  		/* Reserve metadata for this range */
> @@ -3052,7 +3053,7 @@ static int relocate_one_page(struct inode *inode, struct file_ra_state *ra,
>  						      clamped_len, clamped_len,
>  						      false);
>  		if (ret)
> -			goto release_page;
> +			goto release;
>  
>  		/* Mark the range delalloc and dirty for later writeback */
>  		lock_extent(&BTRFS_I(inode)->io_tree, clamped_start, clamped_end,
> @@ -3068,20 +3069,20 @@ static int relocate_one_page(struct inode *inode, struct file_ra_state *ra,
>  							clamped_len, true);
>  			btrfs_delalloc_release_extents(BTRFS_I(inode),
>  						       clamped_len);
> -			goto release_page;
> +			goto release;
>  		}
> -		btrfs_folio_set_dirty(fs_info, page_folio(page),
> +		btrfs_folio_set_dirty(fs_info, folio,
>  				      clamped_start, clamped_len);

This can be joined with the line above

>  
>  		/*
> -		 * Set the boundary if it's inside the page.
> +		 * Set the boundary if it's inside the folio.
>  		 * Data relocation requires the destination extents to have the
>  		 * same size as the source.
>  		 * EXTENT_BOUNDARY bit prevents current extent from being merged
>  		 * with previous extent.
>  		 */
>  		if (in_range(cluster->boundary[*cluster_nr] - offset,
> -			     page_start, PAGE_SIZE)) {
> +			     start, PAGE_SIZE)) {

Can be joined

>  			u64 boundary_start = cluster->boundary[*cluster_nr] -
>  						offset;
>  			u64 boundary_end = boundary_start +

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] btrfs: convert relocate_one_page() to relocate_one_folio()
  2024-01-18 21:43   ` Boris Burkov
@ 2024-01-22 20:53     ` David Sterba
  0 siblings, 0 replies; 15+ messages in thread
From: David Sterba @ 2024-01-22 20:53 UTC (permalink / raw)
  To: Boris Burkov; +Cc: Goldwyn Rodrigues, linux-btrfs, Goldwyn Rodrigues

On Thu, Jan 18, 2024 at 01:43:47PM -0800, Boris Burkov wrote:
> On Thu, Jan 18, 2024 at 01:46:39PM -0600, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > -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;
> 
> This patch throws out this function labelling the start/index/end with
> 'page_' which I think was pretty useful given the other starts/ends like
> 'extent_' and 'clamped_'. Namespacing the indices makes the code easier
> to follow, IMO.

With all the other prefixes around I agree that keeping folio_ (as
replacement of page_) would be useful.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] btrfs: page to folio conversion in put_file_data()
  2024-01-18 19:46 ` [PATCH 4/4] btrfs: page to folio conversion in put_file_data() Goldwyn Rodrigues
  2024-01-18 21:48   ` Boris Burkov
@ 2024-01-22 20:55   ` David Sterba
  1 sibling, 0 replies; 15+ messages in thread
From: David Sterba @ 2024-01-22 20:55 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues

On Thu, Jan 18, 2024 at 01:46:40PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Use folio instead of page in put_file_data(). This converts usage of all
> page based functions to folio-based.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/send.c | 42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 7902298c1f25..0de3d4163f6b 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -5257,10 +5257,11 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
>  {
>  	struct btrfs_root *root = sctx->send_root;
>  	struct btrfs_fs_info *fs_info = root->fs_info;
> -	struct page *page;
> +	struct folio *folio;
>  	pgoff_t index = offset >> PAGE_SHIFT;
>  	pgoff_t last_index;
>  	unsigned pg_offset = offset_in_page(offset);
> +	struct address_space *mapping = sctx->cur_inode->i_mapping;
>  	int ret;
>  
>  	ret = put_data_header(sctx, len);
> @@ -5273,44 +5274,43 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
>  		unsigned cur_len = min_t(unsigned, len,
>  					 PAGE_SIZE - pg_offset);
>  
> -		page = find_lock_page(sctx->cur_inode->i_mapping, index);
> -		if (!page) {
> -			page_cache_sync_readahead(sctx->cur_inode->i_mapping,
> +		folio = filemap_lock_folio(mapping, index);
> +		if (IS_ERR(folio)) {
> +			page_cache_sync_readahead(mapping,
>  						  &sctx->ra, NULL, index,
>  						  last_index + 1 - index);

Same question regarding page cache sync and folios, assertions would be
good or an explanation why it's ok like that. IIRC the explicit sync is
only in some odd code like defrag that does not go though the MM
callbacks so we have to manage things on our own.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Re: [PATCH 3/4] btrfs: convert relocate_one_page() to relocate_one_folio()
  2024-01-22 20:52   ` David Sterba
@ 2024-01-23 16:36     ` Goldwyn Rodrigues
  0 siblings, 0 replies; 15+ messages in thread
From: Goldwyn Rodrigues @ 2024-01-23 16:36 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2024-01-23 16:35 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <cover.1705605787.git.rgoldwyn@suse.com>
2024-01-18 19:46 ` [PATCH 1/4] btrfs: Use IS_ERR() instead of checking folio for NULL Goldwyn Rodrigues
2024-01-18 21:31   ` Boris Burkov
2024-01-19 14:53   ` David Sterba
2024-01-19 14:58     ` Goldwyn Rodrigues
2024-01-18 19:46 ` [PATCH 2/4] btrfs: page to folio conversion: prealloc_file_extent_cluster() Goldwyn Rodrigues
2024-01-18 21:34   ` Boris Burkov
2024-01-22 20:44   ` David Sterba
2024-01-18 19:46 ` [PATCH 3/4] btrfs: convert relocate_one_page() to relocate_one_folio() Goldwyn Rodrigues
2024-01-18 21:43   ` Boris Burkov
2024-01-22 20:53     ` David Sterba
2024-01-22 20:52   ` David Sterba
2024-01-23 16:36     ` Goldwyn Rodrigues
2024-01-18 19:46 ` [PATCH 4/4] btrfs: page to folio conversion in put_file_data() Goldwyn Rodrigues
2024-01-18 21:48   ` Boris Burkov
2024-01-22 20:55   ` David Sterba

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).