| Message ID | 20120110174118.GB3015@zod.bos.redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 1/10/12 11:41 AM, Josh Boyer wrote: > The ext2/ext3 filesystems supported "check=none" and "nocheck" as mount options > even though that was already the default behavior and it essentially did > nothing. When using ext4 to mount ext2/ext3 filesystems, that mount option > causes the mount to fail. That isn't as backward compatible as it could be, > so add support to ext4 to accept the option. The only thing ext2 does with those options today is to clear the flag, and I can't find anything in userspace or kernelspace which would have set it in any case. It seem dead, but we do need the compatibility in ext4 if it's to handle ext2&3 seamlessly, I guess. > Signed-off-by: Josh Boyer <jwboyer@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> > > --- > fs/ext4/super.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 3e1329e..5ff09e7 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -1333,7 +1333,7 @@ enum { > Opt_nomblk_io_submit, Opt_block_validity, Opt_noblock_validity, > Opt_inode_readahead_blks, Opt_journal_ioprio, > Opt_dioread_nolock, Opt_dioread_lock, > - Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, > + Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, Opt_nocheck, > }; > > static const match_table_t tokens = { > @@ -1409,6 +1409,8 @@ static const match_table_t tokens = { > {Opt_init_itable, "init_itable=%u"}, > {Opt_init_itable, "init_itable"}, > {Opt_noinit_itable, "noinit_itable"}, > + {Opt_nocheck, "check=none"}, > + {Opt_nocheck, "nocheck"}, > {Opt_err, NULL}, > }; > > @@ -1905,6 +1907,9 @@ set_qf_format: > case Opt_noinit_itable: > clear_opt(sb, INIT_INODE_TABLE); > break; > + case Opt_nocheck: > + /* ext2/ext3 used to "support" this option. Silently eat it */ > + break; > default: > ext4_msg(sb, KERN_ERR, > "Unrecognized mount option \"%s\" " -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2012-01-10, at 10:43 AM, Eric Sandeen wrote: > On 1/10/12 11:41 AM, Josh Boyer wrote: >> The ext2/ext3 filesystems supported "check=none" and "nocheck" as mount options >> even though that was already the default behavior and it essentially did >> nothing. When using ext4 to mount ext2/ext3 filesystems, that mount option >> causes the mount to fail. That isn't as backward compatible as it could be, >> so add support to ext4 to accept the option. > > The only thing ext2 does with those options today is to clear the flag, > and I can't find anything in userspace or kernelspace which would have set > it in any case. It seem dead, but we do need the compatibility in ext4 > if it's to handle ext2&3 seamlessly, I guess. At a minimum the use of obsolete options like this should print a warning at mount time so that there is some chance the user will notice and remove the old option from their config. Otherwise it is impossible to even get rid of old useless cruft like this if we need to maintain functionally-useless compatibility forever. Dredging through my memories, I recall that this option used to disable the checks done at mount(?) time that compared the bits set in the block and inode bitmaps against the summary values in the group descriptors (AFAIR). Or maybe it was comparing the group descriptor summary values against the superblock values? In any case, I can't imagine why a user would have this set for a kernel option that might have last been valid 10 years ago, and why the 5 users in the world that might have this set cannot simply remove it from their fstab, since it does absolutely nothing? >> --- >> fs/ext4/super.c | 7 ++++++- >> 1 files changed, 6 insertions(+), 1 deletions(-) >> >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >> index 3e1329e..5ff09e7 100644 >> --- a/fs/ext4/super.c >> +++ b/fs/ext4/super.c >> @@ -1333,7 +1333,7 @@ enum { >> Opt_nomblk_io_submit, Opt_block_validity, Opt_noblock_validity, >> Opt_inode_readahead_blks, Opt_journal_ioprio, >> Opt_dioread_nolock, Opt_dioread_lock, >> - Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, >> + Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, Opt_nocheck, >> }; >> >> static const match_table_t tokens = { >> @@ -1409,6 +1409,8 @@ static const match_table_t tokens = { >> {Opt_init_itable, "init_itable=%u"}, >> {Opt_init_itable, "init_itable"}, >> {Opt_noinit_itable, "noinit_itable"}, >> + {Opt_nocheck, "check=none"}, >> + {Opt_nocheck, "nocheck"}, >> {Opt_err, NULL}, >> }; >> >> @@ -1905,6 +1907,9 @@ set_qf_format: >> case Opt_noinit_itable: >> clear_opt(sb, INIT_INODE_TABLE); >> break; >> + case Opt_nocheck: >> + /* ext2/ext3 used to "support" this option. Silently eat it */ >> + break; >> default: >> ext4_msg(sb, KERN_ERR, >> "Unrecognized mount option \"%s\" " > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 11, 2012 at 03:07:33AM -0700, Andreas Dilger wrote: > On 2012-01-10, at 10:43 AM, Eric Sandeen wrote: > > On 1/10/12 11:41 AM, Josh Boyer wrote: > >> The ext2/ext3 filesystems supported "check=none" and "nocheck" as mount options > >> even though that was already the default behavior and it essentially did > >> nothing. When using ext4 to mount ext2/ext3 filesystems, that mount option > >> causes the mount to fail. That isn't as backward compatible as it could be, > >> so add support to ext4 to accept the option. > > > > The only thing ext2 does with those options today is to clear the flag, > > and I can't find anything in userspace or kernelspace which would have set > > it in any case. It seem dead, but we do need the compatibility in ext4 > > if it's to handle ext2&3 seamlessly, I guess. > > At a minimum the use of obsolete options like this should print a warning > at mount time so that there is some chance the user will notice and remove > the old option from their config. Otherwise it is impossible to even get > rid of old useless cruft like this if we need to maintain functionally-useless > compatibility forever. I can add a printk(KERN_WARN in the case statement instead of just having it break. That's not a big deal, but I doubt it's going to get seen much. I'm very hesitant to add a WARN_ON or anything else that is going to generate a backtrace because it's just going to be scary looking and cause more work for distributions that have automated bug reporting tools. > Dredging through my memories, I recall that this option used to disable > the checks done at mount(?) time that compared the bits set in the block > and inode bitmaps against the summary values in the group descriptors > (AFAIR). Or maybe it was comparing the group descriptor summary values > against the superblock values? Yeah, something like that. It's been the default behavior of ext2/ext3 for a long time now. So long that the "check=normal" and "check=strict" options (non-default) aren't even supported there anymore. > In any case, I can't imagine why a user would have this set for a kernel > option that might have last been valid 10 years ago, and why the 5 users > in the world that might have this set cannot simply remove it from their > fstab, since it does absolutely nothing? This was prompted by a bug report against util-linux, as the man page still had the option as valid for ext2 (which it is). Since ext4 is claiming to be directly usable for ext2 filesystems, I don't think it is unreasonable to mount an fs that has that option set. If it makes things easier I could wrap all of this in CONFIG_EXT4_USE_FOR_EXT23 ifdefs, but that seemed to make the code unnecessarily uglier. josh -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/11/12 4:07 AM, Andreas Dilger wrote: > On 2012-01-10, at 10:43 AM, Eric Sandeen wrote: >> On 1/10/12 11:41 AM, Josh Boyer wrote: >>> The ext2/ext3 filesystems supported "check=none" and "nocheck" as mount options >>> even though that was already the default behavior and it essentially did >>> nothing. When using ext4 to mount ext2/ext3 filesystems, that mount option >>> causes the mount to fail. That isn't as backward compatible as it could be, >>> so add support to ext4 to accept the option. >> >> The only thing ext2 does with those options today is to clear the flag, >> and I can't find anything in userspace or kernelspace which would have set >> it in any case. It seem dead, but we do need the compatibility in ext4 >> if it's to handle ext2&3 seamlessly, I guess. > > At a minimum the use of obsolete options like this should print a warning > at mount time so that there is some chance the user will notice and remove > the old option from their config. Otherwise it is impossible to even get > rid of old useless cruft like this if we need to maintain functionally-useless > compatibility forever. > > Dredging through my memories, I recall that this option used to disable > the checks done at mount(?) time that compared the bits set in the block > and inode bitmaps against the summary values in the group descriptors > (AFAIR). Or maybe it was comparing the group descriptor summary values > against the superblock values? > > In any case, I can't imagine why a user would have this set for a kernel > option that might have last been valid 10 years ago, and why the 5 users > in the world that might have this set cannot simply remove it from their > fstab, since it does absolutely nothing? Well, I agree that it should be deprecated, but that's a separate issue from making ext4 handle all current ext2/3 mount options. Let's just push this one in, and can do another patch to add the dire deprecation warnings to all filesystems, ok? Then in 5 more years we can remember to remove it ;) -Eric >>> --- >>> fs/ext4/super.c | 7 ++++++- >>> 1 files changed, 6 insertions(+), 1 deletions(-) >>> >>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >>> index 3e1329e..5ff09e7 100644 >>> --- a/fs/ext4/super.c >>> +++ b/fs/ext4/super.c >>> @@ -1333,7 +1333,7 @@ enum { >>> Opt_nomblk_io_submit, Opt_block_validity, Opt_noblock_validity, >>> Opt_inode_readahead_blks, Opt_journal_ioprio, >>> Opt_dioread_nolock, Opt_dioread_lock, >>> - Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, >>> + Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, Opt_nocheck, >>> }; >>> >>> static const match_table_t tokens = { >>> @@ -1409,6 +1409,8 @@ static const match_table_t tokens = { >>> {Opt_init_itable, "init_itable=%u"}, >>> {Opt_init_itable, "init_itable"}, >>> {Opt_noinit_itable, "noinit_itable"}, >>> + {Opt_nocheck, "check=none"}, >>> + {Opt_nocheck, "nocheck"}, >>> {Opt_err, NULL}, >>> }; >>> >>> @@ -1905,6 +1907,9 @@ set_qf_format: >>> case Opt_noinit_itable: >>> clear_opt(sb, INIT_INODE_TABLE); >>> break; >>> + case Opt_nocheck: >>> + /* ext2/ext3 used to "support" this option. Silently eat it */ >>> + break; >>> default: >>> ext4_msg(sb, KERN_ERR, >>> "Unrecognized mount option \"%s\" " >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > Cheers, Andreas > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 11, 2012 at 09:13:40AM -0600, Eric Sandeen wrote: > > In any case, I can't imagine why a user would have this set for a kernel > > option that might have last been valid 10 years ago, and why the 5 users > > in the world that might have this set cannot simply remove it from their > > fstab, since it does absolutely nothing? > > Well, I agree that it should be deprecated, but that's a separate issue from > making ext4 handle all current ext2/3 mount options. > > Let's just push this one in, and can do another patch to add the dire > deprecation warnings to all filesystems, ok? Then in 5 more years we can > remember to remove it ;) If we're going to deprecate it, we should deprecate it for *all* of the ext 2/3/4 file systems. Otherwise let's not bother, and keep the no-op for backwards compatibility. If we are going to support ext4 taking over for ext2/3, then yes we need to support any common mount options, and if we want to deprecate a particular option in ext4, we should try to deprecate them in ext2/3 as well. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/11/12 9:26 AM, Ted Ts'o wrote: > On Wed, Jan 11, 2012 at 09:13:40AM -0600, Eric Sandeen wrote: >>> In any case, I can't imagine why a user would have this set for a kernel >>> option that might have last been valid 10 years ago, and why the 5 users >>> in the world that might have this set cannot simply remove it from their >>> fstab, since it does absolutely nothing? >> >> Well, I agree that it should be deprecated, but that's a separate issue from >> making ext4 handle all current ext2/3 mount options. >> >> Let's just push this one in, and can do another patch to add the dire >> deprecation warnings to all filesystems, ok? Then in 5 more years we can >> remember to remove it ;) > > If we're going to deprecate it, we should deprecate it for *all* of > the ext 2/3/4 file systems. Otherwise let's not bother, and keep the > no-op for backwards compatibility. If we are going to support ext4 > taking over for ext2/3, then yes we need to support any common mount > options, and if we want to deprecate a particular option in ext4, we > should try to deprecate them in ext2/3 as well. I agree, I'm just saying it can be 2 separate steps; fixing ext4 doesn't need to wait for deprecating the option on ext2/3. -Eric > - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2012-01-11, at 7:50 AM, Josh Boyer wrote: > On Wed, Jan 11, 2012 at 03:07:33AM -0700, Andreas Dilger wrote: >> At a minimum the use of obsolete options like this should print a warning >> at mount time so that there is some chance the user will notice and remove >> the old option from their config. Otherwise it is impossible to even get >> rid of old useless cruft like this if we need to maintain functionally-useless >> compatibility forever. > > I can add a printk(KERN_WARN in the case statement instead of just > having it break. That's not a big deal, but I doubt it's going to get > seen much. It not being seen much/ever is fine (expected even), but at least anyone using this option has some advance warning that it will disappear. As stated separately, a printk() should also be added to ext2/ext3 so that it is deprecated there as well. > I'm very hesitant to add a WARN_ON or anything else that is > going to generate a backtrace because it's just going to be scary > looking and cause more work for distributions that have automated bug > reporting tools. Right, WARN_ON() is too strong, and having a stack trace isn't useful. >> Dredging through my memories, I recall that this option used to disable >> the checks done at mount(?) time that compared the bits set in the block >> and inode bitmaps against the summary values in the group descriptors >> (AFAIR). Or maybe it was comparing the group descriptor summary values >> against the superblock values? > > Yeah, something like that. It's been the default behavior of ext2/ext3 > for a long time now. So long that the "check=normal" and "check=strict" > options (non-default) aren't even supported there anymore. Not surprising. >> In any case, I can't imagine why a user would have this set for a kernel >> option that might have last been valid 10 years ago, and why the 5 users >> in the world that might have this set cannot simply remove it from their >> fstab, since it does absolutely nothing? > > This was prompted by a bug report against util-linux, as the man page > still had the option as valid for ext2 (which it is). Is there a patch being submitted to util-linux that removes this option from the man page as well? > Since ext4 is > claiming to be directly usable for ext2 filesystems, I don't think it is > unreasonable to mount an fs that has that option set. If it makes > things easier I could wrap all of this in CONFIG_EXT4_USE_FOR_EXT23 > ifdefs, but that seemed to make the code unnecessarily uglier. No, I don't think a CONFIG option is needed. What I'd actually prefer is to add a LINUX_VERSION_CODE check for some time in the future (say 3 years with 4 kernel versions per year) that adds a #warning to remove this code: #if LINUX_VERSION_CODE < KERNEL_VERSION(3, 15, 50) /* approx 2014 */ case Opt_nocheck: /* ext2/ext3 used to "support" this option. */ ext4_msg(sb, KERN_INFO, "the '%s' option is deprecated, " "please remove it from /etc/fstab\n", p); break; #else #warning "remove obsolete 'nocheck' and 'check=none' mount options" #endif This at least ensures that the obsolete code is noticed (which is what we do for Lustre), but it puts the burden of removing the obsolete code onto the person who updates the version (which is Linus, and I don't think he would be happy about it). IMHO existing Linux "deprecated" mechanism does not work very well, because there is still tons of deprecated code that is never removed because it isn't "in your face" enough (i.e. stuff that is marked to be deprecated in 2007 or 2008. Another option would be to just have the version check without the #warning and then at least the code will be automatically disabled at that time, and can be deleted by one of the ext4 maintainers at any convenient later time. Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 3e1329e..5ff09e7 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1333,7 +1333,7 @@ enum { Opt_nomblk_io_submit, Opt_block_validity, Opt_noblock_validity, Opt_inode_readahead_blks, Opt_journal_ioprio, Opt_dioread_nolock, Opt_dioread_lock, - Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, + Opt_discard, Opt_nodiscard, Opt_init_itable, Opt_noinit_itable, Opt_nocheck, }; static const match_table_t tokens = { @@ -1409,6 +1409,8 @@ static const match_table_t tokens = { {Opt_init_itable, "init_itable=%u"}, {Opt_init_itable, "init_itable"}, {Opt_noinit_itable, "noinit_itable"}, + {Opt_nocheck, "check=none"}, + {Opt_nocheck, "nocheck"}, {Opt_err, NULL}, }; @@ -1905,6 +1907,9 @@ set_qf_format: case Opt_noinit_itable: clear_opt(sb, INIT_INODE_TABLE); break; + case Opt_nocheck: + /* ext2/ext3 used to "support" this option. Silently eat it */ + break; default: ext4_msg(sb, KERN_ERR, "Unrecognized mount option \"%s\" "
The ext2/ext3 filesystems supported "check=none" and "nocheck" as mount options even though that was already the default behavior and it essentially did nothing. When using ext4 to mount ext2/ext3 filesystems, that mount option causes the mount to fail. That isn't as backward compatible as it could be, so add support to ext4 to accept the option. Signed-off-by: Josh Boyer <jwboyer@redhat.com> --- fs/ext4/super.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)