Patchwork [2/2] xfstest: fsstress add EXT2_IOC_{SET,GET}FLAGS operations

login
register
mail settings
Submitter Dmitri Monakho
Date Sept. 18, 2011, 2:54 p.m.
Message ID <1316357699-22692-2-git-send-email-dmonakhov@openvz.org>
Download mbox | patch
Permalink /patch/115254/
State Not Applicable
Headers show

Comments

Dmitri Monakho - Sept. 18, 2011, 2:54 p.m.
Add two new operations:
- getattr: ioctl(fd, EXT2_IOC_GETFLAGS, &fl)
- setattr: ioctl(fd, EXT2_IOC_SETFLAGS, &random_flags)
By default IOC_SET_SETFLAGS has zero probability because
it may produce inodes with APPEND or IMMUTABLE flags which
are not deletable by default. Let's assumes that one who
enable it knows how to delete such inodes.
For example like follows:
find $TEST_PATH -exec chattr -i -a {} \;
rm -rf $TEST_PATH

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 aclocal.m4            |    6 +++++
 configure.in          |    2 +
 ltp/fsstress.c        |   61 +++++++++++++++++++++++++++++++++++++++++++++++++
 m4/package_e2fslib.m4 |    6 +++++
 4 files changed, 75 insertions(+), 0 deletions(-)
 create mode 100644 m4/package_e2fslib.m4

\ No newline at end of file
Christoph Hellwig - Sept. 18, 2011, 8:03 p.m.
On Sun, Sep 18, 2011 at 06:54:59PM +0400, Dmitry Monakhov wrote:
> Add two new operations:
> - getattr: ioctl(fd, EXT2_IOC_GETFLAGS, &fl)
> - setattr: ioctl(fd, EXT2_IOC_SETFLAGS, &random_flags)
> By default IOC_SET_SETFLAGS has zero probability because
> it may produce inodes with APPEND or IMMUTABLE flags which
> are not deletable by default. Let's assumes that one who
> enable it knows how to delete such inodes.
> For example like follows:
> find $TEST_PATH -exec chattr -i -a {} \;
> rm -rf $TEST_PATH

In general I like this, but:

 - please provide a testcase actually using this new feature, and
 - please don't require e2fsprogs just for the ioctl subcommands,
   and use the FS_IOC_GET/SETFLAGS names provided by recent kernels
   in fs.h instead.  You might still need an ifdef for old kernels,
   like src/t_immutable.c does.  In fact it might be a good idea
   to just provide the values for them if they aren't present in
   a header shared by fsstress and t_immutable.c

--
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
Alex Elder - Oct. 5, 2011, 1:20 p.m.
On Sun, 2011-09-18 at 18:54 +0400, Dmitry Monakhov wrote:
> Add two new operations:
> - getattr: ioctl(fd, EXT2_IOC_GETFLAGS, &fl)
> - setattr: ioctl(fd, EXT2_IOC_SETFLAGS, &random_flags)
> By default IOC_SET_SETFLAGS has zero probability because
> it may produce inodes with APPEND or IMMUTABLE flags which
> are not deletable by default. Let's assumes that one who
> enable it knows how to delete such inodes.
> For example like follows:
> find $TEST_PATH -exec chattr -i -a {} \;
> rm -rf $TEST_PATH
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>

I have a question below.  I think this is probably
a good addition, though it should be made so it
works for more than EXTx.

If I understand the way it would be used, this will
simply be another operation that gets randomly performed
by fsstress while it operates, right?

I have not done any testing with this yet.

Reviewed-by: Alex Elder <aelder@sgi.com>

. . .

> @@ -1729,6 +1738,58 @@ setxattr_f(int opno, long r)
>  }
>  
>  void
> +getattr_f(int opno, long r)
> +{
> +#ifdef HAVE_EXT2_INCLUDE
> +	int		fd;
> +	int		e;
> +	pathname_t	f;
> +	uint		fl;
> +	int		v;
> +
> +	init_pathname(&f);
> +	if (!get_fname(FT_ANYm, r, &f, NULL, NULL, &v))
> +		append_pathname(&f, ".");

I don't understand the purpose of appending a "." to the
end of the path.  Do you intend to just use "." if
no other file matches?  (That may not be a good thing to
do--it might not be testing the intended target.)
  
Or are you intending to append "/." so for a directory
its "." link gets used in the open?  If so that's not
what this does (it simply makes "a/b/x" become "a/b/x.").

Same comments apply to setattr_f().

> +	fd = open_path(&f, O_RDWR);
> +	e = fd < 0 ? errno : 0;
> +	check_cwd();
> +
> +	e = ioctl(fd, EXT2_IOC_GETFLAGS, &fl);
> +	if (v)
> +		printf("%d/%d: getattr %s %u %d\n", procid, opno, f.path, fl, e);
> +	free_pathname(&f);
> +	close(fd);
> +#endif
> +}
> +
> +void
> +setattr_f(int opno, long r)
> +{


--
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
Dmitri Monakho - Oct. 6, 2011, 9:21 a.m.
On Wed, 5 Oct 2011 08:20:40 -0500, Alex Elder <aelder@sgi.com> wrote:
> On Sun, 2011-09-18 at 18:54 +0400, Dmitry Monakhov wrote:
> > Add two new operations:
> > - getattr: ioctl(fd, EXT2_IOC_GETFLAGS, &fl)
> > - setattr: ioctl(fd, EXT2_IOC_SETFLAGS, &random_flags)
> > By default IOC_SET_SETFLAGS has zero probability because
> > it may produce inodes with APPEND or IMMUTABLE flags which
> > are not deletable by default. Let's assumes that one who
> > enable it knows how to delete such inodes.
> > For example like follows:
> > find $TEST_PATH -exec chattr -i -a {} \;
> > rm -rf $TEST_PATH
> > 
> > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> 
> I have a question below.  I think this is probably
> a good addition, though it should be made so it
> works for more than EXTx.
> 
> If I understand the way it would be used, this will
> simply be another operation that gets randomly performed
> by fsstress while it operates, right?
> 
> I have not done any testing with this yet.
> 
> Reviewed-by: Alex Elder <aelder@sgi.com>
> 
> . . .
> 
> > @@ -1729,6 +1738,58 @@ setxattr_f(int opno, long r)
> >  }
> >  
> >  void
> > +getattr_f(int opno, long r)
> > +{
> > +#ifdef HAVE_EXT2_INCLUDE
> > +	int		fd;
> > +	int		e;
> > +	pathname_t	f;
> > +	uint		fl;
> > +	int		v;
> > +
> > +	init_pathname(&f);
> > +	if (!get_fname(FT_ANYm, r, &f, NULL, NULL, &v))
> > +		append_pathname(&f, ".");
> 
> I don't understand the purpose of appending a "." to the
> end of the path.  Do you intend to just use "." if
> no other file matches?  (That may not be a good thing to
> do--it might not be testing the intended target.)
Yes just "." will be used. and infact i've copied that chunk from
chown_f, and similar approach is used for att_remove_f, attr_set_f
and setxattr_f operations. And in fact i'm not quite agree that they
are not independent, the only point they are connected is parent dir,
which IMHO acceptable in this case because operations result in inode
metadata changes only, w/o changing parent dir.
>   
> Or are you intending to append "/." so for a directory
> its "." link gets used in the open?  If so that's not
> what this does (it simply makes "a/b/x" become "a/b/x."). 
> Same comments apply to setattr_f().
> 
> > +	fd = open_path(&f, O_RDWR);
> > +	e = fd < 0 ? errno : 0;
> > +	check_cwd();
> > +
> > +	e = ioctl(fd, EXT2_IOC_GETFLAGS, &fl);
> > +	if (v)
> > +		printf("%d/%d: getattr %s %u %d\n", procid, opno, f.path, fl, e);
> > +	free_pathname(&f);
> > +	close(fd);
> > +#endif
> > +}
> > +
> > +void
> > +setattr_f(int opno, long r)
> > +{
> 
> 
--
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
Christoph Hellwig - Oct. 6, 2011, 7:52 p.m.
On Wed, Oct 05, 2011 at 08:20:40AM -0500, Alex Elder wrote:
> I have a question below.  I think this is probably
> a good addition, though it should be made so it
> works for more than EXTx.

I actually works on more than extN, but at this point it requires
the ext2 headers to compiled, which isn't too nice.

--
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
Christoph Hellwig - Oct. 19, 2011, 9:28 a.m.
Dmitry,

and updates on these patches?

--
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
Dmitri Monakho - Oct. 19, 2011, 10:48 a.m.
On Wed, 19 Oct 2011 05:28:46 -0400, Christoph Hellwig <hch@infradead.org> wrote:
> Dmitry,
> 
> and updates on these patches?
I've posted new version here:
http://article.gmane.org/gmane.comp.file-systems.ext4/28602

Update for set flags patch now is named:
[PATCH 4/8] xfstests: fsstress add FS_IOC_{SET,GET}FLAGS operations

But still, ext4 panic in case of ordered+delalloc=>journalled switch
"-f setattr=1", i've not posted fix for that bug yet, so please run
this test carefully.
> 
> --
> 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

Patch

diff --git a/aclocal.m4 b/aclocal.m4
index 5532606..30ac837 100644
--- a/aclocal.m4
+++ b/aclocal.m4
@@ -21,6 +21,11 @@  AC_DEFUN([AC_PACKAGE_WANT_LINUX_PRCTL_H],
     AC_SUBST(have_prctl)
   ])
 
+AC_DEFUN([AC_PACKAGE_WANT_EXT2_INCLUDE],
+  [ AC_CHECK_HEADER([ext2fs/ext2fs.h],[ have_ext2_include=true ], [ have_ext2_include=false ])
+  AC_SUBST(have_ext2_include)
+  ])
+
 AC_DEFUN([AC_PACKAGE_WANT_FALLOCATE],
   [ AC_MSG_CHECKING([for fallocate])
     AC_TRY_LINK([
@@ -43,3 +48,4 @@  m4_include([m4/package_globals.m4])
 m4_include([m4/package_utilies.m4])
 m4_include([m4/package_uuiddev.m4])
 m4_include([m4/package_xfslibs.m4])
+m4_include([m4/package_e2fslib.m4])
diff --git a/configure.in b/configure.in
index 76d23e4..1e0d138 100644
--- a/configure.in
+++ b/configure.in
@@ -68,6 +68,8 @@  in
 		AC_PACKAGE_WANT_LINUX_FIEMAP_H
 		AC_PACKAGE_WANT_FALLOCATE
 		AC_PACKAGE_WANT_LINUX_PRCTL_H
+
+		AC_PACKAGE_WANT_EXT2_INCLUDE
 		;;
 esac
 
diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index cee2cad..416190b 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -31,6 +31,9 @@ 
 #ifdef HAVE_SYS_PRCTL_H
 #include <sys/prctl.h>
 #endif
+#ifdef HAVE_EXT2_INCLUDE
+#include <ext2fs/ext2fs.h>
+#endif
 #include <math.h>
 #define XFS_ERRTAG_MAX		17
 #define XFS_IDMODULO_MAX	31	/* user/group IDs (1 << x)  */
@@ -68,6 +71,8 @@  typedef enum {
 	OP_UNLINK,
 	OP_UNRESVSP,
 	OP_WRITE,
+	OP_GETATTR,
+	OP_SETATTR,
 	OP_LAST
 } opty_t;
 
@@ -142,6 +147,8 @@  void	resvsp_f(int, long);
 void	rmdir_f(int, long);
 void	setxattr_f(int, long);
 void	stat_f(int, long);
+void	getattr_f(int, long);
+void	setattr_f(int, long);
 void	symlink_f(int, long);
 void	sync_f(int, long);
 void	truncate_f(int, long);
@@ -179,6 +186,8 @@  opdesc_t	ops[] = {
 	{ OP_UNLINK, "unlink", unlink_f, 1, 1 },
 	{ OP_UNRESVSP, "unresvsp", unresvsp_f, 1, 1 },
 	{ OP_WRITE, "write", write_f, 4, 1 },
+	{ OP_GETATTR, "getattr", getattr_f, 1, 0 },
+	{ OP_SETATTR, "setattr", setattr_f, 0, 1 },
 }, *ops_end;
 
 flist_t	flist[FT_nft] = {
@@ -1729,6 +1738,58 @@  setxattr_f(int opno, long r)
 }
 
 void
+getattr_f(int opno, long r)
+{
+#ifdef HAVE_EXT2_INCLUDE
+	int		fd;
+	int		e;
+	pathname_t	f;
+	uint		fl;
+	int		v;
+
+	init_pathname(&f);
+	if (!get_fname(FT_ANYm, r, &f, NULL, NULL, &v))
+		append_pathname(&f, ".");
+	fd = open_path(&f, O_RDWR);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+
+	e = ioctl(fd, EXT2_IOC_GETFLAGS, &fl);
+	if (v)
+		printf("%d/%d: getattr %s %u %d\n", procid, opno, f.path, fl, e);
+	free_pathname(&f);
+	close(fd);
+#endif
+}
+
+void
+setattr_f(int opno, long r)
+{
+#ifdef HAVE_EXT2_INCLUDE
+	int		fd;
+	int		e;
+	pathname_t	f;
+	uint		fl;
+	int		v;
+
+	init_pathname(&f);
+	if (!get_fname(FT_ANYm, r, &f, NULL, NULL, &v))
+		append_pathname(&f, ".");
+	fd = open_path(&f, O_RDWR);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+
+	fl = (uint)random();
+	e = ioctl(fd, EXT2_IOC_SETFLAGS, &fl);
+	if (v)
+		printf("%d/%d: setattr %s %u %d\n", procid, opno, f.path, fl, e);
+	free_pathname(&f);
+	close(fd);
+#endif
+}
+
+
+void
 creat_f(int opno, long r)
 {
 	struct fsxattr	a;
diff --git a/m4/package_e2fslib.m4 b/m4/package_e2fslib.m4
new file mode 100644
index 0000000..6ab48f4
--- /dev/null
+++ b/m4/package_e2fslib.m4
@@ -0,0 +1,6 @@ 
+AC_DEFUN([AC_PACKAGE_WANT_EXT2_INCLUDE],
+  [AC_CHECK_HEADER([ext2fs/ext2fs.h])
+  if test "$ac_cv_header_ext2fs_ext2fs_h" == "yes"; then
+     AC_DEFINE([HAVE_EXT2_INCLUDE], 1, [Header files for e2fslib])
+  fi
+])