Re: [PATCH] ocfs2: update seq_file index in ocfs2_dlm_seq_next

Re: [PATCH] ocfs2: update seq_file index in ocfs2_dlm_seq_next

[Joseph Qi]

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@oracle.com>
> ---
>  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.

Thanks,
Joseph

>  	return iter;
>  }
>  

Re: [PATCH] ocfs2: update seq_file index in ocfs2_dlm_seq_next

[Wengang Wang]

> On Nov 10, 2024, at 5:38 PM, Joseph Qi <joseph.qi@linux.alibaba.com> 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@oracle.com>
>> ---
>> 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.

Thanks,
Wengang

Re: [PATCH] ocfs2: update seq_file index in ocfs2_dlm_seq_next

[Joseph Qi]

On 11/11/24 3:04 PM, Wengang Wang wrote:
> 
> 
>> On Nov 10, 2024, at 5:38 PM, Joseph Qi <joseph.qi@linux.alibaba.com> 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@oracle.com>
>>> ---
>>> 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.

Thanks,
Joseph

Re: [PATCH] ocfs2: update seq_file index in ocfs2_dlm_seq_next

[Andrew Morton]

On Mon, 11 Nov 2024 17:35:49 +0800 Joseph Qi <joseph.qi@linux.alibaba.com> wrote:

> 
> 
> On 11/11/24 3:04 PM, Wengang Wang wrote:
> > 
> > 
> >> On Nov 10, 2024, at 5:38 PM, Joseph Qi <joseph.qi@linux.alibaba.com> 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@oracle.com>
> >>> ---
> >>> 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?

Re: [PATCH] ocfs2: update seq_file index in ocfs2_dlm_seq_next

[Wengang Wang]

> On Feb 14, 2025, at 3:55 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> On Mon, 11 Nov 2024 17:35:49 +0800 Joseph Qi <joseph.qi@linux.alibaba.com> wrote:
> 
>> 
>> 
>> On 11/11/24 3:04 PM, Wengang Wang wrote:
>>> 
>>> 
>>>> On Nov 10, 2024, at 5:38 PM, Joseph Qi <joseph.qi@linux.alibaba.com> 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@oracle.com>
>>>>> ---
>>>>> 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