| Message ID | 1c517f14-1234-7844-fc6a-cd1b9698fb8b@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On Tue, Oct 11, 2016 at 05:04:34PM +0200, Heinz Mauelshagen wrote: > > Andy, > > good catch. > > We should rather check for V190 support only in case any > compat feature flags are actually set. > > { > + if (le32_to_cpu(sb->compat_features) && > + le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) > { > rs->ti->error = "Unable to assemble array: Unknown flag(s) > in compatible feature flags"; > return -EINVAL; > } If the feature flags are single bit combinations then I believe the below does check exactly that. Checking for no 1s outside of the expected features, caring not for the value of the valid bits: + if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) { with the possibilty to or in additional feature bits as they are added. -apw -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 10/11/2016 05:38 PM, Andy Whitcroft wrote: > On Tue, Oct 11, 2016 at 05:04:34PM +0200, Heinz Mauelshagen wrote: >> Andy, >> >> good catch. >> >> We should rather check for V190 support only in case any >> compat feature flags are actually set. >> >> { >> + if (le32_to_cpu(sb->compat_features) && >> + le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) >> { >> rs->ti->error = "Unable to assemble array: Unknown flag(s) >> in compatible feature flags"; >> return -EINVAL; >> } > If the feature flags are single bit combinations then I believe the > below does check exactly that. Checking for no 1s outside of the > expected features, caring not for the value of the valid bits: > > + if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) { > > with the possibilty to or in additional feature bits as they are added. Thanks, I prefer this to be easier readable. > > -apw > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, Oct 11 2016 at 11:44am -0400, Heinz Mauelshagen <heinzm@redhat.com> wrote: > > > On 10/11/2016 05:38 PM, Andy Whitcroft wrote: > >On Tue, Oct 11, 2016 at 05:04:34PM +0200, Heinz Mauelshagen wrote: > >>Andy, > >> > >>good catch. > >> > >>We should rather check for V190 support only in case any > >>compat feature flags are actually set. > >> > >>{ > >>+ if (le32_to_cpu(sb->compat_features) && > >>+ le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) > >>{ > >> rs->ti->error = "Unable to assemble array: Unknown flag(s) > >>in compatible feature flags"; > >> return -EINVAL; > >> } > >If the feature flags are single bit combinations then I believe the > >below does check exactly that. Checking for no 1s outside of the > >expected features, caring not for the value of the valid bits: > > > >+ if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) { > > > >with the possibilty to or in additional feature bits as they are added. > > Thanks, > I prefer this to be easier readable. Readable or not, the code with the != is _not_ future-proof. Whereas Andy's solution is. If/when a new compat feature comes along then FEATURE_FLAG_SUPPORTS_V190 would be replaced to be a macro that ORs all the new compat features together (e.g. FEATURE_FLAG_COMPAT). E.g. how dm-thin-metadata.c:__check_incompat_features() does. We can go with the != code for now, since any future changes would likely cause this test to be changed. Or we could fix it now _for real_. Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 10/11/2016 07:44 PM, Mike Snitzer wrote: > On Tue, Oct 11 2016 at 11:44am -0400, > Heinz Mauelshagen <heinzm@redhat.com> wrote: > >> >> On 10/11/2016 05:38 PM, Andy Whitcroft wrote: >>> On Tue, Oct 11, 2016 at 05:04:34PM +0200, Heinz Mauelshagen wrote: >>>> Andy, >>>> >>>> good catch. >>>> >>>> We should rather check for V190 support only in case any >>>> compat feature flags are actually set. >>>> >>>> { >>>> + if (le32_to_cpu(sb->compat_features) && >>>> + le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) >>>> { >>>> rs->ti->error = "Unable to assemble array: Unknown flag(s) >>>> in compatible feature flags"; >>>> return -EINVAL; >>>> } >>> If the feature flags are single bit combinations then I believe the >>> below does check exactly that. Checking for no 1s outside of the >>> expected features, caring not for the value of the valid bits: >>> >>> + if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) { >>> >>> with the possibilty to or in additional feature bits as they are added. >> Thanks, >> I prefer this to be easier readable. > Readable or not, the code with the != is _not_ future-proof. Whereas > Andy's solution is. If/when a new compat feature comes along then > FEATURE_FLAG_SUPPORTS_V190 would be replaced to be a macro that ORs all > the new compat features together (e.g. FEATURE_FLAG_COMPAT). E.g. how > dm-thin-metadata.c:__check_incompat_features() does. If we'll have to introduce more feature flags in the future (e.g. for clustered raid1 support), this is going to be based on the test_bit() API for consistency with any other flag processing we do in the target. Heinz > We can go with the != code for now, since any future changes would > likely cause this test to be changed. Or we could fix it now _for > real_. > > Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c index 8abde6b..2a39700 100644 --- a/drivers/md/dm-raid.c +++ b/drivers/md/dm-raid.c @@ -2258,7 +2258,8 @@ static int super_validate(struct raid_set *rs, struct md_rdev *rdev) if (!mddev->events && super_init_validation(rs, rdev)) return -EINVAL; - if (le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) { + if (le32_to_cpu(sb->compat_features) && + le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) {
Andy, good catch. We should rather check for V190 support only in case any compat feature flags are actually set. I.e.: rs->ti->error = "Unable to assemble array: Unknown flag(s) in compatible feature flags"; return -EINVAL; } On 10/11/2016 04:28 PM, Andy Whitcroft wrote: > In commit ecbfb9f118bce4 ("dm raid: add raid level takeover support") a new > compatible feature flag was added. Validation for these compat_features > was added but this only passes for new raid mappings with this feature > flag. This causes previously created raid mappings to be failed at import. > > Check compat_features for any valid combinations. > > Fixes: ecbfb9f118bce4 ("dm raid: add raid level takeover support") > BugLink: http://bugs.launchpad.net/bugs/1631298 > Signed-off-by: Andy Whitcroft <apw@canonical.com> > --- > drivers/md/dm-raid.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > It very much looks like these are intended to be optional extended feature > flags. That we should be accepting any valid flag and rejecting any bit > not in that set. We should however not be ensuring that specific bits > are actually set. Certainly as things stand raid sets built on previous > kernel versions cannot be assembled. > > diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c > index 8abde6b..6ddea60 100644 > --- a/drivers/md/dm-raid.c > +++ b/drivers/md/dm-raid.c > @@ -2258,7 +2258,7 @@ static int super_validate(struct raid_set *rs, struct md_rdev *rdev) > if (!mddev->events && super_init_validation(rs, rdev)) > return -EINVAL; > > - if (le32_to_cpu(sb->compat_features) != FEATURE_FLAG_SUPPORTS_V190) { > + if (le32_to_cpu(sb->compat_features) & ~(FEATURE_FLAG_SUPPORTS_V190)) { > rs->ti->error = "Unable to assemble array: Unknown flag(s) in compatible feature flags"; > return -EINVAL; > } -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel