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@xxxxxxxx>
SUSE Labs, CR
