reset/sanlk_resetd.c
by David Teigland
reset/sanlk_resetd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
New commits:
commit ff5cd9d88c55170c3bd255a3fcf24674fc268fb2
Author: Nir Soffer <nsoffer(a)redhat.com>
Date: Mon Aug 11 16:16:11 2014 -0500
reset: Use find_ls to remove lockspace by name
find_ls was introduced recently, use it to find a lockspace by name fro
unregistering, instead of duplicating the search loop.
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
diff --git a/reset/sanlk_resetd.c b/reset/sanlk_resetd.c
index 6e55404..d3ea158 100644
--- a/reset/sanlk_resetd.c
+++ b/reset/sanlk_resetd.c
@@ -519,12 +519,12 @@ static void process_update(int fd)
} else if (!strcmp(cmd, "end")) {
log_debug("process_update end %s", name);
- for (i = 0; i < MAX_LS; i++) {
- if (!ls_names[i] || strcmp(ls_names[i], name))
- continue;
+ i = find_ls(name);
+ if (i > -1) {
unregister_ls(i);
return;
}
+
} else if (!strcmp(cmd, "clear")) {
log_debug("process_update clear %s", name);
9 years, 8 months
[PATCH] reset: Use find_ls to remove lockspace by name
by Nir Soffer
find_ls was introduced recently, use it to find a lockspace by name fro
unregistering, instead of duplicating the search loop.
---
reset/sanlk_resetd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/reset/sanlk_resetd.c b/reset/sanlk_resetd.c
index c9deedb..c9f1573 100644
--- a/reset/sanlk_resetd.c
+++ b/reset/sanlk_resetd.c
@@ -506,12 +506,12 @@ static void process_update(int fd)
} else if (!strcmp(cmd, "end")) {
log_debug("process_update end %s", name);
- for (i = 0; i < MAX_LS; i++) {
- if (!ls_names[i] || strcmp(ls_names[i], name))
- continue;
+ i = find_ls(name);
+ if (i > -1) {
unregister_ls(i);
return;
}
+
} else if (!strcmp(cmd, "clear")) {
log_debug("process_update clear %s", name);
--
1.8.3.1
9 years, 8 months
2 commits - reset/sanlk_reset.c reset/sanlk_resetd.c reset/sanlk_reset.h
by David Teigland
reset/sanlk_reset.c | 7 ++++++-
reset/sanlk_reset.h | 8 +++++---
reset/sanlk_resetd.c | 15 ++++++++++++++-
3 files changed, 25 insertions(+), 5 deletions(-)
New commits:
commit fde9f54e8181935f32f2db9f6f4a4eea92b0308f
Author: David Teigland <teigland(a)redhat.com>
Date: Mon Aug 11 15:33:55 2014 -0500
sanlk-reset: no args should print help
Signed-off-by: David Teigland <teigland(a)redhat.com>
diff --git a/reset/sanlk_reset.c b/reset/sanlk_reset.c
index cba1542..6bff43e 100644
--- a/reset/sanlk_reset.c
+++ b/reset/sanlk_reset.c
@@ -497,6 +497,11 @@ int main(int argc, char *argv[])
memset(&he, 0, sizeof(he));
+ if (argc < 2) {
+ usage();
+ exit(EXIT_FAILURE);
+ }
+
static struct option long_options[] = {
{"help", no_argument, 0, 'h' },
{"version", no_argument, 0, 'V' },
commit 4d13c379dbb53c1653017065e0de3a9130eec473
Author: Nir Soffer <nsoffer(a)redhat.com>
Date: Mon Aug 11 15:31:38 2014 -0500
sanlk-resetd: Allow only privileged user to modify configuration
Previously sanlk-resetd socket used an abstract namespace, allowing any
user to configure the daemon. Now we use a real filename so accessing
the socket is possible only for privileged user.
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
diff --git a/reset/sanlk_reset.c b/reset/sanlk_reset.c
index a69d1e3..cba1542 100644
--- a/reset/sanlk_reset.c
+++ b/reset/sanlk_reset.c
@@ -439,7 +439,7 @@ static int update_local_daemon(char *cmd)
rv = sendto(s, buf, UPDATE_SIZE, 0, (struct sockaddr *)&update_addr, update_addrlen);
if (rv < 0) {
- printf("Failed to update local sanlk-resetd %d\n", rv);
+ printf("Failed to update local sanlk-resetd: %s\n", strerror(errno));
return rv;
} else {
printf("Updated %s %s\n", cmd, ls_names[i]);
diff --git a/reset/sanlk_reset.h b/reset/sanlk_reset.h
index f386301..8c92c8e 100644
--- a/reset/sanlk_reset.h
+++ b/reset/sanlk_reset.h
@@ -14,7 +14,9 @@
#define EVENT_REBOOT 4
#define EVENT_REBOOTING 8
-#define SANLK_RESETD_PATH "sanlk-resetd"
+#define SANLK_RESETD_RUNDIR "/var/run/sanlk-resetd"
+#define SANLK_RESETD_SOCKET SANLK_RESETD_RUNDIR "/sanlk-resetd.sock"
+#define SANLK_RESETD_SOCKET_MODE (S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP)
#define UPDATE_SIZE 256 /* sendmsg size on unix socket */
@@ -28,8 +30,8 @@ static inline int setup_resetd_socket(void)
memset(&update_addr, 0, sizeof(update_addr));
update_addr.sun_family = AF_LOCAL;
- strcpy(&update_addr.sun_path[1], SANLK_RESETD_PATH);
- update_addrlen = sizeof(sa_family_t) + strlen(update_addr.sun_path+1) + 1;
+ strcpy(update_addr.sun_path, SANLK_RESETD_SOCKET);
+ update_addrlen = sizeof(sa_family_t) + strlen(update_addr.sun_path) + 1;
return s;
}
diff --git a/reset/sanlk_resetd.c b/reset/sanlk_resetd.c
index c9deedb..6e55404 100644
--- a/reset/sanlk_resetd.c
+++ b/reset/sanlk_resetd.c
@@ -446,14 +446,27 @@ static int setup_update(void)
{
int s, rv;
+ rv = mkdir(SANLK_RESETD_RUNDIR, 0755);
+ if (rv < 0 && errno != EEXIST)
+ return rv;
+
s = setup_resetd_socket();
+ unlink(update_addr.sun_path);
rv = bind(s, (struct sockaddr *) &update_addr, update_addrlen);
if (rv < 0)
- return rv;
+ goto fail_close;
+
+ rv = chmod(update_addr.sun_path, SANLK_RESETD_SOCKET_MODE);
+ if (rv < 0)
+ goto fail_close;
update_fd = s;
return 0;
+
+fail_close:
+ close(s);
+ return -1;
}
static void process_update(int fd)
9 years, 8 months
[PATCH] sanlk-resetd: Allow only privileged user to modify configuration
by Nir Soffer
Previously sanlk-resetd socket used an abstract namespace, allowing any
user to configure the daemon. Now we use a real filename so accessing
the socket is possible only for privileged user.
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
---
reset/sanlk_reset.c | 2 +-
reset/sanlk_reset.h | 8 +++++---
reset/sanlk_resetd.c | 15 ++++++++++++++-
3 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/reset/sanlk_reset.c b/reset/sanlk_reset.c
index ddd7af2..34b66af 100644
--- a/reset/sanlk_reset.c
+++ b/reset/sanlk_reset.c
@@ -439,7 +439,7 @@ static int update_local_daemon(char *cmd)
rv = sendto(s, buf, UPDATE_SIZE, 0, (struct sockaddr *)&update_addr, update_addrlen);
if (rv < 0) {
- printf("Failed to update local sanlk-resetd %d\n", rv);
+ printf("Failed to update local sanlk-resetd: %s\n", strerror(errno));
return rv;
} else {
printf("Updated %s %s\n", cmd, ls_names[i]);
diff --git a/reset/sanlk_reset.h b/reset/sanlk_reset.h
index f386301..8c92c8e 100644
--- a/reset/sanlk_reset.h
+++ b/reset/sanlk_reset.h
@@ -14,7 +14,9 @@
#define EVENT_REBOOT 4
#define EVENT_REBOOTING 8
-#define SANLK_RESETD_PATH "sanlk-resetd"
+#define SANLK_RESETD_RUNDIR "/var/run/sanlk-resetd"
+#define SANLK_RESETD_SOCKET SANLK_RESETD_RUNDIR "/sanlk-resetd.sock"
+#define SANLK_RESETD_SOCKET_MODE (S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP)
#define UPDATE_SIZE 256 /* sendmsg size on unix socket */
@@ -28,8 +30,8 @@ static inline int setup_resetd_socket(void)
memset(&update_addr, 0, sizeof(update_addr));
update_addr.sun_family = AF_LOCAL;
- strcpy(&update_addr.sun_path[1], SANLK_RESETD_PATH);
- update_addrlen = sizeof(sa_family_t) + strlen(update_addr.sun_path+1) + 1;
+ strcpy(update_addr.sun_path, SANLK_RESETD_SOCKET);
+ update_addrlen = sizeof(sa_family_t) + strlen(update_addr.sun_path) + 1;
return s;
}
diff --git a/reset/sanlk_resetd.c b/reset/sanlk_resetd.c
index 0c0c0f8..1f00144 100644
--- a/reset/sanlk_resetd.c
+++ b/reset/sanlk_resetd.c
@@ -430,14 +430,27 @@ static int setup_update(void)
{
int s, rv;
+ rv = mkdir(SANLK_RESETD_RUNDIR, 0755);
+ if (rv < 0 && errno != EEXIST)
+ return rv;
+
s = setup_resetd_socket();
+ unlink(update_addr.sun_path);
rv = bind(s, (struct sockaddr *) &update_addr, update_addrlen);
if (rv < 0)
- return rv;
+ goto fail_close;
+
+ rv = chmod(update_addr.sun_path, SANLK_RESETD_SOCKET_MODE);
+ if (rv < 0)
+ goto fail_close;
update_fd = s;
return 0;
+
+fail_close:
+ close(s);
+ return -1;
}
static void process_update(int fd)
--
1.8.3.1
9 years, 8 months
sanlock events - events not received after releasing and acquiring host id
by Nir Soffer
Hi David,
While testing host events using the python bindings, I found a corner case which
may cause trouble.
I have 2 hosts running vdsm, using the vdsm ids lockspace.
On host_id 1, I run this code:
lockspace = 'add39817-ed89-4ef7-88cd-f6e4c5a11d1d'
fd = sanlock.reg_event(lockspace)
try:
poll = select.poll()
poll.register(fd, select.POLLIN)
while True:
poll.poll()
events = sanlock.get_event(fd)
for ev in events:
print 'received event'
pprint.pprint(ev)
finally:
poll.unregister(fd)
sanlock.end_event(fd, lockspace)
On the other host_id 3, I run this:
sanlock.set_event(lockspace, 1, 0, 1, flags=sanlock.SETEV_CUR_GENERATION)
When both hosts are up, events send from host_id 3 to host_id 1 are received
correctly.
For example, I get this input:
received event
{'data': 0,
'event': 1,
'from_generation': 12,
'from_host_id': 3,
'generation': 21,
'host_id': 0}
Trouble start when I put host_id 1 to maintenance, which release the host id on
this lockspace.
When host is down, sanlock.get_hosts() on host_id 3 returns:
sanlock.get_hosts('add39817-ed89-4ef7-88cd-f6e4c5a11d1d', 1)
[{'generation': 21, 'host_id': 1, 'flags': 2, 'io_timeout': 10, 'timestamp': 0}]
The code registered for events on lockspace 'add39817-ed89-4ef7-88cd-f6e4c5a11d1d'
does not receive events when the host id is released. This makes sense, as the host
is not part of the cluster.
But, when I activate the host, and it acquire a the same host id again, on the
same lockspace, I still do not receive events from the code registered for
this lockspace.
Each time you put a host to maintenance, releasing the host_id and acquiring it again,
you can see in sanlock.log that sanlock is creating a new lockspace:
2014-08-10 18:40:01+0300 842420 [16247]: s6 lockspace 67ebc639-f790-4b1b-b5d2-526b0f569ac4:1:/dev/67ebc639-f790-4b1b-b5d2-526b0f569ac4/ids:0
2014-08-10 18:44:35+0300 842694 [16659]: s7 lockspace add39817-ed89-4ef7-88cd-f6e4c5a11d1d:1:/dev/add39817-ed89-4ef7-88cd-f6e4c5a11d1d/ids:0
2014-08-10 18:44:35+0300 842694 [16247]: s8 lockspace 67ebc639-f790-4b1b-b5d2-526b0f569ac4:1:/dev/67ebc639-f790-4b1b-b5d2-526b0f569ac4/ids:0
2014-08-10 18:56:16+0300 843394 [16247]: s9 lockspace add39817-ed89-4ef7-88cd-f6e4c5a11d1d:1:/dev/add39817-ed89-4ef7-88cd-f6e4c5a11d1d/ids:0
2014-08-10 18:56:16+0300 843394 [5023]: s10 lockspace 67ebc639-f790-4b1b-b5d2-526b0f569ac4:1:/dev/67ebc639-f790-4b1b-b5d2-526b0f569ac4/ids:0
To continue getting events, I must unregister the event fd, and register a new one
on the same lockspace.
It seems that the listening fd is tied to the lockspace number e.g. "s9", and not to
the lockspace name "67ebc639-f790-4b1b-b5d2-526b0f569ac4". So when "s9" is replaced
by "s10", the listener is broken silently.
Few questions:
1. Why listening for events stop working after releasing host id and acquiring it again
on the same lockspace?
2. Is this the right behavior?
3. Assuming this is the right behavior, why the listener does not fail? I think it is
expected that the event fds would close or fail so the listener can release
the resources.
For example, when I restart sanlock, the listener fails with:
sanlock.SanlockException: (1, 'Unable to get events', 'Operation not permitted')
So a similar failure is expected in this case.
4. Do we have an event fd leak until the listener reg_end() the fd?
I think that we should have either:
- Listening for events works after host id released and acquired again
- Listening for events fail when host id released
Nir
9 years, 8 months
host events - unused/unneeded values
by Nir Soffer
Hi David,
While testing the python bindings, I noticed that get_event always return host_id=0:
{'data': 4,
'event': 3,
'from_generation': 13,
'from_host_id': 3,
'generation': 22,
'host_id': 0}
If I'm not wrong, sanlock needs the host_id only when sending events, so this is
field is not used when sending events to listeners.
The generation field also seems unneeded, as sanlock filter evnets with wrong
generation, so you will never get an event with generation other then the current
generation - right?
Maybe we should change the api so:
When sending events:
host_id = recipient host id (to)
generation = recipient generation
When receiving events:
host_id = sender host id (from)
generation = sender generation
So we can eliminate the from_host_id and from_generation in get_event, simplifying
the api and eliminating some reads/writes.
I can eliminate the unneeded fields in the python bindings, but I prefer to return
the same data you get in the c api.
what do you think?
Nir
9 years, 8 months
reset/sanlk_reset.c reset/sanlk_resetd.c
by David Teigland
reset/sanlk_reset.c | 5 ++++-
reset/sanlk_resetd.c | 39 ++++++++++++++++++++++++++++++++++++---
2 files changed, 40 insertions(+), 4 deletions(-)
New commits:
commit 111109e3e74584ec7cbccfbdeb85dcbe1d81dcab
Author: David Teigland <teigland(a)redhat.com>
Date: Mon Aug 11 12:10:21 2014 -0500
reset: add ability to clear all
"sanlk-reset clear all" will unregister all names.
Also, register for an ls name that already exists
will result in sanlk-resetd doing end, then reg.
Signed-off-by: David Teigland <teigland(a)redhat.com>
diff --git a/reset/sanlk_reset.c b/reset/sanlk_reset.c
index ddd7af2..a69d1e3 100644
--- a/reset/sanlk_reset.c
+++ b/reset/sanlk_reset.c
@@ -463,6 +463,9 @@ static void usage(void)
printf("Update the local sanlk-resetd to not watch lockspaces for reset events:\n");
printf("%s end lockspace_name ...\n", prog_name);
printf("\n");
+ printf("Update the local sanlk-resetd to clear all lockspaces being watched:\n");
+ printf("%s clear all\n", prog_name);
+ printf("\n");
printf("Reset another host through a lockspace it is watching:\n");
printf("%s reset lockspace_name:host_id ...\n", prog_name);
printf(" --host-id | -i <num>\n");
@@ -562,7 +565,7 @@ int main(int argc, char *argv[])
* Update local sanlk-resetd.
*/
- if (!strcmp(cmd, "reg") || !strcmp(cmd, "end")) {
+ if (!strcmp(cmd, "reg") || !strcmp(cmd, "end") || !strcmp(cmd, "clear")) {
return update_local_daemon(cmd);
}
diff --git a/reset/sanlk_resetd.c b/reset/sanlk_resetd.c
index 0c0c0f8..c9deedb 100644
--- a/reset/sanlk_resetd.c
+++ b/reset/sanlk_resetd.c
@@ -251,6 +251,21 @@ static void set_event_out(char *ls_name, uint64_t event_out, uint64_t from_host,
log_error("set_event error %d ls %s", rv, ls_name);
}
+static int find_ls(char *name)
+{
+ int i;
+
+ for (i = 0; i < MAX_LS; i++) {
+ if (!ls_names[i])
+ continue;
+
+ if (!strcmp(name, ls_names[i]))
+ return i;
+ }
+
+ return -1;
+}
+
static int register_ls(int i)
{
int fd;
@@ -260,12 +275,12 @@ static int register_ls(int i)
fd = sanlock_reg_event(ls_names[i], NULL, 0);
if (fd < 0) {
- log_error("reg_event error %d ls %s", fd, ls_names[i]);
+ log_error("reg_event %d error %d ls %s", i, fd, ls_names[i]);
free(ls_names[i]);
ls_names[i] = NULL;
return fd;
} else {
- log_debug("reg_event fd %d ls %s", fd, ls_names[i]);
+ log_debug("reg_event %d fd %d ls %s", i, fd, ls_names[i]);
ls_fd[i] = fd;
pollfd[i].fd = fd;
pollfd[i].events = POLLIN;
@@ -276,6 +291,7 @@ static int register_ls(int i)
static void unregister_ls(int i)
{
+ log_debug("end_event %d fd %d ls %s", i, ls_fd[i], ls_names[i]);
sanlock_end_event(ls_fd[i], ls_names[i], 0);
free(ls_names[i]);
ls_names[i] = NULL;
@@ -458,7 +474,7 @@ static void process_update(int fd)
return;
}
- buf[UPDATE_SIZE] = '\0';
+ buf[UPDATE_SIZE-1] = '\0';
rv = sscanf(buf, "%s %s", cmd, name);
if (rv != 2) {
@@ -469,6 +485,15 @@ static void process_update(int fd)
if (!strcmp(cmd, "reg")) {
log_debug("process_update reg %s", name);
+ /* if the name exists, end then reg */
+ i = find_ls(name);
+ if (i > -1) {
+ unregister_ls(i);
+ ls_names[i] = strdup(name);
+ register_ls(i);
+ return;
+ }
+
for (i = 0; i < MAX_LS; i++) {
if (ls_names[i])
continue;
@@ -487,6 +512,14 @@ static void process_update(int fd)
unregister_ls(i);
return;
}
+ } else if (!strcmp(cmd, "clear")) {
+ log_debug("process_update clear %s", name);
+
+ for (i = 0; i < MAX_LS; i++) {
+ if (!ls_names[i])
+ continue;
+ unregister_ls(i);
+ }
} else {
log_debug("process_update cmd unknown");
}
9 years, 8 months
sanlk-resetd lockspace registeration managment
by Nir Soffer
Hi David,
When registering locksapce (reg xxxxx), sanlk-resetd does not check if
lockspace with same name is already registered, and will register
another listener for the same lockspace. Since listener are not closed
when lockspace is invalidated
(https://lists.fedorahosted.org/pipermail/sanlock-devel/2014-August/000480...)
This will create a listener leak.
Since having more then one listener on the same lockspace does not make sense,
I think we should first end the lockspace, and then register new one.
When unregistering lockspace (end xxxxx), the code assume that there is only
one lockspace with that name, possibly leaving duplicates unwanted listeners.
This assumption is ok, if the registration code did enforce this though.
Here is the relevant code from reset/sanlk-resetd.c:
if (!strcmp(cmd, "reg")) {
log_debug("process_update reg %s", name);
for (i = 0; i < MAX_LS; i++) {
if (ls_names[i])
continue;
ls_names[i] = strdup(name); // There may be already lockspace with this name
register_ls(i);
return;
}
} else if (!strcmp(cmd, "end")) {
log_debug("process_update end %s", name);
for (i = 0; i < MAX_LS; i++) {
if (!ls_names[i] || strcmp(ls_names[i], name))
continue;
unregister_ls(i);
return; // There may be another lockspace with this name
}
Last, there is an issue with the api. vdsm keeps the current lockspaces in
memory (actually is running a monitor thread for each one). So when vdsm
is asked to change the list of active lockspaces by engine, it can easily
end lockspaces which are not needed any more, and register new lockspaces.
However, if vdsm crashes and restart, it does know which lockspaces were
used before, so it cannot end these lockspaces. What will happen is actually
asking to register the same locksapces again, which given the current
sanlk-resetd behavior, will create a leak for all lockspaces on each vdsm
crash.
vdsm can try to end all lockspaces before registering them, but the new
set of lockspaces may be different from the old one, as the list is managed
by the engine, and vdsm keep no state locally.
Replacing listeners with same lockspace name will solve the duplicate issue,
however it will not solve the trouble with unused lockspaces.
I think that a new command, "clear", unregistering all lockspaces will
solve this issue. vdsm will clear the listeners each time it connects to
storage and maybe when going into maintenance mode.
Another option is a way to query the current locksapces that sanlk-resetd
is listening on. Given this list, vdsm can send the correct reg and end
commands easily. But this will require more work on both sanlk-resetd
and vdsm.
Nir
9 years, 8 months
src/lockspace.c
by David Teigland
src/lockspace.c | 1 +
1 file changed, 1 insertion(+)
New commits:
commit a8b379b992412cb2b4d15b9654ec6a98ddd31e67
Author: David Teigland <teigland(a)redhat.com>
Date: Mon Aug 11 10:50:25 2014 -0500
sanlock: fix missing mutex unlock in previous commit
Signed-off-by: David Teigland <teigland(a)redhat.com>
diff --git a/src/lockspace.c b/src/lockspace.c
index 8309d25..fe09d99 100644
--- a/src/lockspace.c
+++ b/src/lockspace.c
@@ -463,6 +463,7 @@ static void close_event_fds(struct space *sp)
close(sp->event_fds[i]);
sp->event_fds[i] = -1;
}
+ pthread_mutex_unlock(&sp->mutex);
}
/*
9 years, 8 months
src/lockspace.c
by David Teigland
src/lockspace.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
New commits:
commit 7079d90a5e1a678a86f5c4ca6bb46c3648425c94
Author: David Teigland <teigland(a)redhat.com>
Date: Mon Aug 11 10:27:29 2014 -0500
sanlock: close event fds when lockspace is removed
Signed-off-by: David Teigland <teigland(a)redhat.com>
diff --git a/src/lockspace.c b/src/lockspace.c
index 352287e..8309d25 100644
--- a/src/lockspace.c
+++ b/src/lockspace.c
@@ -451,6 +451,20 @@ static int corrupt_result(int result)
}
}
+static void close_event_fds(struct space *sp)
+{
+ int i;
+
+ pthread_mutex_lock(&sp->mutex);
+ for (i = 0; i < MAX_EVENT_FDS; i++) {
+ if (sp->event_fds[i] == -1)
+ continue;
+
+ close(sp->event_fds[i]);
+ sp->event_fds[i] = -1;
+ }
+}
+
/*
* This thread must not be stopped unless all pids that may be using any
* resources in it are dead/gone. (The USED flag in the lockspace represents
@@ -671,6 +685,8 @@ static void *lockspace_thread(void *arg_in)
purge_resource_orphans(sp->space_name);
+ close_event_fds(sp);
+
close_task_aio(&task);
return NULL;
}
9 years, 8 months