[GIT PULL] 9p fixes for 6.12-rc4

[GIT PULL] 9p fixes for 6.12-rc4

[Dominique Martinet]

The following changes since commit 98f7e32f20d28ec452afb208f9cffc08448a2652:

  Linux 6.11 (2024-09-15 16:57:56 +0200)

are available in the Git repository at:

  https://github.com/martinetd/linux tags/9p-for-6.12-rc4

for you to fetch changes up to 79efebae4afc2221fa814c3cae001bede66ab259:

  9p: Avoid creating multiple slab caches with the same name (2024-09-23 05:51:27 +0900)

----------------------------------------------------------------
Mashed-up update that I sat on too long:

- fix for multiple slabs created with the same name
- enable multipage folios
- theorical fix to also look for opened fids by inode if none
was found by dentry

----------------------------------------------------------------
David Howells (1):
      9p: Enable multipage folios

Dominique Martinet (1):
      9p: v9fs_fid_find: also lookup by inode if not found dentry

Pedro Falcato (1):
      9p: Avoid creating multiple slab caches with the same name

 fs/9p/fid.c       |  5 ++---
 fs/9p/vfs_inode.c |  1 +
 net/9p/client.c   | 10 +++++++++-
 3 files changed, 12 insertions(+), 4 deletions(-)

-- 
Dominique Martinet | Asmadeus

Re: [GIT PULL] 9p fixes for 6.12-rc4

[pr-tracker-bot]

The pull request you sent on Sat, 19 Oct 2024 08:51:44 +0900:

> https://github.com/martinetd/linux tags/9p-for-6.12-rc4

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/9197b73fd7bb263084a95d1c578b7ee0ad54dfb3

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] 9p fixes for 6.12-rc4

[Andrii Nakryiko]

> The following changes since commit 98f7e32f20d28ec452afb208f9cffc08448a2652:
>
>   Linux 6.11 (2024-09-15 16:57:56 +0200)
>
> are available in the Git repository at:
> 
>   https://github.com/martinetd/linux tags/9p-for-6.12-rc4
> 
> for you to fetch changes up to 79efebae4afc2221fa814c3cae001bede66ab259:
>
>   9p: Avoid creating multiple slab caches with the same name (2024-09-23 05:51:27 +0900)
>
> ----------------------------------------------------------------
> Mashed-up update that I sat on too long:
> 
> - fix for multiple slabs created with the same name
> - enable multipage folios
> - theorical fix to also look for opened fids by inode if none
> was found by dentry
> 
> ----------------------------------------------------------------
> David Howells (1):
>      9p: Enable multipage folios

Are there any known implications of this change on madvise()'s MADV_PAGEOUT
behavior? After most recent pull from Linus's tree, one of BPF selftests
started failing. Bisection points to:

  9197b73fd7bb ("Merge tag '9p-for-6.12-rc4' of https://github.com/martinetd/linux")

... which is just an empty merge commit. So the "9p: Enable multipage folios"
by itself doesn't cause any regression, but when merged with the rest of the
code it does. I confirmed by reverting
1325e4a91a40 ("9p: Enable multipage folios"), after which the test in question
is succeeding again.

The test in question itself is a bit involved, but what it ultimately tries to
do is to ensure that part of ELF file containing build ID is paged out to cause
BPF helper to fail to retrieve said build ID (due to non-faulable context).

For that, we use the following sequence in target binary and process:

madvise(addr, page_sz, MADV_POPULATE_READ);
madvise(addr, page_sz, MADV_PAGEOUT);

First making sure page is paged in, then paged out. We make sure that build ID
is memory mapped in a separate segment with its own single-page memory mapping.
No changes or regressions there. No huge pages seem to be involved.

It used to work reliably, now it doesn't work. Any clue why or what should we
do differently to make sure that memory page with build ID information is not
paged in (reliably)?

Thanks!

P.S. The target binary and madvise() manipulations are at:

  tools/testing/selftests/bpf/uprobe_multi.c, see trigger_uprobe()

The test itself in BPF selftest is at:

  tools/testing/selftests/bpf/prog_tests/build_id.c, see subtest_nofault(),
  build_id_resident is false in this case.

>
> Dominique Martinet (1):
>       9p: v9fs_fid_find: also lookup by inode if not found dentry
> 
> Pedro Falcato (1):
>       9p: Avoid creating multiple slab caches with the same name
> 
>  fs/9p/fid.c       |  5 ++---
>  fs/9p/vfs_inode.c |  1 +
>  net/9p/client.c   | 10 +++++++++-
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> -- 
> Dominique Martinet | Asmadeus

Re: [GIT PULL] 9p fixes for 6.12-rc4

[Dominique Martinet]

Adding David/Willy to recpients as I'm not 100% up to date on folios

Andrii Nakryiko wrote on Wed, Oct 23, 2024 at 09:56:06AM -0700:
> > The following changes since commit 98f7e32f20d28ec452afb208f9cffc08448a2652:
> >
> >   Linux 6.11 (2024-09-15 16:57:56 +0200)
> >
> > are available in the Git repository at:
> > 
> >   https://github.com/martinetd/linux tags/9p-for-6.12-rc4
> > 
> > for you to fetch changes up to 79efebae4afc2221fa814c3cae001bede66ab259:
> >
> >   9p: Avoid creating multiple slab caches with the same name (2024-09-23 05:51:27 +0900)
> >
> > ----------------------------------------------------------------
> > Mashed-up update that I sat on too long:
> > 
> > - fix for multiple slabs created with the same name
> > - enable multipage folios
> > - theorical fix to also look for opened fids by inode if none
> > was found by dentry
> > 
> > ----------------------------------------------------------------
> > David Howells (1):
> >      9p: Enable multipage folios
> 
> Are there any known implications of this change on madvise()'s MADV_PAGEOUT
> behavior? After most recent pull from Linus's tree, one of BPF selftests
> started failing. Bisection points to:
> 
>   9197b73fd7bb ("Merge tag '9p-for-6.12-rc4' of https://github.com/martinetd/linux")
> 
> ... which is just an empty merge commit. So the "9p: Enable multipage folios"
> by itself doesn't cause any regression, but when merged with the rest of the
> code it does. I confirmed by reverting
> 1325e4a91a40 ("9p: Enable multipage folios"), after which the test in question
> is succeeding again.

(looks like 3c217a182018 ("selftests/bpf: add build ID tests") wasn't in
yet on the 9p multipage folios commit)

> The test in question itself is a bit involved, but what it ultimately tries to
> do is to ensure that part of ELF file containing build ID is paged out to cause
> BPF helper to fail to retrieve said build ID (due to non-faulable context).
> 
> For that, we use the following sequence in target binary and process:
> 
> madvise(addr, page_sz, MADV_POPULATE_READ);
> madvise(addr, page_sz, MADV_PAGEOUT);
> 
> First making sure page is paged in, then paged out. We make sure that build ID
> is memory mapped in a separate segment with its own single-page memory mapping.
> No changes or regressions there. No huge pages seem to be involved.

That's probably obvious but I guess the selftest runs the binary
directly from a 9p mount?

> It used to work reliably, now it doesn't work. Any clue why or what should we
> do differently to make sure that memory page with build ID information is not
> paged in (reliably)?

Unless David/Willy has a solution immediately I'd say let's take the time to
sort this out and revert that commit for now -- I'll send a revert patch
immediately and submit it to Linus on Saturday.

Conceptually I guess something is broken with MADV_PAGEOUT on >1 page
folio, perhaps it's only evicting folios if the whole folio is in range
but it should evict any folio that touches the range or something?

Sorry I don't have time to dig further here, hopefully that's not too
difficult to handle and we can try again in rc1 proper of another cycle,
I shouldn't have sent that this late.


(leaving full text below for new recipients)
> Thanks!
> 
> P.S. The target binary and madvise() manipulations are at:
> 
>   tools/testing/selftests/bpf/uprobe_multi.c, see trigger_uprobe()
> The test itself in BPF selftest is at:
> 
>   tools/testing/selftests/bpf/prog_tests/build_id.c, see subtest_nofault(),
>   build_id_resident is false in this case.
> 
> >
> > Dominique Martinet (1):
> >       9p: v9fs_fid_find: also lookup by inode if not found dentry
> > 
> > Pedro Falcato (1):
> >       9p: Avoid creating multiple slab caches with the same name
> > 
> >  fs/9p/fid.c       |  5 ++---
> >  fs/9p/vfs_inode.c |  1 +
> >  net/9p/client.c   | 10 +++++++++-
> >  3 files changed, 12 insertions(+), 4 deletions(-)
> > 
> 

Thanks,
-- 
Dominique Martinet | Asmadeus

Re: [GIT PULL] 9p fixes for 6.12-rc4

[Andrii Nakryiko]

On Wed, Oct 23, 2024 at 4:24 PM Dominique Martinet
<asmadeus@codewreck.org> wrote:
>
> Adding David/Willy to recpients as I'm not 100% up to date on folios
>
> Andrii Nakryiko wrote on Wed, Oct 23, 2024 at 09:56:06AM -0700:
> > > The following changes since commit 98f7e32f20d28ec452afb208f9cffc08448a2652:
> > >
> > >   Linux 6.11 (2024-09-15 16:57:56 +0200)
> > >
> > > are available in the Git repository at:
> > >
> > >   https://github.com/martinetd/linux tags/9p-for-6.12-rc4
> > >
> > > for you to fetch changes up to 79efebae4afc2221fa814c3cae001bede66ab259:
> > >
> > >   9p: Avoid creating multiple slab caches with the same name (2024-09-23 05:51:27 +0900)
> > >
> > > ----------------------------------------------------------------
> > > Mashed-up update that I sat on too long:
> > >
> > > - fix for multiple slabs created with the same name
> > > - enable multipage folios
> > > - theorical fix to also look for opened fids by inode if none
> > > was found by dentry
> > >
> > > ----------------------------------------------------------------
> > > David Howells (1):
> > >      9p: Enable multipage folios
> >
> > Are there any known implications of this change on madvise()'s MADV_PAGEOUT
> > behavior? After most recent pull from Linus's tree, one of BPF selftests
> > started failing. Bisection points to:
> >
> >   9197b73fd7bb ("Merge tag '9p-for-6.12-rc4' of https://github.com/martinetd/linux")
> >
> > ... which is just an empty merge commit. So the "9p: Enable multipage folios"
> > by itself doesn't cause any regression, but when merged with the rest of the
> > code it does. I confirmed by reverting
> > 1325e4a91a40 ("9p: Enable multipage folios"), after which the test in question
> > is succeeding again.
>
> (looks like 3c217a182018 ("selftests/bpf: add build ID tests") wasn't in
> yet on the 9p multipage folios commit)
>
> > The test in question itself is a bit involved, but what it ultimately tries to
> > do is to ensure that part of ELF file containing build ID is paged out to cause
> > BPF helper to fail to retrieve said build ID (due to non-faulable context).
> >
> > For that, we use the following sequence in target binary and process:
> >
> > madvise(addr, page_sz, MADV_POPULATE_READ);
> > madvise(addr, page_sz, MADV_PAGEOUT);
> >
> > First making sure page is paged in, then paged out. We make sure that build ID
> > is memory mapped in a separate segment with its own single-page memory mapping.
> > No changes or regressions there. No huge pages seem to be involved.
>
> That's probably obvious but I guess the selftest runs the binary
> directly from a 9p mount?

Yep, should have pointed that out explicitly.

>
> > It used to work reliably, now it doesn't work. Any clue why or what should we
> > do differently to make sure that memory page with build ID information is not
> > paged in (reliably)?
>
> Unless David/Willy has a solution immediately I'd say let's take the time to
> sort this out and revert that commit for now -- I'll send a revert patch
> immediately and submit it to Linus on Saturday.
>
> Conceptually I guess something is broken with MADV_PAGEOUT on >1 page
> folio, perhaps it's only evicting folios if the whole folio is in range
> but it should evict any folio that touches the range or something?

Could be, yeah. It's not necessarily a bug of 9P itself, but it would
be nice to have some way to page out memory. Maybe we need some extra
flags or a new MADV_PAGEOUT_OVERLAPPING command for madvise(), or
something along those lines?

>
> Sorry I don't have time to dig further here, hopefully that's not too
> difficult to handle and we can try again in rc1 proper of another cycle,
> I shouldn't have sent that this late.
>

No worries, thanks for a quick reply!

>
> (leaving full text below for new recipients)
> > Thanks!
> >
> > P.S. The target binary and madvise() manipulations are at:
> >
> >   tools/testing/selftests/bpf/uprobe_multi.c, see trigger_uprobe()
> > The test itself in BPF selftest is at:
> >
> >   tools/testing/selftests/bpf/prog_tests/build_id.c, see subtest_nofault(),
> >   build_id_resident is false in this case.
> >
> > >
> > > Dominique Martinet (1):
> > >       9p: v9fs_fid_find: also lookup by inode if not found dentry
> > >
> > > Pedro Falcato (1):
> > >       9p: Avoid creating multiple slab caches with the same name
> > >
> > >  fs/9p/fid.c       |  5 ++---
> > >  fs/9p/vfs_inode.c |  1 +
> > >  net/9p/client.c   | 10 +++++++++-
> > >  3 files changed, 12 insertions(+), 4 deletions(-)
> > >
> >
>
> Thanks,
> --
> Dominique Martinet | Asmadeus