[bug report] bcachefs: sleeping in atomic

[bug report] bcachefs: sleeping in atomic

[Dan Carpenter]

Hello Kent Overstreet,

The patch 93a640e2570b: "bcachefs: GFP_NOIO -> GFP_NOFS" from May 28,
2023 (linux-next), leads to the following Smatch static checker
warning:

fs/bcachefs/journal_io.c:1461 journal_buf_realloc() warn: sleeping in atomic context
fs/bcachefs/journal_io.c:1662 bch2_journal_entries_postprocess() warn: sleeping in atomic context
fs/bcachefs/journal_io.c:1764 bch2_journal_write() warn: sleeping in atomic context
fs/bcachefs/journal_io.c:788 jset_validate() warn: sleeping in atomic context
fs/bcachefs/replicas.c:439 bch2_mark_replicas() warn: sleeping in atomic context

All this sleeping in atomic warnings start in journal_write_done()
where it takes spin_lock(&j->lock) so preempt is disabled.

The call tree for the first warning is:

journal_write_done() <- disables preempt
-> bch2_journal_write()
   -> journal_buf_realloc()

fs/bcachefs/journal_io.c
    1452 static void journal_buf_realloc(struct journal *j, struct journal_buf *buf)
    1453 {
    1454         /* we aren't holding j->lock: */
    1455         unsigned new_size = READ_ONCE(j->buf_size_want);
    1456         void *new_buf;
    1457 
    1458         if (buf->buf_size >= new_size)
    1459                 return;
    1460 
--> 1461         new_buf = kvpmalloc(new_size, GFP_NOFS|__GFP_NOWARN);

This sleeps.  GFP_NOWAIT and GFP_ATOMIC don't sleep.  But kvpmalloc() is
always going to have to be treated as a sleeping function because it
allocates giant amounts of memory.

    1462         if (!new_buf)
    1463                 return;
    1464 
    1465         memcpy(new_buf, buf->data, buf->buf_size);
    1466 
    1467         spin_lock(&j->lock);
    1468         swap(buf->data,                new_buf);
    1469         swap(buf->buf_size,        new_size);
    1470         spin_unlock(&j->lock);
    1471 
    1472         kvpfree(new_buf, new_size);
    1473 }

regards,
dan carpenter

Re: [bug report] bcachefs: sleeping in atomic

[Kent Overstreet]

On Fri, Sep 15, 2023 at 03:57:34PM +0300, Dan Carpenter wrote:
> Hello Kent Overstreet,
> 
> The patch 93a640e2570b: "bcachefs: GFP_NOIO -> GFP_NOFS" from May 28,
> 2023 (linux-next), leads to the following Smatch static checker
> warning:

So the smatch warnings are pretty hard for me to deal with - it appears
you're testing each individual commit, which is what _I_ normally like
to do, except in this case there's no way I'm applying all the fixes to
individual commits in a 3k commit branch.

And, dealing with the back-and-forth of going through emails is also a
bit much, I'm going to need to get smatch integrated into my CI.

But...

smatch doesn't build on debian.

Can we get that fixed?

Re: [bug report] bcachefs: sleeping in atomic

[Kent Overstreet]

On Fri, Sep 15, 2023 at 03:57:34PM +0300, Dan Carpenter wrote:
> Hello Kent Overstreet,
> 
> The patch 93a640e2570b: "bcachefs: GFP_NOIO -> GFP_NOFS" from May 28,
> 2023 (linux-next), leads to the following Smatch static checker
> warning:
> 
> fs/bcachefs/journal_io.c:1461 journal_buf_realloc() warn: sleeping in atomic context
> fs/bcachefs/journal_io.c:1662 bch2_journal_entries_postprocess() warn: sleeping in atomic context
> fs/bcachefs/journal_io.c:1764 bch2_journal_write() warn: sleeping in atomic context
> fs/bcachefs/journal_io.c:788 jset_validate() warn: sleeping in atomic context
> fs/bcachefs/replicas.c:439 bch2_mark_replicas() warn: sleeping in atomic context
> 
> All this sleeping in atomic warnings start in journal_write_done()
> where it takes spin_lock(&j->lock) so preempt is disabled.


From 0528bebd8b42ce43a39da8e911bf21825f5085dd Mon Sep 17 00:00:00 2001
From: Kent Overstreet <kent.overstreet@linux.dev>
Date: Tue, 19 Sep 2023 22:36:30 -0400
Subject: [PATCH] bcachefs: drop journal lock before calling journal_write

bch2_journal_write() expects process context, it takes journal_lock as
needed.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>

diff --git a/fs/bcachefs/journal_io.c b/fs/bcachefs/journal_io.c
index 269c8e8a1d95..6a3d6a374e9c 100644
--- a/fs/bcachefs/journal_io.c
+++ b/fs/bcachefs/journal_io.c
@@ -1554,6 +1554,7 @@ static void journal_write_done(struct closure *cl)
 
 	if (!journal_state_count(new, new.unwritten_idx) &&
 	    journal_last_unwritten_seq(j) <= journal_cur_seq(j)) {
+		spin_unlock(&j->lock);
 		closure_call(&j->io, bch2_journal_write, c->io_complete_wq, NULL);
 	} else if (journal_last_unwritten_seq(j) == journal_cur_seq(j) &&
 		   new.cur_entry_offset < JOURNAL_ENTRY_CLOSED_VAL) {
@@ -1566,10 +1567,11 @@ static void journal_write_done(struct closure *cl)
 		 * might want to be written now:
 		 */
 
+		spin_unlock(&j->lock);
 		mod_delayed_work(c->io_complete_wq, &j->write_work, max(0L, delta));
+	} else {
+		spin_unlock(&j->lock);
 	}
-
-	spin_unlock(&j->lock);
 }
 
 static void journal_write_endio(struct bio *bio)

Re: [bug report] bcachefs: sleeping in atomic

[Dan Carpenter]

On Tue, Sep 19, 2023 at 04:56:59PM -0400, Kent Overstreet wrote:
> On Fri, Sep 15, 2023 at 03:57:34PM +0300, Dan Carpenter wrote:
> > Hello Kent Overstreet,
> > 
> > The patch 93a640e2570b: "bcachefs: GFP_NOIO -> GFP_NOFS" from May 28,
> > 2023 (linux-next), leads to the following Smatch static checker
> > warning:
> 
> So the smatch warnings are pretty hard for me to deal with - it appears
> you're testing each individual commit, which is what _I_ normally like
> to do, except in this case there's no way I'm applying all the fixes to
> individual commits in a 3k commit branch.
> 
> And, dealing with the back-and-forth of going through emails is also a
> bit much, I'm going to need to get smatch integrated into my CI.
> 
> But...
> 
> smatch doesn't build on debian.
> 
> Can we get that fixed?

It should.  I'm using it on Debian.  This should be a complete list of
the required packages.  Let me know if that's not complete.

apt-get install gcc make sqlite3 libsqlite3-dev libdbd-sqlite3-perl libssl-dev libtry-tiny-perl

Perhaps send a copy and paste of the build error?

regards,
dan carpenter

Re: [bug report] bcachefs: sleeping in atomic

[Dan Carpenter]

On Tue, Sep 19, 2023 at 10:37:31PM -0400, Kent Overstreet wrote:
> On Fri, Sep 15, 2023 at 03:57:34PM +0300, Dan Carpenter wrote:
> > Hello Kent Overstreet,
> > 
> > The patch 93a640e2570b: "bcachefs: GFP_NOIO -> GFP_NOFS" from May 28,
> > 2023 (linux-next), leads to the following Smatch static checker
> > warning:
> > 
> > fs/bcachefs/journal_io.c:1461 journal_buf_realloc() warn: sleeping in atomic context
> > fs/bcachefs/journal_io.c:1662 bch2_journal_entries_postprocess() warn: sleeping in atomic context
> > fs/bcachefs/journal_io.c:1764 bch2_journal_write() warn: sleeping in atomic context
> > fs/bcachefs/journal_io.c:788 jset_validate() warn: sleeping in atomic context
> > fs/bcachefs/replicas.c:439 bch2_mark_replicas() warn: sleeping in atomic context
> > 
> > All this sleeping in atomic warnings start in journal_write_done()
> > where it takes spin_lock(&j->lock) so preempt is disabled.
> 
> 
> >From 0528bebd8b42ce43a39da8e911bf21825f5085dd Mon Sep 17 00:00:00 2001
> From: Kent Overstreet <kent.overstreet@linux.dev>
> Date: Tue, 19 Sep 2023 22:36:30 -0400
> Subject: [PATCH] bcachefs: drop journal lock before calling journal_write
> 
> bch2_journal_write() expects process context, it takes journal_lock as
> needed.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>

Thanks!

regards,
dan carpenter

Re: [bug report] bcachefs: sleeping in atomic

[Kent Overstreet]

On Tue, Sep 26, 2023 at 05:41:37PM +0300, Dan Carpenter wrote:
> On Tue, Sep 19, 2023 at 04:56:59PM -0400, Kent Overstreet wrote:
> > On Fri, Sep 15, 2023 at 03:57:34PM +0300, Dan Carpenter wrote:
> > > Hello Kent Overstreet,
> > > 
> > > The patch 93a640e2570b: "bcachefs: GFP_NOIO -> GFP_NOFS" from May 28,
> > > 2023 (linux-next), leads to the following Smatch static checker
> > > warning:
> > 
> > So the smatch warnings are pretty hard for me to deal with - it appears
> > you're testing each individual commit, which is what _I_ normally like
> > to do, except in this case there's no way I'm applying all the fixes to
> > individual commits in a 3k commit branch.
> > 
> > And, dealing with the back-and-forth of going through emails is also a
> > bit much, I'm going to need to get smatch integrated into my CI.
> > 
> > But...
> > 
> > smatch doesn't build on debian.
> > 
> > Can we get that fixed?
> 
> It should.  I'm using it on Debian.  This should be a complete list of
> the required packages.  Let me know if that's not complete.
> 
> apt-get install gcc make sqlite3 libsqlite3-dev libdbd-sqlite3-perl libssl-dev libtry-tiny-perl

What of the llvm dependency? I could only get it to build when I
disabled that, is that dependency important?

It also consistently segfaults on the bcachefs code.

Re: [bug report] bcachefs: sleeping in atomic

[Dan Carpenter]

On Tue, Sep 26, 2023 at 12:13:33PM -0400, Kent Overstreet wrote:
> On Tue, Sep 26, 2023 at 05:41:37PM +0300, Dan Carpenter wrote:
> > On Tue, Sep 19, 2023 at 04:56:59PM -0400, Kent Overstreet wrote:
> > > On Fri, Sep 15, 2023 at 03:57:34PM +0300, Dan Carpenter wrote:
> > > > Hello Kent Overstreet,
> > > > 
> > > > The patch 93a640e2570b: "bcachefs: GFP_NOIO -> GFP_NOFS" from May 28,
> > > > 2023 (linux-next), leads to the following Smatch static checker
> > > > warning:
> > > 
> > > So the smatch warnings are pretty hard for me to deal with - it appears
> > > you're testing each individual commit, which is what _I_ normally like
> > > to do, except in this case there's no way I'm applying all the fixes to
> > > individual commits in a 3k commit branch.
> > > 
> > > And, dealing with the back-and-forth of going through emails is also a
> > > bit much, I'm going to need to get smatch integrated into my CI.
> > > 
> > > But...
> > > 
> > > smatch doesn't build on debian.
> > > 
> > > Can we get that fixed?
> > 
> > It should.  I'm using it on Debian.  This should be a complete list of
> > the required packages.  Let me know if that's not complete.
> > 
> > apt-get install gcc make sqlite3 libsqlite3-dev libdbd-sqlite3-perl libssl-dev libtry-tiny-perl
> 
> What of the llvm dependency? I could only get it to build when I
> disabled that, is that dependency important?

No, that doesn't matter.

> 
> It also consistently segfaults on the bcachefs code.

There were some problems parsing bcachefs initially but I pushed the
fixes for that two weeks ago.  Can you give me the filename and your
.config?  Or even better would be to run
smatch_scripts/kchecker --valgrind path/to/file.c and send the stack
trace.

regards,
dan carpenter