[PATCH 0/3] Add a new quotactl command to support 3 quota types in XFS

[PATCH 0/3] Add a new quotactl command to support 3 quota types in XFS

[Chandra Seetharaman]

Hello All,

XFS now supports simultaneous use of 3 quota types (user, group,
and project).

In order to support this a new quotactl command is introduced
in this patch series.

Regards,

Chandra
---
Chandra Seetharaman (3):
  quota: Add a new quotactl command Q_XGETQSTATV
  xfs: Add support for the Q_XGETQSTATV
  gfs2: Add support for the Q_XGETQSTATV

 fs/gfs2/quota.c                |   27 +++++++++++++
 fs/quota/quota.c               |   29 ++++++++++++++
 fs/xfs/xfs_qm.h                |    2 +
 fs/xfs/xfs_qm_syscalls.c       |   82 ++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_quotaops.c          |   13 ++++++
 include/linux/quota.h          |    1 +
 include/uapi/linux/dqblk_xfs.h |   46 ++++++++++++++++++++++
 7 files changed, 200 insertions(+), 0 deletions(-)


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

[PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Chandra Seetharaman]

XFS now supports three types of quotas (user, group and project).

Current version of Q_XGETSTAT has support for only two types of quotas.
In order to support three types of quotas, the interface, specifically
struct fs_quota_stat, need to be expanded. Current version of fs_quota_stat
does not allow expansion without breaking backward compatibility.

So, a quotactl command and new fs_quota_stat structure need to be added.

This patch adds a new command Q_XGETQSTATV to quotactl() which takes
a new data structure fs_quota_statv. This new data structure provides
support for future expansion and backward compatibility.

Callers of the new quotactl command have to set the version of the data
structure being passed, and kernel will fill as much data as requested.
If the kernel does not support the user-space provided version, EINVAL
will be returned. User-space can reduce the version number and call the same
quotactl again.

Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
---
 fs/quota/quota.c               |   29 +++++++++++++++++++++++++
 include/linux/quota.h          |    1 +
 include/uapi/linux/dqblk_xfs.h |   46 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index c7314f1..e61a460 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -27,6 +27,7 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd,
 	case Q_SYNC:
 	case Q_GETINFO:
 	case Q_XGETQSTAT:
+	case Q_XGETQSTATV:
 	case Q_XQUOTASYNC:
 		break;
 	/* allow to query information for dquots we "own" */
@@ -217,6 +218,31 @@ static int quota_getxstate(struct super_block *sb, void __user *addr)
 	return ret;
 }
 
+static int quota_getxstatev(struct super_block *sb, void __user *addr)
+{
+	struct fs_quota_statv fqs;
+	int ret;
+
+	if (!sb->s_qcop->get_xstatev)
+		return -ENOSYS;
+
+	memset(&fqs, 0, sizeof(fqs));
+	if (copy_from_user(&fqs, addr, 1)) /* Just read qs_version */
+		return -EFAULT;
+
+	/* If this kernel doesn't support user specified version, fail */
+	switch (fqs.qs_version) {
+	case FS_QSTATV_VERSION1:
+		break;
+	default:
+		return -EINVAL;
+	}
+	ret = sb->s_qcop->get_xstatev(sb, &fqs);
+	if (!ret && copy_to_user(addr, &fqs, sizeof(fqs)))
+		return -EFAULT;
+	return ret;
+}
+
 static int quota_setxquota(struct super_block *sb, int type, qid_t id,
 			   void __user *addr)
 {
@@ -293,6 +319,8 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id,
 		return quota_setxstate(sb, cmd, addr);
 	case Q_XGETQSTAT:
 		return quota_getxstate(sb, addr);
+	case Q_XGETQSTATV:
+		return quota_getxstatev(sb, addr);
 	case Q_XSETQLIM:
 		return quota_setxquota(sb, type, id, addr);
 	case Q_XGETQUOTA:
@@ -317,6 +345,7 @@ static int quotactl_cmd_write(int cmd)
 	case Q_GETINFO:
 	case Q_SYNC:
 	case Q_XGETQSTAT:
+	case Q_XGETQSTATV:
 	case Q_XGETQUOTA:
 	case Q_XQUOTASYNC:
 		return 0;
diff --git a/include/linux/quota.h b/include/linux/quota.h
index d133711..cc7494a 100644
--- a/include/linux/quota.h
+++ b/include/linux/quota.h
@@ -328,6 +328,7 @@ struct quotactl_ops {
 	int (*set_dqblk)(struct super_block *, struct kqid, struct fs_disk_quota *);
 	int (*get_xstate)(struct super_block *, struct fs_quota_stat *);
 	int (*set_xstate)(struct super_block *, unsigned int, int);
+	int (*get_xstatev)(struct super_block *, struct fs_quota_statv *);
 };
 
 struct quota_format_type {
diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
index 8655280..17f5063 100644
--- a/include/uapi/linux/dqblk_xfs.h
+++ b/include/uapi/linux/dqblk_xfs.h
@@ -38,6 +38,7 @@
 #define Q_XGETQSTAT	XQM_CMD(5)	/* get quota subsystem status */
 #define Q_XQUOTARM	XQM_CMD(6)	/* free disk space used by dquots */
 #define Q_XQUOTASYNC	XQM_CMD(7)	/* delalloc flush, updates dquots */
+#define Q_XGETQSTATV	XQM_CMD(8)	/* newer version of get quota */
 
 /*
  * fs_disk_quota structure:
@@ -163,4 +164,49 @@ typedef struct fs_quota_stat {
 	__u16		qs_iwarnlimit;	/* limit for num warnings */
 } fs_quota_stat_t;
 
+/*
+ * fs_quota_statv is the used by Q_XGETQSTATV for a given file system. It 
+ * provides a centralized way to get meta information about the quota
+ * subsystem. eg. space taken up for user, group, and project quotas, number
+ * of dquots currently incore.
+ *
+ * This version has proper versioning support with appropriate padding for
+ * future expansions, and ability to expand for future without creating any
+ * backwward compatibility issues.
+ *
+ * For Q_XGETQSTATV, user space caller need to specify fs_quota_statv.qs_version
+ * to the version of data they are interested in. Kernel will fill the data
+ * fields relevant to that version.
+ *
+ * If kernel does not support user space caller specified version, EINVAL will
+ * be returned. User space caller can then reduce the version number and retry
+ * the same command.
+ */
+#define FS_QSTATV_VERSION1	1	/* fs_quota_statv.qs_version */
+/*
+ * Some basic information about 'quota files' for Q_XGETQSTATV command
+ */
+struct fs_qfilestatv {
+	__u64		qfs_ino;	/* inode number */
+	__u64		qfs_nblks;	/* number of BBs 512-byte-blks */
+	__u32		qfs_nextents;	/* number of extents */
+	__u32		qfs_pad;	/* pad for 8-byte alignment */
+};
+
+struct fs_quota_statv {
+	__s8			qs_version;	/* version for future changes */
+	__u8			qs_pad1;	/* pad for 16bit alignment */
+	__u16			qs_flags;	/* FS_QUOTA_.* flags */
+	__u32			qs_incoredqs;	/* number of dquots incore */
+	struct fs_qfilestatv	qs_uquota;	/* user quota information */
+	struct fs_qfilestatv	qs_gquota;	/* group quota information */
+	struct fs_qfilestatv	qs_pquota;	/* project quota information */
+	__s32			qs_btimelimit;  /* limit for blks timer */
+	__s32			qs_itimelimit;  /* limit for inodes timer */
+	__s32			qs_rtbtimelimit;/* limit for rt blks timer */
+	__u16			qs_bwarnlimit;	/* limit for num warnings */
+	__u16			qs_iwarnlimit;	/* limit for num warnings */
+	__u64			qs_pad2[8];	/* for future proofing */
+};
+
 #endif	/* _LINUX_DQBLK_XFS_H */
-- 
1.7.1

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

[PATCH 2/3] xfs: Add support for the Q_XGETQSTATV

[Chandra Seetharaman]

For XFS, add support for Q_XGETQSTATV quotactl command.

Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
---
 fs/xfs/xfs_qm.h          |    2 +
 fs/xfs/xfs_qm_syscalls.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_quotaops.c    |   13 +++++++
 3 files changed, 97 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
index 579d6a0..670cd44 100644
--- a/fs/xfs/xfs_qm.h
+++ b/fs/xfs/xfs_qm.h
@@ -160,6 +160,8 @@ extern int		xfs_qm_scall_setqlim(struct xfs_mount *, xfs_dqid_t, uint,
 					struct fs_disk_quota *);
 extern int		xfs_qm_scall_getqstat(struct xfs_mount *,
 					struct fs_quota_stat *);
+extern int		xfs_qm_scall_getqstatv(struct xfs_mount *,
+					struct fs_quota_statv *);
 extern int		xfs_qm_scall_quotaon(struct xfs_mount *, uint);
 extern int		xfs_qm_scall_quotaoff(struct xfs_mount *, uint);
 
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 8d9e4c7..721aabe 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -406,6 +406,7 @@ xfs_qm_scall_quotaon(
 
 /*
  * Return quota status information, such as uquota-off, enforcements, etc.
+ * for Q_XGETQSTAT command.
  */
 int
 xfs_qm_scall_getqstat(
@@ -493,6 +494,87 @@ xfs_qm_scall_getqstat(
 	return 0;
 }
 
+/*
+ * Return quota status information, such as uquota-off, enforcements, etc.
+ * for Q_XGETQSTATV command, to support separate project quota field.
+ */
+int
+xfs_qm_scall_getqstatv(
+	struct xfs_mount	*mp,
+	struct fs_quota_statv	*out)
+{
+	struct xfs_quotainfo	*q = mp->m_quotainfo;
+	struct xfs_inode	*uip = NULL;
+	struct xfs_inode	*gip = NULL;
+	struct xfs_inode	*pip = NULL;
+	bool                    tempuqip = false;
+	bool                    tempgqip = false;
+	bool                    temppqip = false;
+
+	if (!xfs_sb_version_hasquota(&mp->m_sb)) {
+		out->qs_uquota.qfs_ino = NULLFSINO;
+		out->qs_gquota.qfs_ino = NULLFSINO;
+		out->qs_pquota.qfs_ino = NULLFSINO;
+		return (0);
+	}
+
+	out->qs_flags = (__uint16_t) xfs_qm_export_flags(mp->m_qflags &
+							(XFS_ALL_QUOTA_ACCT|
+							 XFS_ALL_QUOTA_ENFD));
+	out->qs_uquota.qfs_ino = mp->m_sb.sb_uquotino;
+	out->qs_gquota.qfs_ino = mp->m_sb.sb_gquotino;
+	out->qs_pquota.qfs_ino = mp->m_sb.sb_pquotino;
+
+	if (q) {
+		uip = q->qi_uquotaip;
+		gip = q->qi_gquotaip;
+		pip = q->qi_pquotaip;
+	}
+	if (!uip && mp->m_sb.sb_uquotino != NULLFSINO) {
+		if (xfs_iget(mp, NULL, mp->m_sb.sb_uquotino,
+					0, 0, &uip) == 0)
+			tempuqip = true;
+	}
+	if (!gip && mp->m_sb.sb_gquotino != NULLFSINO) {
+		if (xfs_iget(mp, NULL, mp->m_sb.sb_gquotino,
+					0, 0, &gip) == 0)
+			tempgqip = true;
+	}
+	if (!pip && mp->m_sb.sb_pquotino != NULLFSINO) {
+		if (xfs_iget(mp, NULL, mp->m_sb.sb_pquotino,
+					0, 0, &pip) == 0)
+			temppqip = true;
+	}
+	if (uip) {
+		out->qs_uquota.qfs_nblks = uip->i_d.di_nblocks;
+		out->qs_uquota.qfs_nextents = uip->i_d.di_nextents;
+		if (tempuqip)
+			IRELE(uip);
+	}
+
+	if (gip) {
+		out->qs_gquota.qfs_nblks = gip->i_d.di_nblocks;
+		out->qs_gquota.qfs_nextents = gip->i_d.di_nextents;
+		if (tempgqip)
+			IRELE(gip);
+	}
+	if (pip) {
+		out->qs_pquota.qfs_nblks = pip->i_d.di_nblocks;
+		out->qs_pquota.qfs_nextents = pip->i_d.di_nextents;
+		if (temppqip)
+			IRELE(pip);
+	}
+	if (q) {
+		out->qs_incoredqs = q->qi_dquots;
+		out->qs_btimelimit = q->qi_btimelimit;
+		out->qs_itimelimit = q->qi_itimelimit;
+		out->qs_rtbtimelimit = q->qi_rtbtimelimit;
+		out->qs_bwarnlimit = q->qi_bwarnlimit;
+		out->qs_iwarnlimit = q->qi_iwarnlimit;
+	}
+	return 0;
+}
+
 #define XFS_DQ_MASK \
 	(FS_DQ_LIMIT_MASK | FS_DQ_TIMER_MASK | FS_DQ_WARNS_MASK)
 
diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
index 20e30f9..fe945e5 100644
--- a/fs/xfs/xfs_quotaops.c
+++ b/fs/xfs/xfs_quotaops.c
@@ -54,6 +54,18 @@ xfs_fs_get_xstate(
 }
 
 STATIC int
+xfs_fs_get_xstatev(
+	struct super_block	*sb,
+	struct fs_quota_statv	*fqs)
+{
+	struct xfs_mount	*mp = XFS_M(sb);
+
+	if (!XFS_IS_QUOTA_RUNNING(mp))
+		return -ENOSYS;
+	return -xfs_qm_scall_getqstatv(mp, fqs);
+}
+
+STATIC int
 xfs_fs_set_xstate(
 	struct super_block	*sb,
 	unsigned int		uflags,
@@ -133,6 +145,7 @@ xfs_fs_set_dqblk(
 }
 
 const struct quotactl_ops xfs_quotactl_operations = {
+	.get_xstatev		= xfs_fs_get_xstatev,
 	.get_xstate		= xfs_fs_get_xstate,
 	.set_xstate		= xfs_fs_set_xstate,
 	.get_dqblk		= xfs_fs_get_dqblk,
-- 
1.7.1

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

[PATCH 3/3] gfs2: Add support for the Q_XGETQSTATV

[Chandra Seetharaman]

For gfs2, add support for Q_XGETQSTATV quotactl command.

Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>

---
 fs/gfs2/quota.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 3768c2f..57f246d 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -1462,6 +1462,32 @@ static int gfs2_quota_get_xstate(struct super_block *sb,
 	return 0;
 }
 
+static int gfs2_quota_get_xstatev(struct super_block *sb,
+				 struct fs_quota_statv *fqs)
+{
+	struct gfs2_sbd *sdp = sb->s_fs_info;
+
+	switch (sdp->sd_args.ar_quota) {
+	case GFS2_QUOTA_ON:
+		fqs->qs_flags |= (FS_QUOTA_UDQ_ENFD | FS_QUOTA_GDQ_ENFD);
+		/*FALLTHRU*/
+	case GFS2_QUOTA_ACCOUNT:
+		fqs->qs_flags |= (FS_QUOTA_UDQ_ACCT | FS_QUOTA_GDQ_ACCT);
+		break;
+	case GFS2_QUOTA_OFF:
+		break;
+	}
+
+	if (sdp->sd_quota_inode) {
+		fqs->qs_uquota.qfs_ino = GFS2_I(sdp->sd_quota_inode)->i_no_addr;
+		fqs->qs_uquota.qfs_nblks = sdp->sd_quota_inode->i_blocks;
+	}
+	fqs->qs_uquota.qfs_nextents = 1; /* unsupported */
+	fqs->qs_gquota = fqs->qs_uquota; /* its the same inode in both cases */
+	fqs->qs_incoredqs = atomic_read(&qd_lru_count);
+	return 0;
+}
+
 static int gfs2_get_dqblk(struct super_block *sb, struct kqid qid,
 			  struct fs_disk_quota *fdq)
 {
@@ -1604,6 +1630,7 @@ out_put:
 
 const struct quotactl_ops gfs2_quotactl_ops = {
 	.quota_sync     = gfs2_quota_sync,
+	.get_xstatev    = gfs2_quota_get_xstatev,
 	.get_xstate     = gfs2_quota_get_xstate,
 	.get_dqblk	= gfs2_get_dqblk,
 	.set_dqblk	= gfs2_set_dqblk,
-- 
1.7.1

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

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Rich Johnston]

Hey Chandra,

Nice addition to quotas.  Just a couple of comments.

On 08/06/2013 05:27 PM, Chandra Seetharaman wrote:

. . .
>
> +/*
> + * fs_quota_statv is the used by Q_XGETQSTATV for a given file system. It
Remove extra word       ^^^^

> + * provides a centralized way to get meta information about the quota
> + * subsystem. eg. space taken up for user, group, and project quotas, number
> + * of dquots currently incore.
> + *
> + * This version has proper versioning support with appropriate padding for
> + * future expansions, and ability to expand for future without creating any
> + * backwward compatibility issues.
backward is misspelled (extra w)

> + *
> + * For Q_XGETQSTATV, user space caller need to specify fs_quota_statv.qs_version
> + * to the version of data they are interested in. Kernel will fill the data
> + * fields relevant to that version.
> + *
> + * If kernel does not support user space caller specified version, EINVAL will
> + * be returned. User space caller can then reduce the version number and retry
> + * the same command.
> + */
This was a little difficult to follow, suggest rewording a little, 
something like:

Q_XGETQSTATV uses the passed in value of the requested version via
fs_quota_statv.qs_version to determine the return data layout of 
fs_quota_statv.


It would be nice to get this information documented in the quotactl(2)
manpage similar to Q_XGETQSTAT.

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Rich Johnston]

Hey Chandra,

Nice addition to quotas.  Just a couple of comments.

On 08/06/2013 05:27 PM, Chandra Seetharaman wrote:

. . .
>
> +/*
> + * fs_quota_statv is the used by Q_XGETQSTATV for a given file system. It
Remove extra word       ^^^^

> + * provides a centralized way to get meta information about the quota
> + * subsystem. eg. space taken up for user, group, and project quotas, number
> + * of dquots currently incore.
> + *
> + * This version has proper versioning support with appropriate padding for
> + * future expansions, and ability to expand for future without creating any
> + * backwward compatibility issues.
backward is misspelled (extra w)

> + *
> + * For Q_XGETQSTATV, user space caller need to specify fs_quota_statv.qs_version
> + * to the version of data they are interested in. Kernel will fill the data
> + * fields relevant to that version.
> + *
> + * If kernel does not support user space caller specified version, EINVAL will
> + * be returned. User space caller can then reduce the version number and retry
> + * the same command.
> + */
This was a little difficult to follow, suggest rewording a little, 
something like:

Q_XGETQSTATV uses the passed in value of the requested version via
fs_quota_statv.qs_version to determine the return data layout of 
fs_quota_statv.


It would be nice to get this information documented in the quotactl(2)
manpage similar to Q_XGETQSTAT.


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

Re: [PATCH 2/3] xfs: Add support for the Q_XGETQSTATV

[Rich Johnston]

Looks good:

Reviewed-by: Rich Johnston <rjohnston@sgi.com>

On 08/06/2013 05:27 PM, Chandra Seetharaman wrote:
> For XFS, add support for Q_XGETQSTATV quotactl command.
>
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> ---
>   fs/xfs/xfs_qm.h          |    2 +
>   fs/xfs/xfs_qm_syscalls.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++
>   fs/xfs/xfs_quotaops.c    |   13 +++++++
>   3 files changed, 97 insertions(+), 0 deletions(-)

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

Re: [PATCH 3/3] gfs2: Add support for the Q_XGETQSTATV

[Rich Johnston]

Looks good:

Reviewed-by: Rich Johnston <rjohnston@sgi.com>

On 08/06/2013 05:27 PM, Chandra Seetharaman wrote:
> For gfs2, add support for Q_XGETQSTATV quotactl command.
>
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
>
> ---
>   fs/gfs2/quota.c |   27 +++++++++++++++++++++++++++
>   1 files changed, 27 insertions(+), 0 deletions(-)
>

Re: [PATCH 3/3] gfs2: Add support for the Q_XGETQSTATV

[Rich Johnston]

Looks good:

Reviewed-by: Rich Johnston <rjohnston@sgi.com>

On 08/06/2013 05:27 PM, Chandra Seetharaman wrote:
> For gfs2, add support for Q_XGETQSTATV quotactl command.
>
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
>
> ---
>   fs/gfs2/quota.c |   27 +++++++++++++++++++++++++++
>   1 files changed, 27 insertions(+), 0 deletions(-)
>

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

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Chandra Seetharaman]

On Tue, 2013-08-13 at 15:42 -0500, Rich Johnston wrote:
> Hey Chandra,
> 
> Nice addition to quotas.  Just a couple of comments.
> 
> On 08/06/2013 05:27 PM, Chandra Seetharaman wrote:
> 
> . . .
> >
> > +/*
> > + * fs_quota_statv is the used by Q_XGETQSTATV for a given file system. It
> Remove extra word       ^^^^
> 
> > + * provides a centralized way to get meta information about the quota
> > + * subsystem. eg. space taken up for user, group, and project quotas, number
> > + * of dquots currently incore.
> > + *
> > + * This version has proper versioning support with appropriate padding for
> > + * future expansions, and ability to expand for future without creating any
> > + * backwward compatibility issues.
> backward is misspelled (extra w)
> 
> > + *
> > + * For Q_XGETQSTATV, user space caller need to specify fs_quota_statv.qs_version
> > + * to the version of data they are interested in. Kernel will fill the data
> > + * fields relevant to that version.
> > + *
> > + * If kernel does not support user space caller specified version, EINVAL will
> > + * be returned. User space caller can then reduce the version number and retry
> > + * the same command.
> > + */
> This was a little difficult to follow, suggest rewording a little, 
> something like:
> 
> Q_XGETQSTATV uses the passed in value of the requested version via
> fs_quota_statv.qs_version to determine the return data layout of 
> fs_quota_statv.
> 
> 

Will make all the suggested fixes.

> It would be nice to get this information documented in the quotactl(2)
> manpage similar to Q_XGETQSTAT.

Once it is in the kernel will work with Jan Kara to add the relevant
changes to user space.

Thanks

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

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Chandra Seetharaman]

On Tue, 2013-08-13 at 15:42 -0500, Rich Johnston wrote:
> Hey Chandra,
> 
> Nice addition to quotas.  Just a couple of comments.
> 
> On 08/06/2013 05:27 PM, Chandra Seetharaman wrote:
> 
> . . .
> >
> > +/*
> > + * fs_quota_statv is the used by Q_XGETQSTATV for a given file system. It
> Remove extra word       ^^^^
> 
> > + * provides a centralized way to get meta information about the quota
> > + * subsystem. eg. space taken up for user, group, and project quotas, number
> > + * of dquots currently incore.
> > + *
> > + * This version has proper versioning support with appropriate padding for
> > + * future expansions, and ability to expand for future without creating any
> > + * backwward compatibility issues.
> backward is misspelled (extra w)
> 
> > + *
> > + * For Q_XGETQSTATV, user space caller need to specify fs_quota_statv.qs_version
> > + * to the version of data they are interested in. Kernel will fill the data
> > + * fields relevant to that version.
> > + *
> > + * If kernel does not support user space caller specified version, EINVAL will
> > + * be returned. User space caller can then reduce the version number and retry
> > + * the same command.
> > + */
> This was a little difficult to follow, suggest rewording a little, 
> something like:
> 
> Q_XGETQSTATV uses the passed in value of the requested version via
> fs_quota_statv.qs_version to determine the return data layout of 
> fs_quota_statv.
> 
> 

Will make all the suggested fixes.

> It would be nice to get this information documented in the quotactl(2)
> manpage similar to Q_XGETQSTAT.

Once it is in the kernel will work with Jan Kara to add the relevant
changes to user space.

Thanks

Chandra
> 
> 
> _______________________________________________
> 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 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Jan Kara]

  Hi,

  Neither me nor linux-fsdevel has been CCed on this change. Please do that
next time. Now looking into the patch in xfs mailing list archive I have
one comment: You declare:
struct fs_quota_statv {
	__s8			qs_version;	/* version for future changes */
	__u8			qs_pad1;	/* pad for 16bit alignment */
	__u16			qs_flags;	/* FS_QUOTA_.* flags */
	__u32			qs_incoredqs;	/* number of dquots incore */
	struct fs_qfilestatv	qs_uquota;	/* user quota information */
	struct fs_qfilestatv	qs_gquota;	/* group quota information */
	struct fs_qfilestatv	qs_pquota;	/* project quota information */
	__s32			qs_btimelimit;  /* limit for blks timer */
	__s32			qs_itimelimit;  /* limit for inodes timer */
	__s32			qs_rtbtimelimit;/* limit for rt blks timer */
	__u16			qs_bwarnlimit;	/* limit for num warnings */
	__u16			qs_iwarnlimit;	/* limit for num warnings */
	__u64			qs_pad2[8];	/* for future proofing */
};

Now do you really need qs_pad2 field? Since the structure is properly
versioned now, even its size can vary between versions, cannot it?
Otherwise the patch looks fine.

								Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Rich Johnston]

On 08/13/2013 04:22 PM, Jan Kara wrote:
>    Hi,
>
>    Neither me nor linux-fsdevel has been CCed on this change. Please do that
> next time. Now looking into the patch in xfs mailing list archive I have
Did you mean the email or the commit header?

As far as I can see you and linux-fsdevel were CCed on this entire email 
thread.

--Rich

> one comment: You declare:
> struct fs_quota_statv {
> 	__s8			qs_version;	/* version for future changes */
> 	__u8			qs_pad1;	/* pad for 16bit alignment */
> 	__u16			qs_flags;	/* FS_QUOTA_.* flags */
> 	__u32			qs_incoredqs;	/* number of dquots incore */
> 	struct fs_qfilestatv	qs_uquota;	/* user quota information */
> 	struct fs_qfilestatv	qs_gquota;	/* group quota information */
> 	struct fs_qfilestatv	qs_pquota;	/* project quota information */
> 	__s32			qs_btimelimit;  /* limit for blks timer */
> 	__s32			qs_itimelimit;  /* limit for inodes timer */
> 	__s32			qs_rtbtimelimit;/* limit for rt blks timer */
> 	__u16			qs_bwarnlimit;	/* limit for num warnings */
> 	__u16			qs_iwarnlimit;	/* limit for num warnings */
> 	__u64			qs_pad2[8];	/* for future proofing */
> };
>
> Now do you really need qs_pad2 field? Since the structure is properly
> versioned now, even its size can vary between versions, cannot it?
> Otherwise the patch looks fine.
>
> 								Honza
>

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

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Chandra Seetharaman]

On Tue, 2013-08-13 at 23:22 +0200, Jan Kara wrote:
> Hi,
> 
>   Neither me nor linux-fsdevel has been CCed on this change. Please do that

Jan,

All the CC in the email you got were from my original email. I did CC
you and linux-fsdevel when I sent this patchset a week ago
(http://oss.sgi.com/archives/xfs/2013-08/msg00171.html). 

I am confused on what happened and how you didn't get the original
email. Just now I checked linux-fsdevel archive. I do not see it there
either. Bizarre. (May be something wrong in the way I used
git-send-email)

Sorry.

> next time. Now looking into the patch in xfs mailing list archive I have
> one comment: You declare:
> struct fs_quota_statv {
> 	__s8			qs_version;	/* version for future changes */
> 	__u8			qs_pad1;	/* pad for 16bit alignment */
> 	__u16			qs_flags;	/* FS_QUOTA_.* flags */
> 	__u32			qs_incoredqs;	/* number of dquots incore */
> 	struct fs_qfilestatv	qs_uquota;	/* user quota information */
> 	struct fs_qfilestatv	qs_gquota;	/* group quota information */
> 	struct fs_qfilestatv	qs_pquota;	/* project quota information */
> 	__s32			qs_btimelimit;  /* limit for blks timer */
> 	__s32			qs_itimelimit;  /* limit for inodes timer */
> 	__s32			qs_rtbtimelimit;/* limit for rt blks timer */
> 	__u16			qs_bwarnlimit;	/* limit for num warnings */
> 	__u16			qs_iwarnlimit;	/* limit for num warnings */
> 	__u64			qs_pad2[8];	/* for future proofing */
> };
> 
> Now do you really need qs_pad2 field? Since the structure is properly
> versioned now, even its size can vary between versions, cannot it?

Yes, it can.

I added the pad based on Dave Chinner's suggestion:

----------
> > Dave:
> > > > future enhancements, maybe we should add 64 bytes of empty
> > > > space at the end of the structure....
> > > Chandra:
> > > Since this version is fully backward compatible, I didn't think a 
> > > future pad was needed. Do you want me to add ?
> > Dave:
> > We only really need to change the structure version when we change
> > input parameters, the size or the shape of the structure being
> > passed in from userspace. If we add padding now, then we can expand
> > output of the call without needing to bump the version of the
> > structure. Old code simply won't know (or care) about the new output
> > in the region of the structure it considers empty padding....
> Chandra:
> Ok. I will all 64 bytes of additional padding at the end.
> Otherwise the patch looks fine.
> 
----------

His argument convinced me to add the padding. What do you think ?


> 								Honza
> 


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

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Jan Kara]

On Tue 13-08-13 17:39:04, Chandra Seetharaman wrote:
> On Tue, 2013-08-13 at 23:22 +0200, Jan Kara wrote:
> > Hi,
> > 
> >   Neither me nor linux-fsdevel has been CCed on this change. Please do that
> 
> Jan,
> 
> All the CC in the email you got were from my original email. I did CC
> you and linux-fsdevel when I sent this patchset a week ago
> (http://oss.sgi.com/archives/xfs/2013-08/msg00171.html). 
> 
> I am confused on what happened and how you didn't get the original
> email. Just now I checked linux-fsdevel archive. I do not see it there
> either. Bizarre. (May be something wrong in the way I used
> git-send-email)
> 
> Sorry.
  No problem.

> > next time. Now looking into the patch in xfs mailing list archive I have
> > one comment: You declare:
> > struct fs_quota_statv {
> > 	__s8			qs_version;	/* version for future changes */
> > 	__u8			qs_pad1;	/* pad for 16bit alignment */
> > 	__u16			qs_flags;	/* FS_QUOTA_.* flags */
> > 	__u32			qs_incoredqs;	/* number of dquots incore */
> > 	struct fs_qfilestatv	qs_uquota;	/* user quota information */
> > 	struct fs_qfilestatv	qs_gquota;	/* group quota information */
> > 	struct fs_qfilestatv	qs_pquota;	/* project quota information */
> > 	__s32			qs_btimelimit;  /* limit for blks timer */
> > 	__s32			qs_itimelimit;  /* limit for inodes timer */
> > 	__s32			qs_rtbtimelimit;/* limit for rt blks timer */
> > 	__u16			qs_bwarnlimit;	/* limit for num warnings */
> > 	__u16			qs_iwarnlimit;	/* limit for num warnings */
> > 	__u64			qs_pad2[8];	/* for future proofing */
> > };
> > 
> > Now do you really need qs_pad2 field? Since the structure is properly
> > versioned now, even its size can vary between versions, cannot it?
> 
> Yes, it can.
> 
> I added the pad based on Dave Chinner's suggestion:
  OK, makes sense. So I'm ok with the patch. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> 
> ----------
> > > Dave:
> > > > > future enhancements, maybe we should add 64 bytes of empty
> > > > > space at the end of the structure....
> > > > Chandra:
> > > > Since this version is fully backward compatible, I didn't think a 
> > > > future pad was needed. Do you want me to add ?
> > > Dave:
> > > We only really need to change the structure version when we change
> > > input parameters, the size or the shape of the structure being
> > > passed in from userspace. If we add padding now, then we can expand
> > > output of the call without needing to bump the version of the
> > > structure. Old code simply won't know (or care) about the new output
> > > in the region of the structure it considers empty padding....
> > Chandra:
> > Ok. I will all 64 bytes of additional padding at the end.
> > Otherwise the patch looks fine.
> > 
> ----------
> 
> His argument convinced me to add the padding. What do you think ?
> 
> 
> > 								Honza
> > 
> 
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Jan Kara]

On Tue 13-08-13 17:39:04, Chandra Seetharaman wrote:
> On Tue, 2013-08-13 at 23:22 +0200, Jan Kara wrote:
> > Hi,
> > 
> >   Neither me nor linux-fsdevel has been CCed on this change. Please do that
> 
> Jan,
> 
> All the CC in the email you got were from my original email. I did CC
> you and linux-fsdevel when I sent this patchset a week ago
> (http://oss.sgi.com/archives/xfs/2013-08/msg00171.html). 
> 
> I am confused on what happened and how you didn't get the original
> email. Just now I checked linux-fsdevel archive. I do not see it there
> either. Bizarre. (May be something wrong in the way I used
> git-send-email)
> 
> Sorry.
  No problem.

> > next time. Now looking into the patch in xfs mailing list archive I have
> > one comment: You declare:
> > struct fs_quota_statv {
> > 	__s8			qs_version;	/* version for future changes */
> > 	__u8			qs_pad1;	/* pad for 16bit alignment */
> > 	__u16			qs_flags;	/* FS_QUOTA_.* flags */
> > 	__u32			qs_incoredqs;	/* number of dquots incore */
> > 	struct fs_qfilestatv	qs_uquota;	/* user quota information */
> > 	struct fs_qfilestatv	qs_gquota;	/* group quota information */
> > 	struct fs_qfilestatv	qs_pquota;	/* project quota information */
> > 	__s32			qs_btimelimit;  /* limit for blks timer */
> > 	__s32			qs_itimelimit;  /* limit for inodes timer */
> > 	__s32			qs_rtbtimelimit;/* limit for rt blks timer */
> > 	__u16			qs_bwarnlimit;	/* limit for num warnings */
> > 	__u16			qs_iwarnlimit;	/* limit for num warnings */
> > 	__u64			qs_pad2[8];	/* for future proofing */
> > };
> > 
> > Now do you really need qs_pad2 field? Since the structure is properly
> > versioned now, even its size can vary between versions, cannot it?
> 
> Yes, it can.
> 
> I added the pad based on Dave Chinner's suggestion:
  OK, makes sense. So I'm ok with the patch. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> 
> ----------
> > > Dave:
> > > > > future enhancements, maybe we should add 64 bytes of empty
> > > > > space at the end of the structure....
> > > > Chandra:
> > > > Since this version is fully backward compatible, I didn't think a 
> > > > future pad was needed. Do you want me to add ?
> > > Dave:
> > > We only really need to change the structure version when we change
> > > input parameters, the size or the shape of the structure being
> > > passed in from userspace. If we add padding now, then we can expand
> > > output of the call without needing to bump the version of the
> > > structure. Old code simply won't know (or care) about the new output
> > > in the region of the structure it considers empty padding....
> > Chandra:
> > Ok. I will all 64 bytes of additional padding at the end.
> > Otherwise the patch looks fine.
> > 
> ----------
> 
> His argument convinced me to add the padding. What do you think ?
> 
> 
> > 								Honza
> > 
> 
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

Re: [PATCH 0/3] Add a new quotactl command to support 3 quota types in XFS

[Ben Myers]

Hi Chandra,

On Tue, Aug 06, 2013 at 05:27:06PM -0500, Chandra Seetharaman wrote:
> XFS now supports simultaneous use of 3 quota types (user, group,
> and project).
> 
> In order to support this a new quotactl command is introduced
> in this patch series.

I've applied patches 1 and 2 at git://oss.sgi.com/xfs/xfs.git, master branch.
I think patch 3 should go through the gfs2 tree.

Steven, I think if you want to pull in patch 3 you'll also need patch 1.  Maybe
the easiest way is to cherry pick it from the xfs tree?  If you do we'll have a
trivial merge conflict.  Anyway, it's af30cb446dd5 if you want.

Thanks,
	Ben

Re: [PATCH 0/3] Add a new quotactl command to support 3 quota types in XFS

[Ben Myers]

Hi Chandra,

On Tue, Aug 06, 2013 at 05:27:06PM -0500, Chandra Seetharaman wrote:
> XFS now supports simultaneous use of 3 quota types (user, group,
> and project).
> 
> In order to support this a new quotactl command is introduced
> in this patch series.

I've applied patches 1 and 2 at git://oss.sgi.com/xfs/xfs.git, master branch.
I think patch 3 should go through the gfs2 tree.

Steven, I think if you want to pull in patch 3 you'll also need patch 1.  Maybe
the easiest way is to cherry pick it from the xfs tree?  If you do we'll have a
trivial merge conflict.  Anyway, it's af30cb446dd5 if you want.

Thanks,
	Ben

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

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Christoph Hellwig]

Sorry for being late to the game, but I don not like the in-kernel
interface here at all.  Given that Q_XGETQSTATV is a strict superset
of Q_XGETQSTAT there is no need for the second method - just always
fill out the larger in-kernel structure and only copy the smaller
information to userspace for the Q_XGETSTAT case.  That keeps the amount
of code required in the implementations of the methods low and follows
the model used elsewhere in the kernel (e.g. stat and statfs)

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

Re: [PATCH 0/3] Add a new quotactl command to support 3 quota types in XFS

[Steven Whitehouse]

Hi,

On Tue, 2013-08-20 at 17:25 -0500, Ben Myers wrote:
> Hi Chandra,
> 
> On Tue, Aug 06, 2013 at 05:27:06PM -0500, Chandra Seetharaman wrote:
> > XFS now supports simultaneous use of 3 quota types (user, group,
> > and project).
> > 
> > In order to support this a new quotactl command is introduced
> > in this patch series.
> 
> I've applied patches 1 and 2 at git://oss.sgi.com/xfs/xfs.git, master branch.
> I think patch 3 should go through the gfs2 tree.
> 
> Steven, I think if you want to pull in patch 3 you'll also need patch 1.  Maybe
> the easiest way is to cherry pick it from the xfs tree?  If you do we'll have a
> trivial merge conflict.  Anyway, it's af30cb446dd5 if you want.
> 
> Thanks,
> 	Ben

I think it would be better if all the patches went through the same tree
to save issues with dependencies. I don't mind if you want to put the
gfs2 patches in the xfs tree to keep everything in the same place. There
are no current quota changes in the gfs2 tree, so there should not be a
conflict due to that.

Also, it would be good if Christoph's comments could be addressed too,

Steve.

Re: [PATCH 0/3] Add a new quotactl command to support 3 quota types in XFS

[Steven Whitehouse]

Hi,

On Tue, 2013-08-20 at 17:25 -0500, Ben Myers wrote:
> Hi Chandra,
> 
> On Tue, Aug 06, 2013 at 05:27:06PM -0500, Chandra Seetharaman wrote:
> > XFS now supports simultaneous use of 3 quota types (user, group,
> > and project).
> > 
> > In order to support this a new quotactl command is introduced
> > in this patch series.
> 
> I've applied patches 1 and 2 at git://oss.sgi.com/xfs/xfs.git, master branch.
> I think patch 3 should go through the gfs2 tree.
> 
> Steven, I think if you want to pull in patch 3 you'll also need patch 1.  Maybe
> the easiest way is to cherry pick it from the xfs tree?  If you do we'll have a
> trivial merge conflict.  Anyway, it's af30cb446dd5 if you want.
> 
> Thanks,
> 	Ben

I think it would be better if all the patches went through the same tree
to save issues with dependencies. I don't mind if you want to put the
gfs2 patches in the xfs tree to keep everything in the same place. There
are no current quota changes in the gfs2 tree, so there should not be a
conflict due to that.

Also, it would be good if Christoph's comments could be addressed too,

Steve.


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

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Jan Kara]

On Tue 20-08-13 23:43:57, Christoph Hellwig wrote:
> Sorry for being late to the game, but I don not like the in-kernel
> interface here at all.  Given that Q_XGETQSTATV is a strict superset
> of Q_XGETQSTAT there is no need for the second method - just always
> fill out the larger in-kernel structure and only copy the smaller
> information to userspace for the Q_XGETSTAT case.  That keeps the amount
> of code required in the implementations of the methods low and follows
> the model used elsewhere in the kernel (e.g. stat and statfs)
  Well, the trouble is with gquota vs pquota - previously we report in
qs_gquota field either group quotas or project quotas depending on what is
turned on. Generic quota code doesn't know this so xfs get_xstatev() would
have to recognize whether it is being called from the old Q_XGETSTAT
quotactl or from the new Q_XGETSTATV quotactl to know where to fill in
project quotas. And at that point you somewhat loose the elegancy of using
one interface - we could set qs_version to some special value so that
.get_xstatev() recognizes this and does the magic but that doesn't seem very
different from the extra call...

Some duplication could be certainly avoided within XFS itself.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Jan Kara]

On Tue 20-08-13 23:43:57, Christoph Hellwig wrote:
> Sorry for being late to the game, but I don not like the in-kernel
> interface here at all.  Given that Q_XGETQSTATV is a strict superset
> of Q_XGETQSTAT there is no need for the second method - just always
> fill out the larger in-kernel structure and only copy the smaller
> information to userspace for the Q_XGETSTAT case.  That keeps the amount
> of code required in the implementations of the methods low and follows
> the model used elsewhere in the kernel (e.g. stat and statfs)
  Well, the trouble is with gquota vs pquota - previously we report in
qs_gquota field either group quotas or project quotas depending on what is
turned on. Generic quota code doesn't know this so xfs get_xstatev() would
have to recognize whether it is being called from the old Q_XGETSTAT
quotactl or from the new Q_XGETSTATV quotactl to know where to fill in
project quotas. And at that point you somewhat loose the elegancy of using
one interface - we could set qs_version to some special value so that
.get_xstatev() recognizes this and does the magic but that doesn't seem very
different from the extra call...

Some duplication could be certainly avoided within XFS itself.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

Re: [PATCH 0/3] Add a new quotactl command to support 3 quota types in XFS

[Ben Myers]

Hi Steven,

On Wed, Aug 21, 2013 at 01:28:02PM +0100, Steven Whitehouse wrote:
> Hi,
> 
> On Tue, 2013-08-20 at 17:25 -0500, Ben Myers wrote:
> > Hi Chandra,
> > 
> > On Tue, Aug 06, 2013 at 05:27:06PM -0500, Chandra Seetharaman wrote:
> > > XFS now supports simultaneous use of 3 quota types (user, group,
> > > and project).
> > > 
> > > In order to support this a new quotactl command is introduced
> > > in this patch series.
> > 
> > I've applied patches 1 and 2 at git://oss.sgi.com/xfs/xfs.git, master branch.
> > I think patch 3 should go through the gfs2 tree.
> > 
> > Steven, I think if you want to pull in patch 3 you'll also need patch 1.  Maybe
> > the easiest way is to cherry pick it from the xfs tree?  If you do we'll have a
> > trivial merge conflict.  Anyway, it's af30cb446dd5 if you want.
> > 
> > Thanks,
> > 	Ben
> 
> I think it would be better if all the patches went through the same tree
> to save issues with dependencies. I don't mind if you want to put the
> gfs2 patches in the xfs tree to keep everything in the same place. There
> are no current quota changes in the gfs2 tree, so there should not be a
> conflict due to that.

Ok, we'll take that route.  Thanks.

> Also, it would be good if Christoph's comments could be addressed too,

Agreed.

-Ben

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

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Ben Myers]

Hey Jan, Christoph,

On Wed, Aug 21, 2013 at 03:01:52PM +0200, Jan Kara wrote:
> On Tue 20-08-13 23:43:57, Christoph Hellwig wrote:
> > Sorry for being late to the game, but I don not like the in-kernel
> > interface here at all.  Given that Q_XGETQSTATV is a strict superset
> > of Q_XGETQSTAT there is no need for the second method - just always
> > fill out the larger in-kernel structure and only copy the smaller
> > information to userspace for the Q_XGETSTAT case.  That keeps the amount
> > of code required in the implementations of the methods low and follows
> > the model used elsewhere in the kernel (e.g. stat and statfs)

So you don't like the addition of .get_xstatev in quotactl_ops, and
fs_quota_stat would need to match with fs_quota_statv, adding the project quota
to the end of the structure?

>   Well, the trouble is with gquota vs pquota - previously we report in
> qs_gquota field either group quotas or project quotas depending on what is
> turned on. Generic quota code doesn't know this so xfs get_xstatev() would
> have to recognize whether it is being called from the old Q_XGETSTAT
> quotactl or from the new Q_XGETSTATV quotactl to know where to fill in
> project quotas. And at that point you somewhat loose the elegancy of using
> one interface - we could set qs_version to some special value so that
> .get_xstatev() recognizes this and does the magic but that doesn't seem very
> different from the extra call...

IIUC to make this happen without the addition of .get_xstate in quotactl_ops,
.get_xstate could also grow an argument to determine whether it was called as
Q_XGETSTAT vs Q_XGETSTATV.  If called as Q_XGETSTATV it can look at qs_version
to determine how much to copy.  That might be a bit cleaner than the qs_version
magic number, as long as you don't mind changing the .get_xstate interface.

> Some duplication could be certainly avoided within XFS itself.

Chandra may have taken this route to limit the possibility of breaking the old
interface...

Anyway, please give a shout if I need to revert this.  I believe the commit
raced with the commentary.  ;)

Thanks,
Ben

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

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Christoph Hellwig]

On Wed, Aug 21, 2013 at 01:12:47PM -0500, Ben Myers wrote:
> So you don't like the addition of .get_xstatev in quotactl_ops, and
> fs_quota_stat would need to match with fs_quota_statv, adding the project quota
> to the end of the structure?

That was what I had in mind initially, before the additional
complication were pointed out, except that we don't need it to look
exactly the same - if we use put_user calls instead of copy_to_user that
layout doesn't have to be the same.

However it seems like going down the stat route and having a kquota_info
structure might be the better way to fully separate the in-kernel API
from the userspace ABI.

> >   Well, the trouble is with gquota vs pquota - previously we report in
> > qs_gquota field either group quotas or project quotas depending on what is
> > turned on. Generic quota code doesn't know this so xfs get_xstatev() would
> > have to recognize whether it is being called from the old Q_XGETSTAT
> > quotactl or from the new Q_XGETSTATV quotactl to know where to fill in
> > project quotas. And at that point you somewhat loose the elegancy of using
> > one interface - we could set qs_version to some special value so that
> > .get_xstatev() recognizes this and does the magic but that doesn't seem very
> > different from the extra call...
> 
> IIUC to make this happen without the addition of .get_xstate in quotactl_ops,
> .get_xstate could also grow an argument to determine whether it was called as
> Q_XGETSTAT vs Q_XGETSTATV.  If called as Q_XGETSTATV it can look at qs_version
> to determine how much to copy.  That might be a bit cleaner than the qs_version
> magic number, as long as you don't mind changing the .get_xstate interface.

I don't think we'd need that argument - the fs would always fill out
both fields, then the implementation of Q_XGETSTAT would:

 (1) fail if both group and project quota information is present
 (2) export the pquota fields as gqouta if only project quota is present

> Anyway, please give a shout if I need to revert this.  I believe the commit
> raced with the commentary.  ;)

As this is in-kernel only I don't think we need to revert anything, but
it would be nice to fix it before 3.12 is released.  I didn't exactly
race either, your reply to Jan made me look a it a bit more.

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

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Chandra Seetharaman]

Chrishtoph,

After we figured that there are ABI/API issues with adding pquota
information to the end (while using the same command), I posted what you
are suggesting now
(http://oss.sgi.com/archives/xfs/2013-07/msg00212.html) as I also do not
like redundant code. Please see Dave's comment at the link above in
which he wanted me to change the code so that the two commands are
totally independent. There was no objections to Dave's suggestion, so I
made the changes as he suggested.

On Wed, 2013-08-21 at 11:19 -0700, Christoph Hellwig wrote: 
> On Wed, Aug 21, 2013e at 01:12:47PM -0500, Ben Myers wrote:
> > So you don't like the addition of .get_xstatev in quotactl_ops, and
> > fs_quota_stat would need to match with fs_quota_statv, adding the project quota
> > to the end of the structure?
> 
> That was what I had in mind initially, before the additional
> complication were pointed out, except that we don't need it to look
> exactly the same - if we use put_user calls instead of copy_to_user that
> layout doesn't have to be the same.
> 
> However it seems like going down the stat route and having a kquota_info
> structure might be the better way to fully separate the in-kernel API
> from the userspace ABI.
> 
> > >   Well, the trouble is with gquota vs pquota - previously we report in
> > > qs_gquota field either group quotas or project quotas depending on what is
> > > turned on. Generic quota code doesn't know this so xfs get_xstatev() would
> > > have to recognize whether it is being called from the old Q_XGETSTAT
> > > quotactl or from the new Q_XGETSTATV quotactl to know where to fill in
> > > project quotas. And at that point you somewhat loose the elegancy of using
> > > one interface - we could set qs_version to some special value so that
> > > .get_xstatev() recognizes this and does the magic but that doesn't seem very
> > > different from the extra call...
> > 
> > IIUC to make this happen without the addition of .get_xstate in quotactl_ops,
> > .get_xstate could also grow an argument to determine whether it was called as
> > Q_XGETSTAT vs Q_XGETSTATV.  If called as Q_XGETSTATV it can look at qs_version
> > to determine how much to copy.  That might be a bit cleaner than the qs_version
> > magic number, as long as you don't mind changing the .get_xstate interface.
> 
> I don't think we'd need that argument - the fs would always fill out
> both fields, then the implementation of Q_XGETSTAT would:
> 
>  (1) fail if both group and project quota information is present

There was a discussion on this issue and it was decided to provide back
only gquota information when both quotas are present and Q_XGETSTAT
command was used (instead of failing, which will break API)

> (2) export the pquota fields as gqouta if only project quota is present
> 
> > Anyway, please give a shout if I need to revert this.  I believe the commit
> > raced with the commentary.  ;)
> 
> As this is in-kernel only I don't think we need to revert anything, but
> it would be nice to fix it before 3.12 is released.  I didn't exactly
> race either, your reply to Jan made me look a it a bit more.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Chandra Seetharaman]

Chrishtoph,

After we figured that there are ABI/API issues with adding pquota
information to the end (while using the same command), I posted what you
are suggesting now
(http://oss.sgi.com/archives/xfs/2013-07/msg00212.html) as I also do not
like redundant code. Please see Dave's comment at the link above in
which he wanted me to change the code so that the two commands are
totally independent. There was no objections to Dave's suggestion, so I
made the changes as he suggested.

On Wed, 2013-08-21 at 11:19 -0700, Christoph Hellwig wrote: 
> On Wed, Aug 21, 2013e at 01:12:47PM -0500, Ben Myers wrote:
> > So you don't like the addition of .get_xstatev in quotactl_ops, and
> > fs_quota_stat would need to match with fs_quota_statv, adding the project quota
> > to the end of the structure?
> 
> That was what I had in mind initially, before the additional
> complication were pointed out, except that we don't need it to look
> exactly the same - if we use put_user calls instead of copy_to_user that
> layout doesn't have to be the same.
> 
> However it seems like going down the stat route and having a kquota_info
> structure might be the better way to fully separate the in-kernel API
> from the userspace ABI.
> 
> > >   Well, the trouble is with gquota vs pquota - previously we report in
> > > qs_gquota field either group quotas or project quotas depending on what is
> > > turned on. Generic quota code doesn't know this so xfs get_xstatev() would
> > > have to recognize whether it is being called from the old Q_XGETSTAT
> > > quotactl or from the new Q_XGETSTATV quotactl to know where to fill in
> > > project quotas. And at that point you somewhat loose the elegancy of using
> > > one interface - we could set qs_version to some special value so that
> > > .get_xstatev() recognizes this and does the magic but that doesn't seem very
> > > different from the extra call...
> > 
> > IIUC to make this happen without the addition of .get_xstate in quotactl_ops,
> > .get_xstate could also grow an argument to determine whether it was called as
> > Q_XGETSTAT vs Q_XGETSTATV.  If called as Q_XGETSTATV it can look at qs_version
> > to determine how much to copy.  That might be a bit cleaner than the qs_version
> > magic number, as long as you don't mind changing the .get_xstate interface.
> 
> I don't think we'd need that argument - the fs would always fill out
> both fields, then the implementation of Q_XGETSTAT would:
> 
>  (1) fail if both group and project quota information is present

There was a discussion on this issue and it was decided to provide back
only gquota information when both quotas are present and Q_XGETSTAT
command was used (instead of failing, which will break API)

> (2) export the pquota fields as gqouta if only project quota is present
> 
> > Anyway, please give a shout if I need to revert this.  I believe the commit
> > raced with the commentary.  ;)
> 
> As this is in-kernel only I don't think we need to revert anything, but
> it would be nice to fix it before 3.12 is released.  I didn't exactly
> race either, your reply to Jan made me look a it a bit more.
> 
> _______________________________________________
> 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 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Chandra Seetharaman]

Christoph, Dave, Ben,

What you suggest I should do w.r.t this patch ?

regards,

Chandra

On Wed, 2013-08-21 at 14:15 -0700, Chandra Seetharaman wrote:
> Chrishtoph,
> 
> After we figured that there are ABI/API issues with adding pquota
> information to the end (while using the same command), I posted what you
> are suggesting now
> (http://oss.sgi.com/archives/xfs/2013-07/msg00212.html) as I also do not
> like redundant code. Please see Dave's comment at the link above in
> which he wanted me to change the code so that the two commands are
> totally independent. There was no objections to Dave's suggestion, so I
> made the changes as he suggested.
> 
> On Wed, 2013-08-21 at 11:19 -0700, Christoph Hellwig wrote: 
> > On Wed, Aug 21, 2013e at 01:12:47PM -0500, Ben Myers wrote:
> > > So you don't like the addition of .get_xstatev in quotactl_ops, and
> > > fs_quota_stat would need to match with fs_quota_statv, adding the project quota
> > > to the end of the structure?
> > 
> > That was what I had in mind initially, before the additional
> > complication were pointed out, except that we don't need it to look
> > exactly the same - if we use put_user calls instead of copy_to_user that
> > layout doesn't have to be the same.
> > 
> > However it seems like going down the stat route and having a kquota_info
> > structure might be the better way to fully separate the in-kernel API
> > from the userspace ABI.
> > 
> > > >   Well, the trouble is with gquota vs pquota - previously we report in
> > > > qs_gquota field either group quotas or project quotas depending on what is
> > > > turned on. Generic quota code doesn't know this so xfs get_xstatev() would
> > > > have to recognize whether it is being called from the old Q_XGETSTAT
> > > > quotactl or from the new Q_XGETSTATV quotactl to know where to fill in
> > > > project quotas. And at that point you somewhat loose the elegancy of using
> > > > one interface - we could set qs_version to some special value so that
> > > > .get_xstatev() recognizes this and does the magic but that doesn't seem very
> > > > different from the extra call...
> > > 
> > > IIUC to make this happen without the addition of .get_xstate in quotactl_ops,
> > > .get_xstate could also grow an argument to determine whether it was called as
> > > Q_XGETSTAT vs Q_XGETSTATV.  If called as Q_XGETSTATV it can look at qs_version
> > > to determine how much to copy.  That might be a bit cleaner than the qs_version
> > > magic number, as long as you don't mind changing the .get_xstate interface.
> > 
> > I don't think we'd need that argument - the fs would always fill out
> > both fields, then the implementation of Q_XGETSTAT would:
> > 
> >  (1) fail if both group and project quota information is present
> 
> There was a discussion on this issue and it was decided to provide back
> only gquota information when both quotas are present and Q_XGETSTAT
> command was used (instead of failing, which will break API)
> 
> > (2) export the pquota fields as gqouta if only project quota is present
> > 
> > > Anyway, please give a shout if I need to revert this.  I believe the commit
> > > raced with the commentary.  ;)
> > 
> > As this is in-kernel only I don't think we need to revert anything, but
> > it would be nice to fix it before 3.12 is released.  I didn't exactly
> > race either, your reply to Jan made me look a it a bit more.
> > 
> > _______________________________________________
> > 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 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Ben Myers]

Hey,

On Thu, Aug 29, 2013 at 12:00:00PM -0500, Chandra Seetharaman wrote:
> Christoph, Dave, Ben,
> 
> What you suggest I should do w.r.t this patch ?

My impression is that Christoph is ok with not reverting the patch for
now, but requested that some cleanups be made soonish.  I suggest you
let it stand as is for now, and try to address his suggestions in a
subsequent patch.

Christoph, could you make it clearer what you'd like seen to?

Thanks,
	Ben

Re: [PATCH 1/3] quota: Add a new quotactl command Q_XGETQSTATV

[Ben Myers]

Hey,

On Thu, Aug 29, 2013 at 12:00:00PM -0500, Chandra Seetharaman wrote:
> Christoph, Dave, Ben,
> 
> What you suggest I should do w.r.t this patch ?

My impression is that Christoph is ok with not reverting the patch for
now, but requested that some cleanups be made soonish.  I suggest you
let it stand as is for now, and try to address his suggestions in a
subsequent patch.

Christoph, could you make it clearer what you'd like seen to?

Thanks,
	Ben

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