Re: [PATCH 2/2] xfs_repair: retain superblock buffer to avoid write hook deadlock

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Aug 12, 2022 at 08:15:41AM +1000, Dave Chinner wrote:
> On Tue, Aug 09, 2022 at 02:06:57PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Every now and then I experience the following deadlock in xfs_repair
> > when I'm running the offline repair fuzz tests:
> > 
> > #0  futex_wait (private=0, expected=2, futex_word=0x55555566df70) at ../sysdeps/nptl/futex-internal.h:146
> > #1  __GI___lll_lock_wait (futex=futex@entry=0x55555566df70, private=0) at ./nptl/lowlevellock.c:49
> > #2  lll_mutex_lock_optimized (mutex=0x55555566df70) at ./nptl/pthread_mutex_lock.c:48
> > #3  ___pthread_mutex_lock (mutex=mutex@entry=0x55555566df70) at ./nptl/pthread_mutex_lock.c:93
> > #4  cache_shake (cache=cache@entry=0x55555566de60, priority=priority@entry=2, purge=purge@entry=false) at cache.c:231
> > #5  cache_node_get (cache=cache@entry=0x55555566de60, key=key@entry=0x7fffe55e01b0, nodep=nodep@entry=0x7fffe55e0168) at cache.c:452
> > #6  __cache_lookup (key=key@entry=0x7fffe55e01b0, flags=0, bpp=bpp@entry=0x7fffe55e0228) at rdwr.c:405
> > #7  libxfs_getbuf_flags (btp=0x55555566de00, blkno=0, len=<optimized out>, flags=<optimized out>, bpp=0x7fffe55e0228) at rdwr.c:457
> > #8  libxfs_buf_read_map (btp=0x55555566de00, map=map@entry=0x7fffe55e0280, nmaps=nmaps@entry=1, flags=flags@entry=0, bpp=bpp@entry=0x7fffe55e0278, ops=0x5555556233e0 <xfs_sb_buf_ops>)
> >     at rdwr.c:704
> > #9  libxfs_buf_read (ops=<optimized out>, bpp=0x7fffe55e0278, flags=0, numblks=<optimized out>, blkno=0, target=<optimized out>)
> >     at /storage/home/djwong/cdev/work/xfsprogs/build-x86_64/libxfs/libxfs_io.h:195
> > #10 libxfs_getsb (mp=mp@entry=0x7fffffffd690) at rdwr.c:162
> > #11 force_needsrepair (mp=0x7fffffffd690) at xfs_repair.c:924
> > #12 repair_capture_writeback (bp=<optimized out>) at xfs_repair.c:1000
> > #13 libxfs_bwrite (bp=0x7fffe011e530) at rdwr.c:869
> > #14 cache_shake (cache=cache@entry=0x55555566de60, priority=priority@entry=2, purge=purge@entry=false) at cache.c:240
> > #15 cache_node_get (cache=cache@entry=0x55555566de60, key=key@entry=0x7fffe55e0470, nodep=nodep@entry=0x7fffe55e0428) at cache.c:452
> > #16 __cache_lookup (key=key@entry=0x7fffe55e0470, flags=1, bpp=bpp@entry=0x7fffe55e0538) at rdwr.c:405
> > #17 libxfs_getbuf_flags (btp=0x55555566de00, blkno=12736, len=<optimized out>, flags=<optimized out>, bpp=0x7fffe55e0538) at rdwr.c:457
> > #18 __libxfs_buf_get_map (btp=<optimized out>, map=map@entry=0x7fffe55e05b0, nmaps=<optimized out>, flags=flags@entry=1, bpp=bpp@entry=0x7fffe55e0538) at rdwr.c:501
> > #19 libxfs_buf_get_map (btp=<optimized out>, map=map@entry=0x7fffe55e05b0, nmaps=<optimized out>, flags=flags@entry=1, bpp=bpp@entry=0x7fffe55e0538) at rdwr.c:525
> > #20 pf_queue_io (args=args@entry=0x5555556722c0, map=map@entry=0x7fffe55e05b0, nmaps=<optimized out>, flag=flag@entry=11) at prefetch.c:124
> > #21 pf_read_bmbt_reclist (args=0x5555556722c0, rp=<optimized out>, numrecs=78) at prefetch.c:220
> > #22 pf_scan_lbtree (dbno=dbno@entry=1211, level=level@entry=1, isadir=isadir@entry=1, args=args@entry=0x5555556722c0, func=0x55555557f240 <pf_scanfunc_bmap>) at prefetch.c:298
> > #23 pf_read_btinode (isadir=1, dino=<optimized out>, args=0x5555556722c0) at prefetch.c:385
> > #24 pf_read_inode_dirs (args=args@entry=0x5555556722c0, bp=bp@entry=0x7fffdc023790) at prefetch.c:459
> > #25 pf_read_inode_dirs (bp=<optimized out>, args=0x5555556722c0) at prefetch.c:411
> > #26 pf_batch_read (args=args@entry=0x5555556722c0, which=which@entry=PF_PRIMARY, buf=buf@entry=0x7fffd001d000) at prefetch.c:609
> > #27 pf_io_worker (param=0x5555556722c0) at prefetch.c:673
> > #28 start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
> > #29 clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> > 
> > From this stack trace, we see that xfs_repair's prefetch module is
> > getting some xfs_buf objects ahead of initiating a read (#19).  The
> > buffer cache has hit its limit, so it calls cache_shake (#14) to free
> > some unused xfs_bufs.  The buffer it finds is a dirty buffer, so it
> > calls libxfs_bwrite to flush it out to disk, which in turn invokes the
> > buffer write hook that xfs_repair set up in 3b7667cb to mark the ondisk
> > filesystem's superblock as NEEDSREPAIR until repair actually completes.
> > 
> > Unfortunately, the NEEDSREPAIR handler itself needs to grab the
> > superblock buffer, so it makes another call into the buffer cache (#9),
> > which sees that the cache is full and tries to shake it(#4).  Hence we
> > deadlock on cm_mutex because shaking is not reentrant.
> > 
> > Fix this by retaining a reference to the superblock buffer when possible
> > so that the writeback hook doesn't have to access the buffer cache to
> > set NEEDSREPAIR.
> 
> If we are going to "retain" a permanent reference to the superblock
> buffer, can we just do it the same way as the kernel does at mount
> time? i.e. do this for every xfs_mount instance via an uncached
> buffer, and attach it to mp->m_sb_bp so that the
> superblock can be grabbed at any time in any context without needing
> to hit the buffer cache?

That was the first method that I tried, and promptly ran into all sorts
of weird issues, such (a) figuring out that someone's calling
libxfs_mount with no devices open so that it can compute something, and
(b) xfs_repair deciding to resize the buffer cache prior to phase 2.
That seemed like a whole lot of extra complexity to enable *one* use
case in one program while I was trying to help Eric get 5.19 out.

(Frankly, I'm already rather annoyed at the scope creep of NEEDSREPAIR,
and now it's creeping even more...)

> If we've got code that exists to do this in a generic manner that
> brings userspace closer to kernel behaviour, then that's what we
> should use rather than create a special one-off implementation for a
> single userspace binary...

I'd rather focus on getting through online repair's design review than
this, although given how sh*t email is, I have no idea if you (Dave)
stopped after patch 4 or if further replies simply got  eaten.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com