diff mbox series

[2/2] xfs: periodically yield scrub threads to the scheduler

Message ID 157301538629.678524.5328247190031479757.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: fix scrub timeout warnings | expand

Commit Message

Darrick J. Wong Nov. 6, 2019, 4:43 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Christoph Hellwig complained about the following soft lockup warning
when running scrub after generic/175 when preemption is disabled and
slub debugging is enabled:

watchdog: BUG: soft lockup - CPU#3 stuck for 22s! [xfs_scrub:161]
Modules linked in:
irq event stamp: 41692326
hardirqs last  enabled at (41692325): [<ffffffff8232c3b7>] _raw_0
hardirqs last disabled at (41692326): [<ffffffff81001c5a>] trace0
softirqs last  enabled at (41684994): [<ffffffff8260031f>] __do_e
softirqs last disabled at (41684987): [<ffffffff81127d8c>] irq_e0
CPU: 3 PID: 16189 Comm: xfs_scrub Not tainted 5.4.0-rc3+ #30
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.124
RIP: 0010:_raw_spin_unlock_irqrestore+0x39/0x40
Code: 89 f3 be 01 00 00 00 e8 d5 3a e5 fe 48 89 ef e8 ed 87 e5 f2
RSP: 0018:ffffc9000233f970 EFLAGS: 00000286 ORIG_RAX: ffffffffff3
RAX: ffff88813b398040 RBX: 0000000000000286 RCX: 0000000000000006
RDX: 0000000000000006 RSI: ffff88813b3988c0 RDI: ffff88813b398040
RBP: ffff888137958640 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffffea00042b0c00
R13: 0000000000000001 R14: ffff88810ac32308 R15: ffff8881376fc040
FS:  00007f6113dea700(0000) GS:ffff88813bb80000(0000) knlGS:00000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f6113de8ff8 CR3: 000000012f290000 CR4: 00000000000006e0
Call Trace:
 free_debug_processing+0x1dd/0x240
 __slab_free+0x231/0x410
 kmem_cache_free+0x30e/0x360
 xchk_ag_btcur_free+0x76/0xb0
 xchk_ag_free+0x10/0x80
 xchk_bmap_iextent_xref.isra.14+0xd9/0x120
 xchk_bmap_iextent+0x187/0x210
 xchk_bmap+0x2e0/0x3b0
 xfs_scrub_metadata+0x2e7/0x500
 xfs_ioc_scrub_metadata+0x4a/0xa0
 xfs_file_ioctl+0x58a/0xcd0
 do_vfs_ioctl+0xa0/0x6f0
 ksys_ioctl+0x5b/0x90
 __x64_sys_ioctl+0x11/0x20
 do_syscall_64+0x4b/0x1a0
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

If preemption is disabled, all metadata buffers needed to perform the
scrub are already in memory, and there are a lot of records to check,
it's possible that the scrub thread will run for an extended period of
time without sleeping for IO or any other reason.  Then the watchdog
timer or the RCU stall timeout can trigger, producing the backtrace
above.

To fix this problem, we detect when preemption is disabled and
explicitly schedule() the scrub thread every few seconds.

Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/common.h |   14 +++++++++++++-
 fs/xfs/scrub/scrub.h  |    9 +++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig Nov. 6, 2019, 2:44 p.m. UTC | #1 
On Tue, Nov 05, 2019 at 08:43:06PM -0800, Darrick J. Wong wrote:
> +++ b/fs/xfs/scrub/common.h
> @@ -14,8 +14,20 @@
>  static inline bool
>  xchk_should_terminate(
>  	struct xfs_scrub	*sc,
> -	int				*error)
> +	int			*error)
>  {
> +#if !IS_ENABLED(CONFIG_PREEMPT)
> +	/*
> +	 * If preemption is disabled, we need to yield to the scheduler every
> +	 * few seconds so that we don't run afoul of the soft lockup watchdog
> +	 * or RCU stall detector.
> +	 */
> +	if (sc->next_yield != 0 && time_after(jiffies, sc->next_yield))
> +		return false;
> +	schedule();
> +	sc->next_yield = jiffies + msecs_to_jiffies(5000);
> +#endif

This looks weird.  Can't we just do a cond_resched() here?
Darrick J. Wong Nov. 6, 2019, 4:13 p.m. UTC | #2 
On Wed, Nov 06, 2019 at 03:44:46PM +0100, Christoph Hellwig wrote:
> On Tue, Nov 05, 2019 at 08:43:06PM -0800, Darrick J. Wong wrote:
> > +++ b/fs/xfs/scrub/common.h
> > @@ -14,8 +14,20 @@
> >  static inline bool
> >  xchk_should_terminate(
> >  	struct xfs_scrub	*sc,
> > -	int				*error)
> > +	int			*error)
> >  {
> > +#if !IS_ENABLED(CONFIG_PREEMPT)
> > +	/*
> > +	 * If preemption is disabled, we need to yield to the scheduler every
> > +	 * few seconds so that we don't run afoul of the soft lockup watchdog
> > +	 * or RCU stall detector.
> > +	 */
> > +	if (sc->next_yield != 0 && time_after(jiffies, sc->next_yield))
> > +		return false;
> > +	schedule();
> > +	sc->next_yield = jiffies + msecs_to_jiffies(5000);
> > +#endif
> 
> This looks weird.  Can't we just do a cond_resched() here?

DOH.  Yes, probably.  Dave even suggested it a few nights ago to fix a
similar problem and apparently I forgot.  Will fix. :(

--D
diff mbox series

Patch

diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index 003a772cd26c..597b4d45e990 100644
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -14,8 +14,20 @@ 
 static inline bool
 xchk_should_terminate(
 	struct xfs_scrub	*sc,
-	int				*error)
+	int			*error)
 {
+#if !IS_ENABLED(CONFIG_PREEMPT)
+	/*
+	 * If preemption is disabled, we need to yield to the scheduler every
+	 * few seconds so that we don't run afoul of the soft lockup watchdog
+	 * or RCU stall detector.
+	 */
+	if (sc->next_yield != 0 && time_after(jiffies, sc->next_yield))
+		return false;
+	schedule();
+	sc->next_yield = jiffies + msecs_to_jiffies(5000);
+#endif
+
 	if (fatal_signal_pending(current)) {
 		if (*error == 0)
 			*error = -EAGAIN;
diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
index ad1ceb44a628..ada8e4976024 100644
--- a/fs/xfs/scrub/scrub.h
+++ b/fs/xfs/scrub/scrub.h
@@ -75,6 +75,15 @@  struct xfs_scrub {
 
 	/* State tracking for single-AG operations. */
 	struct xchk_ag			sa;
+
+#if !IS_ENABLED(CONFIG_PREEMPT)
+	/*
+	 * This is the time (in jiffies) when this scrub thread needs to
+	 * yield the processor back to the scheduler so that we don't run
+	 * afoul of either the soft lockup watchdog or RCU stall detector.
+	 */
+	unsigned long			next_yield;
+#endif
 };
 
 /* XCHK state flags grow up from zero, XREP state flags grown down from 2^31 */