[Cluster-devel] [PATCH RFC gfs2/for-next] fs: gfs2: release dlm gfs2 ls on withdraw

[Alexander Aring]

Hi,

On Mon, Aug 23, 2021 at 11:56 AM Alexander Aring <aahringo at 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 <aahringo at 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