Vasily Averin
2020-Mar-22 07:27 UTC
[Ocfs2-devel] Fwd: Re: [PATCH v2 0/4] ocfs2: seq_file .next functions should increase position index
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 at linux.alibaba.com> >> To: Vasily Averin <vvs at virtuozzo.com>, ocfs2-devel at oss.oracle.com, piaojun <piaojun at huawei.com> >> CC: Mark Fasheh <mark at fasheh.com>, Joel Becker <jlbec at 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(+) >>> >> . >> >> >> . >> >
Vasily Averin
2020-Apr-14 06:16 UTC
[Ocfs2-devel] [PATCH v3 0/4] ocfs2: seq_operation should properly handle position index
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
Vasily Averin
2020-Apr-14 06:17 UTC
[Ocfs2-devel] [PATCH v3 1/4] ocfs2: debug_lockres_ops should properly handle position index
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 at 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
Vasily Averin
2020-Apr-14 06:17 UTC
[Ocfs2-devel] [PATCH v3 2/4] ocfs2: ocfs2_dlm_seq_ops should properly handle position index
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 at 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
Vasily Averin
2020-Apr-14 06:17 UTC
[Ocfs2-devel] [PATCH v3 3/4] ocfs2: nst_seq_ops should properly handle position index
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 at 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
Vasily Averin
2020-Apr-14 06:17 UTC
[Ocfs2-devel] [PATCH v3 4/4] ocfs2: sc_seq_ops should properly handle position index
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 at 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