| Message ID | 20240712171458.77611-48-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath-tools: devmapper API refactored | expand |
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@suse.com> > --- > 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@redhat.com> -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
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, (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;
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@suse.com> --- libmultipath/devmapper.c | 48 +++++++++++++++++++------------ libmultipath/devmapper.h | 2 +- libmultipath/libmultipath.version | 2 +- multipathd/main.c | 2 +- 4 files changed, 33 insertions(+), 21 deletions(-)