[Cluster-devel] [PATCH] gfs2: Save caller ip in gfs2_glock_nq_init

[Cluster-devel] [PATCH] gfs2: Save caller ip in gfs2_glock_nq_init

[Bob Peterson]

Before this patch, when a glock was locked by function gfs2_glock_nq_init,
it initializes the holder gh_ip (return address) as gfs2_glock_nq_init.
That made it extremely difficult to track down problems because many
functions call gfs2_glock_nq_init. This patch changes the function so
that it saves gh_ip from the caller of gfs2_glock_nq_init, which makes
it easy to backtrack which holder took the lock.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 699c5e95006a..8a09379dbf66 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -240,6 +240,7 @@ static inline int gfs2_glock_nq_init(struct gfs2_glock *gl,
 	int error;
 
 	gfs2_holder_init(gl, state, flags, gh);
+	gh->gh_ip = _RET_IP_;
 
 	error = gfs2_glock_nq(gh);
 	if (error)
-- 
2.31.1

[Cluster-devel] [PATCH] gfs2: Save caller ip in gfs2_glock_nq_init

[Andreas Gruenbacher]

Hi Bob,

On Wed, Sep 29, 2021 at 3:16 PM Bob Peterson <rpeterso@redhat.com> wrote:
> Before this patch, when a glock was locked by function gfs2_glock_nq_init,
> it initializes the holder gh_ip (return address) as gfs2_glock_nq_init.
> That made it extremely difficult to track down problems because many
> functions call gfs2_glock_nq_init. This patch changes the function so
> that it saves gh_ip from the caller of gfs2_glock_nq_init, which makes
> it easy to backtrack which holder took the lock.

looks reasonable, but I think we can achieve this more cleanly by
passing _RET_IP_ to gfs2_holder_init as below.  We want the same kind of
treatment for gfs2_glock_poke as well, at least theoretically.

Thanks,
Andreas

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 6dfd33dc206b..ebf541c82635 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -829,7 +829,7 @@ static void gfs2_glock_poke(struct gfs2_glock *gl)
 	struct gfs2_holder gh;
 	int error;
 
-	gfs2_holder_init(gl, LM_ST_SHARED, flags, &gh);
+	__gfs2_holder_init(gl, LM_ST_SHARED, flags, &gh, _RET_IP_);
 	error = gfs2_glock_nq(&gh);
 	if (!error)
 		gfs2_glock_dq(&gh);
@@ -1126,12 +1126,12 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
  *
  */
 
-void gfs2_holder_init(struct gfs2_glock *gl, unsigned int state, u16 flags,
-		      struct gfs2_holder *gh)
+void __gfs2_holder_init(struct gfs2_glock *gl, unsigned int state, u16 flags,
+			struct gfs2_holder *gh, unsigned long ip)
 {
 	INIT_LIST_HEAD(&gh->gh_list);
 	gh->gh_gl = gl;
-	gh->gh_ip = _RET_IP_;
+	gh->gh_ip = ip;
 	gh->gh_owner_pid = get_pid(task_pid(current));
 	gh->gh_state = state;
 	gh->gh_flags = flags;
diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 699c5e95006a..c2b8cc7f5544 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -188,8 +188,15 @@ extern int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
 extern void gfs2_glock_hold(struct gfs2_glock *gl);
 extern void gfs2_glock_put(struct gfs2_glock *gl);
 extern void gfs2_glock_queue_put(struct gfs2_glock *gl);
-extern void gfs2_holder_init(struct gfs2_glock *gl, unsigned int state,
-			     u16 flags, struct gfs2_holder *gh);
+extern void __gfs2_holder_init(struct gfs2_glock *gl, unsigned int state,
+			       u16 flags, struct gfs2_holder *gh,
+			       unsigned long ip);
+
+static inline void gfs2_holder_init(struct gfs2_glock *gl, unsigned int state,
+				    u16 flags, struct gfs2_holder *gh) {
+	__gfs2_holder_init(gl, state, flags, gh, _RET_IP_);
+}
+
 extern void gfs2_holder_reinit(unsigned int state, u16 flags,
 			       struct gfs2_holder *gh);
 extern void gfs2_holder_uninit(struct gfs2_holder *gh);
@@ -239,7 +246,7 @@ static inline int gfs2_glock_nq_init(struct gfs2_glock *gl,
 {
 	int error;
 
-	gfs2_holder_init(gl, state, flags, gh);
+	__gfs2_holder_init(gl, state, flags, gh, _RET_IP_);
 
 	error = gfs2_glock_nq(gh);
 	if (error)
-- 
2.26.3

[Cluster-devel] [PATCH] gfs2: Save caller ip in gfs2_glock_nq_init

[Bob Peterson]

On 9/29/21 9:47 AM, Andreas Gruenbacher wrote:
> Hi Bob,
> 
> On Wed, Sep 29, 2021 at 3:16 PM Bob Peterson <rpeterso@redhat.com> wrote:
>> Before this patch, when a glock was locked by function gfs2_glock_nq_init,
>> it initializes the holder gh_ip (return address) as gfs2_glock_nq_init.
>> That made it extremely difficult to track down problems because many
>> functions call gfs2_glock_nq_init. This patch changes the function so
>> that it saves gh_ip from the caller of gfs2_glock_nq_init, which makes
>> it easy to backtrack which holder took the lock.
> 
> looks reasonable, but I think we can achieve this more cleanly by
> passing _RET_IP_ to gfs2_holder_init as below.  We want the same kind of
> treatment for gfs2_glock_poke as well, at least theoretically.
> 
> Thanks,
> Andreas
> 
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index 6dfd33dc206b..ebf541c82635 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -829,7 +829,7 @@ static void gfs2_glock_poke(struct gfs2_glock *gl)
>   	struct gfs2_holder gh;
>   	int error;
>   
> -	gfs2_holder_init(gl, LM_ST_SHARED, flags, &gh);
> +	__gfs2_holder_init(gl, LM_ST_SHARED, flags, &gh, _RET_IP_);
>   	error = gfs2_glock_nq(&gh);
>   	if (!error)
>   		gfs2_glock_dq(&gh);
> @@ -1126,12 +1126,12 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
>    *
>    */
>   
> -void gfs2_holder_init(struct gfs2_glock *gl, unsigned int state, u16 flags,
> -		      struct gfs2_holder *gh)
> +void __gfs2_holder_init(struct gfs2_glock *gl, unsigned int state, u16 flags,
> +			struct gfs2_holder *gh, unsigned long ip)
>   {
>   	INIT_LIST_HEAD(&gh->gh_list);
>   	gh->gh_gl = gl;
> -	gh->gh_ip = _RET_IP_;
> +	gh->gh_ip = ip;
>   	gh->gh_owner_pid = get_pid(task_pid(current));
>   	gh->gh_state = state;
>   	gh->gh_flags = flags;
> diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
> index 699c5e95006a..c2b8cc7f5544 100644
> --- a/fs/gfs2/glock.h
> +++ b/fs/gfs2/glock.h
> @@ -188,8 +188,15 @@ extern int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
>   extern void gfs2_glock_hold(struct gfs2_glock *gl);
>   extern void gfs2_glock_put(struct gfs2_glock *gl);
>   extern void gfs2_glock_queue_put(struct gfs2_glock *gl);
> -extern void gfs2_holder_init(struct gfs2_glock *gl, unsigned int state,
> -			     u16 flags, struct gfs2_holder *gh);
> +extern void __gfs2_holder_init(struct gfs2_glock *gl, unsigned int state,
> +			       u16 flags, struct gfs2_holder *gh,
> +			       unsigned long ip);
> +
> +static inline void gfs2_holder_init(struct gfs2_glock *gl, unsigned int state,
> +				    u16 flags, struct gfs2_holder *gh) {
> +	__gfs2_holder_init(gl, state, flags, gh, _RET_IP_);
> +}
> +
>   extern void gfs2_holder_reinit(unsigned int state, u16 flags,
>   			       struct gfs2_holder *gh);
>   extern void gfs2_holder_uninit(struct gfs2_holder *gh);
> @@ -239,7 +246,7 @@ static inline int gfs2_glock_nq_init(struct gfs2_glock *gl,
>   {
>   	int error;
>   
> -	gfs2_holder_init(gl, state, flags, gh);
> +	__gfs2_holder_init(gl, state, flags, gh, _RET_IP_);
>   
>   	error = gfs2_glock_nq(gh);
>   	if (error)
> 
Hi Andreas,

I'm not sure I would describe your patch as "more clean" than my 1 line
of code, but I do like your patch, and it makes sense to do it that way.

Can you please add my:
Reviewed-by: Bob Peterson <rpeterso@redhat.com>

and push that? While you're at it, please drop the v1 version of
my patch 9f4754923d87d2 "gfs2: introduce and use new glops 
go_lock_needed" which is replaced by my most recently posted patch.

Looks like I accidentally dropped the "gfs2 PATCH v2" from the subject:
sorry. Or after you review it, if that patch is okay, I can push it
myself. Thanks.

Bob Peterson