[PATCH] btrfs: remove verity Merkle folio from page cache on error

[PATCH] btrfs: remove verity Merkle folio from page cache on error

[Yichong Chen]

btrfs_read_merkle_tree_page() allocates a folio and adds it to the
page cache before reading the Merkle tree data from btree items.

If read_key_bytes() fails, the folio is still locked and present in the
page cache. Drop it from the page cache and unlock it before releasing the
allocation reference so a later read can retry instead of finding a locked,
not-uptodate folio.

Fixes: 06ed09351b67 ("btrfs: convert btrfs_read_merkle_tree_page() to use a folio")
Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
 fs/btrfs/verity.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/verity.c b/fs/btrfs/verity.c
index 983365a73541..99468791a98e 100644
--- a/fs/btrfs/verity.c
+++ b/fs/btrfs/verity.c
@@ -753,6 +753,8 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
 	ret = read_key_bytes(BTRFS_I(inode), BTRFS_VERITY_MERKLE_ITEM_KEY, off,
 			     folio_address(folio), PAGE_SIZE, folio);
 	if (ret < 0) {
+		filemap_remove_folio(folio);
+		folio_unlock(folio);
 		folio_put(folio);
 		return ERR_PTR(ret);
 	}
-- 
2.51.0

Re: [PATCH] btrfs: remove verity Merkle folio from page cache on error

[Yichong Chen]

Hi,

Please ignore this version.

I noticed that this version depends on filemap_remove_folio(). My local
debug tree had this symbol exported, so the patch built there, but exporting
that helper does not look like the right approach for this fix.

I will rework the fix to avoid inserting the folio into the page cache
before the Merkle tree read succeeds, and send a v2.

Sorry for the noise.

Thanks,
Yichong

[PATCH v2] btrfs: add verity Merkle folio to page cache after reading it

[Yichong Chen]

btrfs_read_merkle_tree_page() allocates a folio and adds it to the page
cache before reading the Merkle tree data from btree items.

If read_key_bytes() fails, the folio is still locked and present in the
page cache.  A later read can find the locked, not-uptodate folio instead
of retrying the read.

Avoid installing the folio in the page cache until after the Merkle tree
read has succeeded.  This keeps the failure path simple: the newly
allocated folio is not visible to the page cache yet and only needs to be
released.

Fixes: 06ed09351b67 ("btrfs: convert btrfs_read_merkle_tree_page() to use a folio")
Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
v2:
- Avoid calling filemap_remove_folio(), which is not exported.
- Add the folio to the page cache only after read_key_bytes() succeeds.

 fs/btrfs/verity.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/verity.c b/fs/btrfs/verity.c
index 983365a73541..d705ce3b386a 100644
--- a/fs/btrfs/verity.c
+++ b/fs/btrfs/verity.c
@@ -735,15 +735,6 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
 	if (!folio)
 		return ERR_PTR(-ENOMEM);
 
-	ret = filemap_add_folio(inode->i_mapping, folio, index, GFP_NOFS);
-	if (ret) {
-		folio_put(folio);
-		/* Did someone else insert a folio here? */
-		if (ret == -EEXIST)
-			goto again;
-		return ERR_PTR(ret);
-	}
-
 	/*
 	 * Merkle item keys are indexed from byte 0 in the merkle tree.
 	 * They have the form:
@@ -759,6 +750,15 @@ static struct page *btrfs_read_merkle_tree_page(struct inode *inode,
 	if (ret < PAGE_SIZE)
 		folio_zero_segment(folio, ret, PAGE_SIZE);
 
+	ret = filemap_add_folio(inode->i_mapping, folio, index, GFP_NOFS);
+	if (ret) {
+		folio_put(folio);
+		/* Did someone else insert a folio here? */
+		if (ret == -EEXIST)
+			goto again;
+		return ERR_PTR(ret);
+	}
+
 	folio_mark_uptodate(folio);
 	folio_unlock(folio);
 
-- 
2.51.0