cache: repair and writeback for cachevol

For a dm-cache LV with an attached cachevol using writeback,
'lvconvert --repair LV' will:

. detach the cachevol
. run cache_repair from the cachevol to a temp file
. run cache_writeback to copy blocks from the cachevol
  back to the original LV, using the repaired metadata in
  the temp file

Requires new --fast-device-offset option for cache_writeback
command.

daemons/dmeventd/dmeventd.c

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

daemons/dmeventd/plugins/vdo/dmeventd_vdo.c

@@ -16,12 +16,7 @@
 #include "daemons/dmeventd/plugins/lvm2/dmeventd_lvm.h"
 #include "daemons/dmeventd/libdevmapper-event.h"
 
-/*
- * 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
- */
+/* Use parser from new device_mapper library */
 #include "device_mapper/vdo/status.c"
 
 #include <sys/wait.h>

device_mapper/libdm-common.c

@@ -2012,8 +2012,7 @@ 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);
-			r = _sysfs_find_kernel_name(major, minor, buf, buf_size);
-			goto out;
+			return _sysfs_find_kernel_name(major, minor, buf, buf_size);
 		}
 		goto bad;
 	}
@@ -2034,7 +2033,6 @@ 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,6 +21,7 @@
 #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"
 
@@ -1951,25 +1952,24 @@ 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 {
-				/*
-				 * 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;
-
+			else
 				dm_list_add(&_initial_duplicates, &devl->list);
-			}
 
 			if (is_duplicate)
 				*is_duplicate = 1;

lib/device/dev-type.c

@@ -42,6 +42,7 @@ 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",
@@ -55,16 +56,27 @@ int dev_is_pmem(struct device *dev)
 	if (!(fp = fopen(path, "r")))
 		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 (!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 (fclose(fp))
 		log_sys_debug("fclose", path);
 
-	return is_pmem ? 1 : 0;
+	if (is_pmem) {
+		log_debug("%s is pmem", dev_name(dev));
+		return 1;
+	}
+
+	return 0;
 }
 
 int dev_is_lv(struct device *dev)
@@ -72,7 +84,6 @@ 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(),
@@ -85,15 +96,17 @@ 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);
-	else if (!strncmp(buffer, "LVM-", 4))
-		ret = 1;
+		fclose(fp);
+		return 0;
+	}
 
-	if (fclose(fp))
-		log_sys_debug("fclose", path);
+	fclose(fp);
 
-	return ret;
+	if (!strncmp(buffer, "LVM-", 4))
+		return 1;
+	return 0;
 }
 
 struct dev_types *create_dev_types(const char *proc_dir,

lib/format_text/format-text.c

@@ -277,6 +277,7 @@ 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;
 	}
@@ -341,7 +342,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 || !*vgname)
+	if (!*vgname)
 		return rlocn;
 
 	/*
@@ -531,6 +532,29 @@ 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:
  *
@@ -575,7 +599,6 @@ static struct volume_group *_vg_read_precommit_raw(struct format_instance *fid,
 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;
@@ -650,15 +673,12 @@ 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 {
-		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);
-
+		char *desc = _build_desc_write(fid->fmt->cmd, vg);
 		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) {
@@ -969,6 +989,7 @@ 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;
 	}
 
@@ -981,6 +1002,7 @@ 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,12 +332,14 @@ 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);
-		mda->header_start = mdac->area.start;
+		if (mda)
+			mda->header_start = mdac->area.start;
 		*bad_fields |= BAD_MDA_HEADER;
 		return 0;
 	}
 
-	mda->header_start = mdah->start;
+	if (mda)
+		mda->header_start = mdah->start;
 
 	mda_set_ignored(mda, rlocn_is_ignored(mdah->raw_locns));
 

lib/label/hints.c

@@ -638,8 +638,7 @@ 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 *alloc_hint;
+	struct hint *hint;
 	struct device *dev;
 	char *split[HINT_LINE_WORDS];
 	char *name, *pvid, *devn, *vgname, *p, *filter_str = NULL;
@@ -663,7 +662,11 @@ 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)) {
-		memset(&hint, 0, sizeof(hint));
+		if (!(hint = zalloc(sizeof(struct hint)))) {
+			ret = 0;
+			break;
+		}
+
 		if (_hint_line[0] == '#')
 			continue;
 
@@ -701,11 +704,13 @@ 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");
-				free(filter_str);
+				if (filter_str)
+					free(filter_str);
 				*needs_refresh = 1;
 				break;
 			}
-			free(filter_str);
+			if (filter_str)
+				free(filter_str);
 			continue;
 		}
 
@@ -714,20 +719,23 @@ 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");
-				free(filter_str);
+				if (filter_str)
+					free(filter_str);
 				*needs_refresh = 1;
 				break;
 			}
-			free(filter_str);
+			if (filter_str)
+				free(filter_str);
 			continue;
 		}
 
 		keylen = strlen("scan_lvs:");
 		if (!strncmp(_hint_line, "scan_lvs:", keylen)) {
 			int scan_lvs = 0;
-			if ((sscanf(_hint_line + keylen, "%u", &scan_lvs) != 1) ||
-			    scan_lvs != cmd->scan_lvs) {
-				log_debug("ignore hints with different or unreadable scan_lvs");
+			sscanf(_hint_line + keylen, "%u", &scan_lvs);
+
+			if (scan_lvs != cmd->scan_lvs) {
+				log_debug("ignore hints with different scan_lvs");
 				*needs_refresh = 1;
 				break;
 			}
@@ -736,11 +744,7 @@ 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)) {
-			if (sscanf(_hint_line + keylen, "%u %u", &read_hash, &read_count) != 2) {
-				log_debug("ignore hints with invalid devs_hash");
-				*needs_refresh = 1;
-				break;
-			}
+			sscanf(_hint_line + keylen, "%u %u", &read_hash, &read_count);
 			continue;
 		}
 
@@ -760,28 +764,19 @@ static int _read_hint_file(struct cmd_context *cmd, struct dm_list *hints, int *
 		vgname = split[3];
 
 		if (name && !strncmp(name, "scan:", 5))
-			if (!dm_strncpy(hint.name, name + 5, sizeof(hint.name)))
-				continue;
+			strncpy(hint->name, name+5, PATH_MAX);
 
 		if (pvid && !strncmp(pvid, "pvid:", 5))
-			if (!dm_strncpy(hint.pvid, pvid + 5, sizeof(hint.pvid)))
-				continue;
+			strncpy(hint->pvid, pvid+5, ID_LEN);
 
 		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] != '-'))
-			if (!dm_strncpy(hint.vgname, vgname + 3, sizeof(hint.vgname)))
-				continue;
+			strncpy(hint->vgname, vgname+3, NAME_LEN);
 
-		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);
+		log_debug("add hint %s %s %d:%d %s", hint->name, hint->pvid, major, minor, vgname);
+		dm_list_add(hints, &hint->list);
 		found++;
 	}
 
@@ -916,11 +911,13 @@ 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 ?: "-");
-	free(filter_str);
+	if (filter_str)
+		free(filter_str);
 
 	_filter_to_str(cmd, devices_filter_CFG, &filter_str);
 	fprintf(fp, "filter:%s\n", filter_str ?: "-");
-	free(filter_str);
+	if (filter_str)
+		free(filter_str);
 
 	fprintf(fp, "scan_lvs:%d\n", cmd->scan_lvs);
 
@@ -954,6 +951,7 @@ 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++;
@@ -1181,8 +1179,7 @@ static void _get_single_vgname_cmd_arg(struct cmd_context *cmd,
 
 	if (!(st = strchr(arg, '/'))) {
 		/* simple vgname */
-		if (!(name = strdup(arg)))
-			return;
+		name = strdup(arg);
 		goto check;
 	}
 
@@ -1190,8 +1187,7 @@ static void _get_single_vgname_cmd_arg(struct cmd_context *cmd,
 	for (p = arg; p < st; p++)
 		namebuf[i++] = *p;
 
-	if (!(name = strdup(namebuf)))
-		return;
+	name = strdup(namebuf);
 
 check:
 	/*
@@ -1206,8 +1202,6 @@ check:
 			return;
 		}
 	}
-
-	free(name);
 }
 
 /*
@@ -1267,9 +1261,8 @@ 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())
-			return 0;
-
+		if (!_hints_exists())
+			_touch_hints();
 		if (!_lock_hints(cmd, LOCK_EX, NONBLOCK))
 			return 0;
 		/* create new hints after scan */
@@ -1362,9 +1355,6 @@ 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));
-		return 0;
+		r = 0;
 	}
 
 	dev_unset_last_byte(dev);
@@ -655,6 +655,7 @@ 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);
@@ -701,11 +702,12 @@ 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.", dev_name(devl->dev));
+			log_debug_devs("Scan failed to read %s error %d.", dev_name(devl->dev), error);
 			scan_failed = 1;
 			scan_read_errors++;
 			scan_failed_count++;
@@ -1045,15 +1047,6 @@ 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.
 	 */
@@ -1458,7 +1451,6 @@ 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;
 	}
@@ -1466,7 +1458,6 @@ 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,14 +26,12 @@
 #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)
@@ -52,12 +50,11 @@ 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;
@@ -98,72 +95,3 @@ 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,8 +78,4 @@ 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,6 +23,7 @@
 #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,7 +4586,6 @@ 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;
@@ -4623,27 +4622,16 @@ 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.
+	 * find the same inconsistency.  The VG repair (mistakenly done by
+	 * vg_read below) is supposed to fix that.
 	 *
-	 * 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.
+	 * 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.
 	 */
-
-	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 (!cmd->can_use_one_scan || lvmcache_scan_mismatch(cmd, vgname, vgid)) {
+		log_debug_metadata("Rescanning devices for %s %s", vgname, writing ? "rw" : "");
 		if (writing)
 			lvmcache_label_rescan_vg_rw(cmd, vgname, vgid);
 		else

lib/metadata/mirror.c

@@ -266,6 +266,7 @@ 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)) {
-				(void) init_dmeventd_monitor(monitored);
+				init_dmeventd_monitor(monitored);
 				return_0;
 			}
 			if (!lv_is_active(lv)) {
-				(void) init_dmeventd_monitor(monitored);
+				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,6 +21,8 @@
 #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,16 +25,36 @@
 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;
 
-#define AUX_LOCK_SUFFIX ":aux"
+/* 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);
+}
 
 static struct lock_list *_get_lock_list_entry(const char *file)
 {
@@ -50,19 +70,6 @@ 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;
@@ -71,54 +78,18 @@ 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)) {
-			/*
-			 * 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);
-			}
-
+			dm_list_del(llh);
 			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);
 
-			if (close(ll->lf) < 0)
-				log_sys_debug("close", ll->res);
-
-			ll->lf = -1;
+			free(ll->res);
+			free(llh);
 
 			if (file)
 				return 1;
@@ -183,6 +154,8 @@ 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;
@@ -194,11 +167,9 @@ 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);
-			if (close(fd_aux) < 0)
-				log_sys_debug("close", file_aux);
+			_undo_flock(file_aux, fd_aux);
 		} else {
-			if (close(fd_aux) < 0)
-				log_sys_debug("close", file_aux);
+			_undo_flock(file_aux, fd_aux);
 			r = _do_flock(file, fd, operation, nonblock);
 		}
 	}
@@ -212,7 +183,6 @@ 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;
 
@@ -224,7 +194,6 @@ 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);
@@ -241,27 +210,22 @@ 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) {
-			ll->ex = ex;
+		if (!r)
 			return 1;
-		}
 		log_error("Failed to convert flock on %s %d", file, errno);
 		return 0;
 	}
 
-	if (!(ll = _get_lock_list_entry(file))) {
-		if (!(ll = zalloc(sizeof(struct lock_list))))
-			return_0;
+	if (!(ll = malloc(sizeof(struct lock_list))))
+		return_0;
 
-		if (!(ll->res = strdup(file))) {
-			free(ll);
-			return_0;
-		}
-
-		ll->lf = -1;
-		dm_list_add(&_lock_list, &ll->list);
+	if (!(ll->res = strdup(file))) {
+		free(ll);
+		return_0;
 	}
 
+	ll->lf = -1;
+
 	log_very_verbose("Locking %s %c%c", ll->res, state,
 			 nonblock ? ' ' : 'B');
 
@@ -273,9 +237,12 @@ int lock_file(const char *file, uint32_t flags)
 	(void) dm_prepare_selinux_context(NULL, 0);
 
 	if (r)
-		ll->ex = ex;
-	else
+		dm_list_add(&_lock_list, &ll->list);
+	else {
+		free(ll->res);
+		free(ll);
 		stack;
+	}
 
 	return r;
 }
@@ -287,93 +254,3 @@ 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,12 +16,7 @@
 #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,8 +656,7 @@ void daemon_start(daemon_state s)
 		failed = 1; /* FD out of available selectable set */
 
 	sigfillset(&new_set);
-	if (sigprocmask(SIG_SETMASK, NULL, &old_set))
-		perror("sigprocmask error");
+	sigprocmask(SIG_SETMASK, NULL, &old_set);
 
 	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: .lib-dir-stamp $(ALL) $(LIB) $(SUBDIRS) lib/version-expected lib/dm-version-expected unit-test
+.tests-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=4 conv=fdatasync
+dd if=/dev/urandom of="$mount_dir/random" bs=1M count=50 conv=fdatasync
 checksum_ "$mount_dir/random" >MD5
 
 # FIXME: wait_for_sync - is this really testing anything under load?

test/shell/nomda-missing.sh

@@ -74,8 +74,6 @@ 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,6 +230,9 @@ 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, OO_LVCONVERT
+OO: --usepolicies, --interval Number, --poolmetadataspare Bool, --file_long String, 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.
+DESC: Repair a cache pool or cache.
 RULE: all not lv_is_locked lv_is_pvmove
 RULE: --poolmetadataspare and LV_cache LV_cachepool LV_thinpool
 

tools/lvconvert.c

@@ -2480,10 +2480,339 @@ deactivate_pmslv:
 	return 1;
 }
 
-/* TODO: lots of similar code with  thinpool repair
- *       investigate possible better code sharing...
+/*
+ * Use cache_repair to read metadata from cachevol_lv and
+ * write repaired metadata to new file.
  */
-static int _lvconvert_cache_repair(struct cmd_context *cmd,
+
+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,
 				   struct logical_volume *cache_lv,
 				   struct dm_list *pvh, int poolmetadataspare)
 {
@@ -2502,11 +2831,6 @@ static int _lvconvert_cache_repair(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;
 
@@ -3769,7 +4093,7 @@ static int _lvconvert_repair_pvs(struct cmd_context *cmd, struct logical_volume
 	return ret ? ECMD_PROCESSED : ECMD_FAILED;
 }
 
-static int _lvconvert_repair_cachepool_thinpool(struct cmd_context *cmd, struct logical_volume *lv,
+static int _lvconvert_repair_cache_or_thin(struct cmd_context *cmd, struct logical_volume *lv,
 			struct processing_handle *handle)
 {
 	int poolmetadataspare = arg_int_value(cmd, poolmetadataspare_ARG, DEFAULT_POOL_METADATA_SPARE);
@@ -3790,8 +4114,13 @@ static int _lvconvert_repair_cachepool_thinpool(struct cmd_context *cmd, struct
 		if (!_lvconvert_thin_pool_repair(cmd, lv, use_pvh, poolmetadataspare))
 			return_ECMD_FAILED;
 	} else /* cache */ {
-		if (!_lvconvert_cache_repair(cmd, lv, use_pvh, poolmetadataspare))
-			return_ECMD_FAILED;
+		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;
+		}
 	}
 
 	return ECMD_PROCESSED;
@@ -3803,7 +4132,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_cachepool_thinpool(cmd, lv, handle);
+		return _lvconvert_repair_cache_or_thin(cmd, lv, handle);
 
 	if (lv_is_raid(lv) || lv_is_mirror(lv))
 		return _lvconvert_repair_pvs(cmd, lv, handle);
@@ -3814,12 +4143,6 @@ 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,10 +359,7 @@ static int _dump_all_text(struct cmd_context *cmd, const char *tofile, struct de
 			continue;
 		}
 
-		if (sscanf(line, "seqno = %u", &seqno) != 1) {
-			log_warn("WARNING: Failed to parse seqno.");
-			seqno = 0; /* Skip? */
-		}
+		sscanf(line, "seqno = %u", &seqno);
 
 		/*
 		 * The first three lines look like metadata with
@@ -420,11 +417,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);
 			}
 		}
 
@@ -655,31 +652,27 @@ static int _dump_meta_area(struct device *dev, const char *tofile,
 	if (!tofile)
 		return_0;
 
-	if (!(meta_buf = zalloc(mda_size)))
+	if (!(meta_buf = malloc(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);
-		free(meta_buf);
+		log_error("Failed to create file %s", tofile);
 		return 0;
 	}
 
-	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);
+	fwrite(meta_buf, mda_size - 512, 1, fp);
 
-	free(meta_buf);
 	if (fflush(fp))
 		stack;
 	if (fclose(fp))
 		stack;
-
 	return 1;
 }
 
@@ -712,11 +705,12 @@ static int _dump_current_text(struct device *dev,
 	int ri = rlocn_index; /* 0 or 1 */
 	int bad = 0;
 
-	if (!(meta_buf = zalloc(meta_size))) {
+	if (!(meta_buf = malloc(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
@@ -727,7 +721,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;
@@ -738,7 +732,6 @@ 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;
 		}
 
@@ -746,14 +739,12 @@ 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;
 		}
 	}
@@ -811,9 +802,9 @@ static int _dump_current_text(struct device *dev,
 	}
 
  out:
-	free(meta_buf);
-
-	return (!bad) ? 1 : 0;
+	if (bad)
+		return 0;
+	return 1;
 }
 
 static int _dump_label_and_pv_header(struct cmd_context *cmd, int print_fields,
@@ -857,7 +848,6 @@ 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;
 	}
 
@@ -1052,9 +1042,9 @@ static int _dump_label_and_pv_header(struct cmd_context *cmd, int print_fields,
 			  (unsigned long long)xlate64(dlocn->size));
 	}
 
-	free(buf);
-
-	return (!bad) ? 1 : 0;
+	if (bad)
+		return 0;
+	return 1;
 }
 
 /*
@@ -1107,7 +1097,6 @@ 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;
 	}
 
@@ -1198,9 +1187,11 @@ static int _dump_mda_header(struct cmd_context *cmd,
 
 	/* Should we also check text metadata if it exists in rlocn1? */
  out:
-	free(buf);
-
-	return (!bad) ? 1 : 0;
+	if (buf)
+		free(buf);
+	if (bad)
+		return 0;
+	return 1;
 }
 
 static int _dump_headers(struct cmd_context *cmd,
@@ -1466,8 +1457,7 @@ static int _dump_search(struct cmd_context *cmd,
 		uint64_t dev_bytes;
 		uint64_t extra_bytes;
 
-		if (!dev_get_size(dev, &dev_sectors))
-			return_ECMD_FAILED;
+		dev_get_size(dev, &dev_sectors);
 
 		dev_bytes = dev_sectors * 512;
 		extra_bytes = dev_bytes % ONE_MB_IN_BYTES;
@@ -1494,8 +1484,9 @@ 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 = zalloc(mda_size)))
+	if (!(buf = malloc(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] && vgname && strcmp(file_vgname, vgname))
+	if (file_vgname[0] && 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,8 +4284,7 @@ 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) &&
-		    (!arg_devices || dm_list_empty(arg_devices)))
+		if (!process_all_pvs && dm_list_empty(arg_tags) && dm_list_empty(arg_devices))
 			break;
 		log_set_report_object_name_and_id(NULL, NULL);
 	}

tools/vgremove.c

@@ -78,8 +78,6 @@ 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;
 }