From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Jan Kara <jack@suse.cz>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [git pull] vfs.git sysv pile
Date:   Wed, 08 Mar 2023 18:40:44 +0100
Message-ID: <2907412.VdNmn5OnKV@suse>
In-Reply-To: <ZAD6n+mH/P8LDcOw@ZenIV>
References: <Y/gugbqq858QXJBY@ZenIV> <9074146.CDJkKcVGEf@suse> <ZAD6n+mH/P8LDcOw@ZenIV>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

On gioved=EC 2 marzo 2023 20:35:59 CET Al Viro wrote:

[...]

> Frankly, ext2 patchset had been more along the lines of "here's what
> untangling the calling conventions in ext2 would probably look like" than
> anything else. If you are willing to test (and review) that sucker and it
> turns out to be OK, I'll be happy to slap your tested-by on those during
> rebase and feed them to Jan...

I git-clone(d) and built your "vfs" tree, branch #work.ext2, without and wi=
th=20
the following commits:

f5b399373756 ("ext2: use offset_in_page() instead of open-coding it as=20
subtraction")

c7248e221fb5 ("ext2_get_page(): saner type")

470e54a09898 ("ext2_put_page(): accept any pointer within the page")

15abcc147cf7 ("ext2_{set_link,delete_entry}(): don't bother with page_addr")

16a5ee2027b7 ("ext2_find_entry()/ext2_dotdot(): callers don't need page_add=
r=20
anymore")

Then I read the code and FWIW the five patches look good to me. I think the=
y=20
can work properly.=20

Therefore, if you want to, please feel free to add my "Reviewed-by" tag (OK=
, I=20
know that you don't need my reviews, since you are the one who taught me ho=
w=20
to write patches like yours for sysv and ufs :-)).

As a personal preference, in ext2_get_page() I'd move the two lines of code=
=20
from the "fail" label to the same 'if' block where you have the "goto fail;=
",=20
mainly because that label is only reachable from there. However, it does no=
t=20
matter at all because I'm only expressing my personal preference.

I ran `./check -g quick` without your patches in a QEMU/KVM x86_32 VM, 6GB=
=20
RAM, running a Kernel with HIGHMEM64GB enabled. I ran it three or four time=
s=20
because it kept on hanging at random tests' numbers.

I'm noticing the same pattern due to the oom killer kicking in several time=
s=20
to kill processes until xfstests its is dead.

[ 1171.795551] Out of memory: Killed process 1669 (xdg-desktop-por) total-v=
m:
105068kB, anon-rss:9792kB, file-rss:10972kB, shmem-rss:0kB, UID:1000 pgtabl=
es:
136kB oom_score_adj:200
[ 1172.339920] systemd invoked oom-killer: gfp_mask=3D0xcc0(GFP_KERNEL),=20
order=3D0, oom_score_adj=3D100
[ 1172.339927] CPU: 3 PID: 1413 Comm: systemd Tainted: G S      W   E     =
=20
6.3.0-rc1-x86-32-debug+ #1
[ 1172.339929] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS=20
rel-1.16.0-0-gd239552-rebuilt.opensuse.org 04/01/2014
[ 1172.339931] Call Trace:
[ 1172.339934]  dump_stack_lvl+0x92/0xd4
[ 1172.339939]  dump_stack+0xd/0x10
[ 1172.339941]  dump_header+0x42/0x454
[ 1172.339945]  ? ___ratelimit+0x6f/0x140
[ 1172.339948]  oom_kill_process+0xe9/0x244
[ 1172.339950]  out_of_memory+0xf6/0x424=20

I have not enough experience to understand why we get to that out-of-memory=
=20
condition, so that several processes get killed. I can send the whole decod=
ed=20
stack trace and other information to whoever can look at this issue to figu=
re=20
out how to fix this big issue. I can try to bisect this issue too, but I ne=
ed=20
time because of other commitments and a slow system for building the necess=
ary=20
kernels.

I want to stress that it does not depend on the above-mentioned patches. Ye=
s,=20
I'm running Al's "vfs" tree, #work.ext2 branch, but with one only patch bey=
ond=20
the merge with Linus' tree:

522dad1 ext2_rename(): set_link and delete_entry may fail=20

I have no means to test this tree. However, I think that I'd have the same=
=20
issue with Linus' tree too, unless this issue is due to the only commit not=
=20
yet there (I strongly doubt about this possibility).

Thanks,

=46abio