Message-ID: <CAKEwX=MH2x1Uwjikc_L0EiPm5ptv4xkWhj2ZB9wLxxQQZ-0VuA@mail.gmail.com>
Date:   Wed, 11 Jan 2023 12:39:38 -0800
From:   Nhat Pham <nphamcs@...il.com>
To:     Brian Foster <bfoster@...hat.com>
Cc:     akpm@...ux-foundation.org, hannes@...xchg.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, willy@...radead.org,
        kernel-team@...a.com
Subject: Re: [PATCH v5 2/3] cachestat: implement cachestat syscall

On Wed, Jan 11, 2023 at 4:52 AM Brian Foster <bfoster@...hat.com> wrote:
>
> On Tue, Jan 10, 2023 at 01:16:27PM -0800, Nhat Pham wrote:
> > On Tue, Jan 10, 2023 at 6:21 AM Brian Foster <bfoster@...hat.com> wrote:
> > >
> > > On Wed, Jan 04, 2023 at 03:11:26PM -0800, Nhat Pham wrote:
> > > > Implement a new syscall that queries cache state of a file and
> > > > summarizes the number of cached pages, number of dirty pages, number of
> > > > pages marked for writeback, number of (recently) evicted pages, etc. in
> > > > a given range.
> > > >
> > > > NAME
> > > >     cachestat - query the page cache statistics of a file.
> > > >
> > > > SYNOPSIS
> > > >     #include <sys/mman.h>
> > > >
> > > >     struct cachestat {
> > > >         __u64 nr_cache;
> > > >         __u64 nr_dirty;
> > > >         __u64 nr_writeback;
> > > >         __u64 nr_evicted;
> > > >         __u64 nr_recently_evicted;
> > > >     };
> > > >
> > > >     int cachestat(unsigned int fd, off_t off, size_t len,
> > > >           size_t cstat_size, struct cachestat *cstat,
> > > >           unsigned int flags);
> > > >
> > > > DESCRIPTION
> > > >     cachestat() queries the number of cached pages, number of dirty
> > > >     pages, number of pages marked for writeback, number of evicted
> > > >     pages, number of recently evicted pages, in the bytes range given by
> > > >     `off` and `len`.
> > > >
> > > >     An evicted page is a page that is previously in the page cache but
> > > >     has been evicted since. A page is recently evicted if its last
> > > >     eviction was recent enough that its reentry to the cache would
> > > >     indicate that it is actively being used by the system, and that
> > > >     there is memory pressure on the system.
> > > >
> > > >     These values are returned in a cachestat struct, whose address is
> > > >     given by the `cstat` argument.
> > > >
> > > >     The `off` and `len` arguments must be non-negative integers. If
> > > >     `len` > 0, the queried range is [`off`, `off` + `len`]. If `len` ==
> > > >     0, we will query in the range from `off` to the end of the file.
> > > >
> > > >     `cstat_size` allows users to obtain partial results. The syscall
> > > >     will copy the first `csstat_size` bytes to the specified userspace
> > > >     memory. `cstat_size` must be a non-negative value that is no larger
> > > >     than the current size of the cachestat struct.
> > > >
> > > >     The `flags` argument is unused for now, but is included for future
> > > >     extensibility. User should pass 0 (i.e no flag specified).
> > > >
> > > > RETURN VALUE
> > > >     On success, cachestat returns 0. On error, -1 is returned, and errno
> > > >     is set to indicate the error.
> > > >
> > > > ERRORS
> > > >     EFAULT cstat points to an invalid address.
> > > >
> > > >     EINVAL invalid `cstat_size` or `flags`
> > > >
> > > >     EBADF  invalid file descriptor.
> > > >
> > > > Signed-off-by: Nhat Pham <nphamcs@...il.com>
> > > > ---
> > > >  arch/alpha/kernel/syscalls/syscall.tbl      |   1 +
> > > >  arch/arm/tools/syscall.tbl                  |   1 +
> > > >  arch/ia64/kernel/syscalls/syscall.tbl       |   1 +
> > > >  arch/m68k/kernel/syscalls/syscall.tbl       |   1 +
> > > >  arch/microblaze/kernel/syscalls/syscall.tbl |   1 +
> > > >  arch/parisc/kernel/syscalls/syscall.tbl     |   1 +
> > > >  arch/powerpc/kernel/syscalls/syscall.tbl    |   1 +
> > > >  arch/s390/kernel/syscalls/syscall.tbl       |   1 +
> > > >  arch/sh/kernel/syscalls/syscall.tbl         |   1 +
> > > >  arch/sparc/kernel/syscalls/syscall.tbl      |   1 +
> > > >  arch/x86/entry/syscalls/syscall_32.tbl      |   1 +
> > > >  arch/x86/entry/syscalls/syscall_64.tbl      |   1 +
> > > >  arch/xtensa/kernel/syscalls/syscall.tbl     |   1 +
> > > >  include/linux/fs.h                          |   3 +
> > > >  include/linux/syscalls.h                    |   3 +
> > > >  include/uapi/asm-generic/unistd.h           |   5 +-
> > > >  include/uapi/linux/mman.h                   |   9 ++
> > > >  init/Kconfig                                |  10 ++
> > > >  kernel/sys_ni.c                             |   1 +
> > > >  mm/filemap.c                                | 143 ++++++++++++++++++++
> > > >  20 files changed, 186 insertions(+), 1 deletion(-)
> > > >
> > > ...
> > > > diff --git a/mm/filemap.c b/mm/filemap.c
> > > > index 08341616ae7a..d70d47b20700 100644
> > > > --- a/mm/filemap.c
> > > > +++ b/mm/filemap.c
> > > ...
> > > > @@ -3949,3 +3953,142 @@ bool filemap_release_folio(struct folio *folio, gfp_t gfp)
> > > >       return try_to_free_buffers(folio);
> > > >  }
> > > >  EXPORT_SYMBOL(filemap_release_folio);
> > > ...
> > > > +#ifdef CONFIG_CACHESTAT_SYSCALL
> > > > +/*
> > > > + * The cachestat(5) system call.
> > > > + *
> > > > + * cachestat() returns the page cache statistics of a file in the
> > > > + * bytes range specified by `off` and `len`: number of cached pages,
> > > > + * number of dirty pages, number of pages marked for writeback,
> > > > + * number of evicted pages, and number of recently evicted pages.
> > > > + *
> > > > + * An evicted page is a page that is previously in the page cache
> > > > + * but has been evicted since. A page is recently evicted if its last
> > > > + * eviction was recent enough that its reentry to the cache would
> > > > + * indicate that it is actively being used by the system, and that
> > > > + * there is memory pressure on the system.
> > > > + *
> > > > + * `off` and `len` must be non-negative integers. If `len` > 0,
> > > > + * the queried range is [`off`, `off` + `len`]. If `len` == 0,
> > > > + * we will query in the range from `off` to the end of the file.
> > > > + *
> > > > + * `cstat_size` allows users to obtain partial results. The syscall
> > > > + * will copy the first `csstat_size` bytes to the specified userspace
> > > > + * memory. It also makes the cachestat struct extensible - new fields
> > > > + * can be added in the future without breaking existing usage.
> > > > + * `cstat_size` must be a non-negative value that is no larger than
> > > > + * the current size of the cachestat struct.
> > > > + *
> > > > + * The `flags` argument is unused for now, but is included for future
> > > > + * extensibility. User should pass 0 (i.e no flag specified).
> > > > + *
> > > > + * Because the status of a page can change after cachestat() checks it
> > > > + * but before it returns to the application, the returned values may
> > > > + * contain stale information.
> > > > + *
> > > > + * return values:
> > > > + *  zero    - success
> > > > + *  -EFAULT - cstat points to an illegal address
> > > > + *  -EINVAL - invalid arguments
> > > > + *  -EBADF   - invalid file descriptor
> > > > + */
> > > > +SYSCALL_DEFINE6(cachestat, unsigned int, fd, off_t, off, size_t, len,
> > > > +             size_t, cstat_size, struct cachestat __user *, cstat,
> > > > +             unsigned int, flags)
> > > > +{
> > > > +     struct fd f = fdget(fd);
> > > > +     struct address_space *mapping;
> > > > +     struct cachestat cs;
> > > > +     pgoff_t first_index = off >> PAGE_SHIFT;
> > > > +     pgoff_t last_index =
> > > > +             len == 0 ? ULONG_MAX : (off + len - 1) >> PAGE_SHIFT;
> > > > +
> > > > +     if (off < 0 || cstat_size > sizeof(struct cachestat) || flags != 0)
> > > > +             return -EINVAL;
> > > > +
> > > > +     if (!f.file)
> > > > +             return -EBADF;
> > > > +
> > >
> > > It looks like we miss an fdput() before returning via the above error
> > > checks.
> >
> > Ooops yeah I missed that. I'll fix it.
> >
> > >
> > > The only other thing that stands out as a bit odd to me is the
> > > cstat_size check and associated ability to return a partial cachestat
> > > struct. Do other syscalls do anything like that? I'd think we'd want to
> > > ensure we always at least return a fully populated cachestat struct,
> > > even if it happened to be an old/compat version if the size does ever
> > > increase. Hm?
> >
> > Not that I know of, but the idea is that the user might expect a smaller
> > struct cachestat in their code (and allocate memory accordingly).
> > With this cstat_size, we can make sure that the expansion of cachestat
> > struct (with new fields) does not break existing user's code - we only
> > copy part of the struct.
> >
>

I found a couple of syscalls doing something similar:

statfs64
https://github.com/torvalds/linux/blob/master/fs/statfs.c#L168-L190

io_uring_enter:
https://github.com/torvalds/linux/blob/master/io_uring/io_uring.c#L3232-L3243

It looks like these syscalls just error-out if the size argument does not match
with the size of the kernel struct.

> Yup, I get the idea of supporting future expansion. The part that
> doesn't make sense to me is allowing a partial copy of struct cachestat.
> For example, what happens if the user passes a cstat_size of 2? We copy
> out two bytes of the first field of the struct and return 0 for
> "success?"
>
> The way I've commonly seen this sort of support for future expansion is
> by versioning the structure and providing backwards compatibility for
> old versions of the structure. I'd guess there are various ways to
> implement that, but a simple example that comes to mind for me is
> xfs_ioc_fsgeometry(). The caveat there is that it's an ioctl where IIRC
> the command essentially changes when the structure size does, which
> allows the kernel to determine what version is being used.

Hmm xfs_ioc_fsgeometry looks interesting. From my understanding,
it basically has an argument that can take on a couple of values, each of
which delineates a version, right? I can give that a try and see if it looks
better (and maybe send a new version with this updated API).

>
> I'm not quite sure what the ideal way to do that for a syscall is. I
> suppose the kernel could just check that the size param matches some
> version of the structure it knows about and assume that. Or since you
> have the flags field, you could require a flag to be passed in to return
> values for any new fields. Or yet another option could be to pad out the
> size of the userspace structure to support future expansion for the
> foreseeable future without the need for such a flag.
>
> Perhaps this is something linux-api can help with?

I think I should definitely forward my patch series to linux-api. Thanks for
the suggestion, Brian!

>
> Brian
>
> > >
> > > Brian
> > >
> > > > +     memset(&cs, 0, sizeof(struct cachestat));
> > > > +     mapping = f.file->f_mapping;
> > > > +     filemap_cachestat(mapping, first_index, last_index, &cs);
> > > > +     fdput(f);
> > > > +
> > > > +     if (copy_to_user(cstat, &cs, cstat_size))
> > > > +             return -EFAULT;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +#endif /* CONFIG_CACHESTAT_SYSCALL */
> > > > --
> > > > 2.30.2
> > > >
> > >
> >
>

Powered by blists - more mailing lists