From 8ba6259b24cd4b99e061f2610c5cd0bcde890039 Mon Sep 17 00:00:00 2001
From: David Teigland <teigland@redhat.com>
Date: Thu, 24 Feb 2022 16:03:21 -0600
Subject: [PATCH 36/54] devices: drop incorrect paths from aliases list
along with some basic checks for cases when a device
has no aliases.
lvm itself creates many situations where a struct device
has no valid paths, when it activates and opens an LV,
does something with it, e.g. zeroing, and then closes
and deactivates it. (dev-cache is intended for PVs, and
the use of LVs should be moved out of dev-cache in a
future patch.)
---
lib/device/dev-cache.c | 223 ++++++++++++++++++++++++++---------------
lib/device/dev-cache.h | 2 +-
lib/device/dev-io.c | 34 ++++---
lib/device/device.h | 3 -
4 files changed, 164 insertions(+), 98 deletions(-)
diff --git a/lib/device/dev-cache.c b/lib/device/dev-cache.c
index cc1af7c7a..58e67e130 100644
--- a/lib/device/dev-cache.c
+++ b/lib/device/dev-cache.c
@@ -351,7 +351,7 @@ static int _add_alias(struct device *dev, const char *path, enum add_hash hash)
goto out;
}
- if (!(path = dm_pool_strdup(_cache.mem, path)) ||
+ if (!(path = _strdup(path)) ||
!(sl = _zalloc(sizeof(*sl)))) {
log_error("Failed to add allias to dev cache.");
return 0;
@@ -1162,6 +1162,17 @@ static int _insert(const char *path, const struct stat *info,
return 1;
}
+static void _drop_all_aliases(struct device *dev)
+{
+ struct dm_str_list *strl, *strl2;
+
+ dm_list_iterate_items_safe(strl, strl2, &dev->aliases) {
+ log_debug("Drop alias for %d:%d %s.", (int)MAJOR(dev->dev), (int)MINOR(dev->dev), strl->str);
+ dm_hash_remove(_cache.names, strl->str);
+ dm_list_del(&strl->list);
+ }
+}
+
void dev_cache_scan(struct cmd_context *cmd)
{
log_debug_devs("Creating list of system devices.");
@@ -1371,59 +1382,6 @@ int dev_cache_add_dir(const char *path)
return 1;
}
-/* Check cached device name is still valid before returning it */
-/* This should be a rare occurrence */
-/* set quiet if the cache is expected to be out-of-date */
-/* FIXME Make rest of code pass/cache struct device instead of dev_name */
-const char *dev_name_confirmed(struct device *dev, int quiet)
-{
- struct stat buf;
- const char *name;
- int r;
-
- if ((dev->flags & DEV_REGULAR))
- return dev_name(dev);
-
- while ((r = stat(name = dm_list_item(dev->aliases.n,
- struct dm_str_list)->str, &buf)) ||
- (buf.st_rdev != dev->dev)) {
- if (r < 0) {
- if (quiet)
- log_sys_debug("stat", name);
- else
- log_sys_error("stat", name);
- }
- if (quiet)
- log_debug_devs("Path %s no longer valid for device(%d,%d)",
- name, (int) MAJOR(dev->dev),
- (int) MINOR(dev->dev));
- else
- log_warn("Path %s no longer valid for device(%d,%d)",
- name, (int) MAJOR(dev->dev),
- (int) MINOR(dev->dev));
-
- /* Remove the incorrect hash entry */
- dm_hash_remove(_cache.names, name);
-
- /* Leave list alone if there isn't an alternative name */
- /* so dev_name will always find something to return. */
- /* Otherwise add the name to the correct device. */
- if (dm_list_size(&dev->aliases) > 1) {
- dm_list_del(dev->aliases.n);
- if (!r)
- _insert(name, &buf, 0, obtain_device_list_from_udev());
- continue;
- }
-
- /* Scanning issues this inappropriately sometimes. */
- log_debug_devs("Aborting - please provide new pathname for what "
- "used to be %s", name);
- return NULL;
- }
-
- return dev_name(dev);
-}
-
struct device *dev_hash_get(const char *name)
{
return (struct device *) dm_hash_lookup(_cache.names, name);
@@ -1452,26 +1410,23 @@ static void _remove_alias(struct device *dev, const char *name)
* deactivated LV. Those old paths are all invalid and are dropped here.
*/
-static void _verify_aliases(struct device *dev, const char *newname)
+static void _verify_aliases(struct device *dev)
{
struct dm_str_list *strl, *strl2;
struct stat st;
dm_list_iterate_items_safe(strl, strl2, &dev->aliases) {
- /* newname was just stat'd and added by caller */
- if (newname && !strcmp(strl->str, newname))
- continue;
-
if (stat(strl->str, &st) || (st.st_rdev != dev->dev)) {
- log_debug("Drop invalid path %s for %d:%d (new path %s).",
- strl->str, (int)MAJOR(dev->dev), (int)MINOR(dev->dev), newname ?: "");
+ log_debug("Drop alias for %d:%d invalid path %s %d:%d.",
+ (int)MAJOR(dev->dev), (int)MINOR(dev->dev), strl->str,
+ (int)MAJOR(st.st_rdev), (int)MINOR(st.st_rdev));
dm_hash_remove(_cache.names, strl->str);
dm_list_del(&strl->list);
}
}
}
-struct device *dev_cache_get(struct cmd_context *cmd, const char *name, struct dev_filter *f)
+static struct device *_dev_cache_get(struct cmd_context *cmd, const char *name, struct dev_filter *f, int existing)
{
struct device *dev = (struct device *) dm_hash_lookup(_cache.names, name);
struct stat st;
@@ -1485,13 +1440,18 @@ struct device *dev_cache_get(struct cmd_context *cmd, const char *name, struct d
if (dev && (dev->flags & DEV_REGULAR))
return dev;
+ if (dev && dm_list_empty(&dev->aliases)) {
+ /* shouldn't happen */
+ log_warn("Ignoring dev with no valid paths for %s.", name);
+ return NULL;
+ }
+
/*
- * The requested path is invalid, remove any dev-cache
- * info for it.
+ * The requested path is invalid, remove any dev-cache info for it.
*/
if (stat(name, &st)) {
if (dev) {
- log_print("Device path %s is invalid for %d:%d %s.",
+ log_debug("Device path %s is invalid for %d:%d %s.",
name, (int)MAJOR(dev->dev), (int)MINOR(dev->dev), dev_name(dev));
dm_hash_remove(_cache.names, name);
@@ -1499,11 +1459,17 @@ struct device *dev_cache_get(struct cmd_context *cmd, const char *name, struct d
_remove_alias(dev, name);
/* Remove any other names in dev->aliases that are incorrect. */
- _verify_aliases(dev, NULL);
+ _verify_aliases(dev);
}
return NULL;
}
+ if (dev && dm_list_empty(&dev->aliases)) {
+ /* shouldn't happen */
+ log_warn("Ignoring dev with no valid paths for %s.", name);
+ return NULL;
+ }
+
if (!S_ISBLK(st.st_mode)) {
log_debug("Not a block device %s.", name);
return NULL;
@@ -1514,26 +1480,110 @@ struct device *dev_cache_get(struct cmd_context *cmd, const char *name, struct d
* Remove incorrect info and then add new dev-cache entry.
*/
if (dev && (st.st_rdev != dev->dev)) {
- log_debug("Device path %s does not match %d:%d %s.",
- name, (int)MAJOR(dev->dev), (int)MINOR(dev->dev), dev_name(dev));
+ struct device *dev_by_devt = (struct device *) btree_lookup(_cache.devices, (uint32_t) st.st_rdev);
+
+ /*
+ * lvm commands create this condition when they
+ * activate/deactivate LVs combined with creating new LVs.
+ * The command does not purge dev structs when deactivating
+ * an LV (which it probably should do), but the better
+ * approach would be not using dev-cache at all for LVs.
+ */
- dm_hash_remove(_cache.names, name);
+ log_debug("Dropping aliases for device entry %d:%d %s for new device %d:%d %s.",
+ (int)MAJOR(dev->dev), (int)MINOR(dev->dev), dev_name(dev),
+ (int)MAJOR(st.st_rdev), (int)MINOR(st.st_rdev), name);
- _remove_alias(dev, name);
+ _drop_all_aliases(dev);
- /* Remove any other names in dev->aliases that are incorrect. */
- _verify_aliases(dev, NULL);
+ if (dev_by_devt) {
+ log_debug("Dropping aliases for device entry %d:%d %s for new device %d:%d %s.",
+ (int)MAJOR(dev_by_devt->dev), (int)MINOR(dev_by_devt->dev), dev_name(dev_by_devt),
+ (int)MAJOR(st.st_rdev), (int)MINOR(st.st_rdev), name);
- /* Add new dev-cache entry next. */
- dev = NULL;
+ _drop_all_aliases(dev_by_devt);
+ }
+
+#if 0
+ /*
+ * I think only lvm's own dm devs should be added here, so use
+ * a warning to look for any other unknown cases.
+ */
+ if (MAJOR(st.st_rdev) != cmd->dev_types->device_mapper_major) {
+ log_warn("WARNING: new device appeared %d:%d %s",
+ (int)MAJOR(st.st_rdev), (int)(MINOR(st.st_rdev)), name);
+ }
+#endif
+
+ if (!_insert_dev(name, st.st_rdev))
+ return_NULL;
+
+ /* Get the struct dev that was just added. */
+ dev = (struct device *) dm_hash_lookup(_cache.names, name);
+
+ if (!dev) {
+ log_error("Failed to get device %s", name);
+ return NULL;
+ }
+
+ goto out;
}
+ if (dev && dm_list_empty(&dev->aliases)) {
+ /* shouldn't happen */
+ log_warn("Ignoring dev with no valid paths for %s.", name);
+ return NULL;
+ }
+
+ if (!dev && existing)
+ return_NULL;
+
/*
- * Either add a new struct dev for st_rdev and name,
- * or add name as a new alias for an existing struct dev
- * for st_rdev.
+ * This case should never be hit for a PV. It should only
+ * happen when the command is opening a new LV it has created.
+ * Add an arg to all callers indicating when the arg should be
+ * new (for an LV) and not existing.
+ * FIXME: fix this further by not using dev-cache struct devs
+ * at all for new dm devs (LVs) that lvm uses. Make the
+ * dev-cache contain only devs for PVs.
+ * Places to fix that use a dev for LVs include:
+ * . lv_resize opening lv to discard
+ * . wipe_lv opening lv to zero it
+ * . _extend_sanlock_lv opening lv to extend it
+ * . _write_log_header opening lv to write header
+ * Also, io to LVs should not go through bcache.
+ * bcache should contain only labels and metadata
+ * scanned from PVs.
*/
if (!dev) {
+ /*
+ * This case should only be used for new devices created by this
+ * command (opening LVs it's created), so if a dev exists for the
+ * dev_t referenced by the name, then drop all aliases for before
+ * _insert_dev adds the new name. lvm commands actually hit this
+ * fairly often when it uses some LV, deactivates the LV, then
+ * creates some new LV which ends up with the same major:minor.
+ * Without dropping the aliases, it's plausible that lvm commands
+ * could end up using the wrong dm device.
+ */
+ struct device *dev_by_devt = (struct device *) btree_lookup(_cache.devices, (uint32_t) st.st_rdev);
+ if (dev_by_devt) {
+ log_debug("Dropping aliases for %d:%d before adding new path %s.",
+ (int)MAJOR(st.st_rdev), (int)(MINOR(st.st_rdev)), name);
+ _drop_all_aliases(dev_by_devt);
+ }
+
+#if 0
+ /*
+ * I think only lvm's own dm devs should be added here, so use
+ * a warning to look for any other unknown cases.
+ */
+ if (MAJOR(st.st_rdev) != cmd->dev_types->device_mapper_major) {
+ log_warn("WARNING: new device appeared %d:%d %s",
+ (int)MAJOR(st.st_rdev), (int)(MINOR(st.st_rdev)), name);
+ }
+#endif
+
if (!_insert_dev(name, st.st_rdev))
return_NULL;
@@ -1544,10 +1594,9 @@ struct device *dev_cache_get(struct cmd_context *cmd, const char *name, struct d
log_error("Failed to get device %s", name);
return NULL;
}
-
- _verify_aliases(dev, name);
}
+ out:
/*
* The caller passed a filter if they only want the dev if it
* passes filters.
@@ -1577,6 +1626,16 @@ struct device *dev_cache_get(struct cmd_context *cmd, const char *name, struct d
return dev;
}
+struct device *dev_cache_get_existing(struct cmd_context *cmd, const char *name, struct dev_filter *f)
+{
+ return _dev_cache_get(cmd, name, f, 1);
+}
+
+struct device *dev_cache_get(struct cmd_context *cmd, const char *name, struct dev_filter *f)
+{
+ return _dev_cache_get(cmd, name, f, 0);
+}
+
struct device *dev_cache_get_by_devt(struct cmd_context *cmd, dev_t devt)
{
struct device *dev = (struct device *) btree_lookup(_cache.devices, (uint32_t) devt);
@@ -1653,8 +1712,10 @@ int dev_fd(struct device *dev)
const char *dev_name(const struct device *dev)
{
- return (dev && dev->aliases.n) ? dm_list_item(dev->aliases.n, struct dm_str_list)->str :
- unknown_device_name();
+ if (dev && dev->aliases.n && !dm_list_empty(&dev->aliases))
+ return dm_list_item(dev->aliases.n, struct dm_str_list)->str;
+ else
+ return unknown_device_name();
}
bool dev_cache_has_md_with_end_superblock(struct dev_types *dt)
diff --git a/lib/device/dev-cache.h b/lib/device/dev-cache.h
index 7305eeb0e..51c3fc6c3 100644
--- a/lib/device/dev-cache.h
+++ b/lib/device/dev-cache.h
@@ -53,7 +53,7 @@ int dev_cache_has_scanned(void);
int dev_cache_add_dir(const char *path);
struct device *dev_cache_get(struct cmd_context *cmd, const char *name, struct dev_filter *f);
-
+struct device *dev_cache_get_existing(struct cmd_context *cmd, const char *name, struct dev_filter *f);
struct device *dev_cache_get_by_devt(struct cmd_context *cmd, dev_t devt);
struct device *dev_hash_get(const char *name);
diff --git a/lib/device/dev-io.c b/lib/device/dev-io.c
index b4f1930b1..811ad8978 100644
--- a/lib/device/dev-io.c
+++ b/lib/device/dev-io.c
@@ -58,6 +58,9 @@ static int _dev_get_size_file(struct device *dev, uint64_t *size)
const char *name = dev_name(dev);
struct stat info;
+ if (dm_list_empty(&dev->aliases))
+ return_0;
+
if (dev->size_seqno == _dev_size_seqno) {
log_very_verbose("%s: using cached size %" PRIu64 " sectors",
name, dev->size);
@@ -87,7 +90,7 @@ static int _dev_get_size_dev(struct device *dev, uint64_t *size)
int do_close = 0;
if (dm_list_empty(&dev->aliases))
- return 0;
+ return_0;
if (dev->size_seqno == _dev_size_seqno) {
log_very_verbose("%s: using cached size %" PRIu64 " sectors",
@@ -305,6 +308,13 @@ int dev_open_flags(struct device *dev, int flags, int direct, int quiet)
if ((flags & O_EXCL))
need_excl = 1;
+ if (dm_list_empty(&dev->aliases)) {
+ /* shouldn't happen */
+ log_print("Cannot open device %d:%d with no valid paths.", (int)MAJOR(dev->dev), (int)MINOR(dev->dev));
+ return 0;
+ }
+ name = dev_name(dev);
+
if (dev->fd >= 0) {
if (((dev->flags & DEV_OPENED_RW) || !need_rw) &&
((dev->flags & DEV_OPENED_EXCL) || !need_excl)) {
@@ -314,7 +324,7 @@ int dev_open_flags(struct device *dev, int flags, int direct, int quiet)
if (dev->open_count && !need_excl)
log_debug_devs("%s: Already opened read-only. Upgrading "
- "to read-write.", dev_name(dev));
+ "to read-write.", name);
/* dev_close_immediate will decrement this */
dev->open_count++;
@@ -327,11 +337,7 @@ int dev_open_flags(struct device *dev, int flags, int direct, int quiet)
if (critical_section())
/* FIXME Make this log_error */
- log_verbose("dev_open(%s) called while suspended",
- dev_name(dev));
-
- if (!(name = dev_name_confirmed(dev, quiet)))
- return_0;
+ log_verbose("dev_open(%s) called while suspended", name);
#ifdef O_DIRECT_SUPPORT
if (direct) {
@@ -372,9 +378,9 @@ int dev_open_flags(struct device *dev, int flags, int direct, int quiet)
}
#endif
if (quiet)
- log_sys_debug("open", name);
+ log_debug("Failed to open device path %s (%d).", name, errno);
else
- log_sys_error("open", name);
+ log_error("Failed to open device path %s (%d).", name, errno);
dev->flags |= DEV_OPEN_FAILURE;
return 0;
@@ -415,10 +421,12 @@ int dev_open_flags(struct device *dev, int flags, int direct, int quiet)
if ((flags & O_CREAT) && !(flags & O_TRUNC))
dev->end = lseek(dev->fd, (off_t) 0, SEEK_END);
- log_debug_devs("Opened %s %s%s%s", dev_name(dev),
- dev->flags & DEV_OPENED_RW ? "RW" : "RO",
- dev->flags & DEV_OPENED_EXCL ? " O_EXCL" : "",
- dev->flags & DEV_O_DIRECT ? " O_DIRECT" : "");
+ if (!quiet) {
+ log_debug_devs("Opened %s %s%s%s", name,
+ dev->flags & DEV_OPENED_RW ? "RW" : "RO",
+ dev->flags & DEV_OPENED_EXCL ? " O_EXCL" : "",
+ dev->flags & DEV_O_DIRECT ? " O_DIRECT" : "");
+ }
dev->flags &= ~DEV_OPEN_FAILURE;
return 1;
diff --git a/lib/device/device.h b/lib/device/device.h
index 8c3a8c30e..572994bb9 100644
--- a/lib/device/device.h
+++ b/lib/device/device.h
@@ -204,9 +204,6 @@ struct device *dev_create_file(const char *filename, struct device *dev,
struct dm_str_list *alias, int use_malloc);
void dev_destroy_file(struct device *dev);
-/* Return a valid device name from the alias list; NULL otherwise */
-const char *dev_name_confirmed(struct device *dev, int quiet);
-
int dev_mpath_init(const char *config_wwids_file);
void dev_mpath_exit(void);
--
2.34.3