From: Matthew Wilcox <willy@infradead.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: David Howells <dhowells@redhat.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Steve French <sfrench@samba.org>,
Shyam Prasad N <nspmangalore@gmail.com>,
Rohith Surabattula <rohiths.msft@gmail.com>,
Dave Wysochanski <dwysocha@redhat.com>,
Dominique Martinet <asmadeus@codewreck.org>,
Ilya Dryomov <idryomov@gmail.com>,
linux-cachefs@redhat.com, linux-cifs@vger.kernel.org,
linux-afs@lists.infradead.org,
v9fs-developer@lists.sourceforge.net, ceph-devel@vger.kernel.org,
linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-mm@kvack.org, Jeff Layton <jlayton@kernel.org>,
linux-erofs@lists.ozlabs.org, linux-ext4@lists.ozlabs.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 3/3] mm: Make filemap_release_folio() better inform shrink_folio_list()
Date: Sat, 7 Jan 2023 15:06:14 +0000 [thread overview]
Message-ID: <Y7mKZj/RnD2aW5jU@casper.infradead.org> (raw)
In-Reply-To: <Y6XJwvjKyTgRIiI3@infradead.org>
On Fri, Dec 23, 2022 at 07:31:14AM -0800, Christoph Hellwig wrote:
> On Thu, Dec 22, 2022 at 03:02:29PM +0000, David Howells wrote:
> > Make filemap_release_folio() return one of three values:
> >
> > (0) FILEMAP_CANT_RELEASE_FOLIO
> >
> > Couldn't release the folio's private data, so the folio can't itself
> > be released.
> >
> > (1) FILEMAP_RELEASED_FOLIO
> >
> > The private data on the folio was released and the folio can be
> > released.
> >
> > (2) FILEMAP_FOLIO_HAD_NO_PRIVATE
>
> These names read really odd, due to the different placementments
> of FOLIO, the present vs past tense and the fact that 2 also released
> the folio, and the reliance of callers that one value of an enum
> must be 0, while no unprecedented, is a bit ugly.
Agreed. The thing is that it's not the filemap that's being released,
it's the folio. So these should be:
FOLIO_RELEASE_SUCCESS
FOLIO_RELEASE_FAILED
FOLIO_RELEASE_NO_PRIVATE
... but of course, NO_PRIVATE is also a success. So it's a really weird
thing to be reporting. I'm with you on the latter half of this email:
> But do we even need them? What abut just open coding
> filemap_release_folio (which is a mostly trivial function) in
> shrink_folio_list, which is the only place that cares?
>
> if (folio_has_private(folio) && folio_needs_release(folio)) {
> if (folio_test_writeback(folio))
> goto activate_locked;
>
> if (mapping && mapping->a_ops->release_folio) {
> if (!mapping->a_ops->release_folio(folio, gfp))
> goto activate_locked;
> } else {
> if (!try_to_free_buffers(folio))
> goto activate_locked;
> }
>
> if (!mapping && folio_ref_count(folio) == 1) {
> ...
>
> alternatively just keep using filemap_release_folio and just add the
> folio_needs_release in the first branch. That duplicates the test,
> but makes the change a one-liner.
Or just drop patch 3 entirely?
prev parent reply other threads:[~2023-01-07 15:06 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-22 15:01 [PATCH v5 0/3] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache David Howells
2022-12-22 15:02 ` [PATCH v5 1/3] mm: Merge folio_has_private()/filemap_release_folio() call pairs David Howells
2023-01-07 15:11 ` Matthew Wilcox
2022-12-22 15:02 ` [PATCH v5 2/3] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache David Howells
2023-02-16 13:58 ` David Wysochanski
2022-12-22 15:02 ` [PATCH v5 3/3] mm: Make filemap_release_folio() better inform shrink_folio_list() David Howells
2022-12-23 15:31 ` Christoph Hellwig
2023-01-07 15:06 ` Matthew Wilcox [this message]
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=Y7mKZj/RnD2aW5jU@casper.infradead.org \
--to=willy@infradead.org \
--cc=asmadeus@codewreck.org \
--cc=ceph-devel@vger.kernel.org \
--cc=dhowells@redhat.com \
--cc=dwysocha@redhat.com \
--cc=hch@infradead.org \
--cc=idryomov@gmail.com \
--cc=jlayton@kernel.org \
--cc=linux-afs@lists.infradead.org \
--cc=linux-cachefs@redhat.com \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-erofs@lists.ozlabs.org \
--cc=linux-ext4@lists.ozlabs.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=nspmangalore@gmail.com \
--cc=rohiths.msft@gmail.com \
--cc=sfrench@samba.org \
--cc=torvalds@linux-foundation.org \
--cc=v9fs-developer@lists.sourceforge.net \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).