* [Cluster-devel] [RHEL7.9.z PATCH 0/2] Circular lock dependency on unmount
@ 2020-12-03 15:22 Bob Peterson
2020-12-03 15:22 ` [Cluster-devel] [RHEL7.9.z PATCH 1/2] gfs2: Add common helper for holding and releasing the freeze glock Bob Peterson
2020-12-03 15:22 ` [Cluster-devel] [RHEL7.9.z PATCH 2/2] gfs2: move freeze glock outside the make_fs_rw and _ro functions Bob Peterson
0 siblings, 2 replies; 4+ messages in thread
From: Bob Peterson @ 2020-12-03 15:22 UTC (permalink / raw)
To: cluster-devel.redhat.com
This patch set fixes a circular lock dependency flagged by LOCKDEP when a
gfs2 file system is unmounted. The first patch adds new helper functions
for holding and releasing the freeze glock. Functionality should be the same.
The second patch moves the freeze locks that used to be inside the
make_fs_ro and _rw functions to their callers. The unmount caller, however,
is exempted to avoid the lock dependency. The change means that function
init_threads is now called from within the freeze lock, but that should
not matter.
Bob Peterson (2):
gfs2: Add common helper for holding and releasing the freeze glock
gfs2: move freeze glock outside the make_fs_rw and _ro functions
fs/gfs2/ops_fstype.c | 33 +++++++++++++++--------------
fs/gfs2/recovery.c | 6 ++----
fs/gfs2/super.c | 45 ++++-----------------------------------
fs/gfs2/util.c | 50 ++++++++++++++++++++++++++++++++++++++++++--
fs/gfs2/util.h | 3 +++
5 files changed, 74 insertions(+), 63 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Cluster-devel] [RHEL7.9.z PATCH 1/2] gfs2: Add common helper for holding and releasing the freeze glock
2020-12-03 15:22 [Cluster-devel] [RHEL7.9.z PATCH 0/2] Circular lock dependency on unmount Bob Peterson
@ 2020-12-03 15:22 ` Bob Peterson
2020-12-14 21:58 ` Andreas Gruenbacher
2020-12-03 15:22 ` [Cluster-devel] [RHEL7.9.z PATCH 2/2] gfs2: move freeze glock outside the make_fs_rw and _ro functions Bob Peterson
1 sibling, 1 reply; 4+ messages in thread
From: Bob Peterson @ 2020-12-03 15:22 UTC (permalink / raw)
To: cluster-devel.redhat.com
Many places in the gfs2 code queued and dequeued the freeze glock.
Almost all of them acquire it in SHARED mode, and need to specify the
same LM_FLAG_NOEXP and GL_EXACT flags.
This patch adds common helper functions gfs2_freeze_lock and gfs2_freeze_unlock
to make the code more readable, and to prepare for the next patch.
Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
fs/gfs2/ops_fstype.c | 6 ++----
fs/gfs2/recovery.c | 6 ++----
fs/gfs2/super.c | 45 +++++++++++++++-----------------------------
fs/gfs2/util.c | 35 ++++++++++++++++++++++++++++++++++
fs/gfs2/util.h | 3 +++
5 files changed, 57 insertions(+), 38 deletions(-)
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 61fce59cb4d3..4ee56f5e93cb 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -1198,14 +1198,12 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
if (sb_rdonly(sb)) {
struct gfs2_holder freeze_gh;
- error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_SHARED,
- LM_FLAG_NOEXP | GL_EXACT,
- &freeze_gh);
+ error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
if (error) {
fs_err(sdp, "can't make FS RO: %d\n", error);
goto fail_per_node;
}
- gfs2_glock_dq_uninit(&freeze_gh);
+ gfs2_freeze_unlock(&freeze_gh);
} else {
error = gfs2_make_fs_rw(sdp);
if (error) {
diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
index c26c68ebd29d..2208b0e2dc8c 100644
--- a/fs/gfs2/recovery.c
+++ b/fs/gfs2/recovery.c
@@ -470,9 +470,7 @@ void gfs2_recover_func(struct work_struct *work)
/* Acquire a shared hold on the freeze lock */
- error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_SHARED,
- LM_FLAG_NOEXP | LM_FLAG_PRIORITY |
- GL_EXACT, &thaw_gh);
+ error = gfs2_freeze_lock(sdp, &thaw_gh, LM_FLAG_PRIORITY);
if (error)
goto fail_gunlock_ji;
@@ -522,7 +520,7 @@ void gfs2_recover_func(struct work_struct *work)
clean_journal(jd, &head);
up_read(&sdp->sd_log_flush_lock);
- gfs2_glock_dq_uninit(&thaw_gh);
+ gfs2_freeze_unlock(&thaw_gh);
t_rep = ktime_get();
fs_info(sdp, "jid=%u: Journal replayed in %lldms [jlck:%lldms, "
"jhead:%lldms, tlck:%lldms, replay:%lldms]\n",
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 2f56acc41c04..801361a05e6f 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -173,9 +173,7 @@ int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
if (error)
return error;
- error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_SHARED,
- LM_FLAG_NOEXP | GL_EXACT,
- &freeze_gh);
+ error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
if (error)
goto fail_threads;
@@ -205,12 +203,12 @@ int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
set_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
- gfs2_glock_dq_uninit(&freeze_gh);
+ gfs2_freeze_unlock(&freeze_gh);
return 0;
fail:
- gfs2_glock_dq_uninit(&freeze_gh);
+ gfs2_freeze_unlock(&freeze_gh);
fail_threads:
if (sdp->sd_quotad_process)
kthread_stop(sdp->sd_quotad_process);
@@ -452,7 +450,7 @@ static int gfs2_lock_fs_check_clean(struct gfs2_sbd *sdp)
}
if (error)
- gfs2_glock_dq_uninit(&sdp->sd_freeze_gh);
+ gfs2_freeze_unlock(&sdp->sd_freeze_gh);
out:
while (!list_empty(&list)) {
@@ -614,23 +612,13 @@ int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
int log_write_allowed = test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
gfs2_holder_mark_uninitialized(&freeze_gh);
- if (sdp->sd_freeze_gl &&
- !gfs2_glock_is_locked_by_me(sdp->sd_freeze_gl)) {
- if (!log_write_allowed) {
- error = gfs2_glock_nq_init(sdp->sd_freeze_gl,
- LM_ST_SHARED, LM_FLAG_TRY |
- LM_FLAG_NOEXP | GL_EXACT,
- &freeze_gh);
- if (error == GLR_TRYFAILED)
- error = 0;
- } else {
- error = gfs2_glock_nq_init(sdp->sd_freeze_gl,
- LM_ST_SHARED,
- LM_FLAG_NOEXP | GL_EXACT,
- &freeze_gh);
- if (error && !gfs2_withdrawn(sdp))
- return error;
- }
+ if (sdp->sd_freeze_gl) {
+ error = gfs2_freeze_lock(sdp, &freeze_gh,
+ log_write_allowed ? 0 : LM_FLAG_TRY);
+ if (error == GLR_TRYFAILED)
+ error = 0;
+ if (error && !gfs2_withdrawn(sdp))
+ return error;
}
gfs2_flush_delete_work(sdp);
@@ -661,8 +649,7 @@ int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
atomic_read(&sdp->sd_reserving_log) == 0,
HZ * 5);
}
- if (gfs2_holder_initialized(&freeze_gh))
- gfs2_glock_dq_uninit(&freeze_gh);
+ gfs2_freeze_unlock(&freeze_gh);
gfs2_quota_cleanup(sdp);
@@ -772,10 +759,8 @@ void gfs2_freeze_func(struct work_struct *work)
struct super_block *sb = sdp->sd_vfs;
atomic_inc(&sb->s_active);
- error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_SHARED,
- LM_FLAG_NOEXP | GL_EXACT, &freeze_gh);
+ error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
if (error) {
- fs_info(sdp, "GFS2: couldn't get freeze lock : %d\n", error);
gfs2_assert_withdraw(sdp, 0);
} else {
atomic_set(&sdp->sd_freeze_state, SFS_UNFROZEN);
@@ -785,7 +770,7 @@ void gfs2_freeze_func(struct work_struct *work)
error);
gfs2_assert_withdraw(sdp, 0);
}
- gfs2_glock_dq_uninit(&freeze_gh);
+ gfs2_freeze_unlock(&freeze_gh);
}
deactivate_super(sb);
clear_bit_unlock(SDF_FS_FROZEN, &sdp->sd_flags);
@@ -853,7 +838,7 @@ static int gfs2_unfreeze(struct super_block *sb)
return 0;
}
- gfs2_glock_dq_uninit(&sdp->sd_freeze_gh);
+ gfs2_freeze_unlock(&sdp->sd_freeze_gh);
mutex_unlock(&sdp->sd_freeze_mutex);
return wait_on_bit(&sdp->sd_flags, SDF_FS_FROZEN, TASK_INTERRUPTIBLE);
}
diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
index a374397f4273..9022ea1f5d50 100644
--- a/fs/gfs2/util.c
+++ b/fs/gfs2/util.c
@@ -91,6 +91,41 @@ int check_journal_clean(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
return error;
}
+/**
+ * gfs2_freeze_lock - hold the freeze glock
+ * @sdp: the superblock
+ * @freeze_gh: pointer to the requested holder
+ * @caller_flags: any additional flags needed by the caller
+ */
+int gfs2_freeze_lock(struct gfs2_sbd *sdp, struct gfs2_holder *freeze_gh,
+ int caller_flags)
+{
+ int flags = LM_FLAG_NOEXP | GL_EXACT | caller_flags;
+ int error;
+
+ /*
+ * We mark the caller's holder uninitialized here before the check to
+ * see if it's already locked. If it's already locked, it may have been
+ * locked with a different gh (as will be the case for some withdraws).
+ * We don't want them trying to unlock what we didn't lock.
+ */
+ gfs2_holder_mark_uninitialized(freeze_gh);
+ if (gfs2_glock_is_locked_by_me(sdp->sd_freeze_gl))
+ return 0;
+
+ error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_SHARED, flags,
+ freeze_gh);
+ if (error && error != GLR_TRYFAILED)
+ fs_err(sdp, "can't lock the freeze lock: %d\n", error);
+ return error;
+}
+
+void gfs2_freeze_unlock(struct gfs2_holder *freeze_gh)
+{
+ if (gfs2_holder_initialized(freeze_gh))
+ gfs2_glock_dq_uninit(freeze_gh);
+}
+
static void signal_our_withdraw(struct gfs2_sbd *sdp)
{
struct gfs2_glock *gl = sdp->sd_live_gh.gh_gl;
diff --git a/fs/gfs2/util.h b/fs/gfs2/util.h
index a4443dd8a94b..69e1a0ae5a4d 100644
--- a/fs/gfs2/util.h
+++ b/fs/gfs2/util.h
@@ -149,6 +149,9 @@ int gfs2_io_error_i(struct gfs2_sbd *sdp, const char *function,
extern int check_journal_clean(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
bool verbose);
+extern int gfs2_freeze_lock(struct gfs2_sbd *sdp,
+ struct gfs2_holder *freeze_gh, int caller_flags);
+extern void gfs2_freeze_unlock(struct gfs2_holder *freeze_gh);
#define gfs2_io_error(sdp) \
gfs2_io_error_i((sdp), __func__, __FILE__, __LINE__)
--
2.28.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Cluster-devel] [RHEL7.9.z PATCH 2/2] gfs2: move freeze glock outside the make_fs_rw and _ro functions
2020-12-03 15:22 [Cluster-devel] [RHEL7.9.z PATCH 0/2] Circular lock dependency on unmount Bob Peterson
2020-12-03 15:22 ` [Cluster-devel] [RHEL7.9.z PATCH 1/2] gfs2: Add common helper for holding and releasing the freeze glock Bob Peterson
@ 2020-12-03 15:22 ` Bob Peterson
1 sibling, 0 replies; 4+ messages in thread
From: Bob Peterson @ 2020-12-03 15:22 UTC (permalink / raw)
To: cluster-devel.redhat.com
Before this patch, sister functions gfs2_make_fs_rw and gfs2_make_fs_ro locked
(held) the freeze glock by calling gfs2_freeze_lock and gfs2_freeze_unlock.
The problem is, not all the callers of gfs2_make_fs_ro should be doing this.
The three callers of gfs2_make_fs_ro are: remount (gfs2_reconfigure),
signal_our_withdraw, and unmount (gfs2_put_super). But when unmounting the
file system we can get into the following circular lock dependency:
deactivate_super
down_write(&s->s_umount); <-------------------------------------- s_umount
deactivate_locked_super
gfs2_kill_sb
kill_block_super
generic_shutdown_super
gfs2_put_super
gfs2_make_fs_ro
gfs2_glock_nq_init sd_freeze_gl
freeze_go_sync
if (freeze glock in SH)
freeze_super (vfs)
down_write(&sb->s_umount); <------- s_umount
This patch moves the hold of the freeze glock outside the two sister rw/ro
functions to their callers, but it doesn't request the glock from
gfs2_put_super, thus eliminating the circular dependency.
Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
fs/gfs2/ops_fstype.c | 31 +++++++++++++++++--------------
fs/gfs2/super.c | 22 ----------------------
fs/gfs2/util.c | 15 +++++++++++++--
3 files changed, 30 insertions(+), 38 deletions(-)
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 4ee56f5e93cb..f2c6bbe5cdb8 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -1084,6 +1084,7 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
int silent = fc->sb_flags & SB_SILENT;
struct gfs2_sbd *sdp;
struct gfs2_holder mount_gh;
+ struct gfs2_holder freeze_gh;
int error;
sdp = init_sbd(sb);
@@ -1195,23 +1196,18 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
goto fail_per_node;
}
- if (sb_rdonly(sb)) {
- struct gfs2_holder freeze_gh;
+ error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
+ if (error)
+ goto fail_per_node;
- error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
- if (error) {
- fs_err(sdp, "can't make FS RO: %d\n", error);
- goto fail_per_node;
- }
- gfs2_freeze_unlock(&freeze_gh);
- } else {
+ if (!sb_rdonly(sb))
error = gfs2_make_fs_rw(sdp);
- if (error) {
- fs_err(sdp, "can't make FS RW: %d\n", error);
- goto fail_per_node;
- }
- }
+ gfs2_freeze_unlock(&freeze_gh);
+ if (error) {
+ fs_err(sdp, "can't make FS RW: %d\n", error);
+ goto fail_per_node;
+ }
gfs2_glock_dq_uninit(&mount_gh);
gfs2_online_uevent(sdp);
return 0;
@@ -1512,6 +1508,12 @@ static int gfs2_reconfigure(struct fs_context *fc)
fc->sb_flags |= SB_RDONLY;
if ((sb->s_flags ^ fc->sb_flags) & SB_RDONLY) {
+ struct gfs2_holder freeze_gh;
+
+ error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
+ if (error)
+ return -EINVAL;
+
if (fc->sb_flags & SB_RDONLY) {
error = gfs2_make_fs_ro(sdp);
if (error)
@@ -1521,6 +1523,7 @@ static int gfs2_reconfigure(struct fs_context *fc)
if (error)
errorfc(fc, "unable to remount read-write");
}
+ gfs2_freeze_unlock(&freeze_gh);
}
sdp->sd_args = *newargs;
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 801361a05e6f..754ea2a137b4 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -165,7 +165,6 @@ int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
{
struct gfs2_inode *ip = GFS2_I(sdp->sd_jdesc->jd_inode);
struct gfs2_glock *j_gl = ip->i_gl;
- struct gfs2_holder freeze_gh;
struct gfs2_log_header_host head;
int error;
@@ -173,10 +172,6 @@ int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
if (error)
return error;
- error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
- if (error)
- goto fail_threads;
-
j_gl->gl_ops->go_inval(j_gl, DIO_METADATA);
if (gfs2_withdrawn(sdp)) {
error = -EIO;
@@ -203,13 +198,9 @@ int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
set_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
- gfs2_freeze_unlock(&freeze_gh);
-
return 0;
fail:
- gfs2_freeze_unlock(&freeze_gh);
-fail_threads:
if (sdp->sd_quotad_process)
kthread_stop(sdp->sd_quotad_process);
sdp->sd_quotad_process = NULL;
@@ -607,20 +598,9 @@ static void gfs2_dirty_inode(struct inode *inode, int flags)
int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
{
- struct gfs2_holder freeze_gh;
int error = 0;
int log_write_allowed = test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
- gfs2_holder_mark_uninitialized(&freeze_gh);
- if (sdp->sd_freeze_gl) {
- error = gfs2_freeze_lock(sdp, &freeze_gh,
- log_write_allowed ? 0 : LM_FLAG_TRY);
- if (error == GLR_TRYFAILED)
- error = 0;
- if (error && !gfs2_withdrawn(sdp))
- return error;
- }
-
gfs2_flush_delete_work(sdp);
if (!log_write_allowed && current == sdp->sd_quotad_process)
fs_warn(sdp, "The quotad daemon is withdrawing.\n");
@@ -649,8 +629,6 @@ int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
atomic_read(&sdp->sd_reserving_log) == 0,
HZ * 5);
}
- gfs2_freeze_unlock(&freeze_gh);
-
gfs2_quota_cleanup(sdp);
if (!log_write_allowed)
diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
index 9022ea1f5d50..02f7a7ec7a31 100644
--- a/fs/gfs2/util.c
+++ b/fs/gfs2/util.c
@@ -132,6 +132,7 @@ static void signal_our_withdraw(struct gfs2_sbd *sdp)
struct inode *inode = sdp->sd_jdesc->jd_inode;
struct gfs2_inode *ip = GFS2_I(inode);
u64 no_formal_ino = ip->i_no_formal_ino;
+ int log_write_allowed = test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
int ret = 0;
int tries;
@@ -152,8 +153,18 @@ static void signal_our_withdraw(struct gfs2_sbd *sdp)
* therefore we need to clear SDF_JOURNAL_LIVE manually.
*/
clear_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
- if (!sb_rdonly(sdp->sd_vfs))
- ret = gfs2_make_fs_ro(sdp);
+ if (!sb_rdonly(sdp->sd_vfs)) {
+ struct gfs2_holder freeze_gh;
+
+ ret = gfs2_freeze_lock(sdp, &freeze_gh,
+ log_write_allowed ? 0 : LM_FLAG_TRY);
+ if (ret == GLR_TRYFAILED)
+ ret = 0;
+ if (!ret) {
+ ret = gfs2_make_fs_ro(sdp);
+ gfs2_freeze_unlock(&freeze_gh);
+ }
+ }
if (sdp->sd_lockstruct.ls_ops->lm_lock == NULL) { /* lock_nolock */
if (!ret)
--
2.28.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Cluster-devel] [RHEL7.9.z PATCH 1/2] gfs2: Add common helper for holding and releasing the freeze glock
2020-12-03 15:22 ` [Cluster-devel] [RHEL7.9.z PATCH 1/2] gfs2: Add common helper for holding and releasing the freeze glock Bob Peterson
@ 2020-12-14 21:58 ` Andreas Gruenbacher
0 siblings, 0 replies; 4+ messages in thread
From: Andreas Gruenbacher @ 2020-12-14 21:58 UTC (permalink / raw)
To: cluster-devel.redhat.com
Hi Bob,
On Thu, Dec 3, 2020 at 4:23 PM Bob Peterson <rpeterso@redhat.com> wrote:
> Many places in the gfs2 code queued and dequeued the freeze glock.
> Almost all of them acquire it in SHARED mode, and need to specify the
> same LM_FLAG_NOEXP and GL_EXACT flags.
>
> This patch adds common helper functions gfs2_freeze_lock and gfs2_freeze_unlock
> to make the code more readable, and to prepare for the next patch.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
> fs/gfs2/ops_fstype.c | 6 ++----
> fs/gfs2/recovery.c | 6 ++----
> fs/gfs2/super.c | 45 +++++++++++++++-----------------------------
> fs/gfs2/util.c | 35 ++++++++++++++++++++++++++++++++++
> fs/gfs2/util.h | 3 +++
> 5 files changed, 57 insertions(+), 38 deletions(-)
>
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 61fce59cb4d3..4ee56f5e93cb 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -1198,14 +1198,12 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
> if (sb_rdonly(sb)) {
> struct gfs2_holder freeze_gh;
>
> - error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_SHARED,
> - LM_FLAG_NOEXP | GL_EXACT,
> - &freeze_gh);
> + error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
> if (error) {
> fs_err(sdp, "can't make FS RO: %d\n", error);
> goto fail_per_node;
> }
> - gfs2_glock_dq_uninit(&freeze_gh);
> + gfs2_freeze_unlock(&freeze_gh);
> } else {
> error = gfs2_make_fs_rw(sdp);
> if (error) {
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index c26c68ebd29d..2208b0e2dc8c 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -470,9 +470,7 @@ void gfs2_recover_func(struct work_struct *work)
>
> /* Acquire a shared hold on the freeze lock */
>
> - error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_SHARED,
> - LM_FLAG_NOEXP | LM_FLAG_PRIORITY |
> - GL_EXACT, &thaw_gh);
> + error = gfs2_freeze_lock(sdp, &thaw_gh, LM_FLAG_PRIORITY);
> if (error)
> goto fail_gunlock_ji;
>
> @@ -522,7 +520,7 @@ void gfs2_recover_func(struct work_struct *work)
> clean_journal(jd, &head);
> up_read(&sdp->sd_log_flush_lock);
>
> - gfs2_glock_dq_uninit(&thaw_gh);
> + gfs2_freeze_unlock(&thaw_gh);
> t_rep = ktime_get();
> fs_info(sdp, "jid=%u: Journal replayed in %lldms [jlck:%lldms, "
> "jhead:%lldms, tlck:%lldms, replay:%lldms]\n",
The gfs2_glock_dq_uninit(&thaw_gh) at label fail_gunlock_thaw needs to
be turned into gfs2_freeze_unlock(&thaw_gh) as well.
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 2f56acc41c04..801361a05e6f 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -173,9 +173,7 @@ int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
> if (error)
> return error;
>
> - error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_SHARED,
> - LM_FLAG_NOEXP | GL_EXACT,
> - &freeze_gh);
> + error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
> if (error)
> goto fail_threads;
>
> @@ -205,12 +203,12 @@ int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
>
> set_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
>
> - gfs2_glock_dq_uninit(&freeze_gh);
> + gfs2_freeze_unlock(&freeze_gh);
>
> return 0;
>
> fail:
> - gfs2_glock_dq_uninit(&freeze_gh);
> + gfs2_freeze_unlock(&freeze_gh);
> fail_threads:
> if (sdp->sd_quotad_process)
> kthread_stop(sdp->sd_quotad_process);
> @@ -452,7 +450,7 @@ static int gfs2_lock_fs_check_clean(struct gfs2_sbd *sdp)
> }
>
> if (error)
> - gfs2_glock_dq_uninit(&sdp->sd_freeze_gh);
> + gfs2_freeze_unlock(&sdp->sd_freeze_gh);
>
> out:
> while (!list_empty(&list)) {
> @@ -614,23 +612,13 @@ int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
> int log_write_allowed = test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
>
> gfs2_holder_mark_uninitialized(&freeze_gh);
> - if (sdp->sd_freeze_gl &&
> - !gfs2_glock_is_locked_by_me(sdp->sd_freeze_gl)) {
Nooo, please leave the gfs2_glock_is_locked_by_me checking at the few
callers that actually need it. That check doesn't come for free, and
it also makes the code more difficult to understand.
> - if (!log_write_allowed) {
> - error = gfs2_glock_nq_init(sdp->sd_freeze_gl,
> - LM_ST_SHARED, LM_FLAG_TRY |
> - LM_FLAG_NOEXP | GL_EXACT,
> - &freeze_gh);
> - if (error == GLR_TRYFAILED)
> - error = 0;
> - } else {
> - error = gfs2_glock_nq_init(sdp->sd_freeze_gl,
> - LM_ST_SHARED,
> - LM_FLAG_NOEXP | GL_EXACT,
> - &freeze_gh);
> - if (error && !gfs2_withdrawn(sdp))
> - return error;
> - }
> + if (sdp->sd_freeze_gl) {
> + error = gfs2_freeze_lock(sdp, &freeze_gh,
> + log_write_allowed ? 0 : LM_FLAG_TRY);
> + if (error == GLR_TRYFAILED)
> + error = 0;
> + if (error && !gfs2_withdrawn(sdp))
> + return error;
> }
>
> gfs2_flush_delete_work(sdp);
> @@ -661,8 +649,7 @@ int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
> atomic_read(&sdp->sd_reserving_log) == 0,
> HZ * 5);
> }
> - if (gfs2_holder_initialized(&freeze_gh))
> - gfs2_glock_dq_uninit(&freeze_gh);
> + gfs2_freeze_unlock(&freeze_gh);
>
> gfs2_quota_cleanup(sdp);
>
> @@ -772,10 +759,8 @@ void gfs2_freeze_func(struct work_struct *work)
> struct super_block *sb = sdp->sd_vfs;
>
> atomic_inc(&sb->s_active);
> - error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_SHARED,
> - LM_FLAG_NOEXP | GL_EXACT, &freeze_gh);
> + error = gfs2_freeze_lock(sdp, &freeze_gh, 0);
> if (error) {
> - fs_info(sdp, "GFS2: couldn't get freeze lock : %d\n", error);
> gfs2_assert_withdraw(sdp, 0);
> } else {
> atomic_set(&sdp->sd_freeze_state, SFS_UNFROZEN);
> @@ -785,7 +770,7 @@ void gfs2_freeze_func(struct work_struct *work)
> error);
> gfs2_assert_withdraw(sdp, 0);
> }
> - gfs2_glock_dq_uninit(&freeze_gh);
> + gfs2_freeze_unlock(&freeze_gh);
> }
> deactivate_super(sb);
> clear_bit_unlock(SDF_FS_FROZEN, &sdp->sd_flags);
> @@ -853,7 +838,7 @@ static int gfs2_unfreeze(struct super_block *sb)
> return 0;
> }
>
> - gfs2_glock_dq_uninit(&sdp->sd_freeze_gh);
> + gfs2_freeze_unlock(&sdp->sd_freeze_gh);
> mutex_unlock(&sdp->sd_freeze_mutex);
> return wait_on_bit(&sdp->sd_flags, SDF_FS_FROZEN, TASK_INTERRUPTIBLE);
> }
> diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
> index a374397f4273..9022ea1f5d50 100644
> --- a/fs/gfs2/util.c
> +++ b/fs/gfs2/util.c
> @@ -91,6 +91,41 @@ int check_journal_clean(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
> return error;
> }
>
> +/**
> + * gfs2_freeze_lock - hold the freeze glock
> + * @sdp: the superblock
> + * @freeze_gh: pointer to the requested holder
> + * @caller_flags: any additional flags needed by the caller
> + */
> +int gfs2_freeze_lock(struct gfs2_sbd *sdp, struct gfs2_holder *freeze_gh,
> + int caller_flags)
> +{
> + int flags = LM_FLAG_NOEXP | GL_EXACT | caller_flags;
> + int error;
> +
> + /*
> + * We mark the caller's holder uninitialized here before the check to
> + * see if it's already locked. If it's already locked, it may have been
> + * locked with a different gh (as will be the case for some withdraws).
> + * We don't want them trying to unlock what we didn't lock.
> + */
> + gfs2_holder_mark_uninitialized(freeze_gh);
> + if (gfs2_glock_is_locked_by_me(sdp->sd_freeze_gl))
> + return 0;
> +
> + error = gfs2_glock_nq_init(sdp->sd_freeze_gl, LM_ST_SHARED, flags,
> + freeze_gh);
> + if (error && error != GLR_TRYFAILED)
> + fs_err(sdp, "can't lock the freeze lock: %d\n", error);
> + return error;
> +}
> +
> +void gfs2_freeze_unlock(struct gfs2_holder *freeze_gh)
> +{
> + if (gfs2_holder_initialized(freeze_gh))
> + gfs2_glock_dq_uninit(freeze_gh);
> +}
> +
> static void signal_our_withdraw(struct gfs2_sbd *sdp)
> {
> struct gfs2_glock *gl = sdp->sd_live_gh.gh_gl;
> diff --git a/fs/gfs2/util.h b/fs/gfs2/util.h
> index a4443dd8a94b..69e1a0ae5a4d 100644
> --- a/fs/gfs2/util.h
> +++ b/fs/gfs2/util.h
> @@ -149,6 +149,9 @@ int gfs2_io_error_i(struct gfs2_sbd *sdp, const char *function,
>
> extern int check_journal_clean(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
> bool verbose);
> +extern int gfs2_freeze_lock(struct gfs2_sbd *sdp,
> + struct gfs2_holder *freeze_gh, int caller_flags);
> +extern void gfs2_freeze_unlock(struct gfs2_holder *freeze_gh);
>
> #define gfs2_io_error(sdp) \
> gfs2_io_error_i((sdp), __func__, __FILE__, __LINE__)
> --
> 2.28.0
>
Thanks,
Andreas
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-12-14 21:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-03 15:22 [Cluster-devel] [RHEL7.9.z PATCH 0/2] Circular lock dependency on unmount Bob Peterson
2020-12-03 15:22 ` [Cluster-devel] [RHEL7.9.z PATCH 1/2] gfs2: Add common helper for holding and releasing the freeze glock Bob Peterson
2020-12-14 21:58 ` Andreas Gruenbacher
2020-12-03 15:22 ` [Cluster-devel] [RHEL7.9.z PATCH 2/2] gfs2: move freeze glock outside the make_fs_rw and _ro functions Bob Peterson
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.