Re: [External] : Re: [WIP RFC PATCH] fs: retire I_WILL_FREE

[Joseph Qi <joseph.qi@linux.alibaba.com>]

Hi,

On 2025/9/3 23:19, Mateusz Guzik wrote:
> On Wed, Sep 3, 2025 at 5:16 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>>
>> On Wed, Sep 3, 2025 at 4:03 PM Mark Tinguely <mark.tinguely@oracle.com> wrote:
>>>
>>> On 9/2/25 10:44 PM, Matthew Wilcox wrote:
>>>> On Tue, Sep 02, 2025 at 04:54:28PM +0200, Mateusz Guzik wrote:
>>>>> Following up on my response to the refcount patchset, here is a churn
>>>>> patch to retire I_WILL_FREE.
>>>>>
>>>>> The only consumer is the drop inode routine in ocfs2.
>>>>
>>>> If the only consumer is ocfs2 ... then you should cc the ocfs2 people,
>>>> right?
>>>>
>>
>> Fair point, I just copy pasted the list from the patchset, too sloppy.
>>
>>>>> For the life of me I could not figure out if write_inode_now() is legal
>>>>> to call in ->evict_inode later and have no means to test, so I devised a
>>>>> hack: let the fs set I_FREEING ahead of time. Also note iput_final()
>>>>> issues write_inode_now() anyway but only for the !drop case, which is the
>>>>> opposite of what is being returned.
>>>>>
>>>>> One could further hack around it by having ocfs2 return *DON'T* drop but
>>>>> also set I_DONTCACHE, which would result in both issuing the write in
>>>>> iput_final() and dropping. I think the hack I did implement is cleaner.
>>>>> Preferred option is ->evict_inode from ocfs handling the i/o, but per
>>>>> the above I don't know how to do it.
>>>
>>> I am a lurker in this series and ocfs2. My history has been mostly in
>>> XFS/CXFS/DMAPI. I removed the other CC entries because I did not want
>>> to blast my opinion unnecessaially.
>>>
>>
>> Hello Mark,
>>
>> This needs the opinion of the vfs folk though, so I'm adding some of
>> the cc list back. ;)
>>
>>> The flushing in ocfs2_drop_inode() predates the I_DONTCACHE addition.
>>> IMO, it would be safest and best to maintain to let ocfs2_drop_inode()
>>> return 0 and set I_DONTCACHE and let iput_final() do the correct thing.
>>>
>>
> 
> wow, I'm sorry for really bad case of engrish in the mail. some of it
> gets slightly corrected below.
> 
>> For now that would indeed work in the sense of providing the expected
>> behavior, but there is the obvious mismatch of the filesystem claiming
>> the inode should not be dropped (by returning 0) and but using a side
>> indicator to drop it anyway. This looks like a split-brain scenario
>> and sooner or later someone is going to complain about it when they do
>> other work in iput_final(). If I was maintaining the layer I would
>> reject the idea, but if the actual gatekeepers are fine with it...
>>
>> The absolute best thing to do long run is to move the i/o in
>> ->evict_inode, but someone familiar with the APIs here would do the
>> needful(tm) and that's not me.
> 
> I mean the best thing to do in the long run is to move the the write
> to ->evict_inode, but I don't know how to do it and don't have any
> means to test ocfs2 anyway. Hopefully the ocfs2 folk will be willing
> to sort this out?
> 
Blame the histroy, I've found this commit:
513e2dae9422 ("ocfs2: flush inode data to disk and free inode when
i_count becomes zero")

It just make drop immediately and move up write_node_now() into
drop_inode(). So IMO, it looks fine for ocfs2 if setting I_FREEING
before write_inode_now() is safe.

Thanks,
Joseph