| Message ID | 20241108192829.58813-1-wen.gang.wang@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ocfs2: update seq_file index in ocfs2_dlm_seq_next | expand |
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; > } >
> 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
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
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++; return iter; }
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(+)