Re: 3.4.4-rt13: btrfs + xfstests 006 = BOOM.. and a bonus rt_mutexdeadlock report for absolutely free!

[Mike Galbraith]

On Sat, 2012-07-14 at 12:14 +0200, Mike Galbraith wrote: 
> On Fri, 2012-07-13 at 08:50 -0400, Chris Mason wrote: 
> > On Wed, Jul 11, 2012 at 11:47:40PM -0600, Mike Galbraith wrote:
> > > Greetings,
> > 
> > [ deadlocks with btrfs and the recent RT kernels ]
> > 
> > I talked with Thomas about this and I think the problem is the
> > single-reader nature of the RW rwlocks.  The lockdep report below
> > mentions that btrfs is calling:
> > 
> > > [  692.963099]  [<ffffffff811fabd2>] btrfs_clear_path_blocking+0x32/0x70
> > 
> > In this case, the task has a number of blocking read locks on the btrfs buffers,
> > and we're trying to turn them back into spinning read locks.  Even
> > though btrfs is taking the read rwlock, it doesn't think of this as a new
> > lock operation because we were blocking out new writers.
> > 
> > If the second task has taken the spinning read lock, it is going to
> > prevent that clear_path_blocking operation from progressing, even though
> > it would have worked on a non-RT kernel.
> > 
> > The solution should be to make the blocking read locks in btrfs honor the
> > single-reader semantics.  This means not allowing more than one blocking
> > reader and not allowing a spinning reader when there is a blocking
> > reader.  Strictly speaking btrfs shouldn't need recursive readers on a
> > single lock, so I wouldn't worry about that part.
> > 
> > There is also a chunk of code in btrfs_clear_path_blocking that makes
> > sure to strictly honor top down locking order during the conversion.  It
> > only does this when lockdep is enabled because in non-RT kernels we
> > don't need to worry about it.  For RT we'll want to enable that as well.
> > 
> > I'll give this a shot later today.
> 
> I took a poke at it.  Did I do something similar to what you had in
> mind, or just hide behind performance stealing paranoid trylock loops?
> Box survived 1000 x xfstests 006 and dbench [-s] massive right off the
> bat, so it gets posted despite skepticism.

Seems btrfs isn't entirely convinced either.

[ 2292.336229] use_block_rsv: 1810 callbacks suppressed
[ 2292.336231] ------------[ cut here ]------------
[ 2292.336255] WARNING: at fs/btrfs/extent-tree.c:6344 use_block_rsv+0x17d/0x190 [btrfs]()
[ 2292.336257] Hardware name: System x3550 M3 -[7944K3G]-
[ 2292.336259] btrfs: block rsv returned -28
[ 2292.336260] Modules linked in: joydev st sr_mod ide_gd_mod(N) ide_cd_mod ide_core cdrom ibm_rtl nfsd lockd ipmi_devintf nfs_acl auth_rpcgss sunrpc ipmi_si ipmi_msghandler ipv6 ipv6_lib af_packet cpufreq_conservative cpufreq_userspace cpufreq_powersave acpi_cpufreq mperf edd fuse btrfs zlib_deflate ext3 jbd loop dm_mod usbhid hid cdc_ether usbnet mii sg shpchp pci_hotplug pcspkr bnx2 ioatdma i2c_i801 i2c_core tpm_tis tpm tpm_bios serio_raw i7core_edac edac_core button dca iTCO_wdt iTCO_vendor_support ext4 mbcache jbd2 uhci_hcd ehci_hcd sd_mod usbcore rtc_cmos crc_t10dif usb_common fan processor ata_generic ata_piix libata megaraid_sas scsi_mod thermal thermal_sys hwmon
[ 2292.336296] Supported: Yes
[ 2292.336298] Pid: 12975, comm: bonnie Tainted: G        W  N  3.0.35-rt56-rt #27
[ 2292.336300] Call Trace:
[ 2292.336312]  [<ffffffff81004562>] dump_trace+0x82/0x2e0
[ 2292.336320]  [<ffffffff814542b3>] dump_stack+0x69/0x6f
[ 2292.336325]  [<ffffffff8105900b>] warn_slowpath_common+0x7b/0xc0
[ 2292.336330]  [<ffffffff81059105>] warn_slowpath_fmt+0x45/0x50
[ 2292.336342]  [<ffffffffa034db7d>] use_block_rsv+0x17d/0x190 [btrfs]
[ 2292.336389]  [<ffffffffa0350d49>] btrfs_alloc_free_block+0x49/0x240 [btrfs]
[ 2292.336432]  [<ffffffffa033d49e>] __btrfs_cow_block+0x13e/0x510 [btrfs]
[ 2292.336457]  [<ffffffffa033d96f>] btrfs_cow_block+0xff/0x230 [btrfs]
[ 2292.336482]  [<ffffffffa0341ab0>] btrfs_search_slot+0x360/0x7e0 [btrfs]
[ 2292.336513]  [<ffffffffa03567c5>] btrfs_del_csums+0x175/0x2f0 [btrfs]
[ 2292.336562]  [<ffffffffa034a0f0>] __btrfs_free_extent+0x550/0x760 [btrfs]
[ 2292.336599]  [<ffffffffa034a53d>] run_delayed_data_ref+0x9d/0x190 [btrfs]
[ 2292.336636]  [<ffffffffa034f355>] run_clustered_refs+0xd5/0x3a0 [btrfs]
[ 2292.336678]  [<ffffffffa034f768>] btrfs_run_delayed_refs+0x148/0x350 [btrfs]
[ 2292.336723]  [<ffffffffa0362047>] __btrfs_end_transaction+0xb7/0x2b0 [btrfs]
[ 2292.336796]  [<ffffffffa036d153>] btrfs_evict_inode+0x2d3/0x340 [btrfs]
[ 2292.336863]  [<ffffffff81170121>] evict+0x91/0x190
[ 2292.336868]  [<ffffffff81163c07>] do_unlinkat+0x177/0x1f0
[ 2292.336875]  [<ffffffff8145e312>] system_call_fastpath+0x16/0x1b
[ 2292.336881]  [<00007fea187f9e67>] 0x7fea187f9e66
[ 2292.336887] ---[ end trace 0000000000000004 ]---
[ 2610.370398] use_block_rsv: 1947 callbacks suppressed
[ 2610.370400] ------------[ cut here ]------------

> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 4106264..ae47cc2 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -77,7 +77,7 @@ noinline void btrfs_clear_path_blocking(struct btrfs_path *p,
>  {
>  	int i;
>  
> -#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +#if (defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PREEMPT_RT_BASE))
>  	/* lockdep really cares that we take all of these spinlocks
>  	 * in the right order.  If any of the locks in the path are not
>  	 * currently blocking, it is going to complain.  So, make really
> @@ -104,7 +104,7 @@ noinline void btrfs_clear_path_blocking(struct btrfs_path *p,
>  		}
>  	}
>  
> -#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +#if (defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PREEMPT_RT_BASE))
>  	if (held)
>  		btrfs_clear_lock_blocking_rw(held, held_rw);
>  #endif
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index 272f911..4db7c14 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -19,6 +19,7 @@
>  #include <linux/pagemap.h>
>  #include <linux/spinlock.h>
>  #include <linux/page-flags.h>
> +#include <linux/delay.h>
>  #include <asm/bug.h>
>  #include "ctree.h"
>  #include "extent_io.h"
> @@ -97,7 +98,18 @@ void btrfs_clear_lock_blocking_rw(struct extent_buffer *eb, int rw)
>  void btrfs_tree_read_lock(struct extent_buffer *eb)
>  {
>  again:
> +#ifdef CONFIG_PREEMPT_RT_BASE
> +	while (atomic_read(&eb->blocking_readers))
> +		cpu_chill();
> +	while(!read_trylock(&eb->lock))
> +		cpu_chill();
> +	if (atomic_read(&eb->blocking_readers)) {
> +		read_unlock(&eb->lock);
> +		goto again;
> +	}
> +#else
>  	read_lock(&eb->lock);
> +#endif
>  	if (atomic_read(&eb->blocking_writers) &&
>  	    current->pid == eb->lock_owner) {
>  		/*
> @@ -131,11 +143,26 @@ int btrfs_try_tree_read_lock(struct extent_buffer *eb)
>  	if (atomic_read(&eb->blocking_writers))
>  		return 0;
>  
> +#ifdef CONFIG_PREEMPT_RT_BASE
> +	if (atomic_read(&eb->blocking_readers))
> +		return 0;
> +	while(!read_trylock(&eb->lock))
> +		cpu_chill();
> +#else
>  	read_lock(&eb->lock);
> +#endif
> +
>  	if (atomic_read(&eb->blocking_writers)) {
>  		read_unlock(&eb->lock);
>  		return 0;
>  	}
> +
> +#ifdef CONFIG_PREEMPT_RT_BASE
> +	if (atomic_read(&eb->blocking_readers)) {
> +		read_unlock(&eb->lock);
> +		return 0;
> +	}
> +#endif
>  	atomic_inc(&eb->read_locks);
>  	atomic_inc(&eb->spinning_readers);
>  	return 1;
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/