From 06c789eda19445d5e59a9c8044d91300fa1d2000 Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Wed, 30 May 2018 14:14:59 +0100 Subject: [PATCH 1/2] radix-tree: fix some bugs in remove_prefix and iterate These weren't working if the prefix key was part of a prefix_chain. --- base/data-struct/radix-tree.c | 22 ++++++++++++-- test/unit/radix_tree_t.c | 56 +++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/base/data-struct/radix-tree.c b/base/data-struct/radix-tree.c index 8e8ac9004..222b35047 100644 --- a/base/data-struct/radix-tree.c +++ b/base/data-struct/radix-tree.c @@ -736,11 +736,29 @@ bool radix_tree_remove(struct radix_tree *rt, uint8_t *key_begin, uint8_t *key_e return false; } +static bool _prefix_chain_matches(struct lookup_result *lr, uint8_t *ke) +{ + // It's possible the top node is a prefix chain, and + // the remaining key matches part of it. + if (lr->v->type == PREFIX_CHAIN) { + unsigned i, rlen = ke - lr->kb; + struct prefix_chain *pc = lr->v->value.ptr; + if (rlen < pc->len) { + for (i = 0; i < rlen; i++) + if (pc->prefix[i] != lr->kb[i]) + return false; + return true; + } + } + + return false; +} + unsigned radix_tree_remove_prefix(struct radix_tree *rt, uint8_t *kb, uint8_t *ke) { unsigned count = 0; struct lookup_result lr = _lookup_prefix(&rt->root, kb, ke); - if (lr.kb == ke) { + if (lr.kb == ke || _prefix_chain_matches(&lr, ke)) { count = _free_node(rt, *lr.v); lr.v->type = UNSET; } @@ -837,7 +855,7 @@ void radix_tree_iterate(struct radix_tree *rt, uint8_t *kb, uint8_t *ke, struct radix_tree_iterator *it) { struct lookup_result lr = _lookup_prefix(&rt->root, kb, ke); - if (lr.kb == ke) + if (lr.kb == ke || _prefix_chain_matches(&lr, ke)) _iterate(lr.v, it); } diff --git a/test/unit/radix_tree_t.c b/test/unit/radix_tree_t.c index ba0d5e18e..7266a8a46 100644 --- a/test/unit/radix_tree_t.c +++ b/test/unit/radix_tree_t.c @@ -289,6 +289,18 @@ static void test_remove_prefix(void *fixture) T_ASSERT_EQUAL(radix_tree_remove_prefix(rt, k, k + 1), count); } +static void test_remove_prefix_single(void *fixture) +{ + struct radix_tree *rt = fixture; + uint8_t k[4]; + union radix_value v; + + _gen_key(k, k + sizeof(k)); + v.n = 1234; + T_ASSERT(radix_tree_insert(rt, k, k + sizeof(k), v)); + T_ASSERT_EQUAL(radix_tree_remove_prefix(rt, k, k + 2), 1); +} + static void test_size(void *fixture) { struct radix_tree *rt = fixture; @@ -367,6 +379,47 @@ static void test_iterate_subset(void *fixture) T_ASSERT_EQUAL(vt.count, subset_count); } +static void test_iterate_single(void *fixture) +{ + struct radix_tree *rt = fixture; + uint8_t k[6]; + union radix_value v; + struct visitor vt; + + _gen_key(k, k + sizeof(k)); + v.n = 1234; + T_ASSERT(radix_tree_insert(rt, k, k + sizeof(k), v)); + + vt.count = 0; + vt.it.visit = _visit; + radix_tree_iterate(rt, k, k + 3, &vt.it); + T_ASSERT_EQUAL(vt.count, 1); +} + +static void test_iterate_vary_middle(void *fixture) +{ + struct radix_tree *rt = fixture; + unsigned i; + uint8_t k[6]; + union radix_value v; + struct visitor vt; + + _gen_key(k, k + sizeof(k)); + for (i = 0; i < 16; i++) { + k[3] = i; + v.n = i; + T_ASSERT(radix_tree_insert(rt, k, k + sizeof(k), v)); + } + + vt.it.visit = _visit; + for (i = 0; i < 16; i++) { + vt.count = 0; + k[3] = i; + radix_tree_iterate(rt, k, k + 4, &vt.it); + T_ASSERT_EQUAL(vt.count, 1); + } +} + //---------------------------------------------------------------- #define T(path, desc, fn) register_test(ts, "/base/data-struct/radix-tree/" path, desc, fn) @@ -392,9 +445,12 @@ void radix_tree_tests(struct dm_list *all_tests) T("remove-prefix-keys", "remove a set of keys that have common prefixes", test_remove_prefix_keys); T("remove-prefix-keys-reversed", "remove a set of keys that have common prefixes (reversed)", test_remove_prefix_keys_reversed); T("remove-prefix", "remove a subrange", test_remove_prefix); + T("remove-prefix-single", "remove a subrange with a single entry", test_remove_prefix_single); T("size-spots-duplicates", "duplicate entries aren't counted twice", test_size); T("iterate-all", "iterate all entries in tree", test_iterate_all); T("iterate-subset", "iterate a subset of entries in tree", test_iterate_subset); + T("iterate-single", "iterate a subset that contains a single entry", test_iterate_single); + T("iterate-vary-middle", "iterate keys that vary in the middle", test_iterate_vary_middle); dm_list_add(all_tests, &ts->list); } From 6d14d5d16b92c520b5f4ee464f171684cac40735 Mon Sep 17 00:00:00 2001 From: David Teigland Date: Tue, 29 May 2018 17:02:27 -0500 Subject: [PATCH 2/2] scan: removed failed paths for devices Drop a device path when the scan fails to open it. --- lib/device/dev-cache.c | 187 +++++++++++++++++++++++++++----- lib/device/dev-cache.h | 2 + lib/filters/filter-persistent.c | 10 +- lib/label/label.c | 65 ++++++++--- 4 files changed, 219 insertions(+), 45 deletions(-) diff --git a/lib/device/dev-cache.c b/lib/device/dev-cache.c index dd0155808..c866ff903 100644 --- a/lib/device/dev-cache.c +++ b/lib/device/dev-cache.c @@ -333,10 +333,8 @@ static int _add_alias(struct device *dev, const char *path) /* Is name already there? */ dm_list_iterate_items(strl, &dev->aliases) { - if (!strcmp(strl->str, path)) { - log_debug_devs("%s: Already in device cache", path); + if (!strcmp(strl->str, path)) return 1; - } } sl->str = path; @@ -344,13 +342,7 @@ static int _add_alias(struct device *dev, const char *path) if (!dm_list_empty(&dev->aliases)) { oldpath = dm_list_item(dev->aliases.n, struct dm_str_list)->str; prefer_old = _compare_paths(path, oldpath); - log_debug_devs("%s: Aliased to %s in device cache%s (%d:%d)", - path, oldpath, prefer_old ? "" : " (preferred name)", - (int) MAJOR(dev->dev), (int) MINOR(dev->dev)); - - } else - log_debug_devs("%s: Added to device cache (%d:%d)", path, - (int) MAJOR(dev->dev), (int) MINOR(dev->dev)); + } if (prefer_old) dm_list_add(&dev->aliases, &sl->list); @@ -666,6 +658,29 @@ struct dm_list *dev_cache_get_dev_list_for_lvid(const char *lvid) return dm_hash_lookup(_cache.lvid_index, lvid); } +/* + * Scanning code calls this when it fails to open a device using + * this path. The path is dropped from dev-cache. In the next + * dev_cache_scan it may be added again, but it could be for a + * different device. + */ + +void dev_cache_failed_path(struct device *dev, const char *path) +{ + struct device *dev_by_path; + struct dm_str_list *strl; + + if ((dev_by_path = (struct device *) dm_hash_lookup(_cache.names, path))) + dm_hash_remove(_cache.names, path); + + dm_list_iterate_items(strl, &dev->aliases) { + if (!strcmp(strl->str, path)) { + dm_list_del(&strl->list); + break; + } + } +} + /* * Either creates a new dev, or adds an alias to * an existing dev. @@ -673,6 +688,8 @@ struct dm_list *dev_cache_get_dev_list_for_lvid(const char *lvid) static int _insert_dev(const char *path, dev_t d) { struct device *dev; + struct device *dev_by_devt; + struct device *dev_by_path; static dev_t loopfile_count = 0; int loopfile = 0; char *path_copy; @@ -685,8 +702,26 @@ static int _insert_dev(const char *path, dev_t d) loopfile = 1; } - /* is this device already registered ? */ - if (!(dev = (struct device *) btree_lookup(_cache.devices, (uint32_t) d))) { + dev_by_devt = (struct device *) btree_lookup(_cache.devices, (uint32_t) d); + dev_by_path = (struct device *) dm_hash_lookup(_cache.names, path); + dev = dev_by_devt; + + /* + * Existing device, existing path points to the same device. + */ + if (dev_by_devt && dev_by_path && (dev_by_devt == dev_by_path)) { + log_debug_devs("Found dev %d:%d %s - exists. %.8s", + (int)MAJOR(d), (int)MINOR(d), path, dev->pvid); + return 1; + } + + /* + * No device or path found, add devt to cache.devices, add name to cache.names. + */ + if (!dev_by_devt && !dev_by_path) { + log_debug_devs("Found dev %d:%d %s - new.", + (int)MAJOR(d), (int)MINOR(d), path); + if (!(dev = (struct device *) btree_lookup(_cache.sysfs_only_devices, (uint32_t) d))) { /* create new device */ if (loopfile) { @@ -701,30 +736,126 @@ static int _insert_dev(const char *path, dev_t d) _free(dev); return 0; } - } - if (dm_hash_lookup(_cache.names, path) == dev) { - /* Hash already has matching entry present */ - log_debug("%s: Path already cached.", path); + if (!(path_copy = dm_pool_strdup(_cache.mem, path))) { + log_error("Failed to duplicate path string."); + return 0; + } + + if (!loopfile && !_add_alias(dev, path_copy)) { + log_error("Couldn't add alias to dev cache."); + return 0; + } + + if (!dm_hash_insert(_cache.names, path_copy, dev)) { + log_error("Couldn't add name to hash in dev cache."); + return 0; + } + return 1; } - if (!(path_copy = dm_pool_strdup(_cache.mem, path))) { - log_error("Failed to duplicate path string."); - return 0; + /* + * Existing device, path is new, add path as a new alias for the device. + */ + if (dev_by_devt && !dev_by_path) { + log_debug_devs("Found dev %d:%d %s - new alias.", + (int)MAJOR(d), (int)MINOR(d), path); + + if (!(path_copy = dm_pool_strdup(_cache.mem, path))) { + log_error("Failed to duplicate path string."); + return 0; + } + + if (!loopfile && !_add_alias(dev, path_copy)) { + log_error("Couldn't add alias to dev cache."); + return 0; + } + + if (!dm_hash_insert(_cache.names, path_copy, dev)) { + log_error("Couldn't add name to hash in dev cache."); + return 0; + } + + return 1; } - if (!loopfile && !_add_alias(dev, path_copy)) { - log_error("Couldn't add alias to dev cache."); - return 0; + /* + * No existing device, but path exists and previously pointed + * to a different device. + */ + if (!dev_by_devt && dev_by_path) { + log_debug_devs("Found dev %d:%d %s - new device, path was previously %d:%d.", + (int)MAJOR(d), (int)MINOR(d), path, + (int)MAJOR(dev_by_path->dev), (int)MINOR(dev_by_path->dev)); + + if (!(dev = (struct device *) btree_lookup(_cache.sysfs_only_devices, (uint32_t) d))) { + /* create new device */ + if (loopfile) { + if (!(dev = dev_create_file(path, NULL, NULL, 0))) + return_0; + } else if (!(dev = _dev_create(d))) + return_0; + } + + if (!(btree_insert(_cache.devices, (uint32_t) d, dev))) { + log_error("Couldn't insert device into binary tree."); + _free(dev); + return 0; + } + + if (!(path_copy = dm_pool_strdup(_cache.mem, path))) { + log_error("Failed to duplicate path string."); + return 0; + } + + if (!loopfile && !_add_alias(dev, path_copy)) { + log_error("Couldn't add alias to dev cache."); + return 0; + } + + dm_hash_remove(_cache.names, path); + + if (!dm_hash_insert(_cache.names, path_copy, dev)) { + log_error("Couldn't add name to hash in dev cache."); + return 0; + } + + return 1; + } - if (!dm_hash_insert(_cache.names, path_copy, dev)) { - log_error("Couldn't add name to hash in dev cache."); - return 0; + /* + * Existing device, and path exists and previously pointed to + * a different device. + */ + if (dev_by_devt && dev_by_path) { + log_debug_devs("Found dev %d:%d %s - existing device, path was previously %d:%d.", + (int)MAJOR(d), (int)MINOR(d), path, + (int)MAJOR(dev_by_path->dev), (int)MINOR(dev_by_path->dev)); + + if (!(path_copy = dm_pool_strdup(_cache.mem, path))) { + log_error("Failed to duplicate path string."); + return 0; + } + + if (!loopfile && !_add_alias(dev, path_copy)) { + log_error("Couldn't add alias to dev cache."); + return 0; + } + + dm_hash_remove(_cache.names, path); + + if (!dm_hash_insert(_cache.names, path_copy, dev)) { + log_error("Couldn't add name to hash in dev cache."); + return 0; + } + + return 1; } - return 1; + log_error("Found dev %d:%d %s - failed to use.", (int)MAJOR(d), (int)MINOR(d), path); + return 0; } static char *_join(const char *dir, const char *name) @@ -1064,10 +1195,8 @@ static int _insert(const char *path, const struct stat *info, if (rec && !_insert_dir(path)) return_0; } else { /* add a device */ - if (!S_ISBLK(info->st_mode)) { - log_debug_devs("%s: Not a block device", path); + if (!S_ISBLK(info->st_mode)) return 1; - } if (!_insert_dev(path, info->st_rdev)) return_0; diff --git a/lib/device/dev-cache.h b/lib/device/dev-cache.h index 479727473..df6ba0e73 100644 --- a/lib/device/dev-cache.h +++ b/lib/device/dev-cache.h @@ -70,4 +70,6 @@ struct device *dev_iter_get(struct dev_iter *iter); void dev_reset_error_count(struct cmd_context *cmd); +void dev_cache_failed_path(struct device *dev, const char *path); + #endif diff --git a/lib/filters/filter-persistent.c b/lib/filters/filter-persistent.c index 3fa57f191..e4659aa99 100644 --- a/lib/filters/filter-persistent.c +++ b/lib/filters/filter-persistent.c @@ -286,10 +286,18 @@ out: static int _lookup_p(struct dev_filter *f, struct device *dev) { struct pfilter *pf = (struct pfilter *) f->private; - void *l = dm_hash_lookup(pf->devices, dev_name(dev)); + void *l; struct dm_str_list *sl; int pass = 1; + if (dm_list_empty(&dev->aliases)) { + log_debug_devs("%d:%d: filter cache skipping (no name)", + (int)MAJOR(dev->dev), (int)MINOR(dev->dev)); + return 0; + } + + l = dm_hash_lookup(pf->devices, dev_name(dev)); + /* Cached bad, skip dev */ if (l == PF_BAD_DEVICE) { log_debug_devs("%s: filter cache skipping (cached bad)", dev_name(dev)); diff --git a/lib/label/label.c b/lib/label/label.c index 8ecaa6165..97a43599c 100644 --- a/lib/label/label.c +++ b/lib/label/label.c @@ -501,15 +501,6 @@ retry_open: (int)MAJOR(sbuf.st_rdev), (int)MINOR(sbuf.st_rdev)); } - /* - * FIXME: do we want to try opening this device using - * one of the other path aliases for the same - * major:minor from dev->aliases? We could iterate - * through those aliases to try opening each of them to - * find one that works. What are the consequences of - * using a different, non-preferred alias to a device? - */ - if (!retried) { /* * FIXME: remove this, the theory for this retry is that @@ -550,6 +541,37 @@ static int _scan_dev_close(struct device *dev) return 1; } +static void _drop_bad_aliases(struct device *dev) +{ + struct dm_str_list *strl, *strl2; + const char *name; + struct stat sbuf; + int major = (int)MAJOR(dev->dev); + int minor = (int)MINOR(dev->dev); + int bad; + + dm_list_iterate_items_safe(strl, strl2, &dev->aliases) { + name = strl->str; + bad = 0; + + if (stat(name, &sbuf)) { + bad = 1; + log_debug_devs("Device path check %d:%d %s stat failed errno %d", + major, minor, name, errno); + } else if (sbuf.st_rdev != dev->dev) { + bad = 1; + log_debug_devs("Device path check %d:%d %s stat %d:%d does not match.", + major, minor, name, + (int)MAJOR(sbuf.st_rdev), (int)MINOR(sbuf.st_rdev)); + } + + if (bad) { + log_debug_devs("Device path check %d:%d dropping path %s.", major, minor, name); + dev_cache_failed_path(dev, name); + } + } +} + /* * Read or reread label/metadata from selected devs. * @@ -635,7 +657,11 @@ static int _scan_list(struct cmd_context *cmd, struct dev_filter *f, scan_failed_count++; lvmcache_del_dev(devl->dev); } else { - log_debug_devs("Processing data from device %s fd %d block %p", dev_name(devl->dev), devl->dev->bcache_fd, bb); + log_debug_devs("Processing data from device %s %d:%d fd %d block %p", + dev_name(devl->dev), + (int)MAJOR(devl->dev->dev), + (int)MINOR(devl->dev->dev), + devl->dev->bcache_fd, bb); ret = _process_block(cmd, f, devl->dev, bb, 0, 0, &is_lvm_device); @@ -675,11 +701,6 @@ static int _scan_list(struct cmd_context *cmd, struct dev_filter *f, * to open the devs on the reopen_devs list. * * FIXME: it's not clear if or why this helps. - * - * FIXME: should we delete the first path name from dev->aliases that - * we failed to open the first time before retrying? If that path - * still exists on the system, dev_cache_scan should put it back, but - * if it doesn't exist we don't want to try using it again. */ if (!dm_list_empty(&reopen_devs)) { if (retried_open) { @@ -690,6 +711,20 @@ static int _scan_list(struct cmd_context *cmd, struct dev_filter *f, } retried_open = 1; + dm_list_iterate_items_safe(devl, devl2, &reopen_devs) { + _drop_bad_aliases(devl->dev); + + if (dm_list_empty(&devl->dev->aliases)) { + log_warn("WARNING: Scan ignoring device %d:%d with no paths.", + (int)MAJOR(devl->dev->dev), + (int)MINOR(devl->dev->dev)); + + dm_list_del(&devl->list); + lvmcache_del_dev(devl->dev); + scan_failed_count++; + } + } + /* * This will search the system's /dev for new path names and * could help us reopen the device if it finds a new preferred