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