ceph: Properly clean up after failures in ceph_submit_write()
Subsystems (2)
cover ceph: Properly clean up after failures in ceph_submit_write()
Hi list,
This series contains two patches from my previous series [1] that are independent and have no outstanding review comments.
The patches fix a crash that can occur if ceph_inc_osd_stopping_blocker() fails on write, and split out the complex page-array cleanup logic to a new helper function (per Slava's suggestion). The first patch is intended for stable.
There is no known reproducer for this race condition; it appears to be timing-sensitive. I have a fault-injection patch that can trigger it, which I can provide on request.
Warm regards,
Sam
[1] https://lore.kernel.org/all/[email protected]/
Sam Edwards (2):
ceph: free page array when ceph_submit_write() fails
ceph: split out page-array discarding to a function
fs/ceph/addr.c | 59 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 21 deletions(-)
Signature · 1 line
2.52.0
PATCH 1/2 ceph: free page array when ceph_submit_write() fails
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 (it is later freed when the write concludes). 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 remain in ceph_wbc) is maintained unconditionally, making failures in ceph_submit_write() recoverable as originally intended.
--- fs/ceph/addr.c | 8 ++++++++ 1 file changed, 8 insertions(+)
fs/ceph/addr.c +8 -0
@@ -1470,6 +1470,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 ; }
Signature · 1 line
2.52.0
Re: [PATCH 1/2] ceph: free page array when ceph_submit_write() fails
Quoted 31 lines · Sam Edwards
If
locked_pagesis zero, the page array must not be allocated:ceph_process_folio_batch()useslocked_pagesto decide when to allocatepages, and redundant allocations trigger ceph_allocate_page_array()'sBUG_ON(), resulting in a worker oops (and writeback stall) or even a kernel panic. Consequently, the main loop inceph_writepages_start()assumes that the lifetime ofpagesis confined to a single iteration.The
ceph_submit_write()function claims ownership of the page array on success (it is later freed when the write concludes). But failures only redirty/unlock the pages and fail to free the array, making the failure case inceph_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 remain inceph_wbc) is maintained unconditionally, making failures inceph_submit_write()recoverable as originally intended.Fixes 1551ec61dc55 ("ceph: introduce ceph_submit_write() method")Signed-off-by Sam Edwards <[email protected]>--- fs/ceph/addr.c | 8 ++++++++ 1 file changed, 8 insertions(+)fs/ceph/addr.c +0
-0@@ -1470,6 +1470,14 @@ int ceph_submit_write(struct address_space *mapping,unlock_page (page);
fs/ceph/addr.c int ceph_submit_write(struct address_space *mapping, · line 1480
Quoted 10 lines
fs/ceph/addr.c +8
-0} +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 ; +
I see the completely identical code pattern in two patches:
+ 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;
I believe we need to introduce the inline function that can be reused in two places.
Thanks,
Slava.
Quoted 3 lines
fs/ceph/addr.c +0
-0ceph_osdc_put_request (req);return -EIO ; }
Re: [PATCH 1/2] ceph: free page array when ceph_submit_write() fails
Quoted 35 lines · Viacheslav Dubeyko
Quoted 33 lines · Sam Edwards
If
locked_pagesis zero, the page array must not be allocated:ceph_process_folio_batch()useslocked_pagesto decide when to allocatepages, and redundant allocations trigger ceph_allocate_page_array()'sBUG_ON(), resulting in a worker oops (and writeback stall) or even a kernel panic. Consequently, the main loop inceph_writepages_start()assumes that the lifetime ofpagesis confined to a single iteration.The
ceph_submit_write()function claims ownership of the page array on success (it is later freed when the write concludes). But failures only redirty/unlock the pages and fail to free the array, making the failure case inceph_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 remain inceph_wbc) is maintained unconditionally, making failures inceph_submit_write()recoverable as originally intended.Fixes 1551ec61dc55 ("ceph: introduce ceph_submit_write() method")Signed-off-by Sam Edwards <[email protected]>--- fs/ceph/addr.c | 8 ++++++++ 1 file changed, 8 insertions(+)fs/ceph/addr.c +0
-0@@ -1470,6 +1470,14 @@ int ceph_submit_write(struct address_space *mapping,unlock_page (page); }
Quoted 11 lines
fs/ceph/addr.c int ceph_submit_write(struct address_space *mapping, · line 1480
Quoted 8 lines
fs/ceph/addr.c +8
-0+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 ; +I see the completely identical code pattern in two patches:
The second patch only contains that pattern because it is moving it to a separate function, patch 2 isn't introducing any *new* code.
Quoted 11 lines
fs/ceph/addr.c +7
-0+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 ;I believe we need to introduce the inline function that can be reused in two places.
Patch 2 is introducing that inline function as requested -- but that function is not actually used in two places: for now (in this series), it is only split out for better readability.
These patches are organized like this because of kernel development
norms: bugfixes intended for stable (such as this patch) should consist of minimal, backport-friendly and correctness-focused changes. Moving existing code to a new function is a separate change and does not constitute a bugfix, so it needs to go in its own patch that isn't
Cc: stable.
Cheers,
Sam
Quoted 7 lines
Thanks,
Slava.Quoted 3 lines
fs/ceph/addr.c +0
-0ceph_osdc_put_request (req);return -EIO ; }
Ilya Dryomov
maintainerRe: [PATCH 1/2] ceph: free page array when ceph_submit_write() fails
In reply to Sam EdwardsQuoted 31 lines · Sam Edwards
If
locked_pagesis zero, the page array must not be allocated:ceph_process_folio_batch()useslocked_pagesto decide when to allocatepages, and redundant allocations trigger ceph_allocate_page_array()'sBUG_ON(), resulting in a worker oops (and writeback stall) or even a kernel panic. Consequently, the main loop inceph_writepages_start()assumes that the lifetime ofpagesis confined to a single iteration.The
ceph_submit_write()function claims ownership of the page array on success (it is later freed when the write concludes). But failures only redirty/unlock the pages and fail to free the array, making the failure case inceph_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 remain inceph_wbc) is maintained unconditionally, making failures inceph_submit_write()recoverable as originally intended.Fixes 1551ec61dc55 ("ceph: introduce ceph_submit_write() method")Signed-off-by Sam Edwards <[email protected]>--- fs/ceph/addr.c | 8 ++++++++ 1 file changed, 8 insertions(+)fs/ceph/addr.c +0
-0
fs/ceph/addr.c int ceph_submit_write(struct address_space *mapping, · line 1479
Quoted 10 lines
fs/ceph/addr.c +7
-0@@ -1470,6 +1470,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 ;
Hi Sam,
While I don't see anything wrong with the patch per se, I can't help but question the existence of this entire branch along with the meaning of the error.
ceph_writepages_start() is the only caller of ceph_submit_write() and it already calls ceph_inc_osd_stopping_blocker() at the top where the error can be handled naturally -- nothing needs to be unlocked or freed at that point. Since mdsc->stopping_blockers is just a counter, all calls made by ceph_submit_write() invocations in a loop would be "contained" within that ceph_writepages_start() call. The only benefit achieved is potentially faster response to the MDS client moving to CEPH_MDSC_STOPPING_FLUSHING state, but it's rather dubious because sneaking in/having to wait for some more OSD requests isn't really the end of the world.
Rather than patching the error path, I wonder if instead of calling ceph_inc_osd_stopping_blocker() the counter could just be incremented unconditionally, with the check for CEPH_MDSC_STOPPING_FLUSHING bypassed there? This could be wrapped into a new helper that could also assert that the counter is already elevated before the increment.
Thanks,
Ilya
Ilya Dryomov
maintainerRe: [PATCH 1/2] ceph: free page array when ceph_submit_write() fails
Quoted 61 lines · Ilya Dryomov
Quoted 31 lines · Sam Edwards
If
locked_pagesis zero, the page array must not be allocated:ceph_process_folio_batch()useslocked_pagesto decide when to allocatepages, and redundant allocations trigger ceph_allocate_page_array()'sBUG_ON(), resulting in a worker oops (and writeback stall) or even a kernel panic. Consequently, the main loop inceph_writepages_start()assumes that the lifetime ofpagesis confined to a single iteration.The
ceph_submit_write()function claims ownership of the page array on success (it is later freed when the write concludes). But failures only redirty/unlock the pages and fail to free the array, making the failure case inceph_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 remain inceph_wbc) is maintained unconditionally, making failures inceph_submit_write()recoverable as originally intended.Fixes 1551ec61dc55 ("ceph: introduce ceph_submit_write() method")Signed-off-by Sam Edwards <[email protected]>--- fs/ceph/addr.c | 8 ++++++++ 1 file changed, 8 insertions(+)fs/ceph/addr.c +0
-0fs/ceph/addr.c int ceph_submit_write(struct address_space *mapping, · line 1479
Quoted 10 lines
fs/ceph/addr.c +7
-0@@ -1470,6 +1470,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 ;Hi Sam,
While I don't see anything wrong with the patch per se, I can't help but question the existence of this entire branch along with the meaning of the error.
ceph_writepages_start()is the only caller ofceph_submit_write()and it already callsceph_inc_osd_stopping_blocker()at the top where the error can be handled naturally -- nothing needs to be unlocked or freed at that point. Sincemdsc->stopping_blockersis just a counter, all calls made byceph_submit_write()invocations in a loop would be "contained" within thatceph_writepages_start()call. The only benefit achieved is potentially faster response to the MDS client moving toCEPH_MDSC_STOPPING_FLUSHINGstate, but it's rather dubious because sneaking in/having to wait for some more OSD requests isn't really the end of the world.
Quoted 5 lines
Rather than patching the error path, I wonder if instead of calling
ceph_inc_osd_stopping_blocker()the counter could just be incremented unconditionally, with the check forCEPH_MDSC_STOPPING_FLUSHINGbypassed there? This could be wrapped into a new helper that could also assert that the counter is already elevated before the increment.
Something along the lines of
void __ceph_inc_osd_stopping_blocker(struct ceph_mds_client *mdsc)
{
BUG_ON(!atomic_inc_not_zero(&mdsc->stopping_blockers));
}and switching to __ceph_inc_osd_stopping_blocker() in place of ceph_inc_osd_stopping_blocker() in ceph_submit_write().
Thanks,
Ilya
PATCH 2/2 ceph: split out page-array discarding to a function
In reply to Sam EdwardsDiscarding 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(-)
fs/ceph/addr.c +38 -29
@@ -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)
@@ -1449,35 +1486,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 ; }
Signature · 1 line
2.52.0