Gitweb: https://sourceware.org/git/?p=lvm2.git;a=commitdiff;h=7e70041e324e1a4d49134a... Commit: 7e70041e324e1a4d49134a93323072e1b6ec661f Parent: 1126be8f8dbce482e82d8f98a7e7f4b7cd2cac34 Author: David Teigland teigland@redhat.com AuthorDate: Thu Feb 24 16:03:21 2022 -0600 Committer: David Teigland teigland@redhat.com CommitterDate: Thu Feb 24 17:22:03 2022 -0600
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 1777628a8..8db28bd84 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 a88e3c282..622335982 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);
lvm2-commits@lists.fedorahosted.org
-
David Teigland