| Message ID | 20240830185921.2690798-1-gwendal@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | tune2fs: do not update quota when not needed | expand |
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
On Thu, Sep 12, 2024 at 11:15:58AM +0200, Jan Kara wrote: > 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. I'm not sure why disabling a quota should always "force a deep look"? What was your thinking behind this change? > 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; > } I like Jan's suggestion here, and this could easily just be done at the beginning of handle_quota_options(), replacing what's currently there: for (qtype = 0 ; qtype < MAXQUOTAS; qtype++) if (quota_enable[qtype] != 0) break; if (qtype == MAXQUOTAS) /* Nothing to do. */ return 0; It results in simpler and cleaner code, I think.... - Ted
On Thu, Oct 24, 2024 at 7:53 PM Theodore Ts'o <tytso@mit.edu> wrote: > > On Thu, Sep 12, 2024 at 11:15:58AM +0200, Jan Kara wrote: > > 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. > > I'm not sure why disabling a quota should always "force a deep look"? > What was your thinking behind this change? To execute the same code path, in particular, when we disable `prjquota`, ext2fs_clear_feature_project() is always called, even when the quota inodes were already removed. Rereading the code, that seems unnecessary, so Jan's solution is indeed better. Sending a V2 shortly. Gwendal. > > > 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; > > } > > I like Jan's suggestion here, and this could easily just be done at > the beginning of handle_quota_options(), replacing what's currently > there: > > for (qtype = 0 ; qtype < MAXQUOTAS; qtype++) > if (quota_enable[qtype] != 0) > break; > if (qtype == MAXQUOTAS) > /* Nothing to do. */ > return 0; > > It results in simpler and cleaner code, I think.... > > - Ted
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; + } + } + retval = quota_init_context(&qctx, fs, qtype_bits); if (retval) { com_err(program_name, retval,
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 --- misc/tune2fs.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)