ceph: fix num_ops OBOE when crypto allocation fails

PATCH Superseded Sam Edwards 3 participants 6 messages Mar 15 – Mar 17

Subsystems (2)

Stable branch Ceph client

lore

Share & tools

lore

Sam Edwards

author

2026-03-15 23:25 UTC #0 lore raw

PATCH 1/1 ceph: fix num_ops OBOE when crypto allocation fails

move_dirty_folio_in_page_array() may fail if the file is encrypted, the dirty folio is not the first in the batch, and it fails to allocate a bounce buffer to hold the ciphertext. When that happens, ceph_process_folio_batch() simply redirties the folio and flushes the current batch -- it can retry that folio in a future batch.

However, if this failed folio is not contiguous with the last folio that did make it into the batch, then ceph_process_folio_batch() has already incremented ceph_wbc->num_ops; because it doesn't follow through and add the discontiguous folio to the array, ceph_submit_write() -- which expects that ceph_wbc->num_ops accurately reflects the number of contiguous ranges (and therefore the required number of "write extent" ops) in the writeback -- will panic the kernel:

    BUG_ON(ceph_wbc->op_idx + 1 != req->r_num_ops);

Fix this crash by decrementing ceph_wbc->num_ops back to the correct value when move_dirty_folio_in_page_array() fails, but the folio already started counting a new (i.e. still-empty) extent.

The defect corrected by this patch has existed since 2022 (see first Fixes:), but another bug blocked multi-folio encrypted writeback until recently (see second Fixes:). The second commit made it into 6.18.16, 6.19.6, and 7.0-rc1, unmasking the panic in those versions. This patch therefore fixes a regression (panic) introduced by cac190c7674f.

Cc [email protected] # v6.18+

Fixes d55207717ded ("ceph: add encryption support to writepage and writepages")

Fixes cac190c7674f ("ceph: fix write storm on fscrypted files")

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

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

fs/ceph/addr.c +4 -0

index e87b3bb94ee8..f366e159ffa6 100644
@@ -1366,6 +1366,10 @@ void ceph_process_folio_batch(struct address_space *mapping,
 		rc = move_dirty_folio_in_page_array(mapping, wbc, ceph_wbc,
 				folio);
 		if (rc) {
+			/* Did we just begin a new contiguous op? Nevermind! */
+			if (ceph_wbc->len == 0)
+				ceph_wbc->num_ops--;
+
 			folio_redirty_for_writepage(wbc, folio);
 			folio_unlock(folio);
 			break;

Signature · 1 line

2.52.0

Viacheslav Dubeyko

2026-03-16 17:44 UTC #1 lore raw

Quoted 6 lines · Sam Edwards

move_dirty_folio_in_page_array() may fail if the file is encrypted, the dirty folio is not the first in the batch, and it fails to allocate a bounce buffer to hold the ciphertext. When that happens, ceph_process_folio_batch() simply redirties the folio and flushes the current batch -- it can retry that folio in a future batch.

How this issue can be reproduced? Do you have a reproduction script or anything like this?

Quoted 9 lines

However, if this failed folio is not contiguous with the last folio that did make it into the batch, then ceph_process_folio_batch() has already incremented ceph_wbc->num_ops; because it doesn't follow through and add the discontiguous folio to the array, ceph_submit_write() -- which expects that ceph_wbc->num_ops accurately reflects the number of contiguous ranges (and therefore the required number of "write extent" ops) in the writeback -- will panic the kernel:
    BUG_ON(ceph_wbc->op_idx + 1 != req->r_num_ops);

I don't quite follow. We decrement ceph_wbc->num_ops but BUG_ON() operates by req->r_num_ops. How req->r_num_ops receives the value of ceph_wbc->num_ops?

Quoted 24 lines

Fix this crash by decrementing ceph_wbc->num_ops back to the correct value when move_dirty_folio_in_page_array() fails, but the folio already started counting a new (i.e. still-empty) extent.
The defect corrected by this patch has existed since 2022 (see first Fixes:), but another bug blocked multi-folio encrypted writeback until recently (see second Fixes:). The second commit made it into 6.18.16, 6.19.6, and 7.0-rc1, unmasking the panic in those versions. This patch therefore fixes a regression (panic) introduced by cac190c7674f.
Cc [email protected] # v6.18+
Fixes d55207717ded ("ceph: add encryption support to writepage and writepages")
Fixes cac190c7674f ("ceph: fix write storm on fscrypted files")
Signed-off-by Sam Edwards <[email protected]>
---
 fs/ceph/addr.c | 4 ++++
 1 file changed, 4 insertions(+)
fs/ceph/addr.c +0 -0
index e87b3bb94ee8..f366e159ffa6 100644

fs/ceph/addr.c void ceph_process_folio_batch(struct address_space *mapping, · line 1375

Quoted 10 lines

fs/ceph/addr.c +4 -0

@@ -1366,6 +1366,10 @@ void ceph_process_folio_batch(struct address_space *mapping,
 		rc = move_dirty_folio_in_page_array(mapping, wbc, ceph_wbc,
 				folio);
 		if (rc) {
+			/* Did we just begin a new contiguous op? Nevermind! */
+			if (ceph_wbc->len == 0)
+				ceph_wbc->num_ops--;
+
 			folio_redirty_for_writepage(wbc, folio);
 			folio_unlock(folio);
 			break;

We change ceph_wbc->num_ops, ceph_wbc->offset, and ceph_wbc->len here:

		} else if (!is_folio_index_contiguous(ceph_wbc, folio)) {
			if (is_num_ops_too_big(ceph_wbc)) {
				folio_redirty_for_writepage(wbc, folio);
				folio_unlock(folio);
				break;
			}

			ceph_wbc->num_ops++;
			ceph_wbc->offset = (u64)folio_pos(folio);
			ceph_wbc->len = 0;
		}

First of all, technically speaking, move_dirty_folio_in_page_array() can fail even if is_folio_index_contiguous() is positive. Do you mean that we don't need to decrement the ceph_wbc->num_ops in such case?

Secondly, do we need to correct ceph_wbc->offset?

Thanks,
Slava.

Sam Edwards

author

2026-03-16 20:15 UTC #2 lore raw

Quoted 11 lines · Viacheslav Dubeyko

Quoted 6 lines · Sam Edwards
move_dirty_folio_in_page_array() may fail if the file is encrypted, the dirty folio is not the first in the batch, and it fails to allocate a bounce buffer to hold the ciphertext. When that happens, ceph_process_folio_batch() simply redirties the folio and flushes the current batch -- it can retry that folio in a future batch.
How this issue can be reproduced? Do you have a reproduction script or anything like this?

Good day Slava,

Is this question about the preceding paragraph? If so: that paragraph is just describing current (and intended) behavior, not an issue.

If this is just a general question about the patch, then I don't know of a way to trigger the issue in a short timeframe, but something like this ought to work:
1. Create a reasonably-sized (e.g. 4GiB) fscrypt-protected file in CephFS
2. Put the CephFS client system under heavy memory pressure, so that bounce page allocation is more likely to fail
3. Repeatedly write to the file in a 4KiB-written/4KiB-skipped pattern, starting over upon getting to the end of the file
4. Wait for the system to panic, gradually ramping up the memory pressure until it does

I run a workload that performs fairly random I/O atop CephFS+fscrypt. Before this patch, I'd get a panic after about a day. After this patch, I've been running for 4+ days without this particular issue reappearing.

Quoted 8 lines

Quoted 8 lines
However, if this failed folio is not contiguous with the last folio that did make it into the batch, then ceph_process_folio_batch() has already incremented ceph_wbc->num_ops; because it doesn't follow through and add the discontiguous folio to the array, ceph_submit_write() -- which expects that ceph_wbc->num_ops accurately reflects the number of contiguous ranges (and therefore the required number of "write extent" ops) in the writeback -- will panic the kernel:

Quoted 4 lines

Quoted 1 line
    BUG_ON(ceph_wbc->op_idx + 1 != req->r_num_ops);
I don't quite follow. We decrement ceph_wbc->num_ops but BUG_ON() operates by req->r_num_ops. How req->r_num_ops receives the value of ceph_wbc->num_ops?

ceph_submit_write() passes ceph_wbc->num_ops to ceph_osdc_new_request()...

Quoted 14 lines

We change ceph_wbc->num_ops, ceph_wbc->offset, and ceph_wbc->len here:

                } else if (!is_folio_index_contiguous(ceph_wbc, folio)) {
                        if (is_num_ops_too_big(ceph_wbc)) {
                                folio_redirty_for_writepage(wbc, folio);
                                folio_unlock(folio);
                                break;
                        }

                        ceph_wbc->num_ops++;
                        ceph_wbc->offset = (u64)folio_pos(folio);
                        ceph_wbc->len = 0;
                }

Quoted 3 lines

First of all, technically speaking, move_dirty_folio_in_page_array() can fail even if is_folio_index_contiguous() is positive. Do you mean that we don't need to decrement the ceph_wbc->num_ops in such case?

Yes, exactly: as stated in the commit message, we only need to correct the value "when move_dirty_folio_in_page_array() fails, but the folio already started counting a new (i.e. still-empty) extent." The len == 0 test is checking for that new/still-empty condition.

Quoted 1 line

Secondly, do we need to correct ceph_wbc->offset?

No, we do not; the valid lifetime of offset/len ends when ceph_process_folio_batch() returns. I'd even argue they don't belong in ceph_wbc at all and should be local variables instead, but that's a matter for a different patch.

Cheers,
Sam

Viacheslav Dubeyko

2026-03-17 18:51 UTC #3 lore raw

Quoted 30 lines · Sam Edwards

Quoted 11 lines · Viacheslav Dubeyko
Quoted 6 lines · Sam Edwards
move_dirty_folio_in_page_array() may fail if the file is encrypted, the dirty folio is not the first in the batch, and it fails to allocate a bounce buffer to hold the ciphertext. When that happens, ceph_process_folio_batch() simply redirties the folio and flushes the current batch -- it can retry that folio in a future batch.
How this issue can be reproduced? Do you have a reproduction script or anything like this?
Good day Slava,
Is this question about the preceding paragraph? If so: that paragraph is just describing current (and intended) behavior, not an issue.
If this is just a general question about the patch, then I don't know of a way to trigger the issue in a short timeframe, but something like this ought to work:
1. Create a reasonably-sized (e.g. 4GiB) fscrypt-protected file in CephFS
2. Put the CephFS client system under heavy memory pressure, so that bounce page allocation is more likely to fail
3. Repeatedly write to the file in a 4KiB-written/4KiB-skipped pattern, starting over upon getting to the end of the file
4. Wait for the system to panic, gradually ramping up the memory pressure until it does

Quoted 5 lines

I run a workload that performs fairly random I/O atop CephFS+fscrypt. Before this patch, I'd get a panic after about a day. After this patch, I've been running for 4+ days without this particular issue reappearing.

I think this is good enough description how the issue can be triggered. And I believe that the commit message deserve to have this description.

Frankly speaking, I am trying to reproduce the issue [1]. Do you think that it could be the same issue?

Quoted 9 lines

Quoted 9 lines
Quoted 9 lines
However, if this failed folio is not contiguous with the last folio that did make it into the batch, then ceph_process_folio_batch() has already incremented ceph_wbc->num_ops; because it doesn't follow through and add the discontiguous folio to the array, ceph_submit_write() -- which expects that ceph_wbc->num_ops accurately reflects the number of contiguous ranges (and therefore the required number of "write extent" ops) in the writeback -- will panic the kernel:
    BUG_ON(ceph_wbc->op_idx + 1 != req->r_num_ops);

I believe that it will be great to have the link to the particular location of this code in the commit message.

Quoted 5 lines

Quoted 3 lines
I don't quite follow. We decrement ceph_wbc->num_ops but BUG_ON() operates by req->r_num_ops. How req->r_num_ops receives the value of ceph_wbc->num_ops?
ceph_submit_write() passes ceph_wbc->num_ops to ceph_osdc_new_request()...

I think it makes sense to mention it in the commit message.

Quoted 24 lines

Quoted 15 lines
We change ceph_wbc->num_ops, ceph_wbc->offset, and ceph_wbc->len here:
                } else if (!is_folio_index_contiguous(ceph_wbc, folio)) {
                        if (is_num_ops_too_big(ceph_wbc)) {
                                folio_redirty_for_writepage(wbc, folio);
                                folio_unlock(folio);
                                break;
                        }

                        ceph_wbc->num_ops++;
                        ceph_wbc->offset = (u64)folio_pos(folio);
                        ceph_wbc->len = 0;
                }
Quoted 3 lines
First of all, technically speaking, move_dirty_folio_in_page_array() can fail even if is_folio_index_contiguous() is positive. Do you mean that we don't need to decrement the ceph_wbc->num_ops in such case?
Yes, exactly: as stated in the commit message, we only need to correct the value "when move_dirty_folio_in_page_array() fails, but the folio already started counting a new (i.e. still-empty) extent." The len == 0 test is checking for that new/still-empty condition.

Quoted 7 lines

Quoted 1 line
Secondly, do we need to correct ceph_wbc->offset?
No, we do not; the valid lifetime of offset/len ends when ceph_process_folio_batch() returns. I'd even argue they don't belong in ceph_wbc at all and should be local variables instead, but that's a matter for a different patch.

I think that it makes sense to create the issue in Ceph tracker and to add Closes to the fix.

Thanks,
Slava.

[1] https://tracker.ceph.com/issues/74156

Sam Edwards

author NAK

2026-03-17 19:13 UTC #4 lore raw

Quoted 11 lines · Viacheslav Dubeyko

Quoted 11 lines
If this is just a general question about the patch, then I don't know of a way to trigger the issue in a short timeframe, but something like this ought to work:
1. Create a reasonably-sized (e.g. 4GiB) fscrypt-protected file in CephFS
2. Put the CephFS client system under heavy memory pressure, so that bounce page allocation is more likely to fail
3. Repeatedly write to the file in a 4KiB-written/4KiB-skipped pattern, starting over upon getting to the end of the file
4. Wait for the system to panic, gradually ramping up the memory pressure until it does

Quoted 8 lines

Quoted 5 lines
I run a workload that performs fairly random I/O atop CephFS+fscrypt. Before this patch, I'd get a panic after about a day. After this patch, I've been running for 4+ days without this particular issue reappearing.
I think this is good enough description how the issue can be triggered. And I believe that the commit message deserve to have this description.

Very well, I'll try to condense it in a way that makes it clear to those trying to repro the crash without being overly verbose.

Quoted 2 lines

Frankly speaking, I am trying to reproduce the issue [1]. Do you think that it could be the same issue?

Please double-check; the link you sent is to bug #74156, reported last year. This regression was only introduced last month, so it couldn't be the same issue. Did you send the wrong link?

Quoted 4 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
    BUG_ON(ceph_wbc->op_idx + 1 != req->r_num_ops);
I believe that it will be great to have the link to the particular location of this code in the commit message.

I strongly disagree: The location of the code changes with every commit that adds/removes lines above it (including this patch) so such a link would be rendered stale immediately. What is your reason for believing the link is useful?

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
I don't quite follow. We decrement ceph_wbc->num_ops but BUG_ON() operates by req->r_num_ops. How req->r_num_ops receives the value of ceph_wbc->num_ops?
ceph_submit_write() passes ceph_wbc->num_ops to ceph_osdc_new_request()...
I think it makes sense to mention it in the commit message.

NACK, that relationship is already memorialized in addr.c. But again I'm interested to learn your reasoning.

Quoted 2 lines

I think that it makes sense to create the issue in Ceph tracker and to add Closes to the fix.

I don't currently have a Ceph tracker account and don't think I can add anything of substance to an issue report. Feel free to create the issue on my behalf if it's important for Ceph's processes, and I can
Closes: tag it in v2.

Cheers,
Sam

Ilya Dryomov

maintainer

2026-03-17 19:48 UTC #5 lore raw

Quoted 7 lines · Sam Edwards

Quoted 2 lines
I think that it makes sense to create the issue in Ceph tracker and to add Closes to the fix.
I don't currently have a Ceph tracker account and don't think I can add anything of substance to an issue report. Feel free to create the issue on my behalf if it's important for Ceph's processes, and I can
Closes: tag it in v2.

Hi Sam,

It's not important. A tracker ticket is useful for when the issue is purely reported as the placeholder for investigation, log attachments, etc. In this case you both reported the issue and proposed a fix for it, so the patch description is the only thing that matters. Creating a tracker ticket just for the sake of it isn't needed.

Thanks,

Ilya