From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bob Peterson Date: Mon, 13 Sep 2021 14:30:28 -0500 Subject: [Cluster-devel] [GFS2 PATCH 4/4] gfs2: rework go_lock mechanism for node_scope race In-Reply-To: <20210913193028.75116-1-rpeterso@redhat.com> References: <20210913193028.75116-1-rpeterso@redhat.com> Message-ID: <20210913193028.75116-5-rpeterso@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Before this patch, glocks only performed their go_lock glop function when the first holder was queued. When we introduced the new "node scope" mechanism, we allowed multiple holders to hold a glock in EX at the same time, with local locking. But it introduced a new race: If the first holder specified GL_SKIP (skip the go_lock function until deemed necessary) any secondary holders WITHOUT GL_SKIP assumed the go_lock mechanism had been performed. In the case of rgrps, that was not always the case: for example, when the first process is searching for free blocks for allocation (GL_SKIP) and a second holder (without GL_SKIP) wants to free blocks, which just assumes the buffers exist. To remedy this race, this patch reworks how the GL_SKIP mechanism is managed. The first holder checks if GL_SKIP is specified, and if so, a new glock flag, GLF_GO_LOCK_SKIPPED, is set. Subsequent holders without GL_SKIP who need the lock check this flag to see if the go_lock function was skipped, and call it if necessary. This also "accidentally" fixes a minor bug with glock do-promote tracing: glocks that had no "go_lock" glop would not properly log the "first" flag to the kernel trace. Fixes: 06e908cd9ead ("gfs2: Allow node-wide exclusive glock sharing") Signed-off-by: Bob Peterson --- fs/gfs2/glock.c | 81 ++++++++++++++++++++++++++++++++---------------- fs/gfs2/glops.c | 1 + fs/gfs2/incore.h | 1 + fs/gfs2/rgrp.c | 1 + 4 files changed, 58 insertions(+), 26 deletions(-) diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index b8248ceff3c3..faad2cb105bb 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -380,6 +380,46 @@ static void do_error(struct gfs2_glock *gl, const int ret) } } +static int glock_go_lock(struct gfs2_glock *gl, struct gfs2_holder *gh, + struct gfs2_holder *gh1) +__releases(&gl->gl_lockref.lock) +__acquires(&gl->gl_lockref.lock) +{ + const struct gfs2_glock_operations *glops = gl->gl_ops; + int ret = 0; + + /* + * When the first holder on the list is granted, it can SKIP or not. + * If it skips go_lock, it sets the GLF_GO_LOCK_SKIPPED flag. + * All subsequent holders need to know if it was skipped. They may + * also choose to skip go_lock, but only the first granted should + * set GLF_GO_LOCK_SKIPPED. If not skipped, they need to know if + * go_lock was previously skipped, call go_lock, and clear the flag. + */ + if (gh->gh_flags & GL_SKIP) { + if (gh == gh1) + set_bit(GLF_GO_LOCK_SKIPPED, &gl->gl_flags); + goto out; + } + if ((gh == gh1) || test_bit(GLF_GO_LOCK_SKIPPED, &gl->gl_flags)) { + spin_unlock(&gl->gl_lockref.lock); + /* FIXME: eliminate this eventually */ + ret = glops->go_lock(gh); + spin_lock(&gl->gl_lockref.lock); + clear_bit(GLF_GO_LOCK_SKIPPED, &gl->gl_flags); + if (ret) { + if (ret == 1) + return 2; + gh->gh_error = ret; + list_del_init(&gh->gh_list); + trace_gfs2_glock_queue(gh, 0); + gfs2_holder_wake(gh); + } + } +out: + return ret; +} + /** * do_promote - promote as many requests as possible on the current queue * @gl: The glock @@ -389,46 +429,33 @@ static void do_error(struct gfs2_glock *gl, const int ret) */ static int do_promote(struct gfs2_glock *gl) -__releases(&gl->gl_lockref.lock) -__acquires(&gl->gl_lockref.lock) { const struct gfs2_glock_operations *glops = gl->gl_ops; - struct gfs2_holder *gh, *tmp; + struct gfs2_holder *gh, *gh1, *tmp; int ret; restart: + gh1 = list_first_entry(&gl->gl_holders, struct gfs2_holder, gh_list); + list_for_each_entry_safe(gh, tmp, &gl->gl_holders, gh_list) { if (test_bit(HIF_HOLDER, &gh->gh_iflags)) continue; if (may_grant(gl, gh)) { - if (gh->gh_list.prev == &gl->gl_holders && - glops->go_lock) { - if (!(gh->gh_flags & GL_SKIP)) { - spin_unlock(&gl->gl_lockref.lock); - /* FIXME: eliminate this eventually */ - ret = glops->go_lock(gh); - spin_lock(&gl->gl_lockref.lock); - if (ret) { - if (ret == 1) - return 2; - gh->gh_error = ret; - list_del_init(&gh->gh_list); - trace_gfs2_glock_queue(gh, 0); - gfs2_holder_wake(gh); - goto restart; - } - } - set_bit(HIF_HOLDER, &gh->gh_iflags); - trace_gfs2_promote(gh, 1); - gfs2_holder_wake(gh); - goto restart; + ret = 0; + if (glops->go_lock) { + ret = glock_go_lock(gl, gh, gh1); + if (ret == 2) + return ret; } + set_bit(HIF_HOLDER, &gh->gh_iflags); - trace_gfs2_promote(gh, 0); + trace_gfs2_promote(gh, gh == gh1 ? 1 : 0); gfs2_holder_wake(gh); + if (ret) + goto restart; continue; } - if (gh->gh_list.prev == &gl->gl_holders) + if (gh == gh1) return 1; do_error(gl, 0); break; @@ -2148,6 +2175,8 @@ static const char *gflags2str(char *buf, const struct gfs2_glock *gl) *p++ = 'P'; if (test_bit(GLF_FREEING, gflags)) *p++ = 'x'; + if (test_bit(GLF_GO_LOCK_SKIPPED, gflags)) + *p++ = 's'; *p = 0; return buf; } diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c index 4b19f513570f..923efcd71269 100644 --- a/fs/gfs2/glops.c +++ b/fs/gfs2/glops.c @@ -477,6 +477,7 @@ int gfs2_inode_refresh(struct gfs2_inode *ip) error = gfs2_dinode_in(ip, dibh->b_data); brelse(dibh); clear_bit(GIF_INVALID, &ip->i_flags); + clear_bit(GLF_GO_LOCK_SKIPPED, &ip->i_gl->gl_flags); return error; } diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h index 0fe49770166e..4880818db83d 100644 --- a/fs/gfs2/incore.h +++ b/fs/gfs2/incore.h @@ -315,6 +315,7 @@ struct gfs2_alloc_parms { enum { GLF_LOCK = 1, + GLF_GO_LOCK_SKIPPED = 2, /* go_lock was skipped */ GLF_DEMOTE = 3, GLF_PENDING_DEMOTE = 4, GLF_DEMOTE_IN_PROGRESS = 5, diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c index 1fb66f6e6a0c..83fd5c750041 100644 --- a/fs/gfs2/rgrp.c +++ b/fs/gfs2/rgrp.c @@ -1248,6 +1248,7 @@ static int gfs2_rgrp_bh_get(struct gfs2_rgrpd *rgd) if (rgd->rd_rgl->rl_unlinked == 0) rgd->rd_flags &= ~GFS2_RDF_CHECK; } + clear_bit(GLF_GO_LOCK_SKIPPED, &gl->gl_flags); return 0; fail: -- 2.31.1