Re: [PATCH v3 2/2] ocfs2: Convert remaining read-only checks to ocfs2_emergency_state

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

On 2025/12/3 08:46, Ahmet Eray Karadag wrote:
> On Tue, Dec 02, 2025 at 03:39:30PM +0800, Joseph Qi wrote:
>>
>>
>> On 2025/12/2 10:54, Ahmet Eray Karadag wrote:
>>> To centralize error checking, follow the pattern of other filesystems
>>> like ext4 (which uses `ext4_emergency_state()`), and prepare for
>>> future enhancements, this patch introduces a new helper function:
>>> `ocfs2_emergency_state()`.
>>>
>>> The purpose of this helper is to provide a single, unified location
>>> for checking all filesystem-level emergency conditions. In this
>>> initial implementation, the function only checks for the existing
>>> hard and soft read-only modes, returning -EROFS if either is set.
>>>
>>> This provides a foundation where future checks (e.g., for fatal error
>>> states returning -EIO, or shutdown states) can be easily added in
>>> one place.
>>>
>>> This patch also adds this new check to the beginning of
>>> `ocfs2_setattr()`. This ensures that operations like `ftruncate`
>>> (which triggered the original BUG) fail-fast with -EROFS when the
>>> filesystem is already in a read-only state.
>>>
>>
>> The above commit log is the same with patch 1 and doesn't reflect
>> the following changes.
>>
>>> Co-developed-by: Albin Babu Varghese <albinbabuvarghese20@gmail.com>
>>> Signed-off-by: Albin Babu Varghese <albinbabuvarghese20@gmail.com>
>>> Signed-off-by: Ahmet Eray Karadag <eraykrdg1@gmail.com>
>>> ---
>>> v2:
>>>  - Use `unlikely()` for status check
>>> ---
>>>  fs/ocfs2/buffer_head_io.c |  4 ++--
>>>  fs/ocfs2/file.c           | 17 ++++++++++-------
>>>  fs/ocfs2/inode.c          |  3 +--
>>>  fs/ocfs2/move_extents.c   |  5 +++--
>>>  fs/ocfs2/resize.c         |  8 +++++---
>>>  fs/ocfs2/super.c          |  2 +-
>>>  6 files changed, 22 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
>>> index 8f714406528d..61a0f522c673 100644
>>> --- a/fs/ocfs2/buffer_head_io.c
>>> +++ b/fs/ocfs2/buffer_head_io.c
>>> @@ -434,8 +434,8 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb,
>>>  	BUG_ON(buffer_jbd(bh));
>>>  	ocfs2_check_super_or_backup(osb->sb, bh->b_blocknr);
>>>  
>>> -	if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb)) {
>>> -		ret = -EROFS;
>>> +	ret = ocfs2_emergency_state(osb);
>>> +	if (unlikely(ret)) {
>>
>> I'd like use the following style:
>> if (ocfs2_emergency_state(osb)) {
>> 	ret = -EROFS;
>> 	...
>> }
>>
>> So that ocfs2_emergency_state() can be expended with other cases(properly
>> other return code), without touching these code flows.
> 
> In the future, there might be other return codes added to ocfs2_emergency_state().
> Correct me if I'm wrong, but with your proposed style, we would only return
> -EROFS in any case. Shouldn't we consider future improvements?
> 
> Thanks,
> Ahmet Eray
>>
Return EROFS here is to be consistent with before.
i.e. No functional change.

Joseph