locking: use lock file timestamps to detect vg changes

A reporting command that is run concurrently with another
command modifying a VG may report either the old or new
VG state.  This flexibility means the reporting command
could be optimized to report metadata that was read prior
to taking the VG lock.

Using lock file timestamps, that window can be closed so
that metadata reported is always consistent with the held
VG lock.  In some cases, this additional consistency will
avoid warnings that could be produced when the command
compares the metadata with the dm kernel state.

The end result is that the optimization is used (to read
disks only once) and the reported metadata is consistent
with the dm kernel state, even if a concurrent command
is making changes.

A reporting command will now save the VG lock file
timestamps prior to scanning disks.  The VG metadata that
is read while scanning disks is saved in memory.

After the scan, when reporting each VG, the command will
lock the VG, and then check the lock file timestamp again.
If the timestamp is unchanged, then the metadata saved
from the scan is unchanged and is reused to report the VG.
If the timestamp has changed, then another command has
modified the metadata since the scan, and the metadata is
reread from disk prior to reporting it.

Changes to lock file handling to support this:
- lock files are no longer unlinked and recreated by every
  lvm command, but are left in place.
- a command modifying a VG (holding an exclusive flock)
  will update the lock file timestamp before unlocking it.

daemons/dmeventd/dmeventd.c

@@ -1745,7 +1745,8 @@ static void _init_thread_signals(void)
 	sigdelset(&my_sigset, SIGHUP);
 	sigdelset(&my_sigset, SIGQUIT);
 
-	pthread_sigmask(SIG_BLOCK, &my_sigset, NULL);
+	if (pthread_sigmask(SIG_BLOCK, &my_sigset, NULL))
+		log_sys_error("pthread_sigmask", "SIG_BLOCK");
 }
 
 /*

daemons/dmeventd/plugins/vdo/dmeventd_vdo.c

@@ -16,7 +16,12 @@
 #include "daemons/dmeventd/plugins/lvm2/dmeventd_lvm.h"
 #include "daemons/dmeventd/libdevmapper-event.h"
 
-/* Use parser from new device_mapper library */
+/*
+ * Use parser from new device_mapper library.
+ * Although during compilation we can see dm_vdo_status_parse()
+ * in runtime we are linked agains systems libdm 'older' library
+ * which does not provide this symbol and plugin fails to load
+ */
 #include "device_mapper/vdo/status.c"
 
 #include <sys/wait.h>

device_mapper/libdm-common.c

@@ -2012,7 +2012,8 @@ static int _sysfs_get_kernel_name(uint32_t major, uint32_t minor, char *buf, siz
 			log_sys_error("readlink", sysfs_path);
 		else {
 			log_sys_debug("readlink", sysfs_path);
-			return _sysfs_find_kernel_name(major, minor, buf, buf_size);
+			r = _sysfs_find_kernel_name(major, minor, buf, buf_size);
+			goto out;
 		}
 		goto bad;
 	}
@@ -2033,6 +2034,7 @@ static int _sysfs_get_kernel_name(uint32_t major, uint32_t minor, char *buf, siz
 	strcpy(buf, name);
 	r = 1;
 bad:
+out:
 	free(temp_buf);
 	free(sysfs_path);
 

lib/cache/lvmcache.c

@@ -21,7 +21,6 @@
 #include "lib/locking/locking.h"
 #include "lib/metadata/metadata.h"
 #include "lib/mm/memlock.h"
-#include "lib/datastruct/str_list.h"
 #include "lib/format_text/format-text.h"
 #include "lib/config/config.h"
 
@@ -1952,24 +1951,25 @@ struct lvmcache_info *lvmcache_add(struct labeller *labeller,
 
 			strncpy(dev->pvid, pvid_s, sizeof(dev->pvid));
 
-			/*
-			 * Keep the existing PV/dev in lvmcache, and save the
-			 * new duplicate in the list of duplicates.  After
-			 * scanning is complete, compare the duplicate devs
-			 * with those in lvmcache to check if one of the
-			 * duplicates is preferred and if so switch lvmcache to
-			 * use it.
-			 */
-
-			if (!(devl = zalloc(sizeof(*devl))))
-				return_NULL;
-			devl->dev = dev;
-
 			/* shouldn't happen */
 			if (dev_in_device_list(dev, &_initial_duplicates))
 				log_debug_cache("Initial duplicate already in list %s", dev_name(dev));
-			else
+			else {
+				/*
+				 * Keep the existing PV/dev in lvmcache, and save the
+				 * new duplicate in the list of duplicates.  After
+				 * scanning is complete, compare the duplicate devs
+				 * with those in lvmcache to check if one of the
+				 * duplicates is preferred and if so switch lvmcache to
+				 * use it.
+				 */
+
+				if (!(devl = zalloc(sizeof(*devl))))
+					return_NULL;
+				devl->dev = dev;
+
 				dm_list_add(&_initial_duplicates, &devl->list);
+			}
 
 			if (is_duplicate)
 				*is_duplicate = 1;

lib/device/dev-type.c

@@ -42,7 +42,6 @@ int dev_is_pmem(struct device *dev)
 {
 	FILE *fp;
 	char path[PATH_MAX];
-	char buffer[64];
 	int is_pmem = 0;
 
 	if (dm_snprintf(path, sizeof(path), "%sdev/block/%d:%d/queue/dax",
@@ -56,27 +55,16 @@ int dev_is_pmem(struct device *dev)
 	if (!(fp = fopen(path, "r")))
 		return 0;
 
-	if (!fgets(buffer, sizeof(buffer), fp)) {
-		log_warn("Failed to read %s.", path);
-		if (fclose(fp))
-			log_sys_debug("fclose", path);
-		return 0;
-	} else if (sscanf(buffer, "%d", &is_pmem) != 1) {
-		log_warn("Failed to parse %s '%s'.", path, buffer);
-		if (fclose(fp))
-			log_sys_debug("fclose", path);
-		return 0;
-	}
+	if (fscanf(fp, "%d", &is_pmem) != 1)
+		log_warn("Failed to parse DAX %s.", path);
+
+	if (is_pmem)
+		log_debug("%s is pmem", dev_name(dev));
 
 	if (fclose(fp))
 		log_sys_debug("fclose", path);
 
-	if (is_pmem) {
-		log_debug("%s is pmem", dev_name(dev));
-		return 1;
-	}
-
-	return 0;
+	return is_pmem ? 1 : 0;
 }
 
 int dev_is_lv(struct device *dev)
@@ -84,6 +72,7 @@ int dev_is_lv(struct device *dev)
 	FILE *fp;
 	char path[PATH_MAX];
 	char buffer[64];
+	int ret = 0;
 
 	if (dm_snprintf(path, sizeof(path), "%sdev/block/%d:%d/dm/uuid",
 			dm_sysfs_dir(),
@@ -96,17 +85,15 @@ int dev_is_lv(struct device *dev)
 	if (!(fp = fopen(path, "r")))
 		return 0;
 
-	if (!fgets(buffer, sizeof(buffer), fp)) {
+	if (!fgets(buffer, sizeof(buffer), fp))
 		log_warn("Failed to read %s.", path);
-		fclose(fp);
-		return 0;
-	}
+	else if (!strncmp(buffer, "LVM-", 4))
+		ret = 1;
 
-	fclose(fp);
+	if (fclose(fp))
+		log_sys_debug("fclose", path);
 
-	if (!strncmp(buffer, "LVM-", 4))
-		return 1;
-	return 0;
+	return ret;
 }
 
 struct dev_types *create_dev_types(const char *proc_dir,

lib/format_text/format-text.c

@@ -277,7 +277,6 @@ static int _raw_write_mda_header(const struct format_type *fmt,
 	dev_set_last_byte(dev, start_byte + MDA_HEADER_SIZE);
 
 	if (!dev_write_bytes(dev, start_byte, MDA_HEADER_SIZE, mdah)) {
-		dev_unset_last_byte(dev);
 		log_error("Failed to write mda header to %s fd %d", dev_name(dev), dev->bcache_fd);
 		return 0;
 	}
@@ -342,7 +341,7 @@ static struct raw_locn *_read_metadata_location_vg(struct device_area *dev_area,
 	 * Don't try to check existing metadata
 	 * if given vgname is an empty string.
 	 */
-	if (!*vgname)
+	if (!vgname || !*vgname)
 		return rlocn;
 
 	/*
@@ -532,29 +531,6 @@ static struct volume_group *_vg_read_precommit_raw(struct format_instance *fid,
 	return vg;
 }
 
-#define MAX_DESC_LEN 2048
-
-static char *_build_desc_write(struct cmd_context *cmd, struct volume_group *vg)
-{
-	size_t len = strlen(cmd->cmd_line) + 32;
-	char *desc;
-
-	if (len > MAX_DESC_LEN)
-		len = MAX_DESC_LEN;
-
-	if (!(desc = zalloc(len)))
-		return_NULL;
-
-	vg->write_count++;
-
-	if (vg->write_count == 1)
-		dm_snprintf(desc, len, "Write from %s.", cmd->cmd_line);
-	else
-		dm_snprintf(desc, len, "Write[%u] from %s.", vg->write_count, cmd->cmd_line);
-
-	return desc;
-}
-
 /*
  * VG metadata updates:
  *
@@ -599,6 +575,7 @@ static char *_build_desc_write(struct cmd_context *cmd, struct volume_group *vg)
 static int _vg_write_raw(struct format_instance *fid, struct volume_group *vg,
 			 struct metadata_area *mda)
 {
+	char desc[2048];
 	struct mda_context *mdac = (struct mda_context *) mda->metadata_locn;
 	struct text_fid_context *fidtc = (struct text_fid_context *) fid->private;
 	struct raw_locn *rlocn_old;
@@ -673,12 +650,15 @@ static int _vg_write_raw(struct format_instance *fid, struct volume_group *vg,
 		write_buf_size = fidtc->write_buf_size;
 		new_size = fidtc->new_metadata_size;
 	} else {
-		char *desc = _build_desc_write(fid->fmt->cmd, vg);
+		if (!vg->write_count++)
+			(void) dm_snprintf(desc, sizeof(desc), "Write from %s.", vg->cmd->cmd_line);
+		else
+			(void) dm_snprintf(desc, sizeof(desc), "Write[%u] from %s.", vg->write_count, vg->cmd->cmd_line);
+
 		new_size = text_vg_export_raw(vg, desc, &write_buf, &write_buf_size);
 		fidtc->write_buf = write_buf;
 		fidtc->write_buf_size = write_buf_size;
 		fidtc->new_metadata_size = new_size;
-		free(desc);
 	}
 
 	if (!new_size || !write_buf) {
@@ -989,7 +969,6 @@ static int _vg_write_raw(struct format_instance *fid, struct volume_group *vg,
 
 	if (!dev_write_bytes(mdac->area.dev, write1_start, (size_t)write1_size, write_buf)) {
 		log_error("Failed to write metadata to %s fd %d", devname, mdac->area.dev->bcache_fd);
-		dev_unset_last_byte(mdac->area.dev);
 		goto out;
 	}
 
@@ -1002,7 +981,6 @@ static int _vg_write_raw(struct format_instance *fid, struct volume_group *vg,
 		if (!dev_write_bytes(mdac->area.dev, write2_start, write2_size,
 				     write_buf + new_size - new_wrap)) {
 			log_error("Failed to write metadata wrap to %s fd %d", devname, mdac->area.dev->bcache_fd);
-			dev_unset_last_byte(mdac->area.dev);
 			goto out;
 		}
 	}

lib/format_text/text_label.c

@@ -332,14 +332,12 @@ static int _read_mda_header_and_metadata(const struct format_type *fmt,
 		log_warn("WARNING: bad metadata header on %s at %llu.",
 			 dev_name(mdac->area.dev),
 			 (unsigned long long)mdac->area.start);
-		if (mda)
-			mda->header_start = mdac->area.start;
+		mda->header_start = mdac->area.start;
 		*bad_fields |= BAD_MDA_HEADER;
 		return 0;
 	}
 
-	if (mda)
-		mda->header_start = mdah->start;
+	mda->header_start = mdah->start;
 
 	mda_set_ignored(mda, rlocn_is_ignored(mdah->raw_locns));
 

lib/label/hints.c

@@ -638,7 +638,8 @@ static int _read_hint_file(struct cmd_context *cmd, struct dm_list *hints, int *
 	char devpath[PATH_MAX];
 	FILE *fp;
 	struct dev_iter *iter;
-	struct hint *hint;
+	struct hint hint;
+	struct hint *alloc_hint;
 	struct device *dev;
 	char *split[HINT_LINE_WORDS];
 	char *name, *pvid, *devn, *vgname, *p, *filter_str = NULL;
@@ -662,11 +663,7 @@ static int _read_hint_file(struct cmd_context *cmd, struct dm_list *hints, int *
 		split[i] = NULL;
 
 	while (fgets(_hint_line, sizeof(_hint_line), fp)) {
-		if (!(hint = zalloc(sizeof(struct hint)))) {
-			ret = 0;
-			break;
-		}
-
+		memset(&hint, 0, sizeof(hint));
 		if (_hint_line[0] == '#')
 			continue;
 
@@ -704,13 +701,11 @@ static int _read_hint_file(struct cmd_context *cmd, struct dm_list *hints, int *
 			_filter_to_str(cmd, devices_global_filter_CFG, &filter_str);
 			if (!filter_str || strcmp(filter_str, _hint_line + keylen)) {
 				log_debug("ignore hints with different global_filter");
-				if (filter_str)
-					free(filter_str);
+				free(filter_str);
 				*needs_refresh = 1;
 				break;
 			}
-			if (filter_str)
-				free(filter_str);
+			free(filter_str);
 			continue;
 		}
 
@@ -719,23 +714,20 @@ static int _read_hint_file(struct cmd_context *cmd, struct dm_list *hints, int *
 			_filter_to_str(cmd, devices_filter_CFG, &filter_str);
 			if (!filter_str || strcmp(filter_str, _hint_line + keylen)) {
 				log_debug("ignore hints with different filter");
-				if (filter_str)
-					free(filter_str);
+				free(filter_str);
 				*needs_refresh = 1;
 				break;
 			}
-			if (filter_str)
-				free(filter_str);
+			free(filter_str);
 			continue;
 		}
 
 		keylen = strlen("scan_lvs:");
 		if (!strncmp(_hint_line, "scan_lvs:", keylen)) {
 			int scan_lvs = 0;
-			sscanf(_hint_line + keylen, "%u", &scan_lvs);
-
-			if (scan_lvs != cmd->scan_lvs) {
-				log_debug("ignore hints with different scan_lvs");
+			if ((sscanf(_hint_line + keylen, "%u", &scan_lvs) != 1) ||
+			    scan_lvs != cmd->scan_lvs) {
+				log_debug("ignore hints with different or unreadable scan_lvs");
 				*needs_refresh = 1;
 				break;
 			}
@@ -744,7 +736,11 @@ static int _read_hint_file(struct cmd_context *cmd, struct dm_list *hints, int *
 
 		keylen = strlen("devs_hash:");
 		if (!strncmp(_hint_line, "devs_hash:", keylen)) {
-			sscanf(_hint_line + keylen, "%u %u", &read_hash, &read_count);
+			if (sscanf(_hint_line + keylen, "%u %u", &read_hash, &read_count) != 2) {
+				log_debug("ignore hints with invalid devs_hash");
+				*needs_refresh = 1;
+				break;
+			}
 			continue;
 		}
 
@@ -764,19 +760,28 @@ static int _read_hint_file(struct cmd_context *cmd, struct dm_list *hints, int *
 		vgname = split[3];
 
 		if (name && !strncmp(name, "scan:", 5))
-			strncpy(hint->name, name+5, PATH_MAX);
+			if (!dm_strncpy(hint.name, name + 5, sizeof(hint.name)))
+				continue;
 
 		if (pvid && !strncmp(pvid, "pvid:", 5))
-			strncpy(hint->pvid, pvid+5, ID_LEN);
+			if (!dm_strncpy(hint.pvid, pvid + 5, sizeof(hint.pvid)))
+				continue;
 
 		if (devn && sscanf(devn, "devn:%d:%d", &major, &minor) == 2)
-			hint->devt = makedev(major, minor);
+			hint.devt = makedev(major, minor);
 
 		if (vgname && (strlen(vgname) > 3) && (vgname[4] != '-'))
-			strncpy(hint->vgname, vgname+3, NAME_LEN);
+			if (!dm_strncpy(hint.vgname, vgname + 3, sizeof(hint.vgname)))
+				continue;
 
-		log_debug("add hint %s %s %d:%d %s", hint->name, hint->pvid, major, minor, vgname);
-		dm_list_add(hints, &hint->list);
+		if (!(alloc_hint = malloc(sizeof(struct hint)))) {
+			ret = 0;
+			break;
+		}
+		memcpy(alloc_hint, &hint, sizeof(hint));
+
+		log_debug("add hint %s %s %d:%d %s", hint.name, hint.pvid, major, minor, vgname);
+		dm_list_add(hints, &alloc_hint->list);
 		found++;
 	}
 
@@ -911,13 +916,11 @@ int write_hint_file(struct cmd_context *cmd, int newhints)
 
 	_filter_to_str(cmd, devices_global_filter_CFG, &filter_str);
 	fprintf(fp, "global_filter:%s\n", filter_str ?: "-");
-	if (filter_str)
-		free(filter_str);
+	free(filter_str);
 
 	_filter_to_str(cmd, devices_filter_CFG, &filter_str);
 	fprintf(fp, "filter:%s\n", filter_str ?: "-");
-	if (filter_str)
-		free(filter_str);
+	free(filter_str);
 
 	fprintf(fp, "scan_lvs:%d\n", cmd->scan_lvs);
 
@@ -951,7 +954,6 @@ int write_hint_file(struct cmd_context *cmd, int newhints)
 		 * detect when the devices on the system change, which
 		 * invalidates the existing hints.
 		 */
-		memset(devpath, 0, sizeof(devpath));
 		strncpy(devpath, dev_name(dev), PATH_MAX);
 		hash = calc_crc(hash, (const uint8_t *)devpath, strlen(devpath));
 		count++;
@@ -1179,7 +1181,8 @@ static void _get_single_vgname_cmd_arg(struct cmd_context *cmd,
 
 	if (!(st = strchr(arg, '/'))) {
 		/* simple vgname */
-		name = strdup(arg);
+		if (!(name = strdup(arg)))
+			return;
 		goto check;
 	}
 
@@ -1187,7 +1190,8 @@ static void _get_single_vgname_cmd_arg(struct cmd_context *cmd,
 	for (p = arg; p < st; p++)
 		namebuf[i++] = *p;
 
-	name = strdup(namebuf);
+	if (!(name = strdup(namebuf)))
+		return;
 
 check:
 	/*
@@ -1202,6 +1206,8 @@ check:
 			return;
 		}
 	}
+
+	free(name);
 }
 
 /*
@@ -1261,8 +1267,9 @@ int get_hints(struct cmd_context *cmd, struct dm_list *hints_out, int *newhints,
 	 */
 	if (_newhints_exists()) {
 		log_debug("get_hints: newhints file");
-		if (!_hints_exists())
-			_touch_hints();
+		if (!_hints_exists() && !_touch_hints())
+			return 0;
+
 		if (!_lock_hints(cmd, LOCK_EX, NONBLOCK))
 			return 0;
 		/* create new hints after scan */
@@ -1355,6 +1362,9 @@ int get_hints(struct cmd_context *cmd, struct dm_list *hints_out, int *newhints,
 		  dm_list_size(devs_out), dm_list_size(devs_in));
 
 	dm_list_splice(hints_out, &hints_list);
+
+	free(vgname);
+
 	return 1;
 }
 

lib/label/label.c

@@ -218,7 +218,7 @@ int label_write(struct device *dev, struct label *label)
 
 	if (!dev_write_bytes(dev, offset, LABEL_SIZE, buf)) {
 		log_debug_devs("Failed to write label to %s", dev_name(dev));
-		r = 0;
+		return 0;
 	}
 
 	dev_unset_last_byte(dev);
@@ -655,7 +655,6 @@ static int _scan_list(struct cmd_context *cmd, struct dev_filter *f,
 	int submit_count;
 	int scan_failed;
 	int is_lvm_device;
-	int error;
 	int ret;
 
 	dm_list_init(&wait_devs);
@@ -702,12 +701,11 @@ static int _scan_list(struct cmd_context *cmd, struct dev_filter *f,
 
 	dm_list_iterate_items_safe(devl, devl2, &wait_devs) {
 		bb = NULL;
-		error = 0;
 		scan_failed = 0;
 		is_lvm_device = 0;
 
 		if (!bcache_get(scan_bcache, devl->dev->bcache_fd, 0, 0, &bb)) {
-			log_debug_devs("Scan failed to read %s error %d.", dev_name(devl->dev), error);
+			log_debug_devs("Scan failed to read %s.", dev_name(devl->dev));
 			scan_failed = 1;
 			scan_read_errors++;
 			scan_failed_count++;
@@ -1047,6 +1045,15 @@ int label_scan(struct cmd_context *cmd)
 	 */
 	_prepare_open_file_limit(cmd, dm_list_size(&scan_devs));
 
+	/*
+	 * Read and save the timestamps from VG lock files.  The lock file
+	 * timestamp is updated when a command releases an exclusive lock
+	 * (indicating it has changed the VG.)  So, the timestamps can be
+	 * checked later to detect if another command has changed the VG since
+	 * our label scan.
+	 */
+	file_lock_save_times(cmd);
+
 	/*
 	 * Do the main scan.
 	 */
@@ -1451,6 +1458,7 @@ bool dev_write_bytes(struct device *dev, uint64_t start, size_t len, void *data)
 	if (!bcache_write_bytes(scan_bcache, dev->bcache_fd, start, len, data)) {
 		log_error("Error writing device %s at %llu length %u.",
 			  dev_name(dev), (unsigned long long)start, (uint32_t)len);
+		dev_unset_last_byte(dev);
 		label_scan_invalidate(dev);
 		return false;
 	}
@@ -1458,6 +1466,7 @@ bool dev_write_bytes(struct device *dev, uint64_t start, size_t len, void *data)
 	if (!bcache_flush(scan_bcache)) {
 		log_error("Error writing device %s at %llu length %u.",
 			  dev_name(dev), (unsigned long long)start, (uint32_t)len);
+		dev_unset_last_byte(dev);
 		label_scan_invalidate(dev);
 		return false;
 	}

lib/locking/file_locking.c

@@ -26,12 +26,14 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <signal.h>
+#include <dirent.h>
 
 static char _lock_dir[PATH_MAX];
 
 static void _fin_file_locking(void)
 {
 	release_flocks(1);
+	free_flocks();
 }
 
 static void _reset_file_locking(void)
@@ -50,11 +52,12 @@ static int _file_lock_resource(struct cmd_context *cmd, const char *resource,
 			log_error("Too long locking filename %s/P_%s.", _lock_dir, resource + 1);
 			return 0;
 		}
-	} else
+	} else {
 		if (dm_snprintf(lockfile, sizeof(lockfile), "%s/V_%s", _lock_dir, resource) < 0) {
 			log_error("Too long locking filename %s/V_%s.", _lock_dir, resource);
 			return 0;
 		}
+	}
 
 	if (!lock_file(lockfile, flags))
 		return_0;
@@ -95,3 +98,72 @@ int init_file_locking(struct locking_type *locking, struct cmd_context *cmd,
 
 	return 1;
 }
+
+/*
+ * For each file in locking_dir with V_ and no aux,
+ * stat and save the file time.
+ */
+
+void file_lock_save_times(struct cmd_context *cmd)
+{
+	char lockfile[PATH_MAX];
+	DIR *dir;
+	struct dirent *de;
+	char *aux;
+
+	if (!(dir = opendir(_lock_dir)))
+		return;
+
+	while ((de = readdir(dir))) {
+		if (de->d_name[0] != 'V')
+			continue;
+		if ((aux = strchr(de->d_name, ':'))) {
+			if (!strncmp(aux, ":aux", 4))
+				continue;
+		}
+
+		if (dm_snprintf(lockfile, sizeof(lockfile), "%s/%s", _lock_dir, de->d_name) < 0)
+			continue;
+
+		lock_file_time_init(lockfile);
+	}
+	closedir(dir);
+}
+
+/*
+ * Check if the file lock timestamp is unchanged from when it was saved by
+ * file_lock_save_times.  Return true if unchanged.  Return false if the
+ * timestamp is different, or if there's no info to know.
+ */
+bool file_lock_time_unchanged(struct cmd_context *cmd, const char *resource)
+{
+	char lockfile[PATH_MAX];
+
+	/* shouldn't be used with this function */
+	if (!strcmp(resource, VG_GLOBAL))
+		return false;
+
+	if (dm_snprintf(lockfile, sizeof(lockfile), "%s/V_%s", _lock_dir, resource) < 0) {
+		log_error("Too long locking filename %s/V_%s.", _lock_dir, resource);
+		return false;
+	}
+
+	return lock_file_time_unchanged(lockfile);
+}
+
+void file_lock_remove_on_unlock(struct cmd_context *cmd, const char *resource)
+{
+	char lockfile[PATH_MAX];
+
+	/* shouldn't be used with this function */
+	if (!strcmp(resource, VG_GLOBAL))
+		return;
+
+	if (dm_snprintf(lockfile, sizeof(lockfile), "%s/V_%s", _lock_dir, resource) < 0) {
+		log_error("Too long locking filename %s/V_%s.", _lock_dir, resource);
+		return;
+	}
+
+	lock_file_remove_on_unlock(lockfile);
+}
+

lib/locking/locking.h

@@ -78,4 +78,8 @@ int lockf_global_convert(struct cmd_context *cmd, const char *mode);
 int lock_global(struct cmd_context *cmd, const char *mode);
 int lock_global_convert(struct cmd_context *cmd, const char *mode);
 
+void file_lock_save_times(struct cmd_context *cmd);
+bool file_lock_time_unchanged(struct cmd_context *cmd, const char *resource);
+void file_lock_remove_on_unlock(struct cmd_context *cmd, const char *resource);
+
 #endif

lib/metadata/cache_manip.c

@@ -23,7 +23,6 @@
 #include "lib/config/defaults.h"
 #include "lib/metadata/lv_alloc.h"
 #include "lib/misc/lvm-signal.h"
-#include "lib/activate/dev_manager.h"
 
 /* https://github.com/jthornber/thin-provisioning-tools/blob/master/caching/cache_metadata_size.cc */
 #define DM_TRANSACTION_OVERHEAD		4096  /* KiB */

lib/metadata/metadata.c

@@ -4586,6 +4586,7 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 	struct device *mda_dev, *dev_ret, *dev;
 	struct cached_vg_fmtdata *vg_fmtdata = NULL;	/* Additional format-specific data about the vg */
 	struct pv_list *pvl;
+	const char *rescan_reason = NULL;
 	int found_old_metadata = 0;
 	int found_md_component = 0;
 	unsigned use_previous_vg;
@@ -4622,16 +4623,27 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 	 *
 	 * The devs in the VG may be persistently inconsistent due to some
 	 * previous problem.  In this case, rescanning the labels here will
-	 * find the same inconsistency.  The VG repair (mistakenly done by
-	 * vg_read below) is supposed to fix that.
+	 * find the same inconsistency.
 	 *
-	 * FIXME: sort out the usage of the global lock (which is mixed up
-	 * with the orphan lock), and when we can tell that the global
-	 * lock is taken prior to the label scan, and still held here,
-	 * we can also skip the rescan in that case.
+	 * Even though it's acceptable to report old metadata that was scanned
+	 * prior to taking the VG lock, we can actually detect the rare case
+	 * when another command has written the metadata between the time of
+	 * our scan and us acquiring the VG lock.  We save the VG lock file
+	 * timestamp prior to scan, then check it again aftrr taking the VG
+	 * lock (file_lock_time_unchanged).  If the timestamp is different, it
+	 * means that another command has written the metadata since our scan,
+	 * and we rescan it here to report the latest metadata.
 	 */
-	if (!cmd->can_use_one_scan || lvmcache_scan_mismatch(cmd, vgname, vgid)) {
-		log_debug_metadata("Rescanning devices for %s %s", vgname, writing ? "rw" : "");
+
+	if (!cmd->can_use_one_scan)
+		rescan_reason = "disabled";
+	else if (lvmcache_scan_mismatch(cmd, vgname, vgid))
+		rescan_reason = "mismatch";
+	else if (!file_lock_time_unchanged(cmd, vgname))
+		rescan_reason = "changed";
+
+	if (rescan_reason) {
+		log_debug_metadata("Rescanning devices for %s %s (%s)", vgname, writing ? "rw" : "", rescan_reason);
 		if (writing)
 			lvmcache_label_rescan_vg_rw(cmd, vgname, vgid);
 		else

lib/metadata/mirror.c

@@ -266,7 +266,6 @@ static int _write_log_header(struct cmd_context *cmd, struct logical_volume *lv)
 	dev_set_last_byte(dev, sizeof(log_header));
 
 	if (!dev_write_bytes(dev, UINT64_C(0), sizeof(log_header), &log_header)) {
-		dev_unset_last_byte(dev);
 		log_error("Failed to write log header to %s.", name);
 		return 0;
 	}

lib/metadata/thin_manip.c

@@ -503,11 +503,11 @@ int update_pool_lv(struct logical_volume *lv, int activate)
 			 *   which Node has pool active.
 			 */
 			if (!activate_lv(lv->vg->cmd, lv)) {
-				init_dmeventd_monitor(monitored);
+				(void) init_dmeventd_monitor(monitored);
 				return_0;
 			}
 			if (!lv_is_active(lv)) {
-				init_dmeventd_monitor(monitored);
+				(void) init_dmeventd_monitor(monitored);
 				log_error("Cannot activate thin pool %s, perhaps skipped in lvm.conf volume_list?",
 					  display_lvname(lv));
 				return 0;

lib/metadata/writecache_manip.c

@@ -21,8 +21,6 @@
 #include "lib/metadata/segtype.h"
 #include "lib/activate/activate.h"
 #include "lib/config/defaults.h"
-#include "lib/metadata/lv_alloc.h"
-#include "lib/misc/lvm-signal.h"
 #include "lib/activate/dev_manager.h"
 
 int lv_is_writecache_origin(const struct logical_volume *lv)

lib/misc/lvm-flock.c

@@ -25,36 +25,16 @@
 struct lock_list {
 	struct dm_list list;
 	int lf;
+	unsigned ex:1;
+	unsigned remove_on_unlock:1;
 	char *res;
+	struct timespec save_time;
 };
 
 static struct dm_list _lock_list;
 static int _prioritise_write_locks;
 
-/* Drop lock known to be shared with another file descriptor. */
-static void _drop_shared_flock(const char *file, int fd)
-{
-	log_debug_locking("_drop_shared_flock %s.", file);
-
-	if (close(fd) < 0)
-		log_sys_debug("close", file);
-}
-
-static void _undo_flock(const char *file, int fd)
-{
-	struct stat buf1, buf2;
-
-	log_debug_locking("_undo_flock %s", file);
-	if (!flock(fd, LOCK_NB | LOCK_EX) &&
-	    !stat(file, &buf1) &&
-	    !fstat(fd, &buf2) &&
-	    is_same_inode(buf1, buf2))
-		if (unlink(file))
-			log_sys_debug("unlink", file);
-
-	if (close(fd) < 0)
-		log_sys_debug("close", file);
-}
+#define AUX_LOCK_SUFFIX ":aux"
 
 static struct lock_list *_get_lock_list_entry(const char *file)
 {
@@ -70,6 +50,19 @@ static struct lock_list *_get_lock_list_entry(const char *file)
 	return NULL;
 }
 
+static void _unlink_aux(const char *file)
+{
+	char aux_path[PATH_MAX];
+
+	if (dm_snprintf(aux_path, sizeof(aux_path), "%s%s", file, AUX_LOCK_SUFFIX) < 0) {
+		stack;
+		return;
+	}
+
+	if (unlink(aux_path))
+		log_sys_debug("unlink", aux_path);
+}
+
 static int _release_lock(const char *file, int unlock)
 {
 	struct lock_list *ll;
@@ -78,18 +71,54 @@ static int _release_lock(const char *file, int unlock)
 	dm_list_iterate_safe(llh, llt, &_lock_list) {
 		ll = dm_list_item(llh, struct lock_list);
 
+		if (ll->lf < 0)
+			continue;
+
 		if (!file || !strcmp(ll->res, file)) {
-			dm_list_del(llh);
+			/*
+			 * When a VG is being removed, and the flock is still
+			 * held for the VG, it sets the remove_on_unlock flag,
+			 * so that when the flock is unlocked, the lock file is
+			 * then also removed.
+			 */
+			if (file && unlock && ll->remove_on_unlock) {
+				log_debug("Unlocking %s and removing", ll->res);
+
+				if (_prioritise_write_locks)
+					_unlink_aux(ll->res);
+				if (flock(ll->lf, LOCK_NB | LOCK_UN))
+					log_sys_debug("flock", ll->res);
+				if (unlink(ll->res))
+					log_sys_debug("unlink", ll->res);
+				if (close(ll->lf) < 0)
+					log_sys_debug("close", ll->res);
+
+				dm_list_del(&ll->list);
+				free(ll->res);
+				free(ll);
+				return 1;
+			}
+
+			/*
+			 * Update the lock file timestamp when unlocking an
+			 * exclusive flock.  Other commands may use the
+			 * timestamp change to detect that the VG was changed.
+			 */
+			if (file && unlock && ll->ex) {
+				if (futimens(ll->lf, NULL) < 0)
+					log_debug("lock file %s time update error %d", file, errno);
+			}
+
 			if (unlock) {
 				log_very_verbose("Unlocking %s", ll->res);
 				if (flock(ll->lf, LOCK_NB | LOCK_UN))
 					log_sys_debug("flock", ll->res);
-				_undo_flock(ll->res, ll->lf);
-			} else
-				_drop_shared_flock(ll->res, ll->lf);
+			}
 
-			free(ll->res);
-			free(llh);
+			if (close(ll->lf) < 0)
+				log_sys_debug("close", ll->res);
+
+			ll->lf = -1;
 
 			if (file)
 				return 1;
@@ -154,8 +183,6 @@ static int _do_flock(const char *file, int *fd, int operation, uint32_t nonblock
 	return_0;
 }
 
-#define AUX_LOCK_SUFFIX ":aux"
-
 static int _do_write_priority_flock(const char *file, int *fd, int operation, uint32_t nonblock)
 {
 	int r, fd_aux = -1;
@@ -167,9 +194,11 @@ static int _do_write_priority_flock(const char *file, int *fd, int operation, ui
 	if ((r = _do_flock(file_aux, &fd_aux, LOCK_EX, 0))) {
 		if (operation == LOCK_EX) {
 			r = _do_flock(file, fd, operation, nonblock);
-			_undo_flock(file_aux, fd_aux);
+			if (close(fd_aux) < 0)
+				log_sys_debug("close", file_aux);
 		} else {
-			_undo_flock(file_aux, fd_aux);
+			if (close(fd_aux) < 0)
+				log_sys_debug("close", file_aux);
 			r = _do_flock(file, fd, operation, nonblock);
 		}
 	}
@@ -183,6 +212,7 @@ int lock_file(const char *file, uint32_t flags)
 	uint32_t nonblock = flags & LCK_NONBLOCK;
 	uint32_t convert = flags & LCK_CONVERT;
 	int r;
+	int ex = 0;
 	struct lock_list *ll;
 	char state;
 
@@ -194,6 +224,7 @@ int lock_file(const char *file, uint32_t flags)
 	case LCK_WRITE:
 		operation = LOCK_EX;
 		state = 'W';
+		ex = 1;
 		break;
 	case LCK_UNLOCK:
 		return _release_lock(file, 1);
@@ -210,22 +241,27 @@ int lock_file(const char *file, uint32_t flags)
 		log_very_verbose("Locking %s %c%c convert", ll->res, state,
 			 	 nonblock ? ' ' : 'B');
 		r = flock(ll->lf, operation);
-		if (!r)
+		if (!r) {
+			ll->ex = ex;
 			return 1;
+		}
 		log_error("Failed to convert flock on %s %d", file, errno);
 		return 0;
 	}
 
-	if (!(ll = malloc(sizeof(struct lock_list))))
-		return_0;
+	if (!(ll = _get_lock_list_entry(file))) {
+		if (!(ll = zalloc(sizeof(struct lock_list))))
+			return_0;
 
-	if (!(ll->res = strdup(file))) {
-		free(ll);
-		return_0;
+		if (!(ll->res = strdup(file))) {
+			free(ll);
+			return_0;
+		}
+
+		ll->lf = -1;
+		dm_list_add(&_lock_list, &ll->list);
 	}
 
-	ll->lf = -1;
-
 	log_very_verbose("Locking %s %c%c", ll->res, state,
 			 nonblock ? ' ' : 'B');
 
@@ -237,12 +273,9 @@ int lock_file(const char *file, uint32_t flags)
 	(void) dm_prepare_selinux_context(NULL, 0);
 
 	if (r)
-		dm_list_add(&_lock_list, &ll->list);
-	else {
-		free(ll->res);
-		free(ll);
+		ll->ex = ex;
+	else
 		stack;
-	}
 
 	return r;
 }
@@ -254,3 +287,93 @@ void init_flock(struct cmd_context *cmd)
 	_prioritise_write_locks =
 	    find_config_tree_bool(cmd, global_prioritise_write_locks_CFG, NULL);
 }
+
+void free_flocks(void)
+{
+	struct lock_list *ll, *ll2;
+
+	dm_list_iterate_items_safe(ll, ll2, &_lock_list) {
+		dm_list_del(&ll->list);
+		free(ll->res);
+		free(ll);
+	}
+}
+
+/*
+ * Save the lock file timestamps prior to scanning, so that the timestamps can
+ * be checked later (lock_file_time_unchanged) to see if the VG has been
+ * changed.
+ */
+
+void lock_file_time_init(const char *file)
+{
+	struct lock_list *ll;
+	struct stat buf;
+
+	if (stat(file, &buf) < 0)
+		return;
+
+	if (!(ll = _get_lock_list_entry(file))) {
+		if (!(ll = zalloc(sizeof(struct lock_list))))
+			return;
+
+		if (!(ll->res = strdup(file))) {
+			free(ll);
+			return;
+		}
+
+		ll->lf = -1;
+		ll->save_time = buf.st_mtim;
+		dm_list_add(&_lock_list, &ll->list);
+	}
+}
+
+/*
+ * Check if a lock file timestamp has been changed (by other command) since we
+ * saved it (lock_file_time_init).  Another command may have updated the lock
+ * file timestamp when releasing an ex flock (futimens above.)
+ */
+
+bool lock_file_time_unchanged(const char *file)
+{
+	struct lock_list *ll;
+	struct stat buf;
+	struct timespec *prev, *now;
+
+	if (stat(file, &buf) < 0) {
+		log_debug("lock_file_time_unchanged no file %s", file);
+		return false;
+	}
+
+	if (!(ll = _get_lock_list_entry(file))) {
+		log_debug("lock_file_time_unchanged no list item %s", file);
+		return false;
+	}
+
+	prev = &ll->save_time;
+	now = &buf.st_mtim;
+
+	if ((now->tv_sec == prev->tv_sec) && (now->tv_nsec == prev->tv_nsec)) {
+		log_debug("lock file %s unchanged from %llu.%llu", file,
+			  (unsigned long long)prev->tv_sec,
+			  (unsigned long long)prev->tv_nsec);
+		return true;
+	}
+
+	log_debug("lock file %s changed from %llu.%llu to %llu.%llu", file,
+		  (unsigned long long)prev->tv_sec,
+		  (unsigned long long)prev->tv_nsec,
+		  (unsigned long long)now->tv_sec,
+		  (unsigned long long)now->tv_nsec);
+
+	return false;
+}
+
+void lock_file_remove_on_unlock(const char *file)
+{
+	struct lock_list *ll;
+
+	if ((ll = _get_lock_list_entry(file)))
+		ll->remove_on_unlock = 1;
+}
+

lib/misc/lvm-flock.h

@@ -16,7 +16,12 @@
 #define _LVM_FLOCK_H
 
 void init_flock(struct cmd_context *cmd);
+void free_flocks(void);
 int lock_file(const char *file, uint32_t flags);
 void release_flocks(int unlock);
 
+void lock_file_time_init(const char *file);
+bool lock_file_time_unchanged(const char *file);
+void lock_file_remove_on_unlock(const char *file);
+
 #endif /* _LVM_FLOCK_H */

libdaemon/server/daemon-server.c

@@ -656,7 +656,8 @@ void daemon_start(daemon_state s)
 		failed = 1; /* FD out of available selectable set */
 
 	sigfillset(&new_set);
-	sigprocmask(SIG_SETMASK, NULL, &old_set);
+	if (sigprocmask(SIG_SETMASK, NULL, &old_set))
+		perror("sigprocmask error");
 
 	while (!failed) {
 		_reset_timeout(s);

test/Makefile.in

@@ -315,7 +315,7 @@ lib/dm-version-expected: $(top_srcdir)/VERSION_DM .lib-dir-stamp
 CMDS = lvm $(shell cat $(top_builddir)/tools/.commands 2>/dev/null)
 LIB = $(addprefix lib/, $(LIB_SECURETEST) $(LIB_DMSECURETEST) $(LIB_SHARED) $(LIB_LOCAL) $(LIB_NOT) $(LIB_LINK_NOT) $(LIB_FLAVOURS))
 
-.tests-stamp: $(ALL) $(LIB) $(SUBDIRS) lib/version-expected lib/dm-version-expected unit-test
+.tests-stamp: .lib-dir-stamp $(ALL) $(LIB) $(SUBDIRS) lib/version-expected lib/dm-version-expected unit-test
 	@echo "    [TEST-STAMP]"
 	@if test "$(srcdir)" != . ; then \
 		echo "Linking tests to builddir."; \

test/shell/lvconvert-raid-reshape-stripes-load-reload.sh

@@ -56,7 +56,7 @@ mount "$DM_DEV_DIR/$vg/$lv1" "$mount_dir"
 
 echo 3 >/proc/sys/vm/drop_caches
 # FIXME: This is filling up ram disk. Use sane amount of data please! Rate limit the data written!
-dd if=/dev/urandom of="$mount_dir/random" bs=1M count=50 conv=fdatasync
+dd if=/dev/urandom of="$mount_dir/random" bs=1M count=4 conv=fdatasync
 checksum_ "$mount_dir/random" >MD5
 
 # FIXME: wait_for_sync - is this really testing anything under load?

test/shell/nomda-missing.sh

@@ -74,6 +74,8 @@ check active $vg mirror12
 vgchange -a n $vg
 aux enable_dev "$dev3"
 aux disable_dev "$dev4"
+dmsetup table
+dmsetup info -c
 vgchange -aey $vg
 not vgck $vg
 

tools/args.h

@@ -230,9 +230,6 @@ arg(errorwhenfull_ARG, '\0', "errorwhenfull", bool_VAL, 0, 0,
     "(Also see dm-thin-pool kernel module option no_space_timeout.)\n"
     "See \\fBlvmthin\\fP(7) for more information.\n")
 
-arg(file_long_ARG, '\0', "file", string_VAL, 0, 0,
-    "File name.\n")
-
 arg(force_long_ARG, '\0', "force", 0, ARG_COUNTABLE, 0,
     "Force metadata restore even with thin pool LVs.\n"
     "Use with extreme caution. Most changes to thin metadata\n"

tools/command-lines.in

@@ -719,12 +719,12 @@ FLAGS: SECONDARY_SYNTAX
 # and the LV type is known.
 
 lvconvert --repair LV_cache_cachepool_mirror_raid_thinpool
-OO: --usepolicies, --interval Number, --poolmetadataspare Bool, --file_long String, OO_LVCONVERT
+OO: --usepolicies, --interval Number, --poolmetadataspare Bool, OO_LVCONVERT
 OP: PV ...
 ID: lvconvert_repair
 DESC: Replace failed PVs in a raid or mirror LV.
 DESC: Repair a thin pool.
-DESC: Repair a cache pool or cache.
+DESC: Repair a cache pool.
 RULE: all not lv_is_locked lv_is_pvmove
 RULE: --poolmetadataspare and LV_cache LV_cachepool LV_thinpool
 

tools/lvconvert.c

@@ -2480,339 +2480,10 @@ deactivate_pmslv:
 	return 1;
 }
 
-/*
- * Use cache_repair to read metadata from cachevol_lv and
- * write repaired metadata to new file.
+/* TODO: lots of similar code with  thinpool repair
+ *       investigate possible better code sharing...
  */
-
-static int _run_cache_repair_from_cachevol(struct cmd_context *cmd,
-			     struct logical_volume *cachevol_lv,
-			     const char *meta_file)
-{
-	char cachevol_lv_path[PATH_MAX];
-	char *cachevol_lv_name;
-	const char *dmdir = dm_dir();
-	const char *cache_repair = find_config_tree_str_allow_empty(cmd, global_cache_repair_executable_CFG, NULL);
-	const struct dm_config_node *cn;
-	const struct dm_config_value *cv;
-	const char *argv[MAX_PDATA_ARGS + 7]; /* Max supported args */
-	int args = 0;
-	int status;
-
-	if (!cache_repair || !cache_repair[0]) {
-		log_error("No cache_repair command set in lvm.conf. Repair is disabled.");
-		return 0;
-	}
-
-	if (!(cachevol_lv_name = dm_build_dm_name(cmd->mem, cachevol_lv->vg->name, cachevol_lv->name, NULL))) {
-		log_error("Failed to create dm name for %s.", display_lvname(cachevol_lv));
-		return 0;
-	}
-
-	if (dm_snprintf(cachevol_lv_path, sizeof(cachevol_lv_path), "%s/%s", dmdir, cachevol_lv_name) < 0) {
-		log_error("Failed to create path for %s.", display_lvname(cachevol_lv));
-		return 0;
-	}
-
-	if (!(cn = find_config_tree_array(cmd, global_cache_repair_options_CFG, NULL))) {
-		log_error(INTERNAL_ERROR "Unable to find configuration for global/cache_repair_options");
-		return 0;
-	}
-
-	for (cv = cn->v; cv && args < MAX_PDATA_ARGS; cv = cv->next) {
-		if (cv->type != DM_CFG_STRING) {
-			log_error("Invalid string in config file: "
-				  "global/cache_repair_options");
-			return 0;
-		}
-		argv[++args] = cv->v.str;
-	}
-
-	if (args >= MAX_PDATA_ARGS) {
-		log_error("Too many cache_repair_options set in lvm.conf.");
-		return 0;
-	}
-
-	argv[0] = cache_repair;
-	argv[++args] = "-i";
-	argv[++args] = cachevol_lv_path;
-	argv[++args] = "-o";
-	argv[++args] = meta_file;
-	argv[++args] = NULL;
-
-	if (!exec_cmd(cmd, (const char * const *)argv, &status, 1)) {
-		log_error("The cache_repair command failed (status %d).", status);
-		return 0;
-	}
-
-	log_print("cache_repair of metadata completed from %s to %s", display_lvname(cachevol_lv), meta_file);
-
-	return 1;
-}
-
-static int _run_cache_writeback_from_cachevol(struct cmd_context *cmd,
-			        struct logical_volume *cache_lv,
-			        struct logical_volume *cachevol_lv,
-			        const char *meta_file,
-				uint64_t data_offset_bytes)
-{
-	char orig_path[PATH_MAX];
-	char data_path[PATH_MAX];
-	char *orig_name;
-	char *data_name;
-	const char *dmdir = dm_dir();
-	const char *cache_writeback = "cache_writeback";
-	const char *argv[MAX_PDATA_ARGS + 7]; /* Max supported args */
-	char offset_str[16];
-	int args = 0;
-	int status;
-
-	if (!(orig_name = dm_build_dm_name(cmd->mem, cache_lv->vg->name, cache_lv->name, NULL))) {
-		log_error("Failed to create dm name for %s.", display_lvname(cache_lv));
-	}
-
-	if (!(data_name = dm_build_dm_name(cmd->mem, cachevol_lv->vg->name, cachevol_lv->name, NULL))) {
-		log_error("Failed to create dm name for %s.", display_lvname(cachevol_lv));
-	}
-
-	if (dm_snprintf(orig_path, sizeof(orig_path), "%s/%s", dmdir, orig_name) < 0) {
-		log_error("Failed to create path for %s.", display_lvname(cache_lv));
-		return 0;
-	}
-
-	/* _off suffix is temp hack to allow manual offset */
-	if (dm_snprintf(data_path, sizeof(data_path), "%s/%s_off", dmdir, data_name) < 0) {
-		log_error("Failed to create path for %s.", display_lvname(cachevol_lv));
-		return 0;
-	}
-
-	if (dm_snprintf(offset_str, sizeof(offset_str), "%llu",
-			(unsigned long long)data_offset_bytes) < 0)
-		return_0;
-
-	/* let the user set up an _off dm wrapper */
-	if (yes_no_prompt("Done creating wrapper %s with offset %u sectors? [y/n]: ",
-			  data_path, (uint32_t)data_offset_bytes/512) == 'n')
-		return_0;
-
-	argv[0] = cache_writeback;
-	argv[++args] = "--metadata-device";
-	argv[++args] = meta_file;
-	argv[++args] = "--origin-device";
-	argv[++args] = orig_path;
-	argv[++args] = "--fast-device";
-	argv[++args] = data_path;
-	/*
-	argv[++args] = "--fast-device-offset";
-	argv[++args] = offset_str;
-	*/
-	argv[++args] = "--no-metadata-update";
-	argv[++args] = NULL;
-
-	if (!exec_cmd(cmd, (const char * const *)argv, &status, 1)) {
-		log_error("The cache_writeback command failed (status %d).", status);
-		return 0;
-	}
-
-	log_print("cache_writeback of data completed from %s to %s",
-		  display_lvname(cachevol_lv), display_lvname(cache_lv));
-
-	return 1;
-}
-
-static int _lvconvert_cache_repair_cachevol(struct cmd_context *cmd,
-				   struct logical_volume *cache_lv,
-				   struct dm_list *pvh)
-{
-	char filepath[PATH_MAX];
-	char zero_sector_buf[512];
-	struct volume_group *vg = cache_lv->vg;
-	struct logical_volume *cachevol_lv;
-	struct lv_segment *cache_seg, *cachevol_seg;
-	uint64_t metadata_len_sectors, data_offset_bytes;
-	uint64_t orig_cachevol_seglen;
-	const char *meta_file = NULL;
-	FILE *fp;
-	uint32_t prealloc_sectors_written = 0;
-	int cache_mode;
-	int ret = 0;
-
-	memset(zero_sector_buf, 0, 512);
-
-	if (!(cache_seg = first_seg(cache_lv)))
-		return_0;
-
-	if (!(cachevol_lv = cache_seg->pool_lv))
-		return_0;
-
-	if (!lv_is_cache_vol(cachevol_lv))
-		return_0;
-
-	if (cache_seg->metadata_start) {
-		/* metadata is always at 0, but check in case it changes in future */
-		log_error("Cannot handle non-zero metadata start locations.");
-		return 0;
-	}
-
-	if (lv_is_active(cache_lv)) {
-		log_error("LV %s must not be active.", display_lvname(cache_lv));
-		return 0;
-	}
-
-	if (lv_is_active(cachevol_lv)) {
-		log_error("LV %s must not be active.", display_lvname(cachevol_lv));
-		return 0;
-	}
-
-	if (vg_is_shared(vg)) {
-		/* cache LV using the cachevol cannot be active elsewhere */
-		if (!lockd_lv(cmd, cache_lv, "ex", 0))
-			return_0;
-
-		/* cachevol cannot be active elsewhere. */
-		if (!lockd_lv(cmd, cachevol_lv, "ex", 0))
-			return_0;
-	}
-
-	metadata_len_sectors = cache_seg->metadata_len;
-	cache_mode = cache_seg->cache_mode;
-
-	/* Disable syncing the cache during split */
-	if (cache_mode != CACHE_MODE_WRITETHROUGH)
-		cache_seg->cache_mode = CACHE_MODE_WRITETHROUGH;
-
-	if (!_lvconvert_split_and_keep_cachevol(cmd, cache_lv, cachevol_lv)) {
-		log_error("Failed to detach cachevol, not repairing.");
-		return_0;
-	}
-
-	/*
-	 * Note: following the spilt, cache_lv is no longer technically a cache
-	 * and cachevol_lv is no longer technically a cachevol.  The variable
-	 * names represent their previous types.
-	 */
-
-	if (cache_mode == CACHE_MODE_WRITETHROUGH) {
-		log_print("No repair or writeback required for writethrough cache mode.");
-		return 1;
-	}
-
-	/*
-	 * Prevent the active LVs from being exposed and used outside of lvm.
-	 */
-	cachevol_lv->status |= LV_TEMPORARY;
-	cache_lv->status |= LV_TEMPORARY;
-
-	/*
-	 * Temporarily change the cachevol LV size to be only the
-	 * metadata size, so cache_repair will only look at the
-	 * metadata portion.
-	 * seg->len and seg->metadata_len units are sectors
-	 */
-	cachevol_seg = first_seg(cachevol_lv);
-	orig_cachevol_seglen = cachevol_seg->len;
-	cachevol_seg->len = metadata_len_sectors / vg->extent_size;
-
-	if (!activate_lv(cmd, cachevol_lv)) {
-		log_error("Failed to activate LV %s for cache repair and writeback.", display_lvname(cachevol_lv));
-		goto out;
-	}
-
-	if (!sync_local_dev_names(cmd)) {
-		log_error("Failed to sync local devices before cache_repair.");
-		goto out;
-	}
-
-	/*
-	 * cache_repair wants the destination file to be preallocated.
-	 * FIXME: what's the relationship between the metadata size and
-	 * the file size?
-	 */
-
-	if (arg_is_set(cmd, file_long_ARG))
-		meta_file = arg_str_value(cmd, file_long_ARG, NULL);
-	else {
-		snprintf(filepath, PATH_MAX, "/tmp/lvconvert-cache-repair-metadata-%d", getpid());
-		meta_file = filepath;
-	}
-
-	log_print("Allocating %u MB for metadata in %s",
-		  (uint32_t)(metadata_len_sectors * 512 / 1048576), meta_file);
-
-	if (!(fp = fopen(meta_file, "w+"))) {
-		log_error("Failed to open file for metadata %s", meta_file);
-		meta_file = NULL;
-		goto out;
-	}
-	while (prealloc_sectors_written < metadata_len_sectors) {
-		if (!fwrite(zero_sector_buf, 512, 1, fp)) {
-			fclose(fp);
-			log_error("Failed to allocate space for metadata in %s", meta_file);
-			goto out;
-		}
-		prealloc_sectors_written++;
-	}
-	fflush(fp);
-	fclose(fp);
-
-	if (!_run_cache_repair_from_cachevol(cmd, cachevol_lv, meta_file)) {
-		log_error("cache_repair failed, cache data not written back to original volume.");
-		goto out;
-	}
-
-	/*
-	 * Deactivate cachevol LV, and reactivate with correct size.
-	 * Activate cache LV.
-	 */
-
-	if (!deactivate_lv(cmd, cachevol_lv)) {
-		log_error("Failed to deactivate LV %s", cachevol_lv->name);
-		goto out;
-	}
-
-	cachevol_seg->len = orig_cachevol_seglen;
-
-	if (!activate_lv(cmd, cachevol_lv)) {
-		log_error("Failed to activate LV %s for cache repair and writeback.", display_lvname(cachevol_lv));
-		goto out;
-	}
-
-	if (!activate_lv(cmd, cache_lv)) {
-		log_error("Failed to activate LV %s for cache repair and writeback.", display_lvname(cache_lv));
-		goto out;
-	}
-
-	if (!sync_local_dev_names(cmd)) {
-		log_error("Failed to sync local devices before cache_writeback.");
-		goto out;
-	}
-
-	data_offset_bytes = metadata_len_sectors * 512;
-
-	if (!_run_cache_writeback_from_cachevol(cmd, cache_lv, cachevol_lv, meta_file, data_offset_bytes)) {
-		log_error("cache_writeback failed");
-		goto_out;
-	}
-
-	log_print("cachevol writeback complete, %s now unused.", cachevol_lv->name);
-	ret = 1;
-out:
-	if (!deactivate_lv(cmd, cache_lv))
-		log_error("Failed to deactivate LV %s", cache_lv->name);
-
-	if (!deactivate_lv(cmd, cachevol_lv))
-		log_error("Failed to deactivate LV %s", cachevol_lv->name);
-
-	if (meta_file)
-		unlink(meta_file);
-
-	if (!ret)
-		log_error("Cache data on %s not written back to original LV %s.",
-			  display_lvname(cachevol_lv), display_lvname(cache_lv));
-	return ret;
-}
-
-static int _lvconvert_cache_repair_cachepool(struct cmd_context *cmd,
+static int _lvconvert_cache_repair(struct cmd_context *cmd,
 				   struct logical_volume *cache_lv,
 				   struct dm_list *pvh, int poolmetadataspare)
 {
@@ -2831,6 +2502,11 @@ static int _lvconvert_cache_repair_cachepool(struct cmd_context *cmd,
 	struct logical_volume *pmslv;
 	struct logical_volume *mlv;
 
+	if (lv_is_cache(cache_lv) && lv_is_cache_vol(first_seg(cache_lv)->pool_lv)) {
+		log_error("Manual repair required.");
+		return 0;
+	}
+
 	pool_lv = lv_is_cache_pool(cache_lv) ? cache_lv : first_seg(cache_lv)->pool_lv;
 	mlv = first_seg(pool_lv)->metadata_lv;
 
@@ -4093,7 +3769,7 @@ static int _lvconvert_repair_pvs(struct cmd_context *cmd, struct logical_volume
 	return ret ? ECMD_PROCESSED : ECMD_FAILED;
 }
 
-static int _lvconvert_repair_cache_or_thin(struct cmd_context *cmd, struct logical_volume *lv,
+static int _lvconvert_repair_cachepool_thinpool(struct cmd_context *cmd, struct logical_volume *lv,
 			struct processing_handle *handle)
 {
 	int poolmetadataspare = arg_int_value(cmd, poolmetadataspare_ARG, DEFAULT_POOL_METADATA_SPARE);
@@ -4114,13 +3790,8 @@ static int _lvconvert_repair_cache_or_thin(struct cmd_context *cmd, struct logic
 		if (!_lvconvert_thin_pool_repair(cmd, lv, use_pvh, poolmetadataspare))
 			return_ECMD_FAILED;
 	} else /* cache */ {
-		if (lv_is_cache(lv) && lv_is_cache_vol(first_seg(lv)->pool_lv)) {
-			if (!_lvconvert_cache_repair_cachevol(cmd, lv, use_pvh))
-				return_ECMD_FAILED;
-		} else {
-			if (!_lvconvert_cache_repair_cachepool(cmd, lv, use_pvh, poolmetadataspare))
-				return_ECMD_FAILED;
-		}
+		if (!_lvconvert_cache_repair(cmd, lv, use_pvh, poolmetadataspare))
+			return_ECMD_FAILED;
 	}
 
 	return ECMD_PROCESSED;
@@ -4132,7 +3803,7 @@ static int _lvconvert_repair_single(struct cmd_context *cmd, struct logical_volu
 	if (lv_is_thin_pool(lv) ||
 	    lv_is_cache(lv) ||
 	    lv_is_cache_pool(lv))
-		return _lvconvert_repair_cache_or_thin(cmd, lv, handle);
+		return _lvconvert_repair_cachepool_thinpool(cmd, lv, handle);
 
 	if (lv_is_raid(lv) || lv_is_mirror(lv))
 		return _lvconvert_repair_pvs(cmd, lv, handle);
@@ -4143,6 +3814,12 @@ static int _lvconvert_repair_single(struct cmd_context *cmd, struct logical_volu
 	return ECMD_FAILED;
 }
 
+/*
+ * FIXME: add option --repair-pvs to call _lvconvert_repair_pvs() directly,
+ * and option --repair-thinpool to call _lvconvert_repair_thinpool().
+ * and option --repair-cache to call _lvconvert_repair_cache().
+ * and option --repair-cachepool to call _lvconvert_repair_cachepool().
+ */
 int lvconvert_repair_cmd(struct cmd_context *cmd, int argc, char **argv)
 {
 	struct processing_handle *handle;

tools/pvck.c

@@ -359,7 +359,10 @@ static int _dump_all_text(struct cmd_context *cmd, const char *tofile, struct de
 			continue;
 		}
 
-		sscanf(line, "seqno = %u", &seqno);
+		if (sscanf(line, "seqno = %u", &seqno) != 1) {
+			log_warn("WARNING: Failed to parse seqno.");
+			seqno = 0; /* Skip? */
+		}
 
 		/*
 		 * The first three lines look like metadata with
@@ -417,11 +420,11 @@ static int _dump_all_text(struct cmd_context *cmd, const char *tofile, struct de
 				memset(line, 0, sizeof(line));
 				_copy_line(str1, line, &len);
 				log_print("%s", line);
-			}
-			if ((str2 = strstr(str1, "creation_time = "))) {
-				memset(line, 0, sizeof(line));
-				_copy_line(str2, line, &len);
-				log_print("%s\n", line);
+				if ((str2 = strstr(str1, "creation_time = "))) {
+					memset(line, 0, sizeof(line));
+					_copy_line(str2, line, &len);
+					log_print("%s\n", line);
+				}
 			}
 		}
 
@@ -652,27 +655,31 @@ static int _dump_meta_area(struct device *dev, const char *tofile,
 	if (!tofile)
 		return_0;
 
-	if (!(meta_buf = malloc(mda_size)))
+	if (!(meta_buf = zalloc(mda_size)))
 		return_0;
-	memset(meta_buf, 0, mda_size);
 
 	if (!dev_read_bytes(dev, mda_offset, mda_size, meta_buf)) {
 		log_print("CHECK: failed to read metadata area at offset %llu size %llu",
 			  (unsigned long long)mda_offset, (unsigned long long)mda_size);
+		free(meta_buf);
 		return 0;
 	}
 
 	if (!(fp = fopen(tofile, "wx"))) {
-		log_error("Failed to create file %s", tofile);
+		log_error("Failed to create file %s.", tofile);
+		free(meta_buf);
 		return 0;
 	}
 
-	fwrite(meta_buf, mda_size - 512, 1, fp);
+	if (fwrite(meta_buf, mda_size - 512, 1, fp) < (mda_size - 512))
+		log_warn("WARNING: Failed to write " FMTu64 " bytes to file %s.", mda_size - 512, tofile);
 
+	free(meta_buf);
 	if (fflush(fp))
 		stack;
 	if (fclose(fp))
 		stack;
+
 	return 1;
 }
 
@@ -705,12 +712,11 @@ static int _dump_current_text(struct device *dev,
 	int ri = rlocn_index; /* 0 or 1 */
 	int bad = 0;
 
-	if (!(meta_buf = malloc(meta_size))) {
+	if (!(meta_buf = zalloc(meta_size))) {
 		log_print("CHECK: mda_header_%d.raw_locn[%d] no mem for metadata text size %llu", mn, ri,
 			  (unsigned long long)meta_size);
 		return 0;
 	}
-	memset(meta_buf, 0, meta_size);
 
 	/*
 	 * Read the metadata text specified by the raw_locn so we can
@@ -721,7 +727,7 @@ static int _dump_current_text(struct device *dev,
 	 * mda_offset + meta_offset.
 	 */
 	if (meta_offset + meta_size > mda_size) {
-	 	/* text metadata wraps to start of text metadata area */
+		/* text metadata wraps to start of text metadata area */
 		uint32_t wrap = (uint32_t) ((meta_offset + meta_size) - mda_size);
 		off_t offset_a = mda_offset + meta_offset;
 		uint32_t size_a = meta_size - wrap;
@@ -732,6 +738,7 @@ static int _dump_current_text(struct device *dev,
 			log_print("CHECK: failed to read metadata text at mda_header_%d.raw_locn[%d].offset %llu size %llu part_a %llu %llu", mn, ri,
 				  (unsigned long long)meta_offset, (unsigned long long)meta_size,
 				  (unsigned long long)offset_a, (unsigned long long)size_a);
+			free(meta_buf);
 			return 0;
 		}
 
@@ -739,12 +746,14 @@ static int _dump_current_text(struct device *dev,
 			log_print("CHECK: failed to read metadata text at mda_header_%d.raw_locn[%d].offset %llu size %llu part_b %llu %llu", mn, ri,
 				  (unsigned long long)meta_offset, (unsigned long long)meta_size,
 				  (unsigned long long)offset_b, (unsigned long long)size_b);
+			free(meta_buf);
 			return 0;
 		}
 	} else {
 		if (!dev_read_bytes(dev, mda_offset + meta_offset, meta_size, meta_buf)) {
 			log_print("CHECK: failed to read metadata text at mda_header_%d.raw_locn[%d].offset %llu size %llu", mn, ri,
 				  (unsigned long long)meta_offset, (unsigned long long)meta_size);
+			free(meta_buf);
 			return 0;
 		}
 	}
@@ -802,9 +811,9 @@ static int _dump_current_text(struct device *dev,
 	}
 
  out:
-	if (bad)
-		return 0;
-	return 1;
+	free(meta_buf);
+
+	return (!bad) ? 1 : 0;
 }
 
 static int _dump_label_and_pv_header(struct cmd_context *cmd, int print_fields,
@@ -848,6 +857,7 @@ static int _dump_label_and_pv_header(struct cmd_context *cmd, int print_fields,
 	if (!dev_read_bytes(dev, lh_offset, 512, buf)) {
 		log_print("CHECK: failed to read label_header at %llu",
 			  (unsigned long long)lh_offset);
+		free(buf);
 		return 0;
 	}
 
@@ -1042,9 +1052,9 @@ static int _dump_label_and_pv_header(struct cmd_context *cmd, int print_fields,
 			  (unsigned long long)xlate64(dlocn->size));
 	}
 
-	if (bad)
-		return 0;
-	return 1;
+	free(buf);
+
+	return (!bad) ? 1 : 0;
 }
 
 /*
@@ -1097,6 +1107,7 @@ static int _dump_mda_header(struct cmd_context *cmd,
 
 	if (!dev_read_bytes(dev, mda_offset, 512, buf)) {
 		log_print("CHECK: failed to read mda_header at %llu", (unsigned long long)mda_offset);
+		free(buf);
 		return 0;
 	}
 
@@ -1187,11 +1198,9 @@ static int _dump_mda_header(struct cmd_context *cmd,
 
 	/* Should we also check text metadata if it exists in rlocn1? */
  out:
-	if (buf)
-		free(buf);
-	if (bad)
-		return 0;
-	return 1;
+	free(buf);
+
+	return (!bad) ? 1 : 0;
 }
 
 static int _dump_headers(struct cmd_context *cmd,
@@ -1457,7 +1466,8 @@ static int _dump_search(struct cmd_context *cmd,
 		uint64_t dev_bytes;
 		uint64_t extra_bytes;
 
-		dev_get_size(dev, &dev_sectors);
+		if (!dev_get_size(dev, &dev_sectors))
+			return_ECMD_FAILED;
 
 		dev_bytes = dev_sectors * 512;
 		extra_bytes = dev_bytes % ONE_MB_IN_BYTES;
@@ -1484,9 +1494,8 @@ static int _dump_search(struct cmd_context *cmd,
 	log_print("Searching for metadata in mda%d at offset %llu size %llu", mda_num,
 		  (unsigned long long)mda_offset, (unsigned long long)mda_size);
 
-	if (!(buf = malloc(mda_size)))
+	if (!(buf = zalloc(mda_size)))
 		return ECMD_FAILED;
-	memset(buf, 0, mda_size);
 
 	if (!dev_read_bytes(dev, mda_offset, mda_size, buf)) {
 		log_print("CHECK: failed to read metadata area at offset %llu size %llu",

tools/pvscan.c

@@ -420,7 +420,7 @@ check_duplicate:
 		log_error("pvscan[%d] PV %s is duplicate for PVID %s on %d:%d and %d:%d.",
 			  getpid(), dev_name(dev), dev->pvid, major, minor, file_major, file_minor);
 
-	if (file_vgname[0] && strcmp(file_vgname, vgname))
+	if (file_vgname[0] && vgname && strcmp(file_vgname, vgname))
 		log_error("pvscan[%d] PV %s has unexpected VG %s vs %s.",
 			  getpid(), dev_name(dev), vgname, file_vgname);
 

tools/toollib.c

@@ -4284,7 +4284,8 @@ static int _process_pvs_in_vg(struct cmd_context *cmd,
 		/*
 		 * When processing only specific PVs, we can quit once they've all been found.
 	 	 */
-		if (!process_all_pvs && dm_list_empty(arg_tags) && dm_list_empty(arg_devices))
+		if (!process_all_pvs && dm_list_empty(arg_tags) &&
+		    (!arg_devices || dm_list_empty(arg_devices)))
 			break;
 		log_set_report_object_name_and_id(NULL, NULL);
 	}

tools/vgremove.c

@@ -78,6 +78,8 @@ static int _vgremove_single(struct cmd_context *cmd, const char *vg_name,
 
 	lockd_free_vg_final(cmd, vg);
 
+	file_lock_remove_on_unlock(cmd, vg->name);
+
 	return ECMD_PROCESSED;
 }