[PATCH 0/5] xfsprogs: optimize -L/-U range calls for xfs

public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/5] xfsprogs: optimize -L/-U range calls for xfs_quota's dump/report
@ 2022-03-28 22:24 Andrey Albershteyn
  2022-03-28 22:24 ` [PATCH 1/5] xfs_quota: separate quota info acquisition into get_quota() Andrey Albershteyn
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Andrey Albershteyn @ 2022-03-28 22:24 UTC (permalink / raw)
  To: linux-xfs; +Cc: Andrey Albershteyn

The xfs_quota's 'report' and 'dump' commands have -L and -U
arguments for restricting quota querying to the range of
UIDs/GIDs/PIDs. The current implementation is using XFS_GETQUOTA to
get every ID in specified range. It doesn't perform well on wider
ranges. The XFS_GETNEXTQUOTA is used only when upper limit is not
specified.  Also, the fallback case (UIDs from /etc/passwd) doesn't
take into account range
restriction and outputs all users with quota.

First 3 patches do minor refactoring to split acquisition and
printing of the quota information. This is not that necessary, but
makes it easier to manipulate with acquired data.

The 4th one replaces XFS_GETQUOTA based loop with XFS_GETNEXTQUOTA
one. The latter returns ID of the next user/group/project with
non-empty quota. The ID is then used in further call.

The last patch adds range checks for fallback case when
XFS_GETNEXTQUOTA is not avaliable.

The fallback case will be also executed in case that empty range is
specified (e.g. -L <too high>), but will print nothing.

Andrey Albershteyn (5):
  xfs_quota: separate quota info acquisition into get_quota()
  xfs_quota: create fs_disk_quota_t on upper level
  xfs_quota: split get_quota() and report_mount()/dump_file()
  xfs_quota: utilize XFS_GETNEXTQUOTA for ranged calls in report/dump
  xfs_quota: apply -L/-U range limits in uid/gid/pid loops

 quota/report.c | 319 ++++++++++++++++++++++++-------------------------
 1 file changed, 156 insertions(+), 163 deletions(-)

-- 
2.27.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/5] xfs_quota: separate quota info acquisition into get_quota()
  2022-03-28 22:24 [PATCH 0/5] xfsprogs: optimize -L/-U range calls for xfs_quota's dump/report Andrey Albershteyn
@ 2022-03-28 22:24 ` Andrey Albershteyn
  2022-03-28 23:44   ` Darrick J. Wong
  2022-04-06 16:31   ` Christoph Hellwig
  2022-03-28 22:25 ` [PATCH 2/5] xfs_quota: create fs_disk_quota_t on upper level Andrey Albershteyn
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 14+ messages in thread
From: Andrey Albershteyn @ 2022-03-28 22:24 UTC (permalink / raw)
  To: linux-xfs; +Cc: Andrey Albershteyn

Both report_mount() and dump_file() have identical code to get quota
information. This could be used for further separation of the
functions.

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 quota/report.c | 49 +++++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 26 deletions(-)

diff --git a/quota/report.c b/quota/report.c
index 2eb5b5a9..97a89a92 100644
--- a/quota/report.c
+++ b/quota/report.c
@@ -59,16 +59,15 @@ report_help(void)
 "\n"));
 }
 
-static int 
-dump_file(
-	FILE		*fp,
+static int
+get_quota(
+	fs_disk_quota_t *d,
 	uint		id,
 	uint		*oid,
 	uint		type,
 	char		*dev,
 	int		flags)
 {
-	fs_disk_quota_t	d;
 	int		cmd;
 
 	if (flags & GETNEXTQUOTA_FLAG)
@@ -77,7 +76,7 @@ dump_file(
 		cmd = XFS_GETQUOTA;
 
 	/* Fall back silently if XFS_GETNEXTQUOTA fails, warn on XFS_GETQUOTA */
-	if (xfsquotactl(cmd, dev, type, id, (void *)&d) < 0) {
+	if (xfsquotactl(cmd, dev, type, id, (void *)d) < 0) {
 		if (errno != ENOENT && errno != ENOSYS && errno != ESRCH &&
 		    cmd == XFS_GETQUOTA)
 			perror("XFS_GETQUOTA");
@@ -85,12 +84,29 @@ dump_file(
 	}
 
 	if (oid) {
-		*oid = d.d_id;
+		*oid = d->d_id;
 		/* Did kernelspace wrap? */
 		if (*oid < id)
 			return 0;
 	}
 
+	return 1;
+}
+
+static int
+dump_file(
+	FILE		*fp,
+	uint		id,
+	uint		*oid,
+	uint		type,
+	char		*dev,
+	int		flags)
+{
+	fs_disk_quota_t	d;
+
+	if	(!get_quota(&d, id, oid, type, dev, flags))
+		return 0;
+
 	if (!d.d_blk_softlimit && !d.d_blk_hardlimit &&
 	    !d.d_ino_softlimit && !d.d_ino_hardlimit &&
 	    !d.d_rtb_softlimit && !d.d_rtb_hardlimit)
@@ -329,31 +345,12 @@ report_mount(
 {
 	fs_disk_quota_t	d;
 	time64_t	timer;
-	char		*dev = mount->fs_name;
 	char		c[8], h[8], s[8];
 	uint		qflags;
 	int		count;
-	int		cmd;
 
-	if (flags & GETNEXTQUOTA_FLAG)
-		cmd = XFS_GETNEXTQUOTA;
-	else
-		cmd = XFS_GETQUOTA;
-
-	/* Fall back silently if XFS_GETNEXTQUOTA fails, warn on XFS_GETQUOTA*/
-	if (xfsquotactl(cmd, dev, type, id, (void *)&d) < 0) {
-		if (errno != ENOENT && errno != ENOSYS && errno != ESRCH &&
-		    cmd == XFS_GETQUOTA)
-			perror("XFS_GETQUOTA");
+	if	(!get_quota(&d, id, oid, type, mount->fs_name, flags))
 		return 0;
-	}
-
-	if (oid) {
-		*oid = d.d_id;
-		/* Did kernelspace wrap? */
-		if (* oid < id)
-			return 0;
-	}
 
 	if (flags & TERSE_FLAG) {
 		count = 0;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] xfs_quota: separate quota info acquisition into get_quota()
  2022-03-28 22:24 ` [PATCH 1/5] xfs_quota: separate quota info acquisition into get_quota() Andrey Albershteyn
@ 2022-03-28 23:44   ` Darrick J. Wong
  2022-04-06 16:31   ` Christoph Hellwig
  1 sibling, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2022-03-28 23:44 UTC (permalink / raw)
  To: Andrey Albershteyn; +Cc: linux-xfs

On Tue, Mar 29, 2022 at 12:24:59AM +0200, Andrey Albershteyn wrote:
> Both report_mount() and dump_file() have identical code to get quota
> information. This could be used for further separation of the
> functions.
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  quota/report.c | 49 +++++++++++++++++++++++--------------------------
>  1 file changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/quota/report.c b/quota/report.c
> index 2eb5b5a9..97a89a92 100644
> --- a/quota/report.c
> +++ b/quota/report.c
> @@ -59,16 +59,15 @@ report_help(void)
>  "\n"));
>  }
>  
> -static int 
> -dump_file(
> -	FILE		*fp,
> +static int
> +get_quota(
> +	fs_disk_quota_t *d,

At first I confused this for a quotatools patch, but then I realized
that xfsprogs /and/ quotatools both define this structure.

Anyway... please use the non-typedef version of this, in keeping with
the current style:

static int
get_dquot(
	struct fs_disk_quota	*d,
	uint			id,
	...

Also I think this ought to be called get_dquot in keeping with (AFAICT)
the naming conventions: "dquot" for specific quota records vs. "quota"
to refer to the overall feature.

>  	uint		id,
>  	uint		*oid,
>  	uint		type,
>  	char		*dev,
>  	int		flags)
>  {
> -	fs_disk_quota_t	d;
>  	int		cmd;
>  
>  	if (flags & GETNEXTQUOTA_FLAG)
> @@ -77,7 +76,7 @@ dump_file(
>  		cmd = XFS_GETQUOTA;
>  
>  	/* Fall back silently if XFS_GETNEXTQUOTA fails, warn on XFS_GETQUOTA */
> -	if (xfsquotactl(cmd, dev, type, id, (void *)&d) < 0) {
> +	if (xfsquotactl(cmd, dev, type, id, (void *)d) < 0) {
>  		if (errno != ENOENT && errno != ENOSYS && errno != ESRCH &&
>  		    cmd == XFS_GETQUOTA)
>  			perror("XFS_GETQUOTA");
> @@ -85,12 +84,29 @@ dump_file(
>  	}
>  
>  	if (oid) {
> -		*oid = d.d_id;
> +		*oid = d->d_id;
>  		/* Did kernelspace wrap? */
>  		if (*oid < id)
>  			return 0;
>  	}
>  
> +	return 1;
> +}
> +
> +static int
> +dump_file(
> +	FILE		*fp,
> +	uint		id,
> +	uint		*oid,
> +	uint		type,
> +	char		*dev,
> +	int		flags)
> +{
> +	fs_disk_quota_t	d;
> +
> +	if	(!get_quota(&d, id, oid, type, dev, flags))

Please don't insert a whole tab after 'if'.

--D

> +		return 0;
> +
>  	if (!d.d_blk_softlimit && !d.d_blk_hardlimit &&
>  	    !d.d_ino_softlimit && !d.d_ino_hardlimit &&
>  	    !d.d_rtb_softlimit && !d.d_rtb_hardlimit)
> @@ -329,31 +345,12 @@ report_mount(
>  {
>  	fs_disk_quota_t	d;
>  	time64_t	timer;
> -	char		*dev = mount->fs_name;
>  	char		c[8], h[8], s[8];
>  	uint		qflags;
>  	int		count;
> -	int		cmd;
>  
> -	if (flags & GETNEXTQUOTA_FLAG)
> -		cmd = XFS_GETNEXTQUOTA;
> -	else
> -		cmd = XFS_GETQUOTA;
> -
> -	/* Fall back silently if XFS_GETNEXTQUOTA fails, warn on XFS_GETQUOTA*/
> -	if (xfsquotactl(cmd, dev, type, id, (void *)&d) < 0) {
> -		if (errno != ENOENT && errno != ENOSYS && errno != ESRCH &&
> -		    cmd == XFS_GETQUOTA)
> -			perror("XFS_GETQUOTA");
> +	if	(!get_quota(&d, id, oid, type, mount->fs_name, flags))
>  		return 0;
> -	}
> -
> -	if (oid) {
> -		*oid = d.d_id;
> -		/* Did kernelspace wrap? */
> -		if (* oid < id)
> -			return 0;
> -	}
>  
>  	if (flags & TERSE_FLAG) {
>  		count = 0;
> -- 
> 2.27.0
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] xfs_quota: separate quota info acquisition into get_quota()
  2022-03-28 22:24 ` [PATCH 1/5] xfs_quota: separate quota info acquisition into get_quota() Andrey Albershteyn
  2022-03-28 23:44   ` Darrick J. Wong
@ 2022-04-06 16:31   ` Christoph Hellwig
  1 sibling, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-04-06 16:31 UTC (permalink / raw)
  To: Andrey Albershteyn; +Cc: linux-xfs

On Tue, Mar 29, 2022 at 12:24:59AM +0200, Andrey Albershteyn wrote:
> Both report_mount() and dump_file() have identical code to get quota
> information. This could be used for further separation of the
> functions.
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  quota/report.c | 49 +++++++++++++++++++++++--------------------------
>  1 file changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/quota/report.c b/quota/report.c
> index 2eb5b5a9..97a89a92 100644
> --- a/quota/report.c
> +++ b/quota/report.c
> @@ -59,16 +59,15 @@ report_help(void)
>  "\n"));
>  }
>  
> -static int 
> -dump_file(
> -	FILE		*fp,
> +static int
> +get_quota(
> +	fs_disk_quota_t *d,
>  	uint		id,
>  	uint		*oid,
>  	uint		type,
>  	char		*dev,
>  	int		flags)
>  {
> -	fs_disk_quota_t	d;
>  	int		cmd;
>  
>  	if (flags & GETNEXTQUOTA_FLAG)
> @@ -77,7 +76,7 @@ dump_file(
>  		cmd = XFS_GETQUOTA;
>  
>  	/* Fall back silently if XFS_GETNEXTQUOTA fails, warn on XFS_GETQUOTA */
> -	if (xfsquotactl(cmd, dev, type, id, (void *)&d) < 0) {
> +	if (xfsquotactl(cmd, dev, type, id, (void *)d) < 0) {
>  		if (errno != ENOENT && errno != ENOSYS && errno != ESRCH &&
>  		    cmd == XFS_GETQUOTA)
>  			perror("XFS_GETQUOTA");
> @@ -85,12 +84,29 @@ dump_file(
>  	}
>  
>  	if (oid) {
> -		*oid = d.d_id;
> +		*oid = d->d_id;
>  		/* Did kernelspace wrap? */
>  		if (*oid < id)
>  			return 0;
>  	}
>  
> +	return 1;
> +}
> +
> +static int
> +dump_file(
> +	FILE		*fp,
> +	uint		id,
> +	uint		*oid,
> +	uint		type,
> +	char		*dev,
> +	int		flags)
> +{
> +	fs_disk_quota_t	d;
> +
> +	if	(!get_quota(&d, id, oid, type, dev, flags))

Tab instead of a space after the if here.

Also if you touch this anyway it might be worth to replace
fs_disk_quota_t with struct fs_disk_quota.

> +	if	(!get_quota(&d, id, oid, type, mount->fs_name, flags))

Same here.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/5] xfs_quota: create fs_disk_quota_t on upper level
  2022-03-28 22:24 [PATCH 0/5] xfsprogs: optimize -L/-U range calls for xfs_quota's dump/report Andrey Albershteyn
  2022-03-28 22:24 ` [PATCH 1/5] xfs_quota: separate quota info acquisition into get_quota() Andrey Albershteyn
@ 2022-03-28 22:25 ` Andrey Albershteyn
  2022-04-06 16:34   ` Christoph Hellwig
  2022-03-28 22:25 ` [PATCH 3/5] xfs_quota: split get_quota() and report_mount()/dump_file() Andrey Albershteyn
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Andrey Albershteyn @ 2022-03-28 22:25 UTC (permalink / raw)
  To: linux-xfs; +Cc: Andrey Albershteyn

For further splitting of get_quota() and dump_file()/report_mount().

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 quota/report.c | 165 +++++++++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 81 deletions(-)

diff --git a/quota/report.c b/quota/report.c
index 97a89a92..0462ce97 100644
--- a/quota/report.c
+++ b/quota/report.c
@@ -96,39 +96,38 @@ get_quota(
 static int
 dump_file(
 	FILE		*fp,
+	fs_disk_quota_t *d,
 	uint		id,
 	uint		*oid,
 	uint		type,
 	char		*dev,
 	int		flags)
 {
-	fs_disk_quota_t	d;
-
-	if	(!get_quota(&d, id, oid, type, dev, flags))
+	if	(!get_quota(d, id, oid, type, dev, flags))
 		return 0;
 
-	if (!d.d_blk_softlimit && !d.d_blk_hardlimit &&
-	    !d.d_ino_softlimit && !d.d_ino_hardlimit &&
-	    !d.d_rtb_softlimit && !d.d_rtb_hardlimit)
+	if (!d->d_blk_softlimit && !d->d_blk_hardlimit &&
+	    !d->d_ino_softlimit && !d->d_ino_hardlimit &&
+	    !d->d_rtb_softlimit && !d->d_rtb_hardlimit)
 		return 1;
 	fprintf(fp, "fs = %s\n", dev);
 	/* this branch is for backward compatibility reasons */
-	if (d.d_rtb_softlimit || d.d_rtb_hardlimit)
+	if (d->d_rtb_softlimit || d->d_rtb_hardlimit)
 		fprintf(fp, "%-10d %7llu %7llu %7llu %7llu %7llu %7llu\n",
-			d.d_id,
-			(unsigned long long)d.d_blk_softlimit,
-			(unsigned long long)d.d_blk_hardlimit,
-			(unsigned long long)d.d_ino_softlimit,
-			(unsigned long long)d.d_ino_hardlimit,
-			(unsigned long long)d.d_rtb_softlimit,
-			(unsigned long long)d.d_rtb_hardlimit);
+			d->d_id,
+			(unsigned long long)d->d_blk_softlimit,
+			(unsigned long long)d->d_blk_hardlimit,
+			(unsigned long long)d->d_ino_softlimit,
+			(unsigned long long)d->d_ino_hardlimit,
+			(unsigned long long)d->d_rtb_softlimit,
+			(unsigned long long)d->d_rtb_hardlimit);
 	else
 		fprintf(fp, "%-10d %7llu %7llu %7llu %7llu\n",
-			d.d_id,
-			(unsigned long long)d.d_blk_softlimit,
-			(unsigned long long)d.d_blk_hardlimit,
-			(unsigned long long)d.d_ino_softlimit,
-			(unsigned long long)d.d_ino_hardlimit);
+			d->d_id,
+			(unsigned long long)d->d_blk_softlimit,
+			(unsigned long long)d->d_blk_hardlimit,
+			(unsigned long long)d->d_ino_softlimit,
+			(unsigned long long)d->d_ino_hardlimit);
 
 	return 1;
 }
@@ -142,6 +141,7 @@ dump_limits_any_type(
 	uint		upper)
 {
 	fs_path_t	*mount;
+	fs_disk_quota_t d;
 	uint		id = 0, oid;
 
 	if ((mount = fs_table_lookup(dir, FS_MOUNT_POINT)) == NULL) {
@@ -154,14 +154,14 @@ dump_limits_any_type(
 	/* Range was specified; query everything in it */
 	if (upper) {
 		for (id = lower; id <= upper; id++)
-			dump_file(fp, id, NULL, type, mount->fs_name, 0);
+			dump_file(fp, &d, id, NULL, type, mount->fs_name, 0);
 		return;
 	}
 
 	/* Use GETNEXTQUOTA if it's available */
-	if (dump_file(fp, id, &oid, type, mount->fs_name, GETNEXTQUOTA_FLAG)) {
+	if (dump_file(fp, &d, id, &oid, type, mount->fs_name, GETNEXTQUOTA_FLAG)) {
 		id = oid + 1;
-		while (dump_file(fp, id, &oid, type, mount->fs_name,
+		while (dump_file(fp, &d, id, &oid, type, mount->fs_name,
 				 GETNEXTQUOTA_FLAG))
 			id = oid + 1;
 		return;
@@ -173,7 +173,7 @@ dump_limits_any_type(
 			struct group *g;
 			setgrent();
 			while ((g = getgrent()) != NULL)
-				dump_file(fp, g->gr_gid, NULL, type,
+				dump_file(fp, &d, g->gr_gid, NULL, type,
 					  mount->fs_name, 0);
 			endgrent();
 			break;
@@ -182,7 +182,7 @@ dump_limits_any_type(
 			struct fs_project *p;
 			setprent();
 			while ((p = getprent()) != NULL)
-				dump_file(fp, p->pr_prid, NULL, type,
+				dump_file(fp, &d, p->pr_prid, NULL, type,
 					  mount->fs_name, 0);
 			endprent();
 			break;
@@ -191,7 +191,7 @@ dump_limits_any_type(
 			struct passwd *u;
 			setpwent();
 			while ((u = getpwent()) != NULL)
-				dump_file(fp, u->pw_uid, NULL, type,
+				dump_file(fp, &d, u->pw_uid, NULL, type,
 					  mount->fs_name, 0);
 			endpwent();
 			break;
@@ -335,6 +335,7 @@ report_header(
 static int
 report_mount(
 	FILE		*fp,
+	fs_disk_quota_t *d,
 	uint32_t	id,
 	char		*name,
 	uint32_t	*oid,
@@ -343,22 +344,21 @@ report_mount(
 	fs_path_t	*mount,
 	uint		flags)
 {
-	fs_disk_quota_t	d;
 	time64_t	timer;
 	char		c[8], h[8], s[8];
 	uint		qflags;
 	int		count;
 
-	if	(!get_quota(&d, id, oid, type, mount->fs_name, flags))
+	if	(!get_quota(d, id, oid, type, mount->fs_name, flags))
 		return 0;
 
 	if (flags & TERSE_FLAG) {
 		count = 0;
-		if ((form & XFS_BLOCK_QUOTA) && d.d_bcount)
+		if ((form & XFS_BLOCK_QUOTA) && d->d_bcount)
 			count++;
-		if ((form & XFS_INODE_QUOTA) && d.d_icount)
+		if ((form & XFS_INODE_QUOTA) && d->d_icount)
 			count++;
-		if ((form & XFS_RTBLOCK_QUOTA) && d.d_rtbcount)
+		if ((form & XFS_RTBLOCK_QUOTA) && d->d_rtbcount)
 			count++;
 		if (!count)
 			return 0;
@@ -368,19 +368,19 @@ report_mount(
 		report_header(fp, form, type, mount, flags);
 
 	if (flags & NO_LOOKUP_FLAG) {
-		fprintf(fp, "#%-10u", d.d_id);
+		fprintf(fp, "#%-10u", d->d_id);
 	} else {
 		if (name == NULL) {
 			if (type == XFS_USER_QUOTA) {
-				struct passwd	*u = getpwuid(d.d_id);
+				struct passwd	*u = getpwuid(d->d_id);
 				if (u)
 					name = u->pw_name;
 			} else if (type == XFS_GROUP_QUOTA) {
-				struct group	*g = getgrgid(d.d_id);
+				struct group	*g = getgrgid(d->d_id);
 				if (g)
 					name = g->gr_name;
 			} else if (type == XFS_PROJ_QUOTA) {
-				fs_project_t	*p = getprprid(d.d_id);
+				fs_project_t	*p = getprprid(d->d_id);
 				if (p)
 					name = p->pr_name;
 			}
@@ -389,73 +389,73 @@ report_mount(
 		if (name != NULL)
 			fprintf(fp, "%-10s", name);
 		else
-			fprintf(fp, "#%-9u", d.d_id);
+			fprintf(fp, "#%-9u", d->d_id);
 	}
 
 	if (form & XFS_BLOCK_QUOTA) {
-		timer = decode_timer(&d, d.d_btimer, d.d_btimer_hi);
+		timer = decode_timer(d, d->d_btimer, d->d_btimer_hi);
 		qflags = (flags & HUMAN_FLAG);
-		if (d.d_blk_hardlimit && d.d_bcount > d.d_blk_hardlimit)
+		if (d->d_blk_hardlimit && d->d_bcount > d->d_blk_hardlimit)
 			qflags |= LIMIT_FLAG;
-		if (d.d_blk_softlimit && d.d_bcount > d.d_blk_softlimit)
+		if (d->d_blk_softlimit && d->d_bcount > d->d_blk_softlimit)
 			qflags |= QUOTA_FLAG;
 		if (flags & HUMAN_FLAG)
 			fprintf(fp, " %6s %6s %6s  %02d %8s",
-				bbs_to_string(d.d_bcount, c, sizeof(c)),
-				bbs_to_string(d.d_blk_softlimit, s, sizeof(s)),
-				bbs_to_string(d.d_blk_hardlimit, h, sizeof(h)),
-				d.d_bwarns,
+				bbs_to_string(d->d_bcount, c, sizeof(c)),
+				bbs_to_string(d->d_blk_softlimit, s, sizeof(s)),
+				bbs_to_string(d->d_blk_hardlimit, h, sizeof(h)),
+				d->d_bwarns,
 				time_to_string(timer, qflags));
 		else
 			fprintf(fp, " %10llu %10llu %10llu     %02d %9s",
-				(unsigned long long)d.d_bcount >> 1,
-				(unsigned long long)d.d_blk_softlimit >> 1,
-				(unsigned long long)d.d_blk_hardlimit >> 1,
-				d.d_bwarns,
+				(unsigned long long)d->d_bcount >> 1,
+				(unsigned long long)d->d_blk_softlimit >> 1,
+				(unsigned long long)d->d_blk_hardlimit >> 1,
+				d->d_bwarns,
 				time_to_string(timer, qflags));
 	}
 	if (form & XFS_INODE_QUOTA) {
-		timer = decode_timer(&d, d.d_itimer, d.d_itimer_hi);
+		timer = decode_timer(d, d->d_itimer, d->d_itimer_hi);
 		qflags = (flags & HUMAN_FLAG);
-		if (d.d_ino_hardlimit && d.d_icount > d.d_ino_hardlimit)
+		if (d->d_ino_hardlimit && d->d_icount > d->d_ino_hardlimit)
 			qflags |= LIMIT_FLAG;
-		if (d.d_ino_softlimit && d.d_icount > d.d_ino_softlimit)
+		if (d->d_ino_softlimit && d->d_icount > d->d_ino_softlimit)
 			qflags |= QUOTA_FLAG;
 		if (flags & HUMAN_FLAG)
 			fprintf(fp, " %6s %6s %6s  %02d %8s",
-				num_to_string(d.d_icount, c, sizeof(c)),
-				num_to_string(d.d_ino_softlimit, s, sizeof(s)),
-				num_to_string(d.d_ino_hardlimit, h, sizeof(h)),
-				d.d_iwarns,
+				num_to_string(d->d_icount, c, sizeof(c)),
+				num_to_string(d->d_ino_softlimit, s, sizeof(s)),
+				num_to_string(d->d_ino_hardlimit, h, sizeof(h)),
+				d->d_iwarns,
 				time_to_string(timer, qflags));
 		else
 			fprintf(fp, " %10llu %10llu %10llu     %02d %9s",
-				(unsigned long long)d.d_icount,
-				(unsigned long long)d.d_ino_softlimit,
-				(unsigned long long)d.d_ino_hardlimit,
-				d.d_iwarns,
+				(unsigned long long)d->d_icount,
+				(unsigned long long)d->d_ino_softlimit,
+				(unsigned long long)d->d_ino_hardlimit,
+				d->d_iwarns,
 				time_to_string(timer, qflags));
 	}
 	if (form & XFS_RTBLOCK_QUOTA) {
-		timer = decode_timer(&d, d.d_rtbtimer, d.d_rtbtimer_hi);
+		timer = decode_timer(d, d->d_rtbtimer, d->d_rtbtimer_hi);
 		qflags = (flags & HUMAN_FLAG);
-		if (d.d_rtb_hardlimit && d.d_rtbcount > d.d_rtb_hardlimit)
+		if (d->d_rtb_hardlimit && d->d_rtbcount > d->d_rtb_hardlimit)
 			qflags |= LIMIT_FLAG;
-		if (d.d_rtb_softlimit && d.d_rtbcount > d.d_rtb_softlimit)
+		if (d->d_rtb_softlimit && d->d_rtbcount > d->d_rtb_softlimit)
 			qflags |= QUOTA_FLAG;
 		if (flags & HUMAN_FLAG)
 			fprintf(fp, " %6s %6s %6s  %02d %8s",
-				bbs_to_string(d.d_rtbcount, c, sizeof(c)),
-				bbs_to_string(d.d_rtb_softlimit, s, sizeof(s)),
-				bbs_to_string(d.d_rtb_hardlimit, h, sizeof(h)),
-				d.d_rtbwarns,
+				bbs_to_string(d->d_rtbcount, c, sizeof(c)),
+				bbs_to_string(d->d_rtb_softlimit, s, sizeof(s)),
+				bbs_to_string(d->d_rtb_hardlimit, h, sizeof(h)),
+				d->d_rtbwarns,
 				time_to_string(timer, qflags));
 		else
 			fprintf(fp, " %10llu %10llu %10llu     %02d %9s",
-				(unsigned long long)d.d_rtbcount >> 1,
-				(unsigned long long)d.d_rtb_softlimit >> 1,
-				(unsigned long long)d.d_rtb_hardlimit >> 1,
-				d.d_rtbwarns,
+				(unsigned long long)d->d_rtbcount >> 1,
+				(unsigned long long)d->d_rtb_softlimit >> 1,
+				(unsigned long long)d->d_rtb_hardlimit >> 1,
+				d->d_rtbwarns,
 				time_to_string(timer, qflags));
 	}
 	fputc('\n', fp);
@@ -472,28 +472,29 @@ report_user_mount(
 	uint		flags)
 {
 	struct passwd	*u;
+	fs_disk_quota_t	d;
 	uint		id = 0, oid;
 
 	if (upper) {	/* identifier range specified */
 		for (id = lower; id <= upper; id++) {
-			if (report_mount(fp, id, NULL, NULL,
+			if (report_mount(fp, &d, id, NULL, NULL,
 					form, XFS_USER_QUOTA, mount, flags))
 				flags |= NO_HEADER_FLAG;
 		}
-	} else if (report_mount(fp, id, NULL, &oid, form,
+	} else if (report_mount(fp, &d, id, NULL, &oid, form,
 				XFS_USER_QUOTA, mount,
 				flags|GETNEXTQUOTA_FLAG)) {
 		id = oid + 1;
 		flags |= GETNEXTQUOTA_FLAG;
 		flags |= NO_HEADER_FLAG;
-		while (report_mount(fp, id, NULL, &oid, form, XFS_USER_QUOTA,
+		while (report_mount(fp, &d, id, NULL, &oid, form, XFS_USER_QUOTA,
 				    mount, flags)) {
 			id = oid + 1;
 		}
 	} else {
 		setpwent();
 		while ((u = getpwent()) != NULL) {
-			if (report_mount(fp, u->pw_uid, u->pw_name, NULL,
+			if (report_mount(fp, &d, u->pw_uid, u->pw_name, NULL,
 					form, XFS_USER_QUOTA, mount, flags))
 				flags |= NO_HEADER_FLAG;
 		}
@@ -514,28 +515,29 @@ report_group_mount(
 	uint		flags)
 {
 	struct group	*g;
+	fs_disk_quota_t	d;
 	uint		id = 0, oid;
 
 	if (upper) {	/* identifier range specified */
 		for (id = lower; id <= upper; id++) {
-			if (report_mount(fp, id, NULL, NULL,
+			if (report_mount(fp, &d, id, NULL, NULL,
 					form, XFS_GROUP_QUOTA, mount, flags))
 				flags |= NO_HEADER_FLAG;
 		}
-	} else if (report_mount(fp, id, NULL, &oid, form,
+	} else if (report_mount(fp, &d, id, NULL, &oid, form,
 				XFS_GROUP_QUOTA, mount,
 				flags|GETNEXTQUOTA_FLAG)) {
 		id = oid + 1;
 		flags |= GETNEXTQUOTA_FLAG;
 		flags |= NO_HEADER_FLAG;
-		while (report_mount(fp, id, NULL, &oid, form, XFS_GROUP_QUOTA,
+		while (report_mount(fp, &d, id, NULL, &oid, form, XFS_GROUP_QUOTA,
 				    mount, flags)) {
 			id = oid + 1;
 		}
 	} else {
 		setgrent();
 		while ((g = getgrent()) != NULL) {
-			if (report_mount(fp, g->gr_gid, g->gr_name, NULL,
+			if (report_mount(fp, &d, g->gr_gid, g->gr_name, NULL,
 					form, XFS_GROUP_QUOTA, mount, flags))
 				flags |= NO_HEADER_FLAG;
 		}
@@ -555,21 +557,22 @@ report_project_mount(
 	uint		flags)
 {
 	fs_project_t	*p;
+	fs_disk_quota_t	d;
 	uint		id = 0, oid;
 
 	if (upper) {	/* identifier range specified */
 		for (id = lower; id <= upper; id++) {
-			if (report_mount(fp, id, NULL, NULL,
+			if (report_mount(fp, &d, id, NULL, NULL,
 					form, XFS_PROJ_QUOTA, mount, flags))
 				flags |= NO_HEADER_FLAG;
 		}
-	} else if (report_mount(fp, id, NULL, &oid, form,
+	} else if (report_mount(fp, &d, id, NULL, &oid, form,
 				XFS_PROJ_QUOTA, mount,
 				flags|GETNEXTQUOTA_FLAG)) {
 		id = oid + 1;
 		flags |= GETNEXTQUOTA_FLAG;
 		flags |= NO_HEADER_FLAG;
-		while (report_mount(fp, id, NULL, &oid, form, XFS_PROJ_QUOTA,
+		while (report_mount(fp, &d, id, NULL, &oid, form, XFS_PROJ_QUOTA,
 				    mount, flags)) {
 			id = oid + 1;
 		}
@@ -579,14 +582,14 @@ report_project_mount(
 			 * Print default project quota, even if projid 0
 			 * isn't defined
 			 */
-			if (report_mount(fp, 0, NULL, NULL,
+			if (report_mount(fp, &d, 0, NULL, NULL,
 					form, XFS_PROJ_QUOTA, mount, flags))
 				flags |= NO_HEADER_FLAG;
 		}
 
 		setprent();
 		while ((p = getprent()) != NULL) {
-			if (report_mount(fp, p->pr_prid, p->pr_name, NULL,
+			if (report_mount(fp, &d, p->pr_prid, p->pr_name, NULL,
 					form, XFS_PROJ_QUOTA, mount, flags))
 				flags |= NO_HEADER_FLAG;
 		}
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] xfs_quota: create fs_disk_quota_t on upper level
  2022-03-28 22:25 ` [PATCH 2/5] xfs_quota: create fs_disk_quota_t on upper level Andrey Albershteyn
@ 2022-04-06 16:34   ` Christoph Hellwig
  0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-04-06 16:34 UTC (permalink / raw)
  To: Andrey Albershteyn; +Cc: linux-xfs

On Tue, Mar 29, 2022 at 12:25:00AM +0200, Andrey Albershteyn wrote:
> For further splitting of get_quota() and dump_file()/report_mount().

Same tab / typedef nitpick as the last patch, plus another nitpick:

> +	if (dump_file(fp, &d, id, &oid, type, mount->fs_name, GETNEXTQUOTA_FLAG)) {

Overly long line here.

The actual changes looks fine to me.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 3/5] xfs_quota: split get_quota() and report_mount()/dump_file()
  2022-03-28 22:24 [PATCH 0/5] xfsprogs: optimize -L/-U range calls for xfs_quota's dump/report Andrey Albershteyn
  2022-03-28 22:24 ` [PATCH 1/5] xfs_quota: separate quota info acquisition into get_quota() Andrey Albershteyn
  2022-03-28 22:25 ` [PATCH 2/5] xfs_quota: create fs_disk_quota_t on upper level Andrey Albershteyn
@ 2022-03-28 22:25 ` Andrey Albershteyn
  2022-04-06 16:36   ` Christoph Hellwig
  2022-03-28 22:25 ` [PATCH 4/5] xfs_quota: utilize XFS_GETNEXTQUOTA for ranged calls in report/dump Andrey Albershteyn
  2022-03-28 22:25 ` [PATCH 5/5] xfs_quota: apply -L/-U range limits in uid/gid/pid loops Andrey Albershteyn
  4 siblings, 1 reply; 14+ messages in thread
From: Andrey Albershteyn @ 2022-03-28 22:25 UTC (permalink / raw)
  To: linux-xfs; +Cc: Andrey Albershteyn

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 quota/report.c | 134 ++++++++++++++++++++++++++++---------------------
 1 file changed, 78 insertions(+), 56 deletions(-)

diff --git a/quota/report.c b/quota/report.c
index 0462ce97..14b7f458 100644
--- a/quota/report.c
+++ b/quota/report.c
@@ -97,15 +97,8 @@ static int
 dump_file(
 	FILE		*fp,
 	fs_disk_quota_t *d,
-	uint		id,
-	uint		*oid,
-	uint		type,
-	char		*dev,
-	int		flags)
+	char		*dev)
 {
-	if	(!get_quota(d, id, oid, type, dev, flags))
-		return 0;
-
 	if (!d->d_blk_softlimit && !d->d_blk_hardlimit &&
 	    !d->d_ino_softlimit && !d->d_ino_hardlimit &&
 	    !d->d_rtb_softlimit && !d->d_rtb_hardlimit)
@@ -153,46 +146,54 @@ dump_limits_any_type(
 
 	/* Range was specified; query everything in it */
 	if (upper) {
-		for (id = lower; id <= upper; id++)
-			dump_file(fp, &d, id, NULL, type, mount->fs_name, 0);
+		for (id = lower; id <= upper; id++) {
+			get_quota(&d, id, &oid, type, mount->fs_name, 0);
+			dump_file(fp, &d, mount->fs_name);
+		}
 		return;
 	}
 
 	/* Use GETNEXTQUOTA if it's available */
-	if (dump_file(fp, &d, id, &oid, type, mount->fs_name, GETNEXTQUOTA_FLAG)) {
+	if (get_quota(&d, id, &oid, type, mount->fs_name, GETNEXTQUOTA_FLAG)) {
+		dump_file(fp, &d, mount->fs_name);
 		id = oid + 1;
-		while (dump_file(fp, &d, id, &oid, type, mount->fs_name,
-				 GETNEXTQUOTA_FLAG))
+		while (get_quota(&d, id, &oid, type, mount->fs_name,
+					GETNEXTQUOTA_FLAG)) {
+			dump_file(fp, &d, mount->fs_name);
 			id = oid + 1;
+		}
 		return;
-        }
+	}
 
 	/* Otherwise fall back to iterating over each uid/gid/prjid */
 	switch (type) {
 	case XFS_GROUP_QUOTA: {
 			struct group *g;
 			setgrent();
-			while ((g = getgrent()) != NULL)
-				dump_file(fp, &d, g->gr_gid, NULL, type,
-					  mount->fs_name, 0);
+			while ((g = getgrent()) != NULL) {
+				get_quota(&d, g->gr_gid, NULL, type, mount->fs_name, 0);
+				dump_file(fp, &d, mount->fs_name);
+			}
 			endgrent();
 			break;
 		}
 	case XFS_PROJ_QUOTA: {
 			struct fs_project *p;
 			setprent();
-			while ((p = getprent()) != NULL)
-				dump_file(fp, &d, p->pr_prid, NULL, type,
-					  mount->fs_name, 0);
+			while ((p = getprent()) != NULL) {
+				get_quota(&d, p->pr_prid, NULL, type, mount->fs_name, 0);
+				dump_file(fp, &d, mount->fs_name);
+			}
 			endprent();
 			break;
 		}
 	case XFS_USER_QUOTA: {
 			struct passwd *u;
 			setpwent();
-			while ((u = getpwent()) != NULL)
-				dump_file(fp, &d, u->pw_uid, NULL, type,
-					  mount->fs_name, 0);
+			while ((u = getpwent()) != NULL) {
+				get_quota(&d, u->pw_uid, NULL, type, mount->fs_name, 0);
+				dump_file(fp, &d, mount->fs_name);
+			}
 			endpwent();
 			break;
 		}
@@ -336,9 +337,7 @@ static int
 report_mount(
 	FILE		*fp,
 	fs_disk_quota_t *d,
-	uint32_t	id,
 	char		*name,
-	uint32_t	*oid,
 	uint		form,
 	uint		type,
 	fs_path_t	*mount,
@@ -349,9 +348,6 @@ report_mount(
 	uint		qflags;
 	int		count;
 
-	if	(!get_quota(d, id, oid, type, mount->fs_name, flags))
-		return 0;
-
 	if (flags & TERSE_FLAG) {
 		count = 0;
 		if ((form & XFS_BLOCK_QUOTA) && d->d_bcount)
@@ -477,26 +473,35 @@ report_user_mount(
 
 	if (upper) {	/* identifier range specified */
 		for (id = lower; id <= upper; id++) {
-			if (report_mount(fp, &d, id, NULL, NULL,
-					form, XFS_USER_QUOTA, mount, flags))
+			if (get_quota(&d, id, NULL, XFS_USER_QUOTA,
+						mount->fs_name, flags)) {
+				report_mount(fp, &d, NULL, form, XFS_USER_QUOTA, mount,
+						flags);
 				flags |= NO_HEADER_FLAG;
+			}
 		}
-	} else if (report_mount(fp, &d, id, NULL, &oid, form,
-				XFS_USER_QUOTA, mount,
+	} else if (get_quota(&d, id, &oid, XFS_USER_QUOTA, mount->fs_name,
 				flags|GETNEXTQUOTA_FLAG)) {
+		report_mount(fp, &d, NULL, form, XFS_USER_QUOTA, mount,
+			flags|GETNEXTQUOTA_FLAG);
 		id = oid + 1;
 		flags |= GETNEXTQUOTA_FLAG;
 		flags |= NO_HEADER_FLAG;
-		while (report_mount(fp, &d, id, NULL, &oid, form, XFS_USER_QUOTA,
-				    mount, flags)) {
+		while (get_quota(&d, id, &oid, XFS_USER_QUOTA, mount->fs_name,
+				flags)) {
+			report_mount(fp, &d, NULL, form, XFS_USER_QUOTA,
+				mount, flags);
 			id = oid + 1;
 		}
 	} else {
 		setpwent();
 		while ((u = getpwent()) != NULL) {
-			if (report_mount(fp, &d, u->pw_uid, u->pw_name, NULL,
-					form, XFS_USER_QUOTA, mount, flags))
+			if (get_quota(&d, u->pw_uid, NULL, XFS_USER_QUOTA, mount->fs_name,
+					flags)) {
+				report_mount(fp, &d, u->pw_name, form, XFS_USER_QUOTA, mount,
+						flags);
 				flags |= NO_HEADER_FLAG;
+			}
 		}
 		endpwent();
 	}
@@ -520,26 +525,34 @@ report_group_mount(
 
 	if (upper) {	/* identifier range specified */
 		for (id = lower; id <= upper; id++) {
-			if (report_mount(fp, &d, id, NULL, NULL,
-					form, XFS_GROUP_QUOTA, mount, flags))
+			if (get_quota(&d, id, NULL, XFS_GROUP_QUOTA,
+						mount->fs_name, flags)) {
+				report_mount(fp, &d, NULL, form, XFS_GROUP_QUOTA, mount, flags);
 				flags |= NO_HEADER_FLAG;
+			}
 		}
-	} else if (report_mount(fp, &d, id, NULL, &oid, form,
-				XFS_GROUP_QUOTA, mount,
-				flags|GETNEXTQUOTA_FLAG)) {
+	} else if (get_quota(&d, id, &oid, XFS_GROUP_QUOTA,
+				mount->fs_name, flags|GETNEXTQUOTA_FLAG)) {
+		report_mount(fp, &d, NULL, form, XFS_GROUP_QUOTA, mount,
+				flags|GETNEXTQUOTA_FLAG);
 		id = oid + 1;
 		flags |= GETNEXTQUOTA_FLAG;
 		flags |= NO_HEADER_FLAG;
-		while (report_mount(fp, &d, id, NULL, &oid, form, XFS_GROUP_QUOTA,
-				    mount, flags)) {
+		while (get_quota(&d, id, &oid, XFS_GROUP_QUOTA,
+					mount->fs_name, flags)) {
+			report_mount(fp, &d, NULL, form, XFS_GROUP_QUOTA, mount,
+					flags);
 			id = oid + 1;
 		}
 	} else {
 		setgrent();
 		while ((g = getgrent()) != NULL) {
-			if (report_mount(fp, &d, g->gr_gid, g->gr_name, NULL,
-					form, XFS_GROUP_QUOTA, mount, flags))
+			if (get_quota(&d, g->gr_gid, NULL, XFS_GROUP_QUOTA,
+						mount->fs_name, flags)) {
+				report_mount(fp, &d, g->gr_name, form,
+						XFS_GROUP_QUOTA, mount, flags);
 				flags |= NO_HEADER_FLAG;
+			}
 		}
 	}
 	if (flags & NO_HEADER_FLAG)
@@ -562,18 +575,22 @@ report_project_mount(
 
 	if (upper) {	/* identifier range specified */
 		for (id = lower; id <= upper; id++) {
-			if (report_mount(fp, &d, id, NULL, NULL,
-					form, XFS_PROJ_QUOTA, mount, flags))
+			if (get_quota(&d, id, NULL, XFS_PROJ_QUOTA,
+						mount->fs_name, flags)) {
+				report_mount(fp, &d, NULL, form, XFS_PROJ_QUOTA, mount, flags);
 				flags |= NO_HEADER_FLAG;
+			}
 		}
-	} else if (report_mount(fp, &d, id, NULL, &oid, form,
-				XFS_PROJ_QUOTA, mount,
-				flags|GETNEXTQUOTA_FLAG)) {
+	} else if (get_quota(&d, id, &oid, XFS_PROJ_QUOTA,
+				mount->fs_name, flags|GETNEXTQUOTA_FLAG)) {
+		report_mount(fp, &d, NULL, form, XFS_PROJ_QUOTA, mount,
+				flags|GETNEXTQUOTA_FLAG);
 		id = oid + 1;
 		flags |= GETNEXTQUOTA_FLAG;
 		flags |= NO_HEADER_FLAG;
-		while (report_mount(fp, &d, id, NULL, &oid, form, XFS_PROJ_QUOTA,
-				    mount, flags)) {
+		while (get_quota(&d, id, &oid, XFS_PROJ_QUOTA,
+					mount->fs_name, flags)) {
+			report_mount(fp, &d, NULL, form, XFS_PROJ_QUOTA, mount, flags);
 			id = oid + 1;
 		}
 	} else {
@@ -582,16 +599,21 @@ report_project_mount(
 			 * Print default project quota, even if projid 0
 			 * isn't defined
 			 */
-			if (report_mount(fp, &d, 0, NULL, NULL,
-					form, XFS_PROJ_QUOTA, mount, flags))
+			if (get_quota(&d, 0, NULL, XFS_PROJ_QUOTA,
+						mount->fs_name, flags)) {
+				report_mount(fp, &d, NULL, form, XFS_PROJ_QUOTA, mount, flags);
 				flags |= NO_HEADER_FLAG;
+			}
 		}
 
 		setprent();
 		while ((p = getprent()) != NULL) {
-			if (report_mount(fp, &d, p->pr_prid, p->pr_name, NULL,
-					form, XFS_PROJ_QUOTA, mount, flags))
+			if (get_quota(&d, p->pr_prid, NULL, XFS_PROJ_QUOTA,
+						mount->fs_name, flags)) {
+				report_mount(fp, &d, p->pr_name, form, XFS_PROJ_QUOTA, mount,
+						flags);
 				flags |= NO_HEADER_FLAG;
+			}
 		}
 		endprent();
 	}
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/5] xfs_quota: split get_quota() and report_mount()/dump_file()
  2022-03-28 22:25 ` [PATCH 3/5] xfs_quota: split get_quota() and report_mount()/dump_file() Andrey Albershteyn
@ 2022-04-06 16:36   ` Christoph Hellwig
  2022-04-07 11:06     ` Andrey Albershteyn
  0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2022-04-06 16:36 UTC (permalink / raw)
  To: Andrey Albershteyn; +Cc: linux-xfs

Can you explain the split and the reason for it a little more here?

>  dump_file(
>  	FILE		*fp,
>  	fs_disk_quota_t *d,
> -	uint		id,
> -	uint		*oid,
> -	uint		type,
> -	char		*dev,
> -	int		flags)
> +	char		*dev)
>  {
> -	if	(!get_quota(d, id, oid, type, dev, flags))
> -		return 0;

I think it would make more sense to move this into the previous
patch that passes the fs_disk_quota to dump_file.

And maybe this and the previous patch should be split into one for
dump_file and one for report_mount?

> +			while ((g = getgrent()) != NULL) {
> +				get_quota(&d, g->gr_gid, NULL, type, mount->fs_name, 0);

Overly long line.  (and a few more below).


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/5] xfs_quota: split get_quota() and report_mount()/dump_file()
  2022-04-06 16:36   ` Christoph Hellwig
@ 2022-04-07 11:06     ` Andrey Albershteyn
  2022-04-13 15:54       ` Christoph Hellwig
  0 siblings, 1 reply; 14+ messages in thread
From: Andrey Albershteyn @ 2022-04-07 11:06 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

Hey Christoph,

On Wed, Apr 06, 2022 at 09:36:55AM -0700, Christoph Hellwig wrote:
> Can you explain the split and the reason for it a little more here?
> 
> >  dump_file(
> >  	FILE		*fp,
> >  	fs_disk_quota_t *d,
> > -	uint		id,
> > -	uint		*oid,
> > -	uint		type,
> > -	char		*dev,
> > -	int		flags)
> > +	char		*dev)
> >  {
> > -	if	(!get_quota(d, id, oid, type, dev, flags))
> > -		return 0;
> 
> I think it would make more sense to move this into the previous
> patch that passes the fs_disk_quota to dump_file.
> 
> And maybe this and the previous patch should be split into one for
> dump_file and one for report_mount?

I did it like this initially but it appeared to me that the diff was
messy. As there were many &d -> d and report_mount ->
get_quota/report_mount replacements, so I split it. But I'm not
against reshaping this back, should I do it?

> 
> > +			while ((g = getgrent()) != NULL) {
> > +				get_quota(&d, g->gr_gid, NULL, type, mount->fs_name, 0);
> 
> Overly long line.  (and a few more below).
> 

-- 
- Andrey


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/5] xfs_quota: split get_quota() and report_mount()/dump_file()
  2022-04-07 11:06     ` Andrey Albershteyn
@ 2022-04-13 15:54       ` Christoph Hellwig
  0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-04-13 15:54 UTC (permalink / raw)
  To: Andrey Albershteyn; +Cc: Christoph Hellwig, linux-xfs

On Thu, Apr 07, 2022 at 01:06:56PM +0200, Andrey Albershteyn wrote:
> I did it like this initially but it appeared to me that the diff was
> messy. As there were many &d -> d and report_mount ->
> get_quota/report_mount replacements, so I split it. But I'm not
> against reshaping this back, should I do it?

Well, a large part of this series is churn and we can't do much about
it.  To me doing the changes together seems more logical, but in the
end either way is fine, so feel free to do it the way you prefer.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 4/5] xfs_quota: utilize XFS_GETNEXTQUOTA for ranged calls in report/dump
  2022-03-28 22:24 [PATCH 0/5] xfsprogs: optimize -L/-U range calls for xfs_quota's dump/report Andrey Albershteyn
                   ` (2 preceding siblings ...)
  2022-03-28 22:25 ` [PATCH 3/5] xfs_quota: split get_quota() and report_mount()/dump_file() Andrey Albershteyn
@ 2022-03-28 22:25 ` Andrey Albershteyn
  2022-04-06 16:38   ` Christoph Hellwig
  2022-03-28 22:25 ` [PATCH 5/5] xfs_quota: apply -L/-U range limits in uid/gid/pid loops Andrey Albershteyn
  4 siblings, 1 reply; 14+ messages in thread
From: Andrey Albershteyn @ 2022-03-28 22:25 UTC (permalink / raw)
  To: linux-xfs; +Cc: Andrey Albershteyn

The implementation based on XFS_GETQUOTA call for each ID in range,
specified with -L/-U, is quite slow for wider ranges.

If kernel supports XFS_GETNEXTQUOTA, report_*_mount/dump_any_file
will use that to obtain quota list for the mount. XFS_GETNEXTQUOTA
returns quota of the requested ID and next ID with non-empty quota.

Otherwise, XFS_GETQUOTA will be used for each user/group/project ID
known from password/group/project database.

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 quota/report.c | 113 +++++++++++++++----------------------------------
 1 file changed, 35 insertions(+), 78 deletions(-)

diff --git a/quota/report.c b/quota/report.c
index 14b7f458..074abbc1 100644
--- a/quota/report.c
+++ b/quota/report.c
@@ -135,7 +135,7 @@ dump_limits_any_type(
 {
 	fs_path_t	*mount;
 	fs_disk_quota_t d;
-	uint		id = 0, oid;
+	uint		id = lower, oid, flags = 0;
 
 	if ((mount = fs_table_lookup(dir, FS_MOUNT_POINT)) == NULL) {
 		exitcode = 1;
@@ -144,27 +144,17 @@ dump_limits_any_type(
 		return;
 	}
 
-	/* Range was specified; query everything in it */
-	if (upper) {
-		for (id = lower; id <= upper; id++) {
-			get_quota(&d, id, &oid, type, mount->fs_name, 0);
-			dump_file(fp, &d, mount->fs_name);
-		}
-		return;
-	}
-
-	/* Use GETNEXTQUOTA if it's available */
-	if (get_quota(&d, id, &oid, type, mount->fs_name, GETNEXTQUOTA_FLAG)) {
+	while (get_quota(&d, id, &oid, type,
+				mount->fs_name, flags|GETNEXTQUOTA_FLAG) &&
+			!(upper && (d.d_id > upper))) {
 		dump_file(fp, &d, mount->fs_name);
 		id = oid + 1;
-		while (get_quota(&d, id, &oid, type, mount->fs_name,
-					GETNEXTQUOTA_FLAG)) {
-			dump_file(fp, &d, mount->fs_name);
-			id = oid + 1;
-		}
-		return;
+		flags |= GETNEXTQUOTA_FLAG;
 	}
 
+	if (flags & GETNEXTQUOTA_FLAG)
+		return;
+
 	/* Otherwise fall back to iterating over each uid/gid/prjid */
 	switch (type) {
 	case XFS_GROUP_QUOTA: {
@@ -469,31 +459,19 @@ report_user_mount(
 {
 	struct passwd	*u;
 	fs_disk_quota_t	d;
-	uint		id = 0, oid;
+	uint		id = lower, oid;
 
-	if (upper) {	/* identifier range specified */
-		for (id = lower; id <= upper; id++) {
-			if (get_quota(&d, id, NULL, XFS_USER_QUOTA,
-						mount->fs_name, flags)) {
-				report_mount(fp, &d, NULL, form, XFS_USER_QUOTA, mount,
-						flags);
-				flags |= NO_HEADER_FLAG;
-			}
-		}
-	} else if (get_quota(&d, id, &oid, XFS_USER_QUOTA, mount->fs_name,
-				flags|GETNEXTQUOTA_FLAG)) {
-		report_mount(fp, &d, NULL, form, XFS_USER_QUOTA, mount,
-			flags|GETNEXTQUOTA_FLAG);
+	while (get_quota(&d, id, &oid, XFS_USER_QUOTA,
+				mount->fs_name, flags|GETNEXTQUOTA_FLAG) &&
+			!(upper && (d.d_id > upper))) {
+		report_mount(fp, &d, NULL, form, XFS_USER_QUOTA, mount, flags);
 		id = oid + 1;
 		flags |= GETNEXTQUOTA_FLAG;
 		flags |= NO_HEADER_FLAG;
-		while (get_quota(&d, id, &oid, XFS_USER_QUOTA, mount->fs_name,
-				flags)) {
-			report_mount(fp, &d, NULL, form, XFS_USER_QUOTA,
-				mount, flags);
-			id = oid + 1;
-		}
-	} else {
+	}
+
+	/* No GETNEXTQUOTA support, iterate over all from password file */
+	if (!(flags & GETNEXTQUOTA_FLAG)) {
 		setpwent();
 		while ((u = getpwent()) != NULL) {
 			if (get_quota(&d, u->pw_uid, NULL, XFS_USER_QUOTA, mount->fs_name,
@@ -521,30 +499,19 @@ report_group_mount(
 {
 	struct group	*g;
 	fs_disk_quota_t	d;
-	uint		id = 0, oid;
+	uint		id = lower, oid;
 
-	if (upper) {	/* identifier range specified */
-		for (id = lower; id <= upper; id++) {
-			if (get_quota(&d, id, NULL, XFS_GROUP_QUOTA,
-						mount->fs_name, flags)) {
-				report_mount(fp, &d, NULL, form, XFS_GROUP_QUOTA, mount, flags);
-				flags |= NO_HEADER_FLAG;
-			}
-		}
-	} else if (get_quota(&d, id, &oid, XFS_GROUP_QUOTA,
-				mount->fs_name, flags|GETNEXTQUOTA_FLAG)) {
-		report_mount(fp, &d, NULL, form, XFS_GROUP_QUOTA, mount,
-				flags|GETNEXTQUOTA_FLAG);
+	while (get_quota(&d, id, &oid, XFS_GROUP_QUOTA,
+				mount->fs_name, flags|GETNEXTQUOTA_FLAG) &&
+			!(upper && (oid > upper))) {
+		report_mount(fp, &d, NULL, form, XFS_GROUP_QUOTA, mount, flags);
 		id = oid + 1;
 		flags |= GETNEXTQUOTA_FLAG;
 		flags |= NO_HEADER_FLAG;
-		while (get_quota(&d, id, &oid, XFS_GROUP_QUOTA,
-					mount->fs_name, flags)) {
-			report_mount(fp, &d, NULL, form, XFS_GROUP_QUOTA, mount,
-					flags);
-			id = oid + 1;
-		}
-	} else {
+	}
+
+	/* No GETNEXTQUOTA support, iterate over all from password file */
+	if (!(flags & GETNEXTQUOTA_FLAG)) {
 		setgrent();
 		while ((g = getgrent()) != NULL) {
 			if (get_quota(&d, g->gr_gid, NULL, XFS_GROUP_QUOTA,
@@ -571,29 +538,19 @@ report_project_mount(
 {
 	fs_project_t	*p;
 	fs_disk_quota_t	d;
-	uint		id = 0, oid;
+	uint		id = lower, oid;
 
-	if (upper) {	/* identifier range specified */
-		for (id = lower; id <= upper; id++) {
-			if (get_quota(&d, id, NULL, XFS_PROJ_QUOTA,
-						mount->fs_name, flags)) {
-				report_mount(fp, &d, NULL, form, XFS_PROJ_QUOTA, mount, flags);
-				flags |= NO_HEADER_FLAG;
-			}
-		}
-	} else if (get_quota(&d, id, &oid, XFS_PROJ_QUOTA,
-				mount->fs_name, flags|GETNEXTQUOTA_FLAG)) {
-		report_mount(fp, &d, NULL, form, XFS_PROJ_QUOTA, mount,
-				flags|GETNEXTQUOTA_FLAG);
+	while (get_quota(&d, id, &oid, XFS_PROJ_QUOTA,
+				mount->fs_name, flags|GETNEXTQUOTA_FLAG) &&
+			!(upper && (d.d_id > upper))) {
+		report_mount(fp, &d, NULL, form, XFS_PROJ_QUOTA, mount, flags);
 		id = oid + 1;
 		flags |= GETNEXTQUOTA_FLAG;
 		flags |= NO_HEADER_FLAG;
-		while (get_quota(&d, id, &oid, XFS_PROJ_QUOTA,
-					mount->fs_name, flags)) {
-			report_mount(fp, &d, NULL, form, XFS_PROJ_QUOTA, mount, flags);
-			id = oid + 1;
-		}
-	} else {
+	}
+
+	/* No GETNEXTQUOTA support, iterate over all */
+	if (!(flags & GETNEXTQUOTA_FLAG)) {
 		if (!getprprid(0)) {
 			/*
 			 * Print default project quota, even if projid 0
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] xfs_quota: utilize XFS_GETNEXTQUOTA for ranged calls in report/dump
  2022-03-28 22:25 ` [PATCH 4/5] xfs_quota: utilize XFS_GETNEXTQUOTA for ranged calls in report/dump Andrey Albershteyn
@ 2022-04-06 16:38   ` Christoph Hellwig
  0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-04-06 16:38 UTC (permalink / raw)
  To: Andrey Albershteyn; +Cc: linux-xfs

> +	while (get_quota(&d, id, &oid, type,
> +				mount->fs_name, flags|GETNEXTQUOTA_FLAG) &&

space around the operator please.

The changes themselves look fine to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 5/5] xfs_quota: apply -L/-U range limits in uid/gid/pid loops
  2022-03-28 22:24 [PATCH 0/5] xfsprogs: optimize -L/-U range calls for xfs_quota's dump/report Andrey Albershteyn
                   ` (3 preceding siblings ...)
  2022-03-28 22:25 ` [PATCH 4/5] xfs_quota: utilize XFS_GETNEXTQUOTA for ranged calls in report/dump Andrey Albershteyn
@ 2022-03-28 22:25 ` Andrey Albershteyn
  2022-04-06 16:38   ` Christoph Hellwig
  4 siblings, 1 reply; 14+ messages in thread
From: Andrey Albershteyn @ 2022-03-28 22:25 UTC (permalink / raw)
  To: linux-xfs; +Cc: Andrey Albershteyn

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 quota/report.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/quota/report.c b/quota/report.c
index 074abbc1..c79e95ab 100644
--- a/quota/report.c
+++ b/quota/report.c
@@ -161,8 +161,10 @@ dump_limits_any_type(
 			struct group *g;
 			setgrent();
 			while ((g = getgrent()) != NULL) {
-				get_quota(&d, g->gr_gid, NULL, type, mount->fs_name, 0);
-				dump_file(fp, &d, mount->fs_name);
+				if (get_quota(&d, g->gr_gid, NULL, type, mount->fs_name, 0) &&
+						!(lower && (d.d_id < lower)) &&
+						!(upper && (d.d_id > upper)))
+					dump_file(fp, &d, mount->fs_name);
 			}
 			endgrent();
 			break;
@@ -171,8 +173,10 @@ dump_limits_any_type(
 			struct fs_project *p;
 			setprent();
 			while ((p = getprent()) != NULL) {
-				get_quota(&d, p->pr_prid, NULL, type, mount->fs_name, 0);
-				dump_file(fp, &d, mount->fs_name);
+				if (get_quota(&d, p->pr_prid, NULL, type, mount->fs_name, 0) &&
+						!(lower && (d.d_id < lower)) &&
+						!(upper && (d.d_id > upper)))
+					dump_file(fp, &d, mount->fs_name);
 			}
 			endprent();
 			break;
@@ -181,8 +185,10 @@ dump_limits_any_type(
 			struct passwd *u;
 			setpwent();
 			while ((u = getpwent()) != NULL) {
-				get_quota(&d, u->pw_uid, NULL, type, mount->fs_name, 0);
-				dump_file(fp, &d, mount->fs_name);
+				if (get_quota(&d, u->pw_uid, NULL, type, mount->fs_name, 0) &&
+						!(lower && (d.d_id < lower)) &&
+						!(upper && (d.d_id > upper)))
+					dump_file(fp, &d, mount->fs_name);
 			}
 			endpwent();
 			break;
@@ -474,8 +480,10 @@ report_user_mount(
 	if (!(flags & GETNEXTQUOTA_FLAG)) {
 		setpwent();
 		while ((u = getpwent()) != NULL) {
-			if (get_quota(&d, u->pw_uid, NULL, XFS_USER_QUOTA, mount->fs_name,
-					flags)) {
+			if (get_quota(&d, u->pw_uid, NULL, XFS_USER_QUOTA,
+						mount->fs_name, flags) &&
+					!(lower && (d.d_id < lower)) &&
+					!(upper && (d.d_id > upper))) {
 				report_mount(fp, &d, u->pw_name, form, XFS_USER_QUOTA, mount,
 						flags);
 				flags |= NO_HEADER_FLAG;
@@ -515,7 +523,9 @@ report_group_mount(
 		setgrent();
 		while ((g = getgrent()) != NULL) {
 			if (get_quota(&d, g->gr_gid, NULL, XFS_GROUP_QUOTA,
-						mount->fs_name, flags)) {
+						mount->fs_name, flags) &&
+					!(lower && (d.d_id < lower)) &&
+					!(upper && (d.d_id > upper))) {
 				report_mount(fp, &d, g->gr_name, form,
 						XFS_GROUP_QUOTA, mount, flags);
 				flags |= NO_HEADER_FLAG;
@@ -557,7 +567,9 @@ report_project_mount(
 			 * isn't defined
 			 */
 			if (get_quota(&d, 0, NULL, XFS_PROJ_QUOTA,
-						mount->fs_name, flags)) {
+						mount->fs_name, flags) &&
+					!(lower && (d.d_id < lower)) &&
+					!(upper && (d.d_id > upper))) {
 				report_mount(fp, &d, NULL, form, XFS_PROJ_QUOTA, mount, flags);
 				flags |= NO_HEADER_FLAG;
 			}
@@ -566,7 +578,9 @@ report_project_mount(
 		setprent();
 		while ((p = getprent()) != NULL) {
 			if (get_quota(&d, p->pr_prid, NULL, XFS_PROJ_QUOTA,
-						mount->fs_name, flags)) {
+						mount->fs_name, flags) &&
+					!(lower && (d.d_id < lower)) &&
+					!(upper && (d.d_id > upper))) {
 				report_mount(fp, &d, p->pr_name, form, XFS_PROJ_QUOTA, mount,
 						flags);
 				flags |= NO_HEADER_FLAG;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] xfs_quota: apply -L/-U range limits in uid/gid/pid loops
  2022-03-28 22:25 ` [PATCH 5/5] xfs_quota: apply -L/-U range limits in uid/gid/pid loops Andrey Albershteyn
@ 2022-04-06 16:38   ` Christoph Hellwig
  0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-04-06 16:38 UTC (permalink / raw)
  To: Andrey Albershteyn; +Cc: linux-xfs

Please write a commit log that explains why you are doing this and
maybe some higher level context.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-04-13 15:54 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-28 22:24 [PATCH 0/5] xfsprogs: optimize -L/-U range calls for xfs_quota's dump/report Andrey Albershteyn
2022-03-28 22:24 ` [PATCH 1/5] xfs_quota: separate quota info acquisition into get_quota() Andrey Albershteyn
2022-03-28 23:44   ` Darrick J. Wong
2022-04-06 16:31   ` Christoph Hellwig
2022-03-28 22:25 ` [PATCH 2/5] xfs_quota: create fs_disk_quota_t on upper level Andrey Albershteyn
2022-04-06 16:34   ` Christoph Hellwig
2022-03-28 22:25 ` [PATCH 3/5] xfs_quota: split get_quota() and report_mount()/dump_file() Andrey Albershteyn
2022-04-06 16:36   ` Christoph Hellwig
2022-04-07 11:06     ` Andrey Albershteyn
2022-04-13 15:54       ` Christoph Hellwig
2022-03-28 22:25 ` [PATCH 4/5] xfs_quota: utilize XFS_GETNEXTQUOTA for ranged calls in report/dump Andrey Albershteyn
2022-04-06 16:38   ` Christoph Hellwig
2022-03-28 22:25 ` [PATCH 5/5] xfs_quota: apply -L/-U range limits in uid/gid/pid loops Andrey Albershteyn
2022-04-06 16:38   ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox