Hi, On Mon, Aug 23, 2021 at 11:56 AM Alexander Aring <aahri...@redhat.com> wrote: > > This patch will introduce a new gfs2 lock ops callback for release a > possible lock infrastructure e.g. dlm. There is currently an issue with > gfs2 and dlm by hitting ctrl-c during mount operation (sometimes it > works, most times not). The issue is here that when the gfs2 filesystem > is not withdrawn we don't release the dlm lockspace again and next mount > dlm_new_lockspace() will return -EEXIST. This patch will ensure that we > call dlm_release_lockspace() in the error path of gfs2_fill_super() even if > the filesystem isn't withdrawn yet. Moreover it will do that in all > cases even if the filesystem is not withdrawn yet. > > Signed-off-by: Alexander Aring <aahri...@redhat.com> > --- > Hi, > > no idea if the gfs2_withdrawn(sdp) should be always evaluated to > "false", but then there are cases when it returns "true" and the > gfs2 dlm lockspace will not be released... next mount there will be > a -EEXIST, as described in the commit message. > > If gfs2_withdrawn(sdp) should return "false" always maybe there is some > missing wait and we should printout a warning if it's returning true... > in this case and an error path we have a problem which can be observed > at the next mount. > > - Alex > > fs/gfs2/glock.h | 1 + > fs/gfs2/lock_dlm.c | 24 ++++++++++++++++++------ > fs/gfs2/ops_fstype.c | 6 +++++- > 3 files changed, 24 insertions(+), 7 deletions(-) > > diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h > index 31a8f2f649b5..c8fd1352b0e1 100644 > --- a/fs/gfs2/glock.h > +++ b/fs/gfs2/glock.h > @@ -130,6 +130,7 @@ struct lm_lockops { > void (*lm_recovery_result) (struct gfs2_sbd *sdp, unsigned int jid, > unsigned int result); > void (*lm_unmount) (struct gfs2_sbd *sdp); > + void (*lm_release) (struct gfs2_sbd *sdp); > void (*lm_withdraw) (struct gfs2_sbd *sdp); > void (*lm_put_lock) (struct gfs2_glock *gl); > int (*lm_lock) (struct gfs2_glock *gl, unsigned int req_state, > diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c > index 50578f881e6d..776667ca4da1 100644 > --- a/fs/gfs2/lock_dlm.c > +++ b/fs/gfs2/lock_dlm.c > @@ -1248,6 +1248,14 @@ static const struct dlm_lockspace_ops > gdlm_lockspace_ops = { > .recover_done = gdlm_recover_done, > }; > > +static void gdlm_ls_release(struct lm_lockstruct *ls) > +{ > + if (ls->ls_dlm) { > + dlm_release_lockspace(ls->ls_dlm, 2); > + ls->ls_dlm = NULL; > + }
I believe there must also be a: free_recover_size(ls); > +} > + > static int gdlm_mount(struct gfs2_sbd *sdp, const char *table) > { > struct lm_lockstruct *ls = &sdp->sd_lockstruct; > @@ -1338,7 +1346,7 @@ static int gdlm_mount(struct gfs2_sbd *sdp, const char > *table) > return 0; > > fail_release: > - dlm_release_lockspace(ls->ls_dlm, 2); > + gdlm_ls_release(ls); then don't change this. > fail_free: > free_recover_size(ls); > fail: > @@ -1374,14 +1382,17 @@ static void gdlm_unmount(struct gfs2_sbd *sdp) > > /* mounted_lock and control_lock will be purged in dlm recovery */ > release: > - if (ls->ls_dlm) { > - dlm_release_lockspace(ls->ls_dlm, 2); > - ls->ls_dlm = NULL; > - } > - > + gdlm_ls_release(ls); > free_recover_size(ls); remove that. > } > > +static void gdlm_release(struct gfs2_sbd *sdp) > +{ > + struct lm_lockstruct *ls = &sdp->sd_lockstruct; > + > + gdlm_ls_release(ls); > +} > + > static const match_table_t dlm_tokens = { > { Opt_jid, "jid=%d"}, > { Opt_id, "id=%d"}, > @@ -1396,6 +1407,7 @@ const struct lm_lockops gfs2_dlm_ops = { > .lm_first_done = gdlm_first_done, > .lm_recovery_result = gdlm_recovery_result, > .lm_unmount = gdlm_unmount, > + .lm_release = gdlm_release, > .lm_put_lock = gdlm_put_lock, > .lm_lock = gdlm_lock, > .lm_cancel = gdlm_cancel, > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c > index 7f8410d8fdc1..ef25ed884db2 100644 > --- a/fs/gfs2/ops_fstype.c > +++ b/fs/gfs2/ops_fstype.c > @@ -1075,8 +1075,12 @@ static int gfs2_lm_mount(struct gfs2_sbd *sdp, int > silent) > void gfs2_lm_unmount(struct gfs2_sbd *sdp) > { > const struct lm_lockops *lm = sdp->sd_lockstruct.ls_ops; > - if (likely(!gfs2_withdrawn(sdp)) && lm->lm_unmount) > + if (likely(!gfs2_withdrawn(sdp)) && lm->lm_unmount) { > lm->lm_unmount(sdp); > + } else { > + if (lm->lm_release) > + lm->lm_release(sdp); > + } That means we also have a memory leak for some of the fields in "free_recover_size(ls)". If this approach is okay, I will send a v2. - Alex