Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 21 Nov 2022 09:45:49 -0500
From:   Brian Foster <bfoster@redhat.com>
To:     Nhat Pham <nphamcs@gmail.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        hannes@cmpxchg.org
Subject: Re: [PATCH 3/4] cachestat: implement cachestat syscall
Message-ID: <Y3uPHYX6HxYuE4LX@bfoster>
References: <20221115182901.2755368-1-nphamcs@gmail.com>
 <20221115182901.2755368-4-nphamcs@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221115182901.2755368-4-nphamcs@gmail.com>
Precedence: bulk

On Tue, Nov 15, 2022 at 10:29:00AM -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 status of a file.
> 
> SYNOPSIS
>     #include <sys/mman.h>
> 
>     struct cachestat {
>         unsigned long nr_cache;
>         unsigned long nr_dirty;
>         unsigned long nr_writeback;
>         unsigned long nr_evicted;
>         unsigned long nr_recently_evicted;
>     };
> 
>     int cachestat(unsigned int fd, off_t off, size_t len,
>         struct cachestat *cstat);
> 

Do you have a strong use case for a user specified range vs. just
checking the entire file? If not, have you considered whether it might
be worth expanding statx() to include this data? That call is already
designed to include "extended" file status and avoids the need for a new
syscall. For example, the fields could be added individually with
multiple flags, or the entire struct tied to a new STATX_CACHE flag or
some such.

That said, I'm not sure how sensitive folks might be to increasing the
size of struct statx or populating it with cache data. Just a thought
that it might be worth bringing up on linux-fsdevel if you haven't
considered it already. A few random nits below..

> DESCRIPTION
>     cachestat() queries the number of cached pages, number of dirty
>     pages, number of pages marked for writeback, number of (recently)
>     evicted pages, in the bytes range given by `off` and `len`.
> 
>     These values are returned in a cachestat struct, whose address is
>     given by the `cstat` argument.
> 
>     The `off` argument must be a non-negative integers, If `off` + `len`
>     >= `off`, the queried range is [`off`, `off` + `len`]. Otherwise, we
>     will query in the range from `off` to the end of the file.
> 

(off + len < off) is an error condition on some (most?) other syscalls.
At least some calls (i.e. fadvise(), sync_file_range()) use len == 0 to
explicitly specify "to EOF."

> 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 `off` is negative.
> 
>     EBADF  invalid file descriptor.
> 
> Signed-off-by: Nhat Pham <nphamcs@gmail.com>
> ---
>  MAINTAINERS                            |   7 ++
>  arch/x86/entry/syscalls/syscall_32.tbl |   1 +
>  arch/x86/entry/syscalls/syscall_64.tbl |   1 +
>  include/linux/syscalls.h               |   2 +
>  include/uapi/asm-generic/unistd.h      |   5 +-
>  include/uapi/linux/mman.h              |   8 ++
>  kernel/sys_ni.c                        |   1 +
>  mm/Makefile                            |   2 +-
>  mm/cachestat.c                         | 109 +++++++++++++++++++++++++
>  9 files changed, 134 insertions(+), 2 deletions(-)
>  create mode 100644 mm/cachestat.c
> 
...
> diff --git a/mm/cachestat.c b/mm/cachestat.c
> new file mode 100644
> index 000000000000..193151cb0767
> --- /dev/null
> +++ b/mm/cachestat.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * The cachestat() system call.
> + */
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/syscalls.h>
> +#include <linux/shmem_fs.h>
> +#include <linux/swap.h>
> +#include <linux/swapops.h>
> +#include <uapi/linux/mman.h>
> +
> +#include "swap.h"
> +
> +/*
> + * The cachestat(3) system call.
> + *
> + * cachestat() returns the page cache status of a file in the
> + * bytes specified by `off` and `len`: number of cached pages,
> + * number of dirty pages, number of pages marked for writeback,
> + * number of (recently) evicted pages.
> + *
> + * If `off` + `len` >= `off`, the queried range is [`off`, `off` + `len`].
> + * Otherwise, we will query in the range from `off` to the end of the file.
> + *
> + * 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_DEFINE4(cachestat, unsigned int, fd, off_t, off, size_t, len,
> +	struct cachestat __user *, cstat)
> +{
> +	struct fd f;
> +	struct cachestat cs;
> +
> +	memset(&cs, 0, sizeof(struct cachestat));

Maybe move this after the next couple error checks to avoid unnecessary
work?

> +
> +	if (off < 0)
> +		return -EINVAL;
> +
> +	if (!access_ok(cstat, sizeof(struct cachestat)))
> +		return -EFAULT;
> +
> +	f = fdget(fd);

Doing this:

	if (!f.file)
		return -EBADF;

... removes a level of indentation for the rest of the function, making
it a bit easier to read (and removing the locals defined in the if
branch, which I'm not sure is widely accepted style for the kernel).

Brian

> +	if (f.file) {
> +		struct address_space *mapping = f.file->f_mapping;
> +		pgoff_t first_index = off >> PAGE_SHIFT;
> +		XA_STATE(xas, &mapping->i_pages, first_index);
> +		struct folio *folio;
> +		pgoff_t last_index = (off + len - 1) >> PAGE_SHIFT;
> +
> +		if (last_index < first_index)
> +			last_index = ULONG_MAX;
> +
> +		rcu_read_lock();
> +		xas_for_each(&xas, folio, last_index) {
> +			if (xas_retry(&xas, folio) || !folio)
> +				continue;
> +
> +			if (xa_is_value(folio)) {
> +				/* page is evicted */
> +				void *shadow;
> +				bool workingset; /* not used */
> +
> +				cs.nr_evicted += 1;
> +
> +				if (shmem_mapping(mapping)) {
> +					/* shmem file - in swap cache */
> +					swp_entry_t swp = radix_to_swp_entry(folio);
> +
> +					shadow = get_shadow_from_swap_cache(swp);
> +				} else {
> +					/* in page cache */
> +					shadow = (void *) folio;
> +				}
> +
> +				if (workingset_test_recent(shadow, true, &workingset))
> +					cs.nr_recently_evicted += 1;
> +
> +				continue;
> +			}
> +
> +			/* page is in cache */
> +			cs.nr_cache += 1;
> +
> +			if (folio_test_dirty(folio))
> +				cs.nr_dirty += 1;
> +
> +			if (folio_test_writeback(folio))
> +				cs.nr_writeback += 1;
> +		}
> +		rcu_read_unlock();
> +		fdput(f);
> +
> +		if (copy_to_user(cstat, &cs, sizeof(struct cachestat)))
> +			return -EFAULT;
> +
> +		return 0;
> +	}
> +
> +	return -EBADF;
> +}
> -- 
> 2.30.2
> 
>