[PATCH v2 0/3] xfs_quota: allow operation on ext4 for project quotas

[PATCH v2 0/3] xfs_quota: allow operation on ext4 for project quotas

[Bill O'Donnell]

Hello -

I'm submitting a version 2 to clarify patch 1 loop logic correction.

This is a resubmission of Dave Chinner's original 2-patch series
to enable using xfs_quota for project quotas on foreign filesystems
(e.g. ext4).

Original series: http://oss.sgi.com/archives/xfs/2016-02/msg00107.html

Updated series:
Patch 1: initial capabilities to enable xfs_quota use on foreign filesystems.
Patch 2: userspace changes to accomodate hoisted ioctl defs in kernel
Patch 3: additional changes to accomodate xfs_quota use on foreign filesystems.

History:
-----
v1: http://oss.sgi.com/archives/xfs/2016-08/msg00435.html
v2: patch 1: slight logic modification to clarify loop in init_args_command
             (functional equivalent)

-----

Again, questions and comments are welcome.

Thanks-
Bill

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2 1/3] xfs_quota: add capabilities for use on ext4

[Bill O'Donnell]

This allows xfs_quota to be used on ext4 for project quota testing
in xfstests.

This patch was originally submitted by Dave Chinner
(http://oss.sgi.com/archives/xfs/2016-02/msg00131.html)

Resubmitting with the following change:
quota/init.c: correct logic error in loop contained in init_args_command()
function (lines 85-91).

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 include/command.h |  3 ++-
 include/path.h    |  1 +
 io/init.h         |  2 +-
 libxcmd/paths.c   |  7 +++----
 quota/free.c      |  2 ++
 quota/init.c      | 29 +++++++++++++++++++++++++++--
 quota/init.h      |  1 +
 quota/path.c      |  5 +++--
 quota/project.c   |  1 +
 quota/quot.c      |  1 +
 quota/quota.c     |  2 ++
 quota/report.c    | 11 +++++++++--
 quota/state.c     |  4 +++-
 quota/util.c      |  1 +
 14 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/include/command.h b/include/command.h
index 7b9fc28..81d5a4d 100644
--- a/include/command.h
+++ b/include/command.h
@@ -20,7 +20,8 @@
 
 #include <sys/time.h>
 
-#define CMD_FLAG_GLOBAL	((int)0x80000000)	/* don't iterate "args" */
+#define CMD_FLAG_GLOBAL		(1<<31)	/* don't iterate "args" */
+#define CMD_FLAG_FOREIGN_OK	(1<<30)	/* command not restricted to XFS */
 
 typedef int (*cfunc_t)(int argc, char **argv);
 typedef void (*helpfunc_t)(void);
diff --git a/include/path.h b/include/path.h
index 46a887e..39c1a95 100644
--- a/include/path.h
+++ b/include/path.h
@@ -29,6 +29,7 @@
 
 #define FS_MOUNT_POINT	(1<<0)
 #define FS_PROJECT_PATH	(1<<1)
+#define FS_FOREIGN	(1<<2)
 
 typedef struct fs_path {
 	char		*fs_name;	/* Data device for filesystem 	*/
diff --git a/io/init.h b/io/init.h
index d773b1b..bb25242 100644
--- a/io/init.h
+++ b/io/init.h
@@ -18,7 +18,7 @@
 
 #define CMD_NOFILE_OK	(1<<0)	/* command doesn't need an open file	*/
 #define CMD_NOMAP_OK	(1<<1)	/* command doesn't need a mapped region	*/
-#define CMD_FOREIGN_OK	(1<<2)	/* command not restricted to XFS files	*/
+#define CMD_FOREIGN_OK	CMD_FLAG_FOREIGN_OK
 
 extern char	*progname;
 extern int	exitcode;
diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 71af25f..7c8c673 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -113,6 +113,9 @@ fs_table_insert(
 			goto out_nodev;
 	}
 
+	if (!platform_test_xfs_path(dir))
+		flags |= FS_FOREIGN;
+
 	/*
 	 * Make copies of the directory and data device path.
 	 * The log device and real-time device, if non-null,
@@ -301,8 +304,6 @@ fs_table_initialise_mounts(
 			return errno;
 
 	while ((mnt = getmntent(mtp)) != NULL) {
-		if (strcmp(mnt->mnt_type, "xfs") != 0)
-			continue;
 		if (!realpath(mnt->mnt_dir, rmnt_dir))
 			continue;
 		if (!realpath(mnt->mnt_fsname, rmnt_fsname))
@@ -360,8 +361,6 @@ fs_table_initialise_mounts(
 			return errno;
 
 	for (i = 0; i < count; i++) {
-		if (strcmp(stats[i].f_fstypename, "xfs") != 0)
-			continue;
 		if (!realpath(stats[i].f_mntfromname, rmntfromname))
 			continue;
 		if (!realpath(stats[i].f_mntonname, rmntonname))
diff --git a/quota/free.c b/quota/free.c
index e9e0319..b9be954 100644
--- a/quota/free.c
+++ b/quota/free.c
@@ -16,6 +16,7 @@
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include <stdbool.h>
 #include "command.h"
 #include "init.h"
 #include "quota.h"
@@ -371,6 +372,7 @@ free_init(void)
 	free_cmd.args = _("[-bir] [-hn] [-f file]");
 	free_cmd.oneline = _("show free and used counts for blocks and inodes");
 	free_cmd.help = free_help;
+	free_cmd.flags = CMD_FLAG_FOREIGN_OK;
 
 	add_command(&free_cmd);
 }
diff --git a/quota/init.c b/quota/init.c
index 52f7941..d5d80c2 100644
--- a/quota/init.c
+++ b/quota/init.c
@@ -24,6 +24,7 @@
 char	*progname;
 int	exitcode;
 int	expert;
+bool	foreign_allowed = false;
 
 static char **projopts;	/* table of project names (cmdline) */
 static int nprojopts;	/* number of entries in name table. */
@@ -83,15 +84,36 @@ init_args_command(
 
 	do {
 		fs_path = &fs_table[index++];
-	} while ((fs_path->fs_flags & FS_PROJECT_PATH) && index < fs_count);
+		if (!(fs_path->fs_flags & FS_PROJECT_PATH))
+			break;
+		if (!foreign_allowed && (fs_path->fs_flags & FS_FOREIGN))
+			continue;
+	} while (index < fs_count);
 
 	if (fs_path->fs_flags & FS_PROJECT_PATH)
 		return 0;
+	if (!foreign_allowed && (fs_path->fs_flags & FS_FOREIGN))
+		return 0;
 	if (index > fs_count)
 		return 0;
 	return index;
 }
 
+static int
+init_check_command(
+	const cmdinfo_t	*ct)
+{
+	if (fs_path &&
+	    !(ct->flags & CMD_FLAG_FOREIGN_OK) &&
+	     (fs_path->fs_flags & FS_FOREIGN)) {
+		fprintf(stderr,
+	_("foreign mount active, %s command is for XFS filesystems only\n"),
+			ct->name);
+		return 0;
+	}
+	return 1;
+}
+
 static void
 init(
 	int		argc,
@@ -104,7 +126,7 @@ init(
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 
-	while ((c = getopt(argc, argv, "c:d:D:P:p:t:xV")) != EOF) {
+	while ((c = getopt(argc, argv, "c:d:D:fP:p:t:xV")) != EOF) {
 		switch (c) {
 		case 'c':	/* commands */
 			add_user_command(optarg);
@@ -112,6 +134,8 @@ init(
 		case 'd':
 			add_project_opt(optarg);
 			break;
+		case 'f':
+			foreign_allowed = true;
 		case 't':
 			mtab_file = optarg;
 			break;
@@ -140,6 +164,7 @@ init(
 
 	init_commands();
 	add_args_command(init_args_command);
+	add_check_command(init_check_command);
 
 	/*
 	 * Ensure that global commands don't end up with an invalid path pointer
diff --git a/quota/init.h b/quota/init.h
index 71706cb..6879855 100644
--- a/quota/init.h
+++ b/quota/init.h
@@ -19,6 +19,7 @@
 extern char	*progname;
 extern int	exitcode;
 extern int	expert;
+extern bool	foreign_allowed;
 
 extern void	edit_init(void);
 extern void	free_init(void);
diff --git a/quota/path.c b/quota/path.c
index bdb8c98..a623d25 100644
--- a/quota/path.c
+++ b/quota/path.c
@@ -42,6 +42,7 @@ printpath(
 	if (number) {
 		printf(_("%c%03d%c "), braces? '[':' ', index, braces? ']':' ');
 	}
+	printf("%s ", (path->fs_flags & FS_FOREIGN) ? "(F)" : "   ");
 	printf(_("%-19s %s"), path->fs_dir, path->fs_name);
 	if (path->fs_flags & FS_PROJECT_PATH) {
 		prj = getprprid(path->fs_prid);
@@ -127,7 +128,7 @@ path_init(void)
 	path_cmd.cfunc = path_f;
 	path_cmd.argmin = 0;
 	path_cmd.argmax = 1;
-	path_cmd.flags = CMD_FLAG_GLOBAL;
+	path_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK;
 	path_cmd.oneline = _("set current path, or show the list of paths");
 
 	print_cmd.name = "print";
@@ -135,7 +136,7 @@ path_init(void)
 	print_cmd.cfunc = print_f;
 	print_cmd.argmin = 0;
 	print_cmd.argmax = 0;
-	print_cmd.flags = CMD_FLAG_GLOBAL;
+	print_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK;
 	print_cmd.oneline = _("list known mount points and projects");
 
 	if (expert)
diff --git a/quota/project.c b/quota/project.c
index fb8b9e1..e4e7a01 100644
--- a/quota/project.c
+++ b/quota/project.c
@@ -355,6 +355,7 @@ project_init(void)
 	project_cmd.argmax = -1;
 	project_cmd.oneline = _("check, setup or clear project quota trees");
 	project_cmd.help = project_help;
+	project_cmd.flags = CMD_FLAG_FOREIGN_OK;
 
 	if (expert)
 		add_command(&project_cmd);
diff --git a/quota/quot.c b/quota/quot.c
index 2e583e5..ccc154f 100644
--- a/quota/quot.c
+++ b/quota/quot.c
@@ -16,6 +16,7 @@
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include <stdbool.h>
 #include "command.h"
 #include <ctype.h>
 #include <pwd.h>
diff --git a/quota/quota.c b/quota/quota.c
index e0da7c0..d09e239 100644
--- a/quota/quota.c
+++ b/quota/quota.c
@@ -16,6 +16,7 @@
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include <stdbool.h>
 #include "command.h"
 #include <ctype.h>
 #include <pwd.h>
@@ -469,6 +470,7 @@ quota_init(void)
 	quota_cmd.args = _("[-bir] [-g|-p|-u] [-hnNv] [-f file] [id|name]...");
 	quota_cmd.oneline = _("show usage and limits");
 	quota_cmd.help = quota_help;
+	quota_cmd.flags = CMD_FLAG_FOREIGN_OK;
 
 	add_command(&quota_cmd);
 }
diff --git a/quota/report.c b/quota/report.c
index 70220b4..604f50d 100644
--- a/quota/report.c
+++ b/quota/report.c
@@ -15,7 +15,7 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
-
+#include <stdbool.h>
 #include "command.h"
 #include <sys/types.h>
 #include <pwd.h>
@@ -618,6 +618,8 @@ report_any_type(
 	if (type & XFS_USER_QUOTA) {
 		fs_cursor_initialise(dir, FS_MOUNT_POINT, &cursor);
 		while ((mount = fs_cursor_next_entry(&cursor))) {
+			if (!foreign_allowed && (mount->fs_flags & FS_FOREIGN))
+				continue;
 			if (xfsquotactl(XFS_QSYNC, mount->fs_name,
 						XFS_USER_QUOTA, 0, NULL) < 0
 					&& errno != ENOENT && errno != ENOSYS)
@@ -629,6 +631,8 @@ report_any_type(
 	if (type & XFS_GROUP_QUOTA) {
 		fs_cursor_initialise(dir, FS_MOUNT_POINT, &cursor);
 		while ((mount = fs_cursor_next_entry(&cursor))) {
+			if (!foreign_allowed && (mount->fs_flags & FS_FOREIGN))
+				continue;
 			if (xfsquotactl(XFS_QSYNC, mount->fs_name,
 						XFS_GROUP_QUOTA, 0, NULL) < 0
 					&& errno != ENOENT && errno != ENOSYS)
@@ -640,6 +644,8 @@ report_any_type(
 	if (type & XFS_PROJ_QUOTA) {
 		fs_cursor_initialise(dir, FS_MOUNT_POINT, &cursor);
 		while ((mount = fs_cursor_next_entry(&cursor))) {
+			if (!foreign_allowed && (mount->fs_flags & FS_FOREIGN))
+				continue;
 			if (xfsquotactl(XFS_QSYNC, mount->fs_name,
 						XFS_PROJ_QUOTA, 0, NULL) < 0
 					&& errno != ENOENT && errno != ENOSYS)
@@ -754,16 +760,17 @@ report_init(void)
 	dump_cmd.args = _("[-g|-p|-u] [-f file]");
 	dump_cmd.oneline = _("dump quota information for backup utilities");
 	dump_cmd.help = dump_help;
+	dump_cmd.flags = CMD_FLAG_FOREIGN_OK;
 
 	report_cmd.name = "report";
 	report_cmd.altname = "repquota";
 	report_cmd.cfunc = report_f;
 	report_cmd.argmin = 0;
 	report_cmd.argmax = -1;
-	report_cmd.flags = CMD_FLAG_GLOBAL;
 	report_cmd.args = _("[-bir] [-gpu] [-ahnt] [-f file]");
 	report_cmd.oneline = _("report filesystem quota information");
 	report_cmd.help = report_help;
+	report_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK;
 
 	if (expert) {
 		add_command(&dump_cmd);
diff --git a/quota/state.c b/quota/state.c
index 8186762..d134580 100644
--- a/quota/state.c
+++ b/quota/state.c
@@ -15,7 +15,7 @@
  * along with this program; if not, write the Free Software Foundation,
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
-
+#include <stdbool.h>
 #include "command.h"
 #include "init.h"
 #include "quota.h"
@@ -527,6 +527,7 @@ state_init(void)
 	off_cmd.args = _("[-gpu] [-v]");
 	off_cmd.oneline = _("permanently switch quota off for a path");
 	off_cmd.help = off_help;
+	off_cmd.flags = CMD_FLAG_FOREIGN_OK;
 
 	state_cmd.name = "state";
 	state_cmd.cfunc = state_f;
@@ -535,6 +536,7 @@ state_init(void)
 	state_cmd.args = _("[-gpu] [-a] [-v] [-f file]");
 	state_cmd.oneline = _("get overall quota state information");
 	state_cmd.help = state_help;
+	state_cmd.flags = CMD_FLAG_FOREIGN_OK;
 
 	enable_cmd.name = "enable";
 	enable_cmd.cfunc = enable_f;
diff --git a/quota/util.c b/quota/util.c
index e3c5398..cafd45f 100644
--- a/quota/util.c
+++ b/quota/util.c
@@ -17,6 +17,7 @@
  */
 
 #include <sys/types.h>
+#include <stdbool.h>
 #include <pwd.h>
 #include <grp.h>
 #include <utmp.h>
-- 
2.7.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2 2/3] xfs_quota: changes to accomodate hoisted ioctl defs

[Bill O'Donnell]

The kernel commit to make the ioctl promotion (334e580a6f) moved
the definitions for the XFS ioctl to uapi/linux/fs.h for the
following reason:

    Hoist the ioctl definitions for the XFS_IOC_FS[SG]SETXATTR API
    from fs/xfs/libxfs/xfs_fs.h to include/uapi/linux/fs.h so that
    the ioctls can be used by all filesystems, not just XFS. This
    enables (initially) ext4 to use the ioctl to set project IDs on
    inodes.

This means we now need to handle this change in userspace as the
uapi/linux/fs.h file may not contain the definitions (i.e. new
xfsprogs/ old linux uapi files) xfsprogs needs to build. Hence we
need to massage the definition in xfs_fs.h to take the values from
the system header if it exists, otherwise keep the old definitions
for compatibility and platforms other than linux..

This patch was originally submitted by Dave Chinner
(http://oss.sgi.com/archives/xfs/2016-02/msg00108.html)

Resubmitting with changes to accomodate upstream changes since
initial submission.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 libxfs/xfs_fs.h | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
index 085ea6f..6ecb7f2 100644
--- a/libxfs/xfs_fs.h
+++ b/libxfs/xfs_fs.h
@@ -35,10 +35,41 @@ struct dioattr {
 };
 #endif
 
+/* check whether we have to define FS_IOC_FS[GS]ETXATTR ourselves */
+#ifndef FS_IOC_FSGETXATTR
+struct fsxattr {
+	__u32		fsx_xflags;	/* xflags field value (get/set) */
+	__u32		fsx_extsize;	/* extsize field value (get/set)*/
+	__u32		fsx_projid;	/* project identifier (get/set) */
+	unsigned char	fsx_pad[12];
+};
+
+/*
+ * Flags for the fsx_xflags field
+ */
+#define FS_XFLAG_REALTIME	0x00000001	/* data in realtime volume */
+#define FS_XFLAG_PREALLOC	0x00000002	/* preallocated file extents */
+#define FS_XFLAG_IMMUTABLE	0x00000008	/* file cannot be modified */
+#define FS_XFLAG_APPEND		0x00000010	/* all writes append */
+#define FS_XFLAG_SYNC		0x00000020	/* all writes synchronous */
+#define FS_XFLAG_NOATIME	0x00000040	/* do not update access time */
+#define FS_XFLAG_NODUMP		0x00000080	/* do not include in backups */
+#define FS_XFLAG_RTINHERIT	0x00000100	/* create with rt bit set */
+#define FS_XFLAG_PROJINHERIT	0x00000200	/* create with parents projid */
+#define FS_XFLAG_NOSYMLINKS	0x00000400	/* disallow symlink creation */
+#define FS_XFLAG_EXTSIZE	0x00000800	/* extent size allocator hint */
+#define FS_XFLAG_EXTSZINHERIT	0x00001000	/* inherit inode extent size */
+#define FS_XFLAG_NODEFRAG	0x00002000	/* do not defragment */
+#define FS_XFLAG_FILESTREAM	0x00004000	/* use filestream allocator */
+#define FS_XFLAG_HASATTR	0x80000000	/* no DIFLAG for this	*/
+
+#define FS_IOC_FSGETXATTR     _IOR ('X', 31, struct fsxattr)
+#define FS_IOC_FSSETXATTR     _IOW ('X', 32, struct fsxattr)
+
+#endif
+
 /*
- * Flags for the bs_xflags/fsx_xflags field in XFS_IOC_FS[GS]ETXATTR[A]
- * These are for backwards compatibility only. New code should
- * use the kernel [4.5 onwards] defined FS_XFLAG_* definitions directly.
+ * Flags for the bs_xflags/fsx_xflags field in FS_IOC_FS[GS]ETXATTR[A]
  */
 #define	XFS_XFLAG_REALTIME	FS_XFLAG_REALTIME
 #define	XFS_XFLAG_PREALLOC	FS_XFLAG_PREALLOC
-- 
2.7.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2 3/3] xfs_quota: additional changes to allow use on ext4

[Bill O'Donnell]

Further changes to allow xfs_quota to be used on foreign filesystem(s)
(e.g. ext4) for project quota testing in xfstests.

Add CMD_FLAG_GENERIC to enable generic xfs_quota commands (help and
quit) when xfs_quota is run on foreign filesystems.

Use CMD_FLAG_FOREIGN_OK on commands suitable for foreign filesystems.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 include/command.h | 1 +
 libxcmd/help.c    | 3 ++-
 libxcmd/quit.c    | 3 ++-
 quota/init.c      | 3 ++-
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/command.h b/include/command.h
index 81d5a4d..1c2898a 100644
--- a/include/command.h
+++ b/include/command.h
@@ -22,6 +22,7 @@
 
 #define CMD_FLAG_GLOBAL		(1<<31)	/* don't iterate "args" */
 #define CMD_FLAG_FOREIGN_OK	(1<<30)	/* command not restricted to XFS */
+#define CMD_FLAG_GENERIC	(1<<29) /* command is generic (help, quit) */
 
 typedef int (*cfunc_t)(int argc, char **argv);
 typedef void (*helpfunc_t)(void);
diff --git a/libxcmd/help.c b/libxcmd/help.c
index fad0ab9..be26765 100644
--- a/libxcmd/help.c
+++ b/libxcmd/help.c
@@ -88,7 +88,8 @@ help_init(void)
 	help_cmd.cfunc = help_f;
 	help_cmd.argmin = 0;
 	help_cmd.argmax = 1;
-	help_cmd.flags = CMD_FLAG_GLOBAL;
+	help_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK |
+		CMD_FLAG_GENERIC;
 	help_cmd.args = _("[command]");
 	help_cmd.oneline = _("help for one or all commands");
 
diff --git a/libxcmd/quit.c b/libxcmd/quit.c
index 0183b8f..2a27c89 100644
--- a/libxcmd/quit.c
+++ b/libxcmd/quit.c
@@ -38,7 +38,8 @@ quit_init(void)
 	quit_cmd.cfunc = quit_f;
 	quit_cmd.argmin = -1;
 	quit_cmd.argmax = -1;
-	quit_cmd.flags = CMD_FLAG_GLOBAL;
+	quit_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK |
+		CMD_FLAG_GENERIC;
 	quit_cmd.oneline = _("exit the program");
 
 	add_command(&quit_cmd);
diff --git a/quota/init.c b/quota/init.c
index d5d80c2..85931bf 100644
--- a/quota/init.c
+++ b/quota/init.c
@@ -104,7 +104,8 @@ init_check_command(
 	const cmdinfo_t	*ct)
 {
 	if (fs_path &&
-	    !(ct->flags & CMD_FLAG_FOREIGN_OK) &&
+	    !((ct->flags & CMD_FLAG_FOREIGN_OK) && foreign_allowed) &&
+	    !(ct->flags & CMD_FLAG_GENERIC) &&
 	     (fs_path->fs_flags & FS_FOREIGN)) {
 		fprintf(stderr,
 	_("foreign mount active, %s command is for XFS filesystems only\n"),
-- 
2.7.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 2/3] xfs_quota: changes to accomodate hoisted ioctl defs

[Dave Chinner]

On Tue, Aug 16, 2016 at 09:16:37AM -0500, Bill O'Donnell wrote:
> The kernel commit to make the ioctl promotion (334e580a6f) moved
> the definitions for the XFS ioctl to uapi/linux/fs.h for the
> following reason:
> 
>     Hoist the ioctl definitions for the XFS_IOC_FS[SG]SETXATTR API
>     from fs/xfs/libxfs/xfs_fs.h to include/uapi/linux/fs.h so that
>     the ioctls can be used by all filesystems, not just XFS. This
>     enables (initially) ext4 to use the ioctl to set project IDs on
>     inodes.
> 
> This means we now need to handle this change in userspace as the
> uapi/linux/fs.h file may not contain the definitions (i.e. new
> xfsprogs/ old linux uapi files) xfsprogs needs to build. Hence we
> need to massage the definition in xfs_fs.h to take the values from
> the system header if it exists, otherwise keep the old definitions
> for compatibility and platforms other than linux..
> 
> This patch was originally submitted by Dave Chinner
> (http://oss.sgi.com/archives/xfs/2016-02/msg00108.html)
> 
> Resubmitting with changes to accomodate upstream changes since
> initial submission.

This is unnecessary - the "include local version" changes were made
to the platform headers (e.g. include/linux.h) in commit 83f4b5a
("xfs_fs.h: XFS_IOC_FS[SG]SETXATTR to FS_IOC_FS[SG]ETXATTR
promotion").

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 1/3] xfs_quota: add capabilities for use on ext4

[Dave Chinner]

On Tue, Aug 16, 2016 at 09:16:36AM -0500, Bill O'Donnell wrote:
> This allows xfs_quota to be used on ext4 for project quota testing
> in xfstests.
> 
> This patch was originally submitted by Dave Chinner
> (http://oss.sgi.com/archives/xfs/2016-02/msg00131.html)
> 
> Resubmitting with the following change:
> quota/init.c: correct logic error in loop contained in init_args_command()
> function (lines 85-91).

What logic error?

Commit messages like this really don't tell the reader anything
about what is different the original patch.  I've had to go archive
spelunking to work out what is different, and I'm still not sure
what the logic error you fixed is....

And, FWIW, whilst spelunking, I noticed that Eric's last review
comments on my original patch:

	Looks ok, but now with the new option:

	1) needs a manpage update
	2) usage() should be updated to include -f

	3) and I just noticed,

	_("foreign mount active, %s command is for XFS filesystems only\n"),

	seems kind of unclear; maybe just

	_("%s command is for XFS filesystems only\n"),

have not been addressed by this update.

Can you please add these these changes, update the commit message
and resubmit?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 3/3] xfs_quota: additional changes to allow use on ext4

[Dave Chinner]

On Tue, Aug 16, 2016 at 09:16:38AM -0500, Bill O'Donnell wrote:
> Further changes to allow xfs_quota to be used on foreign filesystem(s)
> (e.g. ext4) for project quota testing in xfstests.
> 
> Add CMD_FLAG_GENERIC to enable generic xfs_quota commands (help and
> quit) when xfs_quota is run on foreign filesystems.
> 
> Use CMD_FLAG_FOREIGN_OK on commands suitable for foreign filesystems.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  include/command.h | 1 +
>  libxcmd/help.c    | 3 ++-
>  libxcmd/quit.c    | 3 ++-
>  quota/init.c      | 3 ++-
>  4 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/include/command.h b/include/command.h
> index 81d5a4d..1c2898a 100644
> --- a/include/command.h
> +++ b/include/command.h
> @@ -22,6 +22,7 @@
>  
>  #define CMD_FLAG_GLOBAL		(1<<31)	/* don't iterate "args" */
>  #define CMD_FLAG_FOREIGN_OK	(1<<30)	/* command not restricted to XFS */
> +#define CMD_FLAG_GENERIC	(1<<29) /* command is generic (help, quit) */

I don't think this belongs in include/command.h - it's an xfs_quota
specific behaviour so I'd put this in quota/init.h:

#define CMD_SKIP_CHECK	(1<<0)	/* command is always run */

> diff --git a/quota/init.c b/quota/init.c
> index d5d80c2..85931bf 100644
> --- a/quota/init.c
> +++ b/quota/init.c
> @@ -104,7 +104,8 @@ init_check_command(
>  	const cmdinfo_t	*ct)
>  {
>  	if (fs_path &&
> -	    !(ct->flags & CMD_FLAG_FOREIGN_OK) &&
> +	    !((ct->flags & CMD_FLAG_FOREIGN_OK) && foreign_allowed) &&
> +	    !(ct->flags & CMD_FLAG_GENERIC) &&
>  	     (fs_path->fs_flags & FS_FOREIGN)) {

This is now sufficiently confusing that it needs to be reworked to
make the logic clear and easily commented. i.e. something like this:

static int
init_check_command(
	const cmdinfo_t *ct)
{
	if (!fspath)
		return 1;

	/* Always run commands that we are told to skip here */
	if (ct->flags & CMD_SKIP_CHECK)
		return 1;

	/* if it's an XFS filesystem, always run the command */
	if (!(fs_path->fs_flags & FS_FOREIGN))
		return 1;

	/* If the user specified foreign filesysetms are ok, run it */
	if (foreign_allowed &&
	    (ct->flags & CMD_FLAG_FOREIGN_OK))
		return 1;

	/* foreign filesystem and it's no a valid command! */
	fprintf(stderr, _("%s command is for XFS filesystems only\n"),
			ct->name);
	return 0;
}

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 3/3] xfs_quota: additional changes to allow use on ext4

[Eric Sandeen]

On 8/21/16 9:06 PM, Dave Chinner wrote:
> On Tue, Aug 16, 2016 at 09:16:38AM -0500, Bill O'Donnell wrote:
>> Further changes to allow xfs_quota to be used on foreign filesystem(s)
>> (e.g. ext4) for project quota testing in xfstests.
>>
>> Add CMD_FLAG_GENERIC to enable generic xfs_quota commands (help and
>> quit) when xfs_quota is run on foreign filesystems.
>>
>> Use CMD_FLAG_FOREIGN_OK on commands suitable for foreign filesystems.
>>
>> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
>> ---
>>  include/command.h | 1 +
>>  libxcmd/help.c    | 3 ++-
>>  libxcmd/quit.c    | 3 ++-
>>  quota/init.c      | 3 ++-
>>  4 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/command.h b/include/command.h
>> index 81d5a4d..1c2898a 100644
>> --- a/include/command.h
>> +++ b/include/command.h
>> @@ -22,6 +22,7 @@
>>  
>>  #define CMD_FLAG_GLOBAL		(1<<31)	/* don't iterate "args" */
>>  #define CMD_FLAG_FOREIGN_OK	(1<<30)	/* command not restricted to XFS */
>> +#define CMD_FLAG_GENERIC	(1<<29) /* command is generic (help, quit) */
> 
> I don't think this belongs in include/command.h - it's an xfs_quota
> specific behaviour so I'd put this in quota/init.h:
> 
> #define CMD_SKIP_CHECK	(1<<0)	/* command is always run */
> 
>> diff --git a/quota/init.c b/quota/init.c
>> index d5d80c2..85931bf 100644
>> --- a/quota/init.c
>> +++ b/quota/init.c
>> @@ -104,7 +104,8 @@ init_check_command(
>>  	const cmdinfo_t	*ct)
>>  {
>>  	if (fs_path &&
>> -	    !(ct->flags & CMD_FLAG_FOREIGN_OK) &&
>> +	    !((ct->flags & CMD_FLAG_FOREIGN_OK) && foreign_allowed) &&
>> +	    !(ct->flags & CMD_FLAG_GENERIC) &&
>>  	     (fs_path->fs_flags & FS_FOREIGN)) {
> 
> This is now sufficiently confusing that it needs to be reworked to
> make the logic clear and easily commented. i.e. something like this:
> 
> static int
> init_check_command(
> 	const cmdinfo_t *ct)
> {
> 	if (!fspath)
> 		return 1;
> 
> 	/* Always run commands that we are told to skip here */
> 	if (ct->flags & CMD_SKIP_CHECK)
> 		return 1;
> 
> 	/* if it's an XFS filesystem, always run the command */
> 	if (!(fs_path->fs_flags & FS_FOREIGN))
> 		return 1;

Sorry for the late review; thanks for getting on it, Dave - but,
isn't "foreign ok" exactly == "skip check?"

The only check that gets skipped is the foreign check, so just
setting FOREIGN_OK seems to accomplish the same thing without
more flag complexity, no?

Thanks,
-Eric


> 	/* If the user specified foreign filesysetms are ok, run it */
> 	if (foreign_allowed &&
> 	    (ct->flags & CMD_FLAG_FOREIGN_OK))
> 		return 1;
> 
> 	/* foreign filesystem and it's no a valid command! */
> 	fprintf(stderr, _("%s command is for XFS filesystems only\n"),
> 			ct->name);
> 	return 0;
> }
> 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 3/3] xfs_quota: additional changes to allow use on ext4

[Eric Sandeen]

On 8/21/16 10:34 PM, Eric Sandeen wrote:
> On 8/21/16 9:06 PM, Dave Chinner wrote:
>> On Tue, Aug 16, 2016 at 09:16:38AM -0500, Bill O'Donnell wrote:

...

>> static int
>> init_check_command(
>> 	const cmdinfo_t *ct)
>> {
>> 	if (!fspath)
>> 		return 1;
>>
>> 	/* Always run commands that we are told to skip here */
>> 	if (ct->flags & CMD_SKIP_CHECK)
>> 		return 1;
>>
>> 	/* if it's an XFS filesystem, always run the command */
>> 	if (!(fs_path->fs_flags & FS_FOREIGN))
>> 		return 1;
> 
> Sorry for the late review; thanks for getting on it, Dave - but,
> isn't "foreign ok" exactly == "skip check?"
> 
> The only check that gets skipped is the foreign check, so just
> setting FOREIGN_OK seems to accomplish the same thing without
> more flag complexity, no?

Oh, the subliminal brain reminded me that we want to be able to
issue help or quit whether or not we had the "-f" flag, regardless
of the filesystem, and that "foreign" isn't ok unless the -f flag
is set, so we do need a class of "always works" commands.

I guess that was the point of the patch, but I suppose some clarity
in comments or commitlog would help slow people like me.  ;)

Thanks,
-Eric

> Thanks,
> -Eric
> 
> 
>> 	/* If the user specified foreign filesysetms are ok, run it */
>> 	if (foreign_allowed &&
>> 	    (ct->flags & CMD_FLAG_FOREIGN_OK))
>> 		return 1;
>>
>> 	/* foreign filesystem and it's no a valid command! */
>> 	fprintf(stderr, _("%s command is for XFS filesystems only\n"),
>> 			ct->name);
>> 	return 0;
>> }
>>
>> Cheers,
>>
>> Dave.
>>
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 3/3] xfs_quota: additional changes to allow use on ext4

[Dave Chinner]

On Sun, Aug 21, 2016 at 11:46:14PM -0500, Eric Sandeen wrote:
> On 8/21/16 10:34 PM, Eric Sandeen wrote:
> > On 8/21/16 9:06 PM, Dave Chinner wrote:
> >> On Tue, Aug 16, 2016 at 09:16:38AM -0500, Bill O'Donnell wrote:
> 
> ...
> 
> >> static int
> >> init_check_command(
> >> 	const cmdinfo_t *ct)
> >> {
> >> 	if (!fspath)
> >> 		return 1;
> >>
> >> 	/* Always run commands that we are told to skip here */
> >> 	if (ct->flags & CMD_SKIP_CHECK)
> >> 		return 1;
> >>
> >> 	/* if it's an XFS filesystem, always run the command */
> >> 	if (!(fs_path->fs_flags & FS_FOREIGN))
> >> 		return 1;
> > 
> > Sorry for the late review; thanks for getting on it, Dave - but,
> > isn't "foreign ok" exactly == "skip check?"
> > 
> > The only check that gets skipped is the foreign check, so just
> > setting FOREIGN_OK seems to accomplish the same thing without
> > more flag complexity, no?
> 
> Oh, the subliminal brain reminded me that we want to be able to
> issue help or quit whether or not we had the "-f" flag, regardless
> of the filesystem, and that "foreign" isn't ok unless the -f flag
> is set, so we do need a class of "always works" commands.

Right, but that was something that was done in patch 1/3. I pointed
out that no mention of it was made in the cmmit message there....

> I guess that was the point of the patch, but I suppose some clarity
> in comments or commitlog would help slow people like me.  ;)

Right. better explanations needed all round :P

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 2/3] xfs_quota: changes to accomodate hoisted ioctl defs

[Bill O'Donnell]

On Mon, Aug 22, 2016 at 11:18:12AM +1000, Dave Chinner wrote:
> On Tue, Aug 16, 2016 at 09:16:37AM -0500, Bill O'Donnell wrote:
> > The kernel commit to make the ioctl promotion (334e580a6f) moved
> > the definitions for the XFS ioctl to uapi/linux/fs.h for the
> > following reason:
> > 
> >     Hoist the ioctl definitions for the XFS_IOC_FS[SG]SETXATTR API
> >     from fs/xfs/libxfs/xfs_fs.h to include/uapi/linux/fs.h so that
> >     the ioctls can be used by all filesystems, not just XFS. This
> >     enables (initially) ext4 to use the ioctl to set project IDs on
> >     inodes.
> > 
> > This means we now need to handle this change in userspace as the
> > uapi/linux/fs.h file may not contain the definitions (i.e. new
> > xfsprogs/ old linux uapi files) xfsprogs needs to build. Hence we
> > need to massage the definition in xfs_fs.h to take the values from
> > the system header if it exists, otherwise keep the old definitions
> > for compatibility and platforms other than linux..
> > 
> > This patch was originally submitted by Dave Chinner
> > (http://oss.sgi.com/archives/xfs/2016-02/msg00108.html)
> > 
> > Resubmitting with changes to accomodate upstream changes since
> > initial submission.
> 
> This is unnecessary - the "include local version" changes were made
> to the platform headers (e.g. include/linux.h) in commit 83f4b5a
> ("xfs_fs.h: XFS_IOC_FS[SG]SETXATTR to FS_IOC_FS[SG]ETXATTR
> promotion").

Sorry, I missed that. I'll remove this patch on v3.
Thanks-
Bill

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 3/3] xfs_quota: additional changes to allow use on ext4

[Bill O'Donnell]

On Mon, Aug 22, 2016 at 03:34:55PM +1000, Dave Chinner wrote:
> On Sun, Aug 21, 2016 at 11:46:14PM -0500, Eric Sandeen wrote:
> > On 8/21/16 10:34 PM, Eric Sandeen wrote:
> > > On 8/21/16 9:06 PM, Dave Chinner wrote:
> > >> On Tue, Aug 16, 2016 at 09:16:38AM -0500, Bill O'Donnell wrote:
> > 
> > ...
> > 
> > >> static int
> > >> init_check_command(
> > >> 	const cmdinfo_t *ct)
> > >> {
> > >> 	if (!fspath)
> > >> 		return 1;
> > >>
> > >> 	/* Always run commands that we are told to skip here */
> > >> 	if (ct->flags & CMD_SKIP_CHECK)
> > >> 		return 1;
> > >>
> > >> 	/* if it's an XFS filesystem, always run the command */
> > >> 	if (!(fs_path->fs_flags & FS_FOREIGN))
> > >> 		return 1;
> > > 
> > > Sorry for the late review; thanks for getting on it, Dave - but,
> > > isn't "foreign ok" exactly == "skip check?"
> > > 
> > > The only check that gets skipped is the foreign check, so just
> > > setting FOREIGN_OK seems to accomplish the same thing without
> > > more flag complexity, no?
> > 
> > Oh, the subliminal brain reminded me that we want to be able to
> > issue help or quit whether or not we had the "-f" flag, regardless
> > of the filesystem, and that "foreign" isn't ok unless the -f flag
> > is set, so we do need a class of "always works" commands.
> 
> Right, but that was something that was done in patch 1/3. I pointed
> out that no mention of it was made in the cmmit message there....
> 
> > I guess that was the point of the patch, but I suppose some clarity
> > in comments or commitlog would help slow people like me.  ;)
> 
> Right. better explanations needed all round :P

Yes, I'll clean it up in v3.
Thanks-
Bill


> 
> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 1/3] xfs_quota: add capabilities for use on ext4

[Bill O'Donnell]

On Mon, Aug 22, 2016 at 11:47:42AM +1000, Dave Chinner wrote:
> On Tue, Aug 16, 2016 at 09:16:36AM -0500, Bill O'Donnell wrote:
> > This allows xfs_quota to be used on ext4 for project quota testing
> > in xfstests.
> > 
> > This patch was originally submitted by Dave Chinner
> > (http://oss.sgi.com/archives/xfs/2016-02/msg00131.html)
> > 
> > Resubmitting with the following change:
> > quota/init.c: correct logic error in loop contained in init_args_command()
> > function (lines 85-91).
> 
> What logic error?

In your original patch, in init_args_command():

   	do {
 		fs_path = &fs_table[index++];
-	} while ((fs_path->fs_flags & FS_PROJECT_PATH) && index < fs_count);
+		if (fs_path->fs_flags & FS_PROJECT_PATH)
+			continue;
+		if (!foreign_allowed && (fs_path->fs_flags & FS_FOREIGN))
+			continue;
+	} while (index < fs_count);

The loop should break out, when (fs_path->fs_flags & FS_PROJECT_PATH) is false,
but instead moves onto the next test (and then back to the top). See in the
original while statement, the loop stops when the false condition occurs, that is,
((fs_path->fs_flags & FS_PROJECT_PATH) && index < fs_count) == False.

My commit message was completely terse, sorry. I'll clarify it in v3.

Thanks-
Bill

> 
> Commit messages like this really don't tell the reader anything
> about what is different the original patch.  I've had to go archive
> spelunking to work out what is different, and I'm still not sure
> what the logic error you fixed is....
> 
> And, FWIW, whilst spelunking, I noticed that Eric's last review
> comments on my original patch:
> 
> 	Looks ok, but now with the new option:
> 
> 	1) needs a manpage update
> 	2) usage() should be updated to include -f
> 
> 	3) and I just noticed,
> 
> 	_("foreign mount active, %s command is for XFS filesystems only\n"),
> 
> 	seems kind of unclear; maybe just
> 
> 	_("%s command is for XFS filesystems only\n"),
> 
> have not been addressed by this update.
> 
> Can you please add these these changes, update the commit message
> and resubmit?
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 1/3] xfs_quota: add capabilities for use on ext4

[Dave Chinner]

On Mon, Aug 22, 2016 at 11:02:07AM -0500, Bill O'Donnell wrote:
> On Mon, Aug 22, 2016 at 11:47:42AM +1000, Dave Chinner wrote:
> > On Tue, Aug 16, 2016 at 09:16:36AM -0500, Bill O'Donnell wrote:
> > > This allows xfs_quota to be used on ext4 for project quota testing
> > > in xfstests.
> > > 
> > > This patch was originally submitted by Dave Chinner
> > > (http://oss.sgi.com/archives/xfs/2016-02/msg00131.html)
> > > 
> > > Resubmitting with the following change:
> > > quota/init.c: correct logic error in loop contained in init_args_command()
> > > function (lines 85-91).
> > 
> > What logic error?
> 
> In your original patch, in init_args_command():
> 
>    	do {
>  		fs_path = &fs_table[index++];
> -	} while ((fs_path->fs_flags & FS_PROJECT_PATH) && index < fs_count);
> +		if (fs_path->fs_flags & FS_PROJECT_PATH)
> +			continue;
> +		if (!foreign_allowed && (fs_path->fs_flags & FS_FOREIGN))
> +			continue;
> +	} while (index < fs_count);
> 
> The loop should break out, when (fs_path->fs_flags & FS_PROJECT_PATH) is false,
> but instead moves onto the next test (and then back to the top). See in the
> original while statement, the loop stops when the false condition occurs, that is,
> ((fs_path->fs_flags & FS_PROJECT_PATH) && index < fs_count) == False.

Hi Bill - now I see *how* you changed the logic changed, but I still
don't know *why* it needed to be changed. What problem did you
encounter that needed to be solved?  The code I wrote had different
logic for good reason: the fstable is now populated with non-XFS
mount points now, and so we have to walk it differently.

In more detail, the fstable is initialised from two place - the
mount table for FS_MOUNT_POINT entries, and the projects files for
FS_PROJECT_PATH entries.  The original init_args_command() code was
searching for the first mount point entry in the filesystem table
(i.e. a FS_MOUNT_POINT entry) and it did so by skipping over
FS_PROJECT_PATH entries. It could do this because it "knew" that the
fs_table was only populated with XFS filesystem mount points. Hence
once it found an entry that was not a project quota path entry, it
could stop knowing it had an XFS mount point to work from.

Now we are populating the fstable with all types of filesystem mount
points as well as project quota paths. Hence if we are operating
only on XFS filesystems we now have to skip over any foreign mount
point entries we find in the table.  IOWs, the original code I wrote
is supposed to skip both project paths and foreign mounts when "-f"
is not set.

But that said, I've analysed your change sufficiently that I can now
see the problem you tried to solve: it doesn't break out of the
search loop when it finds the first mount point it can use. This is
the "why" of the logic change you made, and if you said that in the
commit message, it would have been easy to spot it in the patch.

It would have also been much easier to review, because now it's
clear that the logic change you've made makes it stop searching at
the first FS_MOUNT_POINT entry, regardless of whether it is foreign
or not, or whether we are allowing foreign mounts to be used. This
is incorrect behaviour, as you can now see from the above
explanation of what the code was supposed to be doing.

i.e. the search loop should now look something like this:

	/* lookup the first FS_MOUNT_POINT entry we should use */
	do {
		/* skip project quota entries */
		if (fs_path->fs_flags & FS_PROJECT_PATH)
			continue;

		/* only consider foreign filesystems if told to */
		if (!foreign_allowed && (fs_path->fs_flags & FS_FOREIGN))
			continue;

		/* We can use this one */
		break;
	} while (index < fs_count);

> My commit message was completely terse, sorry. I'll clarify it in v3.

Writing good commit messages is hard and takes practice. If you
read the commit message and you can't answer the following questions
after reading it, the commit message needs more work:

	1 what problem is being solved?
	2 why did the problem need to be solved?
	3 what unforeseen or tricky issues needed to be addressed
	  while solving the problem?
	4 what changed from the last version, and why did it change?
	  (see 1, 2 and 3)

Note that there's no "how did the problem get solved?" in that list?
That's because the "how?" is the code. Reviewers can read the code
change to understand the how - what they can't get from the code is
the "why?" and "what changed from last time?" and that's what needs
to be in comments and commit messages...

Often 1) and 2) can be described in the patch series summary (i.e.
patch 0/N) as it doesn't need to be explained in every patch in the
series.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 1/3] xfs_quota: add capabilities for use on ext4

[Bill O'Donnell]

On Tue, Aug 23, 2016 at 10:09:16AM +1000, Dave Chinner wrote:
> On Mon, Aug 22, 2016 at 11:02:07AM -0500, Bill O'Donnell wrote:
> > On Mon, Aug 22, 2016 at 11:47:42AM +1000, Dave Chinner wrote:
> > > On Tue, Aug 16, 2016 at 09:16:36AM -0500, Bill O'Donnell wrote:
> > > > This allows xfs_quota to be used on ext4 for project quota testing
> > > > in xfstests.
> > > > 
> > > > This patch was originally submitted by Dave Chinner
> > > > (http://oss.sgi.com/archives/xfs/2016-02/msg00131.html)
> > > > 
> > > > Resubmitting with the following change:
> > > > quota/init.c: correct logic error in loop contained in init_args_command()
> > > > function (lines 85-91).
> > > 
> > > What logic error?
> > 
> > In your original patch, in init_args_command():
> > 
> >    	do {
> >  		fs_path = &fs_table[index++];
> > -	} while ((fs_path->fs_flags & FS_PROJECT_PATH) && index < fs_count);
> > +		if (fs_path->fs_flags & FS_PROJECT_PATH)
> > +			continue;
> > +		if (!foreign_allowed && (fs_path->fs_flags & FS_FOREIGN))
> > +			continue;
> > +	} while (index < fs_count);
> > 
> > The loop should break out, when (fs_path->fs_flags & FS_PROJECT_PATH) is false,
> > but instead moves onto the next test (and then back to the top). See in the
> > original while statement, the loop stops when the false condition occurs, that is,
> > ((fs_path->fs_flags & FS_PROJECT_PATH) && index < fs_count) == False.
> 
> Hi Bill - now I see *how* you changed the logic changed, but I still
> don't know *why* it needed to be changed. What problem did you
> encounter that needed to be solved?  The code I wrote had different
> logic for good reason: the fstable is now populated with non-XFS
> mount points now, and so we have to walk it differently.
> 
> In more detail, the fstable is initialised from two place - the
> mount table for FS_MOUNT_POINT entries, and the projects files for
> FS_PROJECT_PATH entries.  The original init_args_command() code was
> searching for the first mount point entry in the filesystem table
> (i.e. a FS_MOUNT_POINT entry) and it did so by skipping over
> FS_PROJECT_PATH entries. It could do this because it "knew" that the
> fs_table was only populated with XFS filesystem mount points. Hence
> once it found an entry that was not a project quota path entry, it
> could stop knowing it had an XFS mount point to work from.
> 
> Now we are populating the fstable with all types of filesystem mount
> points as well as project quota paths. Hence if we are operating
> only on XFS filesystems we now have to skip over any foreign mount
> point entries we find in the table.  IOWs, the original code I wrote
> is supposed to skip both project paths and foreign mounts when "-f"
> is not set.
> 
> But that said, I've analysed your change sufficiently that I can now
> see the problem you tried to solve: it doesn't break out of the
> search loop when it finds the first mount point it can use. This is
> the "why" of the logic change you made, and if you said that in the
> commit message, it would have been easy to spot it in the patch.
> 
> It would have also been much easier to review, because now it's
> clear that the logic change you've made makes it stop searching at
> the first FS_MOUNT_POINT entry, regardless of whether it is foreign
> or not, or whether we are allowing foreign mounts to be used. This
> is incorrect behaviour, as you can now see from the above
> explanation of what the code was supposed to be doing.
> 
> i.e. the search loop should now look something like this:
> 
> 	/* lookup the first FS_MOUNT_POINT entry we should use */
> 	do {
> 		/* skip project quota entries */
> 		if (fs_path->fs_flags & FS_PROJECT_PATH)
> 			continue;
> 
> 		/* only consider foreign filesystems if told to */
> 		if (!foreign_allowed && (fs_path->fs_flags & FS_FOREIGN))
> 			continue;
> 
> 		/* We can use this one */
> 		break;
> 	} while (index < fs_count);
> 
> > My commit message was completely terse, sorry. I'll clarify it in v3.
> 
> Writing good commit messages is hard and takes practice. If you
> read the commit message and you can't answer the following questions
> after reading it, the commit message needs more work:
> 
> 	1 what problem is being solved?
> 	2 why did the problem need to be solved?
> 	3 what unforeseen or tricky issues needed to be addressed
> 	  while solving the problem?
> 	4 what changed from the last version, and why did it change?
> 	  (see 1, 2 and 3)
> 
> Note that there's no "how did the problem get solved?" in that list?
> That's because the "how?" is the code. Reviewers can read the code
> change to understand the how - what they can't get from the code is
> the "why?" and "what changed from last time?" and that's what needs
> to be in comments and commit messages...
> 
> Often 1) and 2) can be described in the patch series summary (i.e.
> patch 0/N) as it doesn't need to be explained in every patch in the
> series.

Hi Dave-

Thanks for your thorough review. I do appreciate it, and I'll fix up
things in v3.

Cheers-
Bill


> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs