Revert "pvscan: only add device args to dev cache"

This reverts commit 33e47182f7.

lib/device/dev-cache.c

@@ -1852,7 +1852,7 @@ int setup_devices_file(struct cmd_context *cmd)
  * Add all system devices to dev-cache, and attempt to
  * match all devices_file entries to dev-cache entries.
  */
-int setup_devices(struct cmd_context *cmd)
+static int _setup_devices(struct cmd_context *cmd, int no_file_match)
 {
 	int file_exists;
 	int lock_mode = 0;
@@ -1979,6 +1979,13 @@ int setup_devices(struct cmd_context *cmd)
 	 */
 	dev_cache_scan(cmd);
 
+	/*
+	 * The caller uses "no_file_match" if it wants to match specific devs
+	 * itself, instead of matching everything in device_ids_match.
+	 */
+	if (no_file_match && cmd->enable_devices_file)
+		return 1;
+
 	/*
 	 * Match entries from cmd->use_devices with device structs in dev-cache.
 	 */
@@ -1987,6 +1994,16 @@ int setup_devices(struct cmd_context *cmd)
 	return 1;
 }
 
+int setup_devices(struct cmd_context *cmd)
+{
+	return _setup_devices(cmd, 0);
+}
+
+int setup_devices_no_file_match(struct cmd_context *cmd)
+{
+	return _setup_devices(cmd, 1);
+}
+
 /*
  * The alternative to setup_devices() when the command is interested
  * in using only one PV.
@@ -2055,188 +2072,3 @@ int setup_device(struct cmd_context *cmd, const char *devname)
 	return 1;
 }
 
-/*
- * pvscan --cache is specialized/optimized to look only at command args,
- * so this just sets up the devices file, then individual devices are
- * added to dev-cache and matched with device_ids later in pvscan.
- */
-
-int setup_devices_for_pvscan_cache(struct cmd_context *cmd)
-{
-	if (cmd->enable_devices_list) {
-		if (!_setup_devices_list(cmd))
-			return_0;
-		return 1;
-	}
-
-	if (!setup_devices_file(cmd))
-		return_0;
-
-	if (!cmd->enable_devices_file)
-		return 1;
-
-	if (!devices_file_exists(cmd)) {
-		log_debug("Devices file not found, ignoring.");
-		cmd->enable_devices_file = 0;
-		return 1;
-	}
-
-	if (!lock_devices_file(cmd, LOCK_SH)) {
-		log_error("Failed to lock the devices file to read.");
-		return 0;
-	}
-
-	if (!device_ids_read(cmd)) {
-		log_error("Failed to read the devices file.");
-		unlock_devices_file(cmd);
-		return 0;
-	}
-
-	unlock_devices_file(cmd);
-	return 1;
-}
-
-
-/* Get a device name from a devno. */
-
-static char *_get_devname_from_devno(struct cmd_context *cmd, dev_t devno)
-{
-	char path[PATH_MAX];
-	char devname[PATH_MAX];
-	char namebuf[NAME_LEN];
-	char line[1024];
-	int major = MAJOR(devno);
-	int minor = MINOR(devno);
-	int line_major;
-	int line_minor;
-	uint64_t line_blocks;
-	DIR *dir;
-	struct dirent *dirent;
-	FILE *fp;
-
-	/*
-	 * $ ls /sys/dev/block/8:0/device/block/
-	 * sda
-	 */
-	if (major_is_scsi_device(cmd->dev_types, major)) {
-		if (dm_snprintf(path, sizeof(path), "%sdev/block/%d:%d/device/block",
-				dm_sysfs_dir(), major, minor) < 0) {
-			return NULL;
-		}
-
-		if (!(dir = opendir(path)))
-			return NULL;
-
-		while ((dirent = readdir(dir))) {
-			if (dirent->d_name[0] == '.')
-				continue;
-			if (dm_snprintf(devname, sizeof(devname), "/dev/%s", dirent->d_name) < 0) {
-				devname[0] = '\0';
-				stack;
-			}
-			break;
-		}
-		closedir(dir);
-
-		if (devname[0]) {
-			log_debug("Found %s for %d:%d from sys", devname, major, minor);
-			return _strdup(devname);
-		}
-		return NULL;
-	}
-
-	/*
-	 * $ cat /sys/dev/block/253:3/dm/name
-	 * mpatha
-	 */
-	if (major == cmd->dev_types->device_mapper_major) {
-		if (dm_snprintf(path, sizeof(path), "%sdev/block/%d:%d/dm/name",
-				dm_sysfs_dir(), major, minor) < 0) {
-			return NULL;
-		}
-
-		if (!get_sysfs_value(path, namebuf, sizeof(namebuf), 0))
-			return NULL;
-
-		if (dm_snprintf(devname, sizeof(devname), "/dev/mapper/%s", namebuf) < 0) {
-			devname[0] = '\0';
-			stack;
-		}
-
-		if (devname[0]) {
-			log_debug("Found %s for %d:%d from sys", devname, major, minor);
-			return _strdup(devname);
-		}
-		return NULL;
-	}
-
-	/*
-	 * /proc/partitions lists
-	 * major minor #blocks name
-	 */
-
-	if (!(fp = fopen("/proc/partitions", "r")))
-		return NULL;
-
-	while (fgets(line, sizeof(line), fp)) {
-		if (sscanf(line, "%u %u %llu %s", &line_major, &line_minor, (unsigned long long *)&line_blocks, namebuf) != 4)
-			continue;
-		if (line_major != major)
-			continue;
-		if (line_minor != minor)
-			continue;
-
-		if (dm_snprintf(devname, sizeof(devname), "/dev/%s", namebuf) < 0) {
-			devname[0] = '\0';
-			stack;
-		}
-		break;
-	}
-	fclose(fp);
-
-	if (devname[0]) {
-		log_debug("Found %s for %d:%d from proc", devname, major, minor);
-		return _strdup(devname);
-	}
-
-	/*
-	 * If necessary, this could continue searching by stat'ing /dev entries.
-	 */
-
-	return NULL;
-}
-
-int setup_devname_in_dev_cache(struct cmd_context *cmd, const char *devname)
-{
-	struct stat buf;
-	struct device *dev;
-
-	if (stat(devname, &buf) < 0) {
-		log_error("Cannot access device %s.", devname);
-		return 0;
-	}
-
-	if (!S_ISBLK(buf.st_mode)) {
-		log_error("Invaild device type %s.", devname);
-		return 0;
-	}
-
-	if (!_insert_dev(devname, buf.st_rdev))
-		return_0;
-
-	if (!(dev = (struct device *) dm_hash_lookup(_cache.names, devname)))
-		return_0;
-
-	return 1;
-}
-
-int setup_devno_in_dev_cache(struct cmd_context *cmd, dev_t devno)
-{
-	const char *devname;
-
-	if (!(devname = _get_devname_from_devno(cmd, devno)))
-		return_0;
-
-	return setup_devname_in_dev_cache(cmd, devname);
-}
-

lib/device/dev-cache.h

@@ -77,11 +77,7 @@ int get_dm_uuid_from_sysfs(char *buf, size_t buf_size, int major, int minor);
 
 int setup_devices_file(struct cmd_context *cmd);
 int setup_devices(struct cmd_context *cmd);
+int setup_devices_no_file_match(struct cmd_context *cmd);
 int setup_device(struct cmd_context *cmd, const char *devname);
 
-/* Normal device setup functions are split up for pvscan optimization. */
-int setup_devices_for_pvscan_cache(struct cmd_context *cmd);
-int setup_devname_in_dev_cache(struct cmd_context *cmd, const char *devname);
-int setup_devno_in_dev_cache(struct cmd_context *cmd, dev_t devno);
-
 #endif

lib/device/device_id.c

@@ -1534,22 +1534,6 @@ int device_ids_match_dev(struct cmd_context *cmd, struct device *dev)
  * passes the filter.
  */
 
-void device_ids_match_device_list(struct cmd_context *cmd)
-{
-	struct dev_use *du;
-
-	dm_list_iterate_items(du, &cmd->use_devices) {
-		if (du->dev)
-			continue;
-		if (!(du->dev = dev_cache_get(cmd, du->devname, NULL))) {
-			log_warn("Device not found for %s.", du->devname);
-		} else {
-			/* Should we set dev->id?  Which idtype?  Use --deviceidtype? */
-			du->dev->flags |= DEV_MATCHED_USE_ID;
-		}
-	}
-}
-
 void device_ids_match(struct cmd_context *cmd)
 {
 	struct dev_iter *iter;
@@ -1557,7 +1541,16 @@ void device_ids_match(struct cmd_context *cmd)
 	struct device *dev;
 
 	if (cmd->enable_devices_list) {
-		device_ids_match_device_list(cmd);
+		dm_list_iterate_items(du, &cmd->use_devices) {
+			if (du->dev)
+				continue;
+			if (!(du->dev = dev_cache_get(cmd, du->devname, NULL))) {
+				log_warn("Device not found for %s.", du->devname);
+			} else {
+				/* Should we set dev->id?  Which idtype?  Use --deviceidtype? */
+				du->dev->flags |= DEV_MATCHED_USE_ID;
+			}
+		}
 		return;
 	}
 

lib/device/device_id.h

@@ -32,7 +32,6 @@ int device_id_add(struct cmd_context *cmd, struct device *dev, const char *pvid,
 void device_id_pvremove(struct cmd_context *cmd, struct device *dev);
 void device_ids_match(struct cmd_context *cmd);
 int device_ids_match_dev(struct cmd_context *cmd, struct device *dev);
-void device_ids_match_device_list(struct cmd_context *cmd);
 void device_ids_validate(struct cmd_context *cmd, struct dm_list *scanned_devs, int *device_ids_invalid, int noupdate);
 int device_ids_version_unchanged(struct cmd_context *cmd);
 void device_ids_find_renamed_devs(struct cmd_context *cmd, struct dm_list *dev_list, int *search_count, int noupdate);

test/shell/devicesfile-basic.sh

@@ -283,7 +283,7 @@ not ls "$RUNDIR/lvm/pvs_online/$PVID3"
 # arg in devices list
 _clear_online_files
 pvscan --devices "$dev3" --cache -aay "$dev3"
-pvscan --devices "$dev4","$dev3" --cache -aay "$dev4"
+pvscan --devices "$dev4" --cache -aay "$dev4"
 check lv_field $vg2/$lv2 lv_active "active"
 vgchange -an $vg2
 

tools/pvscan.c

@@ -857,21 +857,11 @@ static int _get_devs_from_saved_vg(struct cmd_context *cmd, const char *vgname,
 
 		devno = MKDEV(file_major, file_minor);
 
-		if (!setup_devno_in_dev_cache(cmd, devno)) {
-			log_error_pvscan(cmd, "No device set up for %d:%d PVID %s", file_major, file_minor, pvid);
-			goto bad;
-		}
-
 		if (!(dev = dev_cache_get_by_devt(cmd, devno, NULL, NULL))) {
 			log_error_pvscan(cmd, "No device found for %d:%d PVID %s", file_major, file_minor, pvid);
 			goto bad;
 		}
 
-		/*
-		 * Do not need to match device_id here, see comment after
-		 * get_devs_from_saved_vg about relying on pvid online file.
-		 */
-
 		name1 = dev_name(dev);
 		name2 = pvl->pv->device_hint;
 
@@ -1109,11 +1099,17 @@ static int _pvscan_aa(struct cmd_context *cmd, struct pvscan_aa_params *pp,
 		 * PROCESS_SKIP_SCAN: we have already done lvmcache_label_scan
 		 * so tell process_each to skip it.
 		 */
+		if (do_all)
+			read_flags |= PROCESS_SKIP_SCAN;
 
+		/*
+		 * When the command is processing specific devs (not all), it
+		 * has done setup_devices_no_file_match() to avoid matching ids
+		 * fo all devs unnecessarily, but now that we're falling back
+		 * to process_each_vg() we need to complete the id matching.
+		 */
 		if (!do_all)
-			lvmcache_label_scan(cmd);
+			device_ids_match(cmd);
-
-		read_flags |= PROCESS_SKIP_SCAN;
 
 		ret = process_each_vg(cmd, 0, NULL, NULL, vgnames, read_flags, 0, handle, _pvscan_aa_single);
 	}
@@ -1196,15 +1192,11 @@ static int _get_args_devs(struct cmd_context *cmd, struct dm_list *pvscan_args,
 	/* in common usage, no dev will be found for a devno */
 
 	dm_list_iterate_items(arg, pvscan_args) {
-		if (arg->devname) {
+		if (arg->devname)
-			if (!setup_devname_in_dev_cache(cmd, arg->devname))
-				log_error_pvscan(cmd, "No device set up for name arg %s", arg->devname);
 			arg->dev = dev_cache_get(cmd, arg->devname, NULL);
-		} else if (arg->devno) {
+		else if (arg->devno)
-			if (!setup_devno_in_dev_cache(cmd, arg->devno))
-				log_error_pvscan(cmd, "No device set up for devno arg %d", (int)arg->devno);
 			arg->dev = dev_cache_get_by_devt(cmd, arg->devno, NULL, NULL);
-		} else
+		else
 			return_0;
 	}
 
@@ -1680,13 +1672,11 @@ static int _pvscan_cache_args(struct cmd_context *cmd, int argc, char **argv,
 	cmd->pvscan_cache_single = 1;
 
 	/*
-	 * Special pvscan-specific setup steps to avoid looking
+	 * "no_file_match" means that when the devices file is used,
-	 * at any devices except for device args.
+	 * setup_devices will skip matching devs to devices file entries.
-	 * Read devices file and determine if devices file will be used.
+	 * Specific devs must be matched later with device_ids_match_dev().
-	 * Does not do dev_cache_scan (adds nothing to dev-cache), and
-	 * does not do any device id matching.
 	 */
-	if (!setup_devices_for_pvscan_cache(cmd)) {
+	if (!setup_devices_no_file_match(cmd)) {
 		log_error_pvscan(cmd, "Failed to set up devices.");
 		return 0;
 	}
@@ -1745,21 +1735,17 @@ static int _pvscan_cache_args(struct cmd_context *cmd, int argc, char **argv,
 	log_debug("pvscan_cache_args: filter devs nodata");
 
 	/*
-	 * Match dev args with the devices file because special/optimized
+	 * Match dev args with the devices file because
-	 * device setup was used above which does not check the devices file.
+	 * setup_devices_no_file_match() was used above which skipped checking
-	 * If a match fails here do not exclude it, that will be done below by
+	 * the devices file.  If a match fails here do not exclude it, that
-	 * passes_filter() which runs filter-deviceid. The
+	 * will be done below by passes_filter() which runs filter-deviceid.
-	 * relax_deviceid_filter case needs to be able to work around
+	 * The relax_deviceid_filter case needs to be able to work around
 	 * unmatching devs.
 	 */
-
 	if (cmd->enable_devices_file) {
-		dm_list_iterate_items(devl, &pvscan_devs)
+		dm_list_iterate_items_safe(devl, devl2, &pvscan_devs)
 			device_ids_match_dev(cmd, devl->dev);
-
 	}
-	if (cmd->enable_devices_list)
-		device_ids_match_device_list(cmd);
 
 	if (cmd->enable_devices_file && device_ids_use_devname(cmd)) {
 		relax_deviceid_filter = 1;