Re: [dm-devel] [PATCH 1/1] dm raid: fix compat_features validation
On 10/11/2016 07:44 PM, Mike Snitzer wrote: On Tue, Oct 11 2016 at 11:44am -0400, Heinz Mauelshagen 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
Re: [dm-devel] [PATCH 1/1] dm raid: fix compat_features validation
On Tue, Oct 11 2016 at 11:44am -0400, Heinz Mauelshagen 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
Re: [dm-devel] [PATCH 1/1] dm raid: fix compat_features validation
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
Re: [dm-devel] [PATCH 1/1] dm raid: fix compat_features validation
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
Re: [dm-devel] [PATCH 1/1] dm raid: fix compat_features validation
Andy, good catch. We should rather check for V190 support only in case any compat feature flags are actually set. I.e.: 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) { 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 --- 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
Re: [dm-devel] [PATCH 1/1] dm raid: fix compat_features validation
On Tue, Oct 11 2016 at 10:28am -0400, 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 > --- > 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. Right, your patch looks good to me. But I'll wait for confirmation from Heinz before I stage your fix. Thanks, Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel