All of lore.kernel.org
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Matthew Wilcox <willy@infradead.org>, Chris Mason <clm@fb.com>,
	Josef Bacik <josef@toxicpanda.com>,
	David Sterba <dsterba@suse.com>
Cc: David Howells <dhowells@redhat.com>,
	Trond Myklebust <trondmy@hammerspace.com>,
	Anna Schumaker <anna.schumaker@netapp.com>,
	Steve French <sfrench@samba.org>,
	Dominique Martinet <asmadeus@codewreck.org>,
	Christoph Hellwig <hch@lst.de>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Linux-MM <linux-mm@kvack.org>,
	linux-cachefs@redhat.com, linux-afs@lists.infradead.org,
	"open list:NFS, SUNRPC, AND..." <linux-nfs@vger.kernel.org>,
	CIFS <linux-cifs@vger.kernel.org>,
	ceph-devel@vger.kernel.org, v9fs-developer@lists.sourceforge.net,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Jeff Layton <jlayton@redhat.com>,
	David Wysochanski <dwysocha@redhat.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 02/28] mm: Add an unlock function for PG_private_2/PG_fscache
Date: Tue, 16 Mar 2021 17:43:50 -0700	[thread overview]
Message-ID: <CAHk-=wjSGsRj7xwhSMQ6dAQiz53xA39pOG+XA_WeTgwBBu4uqg@mail.gmail.com> (raw)
In-Reply-To: <20210316190707.GD3420@casper.infradead.org>

[ Adding btrfs people explicitly, maybe they see this on the fs-devel
list, but maybe they don't react .. ]

On Tue, Mar 16, 2021 at 12:07 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> This isn't a problem with this patch per se, but I'm concerned about
> private2 and expected page refcounts.

Ugh. You are very right.

It would be good to just change the rules - I get the feeling nobody
actually depended on them anyway because they were _so_ esoteric.

> static inline int is_page_cache_freeable(struct page *page)
> {
>         /*
>          * A freeable page cache page is referenced only by the caller
>          * that isolated the page, the page cache and optional buffer
>          * heads at page->private.
>          */
>         int page_cache_pins = thp_nr_pages(page);
>         return page_count(page) - page_has_private(page) == 1 + page_cache_pins;

You're right, that "page_has_private()" is really really nasty.

The comment is, I think, the traditional usage case, which used to be
about page->buffers. Obviously these days it is now about
page->private with PG_private set, pointing to buffers
(attach_page_private() and detach_page_private()).

But as you point out:

> #define PAGE_FLAGS_PRIVATE                              \
>         (1UL << PG_private | 1UL << PG_private_2)
>
> So ... a page with both flags cleared should have a refcount of N.
> A page with one or both flags set should have a refcount of N+1.

Could we just remove the PG_private_2 thing in this context entirely,
and make the rule be that

 (a) PG_private means that you have some local private data in
page->private, and that's all that matters for the "freeable" thing.

 (b) PG_private_2 does *not* have the same meaning, and has no bearing
on freeability (and only the refcount matters)

I _)think_ the btrfs behavior is to only use PagePrivate2() when it
has a reference to the page, so btrfs doesn't care?

I think fscache is already happy to take the page count when using
PG_private_2 for locking, exactly because I didn't want to have any
confusion about lifetimes. But this "page_has_private()" math ends up
meaning it's confusing anyway.

btrfs people? What are the semantics for PG_private_2? Is it just a
flag, and you really don't want it to have anything to do with any
page lifetime decisions? Or?

        Linus

  reply	other threads:[~2021-03-17  0:50 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-10 16:54 [PATCH v4 00/28] Network fs helper library & fscache kiocb API David Howells
2021-03-10 16:54 ` [PATCH v4 01/28] iov_iter: Add ITER_XARRAY David Howells
2021-03-10 16:54 ` [PATCH v4 02/28] mm: Add an unlock function for PG_private_2/PG_fscache David Howells
2021-03-16 19:07   ` Matthew Wilcox
2021-03-17  0:43     ` Linus Torvalds [this message]
2021-03-17  0:43       ` Linus Torvalds
2021-03-17  2:12       ` Josef Bacik
2021-03-17  2:35         ` Linus Torvalds
2021-03-17  2:35           ` Linus Torvalds
2021-03-17  9:04         ` David Howells
2021-03-17 10:53         ` David Howells
2021-03-16 20:38   ` David Howells
2021-03-16 23:32     ` Matthew Wilcox
2021-03-21 10:53   ` Matthew Wilcox
2021-03-22 10:56   ` David Howells
2021-03-23 13:17   ` David Howells
2021-03-23 13:51     ` Matthew Wilcox
2021-03-23 14:16     ` David Howells
2021-03-23 22:06     ` David Howells
2021-03-23 22:06       ` David Howells
2021-03-10 16:55 ` [PATCH v4 03/28] mm: Implement readahead_control pageset expansion David Howells
2021-03-10 16:55 ` [PATCH v4 04/28] netfs: Make a netfs helper module David Howells
2021-03-10 16:55 ` [PATCH v4 05/28] netfs: Documentation for helper library David Howells
2021-03-10 16:55 ` [PATCH v4 06/28] netfs, mm: Move PG_fscache helper funcs to linux/netfs.h David Howells
2021-03-10 16:56 ` [PATCH v4 07/28] netfs, mm: Add unlock_page_fscache() and wait_on_page_fscache() David Howells
2021-03-10 16:56 ` [PATCH v4 08/28] netfs: Provide readahead and readpage netfs helpers David Howells
2021-03-21  1:42   ` Matthew Wilcox
2021-03-22 17:13   ` David Howells
2021-03-10 16:56 ` [PATCH v4 09/28] netfs: Add tracepoints David Howells
2021-03-10 16:56 ` [PATCH v4 10/28] netfs: Gather stats David Howells
2021-03-10 16:56 ` [PATCH v4 11/28] netfs: Add write_begin helper David Howells
2021-03-10 16:57 ` [PATCH v4 12/28] netfs: Define an interface to talk to a cache David Howells
2021-03-10 16:57 ` [PATCH v4 13/28] netfs: Hold a ref on a page when PG_private_2 is set David Howells
2021-03-10 16:57 ` [PATCH v4 14/28] fscache, cachefiles: Add alternate API to use kiocb for read/write to cache David Howells
2021-03-10 16:57 ` [PATCH v4 15/28] afs: Disable use of the fscache I/O routines David Howells
2021-03-10 16:58 ` [PATCH v4 16/28] afs: Pass page into dirty region helpers to provide THP size David Howells
2021-03-10 16:58 ` [PATCH v4 17/28] afs: Print the operation debug_id when logging an unexpected data version David Howells
2021-03-10 16:58 ` [PATCH v4 18/28] afs: Move key to afs_read struct David Howells
2021-03-10 16:58 ` [PATCH v4 19/28] afs: Don't truncate iter during data fetch David Howells
2021-03-10 16:58 ` [PATCH v4 20/28] afs: Log remote unmarshalling errors David Howells
2021-03-10 16:59 ` [PATCH v4 21/28] afs: Set up the iov_iter before calling afs_extract_data() David Howells
2021-03-10 16:59 ` [PATCH v4 22/28] afs: Use ITER_XARRAY for writing David Howells
2021-03-10 16:59 ` [PATCH v4 23/28] afs: Wait on PG_fscache before modifying/releasing a page David Howells
2021-03-10 16:59 ` [PATCH v4 24/28] afs: Extract writeback extension into its own function David Howells
2021-03-10 16:59 ` [PATCH v4 25/28] afs: Prepare for use of THPs David Howells
2021-03-10 17:00 ` [PATCH v4 26/28] afs: Use the fs operation ops to handle FetchData completion David Howells
2021-03-10 17:00 ` [PATCH v4 27/28] afs: Use new fscache read helper API David Howells
2021-03-10 17:00 ` [PATCH v4 28/28] afs: Use the fscache_write_begin() helper David Howells
2021-03-16 11:34 ` [PATCH v4 08/28] netfs: Provide readahead and readpage netfs helpers David Howells

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHk-=wjSGsRj7xwhSMQ6dAQiz53xA39pOG+XA_WeTgwBBu4uqg@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=anna.schumaker@netapp.com \
    --cc=asmadeus@codewreck.org \
    --cc=ceph-devel@vger.kernel.org \
    --cc=clm@fb.com \
    --cc=dhowells@redhat.com \
    --cc=dsterba@suse.com \
    --cc=dwysocha@redhat.com \
    --cc=hch@lst.de \
    --cc=jlayton@redhat.com \
    --cc=josef@toxicpanda.com \
    --cc=linux-afs@lists.infradead.org \
    --cc=linux-cachefs@redhat.com \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=sfrench@samba.org \
    --cc=trondmy@hammerspace.com \
    --cc=v9fs-developer@lists.sourceforge.net \
    --cc=viro@zeniv.linux.org.uk \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH v4 02/28] mm: Add an unlock function for PG_private_2/PG_fscache' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.