| Message ID | 167172131368.2334525.8569808925687731937.stgit@warthog.procyon.org.uk |
|---|---|
| Headers | show |
| Series | mm, netfs, fscache: Stop read optimisation when folio removed from pagecache | expand |
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. 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.
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?
On Thu, Dec 22, 2022 at 03:02:02PM +0000, David Howells wrote: > Make filemap_release_folio() check folio_has_private(). Then, in most > cases, where a call to folio_has_private() is immediately followed by a > call to filemap_release_folio(), we can get rid of the test in the pair. > > The same is done to page_has_private()/try_to_release_page() call pairs. This line is now obsolete. > There are a couple of sites in mm/vscan.c that this can't so easily be > done. In shrink_folio_list(), there are actually three cases (something > different is done for incompletely invalidated buffers), but > filemap_release_folio() elides two of them. > > In shrink_active_list(), we don't have have the folio lock yet, so the > check allows us to avoid locking the page unnecessarily. > > A wrapper function to check if a folio needs release is provided for those > places that still need to do it in the mm/ directory. This will acquire > additional parts to the condition in a future patch. > > After this, the only remaining caller of folio_has_private() outside of mm/ > is a check in fuse. But there are a few remaining places which check page_has_private(). I think they're all wrong and should be PagePrivate(), but I'll look at them more next week.
Hi Linus, I've split the folio_has_private()/filemap_release_folio() call pair merging into its own patch, separate from the actual bugfix and pulled out the folio_needs_release() function into mm/internal.h and made filemap_release_folio() use it. I've also got rid of the bit clearances from the network filesystem evict_inode functions as they doesn't seem to be necessary. Note that the last vestiges of try_to_release_page() got swept away in the current merge window, so I rebased and dealt with that. One comment remained, which is removed by the first patch. David Changes: ======== ver #5) - Rebased on linus/master. try_to_release_page() has now been entirely replaced by filemap_release_folio(), barring one comment. - Cleaned up some pairs in ext4. ver #4) - Split has_private/release call pairs into own patch. - Moved folio_needs_release() to mm/internal.h and removed open-coded version from filemap_release_folio(). - Don't need to clear AS_RELEASE_ALWAYS in ->evict_inode(). - Added experimental patch to reduce shrink_folio_list(). ver #3) - Fixed mapping_clear_release_always() to use clear_bit() not set_bit(). - Moved a '&&' to the correct line. ver #2) - Rewrote entirely according to Willy's suggestion[1]. Link: https://lore.kernel.org/r/Yk9V/03wgdYi65Lb@casper.infradead.org/ [1] Link: https://lore.kernel.org/r/164928630577.457102.8519251179327601178.stgit@warthog.procyon.org.uk/ # v1 Link: https://lore.kernel.org/r/166844174069.1124521.10890506360974169994.stgit@warthog.procyon.org.uk/ # v2 Link: https://lore.kernel.org/r/166869495238.3720468.4878151409085146764.stgit@warthog.procyon.org.uk/ # v3 Link: https://lore.kernel.org/r/1459152.1669208550@warthog.procyon.org.uk/ # v3 also --- David Howells (3): mm: Merge folio_has_private()/filemap_release_folio() call pairs mm, netfs, fscache: Stop read optimisation when folio removed from pagecache mm: Make filemap_release_folio() better inform shrink_folio_list() fs/9p/cache.c | 2 ++ fs/afs/internal.h | 2 ++ fs/cachefiles/namei.c | 2 ++ fs/ceph/cache.c | 2 ++ fs/cifs/fscache.c | 2 ++ fs/ext4/move_extent.c | 12 ++++-------- fs/splice.c | 3 +-- include/linux/pagemap.h | 23 ++++++++++++++++++++++- mm/filemap.c | 20 +++++++++++++++----- mm/huge_memory.c | 3 +-- mm/internal.h | 11 +++++++++++ mm/khugepaged.c | 3 +-- mm/memory-failure.c | 8 +++----- mm/migrate.c | 3 +-- mm/truncate.c | 6 ++---- mm/vmscan.c | 35 ++++++++++++++++++----------------- 16 files changed, 89 insertions(+), 48 deletions(-)