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?