* Can we deprecate ioctl(RAID_VERSION)? @ 2017-04-05 17:55 jes.sorensen 2017-04-05 19:02 ` jes.sorensen 0 siblings, 1 reply; 13+ messages in thread From: jes.sorensen @ 2017-04-05 17:55 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid Hi Neil, Looking through the code in mdadm, I noticed a number of cases calling ioctl(RAID_VERSION). At first I had it confused with metadata version, but it looks like RAID_VERSION will always return 90000 if it's a valid raid device. In the cases we want to confirm the fd is a valid raid array, ioctl(GET_ARRAY_INFO) should do, or sysfs_read(GET_VERSION). Am I missing something obvious here, or do you see any reason for leaving this around? Thanks, Jes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-05 17:55 Can we deprecate ioctl(RAID_VERSION)? jes.sorensen @ 2017-04-05 19:02 ` jes.sorensen 2017-04-05 22:32 ` NeilBrown 0 siblings, 1 reply; 13+ messages in thread From: jes.sorensen @ 2017-04-05 19:02 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid jes.sorensen@gmail.com writes: > Hi Neil, > > Looking through the code in mdadm, I noticed a number of cases calling > ioctl(RAID_VERSION). At first I had it confused with metadata version, > but it looks like RAID_VERSION will always return 90000 if it's a valid > raid device. > > In the cases we want to confirm the fd is a valid raid array, > ioctl(GET_ARRAY_INFO) should do, or sysfs_read(GET_VERSION). > > Am I missing something obvious here, or do you see any reason for > leaving this around? Sorry the above is wrong, it will always return 900, not 90000. Some of the code that stood out is in util.c: int md_get_version(int fd) { struct stat stb; mdu_version_t vers; if (fstat(fd, &stb)<0) return -1; if ((S_IFMT&stb.st_mode) != S_IFBLK) return -1; if (ioctl(fd, RAID_VERSION, &vers) == 0) return (vers.major*10000) + (vers.minor*100) + vers.patchlevel; if (errno == EACCES) return -1; if (major(stb.st_rdev) == MD_MAJOR) return (3600); return -1; } .... int set_array_info(int mdfd, struct supertype *st, struct mdinfo *info) { /* Initialise kernel's knowledge of array. * This varies between externally managed arrays * and older kernels */ int vers = md_get_version(mdfd); int rv; #ifndef MDASSEMBLE if (st->ss->external) rv = sysfs_set_array(info, vers); else #endif if ((vers % 100) >= 1) { /* can use different versions */ mdu_array_info_t inf; memset(&inf, 0, sizeof(inf)); inf.major_version = info->array.major_version; inf.minor_version = info->array.minor_version; rv = ioctl(mdfd, SET_ARRAY_INFO, &inf); } else rv = ioctl(mdfd, SET_ARRAY_INFO, NULL); return rv; } This has been around since at least 2008, the current code came in f35f25259279573c6274e2783536c0b0a399bdd4, but it looks like even the prior code made the same assumptions. In either case, the above 'if ((vers % 100) >= 1)' will always trigger since the kernel does #define MD_PATCHLEVEL_VERSION 3 It's not like we have been updating MD_PATCHLEVEL_VERSION for a while. Was the code meant to be looking at the superblock minor version? I've been staring at this for a while now, so please beat me over the head if I missed something blatantly obvious. Jes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-05 19:02 ` jes.sorensen @ 2017-04-05 22:32 ` NeilBrown 2017-04-06 15:31 ` Jes Sorensen 0 siblings, 1 reply; 13+ messages in thread From: NeilBrown @ 2017-04-05 22:32 UTC (permalink / raw) To: jes.sorensen; +Cc: linux-raid [-- Attachment #1: Type: text/plain, Size: 3647 bytes --] On Thu, Apr 06 2017, jes.sorensen@gmail.com wrote: > jes.sorensen@gmail.com writes: >> Hi Neil, >> >> Looking through the code in mdadm, I noticed a number of cases calling >> ioctl(RAID_VERSION). At first I had it confused with metadata version, >> but it looks like RAID_VERSION will always return 90000 if it's a valid >> raid device. >> >> In the cases we want to confirm the fd is a valid raid array, >> ioctl(GET_ARRAY_INFO) should do, or sysfs_read(GET_VERSION). >> >> Am I missing something obvious here, or do you see any reason for >> leaving this around? > > Sorry the above is wrong, it will always return 900, not 90000. Some of > the code that stood out is in util.c: > > int md_get_version(int fd) > { > struct stat stb; > mdu_version_t vers; > > if (fstat(fd, &stb)<0) > return -1; > if ((S_IFMT&stb.st_mode) != S_IFBLK) > return -1; > > if (ioctl(fd, RAID_VERSION, &vers) == 0) > return (vers.major*10000) + (vers.minor*100) + vers.patchlevel; > if (errno == EACCES) > return -1; > if (major(stb.st_rdev) == MD_MAJOR) > return (3600); > return -1; > } > > .... > > int set_array_info(int mdfd, struct supertype *st, struct mdinfo *info) > { > /* Initialise kernel's knowledge of array. > * This varies between externally managed arrays > * and older kernels > */ > int vers = md_get_version(mdfd); > int rv; > > #ifndef MDASSEMBLE > if (st->ss->external) > rv = sysfs_set_array(info, vers); > else > #endif > if ((vers % 100) >= 1) { /* can use different versions */ > mdu_array_info_t inf; > memset(&inf, 0, sizeof(inf)); > inf.major_version = info->array.major_version; > inf.minor_version = info->array.minor_version; > rv = ioctl(mdfd, SET_ARRAY_INFO, &inf); > } else > rv = ioctl(mdfd, SET_ARRAY_INFO, NULL); > return rv; > } > > This has been around since at least 2008, the current code came in > f35f25259279573c6274e2783536c0b0a399bdd4, but it looks like even the > prior code made the same assumptions. > > In either case, the above 'if ((vers % 100) >= 1)' will always trigger > since the kernel does #define MD_PATCHLEVEL_VERSION 3 > > It's not like we have been updating MD_PATCHLEVEL_VERSION for a > while. Was the code meant to be looking at the superblock minor version? > I've been staring at this for a while now, so please beat me over the > head if I missed something blatantly obvious. > > Jes It is hard to get versioning right... The version returned by the RAID_VERSION ioctl is meant to reflect the capabilities of the implementation. We could use the kernel version number for that (and sometimes do), but as distro's often backport features, that isn't always reliable. I've incremented the MD_PATCHLEVEL_VERSION when a change is made that cannot easily be detected from user-space. As you note, we are up to three. The last change was in 2.6.15. I've never contemplated changing the other two numbers that RAID_VERSION return. They don't seem to mean anything useful. What exactly do you mean by "deprecate" the ioctl? If you remove the code in mdadm that calls it, mdadm will not work correctly on kernels older than 2.6.15, and it will be harder to and an future capability that is not easily visible from user space. If you remove the code in the kernel that handles it, you'll break mdadm. What is the goal here? Thanks, NeilBrown [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-05 22:32 ` NeilBrown @ 2017-04-06 15:31 ` Jes Sorensen 2017-04-06 18:10 ` Coly Li 2017-04-06 22:44 ` NeilBrown 0 siblings, 2 replies; 13+ messages in thread From: Jes Sorensen @ 2017-04-06 15:31 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid, Hannes Reinecke, kernel-team On 04/05/2017 06:32 PM, NeilBrown wrote: > On Thu, Apr 06 2017, jes.sorensen@gmail.com wrote: > >> jes.sorensen@gmail.com writes: >>> Hi Neil, >>> >>> Looking through the code in mdadm, I noticed a number of cases calling >>> ioctl(RAID_VERSION). At first I had it confused with metadata version, >>> but it looks like RAID_VERSION will always return 90000 if it's a valid >>> raid device. >>> >>> In the cases we want to confirm the fd is a valid raid array, >>> ioctl(GET_ARRAY_INFO) should do, or sysfs_read(GET_VERSION). >>> >>> Am I missing something obvious here, or do you see any reason for >>> leaving this around? >> >> Sorry the above is wrong, it will always return 900, not 90000. Some of >> the code that stood out is in util.c: >> >> int md_get_version(int fd) >> { >> struct stat stb; >> mdu_version_t vers; >> >> if (fstat(fd, &stb)<0) >> return -1; >> if ((S_IFMT&stb.st_mode) != S_IFBLK) >> return -1; >> >> if (ioctl(fd, RAID_VERSION, &vers) == 0) >> return (vers.major*10000) + (vers.minor*100) + vers.patchlevel; >> if (errno == EACCES) >> return -1; >> if (major(stb.st_rdev) == MD_MAJOR) >> return (3600); >> return -1; >> } >> >> .... >> >> int set_array_info(int mdfd, struct supertype *st, struct mdinfo *info) >> { >> /* Initialise kernel's knowledge of array. >> * This varies between externally managed arrays >> * and older kernels >> */ >> int vers = md_get_version(mdfd); >> int rv; >> >> #ifndef MDASSEMBLE >> if (st->ss->external) >> rv = sysfs_set_array(info, vers); >> else >> #endif >> if ((vers % 100) >= 1) { /* can use different versions */ >> mdu_array_info_t inf; >> memset(&inf, 0, sizeof(inf)); >> inf.major_version = info->array.major_version; >> inf.minor_version = info->array.minor_version; >> rv = ioctl(mdfd, SET_ARRAY_INFO, &inf); >> } else >> rv = ioctl(mdfd, SET_ARRAY_INFO, NULL); >> return rv; >> } >> >> This has been around since at least 2008, the current code came in >> f35f25259279573c6274e2783536c0b0a399bdd4, but it looks like even the >> prior code made the same assumptions. >> >> In either case, the above 'if ((vers % 100) >= 1)' will always trigger >> since the kernel does #define MD_PATCHLEVEL_VERSION 3 >> >> It's not like we have been updating MD_PATCHLEVEL_VERSION for a >> while. Was the code meant to be looking at the superblock minor version? >> I've been staring at this for a while now, so please beat me over the >> head if I missed something blatantly obvious. >> >> Jes > > It is hard to get versioning right... > > The version returned by the RAID_VERSION ioctl is meant to reflect the > capabilities of the implementation. We could use the kernel version > number for that (and sometimes do), but as distro's often backport > features, that isn't always reliable. > > I've incremented the MD_PATCHLEVEL_VERSION when a change is made that > cannot easily be detected from user-space. As you note, we are up to > three. The last change was in 2.6.15. > I've never contemplated changing the other two numbers that RAID_VERSION > return. They don't seem to mean anything useful. > > What exactly do you mean by "deprecate" the ioctl? > If you remove the code in mdadm that calls it, mdadm will not work > correctly on kernels older than 2.6.15, and it will be harder to > and an future capability that is not easily visible from user space. > If you remove the code in the kernel that handles it, you'll break > mdadm. Neil, I see, thanks for explaining. The goal is to eventually get out of the ioctl() business and get to a state where we can do everything via sysfs/configfs. Right now we have a big mix between ioctl and sysfs where neither interface does everything. The recent issues with PPL (I think it was) showed that we had to add more ioctl support because the interfaces needed to do it for sysfs weren't quite there. My long term goal is to get that situation improved so we can avoid adding anymore ioctl interfaces and eventually allow for distros to build mdadm with ioctl support disabled. We had a discussion at LSF/MM in Boston about this (Hannes, Shaohua, Song, and myself). Reading the code I found it confusing that it was so tied to the patch level, but didn't do anything with the version numbers. At least intuitively if I bumped the version number, I would reset the patchlevel which would break things. I think it's fair to draw a line in the sand and say that mdadm-4.1+ will not support kernels older than 2.6.15. I am open to the kernel version we pick here, but I would like to start deprecating some of the really old code. I have patches that does this in my tree, but I need to add a check for kernel version > 2.6.15. I am not aware what SuSE's enterprise kernel versions look like, but checking RHEL/CentOS RHEL5 was 2.6.18, while RHEL4 was 2.6.9 - and RHEL4 has been unsupported for quite a while. At least for RHEL/CentOS 2.6.15 as the line in the sand seems fine. For the kernel to expose features to userland in the future, I would prefer to go with a feature-flag style interface exposed via sysfs. That way a distro could enable one feature, but not the other in their kernel without having to worry about actual version numbers. Cheers, Jes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-06 15:31 ` Jes Sorensen @ 2017-04-06 18:10 ` Coly Li 2017-04-06 18:14 ` Jes Sorensen 2017-04-06 22:44 ` NeilBrown 1 sibling, 1 reply; 13+ messages in thread From: Coly Li @ 2017-04-06 18:10 UTC (permalink / raw) To: Jes Sorensen, NeilBrown; +Cc: linux-raid, Hannes Reinecke, kernel-team On 2017/4/6 下午11:31, Jes Sorensen wrote: > On 04/05/2017 06:32 PM, NeilBrown wrote: >> On Thu, Apr 06 2017, jes.sorensen@gmail.com wrote: >> >>> jes.sorensen@gmail.com writes: >>>> Hi Neil, >>>> >>>> Looking through the code in mdadm, I noticed a number of cases calling >>>> ioctl(RAID_VERSION). At first I had it confused with metadata version, >>>> but it looks like RAID_VERSION will always return 90000 if it's a valid >>>> raid device. >>>> >>>> In the cases we want to confirm the fd is a valid raid array, >>>> ioctl(GET_ARRAY_INFO) should do, or sysfs_read(GET_VERSION). >>>> >>>> Am I missing something obvious here, or do you see any reason for >>>> leaving this around? >>> >>> Sorry the above is wrong, it will always return 900, not 90000. Some of >>> the code that stood out is in util.c: >>> >>> int md_get_version(int fd) >>> { >>> struct stat stb; >>> mdu_version_t vers; >>> >>> if (fstat(fd, &stb)<0) >>> return -1; >>> if ((S_IFMT&stb.st_mode) != S_IFBLK) >>> return -1; >>> >>> if (ioctl(fd, RAID_VERSION, &vers) == 0) >>> return (vers.major*10000) + (vers.minor*100) + >>> vers.patchlevel; >>> if (errno == EACCES) >>> return -1; >>> if (major(stb.st_rdev) == MD_MAJOR) >>> return (3600); >>> return -1; >>> } >>> >>> .... >>> >>> int set_array_info(int mdfd, struct supertype *st, struct mdinfo *info) >>> { >>> /* Initialise kernel's knowledge of array. >>> * This varies between externally managed arrays >>> * and older kernels >>> */ >>> int vers = md_get_version(mdfd); >>> int rv; >>> >>> #ifndef MDASSEMBLE >>> if (st->ss->external) >>> rv = sysfs_set_array(info, vers); >>> else >>> #endif >>> if ((vers % 100) >= 1) { /* can use different >>> versions */ >>> mdu_array_info_t inf; >>> memset(&inf, 0, sizeof(inf)); >>> inf.major_version = info->array.major_version; >>> inf.minor_version = info->array.minor_version; >>> rv = ioctl(mdfd, SET_ARRAY_INFO, &inf); >>> } else >>> rv = ioctl(mdfd, SET_ARRAY_INFO, NULL); >>> return rv; >>> } >>> >>> This has been around since at least 2008, the current code came in >>> f35f25259279573c6274e2783536c0b0a399bdd4, but it looks like even the >>> prior code made the same assumptions. >>> >>> In either case, the above 'if ((vers % 100) >= 1)' will always trigger >>> since the kernel does #define MD_PATCHLEVEL_VERSION 3 >>> >>> It's not like we have been updating MD_PATCHLEVEL_VERSION for a >>> while. Was the code meant to be looking at the superblock minor version? >>> I've been staring at this for a while now, so please beat me over the >>> head if I missed something blatantly obvious. >>> >>> Jes >> >> It is hard to get versioning right... >> >> The version returned by the RAID_VERSION ioctl is meant to reflect the >> capabilities of the implementation. We could use the kernel version >> number for that (and sometimes do), but as distro's often backport >> features, that isn't always reliable. >> >> I've incremented the MD_PATCHLEVEL_VERSION when a change is made that >> cannot easily be detected from user-space. As you note, we are up to >> three. The last change was in 2.6.15. >> I've never contemplated changing the other two numbers that RAID_VERSION >> return. They don't seem to mean anything useful. >> >> What exactly do you mean by "deprecate" the ioctl? >> If you remove the code in mdadm that calls it, mdadm will not work >> correctly on kernels older than 2.6.15, and it will be harder to >> and an future capability that is not easily visible from user space. >> If you remove the code in the kernel that handles it, you'll break >> mdadm. > > Neil, > > I see, thanks for explaining. > > The goal is to eventually get out of the ioctl() business and get to a > state where we can do everything via sysfs/configfs. Right now we have a > big mix between ioctl and sysfs where neither interface does everything. > The recent issues with PPL (I think it was) showed that we had to add > more ioctl support because the interfaces needed to do it for sysfs > weren't quite there. My long term goal is to get that situation improved > so we can avoid adding anymore ioctl interfaces and eventually allow for > distros to build mdadm with ioctl support disabled. We had a discussion > at LSF/MM in Boston about this (Hannes, Shaohua, Song, and myself). > > Reading the code I found it confusing that it was so tied to the patch > level, but didn't do anything with the version numbers. At least > intuitively if I bumped the version number, I would reset the patchlevel > which would break things. > > I think it's fair to draw a line in the sand and say that mdadm-4.1+ > will not support kernels older than 2.6.15. I am open to the kernel > version we pick here, but I would like to start deprecating some of the > really old code. I have patches that does this in my tree, but I need to > add a check for kernel version > 2.6.15. I am not aware what SuSE's > enterprise kernel versions look like, but checking RHEL/CentOS RHEL5 was > 2.6.18, while RHEL4 was 2.6.9 - and RHEL4 has been unsupported for quite > a while. At least for RHEL/CentOS 2.6.15 as the line in the sand seems > fine. > > For the kernel to expose features to userland in the future, I would > prefer to go with a feature-flag style interface exposed via sysfs. That > way a distro could enable one feature, but not the other in their kernel > without having to worry about actual version numbers. BTW, I'd like to take on the kernel side stuffs. IMHO, it would be better that we also set a timeline, after the timeline, we should add new interface via configfs, and keep all legancy ioctl implementation before this timeline. Coly ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-06 18:10 ` Coly Li @ 2017-04-06 18:14 ` Jes Sorensen 2017-04-07 3:54 ` Coly Li 0 siblings, 1 reply; 13+ messages in thread From: Jes Sorensen @ 2017-04-06 18:14 UTC (permalink / raw) To: Coly Li, NeilBrown; +Cc: linux-raid, Hannes Reinecke, kernel-team On 04/06/2017 02:10 PM, Coly Li wrote: > On 2017/4/6 下午11:31, Jes Sorensen wrote: >> Neil, >> >> I see, thanks for explaining. >> >> The goal is to eventually get out of the ioctl() business and get to a >> state where we can do everything via sysfs/configfs. Right now we have a >> big mix between ioctl and sysfs where neither interface does everything. >> The recent issues with PPL (I think it was) showed that we had to add >> more ioctl support because the interfaces needed to do it for sysfs >> weren't quite there. My long term goal is to get that situation improved >> so we can avoid adding anymore ioctl interfaces and eventually allow for >> distros to build mdadm with ioctl support disabled. We had a discussion >> at LSF/MM in Boston about this (Hannes, Shaohua, Song, and myself). >> >> Reading the code I found it confusing that it was so tied to the patch >> level, but didn't do anything with the version numbers. At least >> intuitively if I bumped the version number, I would reset the patchlevel >> which would break things. >> >> I think it's fair to draw a line in the sand and say that mdadm-4.1+ >> will not support kernels older than 2.6.15. I am open to the kernel >> version we pick here, but I would like to start deprecating some of the >> really old code. I have patches that does this in my tree, but I need to >> add a check for kernel version > 2.6.15. I am not aware what SuSE's >> enterprise kernel versions look like, but checking RHEL/CentOS RHEL5 was >> 2.6.18, while RHEL4 was 2.6.9 - and RHEL4 has been unsupported for quite >> a while. At least for RHEL/CentOS 2.6.15 as the line in the sand seems >> fine. >> >> For the kernel to expose features to userland in the future, I would >> prefer to go with a feature-flag style interface exposed via sysfs. That >> way a distro could enable one feature, but not the other in their kernel >> without having to worry about actual version numbers. > > BTW, I'd like to take on the kernel side stuffs. > IMHO, it would be better that we also set a timeline, after the > timeline, we should add new interface via configfs, and keep all legancy > ioctl implementation before this timeline. > > Coly A volunteer! A volunteer! I don't think we need to set a firm timeline for removing code from the kernel at this point. What we can do is implement the new interfaces via sysfs/configfs, and then make ioctl support a config option. Down the line it will be evident if/when we can rip out the old code, but I see that being years away. Cheers, Jes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-06 18:14 ` Jes Sorensen @ 2017-04-07 3:54 ` Coly Li 2017-04-07 14:54 ` Jes Sorensen 0 siblings, 1 reply; 13+ messages in thread From: Coly Li @ 2017-04-07 3:54 UTC (permalink / raw) To: Jes Sorensen, NeilBrown; +Cc: linux-raid, Hannes Reinecke, kernel-team On 2017/4/7 上午2:14, Jes Sorensen wrote: > On 04/06/2017 02:10 PM, Coly Li wrote: >> On 2017/4/6 下午11:31, Jes Sorensen wrote: >>> Neil, >>> >>> I see, thanks for explaining. >>> >>> The goal is to eventually get out of the ioctl() business and get to a >>> state where we can do everything via sysfs/configfs. Right now we have a >>> big mix between ioctl and sysfs where neither interface does everything. >>> The recent issues with PPL (I think it was) showed that we had to add >>> more ioctl support because the interfaces needed to do it for sysfs >>> weren't quite there. My long term goal is to get that situation improved >>> so we can avoid adding anymore ioctl interfaces and eventually allow for >>> distros to build mdadm with ioctl support disabled. We had a discussion >>> at LSF/MM in Boston about this (Hannes, Shaohua, Song, and myself). >>> >>> Reading the code I found it confusing that it was so tied to the patch >>> level, but didn't do anything with the version numbers. At least >>> intuitively if I bumped the version number, I would reset the patchlevel >>> which would break things. >>> >>> I think it's fair to draw a line in the sand and say that mdadm-4.1+ >>> will not support kernels older than 2.6.15. I am open to the kernel >>> version we pick here, but I would like to start deprecating some of the >>> really old code. I have patches that does this in my tree, but I need to >>> add a check for kernel version > 2.6.15. I am not aware what SuSE's >>> enterprise kernel versions look like, but checking RHEL/CentOS RHEL5 was >>> 2.6.18, while RHEL4 was 2.6.9 - and RHEL4 has been unsupported for quite >>> a while. At least for RHEL/CentOS 2.6.15 as the line in the sand seems >>> fine. >>> >>> For the kernel to expose features to userland in the future, I would >>> prefer to go with a feature-flag style interface exposed via sysfs. That >>> way a distro could enable one feature, but not the other in their kernel >>> without having to worry about actual version numbers. >> >> BTW, I'd like to take on the kernel side stuffs. >> IMHO, it would be better that we also set a timeline, after the >> timeline, we should add new interface via configfs, and keep all legancy >> ioctl implementation before this timeline. >> >> Coly > > A volunteer! A volunteer! > > I don't think we need to set a firm timeline for removing code from the > kernel at this point. What we can do is implement the new interfaces via > sysfs/configfs, and then make ioctl support a config option. Down the > line it will be evident if/when we can rip out the old code, but I see > that being years away. Not removing code, just after a timeline, new patch which adding new interface should go into configfs. I start to look into this now, hope to compose a first version patch set for comments in not too long time. Coly ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-07 3:54 ` Coly Li @ 2017-04-07 14:54 ` Jes Sorensen 0 siblings, 0 replies; 13+ messages in thread From: Jes Sorensen @ 2017-04-07 14:54 UTC (permalink / raw) To: Coly Li, NeilBrown; +Cc: linux-raid, Hannes Reinecke, kernel-team On 04/06/2017 11:54 PM, Coly Li wrote: > On 2017/4/7 上午2:14, Jes Sorensen wrote: >> A volunteer! A volunteer! >> >> I don't think we need to set a firm timeline for removing code from the >> kernel at this point. What we can do is implement the new interfaces via >> sysfs/configfs, and then make ioctl support a config option. Down the >> line it will be evident if/when we can rip out the old code, but I see >> that being years away. > > Not removing code, just after a timeline, new patch which adding new > interface should go into configfs. > I start to look into this now, hope to compose a first version patch set > for comments in not too long time. > > Coly Sounds good - make sure to also consult with Shaohua how it likes to see it implemented on the kernel side. Cheers, Jes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-06 15:31 ` Jes Sorensen 2017-04-06 18:10 ` Coly Li @ 2017-04-06 22:44 ` NeilBrown 2017-04-07 15:55 ` jes.sorensen 1 sibling, 1 reply; 13+ messages in thread From: NeilBrown @ 2017-04-06 22:44 UTC (permalink / raw) To: Jes Sorensen, NeilBrown; +Cc: linux-raid, Hannes Reinecke, kernel-team [-- Attachment #1: Type: text/plain, Size: 6619 bytes --] On Thu, Apr 06 2017, Jes Sorensen wrote: > On 04/05/2017 06:32 PM, NeilBrown wrote: >> On Thu, Apr 06 2017, jes.sorensen@gmail.com wrote: >> >>> jes.sorensen@gmail.com writes: >>>> Hi Neil, >>>> >>>> Looking through the code in mdadm, I noticed a number of cases calling >>>> ioctl(RAID_VERSION). At first I had it confused with metadata version, >>>> but it looks like RAID_VERSION will always return 90000 if it's a valid >>>> raid device. >>>> >>>> In the cases we want to confirm the fd is a valid raid array, >>>> ioctl(GET_ARRAY_INFO) should do, or sysfs_read(GET_VERSION). >>>> >>>> Am I missing something obvious here, or do you see any reason for >>>> leaving this around? >>> >>> Sorry the above is wrong, it will always return 900, not 90000. Some of >>> the code that stood out is in util.c: >>> >>> int md_get_version(int fd) >>> { >>> struct stat stb; >>> mdu_version_t vers; >>> >>> if (fstat(fd, &stb)<0) >>> return -1; >>> if ((S_IFMT&stb.st_mode) != S_IFBLK) >>> return -1; >>> >>> if (ioctl(fd, RAID_VERSION, &vers) == 0) >>> return (vers.major*10000) + (vers.minor*100) + vers.patchlevel; >>> if (errno == EACCES) >>> return -1; >>> if (major(stb.st_rdev) == MD_MAJOR) >>> return (3600); >>> return -1; >>> } >>> >>> .... >>> >>> int set_array_info(int mdfd, struct supertype *st, struct mdinfo *info) >>> { >>> /* Initialise kernel's knowledge of array. >>> * This varies between externally managed arrays >>> * and older kernels >>> */ >>> int vers = md_get_version(mdfd); >>> int rv; >>> >>> #ifndef MDASSEMBLE >>> if (st->ss->external) >>> rv = sysfs_set_array(info, vers); >>> else >>> #endif >>> if ((vers % 100) >= 1) { /* can use different versions */ >>> mdu_array_info_t inf; >>> memset(&inf, 0, sizeof(inf)); >>> inf.major_version = info->array.major_version; >>> inf.minor_version = info->array.minor_version; >>> rv = ioctl(mdfd, SET_ARRAY_INFO, &inf); >>> } else >>> rv = ioctl(mdfd, SET_ARRAY_INFO, NULL); >>> return rv; >>> } >>> >>> This has been around since at least 2008, the current code came in >>> f35f25259279573c6274e2783536c0b0a399bdd4, but it looks like even the >>> prior code made the same assumptions. >>> >>> In either case, the above 'if ((vers % 100) >= 1)' will always trigger >>> since the kernel does #define MD_PATCHLEVEL_VERSION 3 >>> >>> It's not like we have been updating MD_PATCHLEVEL_VERSION for a >>> while. Was the code meant to be looking at the superblock minor version? >>> I've been staring at this for a while now, so please beat me over the >>> head if I missed something blatantly obvious. >>> >>> Jes >> >> It is hard to get versioning right... >> >> The version returned by the RAID_VERSION ioctl is meant to reflect the >> capabilities of the implementation. We could use the kernel version >> number for that (and sometimes do), but as distro's often backport >> features, that isn't always reliable. >> >> I've incremented the MD_PATCHLEVEL_VERSION when a change is made that >> cannot easily be detected from user-space. As you note, we are up to >> three. The last change was in 2.6.15. >> I've never contemplated changing the other two numbers that RAID_VERSION >> return. They don't seem to mean anything useful. >> >> What exactly do you mean by "deprecate" the ioctl? >> If you remove the code in mdadm that calls it, mdadm will not work >> correctly on kernels older than 2.6.15, and it will be harder to >> and an future capability that is not easily visible from user space. >> If you remove the code in the kernel that handles it, you'll break >> mdadm. > > Neil, > > I see, thanks for explaining. > > The goal is to eventually get out of the ioctl() business and get to a > state where we can do everything via sysfs/configfs. Right now we have a > big mix between ioctl and sysfs where neither interface does everything. > The recent issues with PPL (I think it was) showed that we had to add > more ioctl support because the interfaces needed to do it for sysfs > weren't quite there. My long term goal is to get that situation improved > so we can avoid adding anymore ioctl interfaces and eventually allow for > distros to build mdadm with ioctl support disabled. We had a discussion > at LSF/MM in Boston about this (Hannes, Shaohua, Song, and myself). Sounds like a good goal, if approached cautiously (and it does sound like you are showing proper caution). And I don't think we need the "/configfs" bit. Configfs is just for people who don't understand sysfs ;-) > > Reading the code I found it confusing that it was so tied to the patch > level, but didn't do anything with the version numbers. At least > intuitively if I bumped the version number, I would reset the patchlevel > which would break things. I assumed the version number would never change, because it didn't mean anything useful. > > I think it's fair to draw a line in the sand and say that mdadm-4.1+ > will not support kernels older than 2.6.15. I am open to the kernel > version we pick here, but I would like to start deprecating some of the > really old code. I have patches that does this in my tree, but I need to > add a check for kernel version > 2.6.15. I am not aware what SuSE's > enterprise kernel versions look like, but checking RHEL/CentOS RHEL5 was > 2.6.18, while RHEL4 was 2.6.9 - and RHEL4 has been unsupported for quite > a while. At least for RHEL/CentOS 2.6.15 as the line in the sand seems fine. With my SUSE hat on, I'm happy for new mdadm to not support kernels older than 3.0. Probably even 3.12. > > For the kernel to expose features to userland in the future, I would > prefer to go with a feature-flag style interface exposed via sysfs. That > way a distro could enable one feature, but not the other in their kernel > without having to worry about actual version numbers. I think there are usually better ways than feature flags. If the new feature requires a new file in sysfs, then the existence of the file signals the presence of the feature. I'm happy with a plan to freeze the patchlevel at its current value and use other mechanisms going forward. Thanks, NeilBrown [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-06 22:44 ` NeilBrown @ 2017-04-07 15:55 ` jes.sorensen 2017-04-09 23:01 ` NeilBrown 0 siblings, 1 reply; 13+ messages in thread From: jes.sorensen @ 2017-04-07 15:55 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid, Hannes Reinecke, kernel-team NeilBrown <neilb@suse.com> writes: > On Thu, Apr 06 2017, Jes Sorensen wrote: >> Neil, >> >> I see, thanks for explaining. >> >> The goal is to eventually get out of the ioctl() business and get to a >> state where we can do everything via sysfs/configfs. Right now we have a >> big mix between ioctl and sysfs where neither interface does everything. >> The recent issues with PPL (I think it was) showed that we had to add >> more ioctl support because the interfaces needed to do it for sysfs >> weren't quite there. My long term goal is to get that situation improved >> so we can avoid adding anymore ioctl interfaces and eventually allow for >> distros to build mdadm with ioctl support disabled. We had a discussion >> at LSF/MM in Boston about this (Hannes, Shaohua, Song, and myself). > > Sounds like a good goal, if approached cautiously (and it does sound > like you are showing proper caution). > And I don't think we need the "/configfs" bit. Configfs is just for > people who don't understand sysfs ;-) Glad to hear we're in agreement on this. I definitely want to be cautious about this. While change is good, we have a responsibility towards existing users. This isn't a desktop environment after all :) I am not really tied to sysfs/configfs on this, whoever does that part of the work gets to show us what he/she works best and explain why. >> I think it's fair to draw a line in the sand and say that mdadm-4.1+ >> will not support kernels older than 2.6.15. I am open to the kernel >> version we pick here, but I would like to start deprecating some of the >> really old code. I have patches that does this in my tree, but I need to >> add a check for kernel version > 2.6.15. I am not aware what SuSE's >> enterprise kernel versions look like, but checking RHEL/CentOS RHEL5 was >> 2.6.18, while RHEL4 was 2.6.9 - and RHEL4 has been unsupported for quite >> a while. At least for RHEL/CentOS 2.6.15 as the line in the sand seems fine. > > With my SUSE hat on, I'm happy for new mdadm to not support kernels > older than 3.0. Probably even 3.12. I just pushed the first set of changes into git for this. We no longer support kernels older than 2.6.15. If it breaks something else, I am ready to take public blame! If it ends up biting another distro for a slightly older kernel, we can look at fixing that up. >> For the kernel to expose features to userland in the future, I would >> prefer to go with a feature-flag style interface exposed via sysfs. That >> way a distro could enable one feature, but not the other in their kernel >> without having to worry about actual version numbers. > > I think there are usually better ways than feature flags. > If the new feature requires a new file in sysfs, then the existence of > the file signals the presence of the feature. That is pretty much how I see feature flags. Next question since I am wearing my 'what is this old stuff doing' hat. mdassemble? Does anything still use this? The reason is a lot of the newer features are explicitly included, and switching to sysfs is effectively going to kill it, unless it gets a major upgrade. Cheers, Jes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-07 15:55 ` jes.sorensen @ 2017-04-09 23:01 ` NeilBrown 2017-04-10 9:26 ` Nix 2017-04-11 14:46 ` Jes Sorensen 0 siblings, 2 replies; 13+ messages in thread From: NeilBrown @ 2017-04-09 23:01 UTC (permalink / raw) To: jes.sorensen; +Cc: linux-raid, Hannes Reinecke, kernel-team [-- Attachment #1: Type: text/plain, Size: 583 bytes --] On Fri, Apr 07 2017, jes.sorensen@gmail.com wrote: > > Next question since I am wearing my 'what is this old stuff doing' hat. > mdassemble? Does anything still use this? The reason is a lot of the > newer features are explicitly included, and switching to sysfs is > effectively going to kill it, unless it gets a major upgrade. > I was never a big fan, of mdassemble, but it is smaller than mdadm and some people apparently have (or had) space-constrained boot environments. Maybe post to the linux-raid list with a subject "mdassemble is going way, do you care?". ?? NeilBrown [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-09 23:01 ` NeilBrown @ 2017-04-10 9:26 ` Nix 2017-04-11 14:46 ` Jes Sorensen 1 sibling, 0 replies; 13+ messages in thread From: Nix @ 2017-04-10 9:26 UTC (permalink / raw) To: NeilBrown; +Cc: jes.sorensen, linux-raid, Hannes Reinecke, kernel-team On 10 Apr 2017, NeilBrown verbalised: > On Fri, Apr 07 2017, jes.sorensen@gmail.com wrote: > >> Next question since I am wearing my 'what is this old stuff doing' hat. >> mdassemble? Does anything still use this? The reason is a lot of the >> newer features are explicitly included, and switching to sysfs is >> effectively going to kill it, unless it gets a major upgrade. >> > > I was never a big fan, of mdassemble, but it is smaller than mdadm and > some people apparently have (or had) space-constrained boot > environments. It also has fewer build-time requirements and can build on systems with things like old buggy versions of uclibc that can't build a working mdadm. (Of course, now musl exists, I'm not sure anyone should care about that. I stopped caring many years ago.) -- NULL && (void) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Can we deprecate ioctl(RAID_VERSION)? 2017-04-09 23:01 ` NeilBrown 2017-04-10 9:26 ` Nix @ 2017-04-11 14:46 ` Jes Sorensen 1 sibling, 0 replies; 13+ messages in thread From: Jes Sorensen @ 2017-04-11 14:46 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid, Hannes Reinecke, kernel-team On 04/09/2017 07:01 PM, NeilBrown wrote: > On Fri, Apr 07 2017, jes.sorensen@gmail.com wrote: > >> >> Next question since I am wearing my 'what is this old stuff doing' hat. >> mdassemble? Does anything still use this? The reason is a lot of the >> newer features are explicitly included, and switching to sysfs is >> effectively going to kill it, unless it gets a major upgrade. >> > > I was never a big fan, of mdassemble, but it is smaller than mdadm and > some people apparently have (or had) space-constrained boot > environments. > Maybe post to the linux-raid list with a subject "mdassemble is going > way, do you care?". ?? Makes sense - looking at the code it wasn't clear if it is even usable in modern times since it ignores sysfs access. I'll post another message and get out the big hammer! Cheers, Jes ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-04-11 14:46 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-05 17:55 Can we deprecate ioctl(RAID_VERSION)? jes.sorensen 2017-04-05 19:02 ` jes.sorensen 2017-04-05 22:32 ` NeilBrown 2017-04-06 15:31 ` Jes Sorensen 2017-04-06 18:10 ` Coly Li 2017-04-06 18:14 ` Jes Sorensen 2017-04-07 3:54 ` Coly Li 2017-04-07 14:54 ` Jes Sorensen 2017-04-06 22:44 ` NeilBrown 2017-04-07 15:55 ` jes.sorensen 2017-04-09 23:01 ` NeilBrown 2017-04-10 9:26 ` Nix 2017-04-11 14:46 ` Jes Sorensen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.