On Thu, 28 May 2020 at 02:58, Hugh Dickins <hughd@xxxxxxxxxx> wrote: > > On Tue, 26 May 2020, Johannes Weiner wrote: > > On Sat, May 23, 2020 at 06:50:15PM -0700, Hugh Dickins wrote: > > > When collapse_file() calls try_to_release_page(), it has already > > > isolated the page: so if releasing buffers happens to fail (as it > > > sometimes does), remember to putback_lru_page(): otherwise that page is > > > left unreclaimable and unfreeable, and the file extent uncollapsible. > > > > Oof, I could imagine that was painful to debug (unless you already > > suspected file THP due to a targeted test or similar). Kudos. > > Thanks, but I have to refuse both your admiration and sympathy: > mercifully, it was just something I noticed by source inspection > when working there. > > I did then put in a debug count to see if it ever got hit in practice, > and checked after big multi-testing runs: it was sometimes hit, but > certainly not often, and I don't know what it was racing with when > it happened - would depend on filesystem anyway (ext4 in our case). > > Saying "source inspection" reminds me: there is another funny in there, > but I don't think it matters very much in practice, and might need > rather a lot of testing to justify any particular patch: where > page_cache_sync_readahead() asks for PAGE_SIZE pages! > > "end - index" seems a more reasonable number to me: but then we > might find that reading ahead into the next huge extent had actually > been a useful optimization (and those readahead functions impose > their own caps, so PAGE_SIZE shouldn't work out too outrageously). My two cents, Do you have a test case / test steps to validate this patch ? - Naresh
