Fabio's "switch to kmap_local_page()" patchset (originally after the
ext2 counterpart, with a lot of cleaning up done to it; as the matter of
fact, ext2 side is in need of similar cleanups - calling conventions there
are bloody awful). Plus the equivalents of minix stuff...
The following changes since commit b7bfaa761d760e72a969d116517eaa12e404c262:
Linux 6.2-rc3 (2023-01-08 11:49:43 -0600)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.sysv
for you to fetch changes up to abb7c742397324f8676c5b622effdce911cd52e3:
sysv: fix handling of delete_entry and set_link failures (2023-01-19 23:24:42 -0500)
----------------------------------------------------------------
Al Viro (1):
sysv: fix handling of delete_entry and set_link failures
Christoph Hellwig (1):
sysv: don't flush page immediately for DIRSYNC directories
Fabio M. De Francesco (4):
fs/sysv: Use the offset_in_page() helper
fs/sysv: Change the signature of dir_get_page()
fs/sysv: Use dir_put_page() in sysv_rename()
fs/sysv: Replace kmap() with kmap_local_page()
fs/sysv/dir.c | 154 ++++++++++++++++++++++++++++++++------------------------
fs/sysv/namei.c | 42 ++++++++--------
fs/sysv/sysv.h | 3 +-
3 files changed, 111 insertions(+), 88 deletions(-)
The pull request you sent on Fri, 24 Feb 2023 03:26:57 +0000:
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.sysv
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/d6b9cf417c62601f26fa47f97d6c0681704bf0e3
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
On venerd? 24 febbraio 2023 04:26:57 CET Al Viro wrote:
> Fabio's "switch to kmap_local_page()" patchset (originally after the
> ext2 counterpart, with a lot of cleaning up done to it; as the matter of
> fact, ext2 side is in need of similar cleanups - calling conventions there
> are bloody awful).
If nobody else is already working on these cleanups in ext2 following your
suggestion, I'd be happy to work on this by the end of this week. I only need
a confirmation because I'd hate to duplicate someone else work.
> Plus the equivalents of minix stuff...
I don't know this other filesystem but I could take a look and see whether it
resembles somehow sysv and ext2 (if so, this work would be pretty simple too,
thanks to your kind suggestions when I worked on sysv and ufs).
I'm adding Jan to the Cc list to hear whether he is aware of anybody else
working on this changes for ext2. I'm waiting for a reply from you (@Al) or
Jan to avoid duplication (as said above).
Thanks,
Fabio
> The following changes since commit b7bfaa761d760e72a969d116517eaa12e404c262:
>
> Linux 6.2-rc3 (2023-01-08 11:49:43 -0600)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.sysv
>
> for you to fetch changes up to abb7c742397324f8676c5b622effdce911cd52e3:
>
> sysv: fix handling of delete_entry and set_link failures (2023-01-19
> 23:24:42 -0500)
>
> ----------------------------------------------------------------
> Al Viro (1):
> sysv: fix handling of delete_entry and set_link failures
>
> Christoph Hellwig (1):
> sysv: don't flush page immediately for DIRSYNC directories
>
> Fabio M. De Francesco (4):
> fs/sysv: Use the offset_in_page() helper
> fs/sysv: Change the signature of dir_get_page()
> fs/sysv: Use dir_put_page() in sysv_rename()
> fs/sysv: Replace kmap() with kmap_local_page()
>
> fs/sysv/dir.c | 154
> ++++++++++++++++++++++++++++++++------------------------ fs/sysv/namei.c |
> 42 ++++++++--------
> fs/sysv/sysv.h | 3 +-
> 3 files changed, 111 insertions(+), 88 deletions(-)
On Wed 01-03-23 12:20:56, Fabio M. De Francesco wrote:
> On venerd? 24 febbraio 2023 04:26:57 CET Al Viro wrote:
> > Fabio's "switch to kmap_local_page()" patchset (originally after the
> > ext2 counterpart, with a lot of cleaning up done to it; as the matter of
> > fact, ext2 side is in need of similar cleanups - calling conventions there
> > are bloody awful).
>
> If nobody else is already working on these cleanups in ext2 following your
> suggestion, I'd be happy to work on this by the end of this week. I only need
> a confirmation because I'd hate to duplicate someone else work.
>
> > Plus the equivalents of minix stuff...
>
> I don't know this other filesystem but I could take a look and see whether it
> resembles somehow sysv and ext2 (if so, this work would be pretty simple too,
> thanks to your kind suggestions when I worked on sysv and ufs).
>
> I'm adding Jan to the Cc list to hear whether he is aware of anybody else
> working on this changes for ext2. I'm waiting for a reply from you (@Al) or
> Jan to avoid duplication (as said above).
I'm not sure what exactly Al doesn't like about how ext2 handles pages and
mapping but if you have some cleanups in mind, sure go ahead. I don't have
any plans on working on that code in the near term.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On mercoled? 1 marzo 2023 14:00:18 CET Jan Kara wrote:
> On Wed 01-03-23 12:20:56, Fabio M. De Francesco wrote:
> > On venerd? 24 febbraio 2023 04:26:57 CET Al Viro wrote:
> > > Fabio's "switch to kmap_local_page()" patchset (originally after the
> > >
> > > ext2 counterpart, with a lot of cleaning up done to it; as the matter of
> > > fact, ext2 side is in need of similar cleanups - calling conventions
there
> > > are bloody awful).
> >
> > If nobody else is already working on these cleanups in ext2 following your
> > suggestion, I'd be happy to work on this by the end of this week. I only
> > need
> > a confirmation because I'd hate to duplicate someone else work.
> >
> > > Plus the equivalents of minix stuff...
> >
> > I don't know this other filesystem but I could take a look and see whether
> > it
> > resembles somehow sysv and ext2 (if so, this work would be pretty simple
> > too,
> > thanks to your kind suggestions when I worked on sysv and ufs).
> >
> > I'm adding Jan to the Cc list to hear whether he is aware of anybody else
> > working on this changes for ext2. I'm waiting for a reply from you (@Al)
or
> > Jan to avoid duplication (as said above).
>
> I'm not sure what exactly Al doesn't like about how ext2 handles pages and
> mapping but if you have some cleanups in mind, sure go ahead.
Hi Jan,
I might explain here and now what Al is referring to I'd prefer to show the
code :-)
In brief I had made the conversions of fs/ufs and fs/sysv from kmap() to
kmap_local_page() by porting what had been done for ext2. At that point Al
suggested a much cleaner and elegant approach.
Therefore, I threw away the port from ext2 and sent a series of 4 patches
according to a long list of suggestions that Al kindly provided to me.
Now he is asking for somebody doing the same changes in ext2 too.
Thanks for the immediate reply.
Fabio
> I don't have
> any plans on working on that code in the near term.
>
>
Honza
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
On Wed, Mar 01, 2023 at 02:00:18PM +0100, Jan Kara wrote:
> On Wed 01-03-23 12:20:56, Fabio M. De Francesco wrote:
> > On venerd? 24 febbraio 2023 04:26:57 CET Al Viro wrote:
> > > Fabio's "switch to kmap_local_page()" patchset (originally after the
> > > ext2 counterpart, with a lot of cleaning up done to it; as the matter of
> > > fact, ext2 side is in need of similar cleanups - calling conventions there
> > > are bloody awful).
> >
> > If nobody else is already working on these cleanups in ext2 following your
> > suggestion, I'd be happy to work on this by the end of this week. I only need
> > a confirmation because I'd hate to duplicate someone else work.
> >
> > > Plus the equivalents of minix stuff...
> >
> > I don't know this other filesystem but I could take a look and see whether it
> > resembles somehow sysv and ext2 (if so, this work would be pretty simple too,
> > thanks to your kind suggestions when I worked on sysv and ufs).
> >
> > I'm adding Jan to the Cc list to hear whether he is aware of anybody else
> > working on this changes for ext2. I'm waiting for a reply from you (@Al) or
> > Jan to avoid duplication (as said above).
>
> I'm not sure what exactly Al doesn't like about how ext2 handles pages and
> mapping but if you have some cleanups in mind, sure go ahead. I don't have
> any plans on working on that code in the near term.
I think I've pushed a demo patchset to vfs.git at some point back in January...
Yep - see #work.ext2 in there; completely untested, though.
On Wed 01-03-23 14:14:16, Al Viro wrote:
> On Wed, Mar 01, 2023 at 02:00:18PM +0100, Jan Kara wrote:
> > On Wed 01-03-23 12:20:56, Fabio M. De Francesco wrote:
> > > On venerd? 24 febbraio 2023 04:26:57 CET Al Viro wrote:
> > > > Fabio's "switch to kmap_local_page()" patchset (originally after the
> > > > ext2 counterpart, with a lot of cleaning up done to it; as the matter of
> > > > fact, ext2 side is in need of similar cleanups - calling conventions there
> > > > are bloody awful).
> > >
> > > If nobody else is already working on these cleanups in ext2 following your
> > > suggestion, I'd be happy to work on this by the end of this week. I only need
> > > a confirmation because I'd hate to duplicate someone else work.
> > >
> > > > Plus the equivalents of minix stuff...
> > >
> > > I don't know this other filesystem but I could take a look and see whether it
> > > resembles somehow sysv and ext2 (if so, this work would be pretty simple too,
> > > thanks to your kind suggestions when I worked on sysv and ufs).
> > >
> > > I'm adding Jan to the Cc list to hear whether he is aware of anybody else
> > > working on this changes for ext2. I'm waiting for a reply from you (@Al) or
> > > Jan to avoid duplication (as said above).
> >
> > I'm not sure what exactly Al doesn't like about how ext2 handles pages and
> > mapping but if you have some cleanups in mind, sure go ahead. I don't have
> > any plans on working on that code in the near term.
>
> I think I've pushed a demo patchset to vfs.git at some point back in January...
> Yep - see #work.ext2 in there; completely untested, though.
OK, I think your changes to ext2_rename() in PATCH 1 leak a reference and
mapping of old_page but otherwise I like the patches. So Fabio, if you can
pick them up and push this to completion, it would be nice. Thanks!
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On gioved? 2 marzo 2023 10:59:31 CET Jan Kara wrote:
> On Wed 01-03-23 14:14:16, Al Viro wrote:
> > On Wed, Mar 01, 2023 at 02:00:18PM +0100, Jan Kara wrote:
> > > On Wed 01-03-23 12:20:56, Fabio M. De Francesco wrote:
> > > > On venerd? 24 febbraio 2023 04:26:57 CET Al Viro wrote:
> > > > > Fabio's "switch to kmap_local_page()" patchset (originally after the
> > > > >
> > > > > ext2 counterpart, with a lot of cleaning up done to it; as the
matter
> > > > > of
> > > > > fact, ext2 side is in need of similar cleanups - calling conventions
> > > > > there
> > > > > are bloody awful).
> > > >
> > > > If nobody else is already working on these cleanups in ext2 following
> > > > your
> > > > suggestion, I'd be happy to work on this by the end of this week. I
only
> > > > need
> > > > a confirmation because I'd hate to duplicate someone else work.
> > > >
> > > > > Plus the equivalents of minix stuff...
> > > >
> > > > I don't know this other filesystem but I could take a look and see
> > > > whether it
> > > > resembles somehow sysv and ext2 (if so, this work would be pretty
simple
> > > > too,
> > > > thanks to your kind suggestions when I worked on sysv and ufs).
> > > >
> > > > I'm adding Jan to the Cc list to hear whether he is aware of anybody
> > > > else
> > > > working on this changes for ext2. I'm waiting for a reply from you
(@Al)
> > > > or
> > > > Jan to avoid duplication (as said above).
> > >
> > > I'm not sure what exactly Al doesn't like about how ext2 handles pages
and
> > > mapping but if you have some cleanups in mind, sure go ahead. I don't
have
> > > any plans on working on that code in the near term.
> >
> > I think I've pushed a demo patchset to vfs.git at some point back in
> > January... Yep - see #work.ext2 in there; completely untested, though.
>
> OK, I think your changes to ext2_rename() in PATCH 1 leak a reference and
> mapping of old_page but otherwise I like the patches. So Fabio, if you can
> pick them up and push this to completion, it would be nice. Thanks!
>
@Jan,
I was sure you would have liked them :-)
I'm happy to pick them up and push them to completion.
But... when yesterday Al showed his demo patchset I probably interpreted his
reply the wrong way and thought that since he spent time for the demo he
wanted to put this to completion on his own.
Now I see that you are interpreting his message as an invite to use them to
shorten the time...
Furthermore I'm not sure about how I should credit him. Should I merely add a
"Suggested-by:" tag or more consistent "Co-authored-by: Al Viro <...>"? Since
he did so much I'd rather the second but I need his permission.
@Al,
Can I really proceed with *your* work? What should the better suited tag be to
credit you for the patches?
If you can reply today or at least by Friday, I'll pick your demo patchset,
put it to completion, make the patches and test them with (x)fstests on a
QEMU/KVM x86_32 bit VM, with 6GB RAM, running an HIGHMEM64GB enabled kernel.
Thanks,
Fabio
On Thu, Mar 02, 2023 at 10:59:31AM +0100, Jan Kara wrote:
> OK, I think your changes to ext2_rename() in PATCH 1 leak a reference and
> mapping of old_page
In which case? ext2_delete_entry() failing?
- ext2_delete_entry(old_de, old_page, old_page_addr);
+ err = ext2_delete_entry(old_de, old_page, old_page_addr);
+ if (err)
+ goto out_dir;
and on out_dir: we have
out_dir:
if (dir_de)
ext2_put_page(dir_page, dir_page_addr);
out_old:
ext2_put_page(old_page, old_page_addr);
out:
return err;
How is the old_page leaked here?
On Thu, Mar 02, 2023 at 12:31:46PM +0100, Fabio M. De Francesco wrote:
> But... when yesterday Al showed his demo patchset I probably interpreted his
> reply the wrong way and thought that since he spent time for the demo he
> wanted to put this to completion on his own.
>
> Now I see that you are interpreting his message as an invite to use them to
> shorten the time...
>
> Furthermore I'm not sure about how I should credit him. Should I merely add a
> "Suggested-by:" tag or more consistent "Co-authored-by: Al Viro <...>"? Since
> he did so much I'd rather the second but I need his permission.
What, for sysv part? It's already in mainline; for minixfs and ufs, if you want
to do those - whatever you want, I'd probably go for "modelled after sysv
series in 6.2" - "Suggested-by" in those would suffice...
> @Al,
>
> Can I really proceed with *your* work? What should the better suited tag be to
> credit you for the patches?
>
> If you can reply today or at least by Friday, I'll pick your demo patchset,
> put it to completion, make the patches and test them with (x)fstests on a
> QEMU/KVM x86_32 bit VM, with 6GB RAM, running an HIGHMEM64GB enabled kernel.
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...
On Thu, Mar 02, 2023 at 07:35:59PM +0000, Al Viro wrote:
> On Thu, Mar 02, 2023 at 12:31:46PM +0100, Fabio M. De Francesco wrote:
>
> > But... when yesterday Al showed his demo patchset I probably interpreted his
> > reply the wrong way and thought that since he spent time for the demo he
> > wanted to put this to completion on his own.
> >
> > Now I see that you are interpreting his message as an invite to use them to
> > shorten the time...
> >
> > Furthermore I'm not sure about how I should credit him. Should I merely add a
> > "Suggested-by:" tag or more consistent "Co-authored-by: Al Viro <...>"? Since
> > he did so much I'd rather the second but I need his permission.
>
> What, for sysv part? It's already in mainline; for minixfs and ufs, if you want
> to do those - whatever you want, I'd probably go for "modelled after sysv
> series in 6.2" - "Suggested-by" in those would suffice...
>
> > @Al,
> >
> > Can I really proceed with *your* work? What should the better suited tag be to
> > credit you for the patches?
> >
> > If you can reply today or at least by Friday, I'll pick your demo patchset,
> > put it to completion, make the patches and test them with (x)fstests on a
> > QEMU/KVM x86_32 bit VM, with 6GB RAM, running an HIGHMEM64GB enabled kernel.
>
> 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...
PS: now we can actually turn
kunmap_local((void *)((unsigned long)page_addr & PAGE_MASK));
into
kunmap_local(page_addr);
provided that commit doing that includes something along the lines of
Do-Not-Backport-Without: 88d7b12068b9 "highmem: round down the address passed to kunmap_flush_on_unmap()"
in commit message.
On gioved? 2 marzo 2023 20:35:59 CET Al Viro wrote:
> On Thu, Mar 02, 2023 at 12:31:46PM +0100, Fabio M. De Francesco wrote:
> > But... when yesterday Al showed his demo patchset I probably interpreted
his
> > reply the wrong way and thought that since he spent time for the demo he
> > wanted to put this to completion on his own.
> >
> > Now I see that you are interpreting his message as an invite to use them
to
> > shorten the time...
> >
> > Furthermore I'm not sure about how I should credit him. Should I merely
add
> > a
> > "Suggested-by:" tag or more consistent "Co-authored-by: Al Viro <...>"?
> > Since
> > he did so much I'd rather the second but I need his permission.
>
> What, for sysv part? It's already in mainline;
Yes, I know this. In fact this thread started with the pull request you sent
to Linus on Feb 23. My patches to fs/sysv already credited you with the
"Suggested-by:" tag.
Sorry if I have not been clear about what I was talking about.
> for minix and ufs,
My series of patches for fs/ufs (again all with the "Suggested-by: Al Viro
<...>" tags - it's only missing in the cover letter) are at the following
address since Dec 29, 2022. I don't know why they haven't yet applied to the
relevant tree:
https://lore.kernel.org/lkml/[email protected]/
As far as fs/minix is regarded I submitted nothing for it. I'm not sure about
who wants to work on the patches for that filesystem.
> if you
> want to do those - whatever you want, I'd probably go for "modeled after
> sysv series in 6.2" - "Suggested-by" in those would suffice...
I know nothing about how fs/minix is designed and I don't yet know whether or
not I can easily model the patches to it after sysv and ufs series. I'll take
a look in the next days.
> > @Al,
> >
> > Can I really proceed with *your* work? What should the better suited tag
be
> > to credit you for the patches?
> >
> > If you can reply today or at least by Friday, I'll pick your demo
patchset,
> > put it to completion, make the patches and test them with (x)fstests on a
> > QEMU/KVM x86_32 bit VM, with 6GB RAM, running an HIGHMEM64GB enabled
kernel.
>
> 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...
Sorry for the confusion about ext2. I think I have not been clear about my
intentions. Please let me summarize:
1) You sent the pull request for sysv. In that email to Linus you wrote
"Fabio's "switch to kmap_local_page()" patchset (originally after the
ext2 counterpart, with a lot of cleaning up done to it; as the matter of
fact, ext2 side is in need of similar cleanups - calling conventions there
are bloody awful). Plus the equivalents of minix stuff..."
2) I replied by asking whether someone else were already working on ext2 as
you suggested above. I asked for that information because I thought I could do
the work modeling after sysv and ufs.
3) You wrote about a "demo patchset" somewhere in one of your trees.
4) Jan replied that he likes your "demo patchset" (I haven't yet taken a look
at those because I supposed they were modeled after the suggestions you
provided to me for sysv and ufs, so I thought I have no reasons to take a look
at them) and asked me to "pick your demo patches and put them to completion".
Now I'm confused about what you want to be done with your "demo patchset"
because I don't know what you mean by "demo" and why you showed you have that
patchset.
I mean... do you want them only tested and reviewed? Any other task to be done
on them?
Thanks,
Fabio
On gioved? 2 marzo 2023 23:35:41 CET Al Viro wrote:
> On Thu, Mar 02, 2023 at 07:35:59PM +0000, Al Viro wrote:
> > On Thu, Mar 02, 2023 at 12:31:46PM +0100, Fabio M. De Francesco wrote:
> > > But... when yesterday Al showed his demo patchset I probably interpreted
> > > his
> > > reply the wrong way and thought that since he spent time for the demo he
> > > wanted to put this to completion on his own.
> > >
> > > Now I see that you are interpreting his message as an invite to use them
> > > to
> > > shorten the time...
> > >
> > > Furthermore I'm not sure about how I should credit him. Should I merely
> > > add a
> > > "Suggested-by:" tag or more consistent "Co-authored-by: Al Viro <...>"?
> > > Since
> > > he did so much I'd rather the second but I need his permission.
> >
> > What, for sysv part? It's already in mainline; for minixfs and ufs, if
you
> > want to do those - whatever you want, I'd probably go for "modeled after
> > sysv series in 6.2" - "Suggested-by" in those would suffice...
> >
> > > @Al,
> > >
> > > Can I really proceed with *your* work? What should the better suited tag
> > > be to credit you for the patches?
> > >
> > > If you can reply today or at least by Friday, I'll pick your demo
> > > patchset,
> > > put it to completion, make the patches and test them with (x)fstests on
a
> > > QEMU/KVM x86_32 bit VM, with 6GB RAM, running an HIGHMEM64GB enabled
> > > kernel.
> >
> > 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...
>
> PS: now we can actually turn
> kunmap_local((void *)((unsigned long)page_addr & PAGE_MASK));
> into
> kunmap_local(page_addr);
>
> provided that commit doing that includes something along the lines of
>
> Do-Not-Backport-Without: 88d7b12068b9 "highmem: round down the address
passed
> to kunmap_flush_on_unmap()"
>
> in commit message.
I'll do it for fs/sysv.
Instead there is no need to change anything in my series for fs/ufs (it was
made as if we already had 88d7b12068b9 "highmem: round down the address passed
to kunmap_flush_on_unmap()" in place).
Thanks,
Fabio
On Thu 02-03-23 19:26:36, Al Viro wrote:
> On Thu, Mar 02, 2023 at 10:59:31AM +0100, Jan Kara wrote:
> > OK, I think your changes to ext2_rename() in PATCH 1 leak a reference and
> > mapping of old_page
>
> In which case? ext2_delete_entry() failing?
>
> - ext2_delete_entry(old_de, old_page, old_page_addr);
> + err = ext2_delete_entry(old_de, old_page, old_page_addr);
> + if (err)
> + goto out_dir;
>
> and on out_dir: we have
> out_dir:
> if (dir_de)
> ext2_put_page(dir_page, dir_page_addr);
> out_old:
> ext2_put_page(old_page, old_page_addr);
> out:
> return err;
>
> How is the old_page leaked here?
Ah, sorry, I got confused by the diff. Now that I'm looking into the full
source, it's all fine. Sorry for the noise.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On gioved? 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 with
the following commits:
f5b399373756 ("ext2: use offset_in_page() instead of open-coding it as
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_addr
anymore")
Then I read the code and FWIW the five patches look good to me. I think they
can work properly.
Therefore, if you want to, please feel free to add my "Reviewed-by" tag (OK, I
know that you don't need my reviews, since you are the one who taught me how
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
from the "fail" label to the same 'if' block where you have the "goto fail;",
mainly because that label is only reachable from there. However, it does not
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
RAM, running a Kernel with HIGHMEM64GB enabled. I ran it three or four times
because it kept on hanging at random tests' numbers.
I'm noticing the same pattern due to the oom killer kicking in several times
to kill processes until xfstests its is dead.
[ 1171.795551] Out of memory: Killed process 1669 (xdg-desktop-por) total-vm:
105068kB, anon-rss:9792kB, file-rss:10972kB, shmem-rss:0kB, UID:1000 pgtables:
136kB oom_score_adj:200
[ 1172.339920] systemd invoked oom-killer: gfp_mask=0xcc0(GFP_KERNEL),
order=0, oom_score_adj=100
[ 1172.339927] CPU: 3 PID: 1413 Comm: systemd Tainted: G S W E
6.3.0-rc1-x86-32-debug+ #1
[ 1172.339929] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
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
I have not enough experience to understand why we get to that out-of-memory
condition, so that several processes get killed. I can send the whole decoded
stack trace and other information to whoever can look at this issue to figure
out how to fix this big issue. I can try to bisect this issue too, but I need
time because of other commitments and a slow system for building the necessary
kernels.
I want to stress that it does not depend on the above-mentioned patches. Yes,
I'm running Al's "vfs" tree, #work.ext2 branch, but with one only patch beyond
the merge with Linus' tree:
522dad1 ext2_rename(): set_link and delete_entry may fail
I have no means to test this tree. However, I think that I'd have the same
issue with Linus' tree too, unless this issue is due to the only commit not
yet there (I strongly doubt about this possibility).
Thanks,
Fabio
On mercoled? 8 marzo 2023 18:40:44 CET Fabio M. De Francesco wrote:
> On gioved? 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
with
> the following commits:
>
> f5b399373756 ("ext2: use offset_in_page() instead of open-coding it as
> 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_addr
> anymore")
>
> Then I read the code and FWIW the five patches look good to me. I think they
> can work properly.
>
> Therefore, if you want to, please feel free to add my "Reviewed-by" tag (OK,
I
> know that you don't need my reviews, since you are the one who taught me how
> 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
> from the "fail" label to the same 'if' block where you have the "goto
fail;",
> mainly because that label is only reachable from there. However, it does not
> 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
> RAM, running a Kernel with HIGHMEM64GB enabled. I ran it three or four times
> because it kept on hanging at random tests' numbers.
>
> I'm noticing the same pattern due to the oom killer kicking in several times
> to kill processes until xfstests its is dead.
>
> [ 1171.795551] Out of memory: Killed process 1669 (xdg-desktop-por) total-
vm:
> 105068kB, anon-rss:9792kB, file-rss:10972kB, shmem-rss:0kB, UID:1000
pgtables:
> 136kB oom_score_adj:200
> [ 1172.339920] systemd invoked oom-killer: gfp_mask=0xcc0(GFP_KERNEL),
> order=0, oom_score_adj=100
> [ 1172.339927] CPU: 3 PID: 1413 Comm: systemd Tainted: G S W E
> 6.3.0-rc1-x86-32-debug+ #1
> [ 1172.339929] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
> 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
>
> I have not enough experience to understand why we get to that out-of-memory
> condition, so that several processes get killed. I can send the whole
decoded
> stack trace and other information to whoever can look at this issue to
figure
> out how to fix this big issue. I can try to bisect this issue too, but I
need
> time because of other commitments and a slow system for building the
necessary
> kernels.
>
> I want to stress that it does not depend on the above-mentioned patches.
Yes,
> I'm running Al's "vfs" tree, #work.ext2 branch, but with one only patch
beyond
> the merge with Linus' tree:
>
> 522dad1 ext2_rename(): set_link and delete_entry may fail
>
> I have no means to test this tree. However, I think that I'd have the same
> issue with Linus' tree too, unless this issue is due to the only commit not
> yet there (I strongly doubt about this possibility).
>
> Thanks,
>
> Fabio
I want to confirm that running xfstests on the most recent SUSE Kernel doesn't
trigger the OOM Killer. It only fails 16 of 597 tests. I suppose that those 16
failures are expected to happen.
The kernel provided by openSUSE Tumbleweed is...
uname -a
Linux tweed32 6.2.1-1-pae #1 SMP PREEMPT_DYNAMIC Mon Feb 27 11:39:51 UTC 2023
(69e0e95) i686 athlon i386 GNU/Linux
I'll try a bisection as soon as possible.
Fabio
On mercoled? 1 marzo 2023 15:14:16 CET Al Viro wrote:
> On Wed, Mar 01, 2023 at 02:00:18PM +0100, Jan Kara wrote:
> > On Wed 01-03-23 12:20:56, Fabio M. De Francesco wrote:
> > > On venerd? 24 febbraio 2023 04:26:57 CET Al Viro wrote:
> > > > Fabio's "switch to kmap_local_page()" patchset (originally after the
> > > >
> > > > ext2 counterpart, with a lot of cleaning up done to it; as the matter
of
> > > > fact, ext2 side is in need of similar cleanups - calling conventions
> > > > there
> > > > are bloody awful).
> > >
[snip]
>
> I think I've pushed a demo patchset to vfs.git at some point back in
> January... Yep - see #work.ext2 in there; completely untested, though.
The following commits from the VFS tree, #work.ext2 look good to me.
f5b399373756 ("ext2: use offset_in_page() instead of open-coding it as
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_addr
anymore")
Reviewed-by: Fabio M. De Francesco <[email protected]>
I could only read the code but I could not test it in the same QEMU/KVM x86_32
VM where I test all my HIGHMEM related work.
Btrfs as well as all the other filesystems I converted to kmap_local_page()
don't make the processes in the VM to crash, whereas the xfstests on ext2
trigger the OOM killer at random tests (only sometimes they exit gracefully).
FYI, I tried to run the tests with 6GB of RAM, booting a kernel with
HIGHMEM64GB enabled. I cannot add my "Tested-by" tag.
Fabio
On Wed 15-03-23 19:08:57, Fabio M. De Francesco wrote:
> On mercoled? 1 marzo 2023 15:14:16 CET Al Viro wrote:
> > On Wed, Mar 01, 2023 at 02:00:18PM +0100, Jan Kara wrote:
> > > On Wed 01-03-23 12:20:56, Fabio M. De Francesco wrote:
> > > > On venerd? 24 febbraio 2023 04:26:57 CET Al Viro wrote:
> > > > > Fabio's "switch to kmap_local_page()" patchset (originally after the
> > > > >
> > > > > ext2 counterpart, with a lot of cleaning up done to it; as the matter
> of
> > > > > fact, ext2 side is in need of similar cleanups - calling conventions
> > > > > there
> > > > > are bloody awful).
> > > >
>
> [snip]
>
> >
> > I think I've pushed a demo patchset to vfs.git at some point back in
> > January... Yep - see #work.ext2 in there; completely untested, though.
>
> The following commits from the VFS tree, #work.ext2 look good to me.
>
> f5b399373756 ("ext2: use offset_in_page() instead of open-coding it as
> 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_addr
> anymore")
>
> Reviewed-by: Fabio M. De Francesco <[email protected]>
Thanks!
> I could only read the code but I could not test it in the same QEMU/KVM x86_32
> VM where I test all my HIGHMEM related work.
>
> Btrfs as well as all the other filesystems I converted to kmap_local_page()
> don't make the processes in the VM to crash, whereas the xfstests on ext2
> trigger the OOM killer at random tests (only sometimes they exit gracefully).
>
> FYI, I tried to run the tests with 6GB of RAM, booting a kernel with
> HIGHMEM64GB enabled. I cannot add my "Tested-by" tag.
Hum, interesting. Reading your previous emails this didn't seem to happen
before applying this series, did it?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On gioved? 16 marzo 2023 10:00:35 CET Jan Kara wrote:
> On Wed 15-03-23 19:08:57, Fabio M. De Francesco wrote:
> > On mercoled? 1 marzo 2023 15:14:16 CET Al Viro wrote:
> > > On Wed, Mar 01, 2023 at 02:00:18PM +0100, Jan Kara wrote:
> > > > On Wed 01-03-23 12:20:56, Fabio M. De Francesco wrote:
> > > > > On venerd? 24 febbraio 2023 04:26:57 CET Al Viro wrote:
> > > > > > Fabio's "switch to kmap_local_page()" patchset (originally
after
> > > > > > the
> > > > > >
> > > > > > ext2 counterpart, with a lot of cleaning up done to it; as the
> > > > > > matter
> >
> > of
> >
> > > > > > fact, ext2 side is in need of similar cleanups - calling
conventions
> > > > > > there
> > > > > > are bloody awful).
> >
> > [snip]
> >
> > > I think I've pushed a demo patchset to vfs.git at some point back in
> > > January... Yep - see #work.ext2 in there; completely untested, though.
> >
> > The following commits from the VFS tree, #work.ext2 look good to me.
> >
> > f5b399373756 ("ext2: use offset_in_page() instead of open-coding it as
> > 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_addr
> > anymore")
> >
> > Reviewed-by: Fabio M. De Francesco <[email protected]>
>
> Thanks!
>
> > I could only read the code but I could not test it in the same QEMU/KVM
> > x86_32 VM where I test all my HIGHMEM related work.
> >
> > Btrfs as well as all the other filesystems I converted to
kmap_local_page()
> > don't make the processes in the VM to crash, whereas the xfstests on ext2
> > trigger the OOM killer at random tests (only sometimes they exit
> > gracefully).
> >
> > FYI, I tried to run the tests with 6GB of RAM, booting a kernel with
> > HIGHMEM64GB enabled. I cannot add my "Tested-by" tag.
>
> Hum, interesting. Reading your previous emails this didn't seem to happen
> before applying this series, did it?
>
I wrote too many messages but was probably not able to explain the facts
properly. Please let me summarize...
1) When testing ext2 with "./check -g quick" in a QEMU/KVM x86_32 VM, 6GB RAM,
booting a Vanilla kernel 6.3.0-rc1 with HIGHMEM64GB enabled, the OOM Killer
kicks in at random tests _with_ and _without_ Al's patches.
2) The only case which does never trigger the OOM Killer is running the tests
on ext2 formatted filesystems in loop disks with the stock openSUSE kernel
which is the 6.2.1-1-pae.
3) The same "./check -g quick" on 6.3.0-rc1 runs always to completion with
other filesystems. I ran xfstests several times on Btrfs and I had no
problems.
4) I cannot git-bisect this issue with ext2 because I cannot trust the results
on any particular Kernel version. I mean that I cannot mark any specific
version neither "good" or "bad" because it happens that the same "good"
version instead make xfstests crash at the next run.
My conclusion is that we probably have some kind of race that makes the random
tests crash at random runs of random Kernel versions between (at least) SUSE
6.2.1 and Vanilla current.
But it may be very well the case that I'm doing something stupid (e.g., with
QEMU configuration or setup_disks or I can't imagine whatever else) and that
I'm unable to see where I make mistakes. After all, I'm still a newcomer with
little experience :-)
Therefore, I'd suggest that someone else try to test ext2 in an x86_32 VM.
However, I'm 99.5% sure that Al's patches are good by the mere inspection of
his code.
I hope that this summary contains everything that may help.
However, I remain available to provide any further information and to give my
contribution if you ask me for specific tasks.
For my part I have no idea how to investigate what is happening. In these
months I have run the VM hundreds of times on the most disparate filesystems
to test my conversions to kmap_local_page() and I have never seen anything
like this happen.
Thanks,
Fabio
>
Honza
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
On gioved? 16 marzo 2023 11:30:21 CET Fabio M. De Francesco wrote:
> On gioved? 16 marzo 2023 10:00:35 CET Jan Kara wrote:
> > On Wed 15-03-23 19:08:57, Fabio M. De Francesco wrote:
> > > On mercoled? 1 marzo 2023 15:14:16 CET Al Viro wrote:
> > > > On Wed, Mar 01, 2023 at 02:00:18PM +0100, Jan Kara wrote:
> > > > > On Wed 01-03-23 12:20:56, Fabio M. De Francesco wrote:
> > > > > > On venerd? 24 febbraio 2023 04:26:57 CET Al Viro wrote:
> > > > > > > Fabio's "switch to kmap_local_page()" patchset (originally
>
> after
>
> > > > > > > the
> > > > > > >
> > > > > > > ext2 counterpart, with a lot of cleaning up done to it; as the
> > > > > > > matter
> > >
> > > of
> > >
> > > > > > > fact, ext2 side is in need of similar cleanups - calling
>
> conventions
>
> > > > > > > there
> > > > > > > are bloody awful).
> > >
> > > [snip]
> > >
> > > > I think I've pushed a demo patchset to vfs.git at some point back in
> > > > January... Yep - see #work.ext2 in there; completely untested, though.
> > >
> > > The following commits from the VFS tree, #work.ext2 look good to me.
> > >
> > > f5b399373756 ("ext2: use offset_in_page() instead of open-coding it as
> > > 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_addr
>
> > > anymore")
> > >
> > > Reviewed-by: Fabio M. De Francesco <[email protected]>
> >
> > Thanks!
> >
> > > I could only read the code but I could not test it in the same QEMU/KVM
> > > x86_32 VM where I test all my HIGHMEM related work.
> > >
> > > Btrfs as well as all the other filesystems I converted to
>
> kmap_local_page()
>
> > > don't make the processes in the VM to crash, whereas the xfstests on
ext2
> > > trigger the OOM killer at random tests (only sometimes they exit
> > > gracefully).
> > >
> > > FYI, I tried to run the tests with 6GB of RAM, booting a kernel with
> > > HIGHMEM64GB enabled. I cannot add my "Tested-by" tag.
> >
> > Hum, interesting. Reading your previous emails this didn't seem to happen
> > before applying this series, did it?
>
> I wrote too many messages but was probably not able to explain the facts
> properly. Please let me summarize...
>
> 1) When testing ext2 with "./check -g quick" in a QEMU/KVM x86_32 VM, 6GB
RAM,
> booting a Vanilla kernel 6.3.0-rc1 with HIGHMEM64GB enabled, the OOM Killer
> kicks in at random tests _with_ and _without_ Al's patches.
>
> 2) The only case which does never trigger the OOM Killer is running the
tests
> on ext2 formatted filesystems in loop disks with the stock openSUSE kernel
> which is the 6.2.1-1-pae.
>
> 3) The same "./check -g quick" on 6.3.0-rc1 runs always to completion with
> other filesystems. I ran xfstests several times on Btrfs and I had no
> problems.
>
> 4) I cannot git-bisect this issue with ext2 because I cannot trust the
results
> on any particular Kernel version. I mean that I cannot mark any specific
> version neither "good" or "bad" because it happens that the same "good"
> version instead make xfstests crash at the next run.
>
> My conclusion is that we probably have some kind of race that makes the
random
> tests crash at random runs of random Kernel versions between (at least) SUSE
> 6.2.1 and Vanilla current.
>
> But it may be very well the case that I'm doing something stupid (e.g., with
> QEMU configuration or setup_disks or I can't imagine whatever else) and that
> I'm unable to see where I make mistakes. After all, I'm still a newcomer
with
> little experience :-)
>
> Therefore, I'd suggest that someone else try to test ext2 in an x86_32 VM.
> However, I'm 99.5% sure that Al's patches are good by the mere inspection of
> his code.
>
> I hope that this summary contains everything that may help.
>
> However, I remain available to provide any further information and to give
my
> contribution if you ask me for specific tasks.
>
> For my part I have no idea how to investigate what is happening. In these
> months I have run the VM hundreds of times on the most disparate filesystems
> to test my conversions to kmap_local_page() and I have never seen anything
> like this happen.
>
> Thanks,
>
> Fabio
>
>
> Honza
>
> > --
> > Jan Kara <[email protected]>
> > SUSE Labs, CR
I can't yet figure out which conditions lead to trigger the OOM Killer to kill
the XFCE Desktop Environment, and the xfstests (which I usually run into the
latter). After all, reserving 6GB of main memory to a QEMU/KVM x86_32 VM had
always been more than adequate.
So, I thought I'd better ignore that 6GB for a 32 bit architecture are a
notable amount of RAM and squeezed some more from the host until I went to
reserve 8GB. I know that this is not what who is able to find out what
consumes so much main memory would do, but wanted to get the output from the
tests, one way or the other... :-(
OK, I could finally run my tests to completion and had no crashes at all. I
ran "./check -g quick" on one "test" + three "scratch" loop devices formatted
with "mkfs.ext2 -c". I ran three times _with_ and then three times _without_
Al's following patches cloned from his vfs tree, #work.ext2 branch:
f5b399373756 ("ext2: use offset_in_page() instead of open-coding it as
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
All the six tests were no longer killed by the Kernel :-)
I got 144 failures on 597 tests, regardless of the above listed patches.
My final conclusion is that these patches don't introduce regressions. I see
several tests that produce memory leaks but, I want to stress it again, the
failing tests are always the same with and without the patches.
therefore, I think that now I can safely add my tag to all five patches listed
above...
Tested-by: Fabio M. De Francesco <[email protected]>
Regards,
Fabio
On Mon 20-03-23 12:18:38, Fabio M. De Francesco wrote:
> On gioved? 16 marzo 2023 11:30:21 CET Fabio M. De Francesco wrote:
> > On gioved? 16 marzo 2023 10:00:35 CET Jan Kara wrote:
> > > On Wed 15-03-23 19:08:57, Fabio M. De Francesco wrote:
> > > > On mercoled? 1 marzo 2023 15:14:16 CET Al Viro wrote:
> > > > > On Wed, Mar 01, 2023 at 02:00:18PM +0100, Jan Kara wrote:
> > > > > > On Wed 01-03-23 12:20:56, Fabio M. De Francesco wrote:
> > > > > > > On venerd? 24 febbraio 2023 04:26:57 CET Al Viro wrote:
> > > > > > > > Fabio's "switch to kmap_local_page()" patchset (originally
> >
> > after
> >
> > > > > > > > the
> > > > > > > >
> > > > > > > > ext2 counterpart, with a lot of cleaning up done to it; as the
> > > > > > > > matter
> > > >
> > > > of
> > > >
> > > > > > > > fact, ext2 side is in need of similar cleanups - calling
> >
> > conventions
> >
> > > > > > > > there
> > > > > > > > are bloody awful).
> > > >
> > > > [snip]
> > > >
> > > > > I think I've pushed a demo patchset to vfs.git at some point back in
> > > > > January... Yep - see #work.ext2 in there; completely untested, though.
> > > >
> > > > The following commits from the VFS tree, #work.ext2 look good to me.
> > > >
> > > > f5b399373756 ("ext2: use offset_in_page() instead of open-coding it as
> > > > 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_addr
> >
> > > > anymore")
> > > >
> > > > Reviewed-by: Fabio M. De Francesco <[email protected]>
> > >
> > > Thanks!
> > >
> > > > I could only read the code but I could not test it in the same QEMU/KVM
> > > > x86_32 VM where I test all my HIGHMEM related work.
> > > >
> > > > Btrfs as well as all the other filesystems I converted to
> >
> > kmap_local_page()
> >
> > > > don't make the processes in the VM to crash, whereas the xfstests on
> ext2
> > > > trigger the OOM killer at random tests (only sometimes they exit
> > > > gracefully).
> > > >
> > > > FYI, I tried to run the tests with 6GB of RAM, booting a kernel with
> > > > HIGHMEM64GB enabled. I cannot add my "Tested-by" tag.
> > >
> > > Hum, interesting. Reading your previous emails this didn't seem to happen
> > > before applying this series, did it?
> >
> > I wrote too many messages but was probably not able to explain the facts
> > properly. Please let me summarize...
> >
> > 1) When testing ext2 with "./check -g quick" in a QEMU/KVM x86_32 VM, 6GB
> RAM,
> > booting a Vanilla kernel 6.3.0-rc1 with HIGHMEM64GB enabled, the OOM Killer
> > kicks in at random tests _with_ and _without_ Al's patches.
> >
> > 2) The only case which does never trigger the OOM Killer is running the
> tests
> > on ext2 formatted filesystems in loop disks with the stock openSUSE kernel
> > which is the 6.2.1-1-pae.
> >
> > 3) The same "./check -g quick" on 6.3.0-rc1 runs always to completion with
> > other filesystems. I ran xfstests several times on Btrfs and I had no
> > problems.
> >
> > 4) I cannot git-bisect this issue with ext2 because I cannot trust the
> results
> > on any particular Kernel version. I mean that I cannot mark any specific
> > version neither "good" or "bad" because it happens that the same "good"
> > version instead make xfstests crash at the next run.
> >
> > My conclusion is that we probably have some kind of race that makes the
> random
> > tests crash at random runs of random Kernel versions between (at least) SUSE
> > 6.2.1 and Vanilla current.
> >
> > But it may be very well the case that I'm doing something stupid (e.g., with
> > QEMU configuration or setup_disks or I can't imagine whatever else) and that
> > I'm unable to see where I make mistakes. After all, I'm still a newcomer
> with
> > little experience :-)
> >
> > Therefore, I'd suggest that someone else try to test ext2 in an x86_32 VM.
> > However, I'm 99.5% sure that Al's patches are good by the mere inspection of
> > his code.
> >
> > I hope that this summary contains everything that may help.
> >
> > However, I remain available to provide any further information and to give
> my
> > contribution if you ask me for specific tasks.
> >
> > For my part I have no idea how to investigate what is happening. In these
> > months I have run the VM hundreds of times on the most disparate filesystems
> > to test my conversions to kmap_local_page() and I have never seen anything
> > like this happen.
> >
> > Thanks,
> >
> > Fabio
> >
> >
> > Honza
> >
> > > --
> > > Jan Kara <[email protected]>
> > > SUSE Labs, CR
>
> I can't yet figure out which conditions lead to trigger the OOM Killer to kill
> the XFCE Desktop Environment, and the xfstests (which I usually run into the
> latter). After all, reserving 6GB of main memory to a QEMU/KVM x86_32 VM had
> always been more than adequate.
>
> So, I thought I'd better ignore that 6GB for a 32 bit architecture are a
> notable amount of RAM and squeezed some more from the host until I went to
> reserve 8GB. I know that this is not what who is able to find out what
> consumes so much main memory would do, but wanted to get the output from the
> tests, one way or the other... :-(
>
> OK, I could finally run my tests to completion and had no crashes at all. I
> ran "./check -g quick" on one "test" + three "scratch" loop devices formatted
> with "mkfs.ext2 -c". I ran three times _with_ and then three times _without_
> Al's following patches cloned from his vfs tree, #work.ext2 branch:
>
> f5b399373756 ("ext2: use offset_in_page() instead of open-coding it as
> 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
>
> All the six tests were no longer killed by the Kernel :-)
>
> I got 144 failures on 597 tests, regardless of the above listed patches.
>
> My final conclusion is that these patches don't introduce regressions. I see
> several tests that produce memory leaks but, I want to stress it again, the
> failing tests are always the same with and without the patches.
>
> therefore, I think that now I can safely add my tag to all five patches listed
> above...
>
> Tested-by: Fabio M. De Francesco <[email protected]>
Thanks for the effort! Al, will you submit these patches or should I just
pull your branch into my tree?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On luned? 20 marzo 2023 13:47:25 CEST Jan Kara wrote:
> On Mon 20-03-23 12:18:38, Fabio M. De Francesco wrote:
> > On gioved? 16 marzo 2023 11:30:21 CET Fabio M. De Francesco wrote:
> > > On gioved? 16 marzo 2023 10:00:35 CET Jan Kara wrote:
> > > > On Wed 15-03-23 19:08:57, Fabio M. De Francesco wrote:
> > > > > On mercoled? 1 marzo 2023 15:14:16 CET Al Viro wrote:
[snip]
> > > > > > I think I've pushed a demo patchset to vfs.git at some point back
in
> > > > > > January... Yep - see #work.ext2 in there; completely untested,
> > > > > > though.
Al,
I reviewed and tested your patchset (please see below).
I think that you probably also missed Jan's last message about how you prefer
they to be treated.
Jan asked you whether you will submit these patches or he should just pull
your branch into his tree.
Please look below for my tags and Jan's question.
> > > > >
> > > > > The following commits from the VFS tree, #work.ext2 look good to me.
> > > > >
> > > > > f5b399373756 ("ext2: use offset_in_page() instead of open-coding it
as
> > > > > 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_addr
> > >
> > > > > anymore")
> > > > >
> > > > > Reviewed-by: Fabio M. De Francesco <[email protected]>
> > > >
> > > > Thanks!
> > > >
[snip]
> > OK, I could finally run my tests to completion and had no crashes at all.
I
> > ran "./check -g quick" on one "test" + three "scratch" loop devices
> > formatted
> > with "mkfs.ext2 -c". I ran three times _with_ and then three times
_without_
> > Al's following patches cloned from his vfs tree, #work.ext2 branch:
> >
> > f5b399373756 ("ext2: use offset_in_page() instead of open-coding it as
> > 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
> >
> > All the six tests were no longer killed by the Kernel :-)
> >
> > I got 144 failures on 597 tests, regardless of the above listed patches.
> >
> > My final conclusion is that these patches don't introduce regressions. I
see
> > several tests that produce memory leaks but, I want to stress it again,
the
> > failing tests are always the same with and without the patches.
> >
> > therefore, I think that now I can safely add my tag to all five patches
> > listed above...
> >
> > Tested-by: Fabio M. De Francesco <[email protected]>
>
> Thanks for the effort! Al, will you submit these patches or should I just
> pull your branch into my tree?
>
>
Honza
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
Thanks,
Fabio
On Mon 27-03-23 12:29:56, Fabio M. De Francesco wrote:
> On luned? 20 marzo 2023 13:47:25 CEST Jan Kara wrote:
> > On Mon 20-03-23 12:18:38, Fabio M. De Francesco wrote:
> > > On gioved? 16 marzo 2023 11:30:21 CET Fabio M. De Francesco wrote:
> > > > On gioved? 16 marzo 2023 10:00:35 CET Jan Kara wrote:
> > > > > On Wed 15-03-23 19:08:57, Fabio M. De Francesco wrote:
> > > > > > On mercoled? 1 marzo 2023 15:14:16 CET Al Viro wrote:
>
> [snip]
>
> > > > > > > I think I've pushed a demo patchset to vfs.git at some point back
> in
> > > > > > > January... Yep - see #work.ext2 in there; completely untested,
> > > > > > > though.
>
> Al,
>
> I reviewed and tested your patchset (please see below).
>
> I think that you probably also missed Jan's last message about how you prefer
> they to be treated.
>
> Jan asked you whether you will submit these patches or he should just pull
> your branch into his tree.
>
> Please look below for my tags and Jan's question.
Ok, Al didn't reply so I've just pulled the patches from Al's tree, added
your Tested-by tag and push out the result into linux-next.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On gioved? 25 maggio 2023 22:10:46 CEST Jan Kara wrote:
> On Mon 27-03-23 12:29:56, Fabio M. De Francesco wrote:
> > On luned? 20 marzo 2023 13:47:25 CEST Jan Kara wrote:
> > > On Mon 20-03-23 12:18:38, Fabio M. De Francesco wrote:
> > > > On gioved? 16 marzo 2023 11:30:21 CET Fabio M. De Francesco wrote:
> > > > > On gioved? 16 marzo 2023 10:00:35 CET Jan Kara wrote:
> > > > > > On Wed 15-03-23 19:08:57, Fabio M. De Francesco wrote:
> > > > > > > On mercoled? 1 marzo 2023 15:14:16 CET Al Viro wrote:
> > [snip]
> >
> > > > > > > > I think I've pushed a demo patchset to vfs.git at some point
> > > > > > > > back
> >
> > in
> >
> > > > > > > > January... Yep - see #work.ext2 in there; completely untested,
> > > > > > > > though.
> >
> > Al,
> >
> > I reviewed and tested your patchset (please see below).
> >
> > I think that you probably also missed Jan's last message about how you
> > prefer
> > they to be treated.
> >
> > Jan asked you whether you will submit these patches or he should just pull
> > your branch into his tree.
> >
> > Please look below for my tags and Jan's question.
>
> Ok, Al didn't reply
I noticed it...
> so I've just pulled the patches from Al's tree,
Thank you very much for doing this :-)
> added
> your Tested-by tag
Did you also notice the Reviewed-by tags?
> and push out the result into linux-next.
Great!
Again thanks,
Fabio
>
Honza
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
On venerd? 26 maggio 2023 12:32:59 CEST Fabio M. De Francesco wrote:
> On gioved? 25 maggio 2023 22:10:46 CEST Jan Kara wrote:
> > On Mon 27-03-23 12:29:56, Fabio M. De Francesco wrote:
> > > On luned? 20 marzo 2023 13:47:25 CEST Jan Kara wrote:
> > > > On Mon 20-03-23 12:18:38, Fabio M. De Francesco wrote:
> > > > > On gioved? 16 marzo 2023 11:30:21 CET Fabio M. De Francesco wrote:
> > > > > > On gioved? 16 marzo 2023 10:00:35 CET Jan Kara wrote:
> > > > > > > On Wed 15-03-23 19:08:57, Fabio M. De Francesco wrote:
> > > > > > > > On mercoled? 1 marzo 2023 15:14:16 CET Al Viro wrote:
> > > [snip]
> > >
> > > > > > > > > I think I've pushed a demo patchset to vfs.git at some point
> > > > > > > > > back
> > >
> > > in
> > >
> > > > > > > > > January... Yep - see #work.ext2 in there; completely
untested,
> > > > > > > > > though.
> > >
> > > Al,
> > >
> > > I reviewed and tested your patchset (please see below).
> > >
> > > I think that you probably also missed Jan's last message about how you
> > > prefer
> > > they to be treated.
> > >
> > > Jan asked you whether you will submit these patches or he should just
pull
> > > your branch into his tree.
> > >
> > > Please look below for my tags and Jan's question.
> >
> > Ok, Al didn't reply
>
> I noticed it...
>
> > so I've just pulled the patches from Al's tree,
>
> Thank you very much for doing this :-)
>
> > added
> > your Tested-by tag
>
> Did you also notice the Reviewed-by tags?
>
Well, it looks like you missed my Reviewed-by tags at https://lore.kernel.org/
lkml/3019063.4lk9UinFSI@suse/
FWIW, I'd just like to say that I did the review of Al's patchset because I
know the code that is modeled after a similar series to fs/sysv, which in turn
I made following Al's suggestions.
However, I suppose it's up to you to decide whether or not is worth mentioning
my reviews :-)
Thanks,
Fabio
> > and push out the result into linux-next.
>
> Great!
> Again thanks,
>
> Fabio
>
>
> Honza
>
> > --
> > Jan Kara <[email protected]>
> > SUSE Labs, CR
On Fri 26-05-23 15:25:18, Fabio M. De Francesco wrote:
> On venerd? 26 maggio 2023 12:32:59 CEST Fabio M. De Francesco wrote:
> > On gioved? 25 maggio 2023 22:10:46 CEST Jan Kara wrote:
> > > On Mon 27-03-23 12:29:56, Fabio M. De Francesco wrote:
> > > > On luned? 20 marzo 2023 13:47:25 CEST Jan Kara wrote:
> > > > > On Mon 20-03-23 12:18:38, Fabio M. De Francesco wrote:
> > > > > > On gioved? 16 marzo 2023 11:30:21 CET Fabio M. De Francesco wrote:
> > > > > > > On gioved? 16 marzo 2023 10:00:35 CET Jan Kara wrote:
> > > > > > > > On Wed 15-03-23 19:08:57, Fabio M. De Francesco wrote:
> > > > > > > > > On mercoled? 1 marzo 2023 15:14:16 CET Al Viro wrote:
> > > > [snip]
> > > >
> > > > > > > > > > I think I've pushed a demo patchset to vfs.git at some point
> > > > > > > > > > back
> > > >
> > > > in
> > > >
> > > > > > > > > > January... Yep - see #work.ext2 in there; completely
> untested,
> > > > > > > > > > though.
> > > >
> > > > Al,
> > > >
> > > > I reviewed and tested your patchset (please see below).
> > > >
> > > > I think that you probably also missed Jan's last message about how you
> > > > prefer
> > > > they to be treated.
> > > >
> > > > Jan asked you whether you will submit these patches or he should just
> pull
> > > > your branch into his tree.
> > > >
> > > > Please look below for my tags and Jan's question.
> > >
> > > Ok, Al didn't reply
> >
> > I noticed it...
> >
> > > so I've just pulled the patches from Al's tree,
> >
> > Thank you very much for doing this :-)
> >
> > > added
> > > your Tested-by tag
> >
> > Did you also notice the Reviewed-by tags?
> >
>
> Well, it looks like you missed my Reviewed-by tags at https://lore.kernel.org/
> lkml/3019063.4lk9UinFSI@suse/
>
> FWIW, I'd just like to say that I did the review of Al's patchset because I
> know the code that is modeled after a similar series to fs/sysv, which in turn
> I made following Al's suggestions.
>
> However, I suppose it's up to you to decide whether or not is worth
> mentioning my reviews :-)
Yes, I've missed that you also gave your Reviewed-by tags. I will add
these. Thanks for the reminder :).
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR