[Cluster-devel] Groupd uevent clean up

[Cluster-devel] Groupd uevent clean up

[Steven Whitehouse]

The following patch is designed to clean up a number of items relating to
gfs_controld's handling of uevent notifications. When a uevent is received
is consists of a number of strings whose total length is bounded by the
buffer size and returned by the recv call.

Originally only the initial string was being used to select the various
actions. That was wrong for various reasons, but mostly because its much
easier to simply look for the individual fields, broken out from the
message. The other reason that this is wrong, and also the reason that
I started looking into this area initially is that the initial event
string provides a "physical" view of the event. By using the SUBSYSTEM=
field, we get the same information, but it can also be overridden by
the kernel using the kset_uevent_ops which will become important when
we merge the lock_dlm module into GFS2.

The other problem (unsolved here) is that we have to parse the fs name
from the DEVPATH= variable. Its trivial to add a feature to the kernel
to provide the LOCKTABLE as well, so I plan to do that in the future
so we don't have to jump through hoops to get that information.

At the same time I've marked a bunch of variables const. Mainly
because they started to throw up warnings after I'd added the
new event parsing code.

I've tested the parsing code on its own, but since I don't have the
right packages installed here, I've not had a chance to build &
test a complete new groupd yet.

Also, I've fixed up some broken error handling in process_uevent
at the same time.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>

diff --git a/group/gfs_controld/cpg-new.c b/group/gfs_controld/cpg-new.c
index 020b89b..00ffc86 100644
--- a/group/gfs_controld/cpg-new.c
+++ b/group/gfs_controld/cpg-new.c
@@ -2078,7 +2078,7 @@ static void apply_changes(struct mountgroup *mg)
    and then process the uevent/ipc upon receiving the message for it, so
    that it can be processed in the same order by all nodes. */
 
-void process_recovery_uevent(char *table)
+void process_recovery_uevent(const char *table)
 {
 	struct mountgroup *mg;
 	struct journal *j;
@@ -2678,7 +2678,7 @@ static void leave_mountgroup(struct mountgroup *mg, int mnterr)
 		log_error("cpg_leave error %d", error);
 }
 
-void do_leave(char *table, int mnterr)
+void do_leave(const char *table, int mnterr)
 {
 	struct mountgroup *mg;
 	char *name = strstr(table, ":") + 1;
diff --git a/group/gfs_controld/cpg-old.c b/group/gfs_controld/cpg-old.c
index 192a403..d59d6ca 100644
--- a/group/gfs_controld/cpg-old.c
+++ b/group/gfs_controld/cpg-old.c
@@ -1604,7 +1604,7 @@ static int need_kernel_recovery_done(struct mountgroup *mg)
    remain blocked until an rw node mounts, and the next mounter must
    be rw. */
 
-int process_recovery_uevent_old(char *table)
+int process_recovery_uevent_old(const char *table)
 {
 	struct mountgroup *mg;
 	struct mg_member *memb;
@@ -1724,7 +1724,7 @@ static void leave_mountgroup(struct mountgroup *mg, int mnterr)
 	group_leave(gh, mg->name);
 }
 
-void do_leave_old(char *table, int mnterr)
+void do_leave_old(const char *table, int mnterr)
 {
 	struct mountgroup *mg;
 	char *name = strstr(table, ":") + 1;
diff --git a/group/gfs_controld/gfs_daemon.h b/group/gfs_controld/gfs_daemon.h
index 3beeb0e..ecb45d2 100644
--- a/group/gfs_controld/gfs_daemon.h
+++ b/group/gfs_controld/gfs_daemon.h
@@ -225,10 +225,10 @@ void process_cpg(int ci);
 int setup_dlmcontrol(void);
 void process_dlmcontrol(int ci);
 int set_protocol(void);
-void process_recovery_uevent(char *table);
+void process_recovery_uevent(const char *table);
 void process_mountgroups(void);
 int gfs_join_mountgroup(struct mountgroup *mg);
-void do_leave(char *table, int mnterr);
+void do_leave(const char *table, int mnterr);
 void gfs_mount_done(struct mountgroup *mg);
 void send_remount(struct mountgroup *mg, struct gfsc_mount_args *ma);
 void send_withdraw(struct mountgroup *mg);
@@ -245,12 +245,12 @@ void close_cpg_old(void);
 void process_cpg_old(int ci);
 
 int gfs_join_mountgroup_old(struct mountgroup *mg, struct gfsc_mount_args *ma);
-void do_leave_old(char *table, int mnterr);
+void do_leave_old(const char *table, int mnterr);
 int send_group_message_old(struct mountgroup *mg, int len, char *buf);
 void save_message_old(struct mountgroup *mg, char *buf, int len, int from,
 	int type);
 void send_withdraw_old(struct mountgroup *mg);
-int process_recovery_uevent_old(char *table);
+int process_recovery_uevent_old(const char *table);
 void ping_kernel_mount_old(char *table);
 void send_remount_old(struct mountgroup *mg, struct gfsc_mount_args *ma);
 void send_mount_status_old(struct mountgroup *mg);
@@ -282,7 +282,7 @@ int client_fd(int ci);
 void client_ignore(int ci, int fd);
 void client_back(int ci, int fd);
 struct mountgroup *create_mg(char *name);
-struct mountgroup *find_mg(char *name);
+struct mountgroup *find_mg(const char *name);
 struct mountgroup *find_mg_id(uint32_t id);
 void client_reply_remount(struct mountgroup *mg, int ci, int result);
 void client_reply_join(int ci, struct gfsc_mount_args *ma, int result);
diff --git a/group/gfs_controld/main.c b/group/gfs_controld/main.c
index 09fb105..ee82612 100644
--- a/group/gfs_controld/main.c
+++ b/group/gfs_controld/main.c
@@ -22,7 +22,7 @@ struct client {
 	struct mountgroup *mg;
 };
 
-static void do_withdraw(char *table);
+static void do_withdraw(const char *table);
 
 int do_read(int fd, void *buf, size_t count)
 {
@@ -175,7 +175,7 @@ struct mountgroup *create_mg(char *name)
 	return mg;
 }
 
-struct mountgroup *find_mg(char *name)
+struct mountgroup *find_mg(const char *name)
 {
 	struct mountgroup *mg;
 
@@ -198,39 +198,7 @@ struct mountgroup *find_mg_id(uint32_t id)
 	return NULL;
 }
 
-#define MAXARGS 8
-
-static char *get_args(char *buf, int *argc, char **argv, char sep, int want)
-{
-	char *p = buf, *rp = NULL;
-	int i;
-
-	argv[0] = p;
-
-	for (i = 1; i < MAXARGS; i++) {
-		p = strchr(buf, sep);
-		if (!p)
-			break;
-		*p = '\0';
-
-		if (want == i) {
-			rp = p + 1;
-			break;
-		}
-
-		argv[i] = p + 1;
-		buf = p + 1;
-	}
-	*argc = i;
-
-	/* we ended by hitting \0, return the point following that */
-	if (!rp)
-		rp = strchr(buf, '\0') + 1;
-
-	return rp;
-}
-
-static void ping_kernel_mount(char *table)
+static void ping_kernel_mount(const char *table)
 {
 	struct mountgroup *mg;
 	char *name = strstr(table, ":") + 1;
@@ -245,89 +213,92 @@ static void ping_kernel_mount(char *table)
 	log_group(mg, "ping_kernel_mount %d", rv);
 }
 
-static void process_uevent(int ci)
+/* FIXME: We should have a LOCKTABLE env var rather than trying to 
+ * parse it from DEVPATH
+ */
+
+static void decode_buf(char *buf, int len, const char **action,
+		       const char **subsystem, const char **fsname)
 {
-	char buf[MAXLINE];
-	char *argv[MAXARGS], *act, *sys;
-	int rv, argc = 0;
-	int lock_module = 0;
+	char *ptr;
+
+	*fsname = *subsystem = *action = NULL;
+	while (len > 8) {
+		ptr = buf;
+		buf += strlen(ptr);
+		len -= strlen(ptr);
+		buf++; len--;
+		if (strlen(ptr) < 7)
+			continue;
+		if (strncmp(ptr, "ACTION=", 7) == 0)
+			*action = ptr + 7;
+		if (strncmp(ptr, "SUBSYSTEM=", 10) == 0)
+			*subsystem = ptr + 10;
+		if (strncmp(ptr, "DEVPATH=/fs/gfs", 15) == 0) {
+			ptr += 15;
+			if (*ptr == '2')
+				ptr++;
+			if (*ptr != '/')
+				continue;
+			*fsname = ++ptr;
+			while (*ptr && *ptr != '/')
+				ptr++;
+			*ptr = 0;
+		}
+	}
+}
 
-	memset(buf, 0, sizeof(buf));
-	memset(argv, 0, sizeof(char *) * MAXARGS);
+static void process_uevent(int ci)
+{
+	char buf[MAXLINE + 1];
+	const char *action, *subsystem, *fsname;
+	int rv;
 
  retry_recv:
-	rv = recv(client[ci].fd, &buf, sizeof(buf), 0);
-	if (rv == -1 && rv == EINTR)
-		goto retry_recv;
-	if (rv == -1 && rv == EAGAIN)
-		return;
+	rv = recv(client[ci].fd, &buf, MAXLINE, 0);
 	if (rv < 0) {
-		log_error("uevent recv error %d errno %d", rv, errno);
+		if (errno == EINTR)
+			goto retry_recv;
+		if (errno != EAGAIN)
+			log_error("uevent recv error %d errno %d", rv, errno);
 		return;
 	}
-
-	/* first we get the uevent for removing lock module kobject:
-	     "remove@/fs/gfs/bull:x/lock_module"
-	   second is the uevent for removing gfs kobject:
-	     "remove@/fs/gfs/bull:x"
-	*/
-
-	if (!strstr(buf, "gfs"))
+	buf[rv] = 0;
+	decode_buf(buf, rv, &action, &subsystem, &fsname);
+	if (!fsname || !action || !subsystem)
 		return;
 
-	/* if an fs is named "gfs", it results in dlm uevents
-	   like "remove@/kernel/dlm/gfs" */
-
-	if (strstr(buf, "kernel/dlm"))
-		return;
-
-	log_debug("uevent: %s", buf);
-
-	if (strstr(buf, "lock_module"))
-		lock_module = 1;
-
-	get_args(buf, &argc, argv, '/', 4);
-	if (argc != 4)
-		log_error("uevent message has %d args", argc);
-	act = argv[0];
-	sys = argv[2];
-
-	log_debug("kernel: %s %s", act, argv[3]);
-
-	if (!strcmp(act, "remove@")) {
+/* FIXME: All of the below functions call find_mg as pretty much
+ * the first thing that they do. This ought to be done here and
+ * the found mg (if any) passed to the routines.
+ */
+	if (strcmp(action, "remove") == 0) {
 		/* We want to trigger the leave at the very end of the kernel's
 		   unmount process, i.e.@the end of put_super(), so we do the
 		   leave when the second uevent (from the gfs kobj) arrives. */
 
-		if (lock_module)
+		if (strcmp(subsystem, "lock_dlm") == 0)
 			return;
-
 		if (group_mode == GROUP_LIBGROUP)
-			do_leave_old(argv[3], 0);
+			do_leave_old(fsname, 0);
 		else
-			do_leave(argv[3], 0);
-
-	} else if (!strcmp(act, "change@")) {
-		if (!lock_module)
-			return;
+			do_leave(fsname, 0);
 
+	}
+	if (strcmp(subsystem, "lock_dlm") != 0)
+		return;
+	if (strcmp(action, "change") == 0) {
 		if (group_mode == GROUP_LIBGROUP)
-			process_recovery_uevent_old(argv[3]);
+			process_recovery_uevent_old(fsname);
 		else
-			process_recovery_uevent(argv[3]);
-
-	} else if (!strcmp(act, "offline@")) {
-		if (!lock_module)
-			return;
-
-		do_withdraw(argv[3]);
-
-	} else {
-		if (!lock_module)
-			return;
-
-		ping_kernel_mount(argv[3]);
+			process_recovery_uevent(fsname);
+		return;
+	}
+	if (strcmp(action, "offline") == 0) {
+		do_withdraw(fsname);
+		return;
 	}
+	ping_kernel_mount(fsname);
 }
 
 static int setup_uevent(void)
@@ -737,7 +708,7 @@ static void do_join(int ci, struct gfsc_mount_args *ma)
    and when it's been removed from the group, it tells the locally withdrawing
    gfs to clear out locks. */
 
-static void do_withdraw(char *table)
+static void do_withdraw(const char *table)
 {
 	struct mountgroup *mg;
 	char *name = strstr(table, ":") + 1;

[Cluster-devel] Re: Groupd uevent clean up

[David Teigland]

On Tue, Nov 25, 2008 at 02:08:42PM +0000, Steven Whitehouse wrote:
> 
> The following patch is designed to clean up a number of items relating to
> gfs_controld's handling of uevent notifications. When a uevent is received
> is consists of a number of strings whose total length is bounded by the
> buffer size and returned by the recv call.
> 
> Originally only the initial string was being used to select the various
> actions. That was wrong for various reasons, but mostly because its much
> easier to simply look for the individual fields, broken out from the
> message. The other reason that this is wrong, and also the reason that
> I started looking into this area initially is that the initial event
> string provides a "physical" view of the event. By using the SUBSYSTEM=
> field, we get the same information, but it can also be overridden by
> the kernel using the kset_uevent_ops which will become important when
> we merge the lock_dlm module into GFS2.
> 
> The other problem (unsolved here) is that we have to parse the fs name
> from the DEVPATH= variable. Its trivial to add a feature to the kernel
> to provide the LOCKTABLE as well, so I plan to do that in the future
> so we don't have to jump through hoops to get that information.
> 
> At the same time I've marked a bunch of variables const. Mainly
> because they started to throw up warnings after I'd added the
> new event parsing code.
> 
> I've tested the parsing code on its own, but since I don't have the
> right packages installed here, I've not had a chance to build &
> test a complete new groupd yet.

OK, thanks, I dislike the const sprinkling, and I don't care about string
parsing.  I'll take your word that this string parsing is an improvement
over what's there (I really dislike string parsing, previously get_args()
was used for splitting args from other strings, too).  All I know is that
what's there works, so if you can verify that the new parsing really works
I'll add it (or I can do a quick test if you resend a patch with just that
bit).

> Also, I've fixed up some broken error handling in process_uevent
> at the same time.

Yep, good catch, I'll add that small bit right now.

> +/* FIXME: All of the below functions call find_mg as pretty much
> + * the first thing that they do. This ought to be done here and
> + * the found mg (if any) passed to the routines.
> + */

It didn't work out well last time I tried that, one of the main sticking
points was that the "old" leave code needs to first look for the mg in a
different list... once we get rid of the old code (post cluster3 split) we
can try again.

[Cluster-devel] Re: Groupd uevent clean up

[Steven Whitehouse]

Hi,

On Tue, 2008-11-25 at 09:47 -0600, David Teigland wrote:
> On Tue, Nov 25, 2008 at 02:08:42PM +0000, Steven Whitehouse wrote:
> > 
> > The following patch is designed to clean up a number of items relating to
> > gfs_controld's handling of uevent notifications. When a uevent is received
> > is consists of a number of strings whose total length is bounded by the
> > buffer size and returned by the recv call.
> > 
> > Originally only the initial string was being used to select the various
> > actions. That was wrong for various reasons, but mostly because its much
> > easier to simply look for the individual fields, broken out from the
> > message. The other reason that this is wrong, and also the reason that
> > I started looking into this area initially is that the initial event
> > string provides a "physical" view of the event. By using the SUBSYSTEM=
> > field, we get the same information, but it can also be overridden by
> > the kernel using the kset_uevent_ops which will become important when
> > we merge the lock_dlm module into GFS2.
> > 
> > The other problem (unsolved here) is that we have to parse the fs name
> > from the DEVPATH= variable. Its trivial to add a feature to the kernel
> > to provide the LOCKTABLE as well, so I plan to do that in the future
> > so we don't have to jump through hoops to get that information.
> > 
> > At the same time I've marked a bunch of variables const. Mainly
> > because they started to throw up warnings after I'd added the
> > new event parsing code.
> > 
> > I've tested the parsing code on its own, but since I don't have the
> > right packages installed here, I've not had a chance to build &
> > test a complete new groupd yet.
> 
> OK, thanks, I dislike the const sprinkling, and I don't care about string
Whats wrong with adding the consts? It seems to make sense to mark the
fsname as const since its just being used as a key for looking up the
mg.

> parsing.  I'll take your word that this string parsing is an improvement
> over what's there (I really dislike string parsing, previously get_args()
> was used for splitting args from other strings, too).  All I know is that
> what's there works, so if you can verify that the new parsing really works
> I'll add it (or I can do a quick test if you resend a patch with just that
> bit).
> 
The reason for changing the parsing is really that it was looking at the
wrong string from the set of strings that uevent sends. It should be
trivial to find the right bits, but for some reason, we didn't add the
locktable to those variables. I hope to add that in the future, so that
we might in time be able to avoid messing about chopping bits out of the
DEVPATH= string in order to find the locktable.

My dev machine currently has a strange mix of F-9 and (old) rawhide
packages on it, so that installing the required bits of openais to build
isn't so easy. If you are able to run a quick test, that would be
helpful otherwise, I'll look for another place to build it for testing.

> > Also, I've fixed up some broken error handling in process_uevent
> > at the same time.
> 
> Yep, good catch, I'll add that small bit right now.
> 
I've just noticed dlm_controld has this same bug,

Steve.

[Cluster-devel] Re: Groupd uevent clean up

[David Teigland]

On Tue, Nov 25, 2008 at 04:44:32PM +0000, Steven Whitehouse wrote:
> > OK, thanks, I dislike the const sprinkling, and I don't care about string
> Whats wrong with adding the consts? It seems to make sense to mark the
> fsname as const since its just being used as a key for looking up the
> mg.

The PITA factor outweighs it's advantages, at least in the code I have to
maintain myself.

> My dev machine currently has a strange mix of F-9 and (old) rawhide
> packages on it, so that installing the required bits of openais to build
> isn't so easy. If you are able to run a quick test, that would be
> helpful otherwise, I'll look for another place to build it for testing.

I'll test a new patch that's limited to the parsing change.

> I've just noticed dlm_controld has this same bug,

Fixed that one too.

[Cluster-devel] Re: Groupd uevent clean up

[Steven Whitehouse]

Hi,

On Tue, 2008-11-25 at 11:08 -0600, David Teigland wrote:
> On Tue, Nov 25, 2008 at 04:44:32PM +0000, Steven Whitehouse wrote:
> > > OK, thanks, I dislike the const sprinkling, and I don't care about string
> > Whats wrong with adding the consts? It seems to make sense to mark the
> > fsname as const since its just being used as a key for looking up the
> > mg.
> 
> The PITA factor outweighs it's advantages, at least in the code I have to
> maintain myself.
> 
> > My dev machine currently has a strange mix of F-9 and (old) rawhide
> > packages on it, so that installing the required bits of openais to build
> > isn't so easy. If you are able to run a quick test, that would be
> > helpful otherwise, I'll look for another place to build it for testing.
> 
> I'll test a new patch that's limited to the parsing change.
> 
> > I've just noticed dlm_controld has this same bug,
> 
> Fixed that one too.
> 

So I think this needs looking into some more... I've pushed patches to
the tree which send more useful information with the uevents so that we
can avoid reading the sysfs files in many cases, in future. Each uevent
from gfs[2]/lock_dlm now includes:

LOCKTABLE=<clustername:fsname>
LOCKPROTO=[lock_dlm|lock_nolock]

to avoid all the messy parsing of the initial event string. Also I've
added come further information to the two "change" events, so that we
now have:

FIRSTMOUNT=Done

when the first mounter has finished and:

JID=<journal_id>
RECOVERY=[Done|Failed]

when recovery has finished. Is there anything else that is useful I
wonder, while I'm adding new items here? I think I've covered the
important bits anyway.

So I need to revise the initial patch a bit in order to take into
account the new information,

Steve.

[Cluster-devel] Re: Groupd uevent clean up

[David Teigland]

On Fri, Nov 28, 2008 at 11:07:56AM +0000, Steven Whitehouse wrote:
> LOCKTABLE=<clustername:fsname>
> LOCKPROTO=[lock_dlm|lock_nolock]
> 
> to avoid all the messy parsing of the initial event string. Also I've
> added come further information to the two "change" events, so that we
> now have:
> 
> FIRSTMOUNT=Done
> 
> when the first mounter has finished and:
> 
> JID=<journal_id>
> RECOVERY=[Done|Failed]
> 
> when recovery has finished. Is there anything else that is useful I
> wonder, while I'm adding new items here? I think I've covered the
> important bits anyway.

That's excellent, much better than the sysfs files.  We'll need to keep
the sysfs files around for a while, though, so we don't break the
user/kernel interface.

Dave

[Cluster-devel] Re: Groupd uevent clean up

[Steven Whitehouse]

Hi,

On Mon, 2008-12-01 at 09:28 -0600, David Teigland wrote:
> On Fri, Nov 28, 2008 at 11:07:56AM +0000, Steven Whitehouse wrote:
> > LOCKTABLE=<clustername:fsname>
> > LOCKPROTO=[lock_dlm|lock_nolock]
> > 
> > to avoid all the messy parsing of the initial event string. Also I've
> > added come further information to the two "change" events, so that we
> > now have:
> > 
> > FIRSTMOUNT=Done
> > 
> > when the first mounter has finished and:
> > 
> > JID=<journal_id>
> > RECOVERY=[Done|Failed]
> > 
> > when recovery has finished. Is there anything else that is useful I
> > wonder, while I'm adding new items here? I think I've covered the
> > important bits anyway.
> 
> That's excellent, much better than the sysfs files.  We'll need to keep
> the sysfs files around for a while, though, so we don't break the
> user/kernel interface.
> 
> Dave
> 
We also need to change groupd and the reason that I didn't resend that
original patch is that with these new bits of information, it might be
better to implement something a bit more generic, so that it picks up
all the right bits of information. Shouldn't be too tricky to keep it
backwards compatible either,

Steve.