Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1491391 > unrolled thread

[PATCH 0/2] (Was: BUG_ON in rcu_sync_func triggered)

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] (Was: BUG_ON in rcu_sync_func triggered) Oleg Nesterov <oleg@redhat.com> - 2016-09-26 18:10 +0200
    [PATCH 2/2] fs/super.c: don't fool lockdep in freeze_super() and  thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-09-26 18:10 +0200
      Re: [PATCH 2/2] fs/super.c: don't fool lockdep in freeze_super() and  thaw_super() paths Jan Kara <jack@suse.cz> - 2016-09-26 18:20 +0200
        [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super() and  thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-09-26 19:00 +0200
          Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Jan Kara <jack@suse.cz> - 2016-09-27 09:00 +0200
            Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Dave Chinner <david@fromorbit.com> - 2016-09-27 09:20 +0200
            Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-09-27 19:30 +0200
              Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-09-30 19:20 +0200
                Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Dave Chinner <david@fromorbit.com> - 2016-10-02 23:50 +0200
                  Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in         freeze_super() and thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-10-03 18:50 +0200
                    Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-10-04 19:00 +0200
                      Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Dave Chinner <david@fromorbit.com> - 2016-10-04 22:10 +0200
                        Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-10-05 18:40 +0200
                    Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Dave Chinner <david@fromorbit.com> - 2016-10-04 21:50 +0200
                      Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-10-05 18:50 +0200
                        Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Jan Kara <jack@suse.cz> - 2016-10-06 09:30 +0200
                        Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-10-06 19:20 +0200
                          Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Dave Chinner <david@fromorbit.com> - 2016-10-07 00:00 +0200
                            Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-10-07 19:20 +0200
                              Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Dave Chinner <david@fromorbit.com> - 2016-10-08 01:00 +0200
                                Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in         freeze_super() and thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-10-09 18:20 +0200
                                  Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Dave Chinner <david@fromorbit.com> - 2016-10-10 03:20 +0200
                    Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super()  and thaw_super() paths Johannes Weiner <hannes@cmpxchg.org> - 2016-10-06 15:50 +0200
                      Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in         freeze_super() and thaw_super() paths Oleg Nesterov <oleg@redhat.com> - 2016-10-07 19:00 +0200
    [PATCH 1/2] fs/super.c: fix race between freeze_super() and  thaw_super() Oleg Nesterov <oleg@redhat.com> - 2016-09-26 18:10 +0200
      Re: [PATCH 1/2] fs/super.c: fix race between freeze_super() and  thaw_super() Jan Kara <jack@suse.cz> - 2016-09-26 18:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1497975 — Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super() and thaw_super() paths

On 10/08, Dave Chinner wrote:
>
> On Fri, Oct 07, 2016 at 07:15:18PM +0200, Oleg Nesterov wrote:
> > > >
> > > > --- x/fs/xfs/xfs_trans.c
> > > > +++ x/fs/xfs/xfs_trans.c
> > > > @@ -245,7 +245,8 @@ xfs_trans_alloc(
> > > >  	atomic_inc(&mp->m_active_trans);
> > > >
> > > >  	tp = kmem_zone_zalloc(xfs_trans_zone,
> > > > -		(flags & XFS_TRANS_NOFS) ? KM_NOFS : KM_SLEEP);
> > > > +		(flags & (XFS_TRANS_NOFS | XFS_TRANS_NO_WRITECOUNT))
> > > > +			? KM_NOFS : KM_SLEEP);
> > > >  	tp->t_magic = XFS_TRANS_HEADER_MAGIC;
> > > >  	tp->t_flags = flags;
> > > >  	tp->t_mountp = mp;
> > >
> > > Brief examination says caller should set XFS_TRANS_NOFS, not change
> > > the implementation to make XFS_TRANS_NO_WRITECOUNT flag to also mean
> > > XFS_TRANS_NOFS.
> >
> > I didn't mean the change above can fix the problem, and I don't really
> > understand your suggestion.
>
> xfs_syncsb() does:
>
> 	tp = xfs_trans_alloc(... , XFS_TRANS_NO_WRITECOUNT, ....);
>
> but it's running in a GFP_NOFS context when a freeze is being
> finalised. SO, rather than changing what XFS_TRANS_NO_WRITECOUNT
> does in xfs_trans_alloc(), we should tell it to do a GFP_NOFS
> allocation. i.e.
>
> 	tp = xfs_trans_alloc(... , XFS_TRANS_NOFS | XFS_TRANS_NO_WRITECOUNT, ....);

Ah. This is clear but I am not sure it is enough,

> > Obviously any GFP_FS allocation in xfs_fs_freeze()
> > paths will trigger the same warning.
>
> Of which there should be none except for that xfs_trans_alloc()
> call.

Really? Again, I can be easily wrong, but when I look at xfs_freeze_fs()
paths I can see

xfs_fs_freeze()->xfs_quiesce_attr()->xfs_log_quiesce()->xfs_log_unmount_write()
->xfs_log_reserve()->xlog_ticket_alloc(KM_SLEEP)

at least. But I can test the change above, perhaps this call chain is
not possible...

> > I added this hack
> >
> > 	--- a/fs/xfs/xfs_super.c
> > 	+++ b/fs/xfs/xfs_super.c
> > 	@@ -1333,10 +1333,15 @@ xfs_fs_freeze(
> > 		struct super_block      *sb)
> > 	 {
> > 		struct xfs_mount        *mp = XFS_M(sb);
> > 	+       int ret;
> >
> > 	+       current->flags |= PF_FSTRANS; // tell kmem_flags_convert() to remove GFP_FS
> > 		xfs_save_resvblks(mp);
> > 		xfs_quiesce_attr(mp);
> > 	-       return xfs_sync_sb(mp, true);
> > 	+       ret = xfs_sync_sb(mp, true);
> > 	+       current->flags &= ~PF_FSTRANS;
> > 	+
> > 	+       return ret;
> > 	 }
>
> /me shudders

don't worry, this debugging change won't escape my testing machine!

> > just for testing purposes and after that I got another warning below. I didn't
> > read it carefully yet, but _at first glance_ it looks like the lock inversion
> > uncovered by 2/2, although I can be easily wrong. cancel_delayed_work_sync(l_work)
> > under sb_internal can hang if xfs_log_worker() waits for this rwsem?`
>
> Actually: I *can't read it*. I've got no fucking clue what lockdep
> is trying to say here. This /looks/ like a lockdep is getting
> confused

I can almost never understand what lockdep tells me, it is too clever for me.
But this time I think it is right.

Suppose that freeze_super() races with xfs_log_worker() callback.

freeze_super() takes sb_internal lock and xfs_log_quiesce() calls
cancel_delayed_work_sync(l_work). This will sleep until xfs_log_worker()
finishes.

xfs_log_worker() does a __GFP_FS alloc, triggers reclaim, and blocks
on the same sb_internal lock. Say, in xfs_do_writepage()->xfs_trans_alloc()
path.

Deadlock. The worker thread can't take sb_internal hold by freeze_super(),
cancel_delayed_work_sync() will sleep forever because xfs_log_worker()
can't finish.

So xfs_log_worker() should run in a GFP_NOFS context too. And perhaps
the change above in xfs_trans_alloc() or in xfs_sync_sb() can help if
it doesn't do other allocatiions, I dunno.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1498055 — Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super() and thaw_super() paths

On Sun, Oct 09, 2016 at 06:14:57PM +0200, Oleg Nesterov wrote:
> On 10/08, Dave Chinner wrote:
> >
> > On Fri, Oct 07, 2016 at 07:15:18PM +0200, Oleg Nesterov wrote:
> > > > >
> > > > > --- x/fs/xfs/xfs_trans.c
> > > > > +++ x/fs/xfs/xfs_trans.c
> > > > > @@ -245,7 +245,8 @@ xfs_trans_alloc(
> > > > >  	atomic_inc(&mp->m_active_trans);
> > > > >
> > > > >  	tp = kmem_zone_zalloc(xfs_trans_zone,
> > > > > -		(flags & XFS_TRANS_NOFS) ? KM_NOFS : KM_SLEEP);
> > > > > +		(flags & (XFS_TRANS_NOFS | XFS_TRANS_NO_WRITECOUNT))
> > > > > +			? KM_NOFS : KM_SLEEP);
> > > > >  	tp->t_magic = XFS_TRANS_HEADER_MAGIC;
> > > > >  	tp->t_flags = flags;
> > > > >  	tp->t_mountp = mp;
> > > >
> > > > Brief examination says caller should set XFS_TRANS_NOFS, not change
> > > > the implementation to make XFS_TRANS_NO_WRITECOUNT flag to also mean
> > > > XFS_TRANS_NOFS.
> > >
> > > I didn't mean the change above can fix the problem, and I don't really
> > > understand your suggestion.
> >
> > xfs_syncsb() does:
> >
> > 	tp = xfs_trans_alloc(... , XFS_TRANS_NO_WRITECOUNT, ....);
> >
> > but it's running in a GFP_NOFS context when a freeze is being
> > finalised. SO, rather than changing what XFS_TRANS_NO_WRITECOUNT
> > does in xfs_trans_alloc(), we should tell it to do a GFP_NOFS
> > allocation. i.e.
> >
> > 	tp = xfs_trans_alloc(... , XFS_TRANS_NOFS | XFS_TRANS_NO_WRITECOUNT, ....);
> 
> Ah. This is clear but I am not sure it is enough,
> 
> > > Obviously any GFP_FS allocation in xfs_fs_freeze()
> > > paths will trigger the same warning.
> >
> > Of which there should be none except for that xfs_trans_alloc()
> > call.
> 
> Really?

/Should/ is an indication of *intent*. Reality is that there may be
*bugs*. We all know that testing can't prove the absence of bugs, so
even after years and years of exercising the code with producing any
evidence there may still be problems.

So, it's time to waste more time explaining why lockdep is telling
us about something that *isn't a bug*.

> Again, I can be easily wrong, but when I look at xfs_freeze_fs()
> paths I can see
> 
> xfs_fs_freeze()->xfs_quiesce_attr()->xfs_log_quiesce()->xfs_log_unmount_write()
> ->xfs_log_reserve()->xlog_ticket_alloc(KM_SLEEP)

So, the problem being indicated here is that memory reclaim might
either try to a) write back dirty pages (which require allocation
transactions), b) might run a shrinker that requires running a
transaction or c) we might run periodic background inode reclaim.

For the case of a), this /can't happen/ because we've already run
the part of a freeze that stops pages being dirtied and then written
them all back and made them clean. So we won't run transactions from
page cache reclaim and so we can't deadlock.

For the case of b), well, that's even easier - the only shrinker
path we care about here is inode reclaim through super_cache_scan().
Before that shrinker runs anything it calls:

	if (!trylock_super(sb))
		return SHRINK_STOP;

Now, we're running memory allocation for the freeze context, which
means we are holding the sb->s_umount semaphore in write mode. That
means the shrinker is going to /fail to lock the superblock/ and
therefore not run any reclaim on that superblock.

IOWs, while we hold the s_umount lock in write mode across a memory
allocation, the shrinkers run in GFP_NOFS mode automatically. So we
can't run transactions from memory reclaim and so we will can't
deadlock.

For the case of c), xfs_quiesce_attr() does this:

	/* force the log to unpin objects from the now complete transactions */
	xfs_log_force(mp, XFS_LOG_SYNC);

	/* reclaim inodes to do any IO before the freeze completes */
	xfs_reclaim_inodes(mp, 0);
	xfs_reclaim_inodes(mp, SYNC_WAIT);

We basically unpin, clean, and reclaim all the unused inodes the XFS
inode cache. With the shrinker not reclaiming any inodes, and there
being no cached, dirty, unreclaimed inodes remaining in the cache,
there can be no background memory allocations, transactions or IO
triggered memory reclaim in this filesystem. At this point, memory
reclaim /should never block/ trying to reclaim objects from this
filesystem that require transactions to free.

From this, it should be obvious that we don't even need to
change the code in xfs_syncsb() - in the freeze context that the
allocation is run we've got a clean filesystem where memory reclaim
won't block on the filesystem being frozen, so the code is safe as
it stands.

> > > just for testing purposes and after that I got another warning below. I didn't
> > > read it carefully yet, but _at first glance_ it looks like the lock inversion
> > > uncovered by 2/2, although I can be easily wrong. cancel_delayed_work_sync(l_work)
> > > under sb_internal can hang if xfs_log_worker() waits for this rwsem?`
> >
> > Actually: I *can't read it*. I've got no fucking clue what lockdep
> > is trying to say here. This /looks/ like a lockdep is getting
> > confused
> 
> I can almost never understand what lockdep tells me, it is too clever for me.
> But this time I think it is right.
> 
> Suppose that freeze_super() races with xfs_log_worker() callback.
> 
> freeze_super() takes sb_internal lock and xfs_log_quiesce() calls
> cancel_delayed_work_sync(l_work). This will sleep until xfs_log_worker()
> finishes.
> 
> xfs_log_worker() does a __GFP_FS alloc, triggers reclaim, and blocks
> on the same sb_internal lock. Say, in xfs_do_writepage()->xfs_trans_alloc()
> path.

See above - xfs_log_worker will not block on memory reclaim on the
filesystem because a) there are no dirty pages and b) the superblock
shrinker will not get the sb->s_umount lock and hence operates for
all contexts as though they are doing GFP_NOFS allocations.

Basically, what we are seeing here is yet another case of "lockdep
is just smart enough to be really dumb" because we cannot fully
express or cleanly annotate the contexts in which it is being asked
to validate. Unless we do something we shouldn't be doing (i.e.
marking GFP_KERNEL allocations that are safe with GFP_NOFS to shut
up lockdep), all we've just done is introduce another vector for
lockdep false positives...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1496664 — Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super() and thaw_super() paths

On Tue, Oct 04, 2016 at 01:48:00PM +0200, Michal Hocko wrote:
> Johannes is already looking into this
> http://lkml.kernel.org/r/20161004093216.GA21170@cmpxchg.org
> 
> On Tue 04-10-16 13:43:43, Oleg Nesterov wrote:
> > because of kernel bug:
> > 
> > 	[ 2730.242537] run fstests generic/274 at 2016-10-04 05:17:34
> > 	[ 2730.738352] XFS (loop1): Mounting V5 Filesystem
> > 	[ 2730.741451] XFS (loop1): Ending clean mount
> > 	[ 2744.508698] ------------[ cut here ]------------
> > 	[ 2744.509190] kernel BUG at ./include/linux/swap.h:276!
> > 
> > static inline void workingset_node_shadows_dec(struct radix_tree_node *node)
> > {
> > 	VM_BUG_ON(!workingset_node_shadows(node));
> > 	node->count -= 1U << RADIX_TREE_COUNT_SHIFT;

We tracked this one down and Linus merged a fix for this issue:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d3798ae8c6f3767c726403c2ca6ecc317752c9dd

Let me know if this still fires on kernels with that commit.
Thanks!

[toc] | [prev] | [next] | [standalone]

#1497361 — Re: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super() and thaw_super() paths

On 10/06, Johannes Weiner wrote:
>
> On Tue, Oct 04, 2016 at 01:48:00PM +0200, Michal Hocko wrote:
> > Johannes is already looking into this
> > http://lkml.kernel.org/r/20161004093216.GA21170@cmpxchg.org
> >
> > On Tue 04-10-16 13:43:43, Oleg Nesterov wrote:
> > > because of kernel bug:
> > >
> > > 	[ 2730.242537] run fstests generic/274 at 2016-10-04 05:17:34
> > > 	[ 2730.738352] XFS (loop1): Mounting V5 Filesystem
> > > 	[ 2730.741451] XFS (loop1): Ending clean mount
> > > 	[ 2744.508698] ------------[ cut here ]------------
> > > 	[ 2744.509190] kernel BUG at ./include/linux/swap.h:276!
> > >
> > > static inline void workingset_node_shadows_dec(struct radix_tree_node *node)
> > > {
> > > 	VM_BUG_ON(!workingset_node_shadows(node));
> > > 	node->count -= 1U << RADIX_TREE_COUNT_SHIFT;
>
> We tracked this one down and Linus merged a fix for this issue:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d3798ae8c6f3767c726403c2ca6ecc317752c9dd

Confirm, generic/274 no longer crashes the kernel.

Thanks.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1491394 — [PATCH 1/2] fs/super.c: fix race between freeze_super() and thaw_super()

Change thaw_super() to check frozen != SB_FREEZE_COMPLETE rather than
frozen == SB_UNFROZEN, otherwise it can race with freeze_super() which
drops sb->s_umount after SB_FREEZE_WRITE to preserve the lock ordering.

In this case thaw_super() will wrongly call s_op->unfreeze_fs() before
it was actually frozen, and call sb_freeze_unlock() which leads to the
unbalanced percpu_up_write(). Unfortunately lockdep can't detect this,
so this triggers misc BUG_ON()'s in kernel/rcu/sync.c.

Reported-and-tested-by: Nikolay Borisov <kernel@kyup.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: stable@vger.kernel.org
---
 fs/super.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index d78b984..2549896c 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1324,8 +1324,8 @@ int freeze_super(struct super_block *sb)
 		}
 	}
 	/*
-	 * This is just for debugging purposes so that fs can warn if it
-	 * sees write activity when frozen is set to SB_FREEZE_COMPLETE.
+	 * For debugging purposes so that fs can warn if it sees write activity
+	 * when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
 	 */
 	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
 	up_write(&sb->s_umount);
@@ -1344,7 +1344,7 @@ int thaw_super(struct super_block *sb)
 	int error;
 
 	down_write(&sb->s_umount);
-	if (sb->s_writers.frozen == SB_UNFROZEN) {
+	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
 		up_write(&sb->s_umount);
 		return -EINVAL;
 	}
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1491408 — Re: [PATCH 1/2] fs/super.c: fix race between freeze_super() and thaw_super()

On Mon 26-09-16 18:07:48, Oleg Nesterov wrote:
> Change thaw_super() to check frozen != SB_FREEZE_COMPLETE rather than
> frozen == SB_UNFROZEN, otherwise it can race with freeze_super() which
> drops sb->s_umount after SB_FREEZE_WRITE to preserve the lock ordering.
> 
> In this case thaw_super() will wrongly call s_op->unfreeze_fs() before
> it was actually frozen, and call sb_freeze_unlock() which leads to the
> unbalanced percpu_up_write(). Unfortunately lockdep can't detect this,
> so this triggers misc BUG_ON()'s in kernel/rcu/sync.c.
> 
> Reported-and-tested-by: Nikolay Borisov <kernel@kyup.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Cc: stable@vger.kernel.org

The patch looks good. Thanks!

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  fs/super.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index d78b984..2549896c 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1324,8 +1324,8 @@ int freeze_super(struct super_block *sb)
>  		}
>  	}
>  	/*
> -	 * This is just for debugging purposes so that fs can warn if it
> -	 * sees write activity when frozen is set to SB_FREEZE_COMPLETE.
> +	 * For debugging purposes so that fs can warn if it sees write activity
> +	 * when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
>  	 */
>  	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
>  	up_write(&sb->s_umount);
> @@ -1344,7 +1344,7 @@ int thaw_super(struct super_block *sb)
>  	int error;
>  
>  	down_write(&sb->s_umount);
> -	if (sb->s_writers.frozen == SB_UNFROZEN) {
> +	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
>  		up_write(&sb->s_umount);
>  		return -EINVAL;
>  	}
> -- 
> 2.5.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web