MIME-Version: 1.0
References: <20230219073318.366189-1-nphamcs@gmail.com> <20230219073318.366189-3-nphamcs@gmail.com>
 <Y/IUTiL03C9OOSFx@casper.infradead.org>
In-Reply-To: <Y/IUTiL03C9OOSFx@casper.infradead.org>
From:   Nhat Pham <nphamcs@gmail.com>
Date:   Sun, 5 Mar 2023 02:32:08 -0800
Message-ID: <CAKEwX=PTXLp7oe5HmmTpMuCbupuxsvw3gCwMqe=p4ck2byr7Zw@mail.gmail.com>
Subject: Re: [PATCH v10 2/3] cachestat: implement cachestat syscall
To:     Matthew Wilcox <willy@infradead.org>
Cc:     akpm@linux-foundation.org, hannes@cmpxchg.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, bfoster@redhat.com, arnd@arndb.de,
        linux-api@vger.kernel.org, kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Feb 19, 2023 at 4:21=E2=80=AFAM Matthew Wilcox <willy@infradead.org=
> wrote:
>
> On Sat, Feb 18, 2023 at 11:33:17PM -0800, Nhat Pham wrote:
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index e654435f1651..83300f1491e7 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -75,6 +75,7 @@ struct fs_context;
> >  struct fs_parameter_spec;
> >  struct fileattr;
> >  struct iomap_ops;
> > +struct cachestat;
> >
> >  extern void __init inode_init(void);
> >  extern void __init inode_init_early(void);
> > @@ -830,6 +831,8 @@ void filemap_invalidate_lock_two(struct address_spa=
ce *mapping1,
> >                                struct address_space *mapping2);
> >  void filemap_invalidate_unlock_two(struct address_space *mapping1,
> >                                  struct address_space *mapping2);
> > +void filemap_cachestat(struct address_space *mapping, pgoff_t first_in=
dex,
> > +             pgoff_t last_index, struct cachestat *cs);
>
> 1. Why is this in fs.h instead of pagemap.h?
>
> 2. Why is it not static, since it's only used by the syscall,
> which is also in filemap.c?

Actually, I think I can remove its occurrence in another header file
altogether. I think it was an artifact from a past iteration, which
has filemap_cachestat in filemap.c but the syscall in its own
file (cachestat.c).

And you're right - we only have one use case for it now
(for the syscall). I'll make it static.

>
> > @@ -55,6 +56,9 @@
> >  #include <linux/buffer_head.h> /* for try_to_free_buffers */
> >
> >  #include <asm/mman.h>
> > +#include <uapi/linux/mman.h>
>
> I think this hunk should be:
>
> -#include <asm/mman.h>
> +#include <linux/mman.h>

Will be fixed in the next version - thanks for letting me know!

>
> (linux/mman.h includes uapi/linux/mman.h, which includes asm/mman.h)
>
> > +/**
> > + * filemap_cachestat() - compute the page cache statistics of a mappin=
g
> > + * @mapping: The mapping to compute the statistics for.
> > + * @first_index:     The starting page cache index.
> > + * @last_index:      The final page index (inclusive).
> > + * @cs:      the cachestat struct to write the result to.
> > + *
> > + * This will query the page cache statistics of a mapping in the
> > + * page range of [first_index, last_index] (inclusive). The statistics
> > + * queried include: number of dirty pages, number of pages marked for
> > + * writeback, and the number of (recently) evicted pages.
> > + */
>
> Do we care that this isn't going to work for hugetlbfs?
>
> > +     rcu_read_lock();
> > +     xas_for_each(&xas, folio, last_index) {
> > +             unsigned long nr_pages;
> > +             pgoff_t folio_first_index, folio_last_index;
> > +
> > +             if (xas_retry(&xas, folio))
> > +                     continue;
> > +
> > +             nr_pages =3D folio_nr_pages(folio);
> > +             folio_first_index =3D folio_pgoff(folio);
> > +             folio_last_index =3D folio_first_index + nr_pages - 1;
> > +
> > +             /* Folios might straddle the range boundaries, only count=
 covered subpages */
>
> s/subpages/pages/

Yeah that comment is incorrect/confusing with the pages terminology.
It'll be fixed!