Re: [PATCH] ocfs2: kill osb->system_file_mutex lock

[Tetsuo Handa]

On 2025/06/24 11:40, Heming Zhao wrote:
> On 6/24/25 10:17, Tetsuo Handa wrote:
>> On 2025/06/24 10:33, Heming Zhao wrote:
>>>> @@ -112,11 +110,10 @@ struct inode *ocfs2_get_system_file_inode(struct ocfs2_super *osb,
>>>>        inode = _ocfs2_get_system_file_inode(osb, type, slot);
>>>
>>> In my view, the key of commit 43b10a20372d is to avoid calling
>>> _ocfs2_get_system_file_inode() twice, which lead refcnt+1 but no place to
>>> do refcnt-1.
>>
>> My understanding is that concurrently calling _ocfs2_get_system_file_inode() itself
>> is OK, for the caller of ocfs2_get_system_file_inode() is responsible for calling
>> iput().
> 
> We have different perspectives on calling _ocfs2_get_system_file_inode().
> In the current code logic, _ocfs2_get_system_file_inode() is expected to
> be called only once. Subsequent local system inodes will be retrieved from
> the cache (via get_local_system_inode()).

That expectation is wrong. Since get_local_system_inode() involves memory allocation,
get_local_system_inode() might return NULL. Therefore, ocfs2_get_system_file_inode(),
which is the caller of get_local_system_inode(), has to be logically prepared for
calling _ocfs2_get_system_file_inode() for multiple times.

This cache is only for speeding lookups up.
This cache does not provide "lookup only once" requirement.

> 
> In my view, your patch has logical errors - at least from my perspective,
> I have to vote NAK.

If you NAK, you have to make sure that get_local_system_inode() never fails.