[PATCH] Add extent conversion support to chattr

[PATCH] Add extent conversion support to chattr

[Aneesh Kumar K.V]

This patch adds new option, -E to chattr. The -E option
is used to convert the ext3 format (non extent) file
to ext4 (extent) format. This can be used to migrate
the ext3 file system to ext4 file system.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

---
 lib/e2p/Makefile.in  |    6 +++-
 lib/e2p/e2p.h        |    1 +
 lib/e2p/migrate.c    |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/ext2fs/ext2_fs.h |    1 +
 misc/chattr.1.in     |    5 +++-
 misc/chattr.c        |   20 +++++++++++++++++-
 6 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/lib/e2p/Makefile.in b/lib/e2p/Makefile.in
index c1a45f5..56b6a3a 100644
--- a/lib/e2p/Makefile.in
+++ b/lib/e2p/Makefile.in
@@ -19,7 +19,7 @@ all::	e2p.pc
 OBJS=		feature.o fgetflags.o fsetflags.o fgetversion.o fsetversion.o \
 		getflags.o getversion.o hashstr.o iod.o ls.o mntopts.o \
 		parse_num.o pe.o pf.o ps.o setflags.o setversion.o uuid.o \
-		ostype.o percent.o
+		ostype.o percent.o migrate.o
 
 SRCS=		$(srcdir)/feature.c $(srcdir)/fgetflags.c \
 		$(srcdir)/fsetflags.c $(srcdir)/fgetversion.c \
@@ -28,7 +28,7 @@ SRCS=		$(srcdir)/feature.c $(srcdir)/fgetflags.c \
 		$(srcdir)/ls.c $(srcdir)/mntopts.c $(srcdir)/parse_num.c \
 		$(srcdir)/pe.c $(srcdir)/pf.c $(srcdir)/ps.c \
 		$(srcdir)/setflags.c $(srcdir)/setversion.c $(srcdir)/uuid.c \
-		$(srcdir)/ostype.c $(srcdir)/percent.c
+		$(srcdir)/ostype.c $(srcdir)/percent.c $(srcdir)/migrate.c
 HFILES= e2p.h
 
 LIBRARY= libe2p
@@ -162,3 +162,5 @@ ostype.o: $(srcdir)/ostype.c $(srcdir)/e2p.h \
  $(top_srcdir)/lib/ext2fs/ext2_fs.h $(top_builddir)/lib/ext2fs/ext2_types.h
 percent.o: $(srcdir)/percent.c $(srcdir)/e2p.h \
  $(top_srcdir)/lib/ext2fs/ext2_fs.h $(top_builddir)/lib/ext2fs/ext2_types.h
+migrate.o: $(srcdir)/migrate.c $(srcdir)/e2p.h \
+ $(top_srcdir)/lib/ext2fs/ext2_fs.h $(top_builddir)/lib/ext2fs/ext2_types.h
diff --git a/lib/e2p/e2p.h b/lib/e2p/e2p.h
index 98c97db..8cbf360 100644
--- a/lib/e2p/e2p.h
+++ b/lib/e2p/e2p.h
@@ -60,3 +60,4 @@ char *e2p_os2string(int os_type);
 int e2p_string2os(char *str);
 
 unsigned int e2p_percent(int percent, unsigned int base);
+int ext4_migrate(const char * name);
diff --git a/lib/e2p/migrate.c b/lib/e2p/migrate.c
new file mode 100644
index 0000000..fa5b58c
--- /dev/null
+++ b/lib/e2p/migrate.c
@@ -0,0 +1,52 @@
+/*
+ * Convert a file to extent format
+ *
+ *  Copyright IBM Corporation, 2008
+ *  Author Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
+ *
+ * This file can be redistributed under the terms of the GNU Library General
+ * Public License
+ */
+
+#define _LARGEFILE_SOURCE
+#define _LARGEFILE64_SOURCE
+
+#if HAVE_ERRNO_H
+#include <errno.h>
+#endif
+#if HAVE_UNISTD_H
+#include <unistd.h>
+#endif
+#include <fcntl.h>
+#if HAVE_SYS_IOCTL_H
+#include <sys/ioctl.h>
+#endif
+#include "e2p.h"
+
+#ifdef O_LARGEFILE
+#define OPEN_FLAGS (O_RDONLY|O_NONBLOCK|O_LARGEFILE)
+#else
+#define OPEN_FLAGS (O_RDONLY|O_NONBLOCK)
+#endif
+
+int ext4_migrate(const char * name)
+{
+#if HAVE_EXT2_IOCTLS
+	int fd, r, save_errno = 0;
+
+	fd = open (name, OPEN_FLAGS);
+	if (fd == -1)
+		return -1;
+	r = ioctl (fd, EXT4_IOC_MIGRATE, NULL);
+	if (r == -1)
+		save_errno = errno;
+	close (fd);
+	if (save_errno)
+		errno = save_errno;
+	return r;
+#else /* ! HAVE_EXT2_IOCTLS */
+	extern int errno;
+	errno = EOPNOTSUPP;
+	return -1;
+#endif /* ! HAVE_EXT2_IOCTLS */
+}
diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
index d7d7bdb..2fe6691 100644
--- a/lib/ext2fs/ext2_fs.h
+++ b/lib/ext2fs/ext2_fs.h
@@ -316,6 +316,7 @@ struct ext4_new_group_input {
 #define EXT2_IOC_GROUP_EXTEND		_IOW('f', 7, unsigned long)
 #define EXT2_IOC_GROUP_ADD		_IOW('f', 8,struct ext2_new_group_input)
 #define EXT4_IOC_GROUP_ADD		_IOW('f', 8,struct ext4_new_group_input)
+#define EXT4_IOC_MIGRATE                _IO('f', 7)
 
 /*
  * Structure of an inode on the disk
diff --git a/misc/chattr.1.in b/misc/chattr.1.in
index 960f058..7e7f62a 100644
--- a/misc/chattr.1.in
+++ b/misc/chattr.1.in
@@ -5,7 +5,7 @@ chattr \- change file attributes on a Linux second extended file system
 .SH SYNOPSIS
 .B chattr
 [
-.B \-RVf
+.B \-RVfE
 ]
 [
 .B \-v
@@ -41,6 +41,9 @@ Be verbose with chattr's output and print the program version.
 .B \-f
 Suppress most error messages.
 .TP
+.B \-E
+Convert the file to extent format
+.TP
 .BI \-v " version"
 Set the file's version/generation number.
 .SH ATTRIBUTES
diff --git a/misc/chattr.c b/misc/chattr.c
index 3d67519..95a3998 100644
--- a/misc/chattr.c
+++ b/misc/chattr.c
@@ -60,6 +60,7 @@ static int add;
 static int rem;
 static int set;
 static int set_version;
+static int conv_to_extents;
 
 static unsigned long version;
 
@@ -82,7 +83,7 @@ static unsigned long sf;
 static void usage(void)
 {
 	fprintf(stderr,
-		_("Usage: %s [-RVf] [-+=AacDdijsSu] [-v version] files...\n"),
+		_("Usage: %s [-RVfE] [-+=AacDdijsSu] [-v version] files...\n"),
 		program_name);
 	exit(1);
 }
@@ -156,6 +157,10 @@ static int decode_arg (int * i, int argc, char ** argv)
 				set_version = 1;
 				continue;
 			}
+			if (*p == 'E') {
+				conv_to_extents = 1;
+				continue;
+			}
 			if ((fl = get_flag(*p)) == 0)
 				usage();
 			rf |= fl;
@@ -245,6 +250,17 @@ static int change_attributes(const char * name)
 			return -1;
 		}
 	}
+	if (conv_to_extents) {
+		if (verbose)
+			printf (_("Converting %s to extent format\n"), name);
+		if (ext4_migrate(name) == -1) {
+			if (!silent)
+				com_err (program_name, errno,
+				 _("while converting %s to extent format"),
+				 name);
+			return -1;
+		}
+	}
 	if (S_ISDIR(st.st_mode) && recursive)
 		return iterate_on_dir (name, chattr_dir_proc, NULL);
 	return 0;
@@ -306,7 +322,7 @@ int main (int argc, char ** argv)
 		fputs("Can't both set and unset same flag.\n", stderr);
 		exit (1);
 	}
-	if (!(add || rem || set || set_version)) {
+	if (!(add || rem || set || set_version || conv_to_extents)) {
 		fputs(_("Must use '-v', =, - or +\n"), stderr);
 		exit (1);
 	}
-- 
tg: (5bf3f80..) an/chattr (depends on: master)

Re: [PATCH] Add extent conversion support to chattr

[Christoph Hellwig]

On Tue, Sep 09, 2008 at 02:42:22PM +0530, Aneesh Kumar K.V wrote:
> This patch adds new option, -E to chattr. The -E option
> is used to convert the ext3 format (non extent) file
> to ext4 (extent) format. This can be used to migrate
> the ext3 file system to ext4 file system.

I think this is an awkware interfac.  Chattr is supposed to set simple
binary flags and not a front end to complicated filesystem conversions.

Re: [PATCH] Add extent conversion support to chattr

[Aneesh Kumar K.V]

On Tue, Sep 09, 2008 at 07:46:47AM -0400, Christoph Hellwig wrote:
> On Tue, Sep 09, 2008 at 02:42:22PM +0530, Aneesh Kumar K.V wrote:
> > This patch adds new option, -E to chattr. The -E option
> > is used to convert the ext3 format (non extent) file
> > to ext4 (extent) format. This can be used to migrate
> > the ext3 file system to ext4 file system.
> 
> I think this is an awkware interfac.  Chattr is supposed to set simple
> binary flags and not a front end to complicated filesystem conversions.
> 

Since chattr is used to set inode flags and since migrate can be looked
at as setting extent flags (with the side effect that we change the 
inode format) I used chattr interface. Last patch i sent actually added a new
command e4migrate. Let me know if you have any suggestion related to
which command is best suited for setting extent flags.

-aneesh

Re: [PATCH] Add extent conversion support to chattr

[Theodore Tso]

On Tue, Sep 09, 2008 at 07:46:47AM -0400, Christoph Hellwig wrote:
> On Tue, Sep 09, 2008 at 02:42:22PM +0530, Aneesh Kumar K.V wrote:
> > This patch adds new option, -E to chattr. The -E option
> > is used to convert the ext3 format (non extent) file
> > to ext4 (extent) format. This can be used to migrate
> > the ext3 file system to ext4 file system.
> 
> I think this is an awkware interfac.  Chattr is supposed to set simple
> binary flags and not a front end to complicated filesystem conversions.

The alternate is to create an entire new program (e4migrate) just to
trigger a single ioctl.  The reality is this is probably going to more
used by ext4 developers than anybody else, since it's rare that you
would want to convert a single file from using indrect blocks to using
extents.  In general, most users/system administrators will want to
convert the entire filesystem; eventually this will probably be done
via some combination with the userspace program to trigger online
defrag, but this was just a stopgap measure to allow us to more easily
exercise the kernel code more than anything else.

So given that this is only to enable extents on a single file, "chattr
+e file" is very much in line with the rest of the chattr interface
for setting other flags.

One of the things which we may want to do to use statfs() and checking
f_type to make sure the file in question is located on an ext2/3/4
filesystem before trying the ioctl, since it is true that a number of
other filesystems have "borrowed" the chattr program and use it for
their own purposes.  It's unlikely that the ext4 migration ioctl will
be wired to anything on other filesystems, but it would be a good
safety measure to add just in case.

Regards,

						- Ted

Re: [PATCH] Add extent conversion support to chattr

[Aneesh Kumar K.V]

On Tue, Sep 09, 2008 at 09:43:47AM -0400, Theodore Tso wrote:
> On Tue, Sep 09, 2008 at 07:46:47AM -0400, Christoph Hellwig wrote:
> > On Tue, Sep 09, 2008 at 02:42:22PM +0530, Aneesh Kumar K.V wrote:
> > > This patch adds new option, -E to chattr. The -E option
> > > is used to convert the ext3 format (non extent) file
> > > to ext4 (extent) format. This can be used to migrate
> > > the ext3 file system to ext4 file system.
> > 
> > I think this is an awkware interfac.  Chattr is supposed to set simple
> > binary flags and not a front end to complicated filesystem conversions.
> 
> The alternate is to create an entire new program (e4migrate) just to
> trigger a single ioctl.  The reality is this is probably going to more
> used by ext4 developers than anybody else, since it's rare that you
> would want to convert a single file from using indrect blocks to using
> extents.  In general, most users/system administrators will want to
> convert the entire filesystem; eventually this will probably be done
> via some combination with the userspace program to trigger online
> defrag, but this was just a stopgap measure to allow us to more easily
> exercise the kernel code more than anything else.
> 
> So given that this is only to enable extents on a single file, "chattr
> +e file" is very much in line with the rest of the chattr interface
> for setting other flags.
> 
> One of the things which we may want to do to use statfs() and checking
> f_type to make sure the file in question is located on an ext2/3/4
> filesystem before trying the ioctl, since it is true that a number of
> other filesystems have "borrowed" the chattr program and use it for
> their own purposes.  It's unlikely that the ext4 migration ioctl will
> be wired to anything on other filesystems, but it would be a good
> safety measure to add just in case.
> 

Shouldn't other file system give error when we call an ioctl with
EXT4_IOC_MIGRATE on the fd ?

On ext3 I get the below error
[an/chattr@e2fsprogs-upstream-build]$ ./misc/chattr -E ./misc/e2undo
./misc/chattr: Inappropriate ioctl for device while converting ./misc/e2undo to extent format

-aneesh

Re: [PATCH] Add extent conversion support to chattr

[Theodore Tso]

On Tue, Sep 09, 2008 at 07:39:29PM +0530, Aneesh Kumar K.V wrote:
> 
> Shouldn't other file system give error when we call an ioctl with
> EXT4_IOC_MIGRATE on the fd ?

Only if by some incredible bad luck the ioctl number (which is after
all only at 16 bit number) doesn't happen to do something else random,
like security erase the entire filesystem.  :-)

> On ext3 I get the below error
> [an/chattr@e2fsprogs-upstream-build]$ ./misc/chattr -E ./misc/e2undo
> ./misc/chattr: Inappropriate ioctl for device while converting ./misc/e2undo to extent format
> 

#1, it really should be +e, since we are turning on the extent flag,
and #2, we should give a much more user-understandable error message
in that case.

						- Ted

Re: [PATCH] Add extent conversion support to chattr

[Aneesh Kumar K.V]

On Tue, Sep 09, 2008 at 10:39:15AM -0400, Theodore Tso wrote:
> On Tue, Sep 09, 2008 at 07:39:29PM +0530, Aneesh Kumar K.V wrote:
> > 
> > Shouldn't other file system give error when we call an ioctl with
> > EXT4_IOC_MIGRATE on the fd ?
> 
> Only if by some incredible bad luck the ioctl number (which is after
> all only at 16 bit number) doesn't happen to do something else random,
> like security erase the entire filesystem.  :-)
> 
> > On ext3 I get the below error
> > [an/chattr@e2fsprogs-upstream-build]$ ./misc/chattr -E ./misc/e2undo
> > ./misc/chattr: Inappropriate ioctl for device while converting ./misc/e2undo to extent format
> > 
> 
> #1, it really should be +e, since we are turning on the extent flag,
> and #2, we should give a much more user-understandable error message
> in that case.

something like 

From: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Subject: [PATCH] Add extent conversion support to chattr

This patch adds new option, +e to chattr. The +e option
is used to convert the ext3 format (non extent) file
to ext4 (extent) format. This can be used to migrate
the ext3 file system to ext4 file system.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

---
 lib/e2p/e2p.h        |    1 +
 lib/e2p/fsetflags.c  |   10 ++++++++++
 lib/ext2fs/ext2_fs.h |    1 +
 misc/chattr.1.in     |    9 ++++-----
 misc/chattr.c        |   17 ++++++++++++++++-
 5 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/lib/e2p/e2p.h b/lib/e2p/e2p.h
index 98c97db..8cbf360 100644
--- a/lib/e2p/e2p.h
+++ b/lib/e2p/e2p.h
@@ -60,3 +60,4 @@ char *e2p_os2string(int os_type);
 int e2p_string2os(char *str);
 
 unsigned int e2p_percent(int percent, unsigned int base);
+int ext4_migrate(const char * name);
diff --git a/lib/e2p/fsetflags.c b/lib/e2p/fsetflags.c
index 62189c9..d1bb9d9 100644
--- a/lib/e2p/fsetflags.c
+++ b/lib/e2p/fsetflags.c
@@ -80,9 +80,19 @@ int fsetflags (const char * name, unsigned long flags)
 	if (fd == -1)
 		return -1;
 	f = (int) flags;
+	if (f & EXT4_EXTENTS_FL) {
+		/* extent flags is set using  migrate ioctl */
+		r = ioctl (fd, EXT4_IOC_MIGRATE, NULL);
+		if (r == -1) {
+			save_errno = errno;
+			goto err_out;
+		}
+		f = (int)flags & ~EXT4_EXTENTS_FL;
+	}
 	r = ioctl (fd, EXT2_IOC_SETFLAGS, &f);
 	if (r == -1)
 		save_errno = errno;
+err_out:
 	close (fd);
 	if (save_errno)
 		errno = save_errno;
diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
index d7d7bdb..2fe6691 100644
--- a/lib/ext2fs/ext2_fs.h
+++ b/lib/ext2fs/ext2_fs.h
@@ -316,6 +316,7 @@ struct ext4_new_group_input {
 #define EXT2_IOC_GROUP_EXTEND		_IOW('f', 7, unsigned long)
 #define EXT2_IOC_GROUP_ADD		_IOW('f', 8,struct ext2_new_group_input)
 #define EXT4_IOC_GROUP_ADD		_IOW('f', 8,struct ext4_new_group_input)
+#define EXT4_IOC_MIGRATE                _IO('f', 7)
 
 /*
  * Structure of an inode on the disk
diff --git a/misc/chattr.1.in b/misc/chattr.1.in
index 960f058..1247090 100644
--- a/misc/chattr.1.in
+++ b/misc/chattr.1.in
@@ -19,7 +19,7 @@ chattr \- change file attributes on a Linux second extended file system
 .B chattr
 changes the file attributes on a Linux second extended file system.
 .PP
-The format of a symbolic mode is +-=[ASacDdIijsTtu].
+The format of a symbolic mode is +-=[ASacDdIijsTtue].
 .PP
 The operator `+' causes the selected attributes to be added to the
 existing attributes of the files; `-' causes them to be removed; and
@@ -74,10 +74,9 @@ although it can be displayed by
 .BR lsattr (1).
 .PP
 The 'e' attribute indicates that the file is using extents for mapping
-the blocks on disk.  It may not be set or reset using
-.BR chattr (1),
-although it can be displayed by
-.BR lsattr (1).
+the blocks on disk.  Setting extent flag cause the file to be converted
+to extent format. It may not be unset using
+.BR chattr (1).
 .PP
 The 'I' attribute is used by the htree code to indicate that a directory
 is being indexed using hashed trees.  It may not be set or reset using 
diff --git a/misc/chattr.c b/misc/chattr.c
index 3d67519..c9d6d1e 100644
--- a/misc/chattr.c
+++ b/misc/chattr.c
@@ -82,7 +82,7 @@ static unsigned long sf;
 static void usage(void)
 {
 	fprintf(stderr,
-		_("Usage: %s [-RVf] [-+=AacDdijsSu] [-v version] files...\n"),
+		_("Usage: %s [-RVf] [-+=AacDdijsSu] [+e] [-v version] files...\n"),
 		program_name);
 	exit(1);
 }
@@ -105,6 +105,7 @@ static const struct flags_char flags_array[] = {
 	{ EXT2_UNRM_FL, 'u' },
 	{ EXT2_NOTAIL_FL, 't' },
 	{ EXT2_TOPDIR_FL, 'T' },
+	{ EXT4_EXTENTS_FL, 'e'},
 	{ 0, 0 }
 };
 
@@ -199,6 +200,20 @@ static int change_attributes(const char * name)
 		return -1;
 	}
 
+	if (rf & EXT4_EXTENTS_FL) {
+		/* only allowed format is +e */
+		com_err (program_name, errno,
+				_("Cannot remove the extent flag on %s"), name);
+		return -1;
+	}
+
+	if (sf & EXT4_EXTENTS_FL) {
+		/* only allowed format is +e */
+		com_err (program_name, errno,
+			_("Extent flag cannot be the only attribute on %s"), name);
+		return -1;
+	}
+
 	if (set) {
 		if (verbose) {
 			printf (_("Flags of %s set as "), name);
-- 
tg: (5bf3f80..) an/chattr (depends on: master)

Re: [PATCH] Add extent conversion support to chattr

[Theodore Tso]

On Tue, Sep 09, 2008 at 09:58:14PM +0530, Aneesh Kumar K.V wrote:
> diff --git a/lib/e2p/fsetflags.c b/lib/e2p/fsetflags.c
> index 62189c9..d1bb9d9 100644
> --- a/lib/e2p/fsetflags.c
> +++ b/lib/e2p/fsetflags.c
> @@ -80,9 +80,19 @@ int fsetflags (const char * name, unsigned long flags)
>  	if (fd == -1)
>  		return -1;
>  	f = (int) flags;
> +	if (f & EXT4_EXTENTS_FL) {
> +		/* extent flags is set using  migrate ioctl */
> +		r = ioctl (fd, EXT4_IOC_MIGRATE, NULL);
> +		if (r == -1) {
> +			save_errno = errno;
> +			goto err_out;
> +		}
> +		f = (int)flags & ~EXT4_EXTENTS_FL;
> +	}

No, I wouldn't put this in libe2p; it probably should be in the chattr
program.  If you do it in chattr, it's much easier to have an
user-friendly error message if the EXT4_IOC_MIGRATE ioctl fails.

Also, it should only call EXT4_IOC_MIGRATE if the file was previously
not supporting extents.  Probably a bad idea to unconditionally call
EXT4_IOC_MIGRATE.

					- Ted

Re: [PATCH] Add extent conversion support to chattr

[Christoph Hellwig]

On Tue, Sep 09, 2008 at 09:43:47AM -0400, Theodore Tso wrote:
> The alternate is to create an entire new program (e4migrate) just to
> trigger a single ioctl.  The reality is this is probably going to more
> used by ext4 developers than anybody else, since it's rare that you
> would want to convert a single file from using indrect blocks to using
> extents.  In general, most users/system administrators will want to
> convert the entire filesystem; eventually this will probably be done
> via some combination with the userspace program to trigger online
> defrag, but this was just a stopgap measure to allow us to more easily
> exercise the kernel code more than anything else.
> 
> So given that this is only to enable extents on a single file, "chattr
> +e file" is very much in line with the rest of the chattr interface
> for setting other flags.

Well, if you think setting a flag is a good interface to convert to
extents, then do it all the way and allow setting the extent
flag in the kernel through FS_IOC_SETFLAGS instead of having an
interface schism.

Re: [PATCH] Add extent conversion support to chattr

[Theodore Tso]

On Tue, Sep 09, 2008 at 12:50:05PM -0400, Christoph Hellwig wrote:
> Well, if you think setting a flag is a good interface to convert to
> extents, then do it all the way and allow setting the extent
> flag in the kernel through FS_IOC_SETFLAGS instead of having an
> interface schism.

Thanks, that's a good idea.  It won't help people on older kernels,
but it probably is the right answer.

Regards,

					- Ted

[PATCH] Add extent conversion support to chattr

[Aneesh Kumar K.V]

This patch adds new option, +e to chattr. The +e option
is used to convert the ext3 format (non extent) file
to ext4 (extent) format. This can be used to migrate
the ext3 file system to ext4 file system.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

---
 misc/chattr.c |   58 +++++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/misc/chattr.c b/misc/chattr.c
index 3d67519..7807b9e 100644
--- a/misc/chattr.c
+++ b/misc/chattr.c
@@ -82,7 +82,7 @@ static unsigned long sf;
 static void usage(void)
 {
 	fprintf(stderr,
-		_("Usage: %s [-RVf] [-+=AacDdijsSu] [-v version] files...\n"),
+		_("Usage: %s [-RVf] [-+=AacDdijsSue] [-v version] files...\n"),
 		program_name);
 	exit(1);
 }
@@ -105,6 +105,7 @@ static const struct flags_char flags_array[] = {
 	{ EXT2_UNRM_FL, 'u' },
 	{ EXT2_NOTAIL_FL, 't' },
 	{ EXT2_TOPDIR_FL, 'T' },
+	{ EXT4_EXTENTS_FL, 'e'},
 	{ 0, 0 }
 };
 
@@ -191,6 +192,7 @@ static int change_attributes(const char * name)
 {
 	unsigned long flags;
 	STRUCT_STAT	st;
+	int extent_file = 0;
 
 	if (LSTAT (name, &st) == -1) {
 		if (!silent)
@@ -199,7 +201,22 @@ static int change_attributes(const char * name)
 		return -1;
 	}
 
+	if (fgetflags (name, &flags) == -1) {
+		if (!silent)
+			com_err (program_name, errno,
+					_("while reading flags on %s"), name);
+		return -1;
+	}
+	if (flags & EXT4_EXTENTS_FL)
+		extent_file = 1;
 	if (set) {
+		if (extent_file && !(sf & EXT4_EXTENTS_FL)) {
+			if (!silent)
+				com_err(program_name, 0,
+				_("Clearing extent flag not supported on %s"),
+					name);
+			return -1;
+		}
 		if (verbose) {
 			printf (_("Flags of %s set as "), name);
 			print_flags (stdout, sf, 0);
@@ -208,30 +225,31 @@ static int change_attributes(const char * name)
 		if (fsetflags (name, sf) == -1)
 			perror (name);
 	} else {
-		if (fgetflags (name, &flags) == -1) {
+		if (rem)
+			flags &= ~rf;
+		if (add)
+			flags |= af;
+		if (extent_file && !(flags & EXT4_EXTENTS_FL)) {
 			if (!silent)
-				com_err (program_name, errno,
-					 _("while reading flags on %s"), name);
+				com_err(program_name, 0,
+				_("Clearing extent flag not supported on %s"),
+					name);
 			return -1;
-		} else {
-			if (rem)
-				flags &= ~rf;
-			if (add)
-				flags |= af;
-			if (verbose) {
-				printf (_("Flags of %s set as "), name);
-				print_flags (stdout, flags, 0);
-				printf ("\n");
-			}
-			if (!S_ISDIR(st.st_mode))
-				flags &= ~EXT2_DIRSYNC_FL;
-			if (fsetflags (name, flags) == -1) {
-				if (!silent)
-					com_err(program_name, errno,
+		}
+		if (verbose) {
+			printf (_("Flags of %s set as "), name);
+			print_flags (stdout, flags, 0);
+			printf ("\n");
+		}
+		if (!S_ISDIR(st.st_mode))
+			flags &= ~EXT2_DIRSYNC_FL;
+		if (fsetflags (name, flags) == -1) {
+			if (!silent) {
+				com_err(program_name, errno,
 						_("while setting flags on %s"),
 						name);
-				return -1;
 			}
+			return -1;
 		}
 	}
 	if (set_version) {
-- 
tg: (5bf3f80..) an/chattr3 (depends on: master)

Re: [PATCH] Add extent conversion support to chattr

[Andreas Dilger]

On Sep 12, 2008  14:34 +0530, Aneesh Kumar wrote:
> -		_("Usage: %s [-RVf] [-+=AacDdijsSu] [-v version] files...\n"),
> +		_("Usage: %s [-RVf] [-+=AacDdijsSue] [-v version] files...\n"),

Please keep options in alphabetical order.

> @@ -105,6 +105,7 @@ static const struct flags_char flags_array[] = {
>  	{ EXT2_UNRM_FL, 'u' },
>  	{ EXT2_NOTAIL_FL, 't' },
>  	{ EXT2_TOPDIR_FL, 'T' },
> +	{ EXT4_EXTENTS_FL, 'e'},

I'd also prefer to keep these in alphabetical order, which they almost are.

> @@ -199,7 +201,22 @@ static int change_attributes(const char * name)
>  	if (set) {
> +		if (extent_file && !(sf & EXT4_EXTENTS_FL)) {
> +			if (!silent)
> +				com_err(program_name, 0,
> +				_("Clearing extent flag not supported on %s"),
> +					name);
> +			return -1;
> +		}

Why not just try to set this flag and let the kernel decide what is
possible?

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [PATCH] Add extent conversion support to chattr

[Aneesh Kumar K.V]

On Fri, Sep 12, 2008 at 03:17:13AM -0600, Andreas Dilger wrote:
> On Sep 12, 2008  14:34 +0530, Aneesh Kumar wrote:
> > -		_("Usage: %s [-RVf] [-+=AacDdijsSu] [-v version] files...\n"),
> > +		_("Usage: %s [-RVf] [-+=AacDdijsSue] [-v version] files...\n"),
> 
> Please keep options in alphabetical order.
> 
> > @@ -105,6 +105,7 @@ static const struct flags_char flags_array[] = {
> >  	{ EXT2_UNRM_FL, 'u' },
> >  	{ EXT2_NOTAIL_FL, 't' },
> >  	{ EXT2_TOPDIR_FL, 'T' },
> > +	{ EXT4_EXTENTS_FL, 'e'},
> 
> I'd also prefer to keep these in alphabetical order, which they almost are.
> 
> > @@ -199,7 +201,22 @@ static int change_attributes(const char * name)
> >  	if (set) {
> > +		if (extent_file && !(sf & EXT4_EXTENTS_FL)) {
> > +			if (!silent)
> > +				com_err(program_name, 0,
> > +				_("Clearing extent flag not supported on %s"),
> > +					name);
> > +			return -1;
> > +		}
> 
> Why not just try to set this flag and let the kernel decide what is
> possible?
> 

The motivation is to give a clear message that we still don't support
clearing extent flags. If we pass it to the kernel we will get
operation not supported error which would confuse the user who does
chattr -de f1

-aneesh

Re: [PATCH] Add extent conversion support to chattr

[Andreas Dilger]

On Sep 12, 2008  14:57 +0530, Aneesh Kumar wrote:
> On Fri, Sep 12, 2008 at 03:17:13AM -0600, Andreas Dilger wrote:
> > Why not just try to set this flag and let the kernel decide what is
> > possible?
> 
> The motivation is to give a clear message that we still don't support
> clearing extent flags. If we pass it to the kernel we will get
> operation not supported error which would confuse the user who does
> chattr -de f1

The problem is that if the kernel ever does allow converting extents->
block-mapped (or some other mapping that isn't extents), the chattr
tool will cause this to fail even when it shouldn't.  Better to leave
the check in a single place (the kernel, which knows best).

I don't think the user would be too confused.  You could still check
after the call to the kernel to see if the "e" option is being removed
and print an error if the call failed.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [PATCH] Add extent conversion support to chattr

[Aneesh Kumar K.V]

Updated patch

From: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Subject: [PATCH] Add extent conversion support to chattr

This patch adds new option, +e to chattr. The +e option
is used to convert the ext3 format (non extent) file
to ext4 (extent) format. This can be used to migrate
the ext3 file system to ext4 file system.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

---
 misc/chattr.1.in |    8 ++----
 misc/chattr.c    |   58 +++++++++++++++++++++++++++++++++++------------------
 2 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/misc/chattr.1.in b/misc/chattr.1.in
index 960f058..c49f876 100644
--- a/misc/chattr.1.in
+++ b/misc/chattr.1.in
@@ -19,7 +19,7 @@ chattr \- change file attributes on a Linux second extended file system
 .B chattr
 changes the file attributes on a Linux second extended file system.
 .PP
-The format of a symbolic mode is +-=[ASacDdIijsTtu].
+The format of a symbolic mode is +-=[ASacDdeIijsTtu].
 .PP
 The operator `+' causes the selected attributes to be added to the
 existing attributes of the files; `-' causes them to be removed; and
@@ -74,10 +74,8 @@ although it can be displayed by
 .BR lsattr (1).
 .PP
 The 'e' attribute indicates that the file is using extents for mapping
-the blocks on disk.  It may not be set or reset using
-.BR chattr (1),
-although it can be displayed by
-.BR lsattr (1).
+the blocks on disk.  It may not be removed using
+.BR chattr (1).
 .PP
 The 'I' attribute is used by the htree code to indicate that a directory
 is being indexed using hashed trees.  It may not be set or reset using 
diff --git a/misc/chattr.c b/misc/chattr.c
index 3d67519..067d0b6 100644
--- a/misc/chattr.c
+++ b/misc/chattr.c
@@ -82,7 +82,7 @@ static unsigned long sf;
 static void usage(void)
 {
 	fprintf(stderr,
-		_("Usage: %s [-RVf] [-+=AacDdijsSu] [-v version] files...\n"),
+		_("Usage: %s [-RVf] [-+=AacDdeijsSu] [-v version] files...\n"),
 		program_name);
 	exit(1);
 }
@@ -99,6 +99,7 @@ static const struct flags_char flags_array[] = {
 	{ EXT2_APPEND_FL, 'a' },
 	{ EXT2_COMPR_FL, 'c' },
 	{ EXT2_NODUMP_FL, 'd' },
+	{ EXT4_EXTENTS_FL, 'e'},
 	{ EXT2_IMMUTABLE_FL, 'i' },
 	{ EXT3_JOURNAL_DATA_FL, 'j' },
 	{ EXT2_SECRM_FL, 's' },
@@ -191,6 +192,7 @@ static int change_attributes(const char * name)
 {
 	unsigned long flags;
 	STRUCT_STAT	st;
+	int extent_file = 0;
 
 	if (LSTAT (name, &st) == -1) {
 		if (!silent)
@@ -199,7 +201,22 @@ static int change_attributes(const char * name)
 		return -1;
 	}
 
+	if (fgetflags (name, &flags) == -1) {
+		if (!silent)
+			com_err (program_name, errno,
+					_("while reading flags on %s"), name);
+		return -1;
+	}
+	if (flags & EXT4_EXTENTS_FL)
+		extent_file = 1;
 	if (set) {
+		if (extent_file && !(sf & EXT4_EXTENTS_FL)) {
+			if (!silent)
+				com_err(program_name, 0,
+				_("Clearing extent flag not supported on %s"),
+					name);
+			return -1;
+		}
 		if (verbose) {
 			printf (_("Flags of %s set as "), name);
 			print_flags (stdout, sf, 0);
@@ -208,30 +225,31 @@ static int change_attributes(const char * name)
 		if (fsetflags (name, sf) == -1)
 			perror (name);
 	} else {
-		if (fgetflags (name, &flags) == -1) {
+		if (rem)
+			flags &= ~rf;
+		if (add)
+			flags |= af;
+		if (extent_file && !(flags & EXT4_EXTENTS_FL)) {
 			if (!silent)
-				com_err (program_name, errno,
-					 _("while reading flags on %s"), name);
+				com_err(program_name, 0,
+				_("Clearing extent flag not supported on %s"),
+					name);
 			return -1;
-		} else {
-			if (rem)
-				flags &= ~rf;
-			if (add)
-				flags |= af;
-			if (verbose) {
-				printf (_("Flags of %s set as "), name);
-				print_flags (stdout, flags, 0);
-				printf ("\n");
-			}
-			if (!S_ISDIR(st.st_mode))
-				flags &= ~EXT2_DIRSYNC_FL;
-			if (fsetflags (name, flags) == -1) {
-				if (!silent)
-					com_err(program_name, errno,
+		}
+		if (verbose) {
+			printf (_("Flags of %s set as "), name);
+			print_flags (stdout, flags, 0);
+			printf ("\n");
+		}
+		if (!S_ISDIR(st.st_mode))
+			flags &= ~EXT2_DIRSYNC_FL;
+		if (fsetflags (name, flags) == -1) {
+			if (!silent) {
+				com_err(program_name, errno,
 						_("while setting flags on %s"),
 						name);
-				return -1;
 			}
+			return -1;
 		}
 	}
 	if (set_version) {
-- 
tg: (5bf3f80..) an/chattr3 (depends on: master)