Linux-XFS Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 12/18] xfs: refactor default quota limits by resource
Date: Thu, 2 Jul 2020 09:30:01 +1000
Message-ID: <20200701233001.GD2005@dread.disaster.area> (raw)
In-Reply-To: <159353178739.2864738.11605071453935920102.stgit@magnolia>

On Tue, Jun 30, 2020 at 08:43:07AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Now that we've split up the dquot resource fields into separate structs,
> do the same for the default limits to enable further refactoring.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_dquot.c       |   30 +++++++++++++++---------------
>  fs/xfs/xfs_qm.c          |   36 ++++++++++++++++++------------------
>  fs/xfs/xfs_qm.h          |   22 ++++++++++------------
>  fs/xfs/xfs_qm_syscalls.c |   24 ++++++++++++------------
>  fs/xfs/xfs_quotaops.c    |   12 ++++++------
>  fs/xfs/xfs_trans_dquot.c |   18 +++++++++---------
>  6 files changed, 70 insertions(+), 72 deletions(-)

A few things here, starting with the "defq" naming. These are
quota limits, not "default quotas". I'd suggest taht this whole
set of structures need to be renamed as "quota limits". e.g

struct xfs_quota_limits {
	xfs_qcnt_t		hard;	/* default hard limit */
	xfs_qcnt_t		soft;	/* default soft limit */
	time64_t		time;	/* limit for timers */
	xfs_qwarncnt_t		warn;	/* limit for warnings */
};

Then we have

	qlim = xfs_qm_get_default_limits(q, xfs_dquot_type(dq));
> 
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 2d6b50760962..6975c27145fc 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -76,22 +76,22 @@ xfs_qm_adjust_dqlimits(
>  	ASSERT(dq->q_id);
>  	defq = xfs_get_defquota(q, xfs_dquot_type(dq));
>  
> -	if (defq->bsoftlimit && !dq->q_blk.softlimit) {
> -		dq->q_blk.softlimit = defq->bsoftlimit;
> +	if (defq->dfq_blk.softlimit && !dq->q_blk.softlimit) {
> +		dq->q_blk.softlimit = defq->dfq_blk.softlimit;
>  		prealloc = 1;
>  	}
> -	if (defq->bhardlimit && !dq->q_blk.hardlimit) {
> -		dq->q_blk.hardlimit = defq->bhardlimit;
> +	if (defq->dfq_blk.hardlimit && !dq->q_blk.hardlimit) {
> +		dq->q_blk.hardlimit = defq->dfq_blk.hardlimit;
>  		prealloc = 1;
>  	}
> -	if (defq->isoftlimit && !dq->q_ino.softlimit)
> -		dq->q_ino.softlimit = defq->isoftlimit;
> -	if (defq->ihardlimit && !dq->q_ino.hardlimit)
> -		dq->q_ino.hardlimit = defq->ihardlimit;
> -	if (defq->rtbsoftlimit && !dq->q_rtb.softlimit)
> -		dq->q_rtb.softlimit = defq->rtbsoftlimit;
> -	if (defq->rtbhardlimit && !dq->q_rtb.hardlimit)
> -		dq->q_rtb.hardlimit = defq->rtbhardlimit;
> +	if (defq->dfq_ino.softlimit && !dq->q_ino.softlimit)
> +		dq->q_ino.softlimit = defq->dfq_ino.softlimit;
> +	if (defq->dfq_ino.hardlimit && !dq->q_ino.hardlimit)
> +		dq->q_ino.hardlimit = defq->dfq_ino.hardlimit;
> +	if (defq->dfq_rtb.softlimit && !dq->q_rtb.softlimit)
> +		dq->q_rtb.softlimit = defq->dfq_rtb.softlimit;
> +	if (defq->dfq_rtb.hardlimit && !dq->q_rtb.hardlimit)
> +		dq->q_rtb.hardlimit = defq->dfq_rtb.hardlimit;

And all this turns into somthing much easier to read:

....
	if (qlim->ino.soft && !dq->q_ino.softlimit)
		dq->q_ino.softlimit = qlim->ino.soft;
	if (qlim->ino.hard && !dq->q_ino.hardlimit)
		dq->q_ino.hardlimit = qlim->ino.hard;
....

I'll also suggest we don't need to check qlim values here. It could
just be:

	if (!dq->q_ino.softlimit)
		dq->q_ino.softlimit = qlim->ino.soft;
	if (!dq->q_ino.hardlimit)
		dq->q_ino.hardlimit = qlim->ino.hard;


> @@ -41,20 +41,18 @@ extern struct kmem_zone	*xfs_qm_dqtrxzone;
>   */
>  #define XFS_DQUOT_CLUSTER_SIZE_FSB	(xfs_filblks_t)1
>  
> +struct xfs_def_qres {
> +	xfs_qcnt_t		hardlimit;	/* default hard limit */
> +	xfs_qcnt_t		softlimit;	/* default soft limit */
> +	time64_t		timelimit;	/* limit for timers */
> +	xfs_qwarncnt_t		warnlimit;	/* limit for warnings */
> +};

As I implied above, this is a quota limits structure, not a "default
quota" structure. I'm not sure what the "res" in the name means,
either...

> +
>  /* Defaults for each quota type: time limits, warn limits, usage limits */
>  struct xfs_def_quota {
> -	time64_t	btimelimit;	/* limit for blks timer */
> -	time64_t	itimelimit;	/* limit for inodes timer */
> -	time64_t	rtbtimelimit;	/* limit for rt blks timer */
> -	xfs_qwarncnt_t	bwarnlimit;	/* limit for blks warnings */
> -	xfs_qwarncnt_t	iwarnlimit;	/* limit for inodes warnings */
> -	xfs_qwarncnt_t	rtbwarnlimit;	/* limit for rt blks warnings */
> -	xfs_qcnt_t	bhardlimit;	/* default data blk hard limit */
> -	xfs_qcnt_t	bsoftlimit;	/* default data blk soft limit */
> -	xfs_qcnt_t	ihardlimit;	/* default inode count hard limit */
> -	xfs_qcnt_t	isoftlimit;	/* default inode count soft limit */
> -	xfs_qcnt_t	rtbhardlimit;	/* default realtime blk hard limit */
> -	xfs_qcnt_t	rtbsoftlimit;	/* default realtime blk soft limit */
> +	struct xfs_def_qres	dfq_blk;
> +	struct xfs_def_qres	dfq_ino;
> +	struct xfs_def_qres	dfq_rtb;
>  };

The namespacing of these variables adds no value. It just makes the
code more verbose and harder to read. e.g.

> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 1b2b70b1660f..393b88612cc8 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -502,8 +502,8 @@ xfs_qm_scall_setqlim(
>  		dqp->q_blk.softlimit = soft;
>  		xfs_dquot_set_prealloc_limits(dqp);
>  		if (id == 0) {
> -			defq->bhardlimit = hard;
> -			defq->bsoftlimit = soft;
> +			defq->dfq_blk.hardlimit = hard;
> +			defq->dfq_blk.softlimit = soft;

IMO, these sorts of changes decrease the reability of the code. I'd
much prefer something like:

 		if (id == 0) {
-			defq->bhardlimit = hard;
-			defq->bsoftlimit = soft;
+			qlim->blk.hard = hard;
+			qlim->blk.soft = soft;

As it is still clear we are changing the hard block quota limits...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  parent reply index

Thread overview: 94+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-30 15:41 [PATCH 00/18] xfs: remove xfs_disk_quot from incore dquot Darrick J. Wong
2020-06-30 15:41 ` [PATCH 01/18] xfs: clear XFS_DQ_FREEING if we can't lock the dquot buffer to flush Darrick J. Wong
2020-06-30 21:35   ` Allison Collins
2020-07-01  8:32   ` Chandan Babu R
2020-07-01  8:33   ` Christoph Hellwig
2020-07-01 22:42     ` Dave Chinner
2020-06-30 15:42 ` [PATCH 02/18] xfs: fix inode quota reservation checks Darrick J. Wong
2020-06-30 21:35   ` Allison Collins
2020-07-01  8:33   ` Chandan Babu R
2020-07-01  8:34   ` Christoph Hellwig
2020-06-30 15:42 ` [PATCH 03/18] xfs: validate ondisk/incore dquot flags Darrick J. Wong
2020-06-30 21:35   ` Allison Collins
2020-07-01  8:33   ` Chandan Babu R
2020-07-01  8:42   ` Christoph Hellwig
2020-07-01 18:25     ` Darrick J. Wong
2020-07-02  6:30       ` Christoph Hellwig
2020-07-02 15:13         ` Darrick J. Wong
2020-07-01 22:41   ` Dave Chinner
2020-07-01 23:16     ` Darrick J. Wong
2020-06-30 15:42 ` [PATCH 04/18] xfs: stop using q_core.d_flags in the quota code Darrick J. Wong
2020-07-01  8:34   ` Chandan Babu R
2020-07-01  8:47   ` Christoph Hellwig
2020-07-01 19:02     ` Darrick J. Wong
2020-07-01 20:15   ` Allison Collins
2020-07-01 22:50   ` Dave Chinner
2020-07-01 23:19     ` Darrick J. Wong
2020-07-01 23:44       ` Dave Chinner
2020-07-01 23:50         ` Darrick J. Wong
2020-07-03  0:58           ` Dave Chinner
2020-07-03 18:01             ` Darrick J. Wong
2020-06-30 15:42 ` [PATCH 05/18] xfs: stop using q_core.d_id " Darrick J. Wong
2020-07-01  8:33   ` Chandan Babu R
2020-07-01  8:48   ` Christoph Hellwig
2020-07-01 20:16   ` Allison Collins
2020-06-30 15:42 ` [PATCH 06/18] xfs: use a per-resource struct for incore dquot data Darrick J. Wong
2020-07-01  8:33   ` Chandan Babu R
2020-07-01  8:49   ` Christoph Hellwig
2020-07-01 20:16   ` Allison Collins
2020-06-30 15:42 ` [PATCH 07/18] xfs: stop using q_core limits in the quota code Darrick J. Wong
2020-07-01  8:33   ` Chandan Babu R
2020-07-01  8:50   ` Christoph Hellwig
2020-07-01 20:16   ` Allison Collins
2020-07-01 23:01   ` Dave Chinner
2020-07-01 23:13     ` Darrick J. Wong
2020-07-01 23:33       ` Darrick J. Wong
2020-07-01 23:45         ` Dave Chinner
2020-06-30 15:42 ` [PATCH 08/18] xfs: stop using q_core counters " Darrick J. Wong
2020-07-01  8:33   ` Chandan Babu R
2020-07-01  8:51   ` Christoph Hellwig
2020-07-01 23:20   ` Allison Collins
2020-06-30 15:42 ` [PATCH 09/18] xfs: stop using q_core warning " Darrick J. Wong
2020-07-01  8:33   ` Chandan Babu R
2020-07-01  8:51   ` Christoph Hellwig
2020-07-01 23:20   ` Allison Collins
2020-06-30 15:42 ` [PATCH 10/18] xfs: stop using q_core timers " Darrick J. Wong
2020-07-01  8:34   ` Chandan Babu R
2020-07-01  8:51   ` Christoph Hellwig
2020-07-01 23:20   ` Allison Collins
2020-06-30 15:43 ` [PATCH 11/18] xfs: remove qcore from incore dquots Darrick J. Wong
2020-07-01  8:34   ` Chandan Babu R
2020-07-01  8:52   ` Christoph Hellwig
2020-07-01 23:07   ` Dave Chinner
2020-07-01 23:14     ` Darrick J. Wong
2020-07-02  2:06   ` Allison Collins
2020-06-30 15:43 ` [PATCH 12/18] xfs: refactor default quota limits by resource Darrick J. Wong
2020-07-01  8:52   ` Chandan Babu R
2020-07-01  8:53   ` Christoph Hellwig
2020-07-01 23:30   ` Dave Chinner [this message]
2020-07-02  0:07     ` Darrick J. Wong
2020-07-02  2:06   ` Allison Collins
2020-06-30 15:43 ` [PATCH 13/18] xfs: remove unnecessary arguments from quota adjust functions Darrick J. Wong
2020-07-01  8:53   ` Christoph Hellwig
2020-07-01  8:58   ` Chandan Babu R
2020-07-02  2:06   ` Allison Collins
2020-06-30 15:43 ` [PATCH 14/18] xfs: refactor quota exceeded test Darrick J. Wong
2020-07-01  8:56   ` Christoph Hellwig
2020-07-01 17:51     ` Darrick J. Wong
2020-07-01 18:48       ` Eric Sandeen
2020-07-01 23:34       ` Dave Chinner
2020-07-01 10:19   ` Chandan Babu R
2020-07-02 18:57   ` Allison Collins
2020-06-30 15:43 ` [PATCH 15/18] xfs: refactor xfs_qm_scall_setqlim Darrick J. Wong
2020-07-01  8:56   ` Christoph Hellwig
2020-07-03  4:11   ` Allison Collins
2020-07-03 12:39   ` Chandan Babu R
2020-06-30 15:43 ` [PATCH 16/18] xfs: refactor xfs_trans_dqresv Darrick J. Wong
2020-07-03  4:11   ` Allison Collins
2020-07-03 13:00   ` Chandan Babu R
2020-06-30 15:43 ` [PATCH 17/18] xfs: refactor xfs_trans_apply_dquot_deltas Darrick J. Wong
2020-07-04 20:41   ` Allison Collins
2020-07-06 13:40   ` Chandan Babu R
2020-06-30 15:43 ` [PATCH 18/18] xfs: add more dquot tracepoints Darrick J. Wong
2020-07-04 20:41   ` Allison Collins
2020-07-06 13:42   ` Chandan Babu R

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200701233001.GD2005@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=darrick.wong@oracle.com \
    --cc=linux-xfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-XFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-xfs/0 linux-xfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-xfs linux-xfs/ https://lore.kernel.org/linux-xfs \
		linux-xfs@vger.kernel.org
	public-inbox-index linux-xfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-xfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git