linux-kernel - Re: [PATCH v10 2/3] cachestat: implement cachestat syscall

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y/IUTiL03C9OOSFx@casper.infradead.org>
Date:   Sun, 19 Feb 2023 12:21:34 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Nhat Pham <nphamcs@...il.com>
Cc:     akpm@...ux-foundation.org, hannes@...xchg.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, bfoster@...hat.com, arnd@...db.de,
        linux-api@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH v10 2/3] cachestat: implement cachestat syscall

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/