Re: [PATCH v2 3/6] iomap: advance the ifs allocation if we have more than one blocks per folio

[Zhang Yi <yi.zhang@huaweicloud.com>]

On 2024/8/17 12:42, Matthew Wilcox wrote:
> On Sat, Aug 17, 2024 at 12:27:49PM +0800, Zhang Yi wrote:
>> On 2024/8/14 13:32, Christoph Hellwig wrote:
>>> On Mon, Aug 12, 2024 at 08:11:56PM +0800, Zhang Yi wrote:
>>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>>
>>>> Now we allocate ifs if i_blocks_per_folio is larger than one when
>>>> writing back dirty folios in iomap_writepage_map(), so we don't attach
>>>> an ifs after buffer write to an entire folio until it starts writing
>>>> back, if we partial truncate that folio, iomap_invalidate_folio() can't
>>>> clear counterpart block's dirty bit as expected. Fix this by advance the
>>>> ifs allocation to __iomap_write_begin().
>>>
>>> Wouldn't it make more sense to only allocate the ifѕ in
>>> iomap_invalidate_folio when it actually is needed?
>>>
>>
>> I forget to mention that truncate_inode_partial_folio() call
>> folio_invalidate()->iomap_invalidate_folio() only when the folio has
>> private, if the folio doesn't has ifs, the iomap_invalidate_folio()
>> would nerver be called, hence allocate the ifs in
>> iomap_invalidate_folio() is useless.
>>
>> In my opinion, one solution is change to always call folio_invalidate()
>> in truncate_inode_partial_folio(), all callbacks should handle the no
>> private case. Another solution is add a magic (a fake ifs) to
>> folio->private and then convert it to a real one in
>> iomap_invalidate_folio(), any thoughts?
> 
> Why do we need iomap_invalidate_folio() to be called if there is no ifs?
> Even today, all it does is call ifs_free() if we're freeing the entire
> folio (which is done by truncate_cleanup_folio() and not by
> truncate_inode_partial_folio().
> 
Please see patch 2, if we truncate a partial folio (through punch hole or
truncate) on a filesystem with blocksize < folio size, it will left over
dirty bits of truncated/punched blocks, and this will lead to a hole with
dirty bit set but without any block allocated/reserved, this is not
correct.

Hence we also need to call iomap_invalidate_folio() by
truncate_inode_partial_folio() and clear partial folio of the counterpart
dirty bits. But now we don't allocate ifs in __iomap_write_begin() when
writing an entire folio, so it doesn't has ifs, but we still can
partial truncate this folio, we should allocate ifs for it and update
the dirty bits on it.

Thanks,
Yi.