| Message ID | 20241125143224.51934-3-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Benjamin Marzinski |
| Headers | show |
| Series | multipath-tools fixes | expand |
On Mon, Nov 25, 2024 at 03:32:23PM +0100, Martin Wilck wrote: > pathcmp() makes sure that all paths in pgp have a match in cpgp, but not > vice-versa. Check the number of paths, too. This looks fine. But looking at it made we a nervous about cpgp->id. We only calculate that in pgcmp() (and just for mpp, not cmpp) and in disassemble_map(). But we clearly can have pathgroup changes after that, as the last patch has shown. To be safe, we should either skip the whole pgp->id thing (it's not a huge time savings) or recalculate it for all of cmpp's path groups before we start the loops in pgcmp(). -Ben > > Fixes: 90773ba ("libmultipath: resolve hash collisions in pgcmp()") > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/configure.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libmultipath/configure.c b/libmultipath/configure.c > index d0e9c95..55140d0 100644 > --- a/libmultipath/configure.c > +++ b/libmultipath/configure.c > @@ -442,6 +442,7 @@ pgcmp (struct multipath * mpp, struct multipath * cmpp) > > vector_foreach_slot (cmpp->pg, cpgp, j) { > if (pgp->id == cpgp->id && > + VECTOR_SIZE(pgp->paths) == VECTOR_SIZE(cpgp->paths) && > !pathcmp(pgp, cpgp)) { > r = 0; > break; > -- > 2.47.0
On Mon, 2024-11-25 at 16:06 -0500, Benjamin Marzinski wrote: > On Mon, Nov 25, 2024 at 03:32:23PM +0100, Martin Wilck wrote: > > pathcmp() makes sure that all paths in pgp have a match in cpgp, > > but not > > vice-versa. Check the number of paths, too. > > This looks fine. But looking at it made we a nervous about cpgp->id. > We > only calculate that in pgcmp() (and just for mpp, not cmpp) and in > disassemble_map(). But we clearly can have pathgroup changes after > that, > as the last patch has shown. To be safe, we should either skip the > whole > pgp->id thing (it's not a huge time savings) or recalculate it for > all > of cmpp's path groups before we start the loops in pgcmp(). Agreed. Like before, I am wondering if we should drop pgp->id in 0.11.0 already or postpone. IMO my patch is a valid fix for a minor issue which needs redesign in a future release. Martin
On Tue, 2024-11-26 at 12:15 +0100, Martin Wilck wrote: > > IMO my patch is a valid fix for a minor issue which needs redesign in > a > future release. I meant to say: IMO my patch is a valid fix for a minor issue in a flawed design which needs redesign in a future release. Sorry, Martin
On Tue, Nov 26, 2024 at 12:15:06PM +0100, Martin Wilck wrote: > On Mon, 2024-11-25 at 16:06 -0500, Benjamin Marzinski wrote: > > On Mon, Nov 25, 2024 at 03:32:23PM +0100, Martin Wilck wrote: > > > pathcmp() makes sure that all paths in pgp have a match in cpgp, > > > but not > > > vice-versa. Check the number of paths, too. > > > > This looks fine. But looking at it made we a nervous about cpgp->id. > > We > > only calculate that in pgcmp() (and just for mpp, not cmpp) and in > > disassemble_map(). But we clearly can have pathgroup changes after > > that, > > as the last patch has shown. To be safe, we should either skip the > > whole > > pgp->id thing (it's not a huge time savings) or recalculate it for > > all > > of cmpp's path groups before we start the loops in pgcmp(). > > Agreed. Like before, I am wondering if we should drop pgp->id in 0.11.0 > already or postpone. > > IMO my patch is a valid fix for a minor issue which needs redesign in a > future release. Yep. Like I said, your patch looks fine. I just noticed the gpg->id issue while looking at it. -Ben > > Martin
diff --git a/libmultipath/configure.c b/libmultipath/configure.c index d0e9c95..55140d0 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -442,6 +442,7 @@ pgcmp (struct multipath * mpp, struct multipath * cmpp) vector_foreach_slot (cmpp->pg, cpgp, j) { if (pgp->id == cpgp->id && + VECTOR_SIZE(pgp->paths) == VECTOR_SIZE(cpgp->paths) && !pathcmp(pgp, cpgp)) { r = 0; break;
pathcmp() makes sure that all paths in pgp have a match in cpgp, but not vice-versa. Check the number of paths, too. Fixes: 90773ba ("libmultipath: resolve hash collisions in pgcmp()") Signed-off-by: Martin Wilck <mwilck@suse.com> --- libmultipath/configure.c | 1 + 1 file changed, 1 insertion(+)