[Cluster-devel] [PATCH 4/5] dlm: add recovery callbacks
Steven Whitehouse
swhiteho at redhat.com
Mon Dec 19 12:36:57 UTC 2011
Hi,
On Fri, 2011-12-16 at 16:03 -0600, David Teigland wrote:
> These new callbacks notify the dlm user about lock recovery.
> GFS2, and possibly others, need to be aware of when the dlm
> will be doing lock recovery for a failed lockspace member.
>
> In the past, this coordination has been done between dlm and
> file system daemons in userspace, which then direct their
> kernel counterparts. These callbacks allow the same
> coordination directly, and more simply.
>
> Signed-off-by: David Teigland <teigland at redhat.com>
> ---
> fs/dlm/config.c | 130 ++++++++++++++++++--------------
> fs/dlm/config.h | 17 +++-
> fs/dlm/dlm_internal.h | 20 ++----
> fs/dlm/lockspace.c | 37 +++++++--
> fs/dlm/member.c | 197 +++++++++++++++++++++++++++++++------------------
> fs/dlm/member.h | 3 +-
> fs/dlm/recoverd.c | 10 +-
> fs/dlm/user.c | 4 +-
> fs/gfs2/lock_dlm.c | 4 +-
> fs/ocfs2/stack_user.c | 4 +-
> include/linux/dlm.h | 65 +++++++++++++++-
> 11 files changed, 319 insertions(+), 172 deletions(-)
>
[snip]
> diff --git a/fs/dlm/dlm_internal.h b/fs/dlm/dlm_internal.h
> index f4d132c..cba3859 100644
> --- a/fs/dlm/dlm_internal.h
> +++ b/fs/dlm/dlm_internal.h
> @@ -2,7 +2,7 @@
> *******************************************************************************
> **
> ** Copyright (C) Sistina Software, Inc. 1997-2003 All rights reserved.
> -** Copyright (C) 2004-2010 Red Hat, Inc. All rights reserved.
> +** Copyright (C) 2004-2011 Red Hat, Inc. All rights reserved.
> **
> ** This copyrighted material is made available to anyone wishing to use,
> ** modify, copy, or redistribute it subject to the terms and conditions
> @@ -119,28 +119,18 @@ struct dlm_member {
> int weight;
> int slot;
> int slot_prev;
> + int comm_seq;
> uint32_t generation;
> };
>
> /*
> - * low nodeid saves array of these in ls_slots
> - */
> -
> -struct dlm_slot {
> - int nodeid;
> - int slot;
> -};
> -
> -/*
> * Save and manage recovery state for a lockspace.
> */
>
> struct dlm_recover {
> struct list_head list;
> - int *nodeids; /* nodeids of all members */
> - int node_count;
> - int *new; /* nodeids of new members */
> - int new_count;
> + struct dlm_config_node *nodes;
> + int nodes_count;
> uint64_t seq;
> };
>
> @@ -584,6 +574,8 @@ struct dlm_ls {
> struct list_head ls_root_list; /* root resources */
> struct rw_semaphore ls_root_sem; /* protect root_list */
>
> + struct dlm_lockspace_ops ls_ops;
^^^^^^^^^^ I'd suggest just keeping a pointer to
this, see below.
> +
> int ls_namelen;
> char ls_name[1];
> };
> diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
> index 1441f04..088ce1c 100644
> --- a/fs/dlm/lockspace.c
> +++ b/fs/dlm/lockspace.c
> @@ -2,7 +2,7 @@
> *******************************************************************************
> **
> ** Copyright (C) Sistina Software, Inc. 1997-2003 All rights reserved.
> -** Copyright (C) 2004-2008 Red Hat, Inc. All rights reserved.
> +** Copyright (C) 2004-2011 Red Hat, Inc. All rights reserved.
> **
> ** This copyrighted material is made available to anyone wishing to use,
> ** modify, copy, or redistribute it subject to the terms and conditions
> @@ -386,12 +386,14 @@ static void threads_stop(void)
> dlm_lowcomms_stop();
> }
>
> -static int new_lockspace(const char *name, int namelen, void **lockspace,
> - uint32_t flags, int lvblen)
> +static int new_lockspace(const char *name, const char *cluster, uint32_t flags,
> + int lvblen, struct dlm_lockspace_ops *ops,
^^^^ this should be const
> + dlm_lockspace_t **lockspace)
> {
> struct dlm_ls *ls;
> int i, size, error;
> int do_unreg = 0;
> + int namelen = strlen(name);
>
> if (namelen > DLM_LOCKSPACE_LEN)
> return -EINVAL;
> @@ -403,8 +405,23 @@ static int new_lockspace(const char *name, int namelen, void **lockspace,
> return -EINVAL;
>
> if (!dlm_user_daemon_available()) {
> - module_put(THIS_MODULE);
> - return -EUNATCH;
> + log_print("dlm user daemon not available");
> + error = -EUNATCH;
> + goto out;
> + }
> +
> + if (ops && !dlm_config.ci_recover_callbacks) {
> + log_print("dlm recover callbacks not enabled");
> + error = -EOPNOTSUPP;
> + goto out;
> + }
> +
> + if (cluster && strncmp(cluster, dlm_config.ci_cluster_name,
> + DLM_LOCKSPACE_LEN)) {
> + log_print("dlm cluster name %s mismatch %s",
> + dlm_config.ci_cluster_name, cluster);
> + error = -EBADR;
> + goto out;
> }
>
> error = 0;
> @@ -442,6 +459,9 @@ static int new_lockspace(const char *name, int namelen, void **lockspace,
> ls->ls_flags = 0;
> ls->ls_scan_time = jiffies;
>
> + if (ops)
> + memcpy(&ls->ls_ops, ops, sizeof(struct dlm_lockspace_ops));
> +
Why not just keep a pointer to the ops? There is no need to copy them.
Also - since the ops are specific to a user of the lockspace, this
implies that it is no longer possible to have multiple openers of the
same lockspace on the same node. I think that needs documenting
somewhere at least. The other possibility would be to introduce a
structure to represent a "user of a lockspace" I suppose...
> if (flags & DLM_LSFL_TIMEWARN)
> set_bit(LSFL_TIMEWARN, &ls->ls_flags);
>
> @@ -619,8 +639,9 @@ static int new_lockspace(const char *name, int namelen, void **lockspace,
> return error;
> }
>
> -int dlm_new_lockspace(const char *name, int namelen, void **lockspace,
> - uint32_t flags, int lvblen)
> +int dlm_new_lockspace(const char *name, const char *cluster, uint32_t flags,
> + int lvblen, struct dlm_lockspace_ops *ops,
^^^^ likewise this can also be const
> + dlm_lockspace_t **lockspace)
> {
> int error = 0;
>
> @@ -630,7 +651,7 @@ int dlm_new_lockspace(const char *name, int namelen, void **lockspace,
> if (error)
> goto out;
>
> - error = new_lockspace(name, namelen, lockspace, flags, lvblen);
> + error = new_lockspace(name, cluster, flags, lvblen, ops, lockspace);
> if (!error)
> ls_count++;
> if (error > 0)
[snip]
> diff --git a/include/linux/dlm.h b/include/linux/dlm.h
> index d4e02f5..27d96b3 100644
> --- a/include/linux/dlm.h
> +++ b/include/linux/dlm.h
> @@ -2,7 +2,7 @@
> *******************************************************************************
> **
> ** Copyright (C) Sistina Software, Inc. 1997-2003 All rights reserved.
> -** Copyright (C) 2004-2008 Red Hat, Inc. All rights reserved.
> +** Copyright (C) 2004-2011 Red Hat, Inc. All rights reserved.
> **
> ** This copyrighted material is made available to anyone wishing to use,
> ** modify, copy, or redistribute it subject to the terms and conditions
> @@ -74,15 +74,70 @@ struct dlm_lksb {
>
> #ifdef __KERNEL__
>
> +struct dlm_slot {
> + int nodeid; /* 1 to MAX_INT */
> + int slot; /* 1 to MAX_INT */
> +};
> +
> +/*
> + * recover_prep: called before the dlm begins lock recovery.
> + * Notfies lockspace user that locks from failed members will be granted.
> + * recover_slot: called after recover_prep and before recover_done.
> + * Identifies a failed lockspace member.
> + * recover_done: called after the dlm completes lock recovery.
> + * Identifies lockspace members and lockspace generation number.
> + */
> +
> +struct dlm_lockspace_ops {
> + void *cb_arg;
> + void (*recover_prep) (void *cb_arg);
> + void (*recover_slot) (void *cb_arg, struct dlm_slot *slot);
> + void (*recover_done) (void *cb_arg, struct dlm_slot *slots,
> + int num_slots, int our_slot, uint32_t generation);
> +};
> +
Please don't mix the callback arg and the functions in the same
structure. The functions will be identical for all filesystems, where as
the callback arg will be unique to each filesystem, so it would be
better to just add an extra cb_arg to the new lockspace function.
Otherwise it looks good to me,
Steve.
More information about the Cluster-devel
mailing list