| Message ID | 1316357699-22692-2-git-send-email-dmonakhov@openvz.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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
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
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
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
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
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 +])
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