Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Theodore Ts'o]

On Sun, Sep 03, 2023 at 08:00:01PM +0800, Zorro Lang wrote:
> Hi ext4 folks,
> 
> Recently I found lots of fstests cases which belong to "recoveryloop" (e.g.
> g/388 [1], g/455 [2], g/475 [3] and g/482 [4]) or does fs shutdown/resize test
> (e.g. ext4/059 [5], g/530 [6]) failed ext4 with 1k blocksize, the kernel is
> linux v6.6-rc0+ (HEAD=b84acc11b1c9).
> 
> I tested with MKFS_OPTIONS="-b 1024", no specific MOUNT_OPTIONS. I hit these
> failure several times, and I didn't hit them on my last regression test on
> v6.5-rc7+. So I think this might be a regression problem. And I didn't hit
> this failures on xfs. If this's a known issue will be fixed soon, feel free
> to tell me.

TL;DR: there definitely seenms to be something going on with g/455 and
g/482 with the ext4/1k blocksize case in Linus's latest upstream tree,
although it wasn't there in the ext4 branch which I sent to Linus to
pull.


Unfortunately, generic/475 is a known failure, especially in the 1k
block size case.  The rate seems to change a bit over time.  For
example from 6.2:

ext4/1k: 522 tests, 2 failures, 45 skipped, 6153 seconds
  Flaky: generic/051: 40% (2/5)   generic/475: 60% (3/5)

and from 6.1.0-rc4:

ext4/1k: 522 tests, 2 failures, 45 skipped, 5660 seconds
  Flaky: generic/051: 60% (3/5)   generic/475: 40% (2/5)

In 6.5-rc3, it looks like the rate has gotten worse:

ext4/1k: 30 tests, 29 failures, 2402 seconds
  Flaky: generic/475: 97% (29/30)

Alas, finding a root cause for generic/475 has been challenging.  I
suspect that it happens when we crash while doing a large truncate on
a highly fragmented file system, such as that the truncate has to span
multiple truncates, with the inode on the orphan list so the kernel
can complete the truncate if we trash mid-truncate when we clean up
the orphan list.  However, that's just a theory, and I don't yet have
hard evidence.


The generic/388 test is very different.  It uses the shutdown ioctl,
and that's something that ext4 has never completely handled correctly.
Doing it right would require adding some locks in hot paths, so it's
one which I've suppressed for all of my ext4 tests[1].

[1] https://github.com/tytso/xfstests-bld/blob/master/test-appliance/files/root/fs/ext4/exclude


The generic/455 and generic/482 tests work by using dm-log-writes, and
that was *not* failing on the branch (v6.5.0-rc3-60-g768d612f7982) for
which I sent a pull request to Linus:

ext4/1k: 10 tests, 63 seconds
  generic/455  Pass     4s
  generic/482  Pass     8s
  generic/455  Pass     5s
  generic/482  Pass     8s
  generic/455  Pass     5s
  generic/482  Pass     7s
  generic/455  Pass     5s
  generic/482  Pass     8s
  generic/455  Pass     5s
  generic/482  Pass     8s
Totals: 10 tests, 0 skipped, 0 failures, 0 errors, 63s

... but I can confirm that it's failing on Linus's upstream (I tested
commit 708283abf896):

ext4/1k: 2 tests, 2 failures, 31 seconds
  generic/455  Failed   4s
  generic/455  Failed   2s
  generic/455  Pass     5s
  generic/455  Failed   3s
  generic/455  Failed   2s
  generic/482  Failed   2s
  generic/482  Failed   3s
  generic/482  Failed   1s
  generic/482  Failed   3s
  generic/482  Failed   4s
Totals: 10 tests, 0 skipped, 9 failures, 0 errors, 29s

						- Ted


P.S.  After doing some digging, it appears generic/455 does have some
failures on 6.4 (20%) and 6.5-rc3 (5%) on the ext4/1k blocksize test
config.  But *something* is definitely causing a much greater failure
rate in Linus's upstream.  The good news is that should make it
relatively easy to bisect.  I'll look into it.  Thanks for flagging
this.

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Theodore Ts'o]

#regzbot introduced: 8147c4c4546f9f05ef03bb839b741473b28bb560 ^

OK, I've isolated the regression of generic/455 failing with ext4/1k
to this commit, which came in via the mm tree.  Nothing seems
*obviously* wrong, but I'm not sure if there are any differences in
the semantics of the new folio functions such as kmap_local_folio,
offset_in_folio, set_folio_bh() which might be making a difference.

Using kvm-xfstests[1] I bisected this via the command:

% install-kconfig ; kbuild ; kvm-xfstests -c ext4/1k -C 10 generic/455

[1] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md


And the bisection pointed me at this commit:

    commit 8147c4c4546f9f05ef03bb839b741473b28bb560 (refs/bisect/bad)
    Author: Matthew Wilcox (Oracle) <willy@infradead.org>
    AuthorDate: Thu Jul 13 04:55:11 2023 +0100
    Commit: Andrew Morton <akpm@linux-foundation.org>
    CommitDate: Fri Aug 18 10:12:30 2023 -0700

        jbd2: use a folio in jbd2_journal_write_metadata_buffer()
    
During the bisection, I treated a commit with 3+ failures as "bad",
and 0-2 commits as "good".  Running generic/455 50 times to get a
sense of the failure, with the first bad commit (8147c4c4546f), I got:

    ext4/1k: 50 tests, 21 failures, 223 seconds
      Flaky: generic/455: 42% (21/50)
    Totals: 50 tests, 0 skipped, 21 failures, 0 errors, 223s

While with the immediately preceding commit (07811230c3cd), I got:

    ext4/1k: 50 tests, 4 failures, 235 seconds
      Flaky: generic/455:  8% (4/50)
    Totals: 50 tests, 0 skipped, 4 failures, 0 errors, 235s



Comparing these two commits (8147c4c4546f vs 07811230c3cd) using the
ext4 with a 4k block size, I get:

    ext4/4k: 50 tests, 2 failures, 365 seconds
      Flaky: generic/455:  4% (2/50)
    Totals: 50 tests, 0 skipped, 2 failures, 0 errors, 365s

vs

    ext4/4k: 50 tests, 2 failures, 349 seconds
      Flaky: generic/455:  4% (2/50)
    Totals: 50 tests, 0 skipped, 2 failures, 0 errors, 349s

So issue seems to be specifically with a sub-page size block size,
since ext4/4k doesn't show any issues, while ext4/1k does.

   	       	     		       	 - Ted

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Matthew Wilcox]

On Mon, Sep 04, 2023 at 02:08:19AM -0400, Theodore Ts'o wrote:
> #regzbot introduced: 8147c4c4546f9f05ef03bb839b741473b28bb560 ^
> 
> OK, I've isolated the regression of generic/455 failing with ext4/1k
> to this commit, which came in via the mm tree.  Nothing seems
> *obviously* wrong, but I'm not sure if there are any differences in
> the semantics of the new folio functions such as kmap_local_folio,
> offset_in_folio, set_folio_bh() which might be making a difference.

Thanks for the cc,  Let's see what we can do ...

virt_to_folio() - For an order-0 page, there is no difference.
offset_in_folio() - Ditto
bh->b_page vs bh->b_folio - Ditto
virt_to_folio() - Ditto
folio_set_bh() - Ditto

kmap_local_folio() vs kmap_atomic - Here, we have a difference.
memcpy_from_folio() - Same difference as above.

I suppose it must be this, and yet I cannot understand how it would
make a difference.  Perhaps you can help me?

static inline void *kmap_atomic_prot(struct page *page, pgprot_t prot)
{
        if (IS_ENABLED(CONFIG_PREEMPT_RT))
                migrate_disable();
        else
                preempt_disable();

        pagefault_disable();
        return __kmap_local_page_prot(page, prot);
}

vs

static inline void *kmap_local_folio(struct folio *folio, size_t offset)
{
        struct page *page = folio_page(folio, offset / PAGE_SIZE);
        return __kmap_local_page_prot(page, kmap_prot) + offset % PAGE_SIZE;
}

I don't believe that returning the address with the offset included
is the problem here.  It must be disabling preemption / migration.
There's no chace this funcation accesses userspace (... is there?) so
it can't be the pagefault_disable().

We can try splitting this up into tiny commits and figuring out which
of them is the problem.  I'll be back at work tomorrow and can look
more deeply then.

> Using kvm-xfstests[1] I bisected this via the command:
> 
> % install-kconfig ; kbuild ; kvm-xfstests -c ext4/1k -C 10 generic/455
> 
> [1] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
> 
> 
> And the bisection pointed me at this commit:
> 
>     commit 8147c4c4546f9f05ef03bb839b741473b28bb560 (refs/bisect/bad)
>     Author: Matthew Wilcox (Oracle) <willy@infradead.org>
>     AuthorDate: Thu Jul 13 04:55:11 2023 +0100
>     Commit: Andrew Morton <akpm@linux-foundation.org>
>     CommitDate: Fri Aug 18 10:12:30 2023 -0700
> 
>         jbd2: use a folio in jbd2_journal_write_metadata_buffer()
>     
> During the bisection, I treated a commit with 3+ failures as "bad",
> and 0-2 commits as "good".  Running generic/455 50 times to get a
> sense of the failure, with the first bad commit (8147c4c4546f), I got:
> 
>     ext4/1k: 50 tests, 21 failures, 223 seconds
>       Flaky: generic/455: 42% (21/50)
>     Totals: 50 tests, 0 skipped, 21 failures, 0 errors, 223s
> 
> While with the immediately preceding commit (07811230c3cd), I got:
> 
>     ext4/1k: 50 tests, 4 failures, 235 seconds
>       Flaky: generic/455:  8% (4/50)
>     Totals: 50 tests, 0 skipped, 4 failures, 0 errors, 235s
> 
> 
> 
> Comparing these two commits (8147c4c4546f vs 07811230c3cd) using the
> ext4 with a 4k block size, I get:
> 
>     ext4/4k: 50 tests, 2 failures, 365 seconds
>       Flaky: generic/455:  4% (2/50)
>     Totals: 50 tests, 0 skipped, 2 failures, 0 errors, 365s
> 
> vs
> 
>     ext4/4k: 50 tests, 2 failures, 349 seconds
>       Flaky: generic/455:  4% (2/50)
>     Totals: 50 tests, 0 skipped, 2 failures, 0 errors, 349s
> 
> So issue seems to be specifically with a sub-page size block size,
> since ext4/4k doesn't show any issues, while ext4/1k does.

I doubt I tried it with a 1kB block size, so I'll focus on that too.

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Ritesh Harjani]

Matthew Wilcox <willy@infradead.org> writes:

> On Mon, Sep 04, 2023 at 02:08:19AM -0400, Theodore Ts'o wrote:
>> #regzbot introduced: 8147c4c4546f9f05ef03bb839b741473b28bb560 ^
>> 
>> OK, I've isolated the regression of generic/455 failing with ext4/1k
>> to this commit, which came in via the mm tree.  Nothing seems
>> *obviously* wrong, but I'm not sure if there are any differences in
>> the semantics of the new folio functions such as kmap_local_folio,
>> offset_in_folio, set_folio_bh() which might be making a difference.
>
> Thanks for the cc,  Let's see what we can do ...
>
> virt_to_folio() - For an order-0 page, there is no difference.
> offset_in_folio() - Ditto
> bh->b_page vs bh->b_folio - Ditto
> virt_to_folio() - Ditto
> folio_set_bh() - Ditto
>
> kmap_local_folio() vs kmap_atomic - Here, we have a difference.
> memcpy_from_folio() - Same difference as above.
>
> I suppose it must be this, and yet I cannot understand how it would
> make a difference.  Perhaps you can help me?
>
> static inline void *kmap_atomic_prot(struct page *page, pgprot_t prot)
> {
>         if (IS_ENABLED(CONFIG_PREEMPT_RT))
>                 migrate_disable();
>         else
>                 preempt_disable();
>
>         pagefault_disable();
>         return __kmap_local_page_prot(page, prot);
> }
>
> vs
>
> static inline void *kmap_local_folio(struct folio *folio, size_t offset)
> {
>         struct page *page = folio_page(folio, offset / PAGE_SIZE);
>         return __kmap_local_page_prot(page, kmap_prot) + offset % PAGE_SIZE;
> }
>
> I don't believe that returning the address with the offset included
> is the problem here.  It must be disabling preemption / migration.
> There's no chace this funcation accesses userspace (... is there?) so
> it can't be the pagefault_disable().
>
> We can try splitting this up into tiny commits and figuring out which
> of them is the problem.  I'll be back at work tomorrow and can look
> more deeply then.
>
>> Using kvm-xfstests[1] I bisected this via the command:
>> 
>> % install-kconfig ; kbuild ; kvm-xfstests -c ext4/1k -C 10 generic/455
>> 
>> [1] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
>> 
>> 
>> And the bisection pointed me at this commit:
>> 
>>     commit 8147c4c4546f9f05ef03bb839b741473b28bb560 (refs/bisect/bad)
>>     Author: Matthew Wilcox (Oracle) <willy@infradead.org>
>>     AuthorDate: Thu Jul 13 04:55:11 2023 +0100
>>     Commit: Andrew Morton <akpm@linux-foundation.org>
>>     CommitDate: Fri Aug 18 10:12:30 2023 -0700
>> 
>>         jbd2: use a folio in jbd2_journal_write_metadata_buffer()
>>     

This is inline with my observation too. 

However, is this log expected with below diff when running with ext4/1k?
I am finding a folio with order > 0 here.

<diff>
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 768fa05bcbed..152c08e83fa2 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -369,6 +369,12 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
                new_offset = offset_in_folio(new_folio, jh2bh(jh_in)->b_data);
        }

+       if (folio_size(new_folio) > PAGE_SIZE) {
+               pr_crit("%s: folio_size=%lu, folio_order=%d, new_offset=%u bh_size=%lu folio_test_large=%d\n",
+                       __func__, folio_size(new_folio), folio_order(new_folio), new_offset,
+                       bh_in->b_size, folio_test_large(new_folio));
+       }
+
        mapped_data = kmap_local_folio(new_folio, new_offset);
        /*
         * Fire data frozen trigger if data already wasn't frozen.  Do this

<dmesg log>
[   40.419772] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=0 bh_size=1024 folio_test_large=1
[   40.444737] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=2048 bh_size=1024 folio_test_large=1
[   40.472385] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=3072 bh_size=1024 folio_test_large=1
[   40.560581] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=8192 bh_size=1024 folio_test_large=1
[   40.588512] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=10240 bh_size=1024 folio_test_large=1
[   40.612103] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=7168 bh_size=1024 folio_test_large=1
[   40.636800] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=9216 bh_size=1024 folio_test_large=1
[   40.661166] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=10240 bh_size=1024 folio_test_large=1


Is this code path a possibility, which can cause above logs?

   ptr = jbd2_alloc() -> kmem_cache_alloc()
   <..>
   new_folio = virt_to_folio(ptr)
   new_offset = offset_in_folio(new_folio, ptr)

And then I am still not sure what the problem really is? 
Is it because at the time of checkpointing, the path is still not fully
converted to folio?

I am still missing a lot of pieces here, sorry. 

-ritesh

>> During the bisection, I treated a commit with 3+ failures as "bad",
>> and 0-2 commits as "good".  Running generic/455 50 times to get a
>> sense of the failure, with the first bad commit (8147c4c4546f), I got:
>> 
>>     ext4/1k: 50 tests, 21 failures, 223 seconds
>>       Flaky: generic/455: 42% (21/50)
>>     Totals: 50 tests, 0 skipped, 21 failures, 0 errors, 223s
>> 
>> While with the immediately preceding commit (07811230c3cd), I got:
>> 
>>     ext4/1k: 50 tests, 4 failures, 235 seconds
>>       Flaky: generic/455:  8% (4/50)
>>     Totals: 50 tests, 0 skipped, 4 failures, 0 errors, 235s
>> 
>> 
>> 
>> Comparing these two commits (8147c4c4546f vs 07811230c3cd) using the
>> ext4 with a 4k block size, I get:
>> 
>>     ext4/4k: 50 tests, 2 failures, 365 seconds
>>       Flaky: generic/455:  4% (2/50)
>>     Totals: 50 tests, 0 skipped, 2 failures, 0 errors, 365s
>> 
>> vs
>> 
>>     ext4/4k: 50 tests, 2 failures, 349 seconds
>>       Flaky: generic/455:  4% (2/50)
>>     Totals: 50 tests, 0 skipped, 2 failures, 0 errors, 349s
>> 
>> So issue seems to be specifically with a sub-page size block size,
>> since ext4/4k doesn't show any issues, while ext4/1k does.
>
> I doubt I tried it with a 1kB block size, so I'll focus on that too.

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Matthew Wilcox]

On Wed, Sep 06, 2023 at 04:33:35PM +0530, Ritesh Harjani wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> 
> > On Mon, Sep 04, 2023 at 02:08:19AM -0400, Theodore Ts'o wrote:
> >> #regzbot introduced: 8147c4c4546f9f05ef03bb839b741473b28bb560 ^
> >> 
> >> OK, I've isolated the regression of generic/455 failing with ext4/1k
> >> to this commit, which came in via the mm tree.  Nothing seems
> >> *obviously* wrong, but I'm not sure if there are any differences in
> >> the semantics of the new folio functions such as kmap_local_folio,
> >> offset_in_folio, set_folio_bh() which might be making a difference.
> >
> > Thanks for the cc,  Let's see what we can do ...
> >
> > virt_to_folio() - For an order-0 page, there is no difference.
> > offset_in_folio() - Ditto
> > bh->b_page vs bh->b_folio - Ditto
> > virt_to_folio() - Ditto
> > folio_set_bh() - Ditto
> >
> > kmap_local_folio() vs kmap_atomic - Here, we have a difference.
> > memcpy_from_folio() - Same difference as above.
> >
> > I suppose it must be this, and yet I cannot understand how it would
> > make a difference.  Perhaps you can help me?
> >
> > static inline void *kmap_atomic_prot(struct page *page, pgprot_t prot)
> > {
> >         if (IS_ENABLED(CONFIG_PREEMPT_RT))
> >                 migrate_disable();
> >         else
> >                 preempt_disable();
> >
> >         pagefault_disable();
> >         return __kmap_local_page_prot(page, prot);
> > }
> >
> > vs
> >
> > static inline void *kmap_local_folio(struct folio *folio, size_t offset)
> > {
> >         struct page *page = folio_page(folio, offset / PAGE_SIZE);
> >         return __kmap_local_page_prot(page, kmap_prot) + offset % PAGE_SIZE;
> > }
> >
> > I don't believe that returning the address with the offset included
> > is the problem here.  It must be disabling preemption / migration.
> > There's no chace this funcation accesses userspace (... is there?) so
> > it can't be the pagefault_disable().
> >
> > We can try splitting this up into tiny commits and figuring out which
> > of them is the problem.  I'll be back at work tomorrow and can look
> > more deeply then.
> >
> >> Using kvm-xfstests[1] I bisected this via the command:
> >> 
> >> % install-kconfig ; kbuild ; kvm-xfstests -c ext4/1k -C 10 generic/455
> >> 
> >> [1] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
> >> 
> >> 
> >> And the bisection pointed me at this commit:
> >> 
> >>     commit 8147c4c4546f9f05ef03bb839b741473b28bb560 (refs/bisect/bad)
> >>     Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> >>     AuthorDate: Thu Jul 13 04:55:11 2023 +0100
> >>     Commit: Andrew Morton <akpm@linux-foundation.org>
> >>     CommitDate: Fri Aug 18 10:12:30 2023 -0700
> >> 
> >>         jbd2: use a folio in jbd2_journal_write_metadata_buffer()
> >>     
> 
> This is inline with my observation too. 
> 
> However, is this log expected with below diff when running with ext4/1k?
> I am finding a folio with order > 0 here.
> 
> <diff>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 768fa05bcbed..152c08e83fa2 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -369,6 +369,12 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>                 new_offset = offset_in_folio(new_folio, jh2bh(jh_in)->b_data);
>         }
> 
> +       if (folio_size(new_folio) > PAGE_SIZE) {
> +               pr_crit("%s: folio_size=%lu, folio_order=%d, new_offset=%u bh_size=%lu folio_test_large=%d\n",
> +                       __func__, folio_size(new_folio), folio_order(new_folio), new_offset,
> +                       bh_in->b_size, folio_test_large(new_folio));
> +       }
> +
>         mapped_data = kmap_local_folio(new_folio, new_offset);
>         /*
>          * Fire data frozen trigger if data already wasn't frozen.  Do this
> 
> <dmesg log>
> [   40.419772] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=0 bh_size=1024 folio_test_large=1
> [   40.444737] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=2048 bh_size=1024 folio_test_large=1
> [   40.472385] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=3072 bh_size=1024 folio_test_large=1
> [   40.560581] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=8192 bh_size=1024 folio_test_large=1
> [   40.588512] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=10240 bh_size=1024 folio_test_large=1
> [   40.612103] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=7168 bh_size=1024 folio_test_large=1
> [   40.636800] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=9216 bh_size=1024 folio_test_large=1
> [   40.661166] jbd2_journal_write_metadata_buffer: folio_size=16384, folio_order=2, new_offset=10240 bh_size=1024 folio_test_large=1
> 
> 
> Is this code path a possibility, which can cause above logs?
> 
>    ptr = jbd2_alloc() -> kmem_cache_alloc()
>    <..>
>    new_folio = virt_to_folio(ptr)
>    new_offset = offset_in_folio(new_folio, ptr)
> 
> And then I am still not sure what the problem really is? 
> Is it because at the time of checkpointing, the path is still not fully
> converted to folio?

Oh yikes!  I didn't know that the allocation might come from kmalloc!
Yes, slab might use high-order allocations.  I'll have to look through
this and figure out what the problem might be.

Thanks for debugging this.

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Matthew Wilcox]

On Wed, Sep 06, 2023 at 01:38:23PM +0100, Matthew Wilcox wrote:
> > Is this code path a possibility, which can cause above logs?
> > 
> >    ptr = jbd2_alloc() -> kmem_cache_alloc()
> >    <..>
> >    new_folio = virt_to_folio(ptr)
> >    new_offset = offset_in_folio(new_folio, ptr)
> > 
> > And then I am still not sure what the problem really is? 
> > Is it because at the time of checkpointing, the path is still not fully
> > converted to folio?
> 
> Oh yikes!  I didn't know that the allocation might come from kmalloc!
> Yes, slab might use high-order allocations.  I'll have to look through
> this and figure out what the problem might be.

I think the probable cause is bh_offset().  Before these patches, if
we allocated a buffer at offset 9kB into an order-2 slab, we'd fill in
b_page with the third page of the slab and calculate bh_offset as 1kB.
With these patches, we set b_page to the first page of the slab, and
bh_offset still comes back as 1kB so we read from / write to entirely
the wrong place.

With this redefinition of bh_offset(), we calculate the offset relative
to the base page if it's a tail page, and relative to the folio if it's
a folio.  Works out nicely ;-)

I have three other things I'm trying to debug right now, so this isn't
tested, but if you have time you might want to give it a run.

diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 6cb3e9af78c9..dc8fcdc40e95 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -173,7 +173,10 @@ static __always_inline int buffer_uptodate(const struct buffer_head *bh)
 	return test_bit_acquire(BH_Uptodate, &bh->b_state);
 }
 
-#define bh_offset(bh)		((unsigned long)(bh)->b_data & ~PAGE_MASK)
+static inline unsigned long bh_offset(struct buffer_head *bh)
+{
+	return (unsigned long)(bh)->b_data & (page_size(bh->b_page) - 1);
+}
 
 /* If we *know* page->private refers to buffer_heads */
 #define page_buffers(page)					\

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Ritesh Harjani]

Matthew Wilcox <willy@infradead.org> writes:

> On Wed, Sep 06, 2023 at 01:38:23PM +0100, Matthew Wilcox wrote:
>> > Is this code path a possibility, which can cause above logs?
>> > 
>> >    ptr = jbd2_alloc() -> kmem_cache_alloc()
>> >    <..>
>> >    new_folio = virt_to_folio(ptr)
>> >    new_offset = offset_in_folio(new_folio, ptr)
>> > 
>> > And then I am still not sure what the problem really is? 
>> > Is it because at the time of checkpointing, the path is still not fully
>> > converted to folio?
>> 
>> Oh yikes!  I didn't know that the allocation might come from kmalloc!
>> Yes, slab might use high-order allocations.  I'll have to look through
>> this and figure out what the problem might be.
>
> I think the probable cause is bh_offset().  Before these patches, if
> we allocated a buffer at offset 9kB into an order-2 slab, we'd fill in
> b_page with the third page of the slab and calculate bh_offset as 1kB.
> With these patches, we set b_page to the first page of the slab, and
> bh_offset still comes back as 1kB so we read from / write to entirely
> the wrong place.
>
> With this redefinition of bh_offset(), we calculate the offset relative
> to the base page if it's a tail page, and relative to the folio if it's
> a folio.  Works out nicely ;-)

Thanks Matthew for explaining the problem clearly.


>
> I have three other things I'm trying to debug right now, so this isn't
> tested, but if you have time you might want to give it a run.

sure, I gave it a try.

>
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index 6cb3e9af78c9..dc8fcdc40e95 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -173,7 +173,10 @@ static __always_inline int buffer_uptodate(const struct buffer_head *bh)
>  	return test_bit_acquire(BH_Uptodate, &bh->b_state);
>  }
>  
> -#define bh_offset(bh)		((unsigned long)(bh)->b_data & ~PAGE_MASK)
> +static inline unsigned long bh_offset(struct buffer_head *bh)
> +{
> +	return (unsigned long)(bh)->b_data & (page_size(bh->b_page) - 1);
> +}
>  
>  /* If we *know* page->private refers to buffer_heads */
>  #define page_buffers(page)					\


I used "const" for bh to avoid warnings from fs/nilfs/alloc.c

diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 4ede47649a81..b61fa79cb7f5 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -171,7 +171,10 @@ static __always_inline int buffer_uptodate(const struct buffer_head *bh)
        return test_bit_acquire(BH_Uptodate, &bh->b_state);
 }

-#define bh_offset(bh)          ((unsigned long)(bh)->b_data & ~PAGE_MASK)
+static inline unsigned long bh_offset(const struct buffer_head *bh)
+{
+       return (unsigned long)(bh)->b_data & (page_size(bh->b_page) - 1);
+}

 /* If we *know* page->private refers to buffer_heads */
 #define page_buffers(page)                                     \


But this change alone was still giving me failures. On looking into
usage of b_data, I found we use offset_in_page() instead of bh_offset()
in jbd2. So I added below changes in fs/jbd2 to replace offset_in_page()
to bh_offset()...

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 1073259902a6..0c25640714ac 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -304,7 +304,7 @@ static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)

        addr = kmap_atomic(page);
        checksum = crc32_be(crc32_sum,
-               (void *)(addr + offset_in_page(bh->b_data)), bh->b_size);
+               (void *)(addr + bh_offset(bh)), bh->b_size);
        kunmap_atomic(addr);

        return checksum;
@@ -333,7 +333,7 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
        seq = cpu_to_be32(sequence);
        addr = kmap_atomic(page);
        csum32 = jbd2_chksum(j, j->j_csum_seed, (__u8 *)&seq, sizeof(seq));
-       csum32 = jbd2_chksum(j, csum32, addr + offset_in_page(bh->b_data),
+       csum32 = jbd2_chksum(j, csum32, addr + bh_offset(bh),
                             bh->b_size);
        kunmap_atomic(addr);

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 4d1fda1f7143..2ac57f7a242d 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -942,7 +942,7 @@ static void jbd2_freeze_jh_data(struct journal_head *jh)

        J_EXPECT_JH(jh, buffer_uptodate(bh), "Possible IO failure.\n");
        page = bh->b_page;
-       offset = offset_in_page(bh->b_data);
+       offset = bh_offset(bh);
        source = kmap_atomic(page);
        /* Fire data frozen trigger just before we copy the data */
        jbd2_buffer_frozen_trigger(jh, source + offset, jh->b_triggers);


With all of above diffs, here are the results.

ext4/1k: 15 tests, 1 failures, 1709 seconds
  generic/455  Pass     43s
  generic/475  Pass     128s
  generic/482  Pass     183s
  generic/455  Pass     43s
  generic/475  Pass     134s
  generic/482  Pass     191s
  generic/455  Pass     41s
  generic/475  Pass     139s
  generic/482  Pass     135s
  generic/455  Pass     46s
  generic/475  Pass     132s
  generic/482  Pass     146s
  generic/455  Pass     47s
  generic/475  Failed   145s
  generic/482  Pass     156s
Totals: 15 tests, 0 skipped, 1 failures, 0 errors, 1709s

I guess the above failure (generic/475) could be due to it's flakey
behaviour which Ted was mentioning.


Now, while we are at it, I think we should also make change to reiserfs from
offset_in_page() to bh_offset()

diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index 015bfe4e4524..23411ec163d4 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -4217,7 +4217,7 @@ static int do_journal_end(struct reiserfs_transaction_handle *th, int flags)
                        page = cn->bh->b_page;
                        addr = kmap(page);
                        memcpy(tmp_bh->b_data,
-                              addr + offset_in_page(cn->bh->b_data),
+                              addr + bh_offset(cn->bh),
                               cn->bh->b_size);
                        kunmap(page);
                        mark_buffer_dirty(tmp_bh);


I will also run "auto" group with ext4/1k with all of above change. Will
update the results once it is done.


-ritesh

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Matthew Wilcox]

On Thu, Sep 07, 2023 at 08:26:35AM +0530, Ritesh Harjani wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> 
> > On Wed, Sep 06, 2023 at 01:38:23PM +0100, Matthew Wilcox wrote:
> >> > Is this code path a possibility, which can cause above logs?
> >> > 
> >> >    ptr = jbd2_alloc() -> kmem_cache_alloc()
> >> >    <..>
> >> >    new_folio = virt_to_folio(ptr)
> >> >    new_offset = offset_in_folio(new_folio, ptr)
> >> > 
> >> > And then I am still not sure what the problem really is? 
> >> > Is it because at the time of checkpointing, the path is still not fully
> >> > converted to folio?
> >> 
> >> Oh yikes!  I didn't know that the allocation might come from kmalloc!
> >> Yes, slab might use high-order allocations.  I'll have to look through
> >> this and figure out what the problem might be.
> >
> > I think the probable cause is bh_offset().  Before these patches, if
> > we allocated a buffer at offset 9kB into an order-2 slab, we'd fill in
> > b_page with the third page of the slab and calculate bh_offset as 1kB.
> > With these patches, we set b_page to the first page of the slab, and
> > bh_offset still comes back as 1kB so we read from / write to entirely
> > the wrong place.
> >
> > With this redefinition of bh_offset(), we calculate the offset relative
> > to the base page if it's a tail page, and relative to the folio if it's
> > a folio.  Works out nicely ;-)
> 
> Thanks Matthew for explaining the problem clearly.
> 
> 
> >
> > I have three other things I'm trying to debug right now, so this isn't
> > tested, but if you have time you might want to give it a run.
> 
> sure, I gave it a try.
> 
> >
> > diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> > index 6cb3e9af78c9..dc8fcdc40e95 100644
> > --- a/include/linux/buffer_head.h
> > +++ b/include/linux/buffer_head.h
> > @@ -173,7 +173,10 @@ static __always_inline int buffer_uptodate(const struct buffer_head *bh)
> >  	return test_bit_acquire(BH_Uptodate, &bh->b_state);
> >  }
> >  
> > -#define bh_offset(bh)		((unsigned long)(bh)->b_data & ~PAGE_MASK)
> > +static inline unsigned long bh_offset(struct buffer_head *bh)
> > +{
> > +	return (unsigned long)(bh)->b_data & (page_size(bh->b_page) - 1);
> > +}
> >  
> >  /* If we *know* page->private refers to buffer_heads */
> >  #define page_buffers(page)					\
> 
> 
> I used "const" for bh to avoid warnings from fs/nilfs/alloc.c

Excellent.  I didn't try compiling nilfs ;-)

> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index 4ede47649a81..b61fa79cb7f5 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -171,7 +171,10 @@ static __always_inline int buffer_uptodate(const struct buffer_head *bh)
>         return test_bit_acquire(BH_Uptodate, &bh->b_state);
>  }
> 
> -#define bh_offset(bh)          ((unsigned long)(bh)->b_data & ~PAGE_MASK)
> +static inline unsigned long bh_offset(const struct buffer_head *bh)
> +{
> +       return (unsigned long)(bh)->b_data & (page_size(bh->b_page) - 1);
> +}
> 
>  /* If we *know* page->private refers to buffer_heads */
>  #define page_buffers(page)                                     \
> 
> 
> But this change alone was still giving me failures. On looking into
> usage of b_data, I found we use offset_in_page() instead of bh_offset()
> in jbd2. So I added below changes in fs/jbd2 to replace offset_in_page()
> to bh_offset()...
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 1073259902a6..0c25640714ac 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -304,7 +304,7 @@ static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
> 
>         addr = kmap_atomic(page);
>         checksum = crc32_be(crc32_sum,
> -               (void *)(addr + offset_in_page(bh->b_data)), bh->b_size);
> +               (void *)(addr + bh_offset(bh)), bh->b_size);
>         kunmap_atomic(addr);

Hm, that's not going to work on a highmem machine.  It'll work on 64-bit!
Actually, no, it'll work on a highmem machine because slab doesn't
allocate from highmem.  Still, it's a bit unclean.  Let's go full folio
on this one:

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 1073259902a6..8d6f934c3d95 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -298,14 +298,12 @@ static int journal_finish_inode_data_buffers(journal_t *journal,
 
 static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
 {
-	struct page *page = bh->b_page;
 	char *addr;
 	__u32 checksum;
 
-	addr = kmap_atomic(page);
-	checksum = crc32_be(crc32_sum,
-		(void *)(addr + offset_in_page(bh->b_data)), bh->b_size);
-	kunmap_atomic(addr);
+	addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
+	checksum = crc32_be(crc32_sum, addr, bh->b_size);
+	kunmap_local(addr);
 
 	return checksum;
 }
@@ -322,7 +320,6 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
 				    struct buffer_head *bh, __u32 sequence)
 {
 	journal_block_tag3_t *tag3 = (journal_block_tag3_t *)tag;
-	struct page *page = bh->b_page;
 	__u8 *addr;
 	__u32 csum32;
 	__be32 seq;
@@ -331,11 +328,10 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
 		return;
 
 	seq = cpu_to_be32(sequence);
-	addr = kmap_atomic(page);
+	addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
 	csum32 = jbd2_chksum(j, j->j_csum_seed, (__u8 *)&seq, sizeof(seq));
-	csum32 = jbd2_chksum(j, csum32, addr + offset_in_page(bh->b_data),
-			     bh->b_size);
-	kunmap_atomic(addr);
+	csum32 = jbd2_chksum(j, csum32, addr, bh->b_size);
+	kunmap_local(addr);
 
 	if (jbd2_has_feature_csum3(j))
 		tag3->t_checksum = cpu_to_be32(csum32);
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 4d1fda1f7143..5f08b5fd105a 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -935,19 +935,15 @@ static void warn_dirty_buffer(struct buffer_head *bh)
 /* Call t_frozen trigger and copy buffer data into jh->b_frozen_data. */
 static void jbd2_freeze_jh_data(struct journal_head *jh)
 {
-	struct page *page;
-	int offset;
 	char *source;
 	struct buffer_head *bh = jh2bh(jh);
 
 	J_EXPECT_JH(jh, buffer_uptodate(bh), "Possible IO failure.\n");
-	page = bh->b_page;
-	offset = offset_in_page(bh->b_data);
-	source = kmap_atomic(page);
+	source = kmap_local_folio(bh->b_folio, bh_offset(bh));
 	/* Fire data frozen trigger just before we copy the data */
-	jbd2_buffer_frozen_trigger(jh, source + offset, jh->b_triggers);
-	memcpy(jh->b_frozen_data, source + offset, bh->b_size);
-	kunmap_atomic(source);
+	jbd2_buffer_frozen_trigger(jh, source, jh->b_triggers);
+	memcpy(jh->b_frozen_data, source, bh->b_size);
+	kunmap_local(source);
 
 	/*
 	 * Now that the frozen data is saved off, we need to store any matching

(I've been thinking about adding a kmap_local_bh(bh))

> ext4/1k: 15 tests, 1 failures, 1709 seconds
>   generic/455  Pass     43s
>   generic/475  Pass     128s
>   generic/482  Pass     183s
>   generic/455  Pass     43s
>   generic/475  Pass     134s
>   generic/482  Pass     191s
>   generic/455  Pass     41s
>   generic/475  Pass     139s
>   generic/482  Pass     135s
>   generic/455  Pass     46s
>   generic/475  Pass     132s
>   generic/482  Pass     146s
>   generic/455  Pass     47s
>   generic/475  Failed   145s
>   generic/482  Pass     156s
> Totals: 15 tests, 0 skipped, 1 failures, 0 errors, 1709s
> 
> I guess the above failure (generic/475) could be due to it's flakey
> behaviour which Ted was mentioning.
> 
> 
> Now, while we are at it, I think we should also make change to reiserfs from
> offset_in_page() to bh_offset()
> 
> diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
> index 015bfe4e4524..23411ec163d4 100644
> --- a/fs/reiserfs/journal.c
> +++ b/fs/reiserfs/journal.c
> @@ -4217,7 +4217,7 @@ static int do_journal_end(struct reiserfs_transaction_handle *th, int flags)
>                         page = cn->bh->b_page;
>                         addr = kmap(page);
>                         memcpy(tmp_bh->b_data,
> -                              addr + offset_in_page(cn->bh->b_data),
> +                              addr + bh_offset(cn->bh),
>                                cn->bh->b_size);
>                         kunmap(page);

This one should probably be:

-			addr = kmap(page);
-			memcpy(tmp_bh->b_data,
-				addr + offset_in_page(cn->bh->b_data),
-				cn->bh->b_size);
-			kunmap(page);
+			memcpy_from_folio(tmp_bh->b_data, cn->bh->b_folio,
+					bh_offset(cn->bh), cn->bh->b_size);

> I will also run "auto" group with ext4/1k with all of above change. Will
> update the results once it is done.

Appreciate it!  I don't think you'll see a significant difference with
the patches above; you've nailed the actual problems and I'm just
using slighlty nicer APIs.

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Ritesh Harjani]

Matthew Wilcox <willy@infradead.org> writes:

> On Thu, Sep 07, 2023 at 08:26:35AM +0530, Ritesh Harjani wrote:
>> Matthew Wilcox <willy@infradead.org> writes:
>> 
>> > On Wed, Sep 06, 2023 at 01:38:23PM +0100, Matthew Wilcox wrote:
>> >> > Is this code path a possibility, which can cause above logs?
>> >> > 
>> >> >    ptr = jbd2_alloc() -> kmem_cache_alloc()
>> >> >    <..>
>> >> >    new_folio = virt_to_folio(ptr)
>> >> >    new_offset = offset_in_folio(new_folio, ptr)
>> >> > 
>> >> > And then I am still not sure what the problem really is? 
>> >> > Is it because at the time of checkpointing, the path is still not fully
>> >> > converted to folio?
>> >> 
>> >> Oh yikes!  I didn't know that the allocation might come from kmalloc!
>> >> Yes, slab might use high-order allocations.  I'll have to look through
>> >> this and figure out what the problem might be.
>> >
>> > I think the probable cause is bh_offset().  Before these patches, if
>> > we allocated a buffer at offset 9kB into an order-2 slab, we'd fill in
>> > b_page with the third page of the slab and calculate bh_offset as 1kB.
>> > With these patches, we set b_page to the first page of the slab, and
>> > bh_offset still comes back as 1kB so we read from / write to entirely
>> > the wrong place.
>> >
>> > With this redefinition of bh_offset(), we calculate the offset relative
>> > to the base page if it's a tail page, and relative to the folio if it's
>> > a folio.  Works out nicely ;-)
>> 
>> Thanks Matthew for explaining the problem clearly.
>> 
>> 
>> >
>> > I have three other things I'm trying to debug right now, so this isn't
>> > tested, but if you have time you might want to give it a run.
>> 
>> sure, I gave it a try.
>> 
>> >
>> > diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
>> > index 6cb3e9af78c9..dc8fcdc40e95 100644
>> > --- a/include/linux/buffer_head.h
>> > +++ b/include/linux/buffer_head.h
>> > @@ -173,7 +173,10 @@ static __always_inline int buffer_uptodate(const struct buffer_head *bh)
>> >  	return test_bit_acquire(BH_Uptodate, &bh->b_state);
>> >  }
>> >  
>> > -#define bh_offset(bh)		((unsigned long)(bh)->b_data & ~PAGE_MASK)
>> > +static inline unsigned long bh_offset(struct buffer_head *bh)
>> > +{
>> > +	return (unsigned long)(bh)->b_data & (page_size(bh->b_page) - 1);
>> > +}
>> >  
>> >  /* If we *know* page->private refers to buffer_heads */
>> >  #define page_buffers(page)					\
>> 
>> 
>> I used "const" for bh to avoid warnings from fs/nilfs/alloc.c
>
> Excellent.  I didn't try compiling nilfs ;-)
>
>> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
>> index 4ede47649a81..b61fa79cb7f5 100644
>> --- a/include/linux/buffer_head.h
>> +++ b/include/linux/buffer_head.h
>> @@ -171,7 +171,10 @@ static __always_inline int buffer_uptodate(const struct buffer_head *bh)
>>         return test_bit_acquire(BH_Uptodate, &bh->b_state);
>>  }
>> 
>> -#define bh_offset(bh)          ((unsigned long)(bh)->b_data & ~PAGE_MASK)
>> +static inline unsigned long bh_offset(const struct buffer_head *bh)
>> +{
>> +       return (unsigned long)(bh)->b_data & (page_size(bh->b_page) - 1);
>> +}
>> 
>>  /* If we *know* page->private refers to buffer_heads */
>>  #define page_buffers(page)                                     \
>> 
>> 
>> But this change alone was still giving me failures. On looking into
>> usage of b_data, I found we use offset_in_page() instead of bh_offset()
>> in jbd2. So I added below changes in fs/jbd2 to replace offset_in_page()
>> to bh_offset()...
>> 
>> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
>> index 1073259902a6..0c25640714ac 100644
>> --- a/fs/jbd2/commit.c
>> +++ b/fs/jbd2/commit.c
>> @@ -304,7 +304,7 @@ static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
>> 
>>         addr = kmap_atomic(page);
>>         checksum = crc32_be(crc32_sum,
>> -               (void *)(addr + offset_in_page(bh->b_data)), bh->b_size);
>> +               (void *)(addr + bh_offset(bh)), bh->b_size);
>>         kunmap_atomic(addr);
>
> Hm, that's not going to work on a highmem machine.  It'll work on 64-bit!
> Actually, no, it'll work on a highmem machine because slab doesn't
> allocate from highmem.  Still, it's a bit unclean.  Let's go full folio
> on this one:
>
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 1073259902a6..8d6f934c3d95 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -298,14 +298,12 @@ static int journal_finish_inode_data_buffers(journal_t *journal,
>  
>  static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
>  {
> -	struct page *page = bh->b_page;
>  	char *addr;
>  	__u32 checksum;
>  
> -	addr = kmap_atomic(page);
> -	checksum = crc32_be(crc32_sum,
> -		(void *)(addr + offset_in_page(bh->b_data)), bh->b_size);
> -	kunmap_atomic(addr);
> +	addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
> +	checksum = crc32_be(crc32_sum, addr, bh->b_size);
> +	kunmap_local(addr);
>  
>  	return checksum;
>  }
> @@ -322,7 +320,6 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
>  				    struct buffer_head *bh, __u32 sequence)
>  {
>  	journal_block_tag3_t *tag3 = (journal_block_tag3_t *)tag;
> -	struct page *page = bh->b_page;
>  	__u8 *addr;
>  	__u32 csum32;
>  	__be32 seq;
> @@ -331,11 +328,10 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
>  		return;
>  
>  	seq = cpu_to_be32(sequence);
> -	addr = kmap_atomic(page);
> +	addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
>  	csum32 = jbd2_chksum(j, j->j_csum_seed, (__u8 *)&seq, sizeof(seq));
> -	csum32 = jbd2_chksum(j, csum32, addr + offset_in_page(bh->b_data),
> -			     bh->b_size);
> -	kunmap_atomic(addr);
> +	csum32 = jbd2_chksum(j, csum32, addr, bh->b_size);
> +	kunmap_local(addr);
>  
>  	if (jbd2_has_feature_csum3(j))
>  		tag3->t_checksum = cpu_to_be32(csum32);
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 4d1fda1f7143..5f08b5fd105a 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -935,19 +935,15 @@ static void warn_dirty_buffer(struct buffer_head *bh)
>  /* Call t_frozen trigger and copy buffer data into jh->b_frozen_data. */
>  static void jbd2_freeze_jh_data(struct journal_head *jh)
>  {
> -	struct page *page;
> -	int offset;
>  	char *source;
>  	struct buffer_head *bh = jh2bh(jh);
>  
>  	J_EXPECT_JH(jh, buffer_uptodate(bh), "Possible IO failure.\n");
> -	page = bh->b_page;
> -	offset = offset_in_page(bh->b_data);
> -	source = kmap_atomic(page);
> +	source = kmap_local_folio(bh->b_folio, bh_offset(bh));
>  	/* Fire data frozen trigger just before we copy the data */
> -	jbd2_buffer_frozen_trigger(jh, source + offset, jh->b_triggers);
> -	memcpy(jh->b_frozen_data, source + offset, bh->b_size);
> -	kunmap_atomic(source);
> +	jbd2_buffer_frozen_trigger(jh, source, jh->b_triggers);
> +	memcpy(jh->b_frozen_data, source, bh->b_size);
> +	kunmap_local(source);
>  
>  	/*
>  	 * Now that the frozen data is saved off, we need to store any matching
>
> (I've been thinking about adding a kmap_local_bh(bh))
>
>> ext4/1k: 15 tests, 1 failures, 1709 seconds
>>   generic/455  Pass     43s
>>   generic/475  Pass     128s
>>   generic/482  Pass     183s
>>   generic/455  Pass     43s
>>   generic/475  Pass     134s
>>   generic/482  Pass     191s
>>   generic/455  Pass     41s
>>   generic/475  Pass     139s
>>   generic/482  Pass     135s
>>   generic/455  Pass     46s
>>   generic/475  Pass     132s
>>   generic/482  Pass     146s
>>   generic/455  Pass     47s
>>   generic/475  Failed   145s
>>   generic/482  Pass     156s
>> Totals: 15 tests, 0 skipped, 1 failures, 0 errors, 1709s
>> 
>> I guess the above failure (generic/475) could be due to it's flakey
>> behaviour which Ted was mentioning.
>> 
>> 
>> Now, while we are at it, I think we should also make change to reiserfs from
>> offset_in_page() to bh_offset()
>> 
>> diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
>> index 015bfe4e4524..23411ec163d4 100644
>> --- a/fs/reiserfs/journal.c
>> +++ b/fs/reiserfs/journal.c
>> @@ -4217,7 +4217,7 @@ static int do_journal_end(struct reiserfs_transaction_handle *th, int flags)
>>                         page = cn->bh->b_page;
>>                         addr = kmap(page);
>>                         memcpy(tmp_bh->b_data,
>> -                              addr + offset_in_page(cn->bh->b_data),
>> +                              addr + bh_offset(cn->bh),
>>                                cn->bh->b_size);
>>                         kunmap(page);
>
> This one should probably be:
>
> -			addr = kmap(page);
> -			memcpy(tmp_bh->b_data,
> -				addr + offset_in_page(cn->bh->b_data),
> -				cn->bh->b_size);
> -			kunmap(page);
> +			memcpy_from_folio(tmp_bh->b_data, cn->bh->b_folio,
> +					bh_offset(cn->bh), cn->bh->b_size);
>
>> I will also run "auto" group with ext4/1k with all of above change. Will
>> update the results once it is done.
>
> Appreciate it!  I don't think you'll see a significant difference with
> the patches above; you've nailed the actual problems and I'm just
> using slighlty nicer APIs.

Thanks Matthew for proposing the final changes using folio.
(there were just some minor change required for fs/reiserfs/ for unused variables)
Pasting the final patch below (you as the author with my Signed-off-by &
Tested-by), which I have tested it on my system with "ext4/1k -g auto"

-------------------- Summary report
KERNEL:    kernel 6.5.0-xfstests-11705-ge1ee6db7734e #62 SMP PREEMPT_DYNAMIC Thu Sep  7 10:39:34 IST 2023 x86_64
CMDLINE:   -c ext4/1k -g auto
CPUS:      4
MEM:       7943.72

ext4/1k: 527 tests, 1 failures, 39 skipped, 9182 seconds
  Failures: ext4/059
Totals: 531 tests, 39 skipped, 5 failures, 0 errors, 9123s

You also proposed you would like to add kmap_local_bh(), hence not
sending it as a separate patch, in case if you would like to do it differently.

Thanks again for helping with the fix! 

---

From baeedb714497ae8f3809cc6e7cffa8884af43fac Mon Sep 17 00:00:00 2001
Message-Id: <baeedb714497ae8f3809cc6e7cffa8884af43fac.1694092539.git.ritesh.list@gmail.com>
From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Date: Thu, 7 Sep 2023 10:01:54 +0530
Subject: [PATCH] buffer: Fix definition of bh_offset() for struct buffer_head

Note that buffer head infrastructure is being transitioned from page based to
folio based- d685c668b069: ("buffer: add b_folio as an alias of b_page").

Now, jbd2_alloc() allocates a buffer (bh) from kmem cache when the
buffer_size is < PAGE_SIZE. (for e.g. 1k blocksize on 4k pagesize) and
then we might save this buffer info inside buffer_head, using
folio_set_bh() :-
        bh->b_folio = folio;
        if (!highmem)
          bh->b_data = folio_address(folio) + offset;

So far all good. However, while using this buffer's b_data, we use
bh_offset() or offset_in_page(), which assumes the buffer to be of
a PAGE_SIZE. This is not true anymore with b_folio as slab might use
high-order allocations.

This patch fixes the definition of bh_offset() and make use of
bh_offset() instead of offset_in_page() at places inside fs/jbd2 and
fs/reiserfs.
Also while we are at it, this patch converts these places to use folio
APIs instead.

Fixes: 8147c4c4546f ("jbd2: use a folio in jbd2_journal_write_metadata_buffer()")
Reported-by: Zorro Lang <zlang@kernel.org>
Tested-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/jbd2/commit.c            | 16 ++++++----------
 fs/jbd2/transaction.c       | 12 ++++--------
 fs/reiserfs/journal.c       | 11 +++--------
 include/linux/buffer_head.h |  5 ++++-
 4 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 1073259902a6..8d6f934c3d95 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -298,14 +298,12 @@ static int journal_finish_inode_data_buffers(journal_t *journal,

 static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
 {
-       struct page *page = bh->b_page;
        char *addr;
        __u32 checksum;

-       addr = kmap_atomic(page);
-       checksum = crc32_be(crc32_sum,
-               (void *)(addr + offset_in_page(bh->b_data)), bh->b_size);
-       kunmap_atomic(addr);
+       addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
+       checksum = crc32_be(crc32_sum, addr, bh->b_size);
+       kunmap_local(addr);

        return checksum;
 }
@@ -322,7 +320,6 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
                                    struct buffer_head *bh, __u32 sequence)
 {
        journal_block_tag3_t *tag3 = (journal_block_tag3_t *)tag;
-       struct page *page = bh->b_page;
        __u8 *addr;
        __u32 csum32;
        __be32 seq;
@@ -331,11 +328,10 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
                return;

        seq = cpu_to_be32(sequence);
-       addr = kmap_atomic(page);
+       addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
        csum32 = jbd2_chksum(j, j->j_csum_seed, (__u8 *)&seq, sizeof(seq));
-       csum32 = jbd2_chksum(j, csum32, addr + offset_in_page(bh->b_data),
-                            bh->b_size);
-       kunmap_atomic(addr);
+       csum32 = jbd2_chksum(j, csum32, addr, bh->b_size);
+       kunmap_local(addr);

        if (jbd2_has_feature_csum3(j))
                tag3->t_checksum = cpu_to_be32(csum32);
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 4d1fda1f7143..5f08b5fd105a 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -935,19 +935,15 @@ static void warn_dirty_buffer(struct buffer_head *bh)
 /* Call t_frozen trigger and copy buffer data into jh->b_frozen_data. */
 static void jbd2_freeze_jh_data(struct journal_head *jh)
 {
-       struct page *page;
-       int offset;
        char *source;
        struct buffer_head *bh = jh2bh(jh);

        J_EXPECT_JH(jh, buffer_uptodate(bh), "Possible IO failure.\n");
-       page = bh->b_page;
-       offset = offset_in_page(bh->b_data);
-       source = kmap_atomic(page);
+       source = kmap_local_folio(bh->b_folio, bh_offset(bh));
        /* Fire data frozen trigger just before we copy the data */
-       jbd2_buffer_frozen_trigger(jh, source + offset, jh->b_triggers);
-       memcpy(jh->b_frozen_data, source + offset, bh->b_size);
-       kunmap_atomic(source);
+       jbd2_buffer_frozen_trigger(jh, source, jh->b_triggers);
+       memcpy(jh->b_frozen_data, source, bh->b_size);
+       kunmap_local(source);

        /*
         * Now that the frozen data is saved off, we need to store any matching
diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index 015bfe4e4524..541ee1c5d2b3 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -4205,8 +4205,6 @@ static int do_journal_end(struct reiserfs_transaction_handle *th, int flags)
                /* copy all the real blocks into log area.  dirty log blocks */
                if (buffer_journaled(cn->bh)) {
                        struct buffer_head *tmp_bh;
-                       char *addr;
-                       struct page *page;
                        tmp_bh =
                            journal_getblk(sb,
                                           SB_ONDISK_JOURNAL_1st_BLOCK(sb) +
@@ -4214,12 +4212,9 @@ static int do_journal_end(struct reiserfs_transaction_handle *th, int flags)
                                             jindex) %
                                            SB_ONDISK_JOURNAL_SIZE(sb)));
                        set_buffer_uptodate(tmp_bh);
-                       page = cn->bh->b_page;
-                       addr = kmap(page);
-                       memcpy(tmp_bh->b_data,
-                              addr + offset_in_page(cn->bh->b_data),
-                              cn->bh->b_size);
-                       kunmap(page);
+                       memcpy_from_folio(tmp_bh->b_data, cn->bh->b_folio,
+                                       bh_offset(cn->bh), cn->bh->b_size);
+
                        mark_buffer_dirty(tmp_bh);
                        jindex++;
                        set_buffer_journal_dirty(cn->bh);
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 4ede47649a81..b61fa79cb7f5 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -171,7 +171,10 @@ static __always_inline int buffer_uptodate(const struct buffer_head *bh)
        return test_bit_acquire(BH_Uptodate, &bh->b_state);
 }

-#define bh_offset(bh)          ((unsigned long)(bh)->b_data & ~PAGE_MASK)
+static inline unsigned long bh_offset(const struct buffer_head *bh)
+{
+       return (unsigned long)(bh)->b_data & (page_size(bh->b_page) - 1);
+}

 /* If we *know* page->private refers to buffer_heads */
 #define page_buffers(page)                                     \
--
2.30.2

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Matthew Wilcox]

On Thu, Sep 07, 2023 at 07:05:38PM +0530, Ritesh Harjani wrote:
> Thanks Matthew for proposing the final changes using folio.
> (there were just some minor change required for fs/reiserfs/ for unused variables)
> Pasting the final patch below (you as the author with my Signed-off-by &
> Tested-by), which I have tested it on my system with "ext4/1k -g auto"

I'd rather split that patch up a bit -- I don't think the reiserfs
part fixes any actual problem.  I've pushed out
https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/bh-fixes

or git clone git://git.infradead.org/users/willy/pagecache.git

I credited you as the author on the second two since I just tidied up
your proposed fixes.

I've also checked ocfs2 as the other user of JBD2 and I don't see any
problems there.

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Ritesh Harjani]

Matthew Wilcox <willy@infradead.org> writes:

> On Thu, Sep 07, 2023 at 07:05:38PM +0530, Ritesh Harjani wrote:
>> Thanks Matthew for proposing the final changes using folio.
>> (there were just some minor change required for fs/reiserfs/ for unused variables)
>> Pasting the final patch below (you as the author with my Signed-off-by &
>> Tested-by), which I have tested it on my system with "ext4/1k -g auto"
>
> I'd rather split that patch up a bit -- I don't think the reiserfs
> part fixes any actual problem.  I've pushed out
> https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/bh-fixes
>
> or git clone git://git.infradead.org/users/willy/pagecache.git
>
> I credited you as the author on the second two since I just tidied up
> your proposed fixes.
>
> I've also checked ocfs2 as the other user of JBD2 and I don't see any
> problems there.

Thanks Matthew! :) 

-ritesh 

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Linux regression tracking (Thorsten Leemhuis)]

[TLDR: This mail in primarily relevant for Linux kernel regression
tracking. See link in footer if these mails annoy you.]

On 07.09.23 16:59, Ritesh Harjani (IBM) wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> 
>> On Thu, Sep 07, 2023 at 07:05:38PM +0530, Ritesh Harjani wrote:
>>> Thanks Matthew for proposing the final changes using folio.
>>> (there were just some minor change required for fs/reiserfs/ for unused variables)
>>> Pasting the final patch below (you as the author with my Signed-off-by &
>>> Tested-by), which I have tested it on my system with "ext4/1k -g auto"
>>
>> I'd rather split that patch up a bit -- I don't think the reiserfs
>> part fixes any actual problem.  I've pushed out
>> https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/bh-fixes
>>
>> or git clone git://git.infradead.org/users/willy/pagecache.git
>>
>> I credited you as the author on the second two since I just tidied up
>> your proposed fixes.
>>
>> I've also checked ocfs2 as the other user of JBD2 and I don't see any
>> problems there.
> 
> Thanks Matthew! :) 

#regzbot fix: jbd2: Remove page size assumptions
#regzbot ignore-activity

(fix can currently be found in
https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/bh-fixes
as
https://git.infradead.org/users/willy/pagecache.git/commit/fc0a6fa4a2c7b434665f087801a06c544b16f085
)

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: [fstests generic/388, 455, 475, 482 ...] Ext4 journal recovery test fails

[Theodore Ts'o]

On Sun, Sep 10, 2023 at 11:26:00AM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> 
> #regzbot fix: jbd2: Remove page size assumptions
> #regzbot ignore-activity
> 
> (fix can currently be found in
> https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/bh-fixes
> as
> https://git.infradead.org/users/willy/pagecache.git/commit/fc0a6fa4a2c7b434665f087801a06c544b16f085
> )

Per our discussion at last week's ext4 video chat, I've cherry-picked
the two fixes on the ext4 dev tree:

147d4a092e9a - jbd2: Remove page size assumptions (3 days ago)
f94cf2206b06 - buffer: Make bh_offset() work for compound pages (3 days ago)

I didn't take the reiserfs change, since this is for the ext4 git
tree, and as near as I can tell, it's more of a code cleanup rather
than an immediate fix.

Cheers,

						- Ted