Re: [PATCH] filemap: optimize order0 folio in filemap_map_pages

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2025/8/21 21:35, David Hildenbrand wrote:
> On 21.08.25 15:20, Matthew Wilcox wrote:
>> On Thu, Aug 21, 2025 at 02:57:54PM +0200, David Hildenbrand wrote:
>>> On 21.08.25 14:45, Matthew Wilcox wrote:
>>>> On Thu, Aug 21, 2025 at 09:22:48AM +0800, Jinjiang Tu wrote:
>>>>> 在 2025/8/20 20:42, Matthew Wilcox 写道:
>>>>>> On Wed, Aug 20, 2025 at 09:10:56AM +0800, Jinjiang Tu wrote:
>>>>>>> We should call folio_unlock() before folio_put(). In 
>>>>>>> filemap_map_order0_folio(),
>>>>>>> if we doesn't set folio into pte, we should unlock and put folio.
>>>>>> I agree that folio_unlock() needs to be called before folio_put().
>>>>>> What I don't understand is why we need to delay folio_unlock() until
>>>>>> right before folio_put().  Can't we leave folio_unlock() where it
>>>>>> currently is and only move the folio_put()?
>>>>>
>>>>> In filemap_map_order0_folio(), assuming the page is hwpoisoned, we 
>>>>> skip set_pte_range(),
>>>>> the folio should be unlocked and put. If we only move folio_put() 
>>>>> to filemap_map_order0_folio(),
>>>>> the folio is unlocked when filemap_map_pages() doesn't hold any 
>>>>> folio refcount.
>>>> Oh, I see.  I misread your patch; sorry about that.
>>>>
>>>> However, it is still safe to move only the folio_put() and not move
>>>> the folio_unlock()!  It's a little subtle, so I'll explain.
>>>>
>>>> We must not free a locked folio.  The page allocator has checks for 
>>>> this
>>>> and will complain (assuming appropriate debug options are enabled).  So
>>>> this sequence:
>>>>
>>>>     folio_put(folio);
>>>>     folio_unlock(folio);
>>>>
>>>> is _generally_ unsafe because the folio_put() might be the last put of
>>>> the refcount which will cause the folio to be freed.  However, if we 
>>>> know
>>>> that the folio has a refcount > 1, it's safe because the folio_put()
>>>> won't free the folio.  We do know that the folio has a refcount >1
>>>> because it's in the page cache, which keeps a refcount on the folio.
>>>> Since we have it locked, we know that truncation will wait for the 
>>>> unlock
>>>> to happen, and truncation will be the last one to put the refcount.
>>>
>>> I agree that it is save, but is it worth it having that subtle detail 
>>> here
>>> instead of just doing unlock+put?
>>>
>>> IOW, what do we gain by doing it differently? :)
>>
>> That was in the initial mail:
>>
>>> With this patch, we can get 8% performance gain for lmbench testcase
>>> 'lat_pagefault -P 1 file', the size of file is 512M.
>>
>> Obviously the exact gains are going to depend on how good your CPU is
>> at doing atomic inc/decs, but reducing the number of atomics is always
>> a good thing.
> 
> Ah, yes, obviously. I misread your mail, assuming you would argue for a 
> further change.
> 

So, Andrew, could you help to pick this up?