2008-07-07 20:15:26

by Eric Sandeen

[permalink] [raw]
Subject: [PATCH] do not set extents feature from the kernel

We've talked for a while about getting rid of any feature-
setting from the kernel; this gets rid of the code which would
set the INCOMPAT_EXTENTS flag on the first file write when mounted
as ext4[dev].

With this patch, if the extents feature is not already set on disk,
then mounting as ext4 will fall back to noextents with a warning,
and if -o extents is explicitly requested, the mount will fail,
also with warning.

Does this seem like a decent approach?

Signed-off-by: Eric Sandeen <[email protected]>
---

Index: linux-2.6/fs/ext4/ialloc.c
===================================================================
--- linux-2.6.orig/fs/ext4/ialloc.c 2008-07-07 10:22:00.000000000 -0500
+++ linux-2.6/fs/ext4/ialloc.c 2008-07-07 12:07:02.133292077 -0500
@@ -740,14 +740,10 @@ got:
goto fail_free_drop;

if (test_opt(sb, EXTENTS)) {
- /* set extent flag only for diretory, file and normal symlink*/
+ /* set extent flag only for directory, file and normal symlink*/
if (S_ISDIR(mode) || S_ISREG(mode) || S_ISLNK(mode)) {
EXT4_I(inode)->i_flags |= EXT4_EXTENTS_FL;
ext4_ext_tree_init(handle, inode);
- err = ext4_update_incompat_feature(handle, sb,
- EXT4_FEATURE_INCOMPAT_EXTENTS);
- if (err)
- goto fail_free_drop;
}
}

Index: linux-2.6/fs/ext4/super.c
===================================================================
--- linux-2.6.orig/fs/ext4/super.c 2008-07-07 10:22:19.000000000 -0500
+++ linux-2.6/fs/ext4/super.c 2008-07-07 14:08:43.684291817 -0500
@@ -1309,6 +1309,13 @@ set_qf_format:
clear_opt(sbi->s_mount_opt, NOBH);
break;
case Opt_extents:
+ if (!EXT4_HAS_INCOMPAT_FEATURE(sb,
+ EXT4_FEATURE_INCOMPAT_EXTENTS)) {
+ ext4_warning(sb, __func__,
+ "extents feature not enabled "
+ "on this filesystem, use tune2fs\n");
+ return 0;
+ }
set_opt (sbi->s_mount_opt, EXTENTS);
break;
case Opt_noextents:
@@ -1919,12 +1926,18 @@ static int ext4_fill_super (struct super

/*
* turn on extents feature by default in ext4 filesystem
- * User -o noextents to turn it off
+ * only if feature flag already set by mkfs or tune2fs.
+ * Use -o noextents to turn it off
*/
- set_opt(sbi->s_mount_opt, EXTENTS);
+ if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_EXTENTS))
+ set_opt(sbi->s_mount_opt, EXTENTS);
+ else
+ ext4_warning(sb, __func__,
+ "extents feature not enabled on this filesystem, "
+ "use tune2fs.\n");
/*
- * turn on mballoc feature by default in ext4 filesystem
- * User -o nomballoc to turn it off
+ * turn on mballoc code by default in ext4 filesystem
+ * Use -o nomballoc to turn it off
*/
set_opt(sbi->s_mount_opt, MBALLOC);




2008-07-08 02:23:35

by Peng Tao

[permalink] [raw]
Subject: Re: [PATCH] do not set extents feature from the kernel

Hi, Eric

On Tue, Jul 8, 2008 at 4:15 AM, Eric Sandeen <[email protected]> wrote:
> We've talked for a while about getting rid of any feature-
> setting from the kernel; this gets rid of the code which would
> set the INCOMPAT_EXTENTS flag on the first file write when mounted
> as ext4[dev].
>
> With this patch, if the extents feature is not already set on disk,
> then mounting as ext4 will fall back to noextents with a warning,
> and if -o extents is explicitly requested, the mount will fail,
> also with warning.
Does this mean no more migration from ext3 to ext4?
>
> Does this seem like a decent approach?
>
> Signed-off-by: Eric Sandeen <[email protected]>
> ---
>
> Index: linux-2.6/fs/ext4/ialloc.c
> ===================================================================
> --- linux-2.6.orig/fs/ext4/ialloc.c 2008-07-07 10:22:00.000000000 -0500
> +++ linux-2.6/fs/ext4/ialloc.c 2008-07-07 12:07:02.133292077 -0500
> @@ -740,14 +740,10 @@ got:
> goto fail_free_drop;
>
> if (test_opt(sb, EXTENTS)) {
> - /* set extent flag only for diretory, file and normal symlink*/
> + /* set extent flag only for directory, file and normal symlink*/
> if (S_ISDIR(mode) || S_ISREG(mode) || S_ISLNK(mode)) {
> EXT4_I(inode)->i_flags |= EXT4_EXTENTS_FL;
> ext4_ext_tree_init(handle, inode);
> - err = ext4_update_incompat_feature(handle, sb,
> - EXT4_FEATURE_INCOMPAT_EXTENTS);
> - if (err)
> - goto fail_free_drop;
> }
> }
>
> Index: linux-2.6/fs/ext4/super.c
> ===================================================================
> --- linux-2.6.orig/fs/ext4/super.c 2008-07-07 10:22:19.000000000 -0500
> +++ linux-2.6/fs/ext4/super.c 2008-07-07 14:08:43.684291817 -0500
> @@ -1309,6 +1309,13 @@ set_qf_format:
> clear_opt(sbi->s_mount_opt, NOBH);
> break;
> case Opt_extents:
> + if (!EXT4_HAS_INCOMPAT_FEATURE(sb,
> + EXT4_FEATURE_INCOMPAT_EXTENTS)) {
> + ext4_warning(sb, __func__,
> + "extents feature not enabled "
> + "on this filesystem, use tune2fs\n");
> + return 0;
> + }
> set_opt (sbi->s_mount_opt, EXTENTS);
> break;
> case Opt_noextents:
> @@ -1919,12 +1926,18 @@ static int ext4_fill_super (struct super
>
> /*
> * turn on extents feature by default in ext4 filesystem
> - * User -o noextents to turn it off
> + * only if feature flag already set by mkfs or tune2fs.
> + * Use -o noextents to turn it off
> */
> - set_opt(sbi->s_mount_opt, EXTENTS);
> + if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_EXTENTS))
> + set_opt(sbi->s_mount_opt, EXTENTS);
> + else
> + ext4_warning(sb, __func__,
> + "extents feature not enabled on this filesystem, "
> + "use tune2fs.\n");
> /*
> - * turn on mballoc feature by default in ext4 filesystem
> - * User -o nomballoc to turn it off
> + * turn on mballoc code by default in ext4 filesystem
> + * Use -o nomballoc to turn it off
> */
> set_opt(sbi->s_mount_opt, MBALLOC);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>



--
RGDS

Bergwolf

................
Here lieth one whose name was writ on water.

2008-07-08 02:25:49

by Eric Sandeen

[permalink] [raw]
Subject: Re: [PATCH] do not set extents feature from the kernel

Peng tao wrote:
> Hi, Eric
>
> On Tue, Jul 8, 2008 at 4:15 AM, Eric Sandeen <[email protected]> wrote:
>> We've talked for a while about getting rid of any feature-
>> setting from the kernel; this gets rid of the code which would
>> set the INCOMPAT_EXTENTS flag on the first file write when mounted
>> as ext4[dev].
>>
>> With this patch, if the extents feature is not already set on disk,
>> then mounting as ext4 will fall back to noextents with a warning,
>> and if -o extents is explicitly requested, the mount will fail,
>> also with warning.
> Does this mean no more migration from ext3 to ext4?

I think it just means you need to tune2fs -O extents before you can
mount as ext4 *with* extents.

.... which is, I guess, a bit of a convoluted story, but consistent with
"don't surprise users by setting incompat flags behind their back"

-Eric

>> Does this seem like a decent approach?
>>
>> Signed-off-by: Eric Sandeen <[email protected]>
>> ---
>>
>> Index: linux-2.6/fs/ext4/ialloc.c
>> ===================================================================
>> --- linux-2.6.orig/fs/ext4/ialloc.c 2008-07-07 10:22:00.000000000 -0500
>> +++ linux-2.6/fs/ext4/ialloc.c 2008-07-07 12:07:02.133292077 -0500
>> @@ -740,14 +740,10 @@ got:
>> goto fail_free_drop;
>>
>> if (test_opt(sb, EXTENTS)) {
>> - /* set extent flag only for diretory, file and normal symlink*/
>> + /* set extent flag only for directory, file and normal symlink*/
>> if (S_ISDIR(mode) || S_ISREG(mode) || S_ISLNK(mode)) {
>> EXT4_I(inode)->i_flags |= EXT4_EXTENTS_FL;
>> ext4_ext_tree_init(handle, inode);
>> - err = ext4_update_incompat_feature(handle, sb,
>> - EXT4_FEATURE_INCOMPAT_EXTENTS);
>> - if (err)
>> - goto fail_free_drop;
>> }
>> }
>>
>> Index: linux-2.6/fs/ext4/super.c
>> ===================================================================
>> --- linux-2.6.orig/fs/ext4/super.c 2008-07-07 10:22:19.000000000 -0500
>> +++ linux-2.6/fs/ext4/super.c 2008-07-07 14:08:43.684291817 -0500
>> @@ -1309,6 +1309,13 @@ set_qf_format:
>> clear_opt(sbi->s_mount_opt, NOBH);
>> break;
>> case Opt_extents:
>> + if (!EXT4_HAS_INCOMPAT_FEATURE(sb,
>> + EXT4_FEATURE_INCOMPAT_EXTENTS)) {
>> + ext4_warning(sb, __func__,
>> + "extents feature not enabled "
>> + "on this filesystem, use tune2fs\n");
>> + return 0;
>> + }
>> set_opt (sbi->s_mount_opt, EXTENTS);
>> break;
>> case Opt_noextents:
>> @@ -1919,12 +1926,18 @@ static int ext4_fill_super (struct super
>>
>> /*
>> * turn on extents feature by default in ext4 filesystem
>> - * User -o noextents to turn it off
>> + * only if feature flag already set by mkfs or tune2fs.
>> + * Use -o noextents to turn it off
>> */
>> - set_opt(sbi->s_mount_opt, EXTENTS);
>> + if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_EXTENTS))
>> + set_opt(sbi->s_mount_opt, EXTENTS);
>> + else
>> + ext4_warning(sb, __func__,
>> + "extents feature not enabled on this filesystem, "
>> + "use tune2fs.\n");
>> /*
>> - * turn on mballoc feature by default in ext4 filesystem
>> - * User -o nomballoc to turn it off
>> + * turn on mballoc code by default in ext4 filesystem
>> + * Use -o nomballoc to turn it off
>> */
>> set_opt(sbi->s_mount_opt, MBALLOC);
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
>


2008-07-09 03:10:55

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH] do not set extents feature from the kernel

On Mon, Jul 07, 2008 at 03:15:16PM -0500, Eric Sandeen wrote:
> We've talked for a while about getting rid of any feature-
> setting from the kernel; this gets rid of the code which would
> set the INCOMPAT_EXTENTS flag on the first file write when mounted
> as ext4[dev].
>
> With this patch, if the extents feature is not already set on disk,
> then mounting as ext4 will fall back to noextents with a warning,
> and if -o extents is explicitly requested, the mount will fail,
> also with warning.
>

Looks good, I've added it to the patch queue.

- Ted