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

[Jinjiang Tu <tujinjiang@huawei.com>]

[-- Attachment #1: Type: text/plain, Size: 2038 bytes --]


在 2025/8/21 20:45, Matthew Wilcox 写道:
> 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.

Ah, I see. Thanks for explaining this.

The performance gain is from decreasing atomic operations of folio's refcount,
is irrelevant to folio_unlock() call. So maybe it's no need to introduce this sublte change?

[-- Attachment #2: Type: text/html, Size: 2962 bytes --]