[PATCH 0/7] dmevent mlockall() fixes

[PATCH 0/7] dmevent mlockall() fixes

[Zdenek Kabelac]

This patch set tries to fix dmeventd.

Dmeventd got actually broken with our new mlock() code.
There is no big chance to make current architecture with threads usable
with mlock() code - so mlockall() is used for daemon case.

Snapshot plugin current doesn't use cmd_context and doesn't need memory
locking.

Mirror plugin uses memlock_inc_daemon() and locks while binary to memory.

C locales are enforced to avoid loading of 100MB locale-archive file.

Few minor fixes for syslog message and resource leaks in error path.

Statistic for mlock() could be based on readable sizes.

Zdenek Kabelac (7):
  Remove mlockall() form dmeventd
  Fix resouces leak in error path
  Release pool in the same reversed order
  Updated syslog messages
  Force C locale
  Update memlock
  Count only readable size for memlock stats.

 daemons/dmeventd/dmeventd.c                       |   14 ++---
 daemons/dmeventd/plugins/lvm2/dmeventd_lvm.c      |    4 +-
 daemons/dmeventd/plugins/mirror/dmeventd_mirror.c |   28 +++++-----
 lib/mm/memlock.c                                  |   64 ++++++++++-----------
 4 files changed, 51 insertions(+), 59 deletions(-)

[PATCH 1/7] Remove mlockall() form dmeventd

[Zdenek Kabelac]

As the header file <sys/mman.h> was not included in dmeventd.c
thus missed definition of MCL_CURRENT  - thus patch only makes
it obvious we were not locking memory here.

This patch has no functional change.
Later part of this patch set handles mlockall() via memlock_inc_daemon().

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 daemons/dmeventd/dmeventd.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index 7eb3b6b..37122c9 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -1725,11 +1725,6 @@ int main(int argc, char *argv[])
 
 	pthread_mutex_init(&_global_mutex, NULL);
 
-#ifdef MCL_CURRENT
-	if (mlockall(MCL_CURRENT | MCL_FUTURE) == -1)
-		exit(EXIT_FAILURE);
-#endif
-
 	if ((ret = _open_fifos(&fifos)))
 		exit(EXIT_FIFO_FAILURE);
 
@@ -1749,9 +1744,6 @@ int main(int argc, char *argv[])
 
 	_exit_dm_lib();
 
-#ifdef MCL_CURRENT
-	munlockall();
-#endif
 	pthread_mutex_destroy(&_global_mutex);
 
 	syslog(LOG_NOTICE, "dmeventd shutting down.");
-- 
1.7.0.1

[PATCH 2/7] Fix resouces leak in error path

[Zdenek Kabelac]

If the error path of _register_for_event() calls _free_thread_status()
we need to call _lib_put().
To make thing simplier moving this _lib_put() into common error path code.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 daemons/dmeventd/dmeventd.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index 37122c9..f4b664b 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -243,8 +243,10 @@ static struct thread_status *_alloc_thread_status(struct message_data *data,
 	return ret;
 }
 
+static void _lib_put(struct dso_data *data);
 static void _free_thread_status(struct thread_status *thread)
 {
+	_lib_put(thread->dso_data);
 	if (thread->current_task)
 		dm_task_destroy(thread->current_task);
 	dm_free(thread->device.uuid);
@@ -1481,7 +1483,6 @@ static void _cleanup_unused_threads(void)
 		if (thread->status == DM_THREAD_DONE) {
 			dm_list_del(l);
 			pthread_join(thread->thread, NULL);
-			_lib_put(thread->dso_data);
 			_free_thread_status(thread);
 		}
 	}
-- 
1.7.0.1

[PATCH 3/7] Release pool in the same reversed order

[Zdenek Kabelac]

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 daemons/dmeventd/plugins/lvm2/dmeventd_lvm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/daemons/dmeventd/plugins/lvm2/dmeventd_lvm.c b/daemons/dmeventd/plugins/lvm2/dmeventd_lvm.c
index 37900e4..bff60b4 100644
--- a/daemons/dmeventd/plugins/lvm2/dmeventd_lvm.c
+++ b/daemons/dmeventd/plugins/lvm2/dmeventd_lvm.c
@@ -105,11 +105,11 @@ void dmeventd_lvm2_exit(void)
 	pthread_mutex_lock(&_register_mutex);
 
 	if (!--_register_count) {
-		dm_pool_destroy(_mem_pool);
-		_mem_pool = NULL;
 		lvm2_run(_lvm_handle, "_memlock_dec");
 		lvm2_exit(_lvm_handle);
 		_lvm_handle = NULL;
+		dm_pool_destroy(_mem_pool);
+		_mem_pool = NULL;
 	}
 
 	pthread_mutex_unlock(&_register_mutex);
-- 
1.7.0.1

[PATCH 4/7] Updated syslog messages

[Zdenek Kabelac]

Use our common '.' end format for messages.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 daemons/dmeventd/plugins/mirror/dmeventd_mirror.c |   28 ++++++++++----------
 1 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/daemons/dmeventd/plugins/mirror/dmeventd_mirror.c b/daemons/dmeventd/plugins/mirror/dmeventd_mirror.c
index e5d81ee..4d2eee5 100644
--- a/daemons/dmeventd/plugins/mirror/dmeventd_mirror.c
+++ b/daemons/dmeventd/plugins/mirror/dmeventd_mirror.c
@@ -42,17 +42,17 @@ static int _process_status_code(const char status_code, const char *dev_name,
 	 *    U => Unclassified failure (bug)
 	 */ 
 	if (status_code == 'F') {
-		syslog(LOG_ERR, "%s device %s flush failed.\n",
+		syslog(LOG_ERR, "%s device %s flush failed.",
 		       dev_type, dev_name);
 		r = ME_FAILURE;
 	} else if (status_code == 'S')
-		syslog(LOG_ERR, "%s device %s sync failed.\n",
+		syslog(LOG_ERR, "%s device %s sync failed.",
 		       dev_type, dev_name);
 	else if (status_code == 'R')
-		syslog(LOG_ERR, "%s device %s read failed.\n",
+		syslog(LOG_ERR, "%s device %s read failed.",
 		       dev_type, dev_name);
 	else if (status_code != 'A') {
-		syslog(LOG_ERR, "%s device %s has failed (%c).\n",
+		syslog(LOG_ERR, "%s device %s has failed (%c).",
 		       dev_type, dev_name, status_code);
 		r = ME_FAILURE;
 	}
@@ -143,7 +143,7 @@ static int _remove_failed_devices(const char *device)
 		return -ENAMETOOLONG;	/* FIXME These return code distinctions are not used so remove them! */
 
 	if (!dm_split_lvm_name(dmeventd_lvm2_pool(), device, &vg, &lv, &layer)) {
-		syslog(LOG_ERR, "Unable to determine VG name from %s",
+		syslog(LOG_ERR, "Unable to determine VG name from %s.",
 		       device);
 		return -ENOMEM;	/* FIXME Replace with generic error return - reason for failure has already got logged */
 	}
@@ -156,7 +156,7 @@ static int _remove_failed_devices(const char *device)
 	/* FIXME Is any sanity-checking required on %s? */
 	if (CMD_SIZE <= snprintf(cmd_str, CMD_SIZE, "lvconvert --config devices{ignore_suspended_devices=1} --repair --use-policies %s/%s", vg, lv)) {
 		/* this error should be caught above, but doesn't hurt to check again */
-		syslog(LOG_ERR, "Unable to form LVM command: Device name too long");
+		syslog(LOG_ERR, "Unable to form LVM command: Device name too long.");
 		return -ENAMETOOLONG; /* FIXME Replace with generic error return - reason for failure has already got logged */
 	}
 
@@ -184,12 +184,12 @@ void process_event(struct dm_task *dmt,
 					  &target_type, &params);
 
 		if (!target_type) {
-			syslog(LOG_INFO, "%s mapping lost.\n", device);
+			syslog(LOG_INFO, "%s mapping lost.", device);
 			continue;
 		}
 
 		if (strcmp(target_type, "mirror")) {
-			syslog(LOG_INFO, "%s has unmirrored portion.\n", device);
+			syslog(LOG_INFO, "%s has unmirrored portion.", device);
 			continue;
 		}
 
@@ -199,13 +199,13 @@ void process_event(struct dm_task *dmt,
 			   _part_ of the device is in sync
 			   Also, this is not an error
 			*/
-			syslog(LOG_NOTICE, "%s is now in-sync\n", device);
+			syslog(LOG_NOTICE, "%s is now in-sync.", device);
 			break;
 		case ME_FAILURE:
-			syslog(LOG_ERR, "Device failure in %s\n", device);
+			syslog(LOG_ERR, "Device failure in %s.", device);
 			if (_remove_failed_devices(device))
 				/* FIXME Why are all the error return codes unused? Get rid of them? */
-				syslog(LOG_ERR, "Failed to remove faulty devices in %s\n",
+				syslog(LOG_ERR, "Failed to remove faulty devices in %s.",
 				       device);
 			/* Should check before warning user that device is now linear
 			else
@@ -217,7 +217,7 @@ void process_event(struct dm_task *dmt,
 			break;
 		default:
 			/* FIXME Provide value then! */
-			syslog(LOG_INFO, "Unknown event received.\n");
+			syslog(LOG_INFO, "Unknown event received.");
 		}
 	} while (next);
 
@@ -231,7 +231,7 @@ int register_device(const char *device,
 		    void **unused __attribute((unused)))
 {
 	int r = dmeventd_lvm2_init();
-	syslog(LOG_INFO, "Monitoring mirror device %s for events", device);
+	syslog(LOG_INFO, "Monitoring mirror device %s for events.", device);
 	return r;
 }
 
@@ -241,7 +241,7 @@ int unregister_device(const char *device,
 		      int minor __attribute((unused)),
 		      void **unused __attribute((unused)))
 {
-	syslog(LOG_INFO, "No longer monitoring mirror device %s for events\n",
+	syslog(LOG_INFO, "No longer monitoring mirror device %s for events.",
 	       device);
 	dmeventd_lvm2_exit();
 	return 1;
-- 
1.7.0.1

[PATCH 5/7] Force C locale

[Zdenek Kabelac]

As we need to use mlockall() enforce "C" locales for dmeventd.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 daemons/dmeventd/dmeventd.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index f4b664b..0958e21 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -1698,6 +1698,9 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	/* Force C locale */
+	setenv("LANG", "C", 1);
+
 	if (!_debug)
 		_daemonize();
 
-- 
1.7.0.1

[PATCH 6/7] Update memlock

[Zdenek Kabelac]

Code moves initilization of stats values to _memlock_maps().
For dmeventd we need to use mlockall() - so avoid reading config value
and go with _use_mlockall code path.

Patch assumes dmeventd uses C locales!
Patch needs the call or memlock_inc_daemon() before memlock_inc()
(which is our common use case).

Some minor code cleanup patch for _un/_lock_mem_if_needed().

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/mm/memlock.c |   35 ++++++++++++++++++++++-------------
 1 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/lib/mm/memlock.c b/lib/mm/memlock.c
index 84cc86d..f857bc2 100644
--- a/lib/mm/memlock.c
+++ b/lib/mm/memlock.c
@@ -227,6 +227,10 @@ static int _memlock_maps(struct cmd_context *cmd, lvmlock_t lock, struct maps_st
 #endif
 	}
 
+	/* Reset statistic counters */
+	memset(mstats, 0, sizeof(*mstats));
+	rewind(_mapsh);
+
 	while ((n = getline(&line, &len, _mapsh)) != -1) {
 		line[n > 0 ? n - 1 : 0] = '\0'; /* remove \n */
 		if (!(ret = _maps_line(cmd, lock, line, mstats)))
@@ -246,12 +250,16 @@ static void _lock_mem(struct cmd_context *cmd)
 {
 	_allocate_memory();
 
-	_use_mlockall = find_config_tree_bool(cmd, "activation/use_mlockall", DEFAULT_USE_MLOCKALL);
+	/*
+	 * For daemon we need to use mlockall()
+	 * so even future adition of thread which may not even use lvm lib
+	 * will not block memory locked thread
+	 * Note: assuming _memlock_count_daemon is updated before _memlock_count
+         */
+	_use_mlockall = _memlock_count_daemon ? 1 :
+		find_config_tree_bool(cmd, "activation/use_mlockall", DEFAULT_USE_MLOCKALL);
 
 	if (!_use_mlockall) {
-		/* Reset statistic counters */
-		memset(&_mstats, 0, sizeof(_mstats));
-
 		if (!*_procselfmaps &&
 		    dm_snprintf(_procselfmaps, sizeof(_procselfmaps),
 				"%s" SELF_MAPS, cmd->proc_dir) < 0) {
@@ -280,13 +288,10 @@ static void _lock_mem(struct cmd_context *cmd)
 
 static void _unlock_mem(struct cmd_context *cmd)
 {
-	struct maps_stats unlock_mstats = { 0 };
+	struct maps_stats unlock_mstats;
 
 	log_very_verbose("Unlocking memory");
 
-	if (!_use_mlockall)
-		rewind(_mapsh);
-
 	if (!_memlock_maps(cmd, LVM_MUNLOCK, &unlock_mstats))
 		stack;
 
@@ -294,24 +299,26 @@ static void _unlock_mem(struct cmd_context *cmd)
 		if (fclose(_mapsh))
 			log_sys_error("fclose", _procselfmaps);
 
-		if (memcmp(&_mstats, &unlock_mstats, sizeof(unlock_mstats)))
-			log_error(INTERNAL_ERROR "Maps size mismatch (%ld,%ld,%ld) != (%ld,%ld,%ld)",
+		if (_mstats.r_size < unlock_mstats.r_size)
+			log_error(INTERNAL_ERROR "Maps lock(%ld,%ld,%ld) < unlock(%ld,%ld,%ld)",
 				  (long)_mstats.r_size, (long)_mstats.w_size, (long)_mstats.x_size,
 				  (long)unlock_mstats.r_size, (long)unlock_mstats.w_size, (long)unlock_mstats.x_size);
 	}
 
-	_release_memory();
 	if (setpriority(PRIO_PROCESS, 0, _priority))
 		log_error("setpriority %u failed: %s", _priority,
 			  strerror(errno));
+	_release_memory();
 }
 
-static void _lock_mem_if_needed(struct cmd_context *cmd) {
+static void _lock_mem_if_needed(struct cmd_context *cmd)
+{
 	if ((_memlock_count + _memlock_count_daemon) == 1)
 		_lock_mem(cmd);
 }
 
-static void _unlock_mem_if_possible(struct cmd_context *cmd) {
+static void _unlock_mem_if_possible(struct cmd_context *cmd)
+{
 	if ((_memlock_count + _memlock_count_daemon) == 0)
 		_unlock_mem(cmd);
 }
@@ -342,6 +349,8 @@ void memlock_dec(struct cmd_context *cmd)
 void memlock_inc_daemon(struct cmd_context *cmd)
 {
 	++_memlock_count_daemon;
+	if (_memlock_count_daemon == 1 && _memlock_count > 0)
+                log_error(INTERNAL_ERROR "_memlock_inc_daemon used after _memlock_inc.");
 	_lock_mem_if_needed(cmd);
 	log_debug("memlock_count_daemon inc to %d", _memlock_count_daemon);
 }
-- 
1.7.0.1

[PATCH 7/7] Count only readable size for memlock stats.

[Zdenek Kabelac]

As we mlock() only readable pages, makes statistics only
for readable bytes.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/mm/memlock.c |   37 ++++++++++++-------------------------
 1 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/lib/mm/memlock.c b/lib/mm/memlock.c
index f857bc2..1c6bda4 100644
--- a/lib/mm/memlock.c
+++ b/lib/mm/memlock.c
@@ -80,12 +80,7 @@ static FILE *_mapsh;
 static char _procselfmaps[PATH_MAX] = "";
 #define SELF_MAPS "/self/maps"
 
-struct maps_stats {
-	size_t r_size;
-	size_t w_size;
-	size_t x_size;
-};
-static struct maps_stats _mstats; /* statistic for maps locking */
+static size_t _mstats; /* statistic for maps locking */
 
 static void _touch_memory(void *mem, size_t size)
 {
@@ -125,7 +120,7 @@ static void _release_memory(void)
  * format described in kernel/Documentation/filesystem/proc.txt
  */
 static int _maps_line(struct cmd_context *cmd, lvmlock_t lock,
-		      const char* line, struct maps_stats* mstats)
+		      const char* line, size_t* mstats)
 {
 	const struct config_node *cn;
 	struct config_value *cv;
@@ -175,13 +170,7 @@ static int _maps_line(struct cmd_context *cmd, lvmlock_t lock,
 		}
 	}
 
-	if (fr == 'r')
-		mstats->r_size += sz;
-	if (fw == 'w')
-		mstats->w_size += sz;
-	if (fx == 'x')
-		mstats->x_size += sz;
-
+	*mstats += sz;
 	log_debug("%s %10ldKiB %12lx - %12lx %c%c%c%c %s",
 		  (lock == LVM_MLOCK) ? "mlock" : "munlock",
 		  ((long)sz + 1023) / 1024, from, to, fr, fw, fx, fp, line + pos);
@@ -201,7 +190,7 @@ static int _maps_line(struct cmd_context *cmd, lvmlock_t lock,
 	return 1;
 }
 
-static int _memlock_maps(struct cmd_context *cmd, lvmlock_t lock, struct maps_stats *mstats)
+static int _memlock_maps(struct cmd_context *cmd, lvmlock_t lock, size_t *mstats)
 {
 	char *line = NULL;
 	size_t len;
@@ -228,19 +217,18 @@ static int _memlock_maps(struct cmd_context *cmd, lvmlock_t lock, struct maps_st
 	}
 
 	/* Reset statistic counters */
-	memset(mstats, 0, sizeof(*mstats));
+	*mstats = 0;
 	rewind(_mapsh);
 
 	while ((n = getline(&line, &len, _mapsh)) != -1) {
 		line[n > 0 ? n - 1 : 0] = '\0'; /* remove \n */
-		if (!(ret = _maps_line(cmd, lock, line, mstats)))
-			break;
+		if (!_maps_line(cmd, lock, line, mstats))
+                        ret = 0;
 	}
 
 	free(line);
 
-	log_debug("Mapped sizes: r=%ld, w=%ld, x=%ld",
-		  (long)mstats->r_size,  (long)mstats->w_size, (long)mstats->x_size);
+	log_debug("Mapped size: %ld", (long)*mstats);
 
 	return ret;
 }
@@ -288,7 +276,7 @@ static void _lock_mem(struct cmd_context *cmd)
 
 static void _unlock_mem(struct cmd_context *cmd)
 {
-	struct maps_stats unlock_mstats;
+	size_t unlock_mstats;
 
 	log_very_verbose("Unlocking memory");
 
@@ -299,10 +287,9 @@ static void _unlock_mem(struct cmd_context *cmd)
 		if (fclose(_mapsh))
 			log_sys_error("fclose", _procselfmaps);
 
-		if (_mstats.r_size < unlock_mstats.r_size)
-			log_error(INTERNAL_ERROR "Maps lock(%ld,%ld,%ld) < unlock(%ld,%ld,%ld)",
-				  (long)_mstats.r_size, (long)_mstats.w_size, (long)_mstats.x_size,
-				  (long)unlock_mstats.r_size, (long)unlock_mstats.w_size, (long)unlock_mstats.x_size);
+		if (_mstats < unlock_mstats)
+			log_error(INTERNAL_ERROR "Maps lock %ld < unlock %ld",
+				  (long)_mstats, (long)unlock_mstats);
 	}
 
 	if (setpriority(PRIO_PROCESS, 0, _priority))
-- 
1.7.0.1