* [PATCH] gfs2: Stop using rhashtable_walk_peek
@ 2018-03-28 16:00 ` Andreas Gruenbacher
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Gruenbacher @ 2018-03-28 16:00 UTC (permalink / raw)
To: cluster-devel
Cc: Andreas Gruenbacher, netdev, linux-kernel, NeilBrown,
Thomas Graf, Herbert Xu, Tom Herbert
Function rhashtable_walk_peek is problematic because there is no
guarantee that the glock previously returned still exists; when that key
is deleted, rhashtable_walk_peek can end up returning a different key,
which would cause an inconsistent glock dump. So instead of using
rhashtable_walk_peek, keep track of the current glock in the seq file
iterator functions.
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
fs/gfs2/glock.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 82fb5583445c..f1fc353875d3 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -55,6 +55,7 @@ struct gfs2_glock_iter {
struct gfs2_sbd *sdp; /* incore superblock */
struct rhashtable_iter hti; /* rhashtable iterator */
struct gfs2_glock *gl; /* current glock struct */
+ bool gl_held;
loff_t last_pos; /* last position */
};
@@ -1923,9 +1924,11 @@ void gfs2_glock_exit(void)
static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi, loff_t n)
{
- if (n == 0)
- gi->gl = rhashtable_walk_peek(&gi->hti);
- else {
+ if (n != 0 || !gi->gl) {
+ if (gi->gl_held) {
+ gfs2_glock_queue_put(gi->gl);
+ gi->gl_held = false;
+ }
gi->gl = rhashtable_walk_next(&gi->hti);
n--;
}
@@ -1988,7 +1991,10 @@ static void gfs2_glock_seq_stop(struct seq_file *seq, void *iter_ptr)
{
struct gfs2_glock_iter *gi = seq->private;
- gi->gl = NULL;
+ if (gi->gl) {
+ lockref_get(&gi->gl->gl_lockref);
+ gi->gl_held = true;
+ }
rhashtable_walk_stop(&gi->hti);
}
@@ -2061,6 +2067,7 @@ static int __gfs2_glocks_open(struct inode *inode, struct file *file,
*/
gi->last_pos = -1;
gi->gl = NULL;
+ gi->gl_held = false;
rhashtable_walk_enter(&gl_hash_table, &gi->hti);
}
return ret;
@@ -2076,7 +2083,8 @@ static int gfs2_glocks_release(struct inode *inode, struct file *file)
struct seq_file *seq = file->private_data;
struct gfs2_glock_iter *gi = seq->private;
- gi->gl = NULL;
+ if (gi->gl_held)
+ gfs2_glock_put(gi->gl);
rhashtable_walk_exit(&gi->hti);
return seq_release_private(inode, file);
}
--
2.14.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Cluster-devel] [PATCH] gfs2: Stop using rhashtable_walk_peek
@ 2018-03-28 16:00 ` Andreas Gruenbacher
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Gruenbacher @ 2018-03-28 16:00 UTC (permalink / raw)
To: cluster-devel.redhat.com
Function rhashtable_walk_peek is problematic because there is no
guarantee that the glock previously returned still exists; when that key
is deleted, rhashtable_walk_peek can end up returning a different key,
which would cause an inconsistent glock dump. So instead of using
rhashtable_walk_peek, keep track of the current glock in the seq file
iterator functions.
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
fs/gfs2/glock.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 82fb5583445c..f1fc353875d3 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -55,6 +55,7 @@ struct gfs2_glock_iter {
struct gfs2_sbd *sdp; /* incore superblock */
struct rhashtable_iter hti; /* rhashtable iterator */
struct gfs2_glock *gl; /* current glock struct */
+ bool gl_held;
loff_t last_pos; /* last position */
};
@@ -1923,9 +1924,11 @@ void gfs2_glock_exit(void)
static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi, loff_t n)
{
- if (n == 0)
- gi->gl = rhashtable_walk_peek(&gi->hti);
- else {
+ if (n != 0 || !gi->gl) {
+ if (gi->gl_held) {
+ gfs2_glock_queue_put(gi->gl);
+ gi->gl_held = false;
+ }
gi->gl = rhashtable_walk_next(&gi->hti);
n--;
}
@@ -1988,7 +1991,10 @@ static void gfs2_glock_seq_stop(struct seq_file *seq, void *iter_ptr)
{
struct gfs2_glock_iter *gi = seq->private;
- gi->gl = NULL;
+ if (gi->gl) {
+ lockref_get(&gi->gl->gl_lockref);
+ gi->gl_held = true;
+ }
rhashtable_walk_stop(&gi->hti);
}
@@ -2061,6 +2067,7 @@ static int __gfs2_glocks_open(struct inode *inode, struct file *file,
*/
gi->last_pos = -1;
gi->gl = NULL;
+ gi->gl_held = false;
rhashtable_walk_enter(&gl_hash_table, &gi->hti);
}
return ret;
@@ -2076,7 +2083,8 @@ static int gfs2_glocks_release(struct inode *inode, struct file *file)
struct seq_file *seq = file->private_data;
struct gfs2_glock_iter *gi = seq->private;
- gi->gl = NULL;
+ if (gi->gl_held)
+ gfs2_glock_put(gi->gl);
rhashtable_walk_exit(&gi->hti);
return seq_release_private(inode, file);
}
--
2.14.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] gfs2: Stop using rhashtable_walk_peek
2018-03-28 16:00 ` [Cluster-devel] " Andreas Gruenbacher
@ 2018-03-28 21:53 ` NeilBrown
-1 siblings, 0 replies; 6+ messages in thread
From: NeilBrown @ 2018-03-28 21:53 UTC (permalink / raw)
To: Andreas Gruenbacher, cluster-devel
Cc: netdev, linux-kernel, Thomas Graf, Herbert Xu, Tom Herbert
[-- Attachment #1: Type: text/plain, Size: 2913 bytes --]
On Wed, Mar 28 2018, Andreas Gruenbacher wrote:
> Function rhashtable_walk_peek is problematic because there is no
> guarantee that the glock previously returned still exists; when that key
> is deleted, rhashtable_walk_peek can end up returning a different key,
> which would cause an inconsistent glock dump. So instead of using
> rhashtable_walk_peek, keep track of the current glock in the seq file
> iterator functions.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
> fs/gfs2/glock.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index 82fb5583445c..f1fc353875d3 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -55,6 +55,7 @@ struct gfs2_glock_iter {
> struct gfs2_sbd *sdp; /* incore superblock */
> struct rhashtable_iter hti; /* rhashtable iterator */
> struct gfs2_glock *gl; /* current glock struct */
> + bool gl_held;
> loff_t last_pos; /* last position */
> };
>
> @@ -1923,9 +1924,11 @@ void gfs2_glock_exit(void)
>
> static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi, loff_t n)
> {
> - if (n == 0)
> - gi->gl = rhashtable_walk_peek(&gi->hti);
> - else {
> + if (n != 0 || !gi->gl) {
> + if (gi->gl_held) {
> + gfs2_glock_queue_put(gi->gl);
> + gi->gl_held = false;
> + }
> gi->gl = rhashtable_walk_next(&gi->hti);
> n--;
> }
Thank for this patch!
The above looks a bit fragile to me.
gfs2_glock_iter_next() (And hence gfs2_glock_seq_start()) will sometimes
exit with gl_held true, and sometimes with it false.
gfs2_glock_seq_stop() assumes that it is false.
Normally gfs2_glock_seq_next() will normally be called between these
two and will clear gl_held, but I don't think there is a hard guarantee
of that.
Maybe we should always 'put' gi->gl in iter_next if gl_held??
Thanks,
NeilBrown
> @@ -1988,7 +1991,10 @@ static void gfs2_glock_seq_stop(struct seq_file *seq, void *iter_ptr)
> {
> struct gfs2_glock_iter *gi = seq->private;
>
> - gi->gl = NULL;
> + if (gi->gl) {
> + lockref_get(&gi->gl->gl_lockref);
> + gi->gl_held = true;
> + }
> rhashtable_walk_stop(&gi->hti);
> }
>
> @@ -2061,6 +2067,7 @@ static int __gfs2_glocks_open(struct inode *inode, struct file *file,
> */
> gi->last_pos = -1;
> gi->gl = NULL;
> + gi->gl_held = false;
> rhashtable_walk_enter(&gl_hash_table, &gi->hti);
> }
> return ret;
> @@ -2076,7 +2083,8 @@ static int gfs2_glocks_release(struct inode *inode, struct file *file)
> struct seq_file *seq = file->private_data;
> struct gfs2_glock_iter *gi = seq->private;
>
> - gi->gl = NULL;
> + if (gi->gl_held)
> + gfs2_glock_put(gi->gl);
> rhashtable_walk_exit(&gi->hti);
> return seq_release_private(inode, file);
> }
> --
> 2.14.3
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Cluster-devel] [PATCH] gfs2: Stop using rhashtable_walk_peek
@ 2018-03-28 21:53 ` NeilBrown
0 siblings, 0 replies; 6+ messages in thread
From: NeilBrown @ 2018-03-28 21:53 UTC (permalink / raw)
To: cluster-devel.redhat.com
On Wed, Mar 28 2018, Andreas Gruenbacher wrote:
> Function rhashtable_walk_peek is problematic because there is no
> guarantee that the glock previously returned still exists; when that key
> is deleted, rhashtable_walk_peek can end up returning a different key,
> which would cause an inconsistent glock dump. So instead of using
> rhashtable_walk_peek, keep track of the current glock in the seq file
> iterator functions.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
> fs/gfs2/glock.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index 82fb5583445c..f1fc353875d3 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -55,6 +55,7 @@ struct gfs2_glock_iter {
> struct gfs2_sbd *sdp; /* incore superblock */
> struct rhashtable_iter hti; /* rhashtable iterator */
> struct gfs2_glock *gl; /* current glock struct */
> + bool gl_held;
> loff_t last_pos; /* last position */
> };
>
> @@ -1923,9 +1924,11 @@ void gfs2_glock_exit(void)
>
> static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi, loff_t n)
> {
> - if (n == 0)
> - gi->gl = rhashtable_walk_peek(&gi->hti);
> - else {
> + if (n != 0 || !gi->gl) {
> + if (gi->gl_held) {
> + gfs2_glock_queue_put(gi->gl);
> + gi->gl_held = false;
> + }
> gi->gl = rhashtable_walk_next(&gi->hti);
> n--;
> }
Thank for this patch!
The above looks a bit fragile to me.
gfs2_glock_iter_next() (And hence gfs2_glock_seq_start()) will sometimes
exit with gl_held true, and sometimes with it false.
gfs2_glock_seq_stop() assumes that it is false.
Normally gfs2_glock_seq_next() will normally be called between these
two and will clear gl_held, but I don't think there is a hard guarantee
of that.
Maybe we should always 'put' gi->gl in iter_next if gl_held??
Thanks,
NeilBrown
> @@ -1988,7 +1991,10 @@ static void gfs2_glock_seq_stop(struct seq_file *seq, void *iter_ptr)
> {
> struct gfs2_glock_iter *gi = seq->private;
>
> - gi->gl = NULL;
> + if (gi->gl) {
> + lockref_get(&gi->gl->gl_lockref);
> + gi->gl_held = true;
> + }
> rhashtable_walk_stop(&gi->hti);
> }
>
> @@ -2061,6 +2067,7 @@ static int __gfs2_glocks_open(struct inode *inode, struct file *file,
> */
> gi->last_pos = -1;
> gi->gl = NULL;
> + gi->gl_held = false;
> rhashtable_walk_enter(&gl_hash_table, &gi->hti);
> }
> return ret;
> @@ -2076,7 +2083,8 @@ static int gfs2_glocks_release(struct inode *inode, struct file *file)
> struct seq_file *seq = file->private_data;
> struct gfs2_glock_iter *gi = seq->private;
>
> - gi->gl = NULL;
> + if (gi->gl_held)
> + gfs2_glock_put(gi->gl);
> rhashtable_walk_exit(&gi->hti);
> return seq_release_private(inode, file);
> }
> --
> 2.14.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20180329/cd2b1142/attachment.sig>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gfs2: Stop using rhashtable_walk_peek
2018-03-28 21:53 ` [Cluster-devel] " NeilBrown
@ 2018-03-28 22:58 ` Andreas Gruenbacher
-1 siblings, 0 replies; 6+ messages in thread
From: Andreas Gruenbacher @ 2018-03-28 22:58 UTC (permalink / raw)
To: NeilBrown
Cc: cluster-devel, netdev, LKML, Thomas Graf, Herbert Xu, Tom Herbert
On 28 March 2018 at 23:53, NeilBrown <neilb@suse.com> wrote:
> Thank for this patch!
> The above looks a bit fragile to me.
> gfs2_glock_iter_next() (And hence gfs2_glock_seq_start()) will sometimes
> exit with gl_held true, and sometimes with it false.
> gfs2_glock_seq_stop() assumes that it is false.
> Normally gfs2_glock_seq_next() will normally be called between these
> two and will clear gl_held, but I don't think there is a hard guarantee
> of that.
> Maybe we should always 'put' gi->gl in iter_next if gl_held??
Another problem is that gfs2_glock_seq_stop only does a lockref_get
which is not allowed because the lockref may have been marked dead in
the meantime. We'll have to take a reference on each lock in
gfs2_glock_iter_next instead. Revised patch to follow tomorrow.
Thanks,
Andreas
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Cluster-devel] [PATCH] gfs2: Stop using rhashtable_walk_peek
@ 2018-03-28 22:58 ` Andreas Gruenbacher
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Gruenbacher @ 2018-03-28 22:58 UTC (permalink / raw)
To: cluster-devel.redhat.com
On 28 March 2018 at 23:53, NeilBrown <neilb@suse.com> wrote:
> Thank for this patch!
> The above looks a bit fragile to me.
> gfs2_glock_iter_next() (And hence gfs2_glock_seq_start()) will sometimes
> exit with gl_held true, and sometimes with it false.
> gfs2_glock_seq_stop() assumes that it is false.
> Normally gfs2_glock_seq_next() will normally be called between these
> two and will clear gl_held, but I don't think there is a hard guarantee
> of that.
> Maybe we should always 'put' gi->gl in iter_next if gl_held??
Another problem is that gfs2_glock_seq_stop only does a lockref_get
which is not allowed because the lockref may have been marked dead in
the meantime. We'll have to take a reference on each lock in
gfs2_glock_iter_next instead. Revised patch to follow tomorrow.
Thanks,
Andreas
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-28 22:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-28 16:00 [PATCH] gfs2: Stop using rhashtable_walk_peek Andreas Gruenbacher
2018-03-28 16:00 ` [Cluster-devel] " Andreas Gruenbacher
2018-03-28 21:53 ` NeilBrown
2018-03-28 21:53 ` [Cluster-devel] " NeilBrown
2018-03-28 22:58 ` Andreas Gruenbacher
2018-03-28 22:58 ` [Cluster-devel] " Andreas Gruenbacher
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.