All of lore.kernel.org
 help / color / mirror / Atom feed
From: Josef Bacik <josef@toxicpanda.com>
To: Linus Torvalds <torvalds@linux-foundation.org>,
	Matthew Wilcox <willy@infradead.org>, Chris Mason <clm@fb.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 22:12:21 -0400	[thread overview]
Message-ID: <887b9eb7-2764-3659-d0bf-6a034a031618@toxicpanda.com> (raw)
In-Reply-To: <CAHk-=wjSGsRj7xwhSMQ6dAQiz53xA39pOG+XA_WeTgwBBu4uqg@mail.gmail.com>

On 3/16/21 8:43 PM, Linus Torvalds wrote:
> [ 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?
> 

Yeah it's just a flag, we use it to tell that the page is part of a range that 
has been allocated for IO.  The lifetime of the page is independent of the page, 
but is generally either dirty or under writeback, so either it goes through 
truncate and we clear PagePrivate2 there, or it actually goes through IO and is 
cleared before we drop the page in our endio.  We _always_ have PG_private set 
on the page as long as we own it, and PG_private_2 is only set in this IO 
related context, so we're safe there because of the rules around 
PG_dirty/PG_writeback.  We don't need it to have an extra ref for it being set. 
  Thanks,

Josef


  reply	other threads:[~2021-03-17  2:13 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
2021-03-17  0:43       ` Linus Torvalds
2021-03-17  2:12       ` Josef Bacik [this message]
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=887b9eb7-2764-3659-d0bf-6a034a031618@toxicpanda.com \
    --to=josef@toxicpanda.com \
    --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=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=torvalds@linux-foundation.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.