writeback completion soft lockup BUG in folio_wake_bit()

What seems to happen is that the majority of the fsync calls end up waiting on writeback of a particular page, the wakeup of the writeback bit on that page wakes a task that immediately resets PG_writeback on the page such that N other folio_wait_writeback() waiters see the bit still set and immediately place themselves back onto the tail of the wait queue. Meanwhile the waker task spins in the WQ_FLAG_BOOKMARK loop in folio_wake_bit() (backing off the lock for a cycle or so in each iteration) only to find the same bunch of tasks in the queue. This process repeats for a long enough amount of time to trigger the soft lockup warning. I've confirmed this spinning behavior with a tracepoint in the bookmark loop that indicates we're stuck for many hundreds of thousands of iterations (at least) of this loop when the soft lockup warning triggers.

I've run a few quick experiments to try and corroborate this analysis. The problem goes away completely if I either back out the loop change in folio_wait_writeback() or bump WAITQUEUE_WALK_BREAK_CNT to something like 128 (i.e. greater than the total possible number of waiter tasks in this test). I've also played a few games with bookmark behavior mostly out of curiosity, but usually end up introducing other problems like missed wakeups, etc.

As I recall, the bookmark hack was introduced in order to handle lock_page() problems. It wasn't really supposed to handle writeback, but nobody thought it would cause any harm (and indeed, it didn't at the time). So how about we only use bookmarks for lock_page(), since lock_page() usually doesn't have the multiple-waker semantics that writeback has?

Quoted 7 lines · Matthew Wilcox

As I recall, the bookmark hack was introduced in order to handle lock_page() problems. It wasn't really supposed to handle writeback, but nobody thought it would cause any harm (and indeed, it didn't at the time). So how about we only use bookmarks for lock_page(), since lock_page() usually doesn't have the multiple-waker semantics that writeback has?

I was hoping that some of the page lock problems are gone and we could maybe try to get rid of the bookmarks entirely.

But the page lock issues only ever showed up on some private proprietary load and machine, so we never really got confirmation that they are fixed. There were lots of strong signs to them being related to the migration page locking, and it may be that the bookmark code is only hurting these days.

See for example commit 9a1ea439b16b ("mm:
put_and_wait_on_page_locked() while page is migrated") which doesn't actually change the *locking* side, but drops the page reference when waiting for the locked page to be unlocked, which in turn removes a "loop and try again when migration". And that may have been the real _fix_ for the problem.

Because while the bookmark thing avoids the NMI lockup detector firing due to excessive hold times, the bookmarking also _causes_ that "we now will see the same page multiple times because we dropped the lock and somebody re-added it at the end of the queue" issue. Which seems to be the problem here.

Ugh. I wish we had some way to test "could we just remove the bookmark code entirely again".

Of course, the PG_lock case also works fairly hard to not actually remove and re-add the lock waiter to the queue, but having an actual "wait for and get the lock" operation. The writeback bit isn't done that way.

I do hate how we had to make folio_wait_writeback{_killable}() use "while" rather than an "if". It *almost* works with just a "wait for current writeback", but not quite. See commit c2407cf7d22d ("mm: make wait_on_page_writeback() wait for multiple pending writebacks") for why we have to loop. Ugly, ugly.

Because I do think that "while" in the writeback waiting is a problem. Maybe _the_ problem.

So how about we do something like this:

- Make folio_start_writeback() and set_page_writeback() return void, fixing up AFS and NFS.
- Add a folio_wait_start_writeback() to use in the VFS
- Remove the calls to set_page_writeback() in the filesystems

Quoted 3 lines · Matthew Wilcox

So how about we do something like this:

Quoted 4 lines

- Make folio_start_writeback() and set_page_writeback() return void, fixing up AFS and NFS.
- Add a folio_wait_start_writeback() to use in the VFS
- Remove the calls to set_page_writeback() in the filesystems

That sounds lovely, but it does worry me a bit. Not just the odd 'keepwrite' thing, but also the whole ordering between the folio bit and the tagging bits. Does the ordering possibly matter?

That whole "xyz_writeback_keepwrite()" thing seems odd. It's used in only one place (the folio version isn't used at all):

ext4_writepage():

     ext4_walk_page_buffers() fails:
                redirty_page_for_writepage(wbc, page);
                keep_towrite = true;
      ext4_bio_write_page().

which just looks odd. Why does it even try to continue to do the writepage when the page buffer thing has failed?

In the regular write path (ie ext4_write_begin()), a ext4_walk_page_buffers() failure is fatal or causes a retry). Why is ext4_writepage() any different? Particularly since it wants to keep the page dirty, then trying to do the writeback just seems wrong.

So this code is all a bit odd, I suspect there are decades of "people continued to do what they historically did" changes, and it is all worrisome.

Quoted 4 lines · Linus Torvalds

Quoted 3 lines · Matthew Wilcox

So how about we do something like this:

Quoted 8 lines

Quoted 4 lines

- Make folio_start_writeback() and set_page_writeback() return void, fixing up AFS and NFS.
- Add a folio_wait_start_writeback() to use in the VFS
- Remove the calls to set_page_writeback() in the filesystems

That sounds lovely, but it does worry me a bit. Not just the odd 'keepwrite' thing, but also the whole ordering between the folio bit and the tagging bits. Does the ordering possibly matter?

I wouldn't change the ordering of setting the xarray bits and the writeback flag; they'd just be set a little earlier. It'd all be done while the page was still locked. But you're right, there's lots of subtle interactions here.

Quoted 18 lines

That whole "xyz_writeback_keepwrite()" thing seems odd. It's used in only one place (the folio version isn't used at all):

ext4_writepage():

     ext4_walk_page_buffers() fails:
                redirty_page_for_writepage(wbc, page);
                keep_towrite = true;
      ext4_bio_write_page().

which just looks odd. Why does it even try to continue to do the writepage when the page buffer thing has failed?

In the regular write path (ie ext4_write_begin()), a ext4_walk_page_buffers() failure is fatal or causes a retry). Why is ext4_writepage() any different? Particularly since it wants to keep the page dirty, then trying to do the writeback just seems wrong.

Quoted 3 lines

So this code is all a bit odd, I suspect there are decades of "people continued to do what they historically did" changes, and it is all worrisome.

I found the commit: 1c8349a17137 ("ext4: fix data integrity sync in ordered mode"). Fortunately, we have a documented test for this, generic/127, so we'll know if we've broken it.

Quoted 18 lines · Linus Torvalds

That whole "xyz_writeback_keepwrite()" thing seems odd. It's used in only one place (the folio version isn't used at all):

ext4_writepage():

     ext4_walk_page_buffers() fails:
                redirty_page_for_writepage(wbc, page);
                keep_towrite = true;
      ext4_bio_write_page().

which just looks odd. Why does it even try to continue to do the writepage when the page buffer thing has failed?

In the regular write path (ie ext4_write_begin()), a ext4_walk_page_buffers() failure is fatal or causes a retry). Why is ext4_writepage() any different? Particularly since it wants to keep the page dirty, then trying to do the writeback just seems wrong.

Quoted 3 lines

So this code is all a bit odd, I suspect there are decades of "people continued to do what they historically did" changes, and it is all worrisome.

I found the commit: 1c8349a17137 ("ext4: fix data integrity sync in ordered mode"). Fortunately, we have a documented test for this, generic/127, so we'll know if we've broken it.

I agree with Dave that 'keep_towrite' thing is kind of self-inflicted damage on the ext4 side (we need to write out some blocks underlying the page but cannot write all from the transaction commit code, so we need to keep xarray tags intact so that data integrity sync cannot miss the page). Also it is no longer needed in the current default ext4 setup. But if you have blocksize < pagesize and mount the fs with 'dioreadlock,data=ordered' mount options, the hack is still needed AFAIK and we don't have a reasonable way around it.

Quoted 9 lines · Jan Kara

I agree with Dave that 'keep_towrite' thing is kind of self-inflicted damage on the ext4 side (we need to write out some blocks underlying the page but cannot write all from the transaction commit code, so we need to keep xarray tags intact so that data integrity sync cannot miss the page). Also it is no longer needed in the current default ext4 setup. But if you have blocksize < pagesize and mount the fs with 'dioreadlock,data=ordered' mount options, the hack is still needed AFAIK and we don't have a reasonable way around it.

I assume you meant 'dioread_lock'.

Which seems to be the default (even if 'data=ordered' is not).

IOW, we could simply warn about "data=ordered is no longer supported" and turn it into data=journal.

Obviously *only* do this for the case of "blocksize < PAGE_SIZE".

So the thing that I've been waiting to do for a while is to replace the whole data=ordered vs data=writeback and dioread_nolock and dioread_lock is a complete reworking of the ext4 buffered writeback path, where we write the data blocks *first*, and only then update the ext4 metadata.

*) Determining where the new allocated data blockblocks should be, and
preventing those blocks from being used for any other purposes, but *not* updating the file system metadata to reflect that change.

*) Submit the data block write

*) On write completion, update the metadata blocks in a kernel thread.

Quoted 7 lines · Matthew Wilcox

As I recall, the bookmark hack was introduced in order to handle lock_page() problems. It wasn't really supposed to handle writeback, but nobody thought it would cause any harm (and indeed, it didn't at the time). So how about we only use bookmarks for lock_page(), since lock_page() usually doesn't have the multiple-waker semantics that writeback has?

I was hoping that some of the page lock problems are gone and we could maybe try to get rid of the bookmarks entirely.

But the page lock issues only ever showed up on some private proprietary load and machine, so we never really got confirmation that they are fixed. There were lots of strong signs to them being related to the migration page locking, and it may be that the bookmark code is only hurting these days.

See for example commit 9a1ea439b16b ("mm:
put_and_wait_on_page_locked() while page is migrated") which doesn't actually change the *locking* side, but drops the page reference when waiting for the locked page to be unlocked, which in turn removes a "loop and try again when migration". And that may have been the real _fix_ for the problem.

Because while the bookmark thing avoids the NMI lockup detector firing due to excessive hold times, the bookmarking also _causes_ that "we now will see the same page multiple times because we dropped the lock and somebody re-added it at the end of the queue" issue. Which seems to be the problem here.

Ugh. I wish we had some way to test "could we just remove the bookmark code entirely again".

Of course, the PG_lock case also works fairly hard to not actually remove and re-add the lock waiter to the queue, but having an actual "wait for and get the lock" operation. The writeback bit isn't done that way.

I do hate how we had to make folio_wait_writeback{_killable}() use "while" rather than an "if". It *almost* works with just a "wait for current writeback", but not quite. See commit c2407cf7d22d ("mm: make wait_on_page_writeback() wait for multiple pending writebacks") for why we have to loop. Ugly, ugly.

Because I do think that "while" in the writeback waiting is a problem. Maybe _the_ problem.

Linus

Quoted 13 lines · Brian Foster

What seems to happen is that the majority of the fsync calls end up waiting on writeback of a particular page, the wakeup of the writeback bit on that page wakes a task that immediately resets PG_writeback on the page such that N other folio_wait_writeback() waiters see the bit still set and immediately place themselves back onto the tail of the wait queue. Meanwhile the waker task spins in the WQ_FLAG_BOOKMARK loop in folio_wake_bit() (backing off the lock for a cycle or so in each iteration) only to find the same bunch of tasks in the queue. This process repeats for a long enough amount of time to trigger the soft lockup warning. I've confirmed this spinning behavior with a tracepoint in the bookmark loop that indicates we're stuck for many hundreds of thousands of iterations (at least) of this loop when the soft lockup warning triggers.

[...]

Quoted 7 lines

I've run a few quick experiments to try and corroborate this analysis. The problem goes away completely if I either back out the loop change in folio_wait_writeback() or bump WAITQUEUE_WALK_BREAK_CNT to something like 128 (i.e. greater than the total possible number of waiter tasks in this test). I've also played a few games with bookmark behavior mostly out of curiosity, but usually end up introducing other problems like missed wakeups, etc.

As I recall, the bookmark hack was introduced in order to handle lock_page() problems. It wasn't really supposed to handle writeback, but nobody thought it would cause any harm (and indeed, it didn't at the time). So how about we only use bookmarks for lock_page(), since lock_page() usually doesn't have the multiple-waker semantics that writeback has?

(this is more in the spirit of "minimal patch" -- I think initialising the bookmark should be moved to folio_unlock()).

mm/filemap.c +17 -12

index b2728eb52407..9ee3c5f1f489 100644
@@ -1146,26 +1146,28 @@ static int wake_page_function(wait_queue_entry_t *wait, unsigned mode, int sync,
 	return (flags & WQ_FLAG_EXCLUSIVE) != 0;
 }
 
-static void folio_wake_bit(struct folio *folio, int bit_nr)
+static void folio_wake_bit(struct folio *folio, int bit_nr,
+		wait_queue_entry_t *bookmark)
 {
 	wait_queue_head_t *q = folio_waitqueue(folio);
 	struct wait_page_key key;
 	unsigned long flags;
-	wait_queue_entry_t bookmark;
 
 	key.folio = folio;
 	key.bit_nr = bit_nr;
 	key.page_match = 0;
 
-	bookmark.flags = 0;
-	bookmark.private = NULL;
-	bookmark.func = NULL;
-	INIT_LIST_HEAD(&bookmark.entry);
+	if (bookmark) {
+		bookmark->flags = 0;
+		bookmark->private = NULL;
+		bookmark->func = NULL;
+		INIT_LIST_HEAD(&bookmark->entry);
+	}
 
 	spin_lock_irqsave(&q->lock, flags);
-	__wake_up_locked_key_bookmark(q, TASK_NORMAL, &key, &bookmark);
+	__wake_up_locked_key_bookmark(q, TASK_NORMAL, &key, bookmark);
 
-	while (bookmark.flags & WQ_FLAG_BOOKMARK) {
+	while (bookmark && (bookmark->flags & WQ_FLAG_BOOKMARK)) {
 		/*
 		 * Take a breather from holding the lock,
 		 * allow pages that finish wake up asynchronously

@@ -1175,7 +1177,7 @@ static void folio_wake_bit(struct folio *folio, int bit_nr)
 		spin_unlock_irqrestore(&q->lock, flags);
 		cpu_relax();
 		spin_lock_irqsave(&q->lock, flags);
-		__wake_up_locked_key_bookmark(q, TASK_NORMAL, &key, &bookmark);
+		__wake_up_locked_key_bookmark(q, TASK_NORMAL, &key, bookmark);
 	}
 
 	/*

@@ -1204,7 +1206,7 @@ static void folio_wake(struct folio *folio, int bit)
 {
 	if (!folio_test_waiters(folio))
 		return;
-	folio_wake_bit(folio, bit);
+	folio_wake_bit(folio, bit, NULL);
 }
 
 /*

@@ -1554,12 +1556,15 @@ static inline bool clear_bit_unlock_is_negative_byte(long nr, volatile void *mem
  */
 void folio_unlock(struct folio *folio)
 {
+	wait_queue_entry_t bookmark;
+
 	/* Bit 7 allows x86 to check the byte's sign bit */
 	BUILD_BUG_ON(PG_waiters != 7);
 	BUILD_BUG_ON(PG_locked > 7);
 	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
+
 	if (clear_bit_unlock_is_negative_byte(PG_locked, folio_flags(folio, 0)))
-		folio_wake_bit(folio, PG_locked);
+		folio_wake_bit(folio, PG_locked, &bookmark);
 }
 EXPORT_SYMBOL(folio_unlock);
 

@@ -1578,7 +1583,7 @@ void folio_end_private_2(struct folio *folio)
 {
 	VM_BUG_ON_FOLIO(!folio_test_private_2(folio), folio);
 	clear_bit_unlock(PG_private_2, folio_flags(folio, 0));
-	folio_wake_bit(folio, PG_private_2);
+	folio_wake_bit(folio, PG_private_2, NULL);
 	folio_put(folio);
 }
 EXPORT_SYMBOL(folio_end_private_2);

Quoted 25 lines · Matthew Wilcox

Quoted 13 lines · Brian Foster

What seems to happen is that the majority of the fsync calls end up waiting on writeback of a particular page, the wakeup of the writeback bit on that page wakes a task that immediately resets PG_writeback on the page such that N other folio_wait_writeback() waiters see the bit still set and immediately place themselves back onto the tail of the wait queue. Meanwhile the waker task spins in the WQ_FLAG_BOOKMARK loop in folio_wake_bit() (backing off the lock for a cycle or so in each iteration) only to find the same bunch of tasks in the queue. This process repeats for a long enough amount of time to trigger the soft lockup warning. I've confirmed this spinning behavior with a tracepoint in the bookmark loop that indicates we're stuck for many hundreds of thousands of iterations (at least) of this loop when the soft lockup warning triggers.

[...]

Quoted 7 lines

I've run a few quick experiments to try and corroborate this analysis. The problem goes away completely if I either back out the loop change in folio_wait_writeback() or bump WAITQUEUE_WALK_BREAK_CNT to something like 128 (i.e. greater than the total possible number of waiter tasks in this test). I've also played a few games with bookmark behavior mostly out of curiosity, but usually end up introducing other problems like missed wakeups, etc.

Quoted 7 lines

As I recall, the bookmark hack was introduced in order to handle lock_page() problems. It wasn't really supposed to handle writeback, but nobody thought it would cause any harm (and indeed, it didn't at the time). So how about we only use bookmarks for lock_page(), since lock_page() usually doesn't have the multiple-waker semantics that writeback has?

Oh, interesting. I wasn't aware of the tenuous status of the bookmark code. This is indeed much nicer than anything I was playing with. I suspect it will address the problem, but I'll throw it at my test env for a while and follow up.. thanks!

Brian

Quoted 89 lines

(this is more in the spirit of "minimal patch" -- I think initialising the bookmark should be moved to folio_unlock()).

mm/filemap.c index renamed from mm/filemap.c +0 -0

b2728eb52407..9ee3c5f1f489 100644

mm/filemap.c +17 -12

@@ -1146,26 +1146,28 @@ static int wake_page_function(wait_queue_entry_t *wait, unsigned mode, int sync,
 	return (flags & WQ_FLAG_EXCLUSIVE) != 0;
 }
 
-static void folio_wake_bit(struct folio *folio, int bit_nr)
+static void folio_wake_bit(struct folio *folio, int bit_nr,
+		wait_queue_entry_t *bookmark)
 {
 	wait_queue_head_t *q = folio_waitqueue(folio);
 	struct wait_page_key key;
 	unsigned long flags;
-	wait_queue_entry_t bookmark;
 
 	key.folio = folio;
 	key.bit_nr = bit_nr;
 	key.page_match = 0;
 
-	bookmark.flags = 0;
-	bookmark.private = NULL;
-	bookmark.func = NULL;
-	INIT_LIST_HEAD(&bookmark.entry);
+	if (bookmark) {
+		bookmark->flags = 0;
+		bookmark->private = NULL;
+		bookmark->func = NULL;
+		INIT_LIST_HEAD(&bookmark->entry);
+	}
 
 	spin_lock_irqsave(&q->lock, flags);
-	__wake_up_locked_key_bookmark(q, TASK_NORMAL, &key, &bookmark);
+	__wake_up_locked_key_bookmark(q, TASK_NORMAL, &key, bookmark);
 
-	while (bookmark.flags & WQ_FLAG_BOOKMARK) {
+	while (bookmark && (bookmark->flags & WQ_FLAG_BOOKMARK)) {
 		/*
 		 * Take a breather from holding the lock,
 		 * allow pages that finish wake up asynchronously

@@ -1175,7 +1177,7 @@ static void folio_wake_bit(struct folio *folio, int bit_nr)
 		spin_unlock_irqrestore(&q->lock, flags);
 		cpu_relax();
 		spin_lock_irqsave(&q->lock, flags);
-		__wake_up_locked_key_bookmark(q, TASK_NORMAL, &key, &bookmark);
+		__wake_up_locked_key_bookmark(q, TASK_NORMAL, &key, bookmark);
 	}
 
 	/*

@@ -1204,7 +1206,7 @@ static void folio_wake(struct folio *folio, int bit)
 {
 	if (!folio_test_waiters(folio))
 		return;
-	folio_wake_bit(folio, bit);
+	folio_wake_bit(folio, bit, NULL);
 }
 
 /*

@@ -1554,12 +1556,15 @@ static inline bool clear_bit_unlock_is_negative_byte(long nr, volatile void *mem
  */
 void folio_unlock(struct folio *folio)
 {
+	wait_queue_entry_t bookmark;
+
 	/* Bit 7 allows x86 to check the byte's sign bit */
 	BUILD_BUG_ON(PG_waiters != 7);
 	BUILD_BUG_ON(PG_locked > 7);
 	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
+
 	if (clear_bit_unlock_is_negative_byte(PG_locked, folio_flags(folio, 0)))
-		folio_wake_bit(folio, PG_locked);
+		folio_wake_bit(folio, PG_locked, &bookmark);
 }
 EXPORT_SYMBOL(folio_unlock);
 

@@ -1578,7 +1583,7 @@ void folio_end_private_2(struct folio *folio)
 {
 	VM_BUG_ON_FOLIO(!folio_test_private_2(folio), folio);
 	clear_bit_unlock(PG_private_2, folio_flags(folio, 0));
-	folio_wake_bit(folio, PG_private_2);
+	folio_wake_bit(folio, PG_private_2, NULL);
 	folio_put(folio);
 }
 EXPORT_SYMBOL(folio_end_private_2);

So I'm not clear on where we're at with this patch vs. something that moves (or drops) the wb wait loop vs. the wait and set thing (which seems more invasive and longer term), but FWIW.. this patch survived over 10k iterations of the reproducer test yesterday (the problem typically reproduces in ~1k or so iterations on average) and an overnight fstests run without regression.

Excellent! I'm going to propose these two patches for -rc1 (I don't think we want to be playing with this after -rc8)

Quoted 3 lines · Matthew Wilcox

Excellent! I'm going to propose these two patches for -rc1 (I don't think we want to be playing with this after -rc8)

Ack. I think your commit message may be a bit too optimistic (who knows if other loads can trigger the over-long page locking wait-queue latencies), but since I don't see any other ways to really check this than just trying it, let's do it.

Linus

A report from a tester with this call trace:

 watchdog: BUG: soft lockup - CPU#127 stuck for 134s! [ksoftirqd/127:782]
 RIP: 0010:_raw_spin_unlock_irqrestore+0x19/0x40 [..]

...lead me to this thread. This was after I had them force all softirqs to run in ksoftirqd context, and run with rq_affinity == 2 to force I/O completion work to throttle new submissions.

Willy, are these headed upstream:

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

...or I am missing an alternate solution posted elsewhere?

Quoted 6 lines · Dan Williams

A report from a tester with this call trace:

 watchdog: BUG: soft lockup - CPU#127 stuck for 134s!
[ksoftirqd/127:782]
 RIP: 0010:_raw_spin_unlock_irqrestore+0x19/0x40 [..]

Whee.

Quoted 10 lines

...lead me to this thread. This was after I had them force all softirqs
to run in ksoftirqd context, and run with rq_affinity == 2 to force I/O completion work to throttle new submissions.

Willy, are these headed upstream:

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

...or I am missing an alternate solution posted elsewhere?

Can your reporter test that patch? I think it should still apply pretty much as-is.. And if we actually had somebody who had a test-case that was literally fixed by getting rid of the old bookmark code, that would make applying that patch a no-brainer.

The problem is that the original load that caused us to do that thing in the first place isn't repeatable because it was special production code - so removing that bookmark code because we _think_ it now hurts more than it helps is kind of a big hurdle.

But if we had some hard confirmation from somebody that "yes, the bookmark code is now hurting", that would make it a lot more palatable
to just remove the code that we just _think_ that probably isn't needed any more..

I do think that the original locked page on migration problem was fixed by commit 9a1ea439b16b. Unfortunately the customer did not respond to us when we asked them to test their workload when that patch went into the mainline.

I don't have objection to Matthew's fix to remove the bookmark code, now that it is causing problems with this scenario that I didn't anticipate in my original code.

Quoted 5 lines · Tim Chen

I do think that the original locked page on migration problem was fixed by commit 9a1ea439b16b. Unfortunately the customer did not respond to us when we asked them to test their workload when that patch went into the mainline.

Oh well.

Quoted 3 lines

I don't have objection to Matthew's fix to remove the bookmark code, now that it is causing problems with this scenario that I didn't anticipate in my original code.

I'd really like to avoid *another* "we can't actually verify that this helps" change in this area, so I'm hoping that the reporter that Dan was talking to could test that patch.

Otherwise we're kind of going back-and-forth based on "this might fix things", which just feels really fragile and reminds me of the bad old days when we had the "one step forward, two steps back" dance with some of the suspend/resume issues.

I realize that this code needs some extreme loads (and likely pretty special hardware too) to actually become problematic, so testing is _always_ going to be a bit of a problem, but still...

Quoted 5 lines · Dan Williams

A report from a tester with this call trace:

 watchdog: BUG: soft lockup - CPU#127 stuck for 134s! [ksoftirqd/127:782]
 RIP: 0010:_raw_spin_unlock_irqrestore+0x19/0x40 [..]

Whee.

Quoted 9 lines

...lead me to this thread. This was after I had them force all softirqs to run in ksoftirqd context, and run with rq_affinity == 2 to force I/O completion work to throttle new submissions.

Willy, are these headed upstream:

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

...or I am missing an alternate solution posted elsewhere?

Can your reporter test that patch? I think it should still apply pretty much as-is.. And if we actually had somebody who had a test-case that was literally fixed by getting rid of the old bookmark code, that would make applying that patch a no-brainer.

The problem is that the original load that caused us to do that thing in the first place isn't repeatable because it was special production code - so removing that bookmark code because we _think_ it now hurts more than it helps is kind of a big hurdle.

But if we had some hard confirmation from somebody that "yes, the bookmark code is now hurting", that would make it a lot more palatable to just remove the code that we just _think_ that probably isn't needed any more..

Arechiga reports that his test case that failed "fast" before now ran for 28 hours without a soft lockup report with the proposed patches applied. So, I would consider those:

Tested-by Jesus Arechiga Lopez <[email protected]>

Quoted 6 lines · Dan Williams

Arechiga reports that his test case that failed "fast" before now ran for 28 hours without a soft lockup report with the proposed patches applied. So, I would consider those:

Tested-by Jesus Arechiga Lopez <[email protected]>

Ok, great.

I really like that patch myself (and obviously liked it back when it was originally proposed), but I think it was always held back by the fact that we didn't really have any hard data for it.

It does sound like we now very much have hard data for "the page waitlist complexity is now a bigger problem than the historical problem it tried to solve".

So I'll happily apply it. The only question is whether it's a "let's do this for 6.2", or if it's something that we'd want to back-port anyway, and might as well apply sooner rather than later as a fix.

I think that in turn then depends on just how artificial the test case was. If the test case was triggered by somebody seeing problems in real life loads, that would make the urgency a lot higher. But if it was purely a synthetic test case with no accompanying "this is what made us look at this" problem, it might be a 6.2 thing.

Arechiga?

-----Original Message-----
From: Williams, Dan J <[email protected]>
Sent: Monday, October 24, 2022 3:36 PM
To: Torvalds, Linus <[email protected]>; Williams, Dan J <[email protected]>
Cc: Matthew Wilcox <[email protected]>; Brian Foster <[email protected]>; Linux-MM <[email protected]>; linux-fsdevel <[email protected]>; linux-xfs <[email protected]>; Hugh Dickins <[email protected]>; Arechiga Lopez, Jesus A <[email protected]>; [email protected]
Subject: Re: writeback completion soft lockup BUG in folio_wake_bit()

Quoted 1 line · Linus Torvalds

On Mon, Oct 24, 2022 at 1:13 PM Dan Williams <[email protected]>

Quoted 27 lines · wrote:

Quoted 6 lines

Arechiga reports that his test case that failed "fast" before now ran for 28 hours without a soft lockup report with the proposed patches applied. So, I would consider those:

Tested-by Jesus Arechiga Lopez <[email protected]>

Ok, great.

I really like that patch myself (and obviously liked it back when it was originally proposed), but I think it was always held back by the fact that we didn't really have any hard data for it.

It does sound like we now very much have hard data for "the page waitlist complexity is now a bigger problem than the historical problem it tried to solve".

So I'll happily apply it. The only question is whether it's a "let's do this for 6.2", or if it's something that we'd want to back-port anyway, and might as well apply sooner rather than later as a fix.

I think that in turn then depends on just how artificial the test case was. If the test case was triggered by somebody seeing problems in real life loads, that would make the urgency a lot higher. But if it was purely a synthetic test case with no accompanying "this is what made us look at this" problem, it might be a 6.2 thing.

Quoted 1 line

Arechiga?

I will let Arechiga reply as well, but my sense is that this is more in the latter camp of not urgent because the test case is trying to generate platform stress (success!), not necessarily trying to get real work done.

It does sound like we now very much have hard data for "the page waitlist complexity is now a bigger problem than the historical problem it tried to solve".

So I'll happily apply it. The only question is whether it's a "let's do this for 6.2", or if it's something that we'd want to back-port anyway, and might as well apply sooner rather than later as a fix.

I think that in turn then depends on just how artificial the test case was. If the test case was triggered by somebody seeing problems in real life loads, that would make the urgency a lot higher. But if it was purely a synthetic test case with no accompanying "this is what made us look at this" problem, it might be a 6.2 thing.

Arechiga?

Also, considering that Willy authored the patch (even if it's really just a "remove this whole code logic"), maybe he has opinions? Willy?

I've been carrying a pair of patches in my tree to rip out the wait bookmarks since March, waiting for me to write a userspace testcase to reproduce the problem against v4.13 and then check it no longer does so against v5.0. Unfortunately, that hasn't happened. I'm happy to add Arechiga's Tested-by, and submit them to Andrew and have him bring them into v6.2, since this doesn't seem urgent?