diff mbox

[v1,15/22] tune2fs: add inline_data feature in tune2fs

Message ID 1375436989-18948-16-git-send-email-wenqing.lz@taobao.com
State Superseded, archived
Headers show

Commit Message

Zheng Liu Aug. 2, 2013, 9:49 a.m. UTC 
From: Zheng Liu <wenqing.lz@taobao.com>

Inline_data feature can be set when ext_attr feature is enabled.  Now this
feature only can be enabled because we may be out of space when disabling
it.  If this feature is disabled, we need to allocate a block for every
file and directory, and it might exhaust all space.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
---
 misc/tune2fs.8.in |    5 +++++
 misc/tune2fs.c    |   17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

Comments

Darrick Wong Oct. 12, 2013, 12:39 a.m. UTC | #1 
On Fri, Aug 02, 2013 at 05:49:42PM +0800, Zheng Liu wrote:
> From: Zheng Liu <wenqing.lz@taobao.com>
> 
> Inline_data feature can be set when ext_attr feature is enabled.  Now this
> feature only can be enabled because we may be out of space when disabling
> it.  If this feature is disabled, we need to allocate a block for every
> file and directory, and it might exhaust all space.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> ---
>  misc/tune2fs.8.in |    5 +++++
>  misc/tune2fs.c    |   17 ++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in
> index 55c6dd9..b768fff 100644
> --- a/misc/tune2fs.8.in
> +++ b/misc/tune2fs.8.in
> @@ -531,6 +531,11 @@ Setting the filesystem feature is equivalent to using the
>  .B \-j
>  option.
>  .TP
> +.B inline_data
> +Allow data to be stored in inode.
> +.B Tune2fs
> +only supports setting this filesystem feature.
> +.TP
>  .B large_file
>  Filesystem can contain files that are greater than 2GB.  (Modern kernels
>  set this feature automatically when a file > 2GB is created.)
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index 7d6520e..f22a736 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -137,7 +137,8 @@ static __u32 ok_features[3] = {
>  	EXT2_FEATURE_INCOMPAT_FILETYPE |
>  		EXT3_FEATURE_INCOMPAT_EXTENTS |
>  		EXT4_FEATURE_INCOMPAT_FLEX_BG |
> -		EXT4_FEATURE_INCOMPAT_MMP,
> +		EXT4_FEATURE_INCOMPAT_MMP |
> +		EXT4_FEATURE_INCOMPAT_INLINE_DATA,
>  	/* R/O compat */
>  	EXT2_FEATURE_RO_COMPAT_LARGE_FILE |
>  		EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
> @@ -995,6 +996,20 @@ mmp_error:
>  		disable_uninit_bg(fs,
>  				EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
>  
> +	if (FEATURE_ON(E2P_FEATURE_INCOMPAT,
> +		       EXT4_FEATURE_INCOMPAT_INLINE_DATA)) {
> +		/* Inline_data feature cannot be enabled if ext_attr is
> +		 * disabled.
> +		 */

Multiline comment issue.

> +		if (!(fs->super->s_feature_compat &
> +		      EXT2_FEATURE_COMPAT_EXT_ATTR)) {

EXT2_HAS_COMPAT_FEATURE()?

> +			fputs(_("The inline_data feature cannot "
> +				"be set if ext_attr feature is disabled.\n"),
> +				stderr);
> +			return 1;
> +		}
> +	}
> +

Maybe I'm missing something, but I don't see anything prohibiting the user from
disabling inline_data?  Or from doing the conversion?

--D

>  	if (FEATURE_ON(E2P_FEATURE_RO_INCOMPAT,
>  				EXT4_FEATURE_RO_COMPAT_QUOTA)) {
>  		/*
> -- 
> 1.7.9.7
> 
> --
> 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
--
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
Zheng Liu Oct. 12, 2013, 8:16 a.m. UTC | #2 
On Fri, Oct 11, 2013 at 05:39:45PM -0700, Darrick J. Wong wrote:
> On Fri, Aug 02, 2013 at 05:49:42PM +0800, Zheng Liu wrote:
> > From: Zheng Liu <wenqing.lz@taobao.com>
> > 
> > Inline_data feature can be set when ext_attr feature is enabled.  Now this
> > feature only can be enabled because we may be out of space when disabling
> > it.  If this feature is disabled, we need to allocate a block for every
> > file and directory, and it might exhaust all space.
> > 
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> > ---
> >  misc/tune2fs.8.in |    5 +++++
> >  misc/tune2fs.c    |   17 ++++++++++++++++-
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in
> > index 55c6dd9..b768fff 100644
> > --- a/misc/tune2fs.8.in
> > +++ b/misc/tune2fs.8.in
> > @@ -531,6 +531,11 @@ Setting the filesystem feature is equivalent to using the
> >  .B \-j
> >  option.
> >  .TP
> > +.B inline_data
> > +Allow data to be stored in inode.
> > +.B Tune2fs
> > +only supports setting this filesystem feature.
> > +.TP
> >  .B large_file
> >  Filesystem can contain files that are greater than 2GB.  (Modern kernels
> >  set this feature automatically when a file > 2GB is created.)
> > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > index 7d6520e..f22a736 100644
> > --- a/misc/tune2fs.c
> > +++ b/misc/tune2fs.c
> > @@ -137,7 +137,8 @@ static __u32 ok_features[3] = {
> >  	EXT2_FEATURE_INCOMPAT_FILETYPE |
> >  		EXT3_FEATURE_INCOMPAT_EXTENTS |
> >  		EXT4_FEATURE_INCOMPAT_FLEX_BG |
> > -		EXT4_FEATURE_INCOMPAT_MMP,
> > +		EXT4_FEATURE_INCOMPAT_MMP |
> > +		EXT4_FEATURE_INCOMPAT_INLINE_DATA,
> >  	/* R/O compat */
> >  	EXT2_FEATURE_RO_COMPAT_LARGE_FILE |
> >  		EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
> > @@ -995,6 +996,20 @@ mmp_error:
> >  		disable_uninit_bg(fs,
> >  				EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
> >  
> > +	if (FEATURE_ON(E2P_FEATURE_INCOMPAT,
> > +		       EXT4_FEATURE_INCOMPAT_INLINE_DATA)) {
> > +		/* Inline_data feature cannot be enabled if ext_attr is
> > +		 * disabled.
> > +		 */
> 
> Multiline comment issue.
> 
> > +		if (!(fs->super->s_feature_compat &
> > +		      EXT2_FEATURE_COMPAT_EXT_ATTR)) {
> 
> EXT2_HAS_COMPAT_FEATURE()?
> 
> > +			fputs(_("The inline_data feature cannot "
> > +				"be set if ext_attr feature is disabled.\n"),
> > +				stderr);
> > +			return 1;
> > +		}
> > +	}
> > +
> 
> Maybe I'm missing something, but I don't see anything prohibiting the user from
> disabling inline_data?  Or from doing the conversion?

Yes, now in tune2fs we can only enable inline_data feature.  If we want
to disable inline_data, that means that we need to allocate a block for
every inode with inline data, and we could exhaust all disk space.
Another implementation might be just clear inline_data flag from
super_block and doesn't allocate a block for these inodes.  The former
obviously is not a good choice.  But the latter might be better.  I am
really not sure for this.  What do you think?

Thanks,
                                                - Zheng
--
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
Darrick Wong Oct. 12, 2013, 8:23 a.m. UTC | #3 
On Sat, Oct 12, 2013 at 04:16:38PM +0800, Zheng Liu wrote:
> On Fri, Oct 11, 2013 at 05:39:45PM -0700, Darrick J. Wong wrote:
> > On Fri, Aug 02, 2013 at 05:49:42PM +0800, Zheng Liu wrote:
> > > From: Zheng Liu <wenqing.lz@taobao.com>
> > > 
> > > Inline_data feature can be set when ext_attr feature is enabled.  Now this
> > > feature only can be enabled because we may be out of space when disabling
> > > it.  If this feature is disabled, we need to allocate a block for every
> > > file and directory, and it might exhaust all space.
> > > 
> > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> > > ---
> > >  misc/tune2fs.8.in |    5 +++++
> > >  misc/tune2fs.c    |   17 ++++++++++++++++-
> > >  2 files changed, 21 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in
> > > index 55c6dd9..b768fff 100644
> > > --- a/misc/tune2fs.8.in
> > > +++ b/misc/tune2fs.8.in
> > > @@ -531,6 +531,11 @@ Setting the filesystem feature is equivalent to using the
> > >  .B \-j
> > >  option.
> > >  .TP
> > > +.B inline_data
> > > +Allow data to be stored in inode.
> > > +.B Tune2fs
> > > +only supports setting this filesystem feature.
> > > +.TP
> > >  .B large_file
> > >  Filesystem can contain files that are greater than 2GB.  (Modern kernels
> > >  set this feature automatically when a file > 2GB is created.)
> > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > > index 7d6520e..f22a736 100644
> > > --- a/misc/tune2fs.c
> > > +++ b/misc/tune2fs.c
> > > @@ -137,7 +137,8 @@ static __u32 ok_features[3] = {
> > >  	EXT2_FEATURE_INCOMPAT_FILETYPE |
> > >  		EXT3_FEATURE_INCOMPAT_EXTENTS |
> > >  		EXT4_FEATURE_INCOMPAT_FLEX_BG |
> > > -		EXT4_FEATURE_INCOMPAT_MMP,
> > > +		EXT4_FEATURE_INCOMPAT_MMP |
> > > +		EXT4_FEATURE_INCOMPAT_INLINE_DATA,
> > >  	/* R/O compat */
> > >  	EXT2_FEATURE_RO_COMPAT_LARGE_FILE |
> > >  		EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
> > > @@ -995,6 +996,20 @@ mmp_error:
> > >  		disable_uninit_bg(fs,
> > >  				EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
> > >  
> > > +	if (FEATURE_ON(E2P_FEATURE_INCOMPAT,
> > > +		       EXT4_FEATURE_INCOMPAT_INLINE_DATA)) {
> > > +		/* Inline_data feature cannot be enabled if ext_attr is
> > > +		 * disabled.
> > > +		 */
> > 
> > Multiline comment issue.
> > 
> > > +		if (!(fs->super->s_feature_compat &
> > > +		      EXT2_FEATURE_COMPAT_EXT_ATTR)) {
> > 
> > EXT2_HAS_COMPAT_FEATURE()?
> > 
> > > +			fputs(_("The inline_data feature cannot "
> > > +				"be set if ext_attr feature is disabled.\n"),
> > > +				stderr);
> > > +			return 1;
> > > +		}
> > > +	}
> > > +
> > 
> > Maybe I'm missing something, but I don't see anything prohibiting the user from
> > disabling inline_data?  Or from doing the conversion?
> 
> Yes, now in tune2fs we can only enable inline_data feature.  If we want
> to disable inline_data, that means that we need to allocate a block for
> every inode with inline data, and we could exhaust all disk space.
> Another implementation might be just clear inline_data flag from
> super_block and doesn't allocate a block for these inodes.  The former
> obviously is not a good choice.  But the latter might be better.  I am
> really not sure for this.  What do you think?

I don't think it's a good idea to turn off inline_data once it's been enabled,
just like we don't let people turn off extents...

*Oh*, duh, I didn't notice that INLINE_DATA is *NOT* in the clear_ok_features
list.  Never mind this comment, I'm just making noise.

(The complaints about the comment issue and not using EXT2_HAS_COMPAT_FEATURE
remain.)

--D
> 
> Thanks,
>                                                 - Zheng
--
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
Zheng Liu Oct. 12, 2013, 8:33 a.m. UTC | #4 
On Sat, Oct 12, 2013 at 01:23:15AM -0700, Darrick J. Wong wrote:
> On Sat, Oct 12, 2013 at 04:16:38PM +0800, Zheng Liu wrote:
> > On Fri, Oct 11, 2013 at 05:39:45PM -0700, Darrick J. Wong wrote:
> > > On Fri, Aug 02, 2013 at 05:49:42PM +0800, Zheng Liu wrote:
> > > > From: Zheng Liu <wenqing.lz@taobao.com>
> > > > 
> > > > Inline_data feature can be set when ext_attr feature is enabled.  Now this
> > > > feature only can be enabled because we may be out of space when disabling
> > > > it.  If this feature is disabled, we need to allocate a block for every
> > > > file and directory, and it might exhaust all space.
> > > > 
> > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> > > > ---
> > > >  misc/tune2fs.8.in |    5 +++++
> > > >  misc/tune2fs.c    |   17 ++++++++++++++++-
> > > >  2 files changed, 21 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in
> > > > index 55c6dd9..b768fff 100644
> > > > --- a/misc/tune2fs.8.in
> > > > +++ b/misc/tune2fs.8.in
> > > > @@ -531,6 +531,11 @@ Setting the filesystem feature is equivalent to using the
> > > >  .B \-j
> > > >  option.
> > > >  .TP
> > > > +.B inline_data
> > > > +Allow data to be stored in inode.
> > > > +.B Tune2fs
> > > > +only supports setting this filesystem feature.
> > > > +.TP
> > > >  .B large_file
> > > >  Filesystem can contain files that are greater than 2GB.  (Modern kernels
> > > >  set this feature automatically when a file > 2GB is created.)
> > > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > > > index 7d6520e..f22a736 100644
> > > > --- a/misc/tune2fs.c
> > > > +++ b/misc/tune2fs.c
> > > > @@ -137,7 +137,8 @@ static __u32 ok_features[3] = {
> > > >  	EXT2_FEATURE_INCOMPAT_FILETYPE |
> > > >  		EXT3_FEATURE_INCOMPAT_EXTENTS |
> > > >  		EXT4_FEATURE_INCOMPAT_FLEX_BG |
> > > > -		EXT4_FEATURE_INCOMPAT_MMP,
> > > > +		EXT4_FEATURE_INCOMPAT_MMP |
> > > > +		EXT4_FEATURE_INCOMPAT_INLINE_DATA,
> > > >  	/* R/O compat */
> > > >  	EXT2_FEATURE_RO_COMPAT_LARGE_FILE |
> > > >  		EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
> > > > @@ -995,6 +996,20 @@ mmp_error:
> > > >  		disable_uninit_bg(fs,
> > > >  				EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
> > > >  
> > > > +	if (FEATURE_ON(E2P_FEATURE_INCOMPAT,
> > > > +		       EXT4_FEATURE_INCOMPAT_INLINE_DATA)) {
> > > > +		/* Inline_data feature cannot be enabled if ext_attr is
> > > > +		 * disabled.
> > > > +		 */
> > > 
> > > Multiline comment issue.
> > > 
> > > > +		if (!(fs->super->s_feature_compat &
> > > > +		      EXT2_FEATURE_COMPAT_EXT_ATTR)) {
> > > 
> > > EXT2_HAS_COMPAT_FEATURE()?
> > > 
> > > > +			fputs(_("The inline_data feature cannot "
> > > > +				"be set if ext_attr feature is disabled.\n"),
> > > > +				stderr);
> > > > +			return 1;
> > > > +		}
> > > > +	}
> > > > +
> > > 
> > > Maybe I'm missing something, but I don't see anything prohibiting the user from
> > > disabling inline_data?  Or from doing the conversion?
> > 
> > Yes, now in tune2fs we can only enable inline_data feature.  If we want
> > to disable inline_data, that means that we need to allocate a block for
> > every inode with inline data, and we could exhaust all disk space.
> > Another implementation might be just clear inline_data flag from
> > super_block and doesn't allocate a block for these inodes.  The former
> > obviously is not a good choice.  But the latter might be better.  I am
> > really not sure for this.  What do you think?
> 
> I don't think it's a good idea to turn off inline_data once it's been enabled,
> just like we don't let people turn off extents...
> 
> *Oh*, duh, I didn't notice that INLINE_DATA is *NOT* in the clear_ok_features
> list.  Never mind this comment, I'm just making noise.
> 
> (The complaints about the comment issue and not using EXT2_HAS_COMPAT_FEATURE
> remain.)

Got it. :)

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

Patch

diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in
index 55c6dd9..b768fff 100644
--- a/misc/tune2fs.8.in
+++ b/misc/tune2fs.8.in
@@ -531,6 +531,11 @@  Setting the filesystem feature is equivalent to using the
 .B \-j
 option.
 .TP
+.B inline_data
+Allow data to be stored in inode.
+.B Tune2fs
+only supports setting this filesystem feature.
+.TP
 .B large_file
 Filesystem can contain files that are greater than 2GB.  (Modern kernels
 set this feature automatically when a file > 2GB is created.)
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index 7d6520e..f22a736 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -137,7 +137,8 @@  static __u32 ok_features[3] = {
 	EXT2_FEATURE_INCOMPAT_FILETYPE |
 		EXT3_FEATURE_INCOMPAT_EXTENTS |
 		EXT4_FEATURE_INCOMPAT_FLEX_BG |
-		EXT4_FEATURE_INCOMPAT_MMP,
+		EXT4_FEATURE_INCOMPAT_MMP |
+		EXT4_FEATURE_INCOMPAT_INLINE_DATA,
 	/* R/O compat */
 	EXT2_FEATURE_RO_COMPAT_LARGE_FILE |
 		EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
@@ -995,6 +996,20 @@  mmp_error:
 		disable_uninit_bg(fs,
 				EXT4_FEATURE_RO_COMPAT_GDT_CSUM);
 
+	if (FEATURE_ON(E2P_FEATURE_INCOMPAT,
+		       EXT4_FEATURE_INCOMPAT_INLINE_DATA)) {
+		/* Inline_data feature cannot be enabled if ext_attr is
+		 * disabled.
+		 */
+		if (!(fs->super->s_feature_compat &
+		      EXT2_FEATURE_COMPAT_EXT_ATTR)) {
+			fputs(_("The inline_data feature cannot "
+				"be set if ext_attr feature is disabled.\n"),
+				stderr);
+			return 1;
+		}
+	}
+
 	if (FEATURE_ON(E2P_FEATURE_RO_INCOMPAT,
 				EXT4_FEATURE_RO_COMPAT_QUOTA)) {
 		/*