Date:   Sun, 19 Feb 2023 12:21:34 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     Nhat Pham <nphamcs@gmail.com>
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
Subject: Re: [PATCH v10 2/3] cachestat: implement cachestat syscall
Message-ID: <Y/IUTiL03C9OOSFx@casper.infradead.org>
References: <20230219073318.366189-1-nphamcs@gmail.com>
 <20230219073318.366189-3-nphamcs@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230219073318.366189-3-nphamcs@gmail.com>
Precedence: bulk

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_space *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_index,
> +		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?

> @@ -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>

(linux/mman.h includes uapi/linux/mman.h, which includes asm/mman.h)

> +/**
> + * filemap_cachestat() - compute the page cache statistics of a mapping
> + * @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 = folio_nr_pages(folio);
> +		folio_first_index = folio_pgoff(folio);
> +		folio_last_index = folio_first_index + nr_pages - 1;
> +
> +		/* Folios might straddle the range boundaries, only count covered subpages */

s/subpages/pages/