All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
To: Jan Kara <jack@suse.cz>
Cc: Thilo Fromm <t-lo@linux.microsoft.com>,
	Ye Bin <yebin10@huawei.com>,
	jack@suse.com, tytso@mit.edu, linux-ext4@vger.kernel.org,
	regressions@lists.linux.dev
Subject: Re: [syzbot] possible deadlock in jbd2_journal_lock_updates
Date: Fri, 11 Nov 2022 07:52:38 -0800	[thread overview]
Message-ID: <20221111155238.GA32201@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> (raw)
In-Reply-To: <20221111151029.GA27244@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>

On Fri, Nov 11, 2022 at 07:10:29AM -0800, Jeremi Piotrowski wrote:
> On Fri, Nov 11, 2022 at 03:24:24PM +0100, Jan Kara wrote:
> > On Thu 10-11-22 11:27:01, Jeremi Piotrowski wrote:
> > > On Thu, Nov 10, 2022 at 04:26:37PM +0100, Jan Kara wrote:
> > > > On Thu 10-11-22 04:57:58, Jeremi Piotrowski wrote:
> > > > > On Wed, Oct 26, 2022 at 12:18:54PM +0200, Jan Kara wrote:
> > > > > > On Mon 24-10-22 18:32:51, Thilo Fromm wrote:
> > > > > > > Hello Honza,
> > > > > > > 
> > > > > > > > Yeah, I was pondering about this for some time but still I have no clue who
> > > > > > > > could be holding the buffer lock (which blocks the task holding the
> > > > > > > > transaction open) or how this could related to the commit you have
> > > > > > > > identified. I have two things to try:
> > > > > > > > 
> > > > > > > > 1) Can you please check whether the deadlock reproduces also with 6.0
> > > > > > > > kernel? The thing is that xattr handling code in ext4 has there some
> > > > > > > > additional changes, commit 307af6c8793 ("mbcache: automatically delete
> > > > > > > > entries from cache on freeing") in particular.
> > > > > > > 
> > > > > > > This would be complex; we currently do not integrate 6.0 with Flatcar and
> > > > > > > would need to spend quite some effort ingesting it first (mostly, make sure
> > > > > > > the new kernel does not break something unrelated). Flatcar is an
> > > > > > > image-based distro, so kernel updates imply full distro updates.
> > > > > > 
> > > > > > OK, understood.
> > > > > > 
> > > > > > > > 2) I have created a debug patch (against 5.15.x stable kernel). Can you
> > > > > > > > please reproduce the failure with it and post the output of "echo w
> > > > > > > > > /proc/sysrq-trigger" and also the output the debug patch will put into the
> > > > > > > > kernel log? It will dump the information about buffer lock owner if we > cannot get the lock for more than 32 seconds.
> > > > > > > 
> > > > > > > This would be more straightforward - I can reach out to one of our users
> > > > > > > suffering from the issue; they can reliably reproduce it and don't shy away
> > > > > > > from patching their kernel. Where can I find the patch?
> > > > > > 
> > > > > > Ha, my bad. I forgot to attach it. Here it is.
> > > > > > 
> > > > > 
> > > > > Unfortunately this patch produced no output, but I have been able to repro so I
> > > > > understand why: except for the hung tasks, we have 1+ tasks busy-looping through
> > > > > the following code in ext4_xattr_block_set():
> > > > > 
> > > > > inserted:
> > > > >         if (!IS_LAST_ENTRY(s->first)) {
> > > > >                 new_bh = ext4_xattr_block_cache_find(inode, header(s->base),
> > > > >                                                      &ce);
> > > > >                 if (new_bh) {
> > > > >                         /* We found an identical block in the cache. */
> > > > >                         if (new_bh == bs->bh)
> > > > >                                 ea_bdebug(new_bh, "keeping");
> > > > >                         else {
> > > > >                                 u32 ref;
> > > > > 
> > > > >                                 WARN_ON_ONCE(dquot_initialize_needed(inode));
> > > > > 
> > > > >                                 /* The old block is released after updating
> > > > >                                    the inode. */
> > > > >                                 error = dquot_alloc_block(inode,
> > > > >                                                 EXT4_C2B(EXT4_SB(sb), 1));
> > > > >                                 if (error)
> > > > >                                         goto cleanup;
> > > > >                                 BUFFER_TRACE(new_bh, "get_write_access");
> > > > >                                 error = ext4_journal_get_write_access(
> > > > >                                                 handle, sb, new_bh,
> > > > >                                                 EXT4_JTR_NONE);
> > > > >                                 if (error)
> > > > >                                         goto cleanup_dquot;
> > > > >                                 lock_buffer(new_bh);
> > > > >                                 /*
> > > > >                                  * We have to be careful about races with
> > > > >                                  * adding references to xattr block. Once we
> > > > >                                  * hold buffer lock xattr block's state is
> > > > >                                  * stable so we can check the additional
> > > > >                                  * reference fits.
> > > > >                                  */
> > > > >                                 ref = le32_to_cpu(BHDR(new_bh)->h_refcount) + 1;
> > > > >                                 if (ref > EXT4_XATTR_REFCOUNT_MAX) {
> > > > >                                         /*
> > > > >                                          * Undo everything and check mbcache
> > > > >                                          * again.
> > > > >                                          */
> > > > >                                         unlock_buffer(new_bh);
> > > > >                                         dquot_free_block(inode,
> > > > >                                                          EXT4_C2B(EXT4_SB(sb),
> > > > >                                                                   1));
> > > > >                                         brelse(new_bh);
> > > > >                                         mb_cache_entry_put(ea_block_cache, ce);
> > > > >                                         ce = NULL;
> > > > >                                         new_bh = NULL;
> > > > >                                         goto inserted;
> > > > >                                 }
> > > > > 
> > > > > The tasks keep taking the 'goto inserted' branch, and never finish. I've been
> > > > > able to repro with kernel v6.0.7 as well.
> > > > 
> > > > Interesting! That makes is much clearer (and also makes my debug patch
> > > > unnecessary). So clearly the e_reusable variable in the mb_cache_entry got
> > > > out of sync with the number of references really in the xattr block - in
> > > > particular the block likely has h_refcount >= EXT4_XATTR_REFCOUNT_MAX but
> > > > e_reusable is set to true. Now I can see how e_reusable can stay at false due
> > > > to a race when refcount is actually smaller but I don't see how it could
> > > > stay at true when refcount is big enough - that part seems to be locked
> > > > properly. If you can reproduce reasonably easily, can you try reproducing
> > > > with attached patch? Thanks!
> > > > 
> > > 
> > > Sure, with that patch I'm getting the following output, reusable is false on
> > > most items until we hit something with reusable true and then that loops
> > > indefinitely:
> > 
> > Thanks. So that is what I've suspected. I'm still not 100% clear on how
> > this inconsistency can happen although I have a suspicion - does attached
> > patch fix the problem for you?
> > 
> > Also is it possible to share the reproducer or it needs some special
> > infrastructure?
> > 
> > 								Honza
> 
> I'll test the patch and report back.
> 
> Attached you'll find the reproducer, for me it reproduces within a few minutes.
> It brings up a k8s node and then runs 3 instances of the application which
> creates a lot of small files in a loop. The OS we run it on has selinux enabled
> in permissive mode, that might play a role.
> 

I can still reproduce it with the patch.

> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
> 
> > >From 6132433e400ff7be348fe04fdf8ee67eb105ec21 Mon Sep 17 00:00:00 2001
> > From: Jan Kara <jack@suse.cz>
> > Date: Thu, 10 Nov 2022 16:22:06 +0100
> > Subject: [PATCH] ext4: Lock xattr buffer before inserting cache entry
> > 
> > ---
> >  fs/ext4/xattr.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> > index 36d6ba7190b6..02e265bb94e2 100644
> > --- a/fs/ext4/xattr.c
> > +++ b/fs/ext4/xattr.c
> > @@ -2970,15 +2970,18 @@ ext4_xattr_block_cache_insert(struct mb_cache *ea_block_cache,
> >  			      struct buffer_head *bh)
> >  {
> >  	struct ext4_xattr_header *header = BHDR(bh);
> > -	__u32 hash = le32_to_cpu(header->h_hash);
> > -	int reusable = le32_to_cpu(header->h_refcount) <
> > -		       EXT4_XATTR_REFCOUNT_MAX;
> > +	__u32 hash;
> > +	int reusable;
> >  	int error;
> >  
> >  	if (!ea_block_cache)
> >  		return;
> > +	lock_buffer(bh);
> > +	hash = le32_to_cpu(header->h_hash);
> > +	reusable = le32_to_cpu(header->h_refcount) < EXT4_XATTR_REFCOUNT_MAX;
> >  	error = mb_cache_entry_create(ea_block_cache, GFP_NOFS, hash,
> >  				      bh->b_blocknr, reusable);
> > +	unlock_buffer(bh);
> >  	if (error) {
> >  		if (error == -EBUSY)
> >  			ea_bdebug(bh, "already in cache");
> > -- 
> > 2.35.3
> > 
> 



  reply	other threads:[~2022-11-11 15:52 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-08  7:34 [syzbot] possible deadlock in jbd2_journal_lock_updates syzbot
2022-08-08 16:38 ` syzbot
2022-08-24 10:06   ` Jan Kara
2022-09-28  7:30     ` Thilo Fromm
2022-09-29  8:27       ` Jan Kara
2022-09-29 13:18         ` Thilo Fromm
2022-10-04  6:38           ` Jeremi Piotrowski
2022-10-04  9:10             ` Jan Kara
2022-10-04 14:21               ` Thilo Fromm
2022-10-05 15:10                 ` Jan Kara
2022-10-10 14:24                   ` Jeremi Piotrowski
2022-10-14  6:42                     ` Thilo Fromm
2022-10-14 13:25                       ` Jan Kara
2022-10-21 10:23                         ` Thilo Fromm
2022-10-24 10:46                           ` Jan Kara
2022-10-24 16:32                             ` Thilo Fromm
2022-10-26 10:18                               ` Jan Kara
2022-11-10 12:57                                 ` Jeremi Piotrowski
2022-11-10 15:26                                   ` Jan Kara
2022-11-10 19:27                                     ` Jeremi Piotrowski
2022-11-11 14:24                                       ` Jan Kara
2022-11-11 15:10                                         ` Jeremi Piotrowski
2022-11-11 15:52                                           ` Jeremi Piotrowski [this message]
2022-11-21 13:35                                             ` Jan Kara
2022-11-21 15:00                                               ` Jan Kara
2022-11-21 15:18                                                 ` Thorsten Leemhuis
2022-11-21 15:40                                                   ` Jan Kara
2022-11-21 18:15                                                 ` Jeremi Piotrowski
2022-11-22 11:57                                                   ` Jan Kara
2022-11-22 17:48                                                     ` Jeremi Piotrowski
2022-11-23 19:41                                                       ` Jan Kara
2022-09-30 12:16       ` [syzbot] possible deadlock in jbd2_journal_lock_updates #forregzbot Thorsten Leemhuis
2022-11-23  9:56         ` Thorsten Leemhuis
2023-04-30 23:38 ` [syzbot] possible deadlock in jbd2_journal_lock_updates Theodore Ts'o
     [not found] <20220819122008.1561-1-hdanton@sina.com>
2022-08-19 16:00 ` syzbot
     [not found] <20220821023626.1810-1-hdanton@sina.com>
2022-08-21 10:34 ` syzbot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221111155238.GA32201@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net \
    --to=jpiotrowski@linux.microsoft.com \
    --cc=jack@suse.com \
    --cc=jack@suse.cz \
    --cc=linux-ext4@vger.kernel.org \
    --cc=regressions@lists.linux.dev \
    --cc=t-lo@linux.microsoft.com \
    --cc=tytso@mit.edu \
    --cc=yebin10@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.