Re: [PATCH v3 bpf-next 06/10] lib/buildid: implement sleepable build_id_parse() API

[Shakeel Butt <shakeel.butt@linux.dev>]

On Wed, Jul 31, 2024 at 02:56:00PM GMT, Andrii Nakryiko wrote:
> On Tue, Jul 30, 2024 at 1:39 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > Extend freader with a flag specifying whether it's OK to cause page
> > fault to fetch file data that is not already physically present in
> > memory. With this, it's now easy to wait for data if the caller is
> > running in sleepable (faultable) context.
> >
> > We utilize read_cache_folio() to bring the desired file page into page
> > cache, after which the rest of the logic works just the same at page level.
> >
> > Suggested-by: Omar Sandoval <osandov@fb.com>
> > Cc: Shakeel Butt <shakeel.butt@linux.dev>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  lib/buildid.c | 50 ++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 34 insertions(+), 16 deletions(-)
> >
> > diff --git a/lib/buildid.c b/lib/buildid.c
> > index 5c869a2a30ab..6b5558cd95bf 100644
> > --- a/lib/buildid.c
> > +++ b/lib/buildid.c
> > @@ -20,6 +20,7 @@ struct freader {
> >                         struct page *page;
> >                         void *page_addr;
> >                         u64 file_off;
> > +                       bool may_fault;
> >                 };
> >                 struct {
> >                         const char *data;
> > @@ -29,12 +30,13 @@ struct freader {
> >  };
> >
> >  static void freader_init_from_file(struct freader *r, void *buf, u32 buf_sz,
> > -                                  struct address_space *mapping)
> > +                                  struct address_space *mapping, bool may_fault)
> >  {
> >         memset(r, 0, sizeof(*r));
> >         r->buf = buf;
> >         r->buf_sz = buf_sz;
> >         r->mapping = mapping;
> > +       r->may_fault = may_fault;
> >  }
> >
> >  static void freader_init_from_mem(struct freader *r, const char *data, u64 data_sz)
> > @@ -60,6 +62,17 @@ static int freader_get_page(struct freader *r, u64 file_off)
> >         freader_put_page(r);
> >
> >         r->page = find_get_page(r->mapping, pg_off);
> > +
> > +       if (!r->page && r->may_fault) {
> > +               struct folio *folio;
> > +
> > +               folio = read_cache_folio(r->mapping, pg_off, NULL, NULL);
> > +               if (IS_ERR(folio))
> > +                       return PTR_ERR(folio);
> > +
> > +               r->page = folio_file_page(folio, pg_off);
> > +       }
> > +
> 
> mm folks, is this the sane way to do this? Can you please take a look
> and provide your ack? Thank you!

Yes I think this is sane. I just had to check if read_cache_folio() is
also elevating a reference to the folio similar to find_get_page() and I
think it is doing so. I will go through the series soon.