Re: [PATCH] tune2fs: do not update quota when not needed

[Jan Kara <jack@suse.cz>]

On Fri 30-08-24 11:59:21, Gwendal Grignou wrote:
> Enabling quota is expensive: All inodes in the filesystem are scanned.
> Only do it when the requested quota configuration does not match the
> existing configuration.
> 
> Test:
> Add a tiny patch to print out when core of function
> handle_quota_options() is triggered.
> Issue commands:
> truncate -s 1G unused ; mkfs.ext4 unused
> 
> | commands                                                | trigger | comments
> +---------------------------------------------------------+---------+---------
> | tune2fs -Qusrquota,grpquota -Qprjquota -O quota unused  | Y       |
>                   Quota not set at formatting.
> | tune2fs -Qusrquota,grpquota -Qprjquota -O quota unused  | N       |
>                   Already set just above
> | tune2fs -Qusrquota,grpquota -Q^prjquota -O quota unused | Y       |
>                   Disabling a quota option always force a deep look.
> | tune2fs -Qusrquota,grpquota -Q^prjquota -O quota unused | Y       |
>                   See just above
> | tune2fs -Qusrquota,grpquota -O quota unused             | N       |
>                   No change from previous line.
> | tune2fs -Qusrquota,^grpquota -O quota unused            | Y       |
>                   Disabling a quota option always force a deep look.
> | tune2fs -Qusrquota -O quota unused                      | N       |
>                   No change from previous line.
> | tune2fs -O ^quota unused                                | Y       |
>                   Remove quota
> | tune2fs -O quota unused                                 | X       |
>                   function handle_quota_options() not called, default values
>                   (-Qusrquota,grpquota) used.
> | tune2fs -O quota -Qusrquota unused                      | N       |
>                   Already set just above

Good idea. One comment regarding the code:

> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index 6de40e93..3cce8861 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -1804,6 +1804,41 @@ static int handle_quota_options(ext2_filsys fs)
>  			qtype_bits |= 1 << qtype;
>  	}
>  
> +	/*
> +	 * Check if the filesystem already has quota enabled and more features
> +	 * need to be enabled and are not, or some features need to be disabled.
> +	 */
> +	if (ext2fs_has_feature_quota(fs->super) && qtype_bits) {
> +		for (qtype = 0 ; qtype < MAXQUOTAS; qtype++) {
> +			if ((quota_enable[qtype] == QOPT_ENABLE &&
> +			     *quota_sb_inump(fs->super, qtype) == 0) ||
> +			    (quota_enable[qtype] == QOPT_DISABLE)) {
> +				/* Some work needed to match the configuration. */
> +				break;
> +			}
> +		}
> +		if (qtype == MAXQUOTAS) {
> +			/* Nothing to do. */
> +			return 0;
> +		}
> +	}
> +	/*
> +	 * Check if the user wants all features disabled and it is already
> +	 * the case.
> +	 */
> +	if (!ext2fs_has_feature_quota(fs->super) && !qtype_bits) {
> +		for (qtype = 0 ; qtype < MAXQUOTAS; qtype++) {
> +			if (*quota_sb_inump(fs->super, qtype)) {
> +				/* Some work needed to match the configuration. */
> +				break;
> +			}
> +		}
> +		if (qtype == MAXQUOTAS) {
> +			/* Nothing to do. */
> +			return 0;
> +		}
> +	}

Why don't you do it like:

	for (qtype = 0 ;qtype < MAXQUOTAS; qtype++) {
		if (quota_enable[qtype] == QOPT_ENABLE &&
		     *quota_sb_inump(fs->super, qtype) == 0) {
			/* Need to enable this quota type. */
			break;
		}
		if (quota_enable[qtype] == QOPT_DISABLE &&
		    *quota_sb_inump(fs->super, qtype)) {
			/* Need to disable this quota type. */
			break;
		}
	}
	if (qtype == MAXQUOTAS) {
		/* Nothing to do. */
		return 0;
	}

As far as I can tell this should result in similar decisions and should be
much easier to understand what's going on...

								Honza

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