Re: [f2fs-dev] [PATCH v2] f2fs: evict: truncate page cache before clear_inode

[kth5965@gmail.com]

Hi Chao,

Thanks for the comment.

I took another look at the path, and I think you are right that my
current fix is too broad and may hide the real problem.

It looks like this happens in a more specific case, where an inode
still has FI_INLINE_DATA set, but FI_DATA_EXIST is not set.

In that case, f2fs_truncate() goes into the inline conversion path,
and f2fs_convert_inline_inode() grabs folio 0 before it checks
whether there is real inline data to move.

Then f2fs_convert_inline_folio() does this:

	if (!f2fs_exist_data(dn->inode))
		goto clear_out;

So for the empty-inline case, it returns success, but folio 0 seems to
have already been added to the page cache by then.

From what I can see, f2fs_grab_cache_folio() may create folio 0 and add
it to inode->i_mapping when there is no folio at that index, so
inode->i_data.nrpages becomes 1 there.

After that, f2fs_folio_put() only drops the ref, and the folio stays
there. Because of that, clear_inode() later sees nrpages != 0 and
hits the BUG.

This is the flow I am seeing:

f2fs_evict_inode()
  -> truncate_inode_pages_final(&inode->i_data)
     // nrpages = 0

  -> i_size_write(inode, 0)
  -> f2fs_truncate(inode)
     -> !f2fs_may_inline_data(inode)
     -> f2fs_convert_inline_inode(inode)
        -> f2fs_grab_cache_folio(inode->i_mapping, 0, false)
           // folio 0 is inserted into page cache
           // nrpages = 1
        -> f2fs_convert_inline_folio(&dn, folio)
           -> !f2fs_exist_data(inode)
           -> clear_out
        -> f2fs_folio_put(folio, true)
           // only drops the ref
           // folio stays in page cache

  -> clear_inode()
     -> BUG_ON(inode->i_data.nrpages)

So it seems better to fix this in the inline conversion path,
instead of truncating all page cache again at the end of eviction.

If you agree, I can send a new patch in that direction.

Thanks,