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?