From 60c6273131330a57b3eb55233d3706f48e4c4b3e Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Fri, 25 Jul 2025 15:09:00 +0200 Subject: [PATCH 01/20] gfs2: Remove unused GIF_FREE_VFS_INODE flag Since commit 38552ff676f0 ("gfs2: Fix and clean up create / evict interaction"), the GIF_FREE_VFS_INODE flag isn't used anymore. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/incore.h | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h index d4ad82f47eee..e3a8a28acb99 100644 --- a/fs/gfs2/incore.h +++ b/fs/gfs2/incore.h @@ -376,7 +376,6 @@ struct gfs2_glock { enum { GIF_QD_LOCKED = 1, GIF_SW_PAGED = 3, - GIF_FREE_VFS_INODE = 5, GIF_GLOP_PENDING = 6, }; From 37b1c0f120b78f804f588ce78cfbd5a2df352a33 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Fri, 25 Jul 2025 23:41:37 +0200 Subject: [PATCH 02/20] gfs2: Remove unused sd_withdraw_wait field This field was introduced in commit 601ef0d52e96 ("gfs2: Force withdraw to replay journals and wait for it to finish") and never used. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/incore.h | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h index e3a8a28acb99..78a00239e384 100644 --- a/fs/gfs2/incore.h +++ b/fs/gfs2/incore.h @@ -822,7 +822,6 @@ struct gfs2_sbd { atomic_t sd_log_in_flight; wait_queue_head_t sd_log_flush_wait; int sd_log_error; /* First log error */ - wait_queue_head_t sd_withdraw_wait; unsigned int sd_log_tail; unsigned int sd_log_flush_tail; From aa94ad9ab230d08741e6630a20fd1296b52c1009 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Tue, 29 Jul 2025 12:36:38 +0100 Subject: [PATCH 03/20] gfs2: Remove space before newline There is an extraneous space before a newline in a fs_err message. Remove it Signed-off-by: Colin Ian King Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index b6fd1cb17de7..09af1fe12503 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -733,7 +733,7 @@ __acquires(&gl->gl_lockref.lock) */ if (ret) { if (cmpxchg(&sdp->sd_log_error, 0, ret)) { - fs_err(sdp, "Error %d syncing glock \n", ret); + fs_err(sdp, "Error %d syncing glock\n", ret); gfs2_dump_glock(NULL, gl, true); } spin_lock(&gl->gl_lockref.lock); From 2309a01351e56446f43c89e200d643647d47e739 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Tue, 8 Jul 2025 19:13:32 +0200 Subject: [PATCH 04/20] gfs2: do_xmote cleanup Check for asynchronous completion and clear the GLF_PENDING_REPLY flag earlier in do_xmote(). This will make future changes more readable. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 09af1fe12503..f1383e9445be 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -816,6 +816,12 @@ __acquires(&gl->gl_lockref.lock) ret = ls->ls_ops->lm_lock(gl, target, lck_flags); spin_lock(&gl->gl_lockref.lock); + if (!ret) { + /* The operation will be completed asynchronously. */ + return; + } + clear_bit(GLF_PENDING_REPLY, &gl->gl_flags); + if (ret == -EINVAL && gl->gl_target == LM_ST_UNLOCKED && target == LM_ST_UNLOCKED && test_bit(DFL_UNMOUNT, &ls->ls_recover_flags)) { @@ -823,14 +829,10 @@ __acquires(&gl->gl_lockref.lock) * The lockspace has been released and the lock has * been unlocked implicitly. */ - } else if (ret) { + } else { fs_err(sdp, "lm_lock ret %d\n", ret); target = gl->gl_state | LM_OUT_ERROR; - } else { - /* The operation will be completed asynchronously. */ - return; } - clear_bit(GLF_PENDING_REPLY, &gl->gl_flags); } /* Complete the operation now. */ From 4250e683de69a637b93f7c7bda7818b36b1cf32e Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Sat, 2 Aug 2025 23:41:24 +0200 Subject: [PATCH 05/20] gfs2: Simplify refcounting in do_xmote In do_xmote(), take the additional glock references close to where those references are needed. This will simplify the next commit. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index f1383e9445be..8a7f947883cd 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -760,7 +760,6 @@ __acquires(&gl->gl_lockref.lock) spin_lock(&gl->gl_lockref.lock); skip_inval: - gl->gl_lockref.count++; /* * Check for an error encountered since we called go_sync and go_inval. * If so, we can't withdraw from the glock code because the withdraw @@ -803,6 +802,7 @@ __acquires(&gl->gl_lockref.lock) if (!test_bit(GLF_CANCELING, &gl->gl_flags)) clear_bit(GLF_LOCK, &gl->gl_flags); clear_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags); + gl->gl_lockref.count++; gfs2_glock_queue_work(gl, GL_GLOCK_DFT_HOLD); return; } else { @@ -818,6 +818,7 @@ __acquires(&gl->gl_lockref.lock) if (!ret) { /* The operation will be completed asynchronously. */ + gl->gl_lockref.count++; return; } clear_bit(GLF_PENDING_REPLY, &gl->gl_flags); @@ -837,6 +838,7 @@ __acquires(&gl->gl_lockref.lock) /* Complete the operation now. */ finish_xmote(gl, target); + gl->gl_lockref.count++; gfs2_glock_queue_work(gl, 0); } From 418c854759341cdf687240bd064de09ce38b92c5 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Tue, 8 Jul 2025 22:07:17 +0200 Subject: [PATCH 06/20] gfs2: Partially revert "gfs2: do_xmote fixes" When the lm_lock hook which calls dlm_lock() returns an error, do_xmote() previously reported the error to the syslog ("lm_lock ret %d\n") but otherwise ignored it during withdraws. Commit 9947a06d29c0 ("gfs2: do_xmote fixes") changed that to pass the error on to the glock layer, but the error would then only result in an unconditional BUG() in finish_xmote(), which doesn't help. Instead, revert to the previous behavior. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 8a7f947883cd..5bdb11de5b13 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -832,7 +832,8 @@ __acquires(&gl->gl_lockref.lock) */ } else { fs_err(sdp, "lm_lock ret %d\n", ret); - target = gl->gl_state | LM_OUT_ERROR; + GLOCK_BUG_ON(gl, !gfs2_withdrawing_or_withdrawn(sdp)); + return; } } From 6e4224082696940d57c9129ed3aeb5e69904a5b2 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Tue, 5 Aug 2025 19:51:47 +0200 Subject: [PATCH 07/20] gfs2: Turn gfs2_withdraw into a void function Since commit 601ef0d52e96 ("gfs2: Force withdraw to replay journals and wait for it to finish"), gfs2_withdraw() always returns -1, so turn it into a straight void function. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/util.c | 21 ++++++++------------- fs/gfs2/util.h | 2 +- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c index 24864a66074b..020dd21ad51e 100644 --- a/fs/gfs2/util.c +++ b/fs/gfs2/util.c @@ -309,7 +309,7 @@ void gfs2_lm(struct gfs2_sbd *sdp, const char *fmt, ...) va_end(args); } -int gfs2_withdraw(struct gfs2_sbd *sdp) +void gfs2_withdraw(struct gfs2_sbd *sdp) { struct lm_lockstruct *ls = &sdp->sd_lockstruct; const struct lm_lockops *lm = ls->ls_ops; @@ -322,7 +322,7 @@ int gfs2_withdraw(struct gfs2_sbd *sdp) wait_on_bit(&sdp->sd_flags, SDF_WITHDRAW_IN_PROG, TASK_UNINTERRUPTIBLE); - return -1; + return; } new = old | BIT(SDF_WITHDRAWN) | BIT(SDF_WITHDRAW_IN_PROG); } while (unlikely(!try_cmpxchg(&sdp->sd_flags, &old, new))); @@ -350,8 +350,6 @@ int gfs2_withdraw(struct gfs2_sbd *sdp) if (sdp->sd_args.ar_errors == GFS2_ERRORS_PANIC) panic("GFS2: fsid=%s: panic requested\n", sdp->sd_fsname); - - return -1; } /* @@ -481,16 +479,14 @@ int gfs2_meta_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, const char *function, char *file, unsigned int line) { - int me; - gfs2_lm(sdp, "fatal: invalid metadata block - " "bh = %llu (bad magic number), " "function = %s, file = %s, line = %u\n", (unsigned long long)bh->b_blocknr, function, file, line); - me = gfs2_withdraw(sdp); - return (me) ? -1 : -2; + gfs2_withdraw(sdp); + return -1; } /* @@ -503,16 +499,14 @@ int gfs2_metatype_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, u16 type, u16 t, const char *function, char *file, unsigned int line) { - int me; - gfs2_lm(sdp, "fatal: invalid metadata block - " "bh = %llu (type: exp=%u, found=%u), " "function = %s, file = %s, line = %u\n", (unsigned long long)bh->b_blocknr, type, t, function, file, line); - me = gfs2_withdraw(sdp); - return (me) ? -1 : -2; + gfs2_withdraw(sdp); + return -1; } /* @@ -528,7 +522,8 @@ int gfs2_io_error_i(struct gfs2_sbd *sdp, const char *function, char *file, "fatal: I/O error - " "function = %s, file = %s, line = %u\n", function, file, line); - return gfs2_withdraw(sdp); + gfs2_withdraw(sdp); + return -1; } /* diff --git a/fs/gfs2/util.h b/fs/gfs2/util.h index 27d03b641024..2a8a1fac611a 100644 --- a/fs/gfs2/util.h +++ b/fs/gfs2/util.h @@ -228,6 +228,6 @@ gfs2_tune_get_i(&(sdp)->sd_tune, &(sdp)->sd_tune.field) __printf(2, 3) void gfs2_lm(struct gfs2_sbd *sdp, const char *fmt, ...); -int gfs2_withdraw(struct gfs2_sbd *sdp); +void gfs2_withdraw(struct gfs2_sbd *sdp); #endif /* __UTIL_DOT_H__ */ From 13c0004168633845fba328edf2cdec0de0292307 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Tue, 5 Aug 2025 20:08:35 +0200 Subject: [PATCH 08/20] gfs2: Sanitize gfs2_meta_check, gfs2_metatype_check, gfs2_io_error Change those functions to either return a useful value, or nothing at all. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/util.c | 23 ++++++++--------------- fs/gfs2/util.h | 34 +++++++++++++++++++--------------- 2 files changed, 27 insertions(+), 30 deletions(-) diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c index 020dd21ad51e..56412f63f3bb 100644 --- a/fs/gfs2/util.c +++ b/fs/gfs2/util.c @@ -471,13 +471,11 @@ void gfs2_consist_rgrpd_i(struct gfs2_rgrpd *rgd, /* * gfs2_meta_check_ii - Flag a magic number consistency error and withdraw - * Returns: -1 if this call withdrew the machine, - * -2 if it was already withdrawn */ -int gfs2_meta_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, - const char *function, char *file, - unsigned int line) +void gfs2_meta_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, + const char *function, char *file, + unsigned int line) { gfs2_lm(sdp, "fatal: invalid metadata block - " @@ -486,18 +484,15 @@ int gfs2_meta_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, (unsigned long long)bh->b_blocknr, function, file, line); gfs2_withdraw(sdp); - return -1; } /* * gfs2_metatype_check_ii - Flag a metadata type consistency error and withdraw - * Returns: -1 if this call withdrew the machine, - * -2 if it was already withdrawn */ -int gfs2_metatype_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, - u16 type, u16 t, const char *function, - char *file, unsigned int line) +void gfs2_metatype_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, + u16 type, u16 t, const char *function, + char *file, unsigned int line) { gfs2_lm(sdp, "fatal: invalid metadata block - " @@ -506,7 +501,6 @@ int gfs2_metatype_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, (unsigned long long)bh->b_blocknr, type, t, function, file, line); gfs2_withdraw(sdp); - return -1; } /* @@ -515,15 +509,14 @@ int gfs2_metatype_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, * 0 if it was already withdrawn */ -int gfs2_io_error_i(struct gfs2_sbd *sdp, const char *function, char *file, - unsigned int line) +void gfs2_io_error_i(struct gfs2_sbd *sdp, const char *function, char *file, + unsigned int line) { gfs2_lm(sdp, "fatal: I/O error - " "function = %s, file = %s, line = %u\n", function, file, line); gfs2_withdraw(sdp); - return -1; } /* diff --git a/fs/gfs2/util.h b/fs/gfs2/util.h index 2a8a1fac611a..da0373b1e82b 100644 --- a/fs/gfs2/util.h +++ b/fs/gfs2/util.h @@ -91,9 +91,9 @@ void gfs2_consist_rgrpd_i(struct gfs2_rgrpd *rgd, gfs2_consist_rgrpd_i((rgd), __func__, __FILE__, __LINE__) -int gfs2_meta_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, - const char *function, - char *file, unsigned int line); +void gfs2_meta_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, + const char *function, + char *file, unsigned int line); static inline int gfs2_meta_check(struct gfs2_sbd *sdp, struct buffer_head *bh) @@ -108,10 +108,10 @@ static inline int gfs2_meta_check(struct gfs2_sbd *sdp, return 0; } -int gfs2_metatype_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, - u16 type, u16 t, - const char *function, - char *file, unsigned int line); +void gfs2_metatype_check_ii(struct gfs2_sbd *sdp, struct buffer_head *bh, + u16 type, u16 t, + const char *function, + char *file, unsigned int line); static inline int gfs2_metatype_check_i(struct gfs2_sbd *sdp, struct buffer_head *bh, @@ -122,12 +122,16 @@ static inline int gfs2_metatype_check_i(struct gfs2_sbd *sdp, struct gfs2_meta_header *mh = (struct gfs2_meta_header *)bh->b_data; u32 magic = be32_to_cpu(mh->mh_magic); u16 t = be32_to_cpu(mh->mh_type); - if (unlikely(magic != GFS2_MAGIC)) - return gfs2_meta_check_ii(sdp, bh, function, - file, line); - if (unlikely(t != type)) - return gfs2_metatype_check_ii(sdp, bh, type, t, function, - file, line); + if (unlikely(magic != GFS2_MAGIC)) { + gfs2_meta_check_ii(sdp, bh, function, + file, line); + return -EIO; + } + if (unlikely(t != type)) { + gfs2_metatype_check_ii(sdp, bh, type, t, function, + file, line); + return -EIO; + } return 0; } @@ -144,8 +148,8 @@ static inline void gfs2_metatype_set(struct buffer_head *bh, u16 type, } -int gfs2_io_error_i(struct gfs2_sbd *sdp, const char *function, - char *file, unsigned int line); +void gfs2_io_error_i(struct gfs2_sbd *sdp, const char *function, + char *file, unsigned int line); int check_journal_clean(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd, bool verbose); From cd718046646593ced5ad98f9cde22aaf2a2eb8f2 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Tue, 5 Aug 2025 01:58:47 +0200 Subject: [PATCH 09/20] gfs2: Do not use atomic operations unnecessarily The GLF_DEMOTE_IN_PROGRESS and GLF_LOCK flags and the glock refcount are all protected by the glock spin lock, so there is no need for atomic operations / barriers here. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 5bdb11de5b13..1ced38b9a5a2 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -646,8 +646,10 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret) } /* Fast path - we got what we asked for */ - if (test_and_clear_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags)) + if (test_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags)) { + clear_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags); gfs2_demote_wake(gl); + } if (gl->gl_state != LM_ST_UNLOCKED) { if (glops->go_xmote_bh) { int rv; @@ -891,14 +893,12 @@ __acquires(&gl->gl_lockref.lock) out_sched: clear_bit(GLF_LOCK, &gl->gl_flags); - smp_mb__after_atomic(); gl->gl_lockref.count++; gfs2_glock_queue_work(gl, 0); return; out_unlock: clear_bit(GLF_LOCK, &gl->gl_flags); - smp_mb__after_atomic(); } /** From fd70ab7155c4b92a9747d42c02791a0793ab9c66 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Thu, 7 Aug 2025 10:21:33 +0200 Subject: [PATCH 10/20] gfs2: Further sanitize lock_dlm.c The gl_req field and GLF_BLOCKING flag are only relevant to gdlm_lock(), its callback gdlm_ast(), and their helpers, so set and clear them inside lock_dlm.c. Also, the LM_FLAG_ANY flag is relevant to gdlm_lock(), but do_xmote() doesn't pass that flag down to gdlm_lock() as it should. Fix that by passing down all the flags. In addition, document the effect of the LM_FLAG_ANY flag on locks held in EX mode locally. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 6 ------ fs/gfs2/glock.h | 4 ++++ fs/gfs2/lock_dlm.c | 26 ++++++++++++++++++-------- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 1ced38b9a5a2..595891015415 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -717,12 +717,6 @@ __acquires(&gl->gl_lockref.lock) return; do_error(gl, 0); /* Fail queued try locks */ } - gl->gl_req = target; - set_bit(GLF_BLOCKING, &gl->gl_flags); - if ((gl->gl_req == LM_ST_UNLOCKED) || - (gl->gl_state == LM_ST_EXCLUSIVE) || - (lck_flags & (LM_FLAG_TRY|LM_FLAG_TRY_1CB))) - clear_bit(GLF_BLOCKING, &gl->gl_flags); if (!glops->go_inval && !glops->go_sync) goto skip_inval; diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h index 9339a3bff6ee..d041b922b45e 100644 --- a/fs/gfs2/glock.h +++ b/fs/gfs2/glock.h @@ -68,6 +68,10 @@ enum { * also be granted in SHARED. The preferred state is whichever is compatible * with other granted locks, or the specified state if no other locks exist. * + * In addition, when a lock is already held in EX mode locally, a SHARED or + * DEFERRED mode request with the LM_FLAG_ANY flag set will be granted. + * (The LM_FLAG_ANY flag is only use for SHARED mode requests currently.) + * * LM_FLAG_NODE_SCOPE * This holder agrees to share the lock within this node. In other words, * the glock is held in EX mode according to DLM, but local holders on the diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c index cee5d199d2d8..5daaeaaaf18d 100644 --- a/fs/gfs2/lock_dlm.c +++ b/fs/gfs2/lock_dlm.c @@ -58,6 +58,7 @@ static inline void gfs2_update_stats(struct gfs2_lkstats *s, unsigned index, /** * gfs2_update_reply_times - Update locking statistics * @gl: The glock to update + * @blocking: The operation may have been blocking * * This assumes that gl->gl_dstamp has been set earlier. * @@ -72,12 +73,12 @@ static inline void gfs2_update_stats(struct gfs2_lkstats *s, unsigned index, * TRY_1CB flags are set are classified as non-blocking. All * other DLM requests are counted as (potentially) blocking. */ -static inline void gfs2_update_reply_times(struct gfs2_glock *gl) +static inline void gfs2_update_reply_times(struct gfs2_glock *gl, + bool blocking) { struct gfs2_pcpu_lkstats *lks; const unsigned gltype = gl->gl_name.ln_type; - unsigned index = test_bit(GLF_BLOCKING, &gl->gl_flags) ? - GFS2_LKS_SRTTB : GFS2_LKS_SRTT; + unsigned index = blocking ? GFS2_LKS_SRTTB : GFS2_LKS_SRTT; s64 rtt; preempt_disable(); @@ -119,14 +120,18 @@ static inline void gfs2_update_request_times(struct gfs2_glock *gl) static void gdlm_ast(void *arg) { struct gfs2_glock *gl = arg; + bool blocking; unsigned ret; + blocking = test_bit(GLF_BLOCKING, &gl->gl_flags); + gfs2_update_reply_times(gl, blocking); + clear_bit(GLF_BLOCKING, &gl->gl_flags); + /* If the glock is dead, we only react to a dlm_unlock() reply. */ if (__lockref_is_dead(&gl->gl_lockref) && gl->gl_lksb.sb_status != -DLM_EUNLOCK) return; - gfs2_update_reply_times(gl); BUG_ON(gl->gl_lksb.sb_flags & DLM_SBF_DEMOTED); if ((gl->gl_lksb.sb_flags & DLM_SBF_VALNOTVALID) && gl->gl_lksb.sb_lvbptr) @@ -241,7 +246,7 @@ static bool down_conversion(int cur, int req) } static u32 make_flags(struct gfs2_glock *gl, const unsigned int gfs_flags, - const int cur, const int req) + const int req, bool blocking) { u32 lkf = 0; @@ -274,7 +279,7 @@ static u32 make_flags(struct gfs2_glock *gl, const unsigned int gfs_flags, * "upward" lock conversions or else DLM will reject the * request as invalid. */ - if (!down_conversion(cur, req)) + if (blocking) lkf |= DLM_LKF_QUECVT; } @@ -294,14 +299,20 @@ static int gdlm_lock(struct gfs2_glock *gl, unsigned int req_state, unsigned int flags) { struct lm_lockstruct *ls = &gl->gl_name.ln_sbd->sd_lockstruct; + bool blocking; int cur, req; u32 lkf; char strname[GDLM_STRNAME_BYTES] = ""; int error; + gl->gl_req = req_state; cur = make_mode(gl->gl_name.ln_sbd, gl->gl_state); req = make_mode(gl->gl_name.ln_sbd, req_state); - lkf = make_flags(gl, flags, cur, req); + blocking = !down_conversion(cur, req) && + !(flags & (LM_FLAG_TRY|LM_FLAG_TRY_1CB)); + lkf = make_flags(gl, flags, req, blocking); + if (blocking) + set_bit(GLF_BLOCKING, &gl->gl_flags); gfs2_glstats_inc(gl, GFS2_LKS_DCOUNT); gfs2_sbstats_inc(gl, GFS2_LKS_DCOUNT); if (test_bit(GLF_INITIAL, &gl->gl_flags)) { @@ -341,7 +352,6 @@ static void gdlm_put_lock(struct gfs2_glock *gl) return; } - clear_bit(GLF_BLOCKING, &gl->gl_flags); gfs2_glstats_inc(gl, GFS2_LKS_DCOUNT); gfs2_sbstats_inc(gl, GFS2_LKS_DCOUNT); gfs2_update_request_times(gl); From 2b813a72880dcc90ab5997e2307f961b5a8650bd Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Thu, 7 Aug 2025 10:21:33 +0200 Subject: [PATCH 11/20] gfs2: Remove DLM_LKF_ALTCW / DLM_LKF_ALTPR code Commit 6802e3400ff45 ("[GFS2] Clean up the glock core") stopped passing the LM_FLAG_ANY flag down to gdlm_lock() (then gfs2_lm_lock()). Since then, gfs2 effectively hasn't been using dlm's DLM_LKF_ALTCW / DLM_LKF_ALTPR flags, but the code still suggests that it does. Recent testing shows that those flags don't even work reliably anymore, so instead of fixing code that hasn't been used since 2008, remove it. In addition, clean up how the flags are passed to [gd]lm_lock(). Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 4 +--- fs/gfs2/lock_dlm.c | 17 ----------------- 2 files changed, 1 insertion(+), 20 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 595891015415..6294a9f36318 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -695,14 +695,12 @@ __acquires(&gl->gl_lockref.lock) const struct gfs2_glock_operations *glops = gl->gl_ops; struct gfs2_sbd *sdp = gl->gl_name.ln_sbd; struct lm_lockstruct *ls = &sdp->sd_lockstruct; - unsigned int lck_flags = (unsigned int)(gh ? gh->gh_flags : 0); int ret; if (target != LM_ST_UNLOCKED && glock_blocked_by_withdraw(gl) && gh && !(gh->gh_flags & LM_FLAG_NOEXP)) goto skip_inval; - lck_flags &= (LM_FLAG_TRY | LM_FLAG_TRY_1CB | LM_FLAG_NOEXP); GLOCK_BUG_ON(gl, gl->gl_state == target); GLOCK_BUG_ON(gl, gl->gl_state == gl->gl_target); if ((target == LM_ST_UNLOCKED || target == LM_ST_DEFERRED) && @@ -809,7 +807,7 @@ __acquires(&gl->gl_lockref.lock) if (ls->ls_ops->lm_lock) { set_bit(GLF_PENDING_REPLY, &gl->gl_flags); spin_unlock(&gl->gl_lockref.lock); - ret = ls->ls_ops->lm_lock(gl, target, lck_flags); + ret = ls->ls_ops->lm_lock(gl, target, gh ? gh->gh_flags : 0); spin_lock(&gl->gl_lockref.lock); if (!ret) { diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c index 5daaeaaaf18d..427150a206cf 100644 --- a/fs/gfs2/lock_dlm.c +++ b/fs/gfs2/lock_dlm.c @@ -162,14 +162,6 @@ static void gdlm_ast(void *arg) } ret = gl->gl_req; - if (gl->gl_lksb.sb_flags & DLM_SBF_ALTMODE) { - if (gl->gl_req == LM_ST_SHARED) - ret = LM_ST_DEFERRED; - else if (gl->gl_req == LM_ST_DEFERRED) - ret = LM_ST_SHARED; - else - BUG(); - } /* * The GLF_INITIAL flag is initially set for new glocks. Upon the @@ -261,15 +253,6 @@ static u32 make_flags(struct gfs2_glock *gl, const unsigned int gfs_flags, lkf |= DLM_LKF_NOQUEUEBAST; } - if (gfs_flags & LM_FLAG_ANY) { - if (req == DLM_LOCK_PR) - lkf |= DLM_LKF_ALTCW; - else if (req == DLM_LOCK_CW) - lkf |= DLM_LKF_ALTPR; - else - BUG(); - } - if (!test_bit(GLF_INITIAL, &gl->gl_flags)) { lkf |= DLM_LKF_CONVERT; From 0c23e24164d83086e75581b0cf930f4e161636d6 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Fri, 8 Aug 2025 22:31:59 +0200 Subject: [PATCH 12/20] gfs2: Fix LM_FLAG_TRY* logic in add_to_queue The logic in add_to_queue() for determining whether a LM_FLAG_TRY or LM_FLAG_TRY_1CB holder should be queued does not make any sense: we are interested in wether or not the new operation will block behind an existing or future holder in the queue, but the current code checks for ongoing locking or ->go_inval() operations, which has little to do with that. Replace that code with something more sensible, remove the incorrect add_to_queue() function annotations, remove the similarly misguided do_error(gl, 0) call in do_xmote(), and add a missing comment to the same call in do_promote(). Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 6294a9f36318..e754214fdb1c 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -502,7 +502,7 @@ static bool do_promote(struct gfs2_glock *gl) */ if (list_is_first(&gh->gh_list, &gl->gl_holders)) return false; - do_error(gl, 0); + do_error(gl, 0); /* Fail queued try locks */ break; } set_bit(HIF_HOLDER, &gh->gh_iflags); @@ -713,7 +713,6 @@ __acquires(&gl->gl_lockref.lock) if (test_and_set_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags)) return; - do_error(gl, 0); /* Fail queued try locks */ } if (!glops->go_inval && !glops->go_sync) goto skip_inval; @@ -1459,6 +1458,24 @@ void gfs2_print_dbg(struct seq_file *seq, const char *fmt, ...) va_end(args); } +static bool gfs2_should_queue_trylock(struct gfs2_glock *gl, + struct gfs2_holder *gh) +{ + struct gfs2_holder *current_gh, *gh2; + + current_gh = find_first_holder(gl); + if (current_gh && !may_grant(gl, current_gh, gh)) + return false; + + list_for_each_entry(gh2, &gl->gl_holders, gh_list) { + if (test_bit(HIF_HOLDER, &gh2->gh_iflags)) + continue; + if (!(gh2->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB))) + return false; + } + return true; +} + static inline bool pid_is_meaningful(const struct gfs2_holder *gh) { if (!(gh->gh_flags & GL_NOPID)) @@ -1477,27 +1494,20 @@ static inline bool pid_is_meaningful(const struct gfs2_holder *gh) */ static inline void add_to_queue(struct gfs2_holder *gh) -__releases(&gl->gl_lockref.lock) -__acquires(&gl->gl_lockref.lock) { struct gfs2_glock *gl = gh->gh_gl; struct gfs2_sbd *sdp = gl->gl_name.ln_sbd; struct gfs2_holder *gh2; - int try_futile = 0; GLOCK_BUG_ON(gl, gh->gh_owner_pid == NULL); if (test_and_set_bit(HIF_WAIT, &gh->gh_iflags)) GLOCK_BUG_ON(gl, true); - if (gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB)) { - if (test_bit(GLF_LOCK, &gl->gl_flags)) { - struct gfs2_holder *current_gh; - - current_gh = find_first_holder(gl); - try_futile = !may_grant(gl, current_gh, gh); - } - if (test_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags)) - goto fail; + if ((gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB)) && + !gfs2_should_queue_trylock(gl, gh)) { + gh->gh_error = GLR_TRYFAILED; + gfs2_holder_wake(gh); + return; } list_for_each_entry(gh2, &gl->gl_holders, gh_list) { @@ -1509,15 +1519,6 @@ __acquires(&gl->gl_lockref.lock) continue; goto trap_recursive; } - list_for_each_entry(gh2, &gl->gl_holders, gh_list) { - if (try_futile && - !(gh2->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB))) { -fail: - gh->gh_error = GLR_TRYFAILED; - gfs2_holder_wake(gh); - return; - } - } trace_gfs2_glock_queue(gh, 1); gfs2_glstats_inc(gl, GFS2_LKS_QCOUNT); gfs2_sbstats_inc(gl, GFS2_LKS_QCOUNT); From 9b54770b68ae793a3a8d378be4cda2bb7be6c8cc Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Fri, 8 Aug 2025 23:18:45 +0200 Subject: [PATCH 13/20] gfs2: Remove duplicate check in do_xmote In do_xmote(), remove the duplicate check for the ->go_sync and ->go_inval glock operations. They are either both defined, or none of them are. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index e754214fdb1c..15a90ab8a979 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -714,25 +714,24 @@ __acquires(&gl->gl_lockref.lock) &gl->gl_flags)) return; } - if (!glops->go_inval && !glops->go_sync) + if (!glops->go_inval || !glops->go_sync) goto skip_inval; spin_unlock(&gl->gl_lockref.lock); - if (glops->go_sync) { - ret = glops->go_sync(gl); - /* If we had a problem syncing (due to io errors or whatever, - * we should not invalidate the metadata or tell dlm to - * release the glock to other nodes. - */ - if (ret) { - if (cmpxchg(&sdp->sd_log_error, 0, ret)) { - fs_err(sdp, "Error %d syncing glock\n", ret); - gfs2_dump_glock(NULL, gl, true); - } - spin_lock(&gl->gl_lockref.lock); - goto skip_inval; + ret = glops->go_sync(gl); + /* If we had a problem syncing (due to io errors or whatever, + * we should not invalidate the metadata or tell dlm to + * release the glock to other nodes. + */ + if (ret) { + if (cmpxchg(&sdp->sd_log_error, 0, ret)) { + fs_err(sdp, "Error %d syncing glock\n", ret); + gfs2_dump_glock(NULL, gl, true); } + spin_lock(&gl->gl_lockref.lock); + goto skip_inval; } + if (test_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags)) { /* * The call to go_sync should have cleared out the ail list. From 061df28b82af6b22fb5fa529a8f2ef00474ee004 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Sat, 2 Aug 2025 23:57:24 +0200 Subject: [PATCH 14/20] gfs2: Fix GLF_INVALIDATE_IN_PROGRESS flag clearing in do_xmote Commit 865cc3e9cc0b ("gfs2: fix a deadlock on withdraw-during-mount") added a statement to do_xmote() to clear the GLF_INVALIDATE_IN_PROGRESS flag a second time after it has already been cleared. Fix that. Fixes: 865cc3e9cc0b ("gfs2: fix a deadlock on withdraw-during-mount") Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 15a90ab8a979..5361a2641cdb 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -797,8 +797,6 @@ __acquires(&gl->gl_lockref.lock) gl->gl_lockref.count++; gfs2_glock_queue_work(gl, GL_GLOCK_DFT_HOLD); return; - } else { - clear_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags); } } From bddb53b776fb7ce81dfba7c24884d9f2c0c68e50 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Fri, 8 Aug 2025 23:26:12 +0200 Subject: [PATCH 15/20] gfs2: Get rid of GLF_INVALIDATE_IN_PROGRESS Get rid of the GLF_INVALIDATE_IN_PROGRESS flag: it was originally used to indicate to add_to_queue() that the ->go_sync() and ->go_invalid() operations were in progress, but as we have established in commit "gfs2: Fix LM_FLAG_TRY* logic in add_to_queue", add_to_queue() has no need to know. Commit d99724c3c36a describes a race in which GLF_INVALIDATE_IN_PROGRESS is used to serialize two processes which are both in do_xmote() at the same time. That analysis is wrong: the serialization happens via the GLF_LOCK flag, which ensures that at most one glock operation can be active at any time. Fixes: d99724c3c36a ("gfs2: Close timing window with GLF_INVALIDATE_IN_PROGRESS") Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 16 +--------------- fs/gfs2/incore.h | 1 - fs/gfs2/trace_gfs2.h | 1 - 3 files changed, 1 insertion(+), 17 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 5361a2641cdb..e8b630a58da8 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -703,17 +703,6 @@ __acquires(&gl->gl_lockref.lock) GLOCK_BUG_ON(gl, gl->gl_state == target); GLOCK_BUG_ON(gl, gl->gl_state == gl->gl_target); - if ((target == LM_ST_UNLOCKED || target == LM_ST_DEFERRED) && - glops->go_inval) { - /* - * If another process is already doing the invalidate, let that - * finish first. The glock state machine will get back to this - * holder again later. - */ - if (test_and_set_bit(GLF_INVALIDATE_IN_PROGRESS, - &gl->gl_flags)) - return; - } if (!glops->go_inval || !glops->go_sync) goto skip_inval; @@ -732,7 +721,7 @@ __acquires(&gl->gl_lockref.lock) goto skip_inval; } - if (test_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags)) { + if (target == LM_ST_UNLOCKED || target == LM_ST_DEFERRED) { /* * The call to go_sync should have cleared out the ail list. * If there are still items, we have a problem. We ought to @@ -747,7 +736,6 @@ __acquires(&gl->gl_lockref.lock) gfs2_dump_glock(NULL, gl, true); } glops->go_inval(gl, target == LM_ST_DEFERRED ? 0 : DIO_METADATA); - clear_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags); } spin_lock(&gl->gl_lockref.lock); @@ -2316,8 +2304,6 @@ static const char *gflags2str(char *buf, const struct gfs2_glock *gl) *p++ = 'y'; if (test_bit(GLF_LFLUSH, gflags)) *p++ = 'f'; - if (test_bit(GLF_INVALIDATE_IN_PROGRESS, gflags)) - *p++ = 'i'; if (test_bit(GLF_PENDING_REPLY, gflags)) *p++ = 'R'; if (test_bit(GLF_HAVE_REPLY, gflags)) diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h index 78a00239e384..cd8a3f469291 100644 --- a/fs/gfs2/incore.h +++ b/fs/gfs2/incore.h @@ -319,7 +319,6 @@ enum { GLF_DEMOTE_IN_PROGRESS = 5, GLF_DIRTY = 6, GLF_LFLUSH = 7, - GLF_INVALIDATE_IN_PROGRESS = 8, GLF_HAVE_REPLY = 9, GLF_INITIAL = 10, GLF_HAVE_FROZEN_REPLY = 11, diff --git a/fs/gfs2/trace_gfs2.h b/fs/gfs2/trace_gfs2.h index 26036ffc3f33..1c2507a27318 100644 --- a/fs/gfs2/trace_gfs2.h +++ b/fs/gfs2/trace_gfs2.h @@ -52,7 +52,6 @@ {(1UL << GLF_DEMOTE_IN_PROGRESS), "p" }, \ {(1UL << GLF_DIRTY), "y" }, \ {(1UL << GLF_LFLUSH), "f" }, \ - {(1UL << GLF_INVALIDATE_IN_PROGRESS), "i" }, \ {(1UL << GLF_PENDING_REPLY), "R" }, \ {(1UL << GLF_HAVE_REPLY), "r" }, \ {(1UL << GLF_INITIAL), "a" }, \ From 2045364497dbb16663df0267d6f733d964f22866 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Mon, 11 Aug 2025 22:34:50 +0200 Subject: [PATCH 16/20] gfs2: Simplify do_promote While not immediately obvious, do_promote() returns whether or not there are any active holders in the queue. But the function description is confusing, and this information is easy to come by for callers anyway, so turn do_promote() into a void function. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index e8b630a58da8..38f320e13a36 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -481,11 +481,9 @@ int gfs2_instantiate(struct gfs2_holder *gh) /** * do_promote - promote as many requests as possible on the current queue * @gl: The glock - * - * Returns true on success (i.e., progress was made or there are no waiters). */ -static bool do_promote(struct gfs2_glock *gl) +static void do_promote(struct gfs2_glock *gl) { struct gfs2_holder *gh, *current_gh; @@ -496,13 +494,10 @@ static bool do_promote(struct gfs2_glock *gl) if (!may_grant(gl, current_gh, gh)) { /* * If we get here, it means we may not grant this - * holder for some reason. If this holder is at the - * head of the list, it means we have a blocked holder - * at the head, so return false. + * holder for some reason. */ - if (list_is_first(&gh->gh_list, &gl->gl_holders)) - return false; - do_error(gl, 0); /* Fail queued try locks */ + if (current_gh) + do_error(gl, 0); /* Fail queued try locks */ break; } set_bit(HIF_HOLDER, &gh->gh_iflags); @@ -511,7 +506,6 @@ static bool do_promote(struct gfs2_glock *gl) if (!current_gh) current_gh = gh; } - return true; } /** @@ -855,7 +849,8 @@ __acquires(&gl->gl_lockref.lock) } else { if (test_bit(GLF_DEMOTE, &gl->gl_flags)) gfs2_demote_wake(gl); - if (do_promote(gl)) + do_promote(gl); + if (find_first_holder(gl)) goto out_unlock; gh = find_first_waiter(gl); if (!gh) From cd493dcf4f827700b66d1adb43f20d3838759beb Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Mon, 11 Aug 2025 21:35:23 +0200 Subject: [PATCH 17/20] gfs2: run_queue cleanup Transform the code in run_queue() to make it more readable. No change in functionality. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 38f320e13a36..1eba27bb5f3c 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -835,8 +835,12 @@ __acquires(&gl->gl_lockref.lock) /* While a demote is in progress, the GLF_LOCK flag must be set. */ GLOCK_BUG_ON(gl, test_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags)); - if (test_bit(GLF_DEMOTE, &gl->gl_flags) && - gl->gl_demote_state != gl->gl_state) { + if (test_bit(GLF_DEMOTE, &gl->gl_flags)) { + if (gl->gl_demote_state == gl->gl_state) { + gfs2_demote_wake(gl); + goto promote; + } + if (find_first_holder(gl)) goto out_unlock; if (nonblock) @@ -846,22 +850,21 @@ __acquires(&gl->gl_lockref.lock) gl->gl_target = gl->gl_demote_state; do_xmote(gl, NULL, gl->gl_target); return; - } else { - if (test_bit(GLF_DEMOTE, &gl->gl_flags)) - gfs2_demote_wake(gl); - do_promote(gl); - if (find_first_holder(gl)) - goto out_unlock; - gh = find_first_waiter(gl); - if (!gh) - goto out_unlock; - gl->gl_target = gh->gh_state; - if (!(gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB))) - do_error(gl, 0); /* Fail queued try locks */ - do_xmote(gl, gh, gl->gl_target); - return; } +promote: + do_promote(gl); + if (find_first_holder(gl)) + goto out_unlock; + gh = find_first_waiter(gl); + if (!gh) + goto out_unlock; + gl->gl_target = gh->gh_state; + if (!(gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB))) + do_error(gl, 0); /* Fail queued try locks */ + do_xmote(gl, gh, gl->gl_target); + return; + out_sched: clear_bit(GLF_LOCK, &gl->gl_flags); gl->gl_lockref.count++; From 47faf937da43df9a84e65e5a880cf813e764971e Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Mon, 11 Aug 2025 21:38:23 +0200 Subject: [PATCH 18/20] gfs2: Minor run_queue fixes Provide a better description of why the GLF_DEMOTE_IN_PROGRESS flag cannot be set. Function do_xmote() may block, so make sure it isn't called when nonblock is true. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/glock.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 1eba27bb5f3c..747ee7ca44e2 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -832,7 +832,12 @@ __acquires(&gl->gl_lockref.lock) return; set_bit(GLF_LOCK, &gl->gl_flags); - /* While a demote is in progress, the GLF_LOCK flag must be set. */ + /* + * The GLF_DEMOTE_IN_PROGRESS flag is only set intermittently during + * locking operations. We have just started a locking operation by + * setting the GLF_LOCK flag, so the GLF_DEMOTE_IN_PROGRESS flag must + * be cleared. + */ GLOCK_BUG_ON(gl, test_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags)); if (test_bit(GLF_DEMOTE, &gl->gl_flags)) { @@ -859,6 +864,8 @@ __acquires(&gl->gl_lockref.lock) gh = find_first_waiter(gl); if (!gh) goto out_unlock; + if (nonblock) + goto out_sched; gl->gl_target = gh->gh_state; if (!(gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB))) do_error(gl, 0); /* Fail queued try locks */ From 6ab26555c9ffef96c56ca16356e55ac5ab61ec93 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Mon, 7 Jul 2025 14:31:41 +0200 Subject: [PATCH 19/20] gfs2: Add proper lockspace locking GFS2 has been calling functions like dlm_lock() even after the lockspace that these functions operate on has been released with dlm_release_lockspace(). It has always assumed that those functions would return -EINVAL in that case, but that was never guaranteed, and it certainly is no longer the case since commit 4db41bf4f04f ("dlm: remove ls_local_handle from struct dlm_ls"). To fix that, add proper lockspace locking. Fixes: 3e11e5304150 ("GFS2: ignore unlock failures after withdraw") Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/file.c | 23 +++++++++++++++-------- fs/gfs2/glock.c | 5 ++--- fs/gfs2/incore.h | 2 ++ fs/gfs2/lock_dlm.c | 46 +++++++++++++++++++++++++++++++++++++--------- 4 files changed, 56 insertions(+), 20 deletions(-) diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index 72d95185a39f..bc67fa058c84 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -1442,6 +1442,7 @@ static int gfs2_lock(struct file *file, int cmd, struct file_lock *fl) struct gfs2_inode *ip = GFS2_I(file->f_mapping->host); struct gfs2_sbd *sdp = GFS2_SB(file->f_mapping->host); struct lm_lockstruct *ls = &sdp->sd_lockstruct; + int ret; if (!(fl->c.flc_flags & FL_POSIX)) return -ENOLCK; @@ -1450,14 +1451,20 @@ static int gfs2_lock(struct file *file, int cmd, struct file_lock *fl) locks_lock_file_wait(file, fl); return -EIO; } - if (cmd == F_CANCELLK) - return dlm_posix_cancel(ls->ls_dlm, ip->i_no_addr, file, fl); - else if (IS_GETLK(cmd)) - return dlm_posix_get(ls->ls_dlm, ip->i_no_addr, file, fl); - else if (lock_is_unlock(fl)) - return dlm_posix_unlock(ls->ls_dlm, ip->i_no_addr, file, fl); - else - return dlm_posix_lock(ls->ls_dlm, ip->i_no_addr, file, cmd, fl); + down_read(&ls->ls_sem); + ret = -ENODEV; + if (likely(ls->ls_dlm != NULL)) { + if (cmd == F_CANCELLK) + ret = dlm_posix_cancel(ls->ls_dlm, ip->i_no_addr, file, fl); + else if (IS_GETLK(cmd)) + ret = dlm_posix_get(ls->ls_dlm, ip->i_no_addr, file, fl); + else if (lock_is_unlock(fl)) + ret = dlm_posix_unlock(ls->ls_dlm, ip->i_no_addr, file, fl); + else + ret = dlm_posix_lock(ls->ls_dlm, ip->i_no_addr, file, cmd, fl); + } + up_read(&ls->ls_sem); + return ret; } static void __flock_holder_uninit(struct file *file, struct gfs2_holder *fl_gh) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 747ee7ca44e2..b677c0e6b9ab 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -795,9 +795,8 @@ __acquires(&gl->gl_lockref.lock) } clear_bit(GLF_PENDING_REPLY, &gl->gl_flags); - if (ret == -EINVAL && gl->gl_target == LM_ST_UNLOCKED && - target == LM_ST_UNLOCKED && - test_bit(DFL_UNMOUNT, &ls->ls_recover_flags)) { + if (ret == -ENODEV && gl->gl_target == LM_ST_UNLOCKED && + target == LM_ST_UNLOCKED) { /* * The lockspace has been released and the lock has * been unlocked implicitly. diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h index cd8a3f469291..5a0ea416cfda 100644 --- a/fs/gfs2/incore.h +++ b/fs/gfs2/incore.h @@ -656,6 +656,8 @@ struct lm_lockstruct { struct completion ls_sync_wait; /* {control,mounted}_{lock,unlock} */ char *ls_lvb_bits; + struct rw_semaphore ls_sem; + spinlock_t ls_recover_spin; /* protects following fields */ unsigned long ls_recover_flags; /* DFL_ */ uint32_t ls_recover_mount; /* gen in first recover_done cb */ diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c index 427150a206cf..3c7db20ce564 100644 --- a/fs/gfs2/lock_dlm.c +++ b/fs/gfs2/lock_dlm.c @@ -312,8 +312,13 @@ static int gdlm_lock(struct gfs2_glock *gl, unsigned int req_state, */ again: - error = dlm_lock(ls->ls_dlm, req, &gl->gl_lksb, lkf, strname, - GDLM_STRNAME_BYTES - 1, 0, gdlm_ast, gl, gdlm_bast); + down_read(&ls->ls_sem); + error = -ENODEV; + if (likely(ls->ls_dlm != NULL)) { + error = dlm_lock(ls->ls_dlm, req, &gl->gl_lksb, lkf, strname, + GDLM_STRNAME_BYTES - 1, 0, gdlm_ast, gl, gdlm_bast); + } + up_read(&ls->ls_sem); if (error == -EBUSY) { msleep(20); goto again; @@ -362,8 +367,13 @@ static void gdlm_put_lock(struct gfs2_glock *gl) flags |= DLM_LKF_VALBLK; again: - error = dlm_unlock(ls->ls_dlm, gl->gl_lksb.sb_lkid, flags, - NULL, gl); + down_read(&ls->ls_sem); + error = -ENODEV; + if (likely(ls->ls_dlm != NULL)) { + error = dlm_unlock(ls->ls_dlm, gl->gl_lksb.sb_lkid, flags, + NULL, gl); + } + up_read(&ls->ls_sem); if (error == -EBUSY) { msleep(20); goto again; @@ -379,7 +389,12 @@ static void gdlm_put_lock(struct gfs2_glock *gl) static void gdlm_cancel(struct gfs2_glock *gl) { struct lm_lockstruct *ls = &gl->gl_name.ln_sbd->sd_lockstruct; - dlm_unlock(ls->ls_dlm, gl->gl_lksb.sb_lkid, DLM_LKF_CANCEL, NULL, gl); + + down_read(&ls->ls_sem); + if (likely(ls->ls_dlm != NULL)) { + dlm_unlock(ls->ls_dlm, gl->gl_lksb.sb_lkid, DLM_LKF_CANCEL, NULL, gl); + } + up_read(&ls->ls_sem); } /* @@ -560,7 +575,11 @@ static int sync_unlock(struct gfs2_sbd *sdp, struct dlm_lksb *lksb, char *name) struct lm_lockstruct *ls = &sdp->sd_lockstruct; int error; - error = dlm_unlock(ls->ls_dlm, lksb->sb_lkid, 0, lksb, ls); + down_read(&ls->ls_sem); + error = -ENODEV; + if (likely(ls->ls_dlm != NULL)) + error = dlm_unlock(ls->ls_dlm, lksb->sb_lkid, 0, lksb, ls); + up_read(&ls->ls_sem); if (error) { fs_err(sdp, "%s lkid %x error %d\n", name, lksb->sb_lkid, error); @@ -587,9 +606,14 @@ static int sync_lock(struct gfs2_sbd *sdp, int mode, uint32_t flags, memset(strname, 0, GDLM_STRNAME_BYTES); snprintf(strname, GDLM_STRNAME_BYTES, "%8x%16x", LM_TYPE_NONDISK, num); - error = dlm_lock(ls->ls_dlm, mode, lksb, flags, - strname, GDLM_STRNAME_BYTES - 1, - 0, sync_wait_cb, ls, NULL); + down_read(&ls->ls_sem); + error = -ENODEV; + if (likely(ls->ls_dlm != NULL)) { + error = dlm_lock(ls->ls_dlm, mode, lksb, flags, + strname, GDLM_STRNAME_BYTES - 1, + 0, sync_wait_cb, ls, NULL); + } + up_read(&ls->ls_sem); if (error) { fs_err(sdp, "%s lkid %x flags %x mode %d error %d\n", name, lksb->sb_lkid, flags, mode, error); @@ -1316,6 +1340,7 @@ static int gdlm_mount(struct gfs2_sbd *sdp, const char *table) */ INIT_DELAYED_WORK(&sdp->sd_control_work, gfs2_control_func); + ls->ls_dlm = NULL; spin_lock_init(&ls->ls_recover_spin); ls->ls_recover_flags = 0; ls->ls_recover_mount = 0; @@ -1350,6 +1375,7 @@ static int gdlm_mount(struct gfs2_sbd *sdp, const char *table) * create/join lockspace */ + init_rwsem(&ls->ls_sem); error = dlm_new_lockspace(fsname, cluster, flags, GDLM_LVB_SIZE, &gdlm_lockspace_ops, sdp, &ops_result, &ls->ls_dlm); @@ -1429,10 +1455,12 @@ static void gdlm_unmount(struct gfs2_sbd *sdp) /* mounted_lock and control_lock will be purged in dlm recovery */ release: + down_write(&ls->ls_sem); if (ls->ls_dlm) { dlm_release_lockspace(ls->ls_dlm, 2); ls->ls_dlm = NULL; } + up_write(&ls->ls_sem); free_recover_size(ls); } From 28c4d9bc0708956c1a736a9e49fee71b65deee81 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Wed, 6 Aug 2025 23:34:03 +0200 Subject: [PATCH 20/20] gfs2: Fix unlikely race in gdlm_put_lock In gdlm_put_lock(), there is a small window of time in which the DFL_UNMOUNT flag has been set but the lockspace hasn't been released, yet. In that window, dlm may still call gdlm_ast() and gdlm_bast(). To prevent it from dereferencing freed glock objects, only free the glock if the lockspace has actually been released. Signed-off-by: Andreas Gruenbacher Reviewed-by: Andrew Price --- fs/gfs2/lock_dlm.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c index 3c7db20ce564..ae058b1f9435 100644 --- a/fs/gfs2/lock_dlm.c +++ b/fs/gfs2/lock_dlm.c @@ -344,12 +344,6 @@ static void gdlm_put_lock(struct gfs2_glock *gl) gfs2_sbstats_inc(gl, GFS2_LKS_DCOUNT); gfs2_update_request_times(gl); - /* don't want to call dlm if we've unmounted the lock protocol */ - if (test_bit(DFL_UNMOUNT, &ls->ls_recover_flags)) { - gfs2_glock_free(gl); - return; - } - /* * When the lockspace is released, all remaining glocks will be * unlocked automatically. This is more efficient than unlocking them @@ -379,6 +373,11 @@ static void gdlm_put_lock(struct gfs2_glock *gl) goto again; } + if (error == -ENODEV) { + gfs2_glock_free(gl); + return; + } + if (error) { fs_err(sdp, "gdlm_unlock %x,%llx err=%d\n", gl->gl_name.ln_type,