[PATCH] ext4: Support "check=none" "nocheck" mount options

[PATCH] ext4: Support "check=none" "nocheck" mount options

[Josh Boyer]

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(-)

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\" "
-- 
1.7.7.5

Re: [PATCH] ext4: Support "check=none" "nocheck" mount options

[Eric Sandeen]

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\" "

Re: [PATCH] ext4: Support "check=none" "nocheck" mount options

[Andreas Dilger]

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

Re: [PATCH] ext4: Support "check=none" "nocheck" mount options

[Josh Boyer]

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

Re: [PATCH] ext4: Support "check=none" "nocheck" mount options

[Andreas Dilger]

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

Re: [PATCH] ext4: Support "check=none" "nocheck" mount options

[Eric Sandeen]

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
> 
> 
> 
> 
> 

Re: [PATCH] ext4: Support "check=none" "nocheck" mount options

[Ted Ts'o]

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

Re: [PATCH] ext4: Support "check=none" "nocheck" mount options

[Eric Sandeen]

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