fsverity: stop using PG_error to track error status

PATCH Superseded Eric Biggers 2 participants 7 messages Oct 28, 2022 – Nov 25, 2022

Subsystems (5)

VFS ext4 file system F2FS file system fscrypt: file system level encryption support +1 more subsystems

lore

Share & tools

lore

Eric Biggers

author maintainer

2022-10-28 17:59 UTC #0 lore raw

PATCH v3 1/1 fsverity: stop using PG_error to track error status

From: Eric Biggers <[email protected]>

As a step towards freeing the PG_error flag for other uses, change ext4 and f2fs to stop using PG_error to track verity errors. Instead, if a verity error occurs, just mark the whole bio as failed. The coarser granularity isn't really a problem since it isn't any worse than what the block layer provides, and errors from a multi-page readahead aren't reported to applications unless a single-page read fails too.

f2fs supports compression, which makes the f2fs changes a bit more complicated than desired, but the basic premise still works.

Signed-off-by Eric Biggers <[email protected]>

---

In v3, I made a small simplification to the f2fs changes. I'm also only sending the fsverity patch now, since the fscrypt one is now upstream.

 fs/ext4/readpage.c |  8 ++----
 fs/f2fs/compress.c | 64 ++++++++++++++++++++++------------------------
 fs/f2fs/data.c     | 48 +++++++++++++++++++---------------
 fs/verity/verify.c | 12 ++++-----
 4 files changed, 67 insertions(+), 65 deletions(-)

fs/ext4/readpage.c +2 -6

index 3d21eae267fca..e604ea4e102b7 100644
@@ -75,14 +75,10 @@ static void __read_end_io(struct bio *bio)
 	bio_for_each_segment_all(bv, bio, iter_all) {
 		page = bv->bv_page;
 
-		/* PG_error was set if verity failed. */
-		if (bio->bi_status || PageError(page)) {
+		if (bio->bi_status)
 			ClearPageUptodate(page);
-			/* will re-read again later */
-			ClearPageError(page);
-		} else {
+		else
 			SetPageUptodate(page);
-		}
 		unlock_page(page);
 	}
 	if (bio->bi_private)

fs/f2fs/compress.c +31 ~~-33~~

96 lines not on this page · raw message

fs/f2fs/data.c +28 ~~-20~~

126 lines not on this page · raw message

fs/verity/verify.c +6 -6

index bde8c9b7d25f6..961ba248021f9 100644
@@ -200,9 +200,8 @@ EXPORT_SYMBOL_GPL(fsverity_verify_page);
  * @bio: the bio to verify
  *
  * Verify a set of pages that have just been read from a verity file.  The pages
- * must be pagecache pages that are still locked and not yet uptodate.  Pages
- * that fail verification are set to the Error state.  Verification is skipped
- * for pages already in the Error state, e.g. due to fscrypt decryption failure.
+ * must be pagecache pages that are still locked and not yet uptodate.  If a
+ * page fails verification, then bio->bi_status is set to an error status.
  *
  * This is a helper function for use by the ->readahead() method of filesystems
  * that issue bios to read data directly into the page cache.  Filesystems that

@@ -244,9 +243,10 @@ void fsverity_verify_bio(struct bio *bio)
 		unsigned long level0_ra_pages =
 			min(max_ra_pages, params->level0_blocks - level0_index);
 
-		if (!PageError(page) &&
-		    !verify_page(inode, vi, req, page, level0_ra_pages))
-			SetPageError(page);
+		if (!verify_page(inode, vi, req, page, level0_ra_pages)) {
+			bio->bi_status = BLK_STS_IOERR;
+			break;
+		}
 	}
 
 	fsverity_free_hash_request(params->hash_alg, req);

base-commit: 247f34f7b80357943234f93f247a1ae6b6c3a740

Signature · 8 lines

2.38.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author maintainer

2022-11-10 08:21 UTC #1 lore raw

Quoted 15 lines · Eric Biggers

From: Eric Biggers <[email protected]>
As a step towards freeing the PG_error flag for other uses, change ext4 and f2fs to stop using PG_error to track verity errors. Instead, if a verity error occurs, just mark the whole bio as failed. The coarser granularity isn't really a problem since it isn't any worse than what the block layer provides, and errors from a multi-page readahead aren't reported to applications unless a single-page read fails too.
f2fs supports compression, which makes the f2fs changes a bit more complicated than desired, but the basic premise still works.
Signed-off-by Eric Biggers <[email protected]>
---

Quoted 8 lines

In v3, I made a small simplification to the f2fs changes. I'm also only sending the fsverity patch now, since the fscrypt one is now upstream.
 fs/ext4/readpage.c |  8 ++----
 fs/f2fs/compress.c | 64 ++++++++++++++++++++++------------------------
 fs/f2fs/data.c     | 48 +++++++++++++++++++---------------
 fs/verity/verify.c | 12 ++++-----
 4 files changed, 67 insertions(+), 65 deletions(-)

I've applied this to the fsverity tree for 6.2.

Reviews would be greatly appreciated, of course.

- Eric

Eric Biggers

author maintainer

2022-11-23 22:19 UTC #2 lore raw

Quoted 25 lines · Eric Biggers

Quoted 15 lines · Eric Biggers
From: Eric Biggers <[email protected]>
As a step towards freeing the PG_error flag for other uses, change ext4 and f2fs to stop using PG_error to track verity errors. Instead, if a verity error occurs, just mark the whole bio as failed. The coarser granularity isn't really a problem since it isn't any worse than what the block layer provides, and errors from a multi-page readahead aren't reported to applications unless a single-page read fails too.
f2fs supports compression, which makes the f2fs changes a bit more complicated than desired, but the basic premise still works.
Signed-off-by Eric Biggers <[email protected]>
---
Quoted 8 lines
In v3, I made a small simplification to the f2fs changes. I'm also only sending the fsverity patch now, since the fscrypt one is now upstream.
 fs/ext4/readpage.c |  8 ++----
 fs/f2fs/compress.c | 64 ++++++++++++++++++++++------------------------
 fs/f2fs/data.c     | 48 +++++++++++++++++++---------------
 fs/verity/verify.c | 12 ++++-----
 4 files changed, 67 insertions(+), 65 deletions(-)

Quoted 4 lines

I've applied this to the fsverity tree for 6.2.
Reviews would be greatly appreciated, of course.

Jaegeuk and Chao, can I get a review or ack from one of you?

- Eric

Chao Yu

maintainer

2022-11-25 03:06 UTC #3 lore raw

Quoted 17 lines · Eric Biggers

Quoted 16 lines · Eric Biggers
Quoted 15 lines · Eric Biggers
From: Eric Biggers <[email protected]>
As a step towards freeing the PG_error flag for other uses, change ext4 and f2fs to stop using PG_error to track verity errors. Instead, if a verity error occurs, just mark the whole bio as failed. The coarser granularity isn't really a problem since it isn't any worse than what the block layer provides, and errors from a multi-page readahead aren't reported to applications unless a single-page read fails too.
f2fs supports compression, which makes the f2fs changes a bit more complicated than desired, but the basic premise still works.
Signed-off-by Eric Biggers <[email protected]>
---

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
In v3, I made a small simplification to the f2fs changes. I'm also only sending the fsverity patch now, since the fscrypt one is now upstream.
  fs/ext4/readpage.c |  8 ++----
  fs/f2fs/compress.c | 64 ++++++++++++++++++++++------------------------
  fs/f2fs/data.c     | 48 +++++++++++++++++++---------------

Hi Eric,

Result of "grep PageError fs/f2fs/* -n"

...

fs/f2fs/gc.c:1364:      ClearPageError(page);
fs/f2fs/inline.c:177:   ClearPageError(page);
fs/f2fs/node.c:1649:    ClearPageError(page);

fs/f2fs/node.c:2078: if (TestClearPageError(page))
fs/f2fs/segment.c:3406: ClearPageError(page);

Any plan to remove above PG_error flag operations? Maybe in a separated patch?

Thanks,

Quoted 11 lines

Quoted 7 lines
Quoted 2 lines
  fs/verity/verify.c | 12 ++++-----
  4 files changed, 67 insertions(+), 65 deletions(-)
I've applied this to the fsverity tree for 6.2.
Reviews would be greatly appreciated, of course.
Jaegeuk and Chao, can I get a review or ack from one of you?
- Eric

Eric Biggers

author maintainer

2022-11-25 03:09 UTC #4 lore raw

Quoted 29 lines · Chao Yu

Quoted 17 lines · Eric Biggers
Quoted 16 lines · Eric Biggers
Quoted 15 lines · Eric Biggers
From: Eric Biggers <[email protected]>
As a step towards freeing the PG_error flag for other uses, change ext4 and f2fs to stop using PG_error to track verity errors. Instead, if a verity error occurs, just mark the whole bio as failed. The coarser granularity isn't really a problem since it isn't any worse than what the block layer provides, and errors from a multi-page readahead aren't reported to applications unless a single-page read fails too.
f2fs supports compression, which makes the f2fs changes a bit more complicated than desired, but the basic premise still works.
Signed-off-by Eric Biggers <[email protected]>
---
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
In v3, I made a small simplification to the f2fs changes. I'm also only sending the fsverity patch now, since the fscrypt one is now upstream.
  fs/ext4/readpage.c |  8 ++----
  fs/f2fs/compress.c | 64 ++++++++++++++++++++++------------------------
  fs/f2fs/data.c     | 48 +++++++++++++++++++---------------
Hi Eric,
Result of "grep PageError fs/f2fs/* -n"

Quoted 9 lines

...
fs/f2fs/gc.c:1364:      ClearPageError(page);
fs/f2fs/inline.c:177:   ClearPageError(page);
fs/f2fs/node.c:1649:    ClearPageError(page);
fs/f2fs/node.c:2078: if (TestClearPageError(page))
fs/f2fs/segment.c:3406: ClearPageError(page);
Any plan to remove above PG_error flag operations? Maybe in a separated patch?

Those are all for writes, not reads. So I didn't want to touch them in this patch, which is only about reads.

- Eric

Chao Yu

maintainer Reviewed-by on v3 1/1

2022-11-25 03:36 UTC #5 lore raw

In reply to Eric Biggers

fs/f2fs/data.c struct bio_post_read_ctx { · line 119

Quoted 5 lines · Eric Biggers

fs/f2fs/data.c +1 -0

@@ -116,43 +116,51 @@ struct bio_post_read_ctx {
  	struct f2fs_sb_info *sbi;
  	struct work_struct work;
  	unsigned int enabled_steps;
+	bool decompression_attempted;

How about adding some comments for decompression_attempted? Otherwise it looks good to me.

Reviewed-by Chao Yu <[email protected]>

Thanks,

Eric Biggers

author maintainer

2022-11-25 19:22 UTC #6 lore raw

Quoted 10 lines · Chao Yu

fs/f2fs/data.c struct bio_post_read_ctx { · line 119
Quoted 5 lines · Eric Biggers
fs/f2fs/data.c +1 -0
@@ -116,43 +116,51 @@ struct bio_post_read_ctx {
  	struct f2fs_sb_info *sbi;
  	struct work_struct work;
  	unsigned int enabled_steps;
+	bool decompression_attempted;
How about adding some comments for decompression_attempted? Otherwise it looks good to me.

I added the following:

/*

* decompression_attempted keeps track of whether
* f2fs_end_read_compressed_page() has been called on the pages in the
* bio that belong to a compressed cluster yet.
*/

- Eric