[Ocfs2-devel] [PATCH 1/4] lockres_seq_next should increase position index

[Ocfs2-devel] [PATCH 1/4] lockres_seq_next should increase position index

[Joseph Qi]

On 20/1/23 15:39, Vasily Averin wrote:
> if seq_file .next fuction does not change position index,
> read after some lseek can generate unexpected output.
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.kernel.org_show-5Fbug.cgi-3Fid-3D206283&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=C7gAd4uDxlAvTdc0vmU6X8CMk6L2iDY8-HD0qT6Fo7Y&m=lFWWo4w3KGQSd5EANoUFCp9aybqo1xeFs5MD0dgTnkA&s=dFZ-8He72Ffi8NGZB7iFik1nYOMs9wfzjDFpATCr72I&e= 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

We'd better add the missing tag:

Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")

> ---
>  fs/ocfs2/dlm/dlmdebug.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
> index 4d0b452..08a19c9 100644
> --- a/fs/ocfs2/dlm/dlmdebug.c
> +++ b/fs/ocfs2/dlm/dlmdebug.c
> @@ -590,6 +590,7 @@ static void lockres_seq_stop(struct seq_file *m, void *v)
>  
>  static void *lockres_seq_next(struct seq_file *m, void *v, loff_t *pos)
>  {
> +	(*pos)++;
>  	return NULL;
>  }
>  
> 

[Ocfs2-devel] [PATCH v2 0/4] ocfs2: seq_file .next functions should increase position index

[Vasily Averin]

v2: resend with improved patch description

In Aug 2018 NeilBrown noticed 
commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
"Some ->next functions do not increment *pos when they return NULL...
Note that such ->next functions are buggy and should be fixed. 
A simple demonstration is
   
dd if=/proc/swaps bs=1000 skip=1
    
Choose any block size larger than the size of /proc/swaps.  This will
always show the whole last line of /proc/swaps"

/proc/swaps output was fixed recently, however there are lot of other
affected files and 4 of them are related to ocfs2.

Unfortunately I'm not familiar with ocfs2 and cannot verify the patches locally.

Usually you can observe following related problems:
- read after lseek beyond end of file, described above by NeilBrown
 "dd if=<AFFECTED_FILE> bs=1000 skip=1" will incorrectly generate whole last line

- read after lseek on into middle of last line will output expected rest of
 last line but then repeat whole last line once again. 

- If .show() function generates multi-line output following bash script will never finish.

 $ q=;while read -r r;do echo "$((++q)) $r";done < AFFECTED_FILE

Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!Jq0fspl3uRa_Xiu-ifjc60ajewi308CVdv8Dq-9kHzOjkRYAd1YKlDU1a1TQdmMTBPWO8g$ 

Vasily Averin (4):
  lockres_seq_next should increase position index
  ocfs2_dlm_seq_next should increase position index
  nst_seq_next should increase position index
  sc_seq_next should increase position index

 fs/ocfs2/cluster/netdebug.c | 2 ++
 fs/ocfs2/dlm/dlmdebug.c     | 1 +
 fs/ocfs2/dlmglue.c          | 1 +
 3 files changed, 4 insertions(+)

-- 
1.8.3.1

[Ocfs2-devel] [PATCH v2 1/4] lockres_seq_next should increase position index

[Vasily Averin]

If .next function does not change position index,
following .show function will repeat output related
to current position index.

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!L2HMxpDQh7db9dQNbu_Tp0uSxEzRVFvZ0bxczbKOsvt3hiKOnWevYToiSclR_wkhhve7qA$ 
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ocfs2/dlm/dlmdebug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
index c5c6efb..3e395cd 100644
--- a/fs/ocfs2/dlm/dlmdebug.c
+++ b/fs/ocfs2/dlm/dlmdebug.c
@@ -590,6 +590,7 @@ static void lockres_seq_stop(struct seq_file *m, void *v)
 
 static void *lockres_seq_next(struct seq_file *m, void *v, loff_t *pos)
 {
+	(*pos)++;
 	return NULL;
 }
 
-- 
1.8.3.1

[Ocfs2-devel] [PATCH v2 2/4] ocfs2_dlm_seq_next should increase position index

[Vasily Averin]

If .next function does not change position index,
following .show function will repeat output related
to current position index.

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!MSixDxrphasW57tPl4VD6FKe6ZxcdQkP0oN-cWbZFdRxlNR1AOaFgm3-OunznZSXFEf0Nw$ 
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ocfs2/dlmglue.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
index cb9e6a7..170eb75 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -3080,6 +3080,7 @@ static void *ocfs2_dlm_seq_next(struct seq_file *m, void *v, loff_t *pos)
 	struct ocfs2_lock_res *iter = v;
 	struct ocfs2_lock_res *dummy = &priv->p_iter_res;
 
+	(*pos)++;
 	spin_lock(&ocfs2_dlm_tracking_lock);
 	iter = ocfs2_dlm_next_res(iter, priv);
 	list_del_init(&dummy->l_debug_list);
-- 
1.8.3.1

[Ocfs2-devel] [PATCH v2 3/4] nst_seq_next should increase position index

[Vasily Averin]

If .next function does not change position index,
following .show function will repeat output related
to current position index.

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!PE9FFsjnlLuJDzt36R28ZRQGJ-jhWo4rlejw_3MAX2zd1KvSlIomyKIzzBvc5psf88EhGw$ 
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ocfs2/cluster/netdebug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ocfs2/cluster/netdebug.c b/fs/ocfs2/cluster/netdebug.c
index 02bf4a1..26fc6a2 100644
--- a/fs/ocfs2/cluster/netdebug.c
+++ b/fs/ocfs2/cluster/netdebug.c
@@ -97,6 +97,7 @@ static void *nst_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 {
 	struct o2net_send_tracking *nst, *dummy_nst = seq->private;
 
+	++(*pos);
 	spin_lock(&o2net_debug_lock);
 	nst = next_nst(dummy_nst);
 	list_del_init(&dummy_nst->st_net_debug_item);
-- 
1.8.3.1

[Ocfs2-devel] [PATCH v2 4/4] sc_seq_next should increase position index

[Vasily Averin]

If .next function does not change position index,
following .show function will repeat output related
to current position index.

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!OngcjY3Xm--sW3K-NHZ9tpRCS9bSp-bm45QwSwULt5bj3vAbmwAs1_LXYQm9uQ3rK7wmDA$ 
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ocfs2/cluster/netdebug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ocfs2/cluster/netdebug.c b/fs/ocfs2/cluster/netdebug.c
index 26fc6a2..71ddd65 100644
--- a/fs/ocfs2/cluster/netdebug.c
+++ b/fs/ocfs2/cluster/netdebug.c
@@ -251,6 +251,7 @@ static void *sc_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 	struct o2net_sock_debug *sd = seq->private;
 	struct o2net_sock_container *sc, *dummy_sc = sd->dbg_sock;
 
+	++(*pos);
 	spin_lock(&o2net_debug_lock);
 	sc = next_sc(dummy_sc);
 	list_del_init(&dummy_sc->sc_net_debug_item);
-- 
1.8.3.1

[Ocfs2-devel] [PATCH v2 0/4] ocfs2: seq_file .next functions should increase position index

[Joseph Qi]

Looks good.
Jun, could you please help verify these changes and give your tested-by? 
Since I don't have ocfs2 cluster locally...

Thanks,
Joseph

On 2020/2/26 14:52, Vasily Averin wrote:
> v2: resend with improved patch description
> 
> In Aug 2018 NeilBrown noticed 
> commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
> "Some ->next functions do not increment *pos when they return NULL...
> Note that such ->next functions are buggy and should be fixed. 
> A simple demonstration is
>    
> dd if=/proc/swaps bs=1000 skip=1
>     
> Choose any block size larger than the size of /proc/swaps.  This will
> always show the whole last line of /proc/swaps"
> 
> /proc/swaps output was fixed recently, however there are lot of other
> affected files and 4 of them are related to ocfs2.
> 
> Unfortunately I'm not familiar with ocfs2 and cannot verify the patches locally.
> 
> Usually you can observe following related problems:
> - read after lseek beyond end of file, described above by NeilBrown
>  "dd if=<AFFECTED_FILE> bs=1000 skip=1" will incorrectly generate whole last line
> 
> - read after lseek on into middle of last line will output expected rest of
>  last line but then repeat whole last line once again. 
> 
> - If .show() function generates multi-line output following bash script will never finish.
> 
>  $ q=;while read -r r;do echo "$((++q)) $r";done < AFFECTED_FILE
> 
> Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!MWBujxdP_p0jZT4u4PHj-NBdhW4f9I2yRxwrFk8cG8MMHecBByM6r53K0M3eU9W5Oe1rdQ$ 
> 
> Vasily Averin (4):
>   lockres_seq_next should increase position index
>   ocfs2_dlm_seq_next should increase position index
>   nst_seq_next should increase position index
>   sc_seq_next should increase position index
> 
>  fs/ocfs2/cluster/netdebug.c | 2 ++
>  fs/ocfs2/dlm/dlmdebug.c     | 1 +
>  fs/ocfs2/dlmglue.c          | 1 +
>  3 files changed, 4 insertions(+)
> 

[Ocfs2-devel] [PATCH v2 0/4] ocfs2: seq_file .next functions should increase position index

[piaojun]

On 2020/2/26 16:32, Joseph Qi wrote:
> Looks good.
> Jun, could you please help verify these changes and give your tested-by? 
> Since I don't have ocfs2 cluster locally...

OK, Shan will help testing this patch and give a 'tested-by' later.

Jun

[Ocfs2-devel] Fwd: Re: [PATCH v2 0/4] ocfs2: seq_file .next functions should increase position index

[lishan]

test environment:
	kernel version: 5.6.0-rc5

lockres_seq_next -->
/sys/kernel/debug/o2dlm/2C9B43EBE44D4715A0B0DBE4D72016A3/locking_state
The output content is incomplete and endless, and the subsequent output is skip reading.
The operation and print is as follows:
dd if=/sys/kernel/debug/o2dlm/2C9B43EBE44D4715A0B0DBE4D72016A3/locking_state bs=40 skip=1
S:1,5,0,0,0,0,0,0,0,0,3
RMAP:
LVBX:00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
LOCK:1,0,5,-1,5,5:1,0,0,0,0,0,0,0,0,2

NAME:M0000000000000000000042e7349704
LRES:1,5,0,0,0,0,0,0,0,0,3
RMAP:
LVBX:00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
LOCK:1,0,5,-1,5,5:2,0,0,0,0,0,0,0,0,2
........
NAME:M0000000000000000000042e7349704
LRES:1,5,0,0,0,0,0,0,0,0,3
RMAP:
LVBX:00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
LOCK:1,0,5,-1,5,5:2,0,0,0,0,0,0,0,0,2

NAME:M0000000000000000000010e7349704
......

but, there is no endless output using the following script:
linux-EalnPD:~ # q=;while read -r r;do echo "$((++q)) $r";done < /sys/kernel/debug/o2dlm/2C9B43EBE44D4715A0B0DBE4D72016A3/locking_state
1 NAME:S000000000000000000000200000000
2 LRES:1,5,0,0,0,0,0,0,0,0,3
3 RMAP:
4 LVBX:00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
5 LOCK:1,0,5,-1,5,5:6,0,0,0,0,0,0,0,0,2
6
7
8 NAME:M0000000000000000000016e7349704
9 NAME:M0000000000000000000018e7349704
10 NAME:M000000000000000000001ae7349704
11 NAME:M000000000000000000001ce7349704
12 NAME:M000000000000000000001ee7349704
13 NAME:M0000000000000000000020e7349704
....
534 NAME:W00000000000000000843c81d0b7768
535 NAME:O00000000000000000843c800000000

Other interface test results:
nst_seq_next --> /sys/kernel/debug/o2net/send_tracking
sc_seq_next --> /sys/kernel/debug/o2net/stats
File content is empty, so, read it like use dd command, No effect.
ocfs2_dlm_seq_next -->
/sys/kernel/debug/ocfs2/2C9B43EBE44D4715A0B0DBE4D72016A3/locking_state
Read file normal output.

And, output is still endless, after adding Vasily Averin's modifications, patch are as follow:
[Ocfs2-devel] [PATCH v2 1/4] lockres_seq_next should increase position index   Vasily Averin
[Ocfs2-devel] [PATCH v2 2/4] ocfs2_dlm_seq_next should increase position index   Vasily Averin
[Ocfs2-devel] [PATCH v2 3/4] nst_seq_next should increase position index   Vasily Averin
[Ocfs2-devel] [PATCH v2 4/4] sc_seq_next should increase position index

I added the log in seq_read, use the 'dd' read command above, print as follows:
2020-03-21T11:27:50.691302+08:00|notice|kernel[-]|[92756.285422] [22000/dd] --> seq_read, init, start ppos 250434, max read size 10, m->read_pos 250434, mem index 1028, mem size 4096, mem from 233, mem count 11.
2020-03-21T11:27:50.691328+08:00|notice|kernel[-]|[92756.285430] [22000/dd] --> seq_read, init, start ppos 250444, max read size 10, m->read_pos 250444, mem index 1028, mem size 4096, mem from 243, mem count 1.
2020-03-21T11:27:50.691355+08:00|notice|kernel[-]|[92756.285442] [22000/dd] --> seq_read, read start, m->read_pos 250444, mem index 1028, mem size 4096, mem from 0, mem count 0.
2020-03-21T11:27:50.691381+08:00|notice|kernel[-]|[92756.285444] [22000/dd] --> seq_read, before go to file, mem file count 244, mem file size 4096
2020-03-21T11:27:50.691406+08:00|notice|kernel[-]|[92756.285453] [22000/dd] --> seq_read, init, start ppos 250454, max read size 10, m->read_pos 250454, mem index 1029, mem size 4096, mem from 9, mem count 235.
2020-03-21T11:27:50.691434+08:00|notice|kernel[-]|[92756.285458] [22000/dd] --> seq_read, init, start ppos 250464, max read size 10, m->read_pos 250464, mem index 1029, mem size 4096, mem from 19, mem count 225.
......

I haven't figured out what the reason is, but the result is not as expected.

On 2020/3/9 16:27, piaojun wrote:
> 
> 
> 
> -------- Forwarded Message --------
> Subject: Re: [PATCH v2 0/4] ocfs2: seq_file .next functions should increase position index
> Date: Wed, 26 Feb 2020 16:32:30 +0800
> From: Joseph Qi <joseph.qi@linux.alibaba.com>
> To: Vasily Averin <vvs@virtuozzo.com>, ocfs2-devel at oss.oracle.com, piaojun <piaojun@huawei.com>
> CC: Mark Fasheh <mark@fasheh.com>, Joel Becker <jlbec@evilplan.org>
> 
> Looks good.
> Jun, could you please help verify these changes and give your tested-by? Since I don't have ocfs2 cluster locally...
> 
> Thanks,
> Joseph
> 
> On 2020/2/26 14:52, Vasily Averin wrote:
>> v2: resend with improved patch description
>>
>> In Aug 2018 NeilBrown noticed 
>> commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
>> "Some ->next functions do not increment *pos when they return NULL...
>> Note that such ->next functions are buggy and should be fixed. 
>> A simple demonstration is
>>    
>> dd if=/proc/swaps bs=1000 skip=1
>>     
>> Choose any block size larger than the size of /proc/swaps.  This will
>> always show the whole last line of /proc/swaps"
>>
>> /proc/swaps output was fixed recently, however there are lot of other
>> affected files and 4 of them are related to ocfs2.
>>
>> Unfortunately I'm not familiar with ocfs2 and cannot verify the patches locally.
>>
>> Usually you can observe following related problems:
>> - read after lseek beyond end of file, described above by NeilBrown
>>  "dd if=<AFFECTED_FILE> bs=1000 skip=1" will incorrectly generate whole last line
>>
>> - read after lseek on into middle of last line will output expected rest of
>>  last line but then repeat whole last line once again. 
>>
>> - If .show() function generates multi-line output following bash script will never finish.
>>
>>  $ q=;while read -r r;do echo "$((++q)) $r";done < AFFECTED_FILE
>>
>> Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!KCKPYUMw_U1HxUL22F1ZSc3jh5A-b_7ZVR-iw9uPAprEKFstRKwLXaJmpXuqsTjpGIByXQ$ 
>>
>> Vasily Averin (4):
>>   lockres_seq_next should increase position index
>>   ocfs2_dlm_seq_next should increase position index
>>   nst_seq_next should increase position index
>>   sc_seq_next should increase position index
>>
>>  fs/ocfs2/cluster/netdebug.c | 2 ++
>>  fs/ocfs2/dlm/dlmdebug.c     | 1 +
>>  fs/ocfs2/dlmglue.c          | 1 +
>>  3 files changed, 4 insertions(+)
>>
> .
> 
> 
> .
> 

[Ocfs2-devel] Fwd: Re: [PATCH v2 0/4] ocfs2: seq_file .next functions should increase position index

[Vasily Averin]

Dear lishan,
thank you for your experiments.

Dear all, please drop my patch-set, they are incomplete.

seq_opetations used in ocfs2 are incorrect -- they ignores position index provided by seq_read().
by design .start() functions should skip first elements specified by *pos argument,
however lockres_seq_start(), ocfs2_dlm_seq_start() and other igores *pos, and uses own logic instead.

It should be reworked.

Thank you,
	Vasily Averin

On 3/21/20 7:24 AM, lishan wrote:
> test environment:
> 	kernel version: 5.6.0-rc5
> 
> lockres_seq_next -->
> /sys/kernel/debug/o2dlm/2C9B43EBE44D4715A0B0DBE4D72016A3/locking_state
> The output content is incomplete and endless, and the subsequent output is skip reading.
> The operation and print is as follows:
> dd if=/sys/kernel/debug/o2dlm/2C9B43EBE44D4715A0B0DBE4D72016A3/locking_state bs=40 skip=1
> S:1,5,0,0,0,0,0,0,0,0,3
> RMAP:
> LVBX:00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> LOCK:1,0,5,-1,5,5:1,0,0,0,0,0,0,0,0,2
> 
> NAME:M0000000000000000000042e7349704
> LRES:1,5,0,0,0,0,0,0,0,0,3
> RMAP:
> LVBX:00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> LOCK:1,0,5,-1,5,5:2,0,0,0,0,0,0,0,0,2
> ........
> NAME:M0000000000000000000042e7349704
> LRES:1,5,0,0,0,0,0,0,0,0,3
> RMAP:
> LVBX:00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> LOCK:1,0,5,-1,5,5:2,0,0,0,0,0,0,0,0,2
> 
> NAME:M0000000000000000000010e7349704
> ......
> 
> but, there is no endless output using the following script:
> linux-EalnPD:~ # q=;while read -r r;do echo "$((++q)) $r";done < /sys/kernel/debug/o2dlm/2C9B43EBE44D4715A0B0DBE4D72016A3/locking_state
> 1 NAME:S000000000000000000000200000000
> 2 LRES:1,5,0,0,0,0,0,0,0,0,3
> 3 RMAP:
> 4 LVBX:00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 5 LOCK:1,0,5,-1,5,5:6,0,0,0,0,0,0,0,0,2
> 6
> 7
> 8 NAME:M0000000000000000000016e7349704
> 9 NAME:M0000000000000000000018e7349704
> 10 NAME:M000000000000000000001ae7349704
> 11 NAME:M000000000000000000001ce7349704
> 12 NAME:M000000000000000000001ee7349704
> 13 NAME:M0000000000000000000020e7349704
> ....
> 534 NAME:W00000000000000000843c81d0b7768
> 535 NAME:O00000000000000000843c800000000
> 
> Other interface test results:
> nst_seq_next --> /sys/kernel/debug/o2net/send_tracking
> sc_seq_next --> /sys/kernel/debug/o2net/stats
> File content is empty, so, read it like use dd command, No effect.
> ocfs2_dlm_seq_next -->
> /sys/kernel/debug/ocfs2/2C9B43EBE44D4715A0B0DBE4D72016A3/locking_state
> Read file normal output.
> 
> And, output is still endless, after adding Vasily Averin's modifications, patch are as follow:
> [Ocfs2-devel] [PATCH v2 1/4] lockres_seq_next should increase position index   Vasily Averin
> [Ocfs2-devel] [PATCH v2 2/4] ocfs2_dlm_seq_next should increase position index   Vasily Averin
> [Ocfs2-devel] [PATCH v2 3/4] nst_seq_next should increase position index   Vasily Averin
> [Ocfs2-devel] [PATCH v2 4/4] sc_seq_next should increase position index
> 
> I added the log in seq_read, use the 'dd' read command above, print as follows:
> 2020-03-21T11:27:50.691302+08:00|notice|kernel[-]|[92756.285422] [22000/dd] --> seq_read, init, start ppos 250434, max read size 10, m->read_pos 250434, mem index 1028, mem size 4096, mem from 233, mem count 11.
> 2020-03-21T11:27:50.691328+08:00|notice|kernel[-]|[92756.285430] [22000/dd] --> seq_read, init, start ppos 250444, max read size 10, m->read_pos 250444, mem index 1028, mem size 4096, mem from 243, mem count 1.
> 2020-03-21T11:27:50.691355+08:00|notice|kernel[-]|[92756.285442] [22000/dd] --> seq_read, read start, m->read_pos 250444, mem index 1028, mem size 4096, mem from 0, mem count 0.
> 2020-03-21T11:27:50.691381+08:00|notice|kernel[-]|[92756.285444] [22000/dd] --> seq_read, before go to file, mem file count 244, mem file size 4096
> 2020-03-21T11:27:50.691406+08:00|notice|kernel[-]|[92756.285453] [22000/dd] --> seq_read, init, start ppos 250454, max read size 10, m->read_pos 250454, mem index 1029, mem size 4096, mem from 9, mem count 235.
> 2020-03-21T11:27:50.691434+08:00|notice|kernel[-]|[92756.285458] [22000/dd] --> seq_read, init, start ppos 250464, max read size 10, m->read_pos 250464, mem index 1029, mem size 4096, mem from 19, mem count 225.
> ......
> 
> I haven't figured out what the reason is, but the result is not as expected.
> 
> On 2020/3/9 16:27, piaojun wrote:
>>
>>
>>
>> -------- Forwarded Message --------
>> Subject: Re: [PATCH v2 0/4] ocfs2: seq_file .next functions should increase position index
>> Date: Wed, 26 Feb 2020 16:32:30 +0800
>> From: Joseph Qi <joseph.qi@linux.alibaba.com>
>> To: Vasily Averin <vvs@virtuozzo.com>, ocfs2-devel at oss.oracle.com, piaojun <piaojun@huawei.com>
>> CC: Mark Fasheh <mark@fasheh.com>, Joel Becker <jlbec@evilplan.org>
>>
>> Looks good.
>> Jun, could you please help verify these changes and give your tested-by? Since I don't have ocfs2 cluster locally...
>>
>> Thanks,
>> Joseph
>>
>> On 2020/2/26 14:52, Vasily Averin wrote:
>>> v2: resend with improved patch description
>>>
>>> In Aug 2018 NeilBrown noticed 
>>> commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
>>> "Some ->next functions do not increment *pos when they return NULL...
>>> Note that such ->next functions are buggy and should be fixed. 
>>> A simple demonstration is
>>>    
>>> dd if=/proc/swaps bs=1000 skip=1
>>>     
>>> Choose any block size larger than the size of /proc/swaps.  This will
>>> always show the whole last line of /proc/swaps"
>>>
>>> /proc/swaps output was fixed recently, however there are lot of other
>>> affected files and 4 of them are related to ocfs2.
>>>
>>> Unfortunately I'm not familiar with ocfs2 and cannot verify the patches locally.
>>>
>>> Usually you can observe following related problems:
>>> - read after lseek beyond end of file, described above by NeilBrown
>>>  "dd if=<AFFECTED_FILE> bs=1000 skip=1" will incorrectly generate whole last line
>>>
>>> - read after lseek on into middle of last line will output expected rest of
>>>  last line but then repeat whole last line once again. 
>>>
>>> - If .show() function generates multi-line output following bash script will never finish.
>>>
>>>  $ q=;while read -r r;do echo "$((++q)) $r";done < AFFECTED_FILE
>>>
>>> Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!NwJIBH5pk_PU0nl3GdikAyWr88AIZXuC-YDPoiMU1ltEcROIRtcSY_EHVaiSzeRD-Jpafw$ 
>>>
>>> Vasily Averin (4):
>>>   lockres_seq_next should increase position index
>>>   ocfs2_dlm_seq_next should increase position index
>>>   nst_seq_next should increase position index
>>>   sc_seq_next should increase position index
>>>
>>>  fs/ocfs2/cluster/netdebug.c | 2 ++
>>>  fs/ocfs2/dlm/dlmdebug.c     | 1 +
>>>  fs/ocfs2/dlmglue.c          | 1 +
>>>  3 files changed, 4 insertions(+)
>>>
>> .
>>
>>
>> .
>>
> 

[Ocfs2-devel] [PATCH v3 0/4] ocfs2: seq_operation should properly handle position index

[Vasily Averin]

v3: rework of ocfs2-related seq_operation functions to properly handle position index argument

v2: resend with improved patch description

In Aug 2018 NeilBrown noticed
commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
"Some ->next functions do not increment *pos when they return NULL...
Note that such ->next functions are buggy and should be fixed.
A simple demonstration is
   
dd if=/proc/swaps bs=1000 skip=1
  
Choose any block size larger than the size of /proc/swaps.  This will
always show the whole last line of /proc/swaps"

/proc/swaps output was fixed recently [1], however there are lot of other
affected files and 4 of them are related to ocfs2.

Moreover lishan@ experiments showed that ocfs2-related seq_operations
completely ignores position index argument and it leads to incorrect output. [2]

Usually you can observe following related problems:
- read after lseek beyond end of file, described above by NeilBrown
 "dd if=<AFFECTED_FILE> bs=1000 skip=1" will incorrectly generate whole last line

- read after lseek on into middle of last line will output expected rest of
 last line but then repeat whole last line once again.

- If .show() function generates multi-line output following bash script will never finish.

 $ q=;while read -r r;do echo "$((++q)) $r";done < AFFECTED_FILE

[1] Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!M3Zvpw7fOUG9cadeYNB9Nc7yQqx_CxPjQKa1xVL-HV09pGqXsi_4x0J6l5LuvKgjTUVSrQ$ 
[2] Link: https://oss.oracle.com/pipermail/ocfs2-devel/2020-March/014822.html

Vasily Averin (4):
  ocfs2: debug_lockres_ops should properly handle position index
  ocfs2: ocfs2_dlm_seq_ops should properly handle position index
  ocfs2: nst_seq_ops should properly handle position index
  ocfs2: sc_seq_ops should properly handle position index

 fs/ocfs2/cluster/netdebug.c | 24 +++++++-----
 fs/ocfs2/dlm/dlmdebug.c     | 78 ++++++++++++++++++-------------------
 fs/ocfs2/dlmglue.c          | 10 +++--
 3 files changed, 59 insertions(+), 53 deletions(-)

-- 
2.17.1

[Ocfs2-devel] [PATCH v3 1/4] ocfs2: debug_lockres_ops should properly handle position index

[Vasily Averin]

Currently debug_lockres_ops ignores position index argument,
and it leads to incorrect output in case of read with offset.
Link: https://oss.oracle.com/pipermail/ocfs2-devel/2020-March/014822.html

By design .start function should skip first *pos elements,
and .next function must update position index unconditionally.

debug_lockres_ops was reworked to satisfy these requirements:
- .start and .next functions iterates on dlm->tracking_list
    by using seq_list_* primitives
- .show generates output related to selected dlm_lock_resource
- .stop function is used to release taken dlm_lock_resource

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!KOf5kpSuieredwp9lEzV0BQklJAQy4ix8PUtCrqkLdv2TsSnX570XW7JT9gBvV6thJkj2g$ 
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ocfs2/dlm/dlmdebug.c | 78 ++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
index 4b8b41d23e91..1887affbbf2c 100644
--- a/fs/ocfs2/dlm/dlmdebug.c
+++ b/fs/ocfs2/dlm/dlmdebug.c
@@ -542,63 +542,63 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
 {
 	struct debug_lockres *dl = m->private;
 	struct dlm_ctxt *dlm = dl->dl_ctxt;
-	struct dlm_lock_resource *oldres = dl->dl_res;
 	struct dlm_lock_resource *res = NULL;
-	struct list_head *track_list;
+	struct list_head *lh;
 
 	spin_lock(&dlm->track_lock);
-	if (oldres)
-		track_list = &oldres->tracking;
-	else {
-		track_list = &dlm->tracking_list;
-		if (list_empty(track_list)) {
-			dl = NULL;
-			spin_unlock(&dlm->track_lock);
-			goto bail;
-		}
-	}
-
-	list_for_each_entry(res, track_list, tracking) {
-		if (&res->tracking == &dlm->tracking_list)
-			res = NULL;
-		else
-			dlm_lockres_get(res);
-		break;
+	lh = seq_list_start(&dlm->tracking_list, *pos);
+	if (lh) {
+		res = list_entry(lh, struct dlm_lock_resource, tracking);
+		dlm_lockres_get(res);
 	}
+	dl->dl_res = res;
 	spin_unlock(&dlm->track_lock);
+	/* passed to seq_show */
+	return res ? dl : NULL;
+}
 
-	if (oldres)
-		dlm_lockres_put(oldres);
+static void *lockres_seq_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	struct debug_lockres *dl = (struct debug_lockres *)v;
+	struct dlm_ctxt *dlm = dl->dl_ctxt;
+	struct dlm_lock_resource *oldres = dl->dl_res;
+	struct dlm_lock_resource *res = NULL;
+	struct list_head *lh;
 
+	spin_lock(&dlm->track_lock);
+	lh = seq_list_next(&oldres->tracking, &dlm->tracking_list, pos);
+	if (lh) {
+		res = list_entry(lh, struct dlm_lock_resource, tracking);
+		dlm_lockres_get(res);
+	}
 	dl->dl_res = res;
-
-	if (res) {
-		spin_lock(&res->spinlock);
-		dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
-		spin_unlock(&res->spinlock);
-	} else
-		dl = NULL;
-
-bail:
-	/* passed to seq_show */
-	return dl;
+	spin_unlock(&dlm->track_lock);
+	dlm_lockres_put(oldres);
+	return res ? dl : NULL;
 }
 
 static void lockres_seq_stop(struct seq_file *m, void *v)
 {
-}
+	struct debug_lockres *dl = (struct debug_lockres *)v;
+	struct dlm_lock_resource *res = dl->dl_res;
 
-static void *lockres_seq_next(struct seq_file *m, void *v, loff_t *pos)
-{
-	return NULL;
+	if (res) {
+		dlm_lockres_put(res);
+		dl->dl_res = NULL;
+	}
 }
 
-static int lockres_seq_show(struct seq_file *s, void *v)
+static int lockres_seq_show(struct seq_file *m, void *v)
 {
 	struct debug_lockres *dl = (struct debug_lockres *)v;
+	struct dlm_lock_resource *res = dl->dl_res;
 
-	seq_printf(s, "%s", dl->dl_buf);
-
+	if (res) {
+		spin_lock(&res->spinlock);
+		dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
+		spin_unlock(&res->spinlock);
+		seq_printf(m, "%s", dl->dl_buf);
+	}
 	return 0;
 }
 
-- 
2.17.1

[Ocfs2-devel] [PATCH v3 2/4] ocfs2: ocfs2_dlm_seq_ops should properly handle position index

[Vasily Averin]

Currently ocfs2_dlm_seq_ops ignores position index argument,
and it leads to incorrect output in case of read with offset.

By design .start function should skip first *pos elements,
and .next function must update position index unconditionally.

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!MW4at7q5bwj5ULIntJ6umw403Evx0mZ3_sIwjVdK1ltQi595oNDR7IyKESTtl61485TN7g$ 
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ocfs2/dlmglue.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
index 152a0fc4e905..3f5fe6af9f3a 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -3022,7 +3022,8 @@ struct ocfs2_dlm_seq_priv {
 };
 
 static struct ocfs2_lock_res *ocfs2_dlm_next_res(struct ocfs2_lock_res *start,
-						 struct ocfs2_dlm_seq_priv *priv)
+						 struct ocfs2_dlm_seq_priv *priv,
+						 loff_t pos)
 {
 	struct ocfs2_lock_res *iter, *ret = NULL;
 	struct ocfs2_dlm_debug *dlm_debug = priv->p_dlm_debug;
@@ -3038,7 +3039,7 @@ static struct ocfs2_lock_res *ocfs2_dlm_next_res(struct ocfs2_lock_res *start,
 
 		/* We track our "dummy" iteration lockres' by a NULL
 		 * l_ops field. */
-		if (iter->l_ops != NULL) {
+		if ((iter->l_ops != NULL) && (pos-- == 0)) {
 			ret = iter;
 			break;
 		}
@@ -3053,7 +3054,7 @@ static void *ocfs2_dlm_seq_start(struct seq_file *m, loff_t *pos)
 	struct ocfs2_lock_res *iter;
 
 	spin_lock(&ocfs2_dlm_tracking_lock);
-	iter = ocfs2_dlm_next_res(&priv->p_iter_res, priv);
+	iter = ocfs2_dlm_next_res(&priv->p_iter_res, priv, *pos);
 	if (iter) {
 		/* Since lockres' have the lifetime of their container
 		 * (which can be inodes, ocfs2_supers, etc) we want to
@@ -3080,8 +3081,9 @@ static void *ocfs2_dlm_seq_next(struct seq_file *m, void *v, loff_t *pos)
 	struct ocfs2_lock_res *iter = v;
 	struct ocfs2_lock_res *dummy = &priv->p_iter_res;
 
+	(*pos)++;
 	spin_lock(&ocfs2_dlm_tracking_lock);
-	iter = ocfs2_dlm_next_res(iter, priv);
+	iter = ocfs2_dlm_next_res(iter, priv, 0);
 	list_del_init(&dummy->l_debug_list);
 	if (iter) {
 		list_add(&dummy->l_debug_list, &iter->l_debug_list);
-- 
2.17.1

[Ocfs2-devel] [PATCH v3 3/4] ocfs2: nst_seq_ops should properly handle position index

[Vasily Averin]

Currently nst_seq_ops ignores position index argument,
and it leads to incorrect output in case of read with offset.

By design .start function should skip first *pos elements,
and .next function must update position index unconditionally.

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!McCQFYlmRnNmoAtY2bRAeiRzKSgC0c8B5r-Oy1meJpaRZaM5TbD5lECuzD96ZyERbIiA3Q$ 
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ocfs2/cluster/netdebug.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/ocfs2/cluster/netdebug.c b/fs/ocfs2/cluster/netdebug.c
index 667a5c5e1f66..8ca8a407781b 100644
--- a/fs/ocfs2/cluster/netdebug.c
+++ b/fs/ocfs2/cluster/netdebug.c
@@ -60,7 +60,8 @@ void o2net_debug_del_nst(struct o2net_send_tracking *nst)
 }
 
 static struct o2net_send_tracking
-			*next_nst(struct o2net_send_tracking *nst_start)
+			*next_nst(struct o2net_send_tracking *nst_start,
+				  loff_t pos)
 {
 	struct o2net_send_tracking *nst, *ret = NULL;
 
@@ -73,7 +74,7 @@ static struct o2net_send_tracking
 			break;
 
 		/* use st_task to detect real nsts in the list */
-		if (nst->st_task != NULL) {
+		if ((nst->st_task != NULL) && (pos-- == 0)) {
 			ret = nst;
 			break;
 		}
@@ -87,7 +88,7 @@ static void *nst_seq_start(struct seq_file *seq, loff_t *pos)
 	struct o2net_send_tracking *nst, *dummy_nst = seq->private;
 
 	spin_lock(&o2net_debug_lock);
-	nst = next_nst(dummy_nst);
+	nst = next_nst(dummy_nst, *pos);
 	spin_unlock(&o2net_debug_lock);
 
 	return nst;
@@ -97,8 +98,9 @@ static void *nst_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 {
 	struct o2net_send_tracking *nst, *dummy_nst = seq->private;
 
+	(*pos)++;
 	spin_lock(&o2net_debug_lock);
-	nst = next_nst(dummy_nst);
+	nst = next_nst(dummy_nst, 0);
 	list_del_init(&dummy_nst->st_net_debug_item);
 	if (nst)
 		list_add(&dummy_nst->st_net_debug_item,
@@ -115,7 +117,7 @@ static int nst_seq_show(struct seq_file *seq, void *v)
 	s64 sock, send, status;
 
 	spin_lock(&o2net_debug_lock);
-	nst = next_nst(dummy_nst);
+	nst = next_nst(dummy_nst, 0);
 	if (!nst)
 		goto out;
 
-- 
2.17.1

[Ocfs2-devel] [PATCH v3 4/4] ocfs2: sc_seq_ops should properly handle position index

[Vasily Averin]

Currently sc_seq_ops ignores position index argument,
and it leads to incorrect output in case of read with offset.

By design .start function should skip first *pos elements,
and .next function must update position index unconditionally.

Cc: stable at vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
Link: https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=206283__;!!GqivPVa7Brio!MJIyl9g4rZXkCRJpUtfNR70zABIYk0zKHrl00gKDdYZpx41lpyanlE7z50esOWCIqgd95w$ 
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/ocfs2/cluster/netdebug.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/ocfs2/cluster/netdebug.c b/fs/ocfs2/cluster/netdebug.c
index 8ca8a407781b..832fd762e4ef 100644
--- a/fs/ocfs2/cluster/netdebug.c
+++ b/fs/ocfs2/cluster/netdebug.c
@@ -213,7 +213,8 @@ struct o2net_sock_debug {
 };
 
 static struct o2net_sock_container
-			*next_sc(struct o2net_sock_container *sc_start)
+			*next_sc(struct o2net_sock_container *sc_start,
+				 loff_t pos)
 {
 	struct o2net_sock_container *sc, *ret = NULL;
 
@@ -226,7 +227,7 @@ static struct o2net_sock_container
 			break;
 
 		/* use sc_page to detect real scs in the list */
-		if (sc->sc_page != NULL) {
+		if ((sc->sc_page != NULL) && (pos-- == 0)) {
 			ret = sc;
 			break;
 		}
@@ -241,7 +242,7 @@ static void *sc_seq_start(struct seq_file *seq, loff_t *pos)
 	struct o2net_sock_container *sc, *dummy_sc = sd->dbg_sock;
 
 	spin_lock(&o2net_debug_lock);
-	sc = next_sc(dummy_sc);
+	sc = next_sc(dummy_sc, *pos);
 	spin_unlock(&o2net_debug_lock);
 
 	return sc;
@@ -252,8 +253,9 @@ static void *sc_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 	struct o2net_sock_debug *sd = seq->private;
 	struct o2net_sock_container *sc, *dummy_sc = sd->dbg_sock;
 
+	++(*pos);
 	spin_lock(&o2net_debug_lock);
-	sc = next_sc(dummy_sc);
+	sc = next_sc(dummy_sc, 0);
 	list_del_init(&dummy_sc->sc_net_debug_item);
 	if (sc)
 		list_add(&dummy_sc->sc_net_debug_item, &sc->sc_net_debug_item);
@@ -354,7 +356,7 @@ static int sc_seq_show(struct seq_file *seq, void *v)
 	struct o2net_sock_container *sc, *dummy_sc = sd->dbg_sock;
 
 	spin_lock(&o2net_debug_lock);
-	sc = next_sc(dummy_sc);
+	sc = next_sc(dummy_sc, 0);
 
 	if (sc) {
 		if (sd->dbg_ctxt == SHOW_SOCK_CONTAINERS)
-- 
2.17.1