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 <mwilck@xxxxxxxx> > --- > 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 <bmarzins@xxxxxxxxxx> -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