ceph: CephFS writeback correctness and performance fixes

Hello list,

This is v2 of my series that addresses interrelated issues in CephFS writeback, fixing crashes, improving robustness, and correcting performance behavior, particularly for fscrypted files. [1]

Changes v1->v2:
- Clarify patch #1's commit message to establish that failures on the first folio are not possible.
- Add another patch to move the "clean up page array on abort" logic to a new ceph_discard_page_array() function. (Thanks Slava!)
- Change the wording "grossly degraded performance" to instead read "correspondingly degraded performance." This makes the causal relationship clearer (that write throughput is limited much more significantly by write op/s due to the bug) without making any claims (qualitative or otherwise) about significance. (Thanks Slava!)
- Reset locked_pages = 0 immediately when the page array is discarded, simplifying patch #5 ("ceph: Assert writeback loop invariants")
- Reword "as evidenced by the previous two patches which fix oopses" to "as evidenced by two recent patches which fix oopses" and refer to the patches by subject (being in the same series, I cannot refer to them by hash)

I received several items of feedback on v1 that I have chosen not to adopt -- mostly because I believe they run contrary to kernel norms about strong contracts, redundancy, not masking bugs, and regressions. (It is possible that I am mistaken on these norms, and may still include them in a v3 if someone makes good points in favor of them or consensus overrules me.)

Feedback on v1 not adopted:
- "Patch #1, which fixes a crash in unreachable code, should be reordered after patch #6 (#5 in v1), which fixes a bug that makes the code unreachable, in order to simplify crash reproduction in review"
The order of the patchset represents the canonical commit order of the series. Committing patch #6 before patch #1 would therefore introduce a regression, in direct violation of longstanding kernel policy.

- "Patch #1 should not swallow errors from move_dirty_folio_in_page_array() if they happen on the first folio."
It is not possible for move_dirty_folio_in_page_array() to encounter an error on the first folio, and this is not likely to change in the future. Even if such an error were to occur, the caller already expects ceph_process_folio_batch() to "successfully" lock zero pages from time to time. Swallowing errors in ceph_process_folio_batch() is consistent with its design.

- "Patch #1 should include the call trace and reproduction steps in the commit message."
The commit message already explains the execution path to the failure, which is what people really care about (the call trace is just a means to that end). Inlining makes the call trace completely opaque about why the oops happens. Reproduction steps are not particularly valuable for posterity, but a curious party in the future can always refer back to mailing list discussions to find them.

- "Patch #2 should not exist: it removes the return code from ceph_process_folio_batch(), which is essential to indicate failures to the caller."
The caller of ceph_process_folio_batch() only cares about success/failure as a
boolean: the exact error code is discarded. This is redundant with ceph_wbc.locked_pages, which not only indicates success/failure, but also the degree of success (how many pages were locked). This makes ceph_wbc.locked_pages a more appealing single source of truth than the error code.

Further, the error return mechanism is misleading to future programmers: it implies that it is acceptable for ceph_process_folio_batch() to abort the operation (after already selecting some folios for write). The caller cannot handle this. Removing the return code altogether makes the contract explicit, which is the central point of the patch.

- "Patch #5 (#4 in v1) should not introduce BUG_ON() in the writeback path." The writeback path already has BUG_ON(), and this is consistent with kernel
norms: check invariants, fail fast, and don't try to tolerate ambiguity. There are already BUG_ONs that check several invariants, so rather than introducing new failure possibilities to the writeback loop, patch #5 actually catches invariant errors sooner. This is to tighten up the code and prevent regressions, not fix any particular bug.

- "Patch #6 (#5 in v1) should include benchmark results to support its claim of improved performance." My environment is not very representative of a typical Ceph deployment, and benchmarking is tough to get right. I am not prepared to stand behind any
particular estimated/expected speedup factor. Rather, the rationale for this
patch is a simple computer science principle: increasing the amount of useful
work done per operation reduces total overhead. I have changed the phrasing
"grossly degraded performance" to "correspondingly degraded performance" to
emphasize that the performance degradation follows from the bottleneck, without implying that I'm making some kind of claim about magnitude.

Warm regards,
Sam

[1]: https://lore.kernel.org/ceph-devel/[email protected]/T/

Sam Edwards (6):
ceph: Do not propagate page array emplacement errors as batch errors
ceph: Remove error return from ceph_process_folio_batch()
ceph: Free page array when ceph_submit_write fails
ceph: Split out page-array discarding to a function
ceph: Assert writeback loop invariants
ceph: Fix write storm on fscrypted files

 fs/ceph/addr.c | 82 +++++++++++++++++++++++++++++---------------------
 1 file changed, 48 insertions(+), 34 deletions(-)

2.51.2

When fscrypt is enabled, move_dirty_folio_in_page_array() may fail because it needs to allocate bounce buffers to store the encrypted versions of each folio. Each folio beyond the first allocates its bounce buffer with GFP_NOWAIT. Failures are common (and expected) under this allocation mode; they should flush (not abort) the batch.

However, ceph_process_folio_batch() uses the same rc variable for its own return code and for capturing the return codes of its routine calls; failing to reset rc back to 0 results in the error being propagated out to the main writeback loop, which cannot actually tolerate any errors here: once ceph_wbc.pages is allocated, it must be passed to ceph_submit_write() to be freed. If it survives until the next iteration (e.g. due to the goto being followed), ceph_allocate_page_array()'s BUG_ON() will oops the worker. (Subsequent patches in this series make the loop more robust.)

Note that this failure mode is currently masked due to another bug (addressed later in this series) that prevents multiple encrypted folios from being selected for the same write.

For now, just reset rc when redirtying the folio to prevent errors in move_dirty_folio_in_page_array() from propagating. (Note that move_dirty_folio_in_page_array() is careful never to return errors on the first folio, so there is no need to check for that.) After this change, ceph_process_folio_batch() no longer returns errors; its only remaining failure indicator is locked_pages == 0, which the caller already handles correctly. The next patch in this series addresses this.

---
 fs/ceph/addr.c | 1 +
 1 file changed, 1 insertion(+)

index 63b75d214210..3462df35d245 100644
@@ -1369,6 +1369,7 @@ int ceph_process_folio_batch(struct address_space *mapping,
 		rc = move_dirty_folio_in_page_array(mapping, wbc, ceph_wbc,
 				folio);
 		if (rc) {
+			rc = 0;
 			folio_redirty_for_writepage(wbc, folio);
 			folio_unlock(folio);
 			break;

2.51.2

Following the previous patch, ceph_process_folio_batch() no longer returns errors because the writeback loop cannot handle them.

Since this function already indicates failure to lock any pages by leaving ceph_wbc.locked_pages == 0, and the writeback loop has no way to handle abandonment of a locked batch, change the return type of ceph_process_folio_batch() to void and remove the pathological goto in the writeback loop. The lack of a return code emphasizes that ceph_process_folio_batch() is designed to be abort-free: that is, once it commits a folio for writeback, it will not later abandon it or propagate an error for that folio.

---
 fs/ceph/addr.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

index 3462df35d245..2b722916fb9b 100644
@@ -1283,16 +1283,16 @@ static inline int move_dirty_folio_in_page_array(struct address_space *mapping,
 }
 
 static
-int ceph_process_folio_batch(struct address_space *mapping,
-			     struct writeback_control *wbc,
-			     struct ceph_writeback_ctl *ceph_wbc)
+void ceph_process_folio_batch(struct address_space *mapping,
+			      struct writeback_control *wbc,
+			      struct ceph_writeback_ctl *ceph_wbc)
 {
 	struct inode *inode = mapping->host;
 	struct ceph_fs_client *fsc = ceph_inode_to_fs_client(inode);
 	struct ceph_client *cl = fsc->client;
 	struct folio *folio = NULL;
 	unsigned i;
-	int rc = 0;
+	int rc;
 
 	for (i = 0; can_next_page_be_processed(ceph_wbc, i); i++) {
 		folio = ceph_wbc->fbatch.folios[i];

@@ -1322,12 +1322,10 @@ int ceph_process_folio_batch(struct address_space *mapping,
 		rc = ceph_check_page_before_write(mapping, wbc,
 						  ceph_wbc, folio);
 		if (rc == -ENODATA) {
-			rc = 0;
 			folio_unlock(folio);
 			ceph_wbc->fbatch.folios[i] = NULL;
 			continue;
 		} else if (rc == -E2BIG) {
-			rc = 0;
 			folio_unlock(folio);
 			ceph_wbc->fbatch.folios[i] = NULL;
 			break;

@@ -1369,7 +1367,6 @@ int ceph_process_folio_batch(struct address_space *mapping,
 		rc = move_dirty_folio_in_page_array(mapping, wbc, ceph_wbc,
 				folio);
 		if (rc) {
-			rc = 0;
 			folio_redirty_for_writepage(wbc, folio);
 			folio_unlock(folio);
 			break;

@@ -1380,8 +1377,6 @@ int ceph_process_folio_batch(struct address_space *mapping,
 	}
 
 	ceph_wbc->processed_in_fbatch = i;
-
-	return rc;
 }
 
 static inline

@@ -1685,10 +1680,8 @@ static int ceph_writepages_start(struct address_space *mapping,
 			break;
 
 process_folio_batch:
-		rc = ceph_process_folio_batch(mapping, wbc, &ceph_wbc);
+		ceph_process_folio_batch(mapping, wbc, &ceph_wbc);
 		ceph_shift_unused_folios_left(&ceph_wbc.fbatch);
-		if (rc)
-			goto release_folios;
 
 		/* did we get anything? */
 		if (!ceph_wbc.locked_pages)

2.51.2

Hey Slava,

I've tried to clarify this patch a few ways now, but I still haven't seen actionable technical feedback, only misunderstanding or preference rather than concrete issues. I'm making one last, detailed attempt to explain the rationale; if it still isn't clear, I believe our only remaining option is to involve another maintainer to help complete the review.

As the function ceph_process_folio_batch() exists today (i.e. in Linus's tree), the API allows for 3 different possible outcomes (names my own, for illustrative purposes):
- ERROR (rc != 0): The function returns an error code, maybe after already allocating the page array and locking some pages, maybe not. It's ambiguous.
- EMPTY (!rc && !locked_pages): The function executes normally, but does not lock any folios. It may do this if it encounters a problem on the first folio.
- LOCKED (!rc && locked_pages >= 1): The function executes to completion without erroring after locking at least one folio. It may have encountered errors on subsequent folios, but it handles these by flushing the batch and trying again next time it's called.

The ceph_writepages_start() loop that calls it, today, does not differentiate between the ERROR and EMPTY result:

if (rc)

    goto release_folios;

/* did we get anything? */
if (!ceph_wbc.locked_pages)

    goto release_folios;

So right now it's just a redundant degree of freedom. Standard software engineering principles dictate that a function's API should not create complexity beyond the actual needs of its caller. So either we remove EMPTY... or remove ERROR.

The current problem with ERROR is that it (often, not always) results in a crash: the loop will come around without freeing the page array and violate the expectations of the next iteration, and we violate the BUG_ON() in ceph_allocate_page_array(). The loop is, today, not written in a way that "aborting" a batch is ever appropriate; it assumes the page array only exists when LOCKED. We must either add logic to the caller to fix it... or remove ERROR.

The only way that the ERROR result could occur today is if fscrypt is enabled, the write storm bug is fixed, and a non-first folio fails its bounce page allocation. That means that it isn't possible now, and it wouldn't be made possible by applying this patchset. The "no dead code" rule requires that we either add some logic to ceph_allocate_page_array() that can actually result in ERROR... or remove ERROR.

Finally, there is a big advantage to removing the return code from ceph_process_folio_batch(): it uses C's type system to enforce the guarantee that it doesn't abandon the page array. (Note! That does not mean that ceph_process_folio_batch() is not allowed to encounter failures -- I fully agree that it's a complex function and it may have more fallible logic added in the future -- just that it is responsible for cleaning up after those failures. Put simply, the lack of a return code gives it no way for it to make the cleanup the caller's problem!)

It can always be brought back in the future if something genuinely needs it (such as a hypothetical "major failure" in ceph_process_folio_batch() that needs to fail the entire writeback, not merely retry). But Linux doesn't keep "vestigial" code around for hypothetical future needs. If there isn't a concrete reason to add or use it now, then there's no reason to preserve it.

Hope this helps,
Sam

If locked_pages is zero, the page array must not be allocated:
ceph_process_folio_batch() uses locked_pages to decide when to allocate pages, and redundant allocations trigger ceph_allocate_page_array()'s BUG_ON(), resulting in a worker oops (and writeback stall) or even a kernel panic. Consequently, the main loop in ceph_writepages_start() assumes that the lifetime of pages is confined to a single iteration.

The ceph_submit_write() function claims ownership of the page array on success. But failures only redirty/unlock the pages and fail to free the array, making the failure case in ceph_submit_write() fatal.

Free the page array (and reset locked_pages) in ceph_submit_write()'s error-handling 'if' block so that the caller's invariant (that the array does not outlive the iteration) is maintained unconditionally, making failures in ceph_submit_write() recoverable as originally intended.

---
 fs/ceph/addr.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index 2b722916fb9b..467aa7242b49 100644
@@ -1466,6 +1466,14 @@ int ceph_submit_write(struct address_space *mapping,
 			unlock_page(page);
 		}
 
+		if (ceph_wbc->from_pool) {
+			mempool_free(ceph_wbc->pages, ceph_wb_pagevec_pool);
+			ceph_wbc->from_pool = false;
+		} else
+			kfree(ceph_wbc->pages);
+		ceph_wbc->pages = NULL;
+		ceph_wbc->locked_pages = 0;
+
 		ceph_osdc_put_request(req);
 		return -EIO;
 	}

2.51.2

Discarding a page array (i.e. after failure to submit it) is a little complex:
- Every folio in the batch needs to be redirtied and unlocked.
- Some folios are bounce pages created for fscrypt; the underlying plaintext folios also need to be redirtied and unlocked.
- The array itself can come either from the mempool or general kalloc, so different free functions need to be used depending on which.

Although currently only ceph_submit_write() does this, this logic is complex enough to warrant its own function. Move it to a new ceph_discard_page_array() function that is called by ceph_submit_write() instead.

---
 fs/ceph/addr.c | 67 ++++++++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 29 deletions(-)

index 467aa7242b49..3becb13a09fe 100644
@@ -1222,6 +1222,43 @@ void ceph_allocate_page_array(struct address_space *mapping,
 	ceph_wbc->len = 0;
 }
 
+static inline
+void ceph_discard_page_array(struct writeback_control *wbc,
+			     struct ceph_writeback_ctl *ceph_wbc)
+{
+	int i;
+	struct page *page;
+
+	for (i = 0; i < folio_batch_count(&ceph_wbc->fbatch); i++) {
+		struct folio *folio = ceph_wbc->fbatch.folios[i];
+
+		if (!folio)
+			continue;
+
+		page = &folio->page;
+		redirty_page_for_writepage(wbc, page);
+		unlock_page(page);
+	}
+
+	for (i = 0; i < ceph_wbc->locked_pages; i++) {
+		page = ceph_fscrypt_pagecache_page(ceph_wbc->pages[i]);
+
+		if (!page)
+			continue;
+
+		redirty_page_for_writepage(wbc, page);
+		unlock_page(page);
+	}
+
+	if (ceph_wbc->from_pool) {
+		mempool_free(ceph_wbc->pages, ceph_wb_pagevec_pool);
+		ceph_wbc->from_pool = false;
+	} else
+		kfree(ceph_wbc->pages);
+	ceph_wbc->pages = NULL;
+	ceph_wbc->locked_pages = 0;
+}
+
 static inline
 bool is_folio_index_contiguous(const struct ceph_writeback_ctl *ceph_wbc,
 			      const struct folio *folio)

@@ -1445,35 +1482,7 @@ int ceph_submit_write(struct address_space *mapping,
 	BUG_ON(len < ceph_fscrypt_page_offset(page) + thp_size(page) - offset);
 
 	if (!ceph_inc_osd_stopping_blocker(fsc->mdsc)) {
-		for (i = 0; i < folio_batch_count(&ceph_wbc->fbatch); i++) {
-			struct folio *folio = ceph_wbc->fbatch.folios[i];
-
-			if (!folio)
-				continue;
-
-			page = &folio->page;
-			redirty_page_for_writepage(wbc, page);
-			unlock_page(page);
-		}
-
-		for (i = 0; i < ceph_wbc->locked_pages; i++) {
-			page = ceph_fscrypt_pagecache_page(ceph_wbc->pages[i]);
-
-			if (!page)
-				continue;
-
-			redirty_page_for_writepage(wbc, page);
-			unlock_page(page);
-		}
-
-		if (ceph_wbc->from_pool) {
-			mempool_free(ceph_wbc->pages, ceph_wb_pagevec_pool);
-			ceph_wbc->from_pool = false;
-		} else
-			kfree(ceph_wbc->pages);
-		ceph_wbc->pages = NULL;
-		ceph_wbc->locked_pages = 0;
-
+		ceph_discard_page_array(wbc, ceph_wbc);
 		ceph_osdc_put_request(req);
 		return -EIO;
 	}

2.51.2

If locked_pages is zero, the page array must not be allocated:
ceph_process_folio_batch() uses locked_pages to decide when to allocate pages, and redundant allocations trigger ceph_allocate_page_array()'s BUG_ON(), resulting in a worker oops (and writeback stall) or even a kernel panic. Consequently, the main loop in ceph_writepages_start() assumes that the lifetime of pages is confined to a single iteration.

This expectation is currently not clear enough, as evidenced by two recent patches which fix oopses caused by pages persisting into the next loop iteration:
- "ceph: Do not propagate page array emplacement errors as batch errors"
- "ceph: Free page array when ceph_submit_write fails"

Use an explicit BUG_ON() at the top of the loop to assert the loop's preexisting expectation that pages is cleaned up by the previous iteration. Because this is closely tied to locked_pages, also make it the previous iteration's responsibility to guarantee its reset, and verify with a second new BUG_ON() instead of handling (and masking) failures to do so.

---
 fs/ceph/addr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 3becb13a09fe..f2db05b51a3b 100644
@@ -1679,7 +1679,9 @@ static int ceph_writepages_start(struct address_space *mapping,
 		tag_pages_for_writeback(mapping, ceph_wbc.index, ceph_wbc.end);
 
 	while (!has_writeback_done(&ceph_wbc)) {
-		ceph_wbc.locked_pages = 0;
+		BUG_ON(ceph_wbc.locked_pages);
+		BUG_ON(ceph_wbc.pages);
+
 		ceph_wbc.max_pages = ceph_wbc.wsize >> PAGE_SHIFT;
 
 get_more_pages:

2.51.2

CephFS stores file data across multiple RADOS objects. An object is the atomic unit of storage, so the writeback code must clean only folios that belong to the same object with each OSD request.

CephFS also supports RAID0-style striping of file contents: if enabled, each object stores multiple unbroken "stripe units" covering different portions of the file; if disabled, a "stripe unit" is simply the whole object. The stripe unit is (usually) reported as the inode's block size.

Though the writeback logic could, in principle, lock all dirty folios belonging to the same object, its current design is to lock only a single stripe unit at a time. Ever since this code was first written, it has determined this size by checking the inode's block size. However, the relatively-new fscrypt support needed to reduce the block size for encrypted inodes to the crypto block size (see 'fixes' commit), which causes an unnecessarily high number of write operations (~1024x as many, with 4MiB objects) and correspondingly degraded performance.

Fix this (and clarify intent) by using i_layout.stripe_unit directly in ceph_define_write_size() so that encrypted inodes are written back with the same number of operations as if they were unencrypted.

---
 fs/ceph/addr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index f2db05b51a3b..b97a6120d4b9 100644
@@ -1000,7 +1000,8 @@ unsigned int ceph_define_write_size(struct address_space *mapping)
 {
 	struct inode *inode = mapping->host;
 	struct ceph_fs_client *fsc = ceph_inode_to_fs_client(inode);
-	unsigned int wsize = i_blocksize(inode);
+	struct ceph_inode_info *ci = ceph_inode(inode);
+	unsigned int wsize = ci->i_layout.stripe_unit;
 
 	if (fsc->mount_options->wsize < wsize)
 		wsize = fsc->mount_options->wsize;

2.51.2