Re: [PATCH] ocfs2: update seq_file index in ocfs2_dlm_seq_next
From: Wengang Wang
Date: Fri Feb 14 2025 - 19:24:15 EST
> On Feb 14, 2025, at 3:55 PM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, 11 Nov 2024 17:35:49 +0800 Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx> wrote:
>
>>
>>
>> On 11/11/24 3:04 PM, Wengang Wang wrote:
>>>
>>>
>>>> On Nov 10, 2024, at 5:38 PM, Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx> wrote:
>>>>
>>>>
>>>>
>>>> On 11/9/24 3:28 AM, Wengang Wang wrote:
>>>>> The following INFO level message was seen:
>>>>>
>>>>> seq_file: buggy .next function ocfs2_dlm_seq_next [ocfs2] did not
>>>>> update position index
>>>>>
>>>>> Fix:
>>>>> Updata m->index to make seq_read_iter happy though the index its self makes
>>>>> no sense to ocfs2_dlm_seq_next.
>>>>>
>>>>> Signed-off-by: Wengang Wang <wen.gang.wang@xxxxxxxxxx>
>>>>> ---
>>>>> fs/ocfs2/dlmglue.c | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
>>>>> index 60df52e4c1f8..349d131369cf 100644
>>>>> --- a/fs/ocfs2/dlmglue.c
>>>>> +++ b/fs/ocfs2/dlmglue.c
>>>>> @@ -3120,6 +3120,7 @@ static void *ocfs2_dlm_seq_next(struct seq_file *m, void *v, loff_t *pos)
>>>>> }
>>>>> spin_unlock(&ocfs2_dlm_tracking_lock);
>>>>>
>>>>> + m->index++;
>>>>
>>>> We can directly use '(*pos)++' instead.
>>>>
>>>
>>> The input/output "pos” indicates more an offset into the file. Actually the output for an item is not really 1 byte in length, so incrementing the offset by 1 sounds a bit strange to me. Instead If we increment the “index”, It would be easier to understand it as for next item. Though updating “index” or updating “*pos” instead makes no difference to binary running, the code understanding is different. I know other seq_operations.next functions are directly incrementing the “*pos”, I think updating “index” is better. Well, if you persist (*pos)++, I will also let it go.
>>>
>>> From seq_read_iter(), the input pos is equivalent to '&m->index'. So the
>> above two ways seems have no functional difference.
>> IMO, we'd better hide the m->index logic into seqfile and just use pos
>> instead like other .next implementations.
>
> Did we ever fix this?
Yes, fix is already in upstream code.
Thanks,
Wengang