[git,pull] vfs.git sysv pile

  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!
  

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
  

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 <jack@suse.com>
> 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
  

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/20221229225100.22141-1-fmdefrancesco@gmail.com/

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
  

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 <fmdefrancesco@gmail.com>

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 <fmdefrancesco@gmail.com>

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
  

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 <fmdefrancesco@gmail.com>
> 
> 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 <jack@suse.com>
> 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 <fmdefrancesco@gmail.com>
> > 
> > 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 <jack@suse.com>
> > 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 <fmdefrancesco@gmail.com>

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 <fmdefrancesco@gmail.com>
> > > 
> > > 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 <jack@suse.com>
> > > 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 <fmdefrancesco@gmail.com>

Thanks for the effort! Al, will you submit these patches or should I just
pull your branch into my tree?

								Honza
  

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 <fmdefrancesco@gmail.com>
> > > > 
> > > > 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 <fmdefrancesco@gmail.com>
> 
> Thanks for the effort! Al, will you submit these patches or should I just
> pull your branch into my tree?
> 
> 								
Honza
> --
> Jan Kara <jack@suse.com>
> 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
  

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 <jack@suse.com>
> 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 <jack@suse.com>
> > 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