ceph: CephFS writeback correctness and performance fixes

Following an earlier commit, 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. Any future changes requiring "abort" logic should follow this invariant by cleaning up its array and resetting ceph_wbc.locked_pages appropriately.

Signed-off-by Sam Edwards <[email protected]>

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

fs/ceph/addr.c +0 -0

index 39064893f35b..cdf11288d6b7 100644

fs/ceph/addr.c +5 -9

+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];

@@ -1323,12 +1323,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;

@@ -1370,7 +1368,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;

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

@@ -1686,10 +1681,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)

Quoted 26 lines · Sam Edwards

Following an earlier commit, 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. Any future changes requiring "abort" logic should follow this invariant by cleaning up its array and resetting ceph_wbc.locked_pages appropriately.

Signed-off-by Sam Edwards <[email protected]>

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

fs/ceph/addr.c +0 -0

index 39064893f35b..cdf11288d6b7 100644
@@ -1284,16 +1284,16 @@ static inline int move_dirty_folio_in_page_array(struct address_space *mapping,
 }
 

fs/ceph/addr.c static inline int move_dirty_folio_in_page_array(struct address_space *mapping, · line 1289

Quoted 4 lines

fs/ceph/addr.c +0 -3

 static
-int ceph_process_folio_batch(struct address_space *mapping,
-                          struct writeback_control *wbc,
-                          struct ceph_writeback_ctl *ceph_wbc)

I still prefer to return the error code from the function.

Thanks,
Slava.

Quoted 56 lines

fs/ceph/addr.c +5 -9

+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];

@@ -1323,12 +1323,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;

@@ -1370,7 +1368,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;

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

@@ -1686,10 +1681,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)

Quoted 28 lines · Viacheslav Dubeyko

Quoted 26 lines · Sam Edwards

Following an earlier commit, 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. Any future changes requiring "abort" logic should follow this invariant by cleaning up its array and resetting ceph_wbc.locked_pages appropriately.

Signed-off-by Sam Edwards <[email protected]>

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

fs/ceph/addr.c +0 -0

index 39064893f35b..cdf11288d6b7 100644
@@ -1284,16 +1284,16 @@ static inline int move_dirty_folio_in_page_array(struct address_space *mapping,
 }
 

Quoted 6 lines

fs/ceph/addr.c static inline int move_dirty_folio_in_page_array(struct address_space *mapping, · line 1289

Quoted 4 lines

fs/ceph/addr.c +0 -3

 static
-int ceph_process_folio_batch(struct address_space *mapping,
-                          struct writeback_control *wbc,
-                          struct ceph_writeback_ctl *ceph_wbc)

I still prefer to return the error code from the function.

Hi Slava,

Since the objection to this patch is mere preference at this point, we need to seek broader consensus before either of us unilaterally decides whether this style is appropriate for the CephFS subsystem. I'm willing to follow the preferences of the group, we just need a "group" first. :)

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.

This patch does not change invariants, behavior, or failure modes. The added BUG_ON() lines catch conditions that would already trigger oops, but do so earlier for easier debugging and programmer clarity.

Signed-off-by Sam Edwards <[email protected]>

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

fs/ceph/addr.c +0 -0

index cdf11288d6b7..4e392fc70d33 100644

fs/ceph/addr.c +1 -0

+
 		ceph_wbc.max_pages = ceph_wbc.wsize >> PAGE_SHIFT;
 
 get_more_pages:

Quoted 37 lines · Sam Edwards

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.

This patch does not change invariants, behavior, or failure modes. The added BUG_ON() lines catch conditions that would already trigger oops, but do so earlier for easier debugging and programmer clarity.

Signed-off-by Sam Edwards <[email protected]>

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

fs/ceph/addr.c +0 -0

index cdf11288d6b7..4e392fc70d33 100644
@@ -1663,7 +1663,9 @@ static int ceph_writepages_start(struct address_space *mapping,
              tag_pages_for_writeback(mapping, ceph_wbc.index, ceph_wbc.end);
 

fs/ceph/addr.c static int ceph_writepages_start(struct address_space *mapping, · line 1667

Quoted 4 lines

fs/ceph/addr.c +2 -1

      while (!has_writeback_done(&ceph_wbc)) {
-             ceph_wbc.locked_pages = 0;
+             BUG_ON(ceph_wbc.locked_pages);
+             BUG_ON(ceph_wbc.pages);

My complains are still the same. I would like not have BUG_ON() here.

Thanks,
Slava.

Quoted 4 lines

fs/ceph/addr.c +1 -0

+
              ceph_wbc.max_pages = ceph_wbc.wsize >> PAGE_SHIFT;
 
 get_more_pages:

Quoted 39 lines · Viacheslav Dubeyko

Quoted 37 lines · Sam Edwards

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.

This patch does not change invariants, behavior, or failure modes. The added BUG_ON() lines catch conditions that would already trigger oops, but do so earlier for easier debugging and programmer clarity.

Signed-off-by Sam Edwards <[email protected]>

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

fs/ceph/addr.c +0 -0

index cdf11288d6b7..4e392fc70d33 100644
@@ -1663,7 +1663,9 @@ static int ceph_writepages_start(struct address_space *mapping,
              tag_pages_for_writeback(mapping, ceph_wbc.index, ceph_wbc.end);
 

Quoted 6 lines

fs/ceph/addr.c static int ceph_writepages_start(struct address_space *mapping, · line 1667

Quoted 4 lines

fs/ceph/addr.c +2 -1

      while (!has_writeback_done(&ceph_wbc)) {
-             ceph_wbc.locked_pages = 0;
+             BUG_ON(ceph_wbc.locked_pages);
+             BUG_ON(ceph_wbc.pages);

My complains are still the same. I would like not have BUG_ON() here.

Hey Slava,

I understand your preference, but as discussed before, the BUG_ON() is defensive programming to catch an invariant violation earlier than the existing BUG_ON() in ceph_allocate_page_array(). If the invariant is broken, we'll oops anyway; this just makes the oops happen sooner and easier to debug, and reduces cognitive load for future programmers trying to understand the loop invariants.

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 v2->v3:
- Split out two patches ("ceph: free page array when ceph_submit_write() fails" and "ceph: split out page-array discarding to a function") to a new series
[2] since they are independent and had no outstanding review comments.
- Lowercase the subject lines of commit messages, per subsystem-local style.
- Update the commit message of ("ceph: fix write storm on fscrypted files") to mention the explicit dependency on ("ceph: do not propagate page array emplacement errors as batch errors") for correctness, to prevent the former from being accidentally backported without the latter.
- Reorder the series to make the aforementioned patches consecutive. The series cadence is now: bugfix, bugfix, cleanup, cleanup
- Add a clarification to ("ceph: remove error return from ceph_process_folio_batch()") that "abort" logic is still possible, just that it is responsible for cleaning up after itself.

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)

Warm regards,
Sam

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

Sam Edwards (4):
ceph: do not propagate page array emplacement errors as batch errors
ceph: fix write storm on fscrypted files
ceph: remove error return from ceph_process_folio_batch()
ceph: assert writeback loop invariants

 fs/ceph/addr.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

--
2.52.0