On Fri, Jul 12, 2024 at 07:14:55PM +0200, Martin Wilck wrote:
> We've removed partition mappings recursively since 83fb936 ("Correctly remove
> logical partition maps"). This was wrong, because kpartx doesn't create
> logical partitions as mappings onto the extended partition. Rather, logical
> partitions are created by kpartx as mappings to the multipath device, and
> afaics, this has always been the case. Therefore, the loop in
> do_foreach_partmaps() will detect all partition mappings (primary, extended,
> and logical) without recursion. At least since 4059e42 ("libmultipath: fix
> partition detection"), the recursion has actually been pointless, because
> is_mpath_part() would never have returned "true" for a pair of two partition
> mappings (one representing an extended partition and one a logical partition).
>
> Avoiding the recursion has the additional benefit that the complexity of
> removing maps scales with N, where N is the number of dm devices, rather than
> with N^2. Also, it simplifies the code.
>
> Split partmap_in_use() into two separate functions, mpath_in_use() (to be
> called
> for multipath maps) and count_partitions(), which is called from
> do_foreach_partmaps().
>
> Because do_foreach_partmaps() is now only legitimately called for multipath
> maps, quit early if called for another map type.
>
> Signed-off-by: Martin Wilck <[email protected]>
> ---
> libmultipath/devmapper.c | 48 +++++++++++++++++++------------
> libmultipath/devmapper.h | 2 +-
> libmultipath/libmultipath.version | 2 +-
> multipathd/main.c | 2 +-
> 4 files changed, 33 insertions(+), 21 deletions(-)
>
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 5749d63..d9d96be 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -933,22 +933,37 @@ has_partmap(const char *name __attribute__((unused)),
> return 1;
> }
>
> -int
> -partmap_in_use(const char *name, void *data)
> +/*
> + * This will be called from mpath_in_use, for each partition.
> + * If the partition itself in use, returns 1 immediately, causing
> + * do_foreach_partmaps() to stop iterating and return 1.
> + * Otherwise, increases the partition count.
> + */
> +static int count_partitions(const char *name, void *data)
> +{
> + int *ret_count = (int *)data;
> + int open_count = dm_get_opencount(name);
> +
> + if (open_count)
> + return 1;
> + (*ret_count)++;
> + return 0;
> +}
> +
> +int mpath_in_use(const char *name)
> {
> - int part_count, *ret_count = (int *)data;
> int open_count = dm_get_opencount(name);
>
> - if (ret_count)
> - (*ret_count)++;
> - part_count = 0;
> if (open_count) {
> - if (do_foreach_partmaps(name, partmap_in_use, &part_count))
> - return 1;
> - if (open_count != part_count) {
> - condlog(2, "%s: map in use", name);
> + int part_count = 0;
> +
> + if (do_foreach_partmaps(name, count_partitions, &part_count)) {
> + condlog(4, "%s: %s has open partitions", __func__,
> name);
> return 1;
> }
> + condlog(4, "%s: %s: %d openers, %d partitions", __func__, name,
> + open_count, part_count);
> + return open_count > part_count;
> }
> return 0;
> }
> @@ -976,7 +991,7 @@ int _dm_flush_map (const char *mapname, int flags, int
> retries)
>
> /* If you aren't doing a deferred remove, make sure that no
> * devices are in use */
> - if (!(flags & DMFL_DEFERRED) && partmap_in_use(mapname, NULL))
> + if (!(flags & DMFL_DEFERRED) && mpath_in_use(mapname))
> return DM_FLUSH_BUSY;
>
> if ((flags & DMFL_SUSPEND) &&
> @@ -1314,7 +1329,7 @@ do_foreach_partmaps (const char *mapname,
> char map_uuid[DM_UUID_LEN];
> struct dm_info info;
>
> - if (libmp_mapinfo(DM_MAP_BY_NAME,
> + if (libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
This patch is out of order. You add MAPINFO_CHECK_UUID in the next one.
Otherwise
Reviewed-by: Benjamin Marzinski <[email protected]>
-Ben
> (mapid_t) { .str = mapname },
> (mapinfo_t) { .uuid = map_uuid, .dmi = &info }) !=
> DMP_OK)
> return 1;
> @@ -1381,12 +1396,9 @@ remove_partmap(const char *name, void *data)
> {
> struct remove_data *rd = (struct remove_data *)data;
>
> - if (dm_get_opencount(name)) {
> - dm_remove_partmaps(name, rd->flags);
> - if (!(rd->flags & DMFL_DEFERRED) && dm_get_opencount(name)) {
> - condlog(2, "%s: map in use", name);
> - return DM_FLUSH_BUSY;
> - }
> + if (!(rd->flags & DMFL_DEFERRED) && dm_get_opencount(name)) {
> + condlog(2, "%s: map in use", name);
> + return DM_FLUSH_BUSY;
> }
> condlog(4, "partition map %s removed", name);
> dm_device_remove(name, rd->flags);
> diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> index d01f9f2..6eb5ab9 100644
> --- a/libmultipath/devmapper.h
> +++ b/libmultipath/devmapper.h
> @@ -149,7 +149,7 @@ enum {
> DM_FLUSH_BUSY,
> };
>
> -int partmap_in_use(const char *name, void *data);
> +int mpath_in_use(const char *name);
>
> enum {
> DMFL_NONE = 0,
> diff --git a/libmultipath/libmultipath.version
> b/libmultipath/libmultipath.version
> index 54b5a23..649c1cb 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -139,10 +139,10 @@ global:
> libmultipath_exit;
> libmultipath_init;
> load_config;
> + mpath_in_use;
> need_io_err_check;
> orphan_path;
> parse_prkey_flags;
> - partmap_in_use;
> pathcount;
> path_discovery;
> path_get_tpgs;
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 536974c..13af94e 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -597,7 +597,7 @@ flush_map_nopaths(struct multipath *mpp, struct vectors
> *vecs) {
> return false;
> }
> if (mpp->flush_on_last_del == FLUSH_UNUSED &&
> - partmap_in_use(mpp->alias, NULL) && is_queueing) {
> + mpath_in_use(mpp->alias) && is_queueing) {
> condlog(2, "%s: map in use and queueing, can't remove",
> mpp->alias);
> return false;
> --
> 2.45.2
