* [PATCH] tune2fs: do not update quota when not needed
@ 2024-08-30 18:59 Gwendal Grignou
2024-09-12 9:15 ` Jan Kara
0 siblings, 1 reply; 7+ messages in thread
From: Gwendal Grignou @ 2024-08-30 18:59 UTC (permalink / raw)
To: tytso, uekawa; +Cc: linux-ext4, Gwendal Grignou
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(+)
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,
--
2.46.0.469.g59c65b2a67-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] tune2fs: do not update quota when not needed
2024-08-30 18:59 [PATCH] tune2fs: do not update quota when not needed Gwendal Grignou
@ 2024-09-12 9:15 ` Jan Kara
2024-10-25 2:53 ` Theodore Ts'o
2025-01-03 23:50 ` [PATCH v2] " Gwendal Grignou
0 siblings, 2 replies; 7+ messages in thread
From: Jan Kara @ 2024-09-12 9:15 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: tytso, uekawa, linux-ext4
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tune2fs: do not update quota when not needed
2024-09-12 9:15 ` Jan Kara
@ 2024-10-25 2:53 ` Theodore Ts'o
2025-01-03 23:47 ` Gwendal Grignou
2025-01-03 23:50 ` [PATCH v2] " Gwendal Grignou
1 sibling, 1 reply; 7+ messages in thread
From: Theodore Ts'o @ 2024-10-25 2:53 UTC (permalink / raw)
To: Jan Kara; +Cc: Gwendal Grignou, uekawa, linux-ext4
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tune2fs: do not update quota when not needed
2024-10-25 2:53 ` Theodore Ts'o
@ 2025-01-03 23:47 ` Gwendal Grignou
0 siblings, 0 replies; 7+ messages in thread
From: Gwendal Grignou @ 2025-01-03 23:47 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Jan Kara, uekawa, linux-ext4
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] tune2fs: do not update quota when not needed
2024-09-12 9:15 ` Jan Kara
2024-10-25 2:53 ` Theodore Ts'o
@ 2025-01-03 23:50 ` Gwendal Grignou
2025-01-06 10:10 ` Jan Kara
2025-05-21 14:51 ` Theodore Ts'o
1 sibling, 2 replies; 7+ messages in thread
From: Gwendal Grignou @ 2025-01-03 23:50 UTC (permalink / raw)
To: jack; +Cc: gwendal, linux-ext4, tytso, uekawa
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
| tune2fs -Qusrquota,grpquota -Q^prjquota -O quota unused | N |
No change from previous line.
| tune2fs -Qusrquota,grpquota -O quota unused | N |
No change from previous line.
| tune2fs -Qusrquota,^grpquota -O quota unused | Y |
Disabling a quota
| tune2fs -Qusrquota -O quota unused | N |
No change from previous line.
| tune2fs -O ^quota unused | Y |
Remove quota
| tune2fs -O quota unused | Y |
Re-enable quota, default values
(-Qusrquota,grpquota) used.
| tune2fs -O quota -Qusrquota unused | N |
Already set just above
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
Changes in v2:
Logic has been simplified, based on jack@suse.cz feedback.
misc/tune2fs.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index 2548a766..3db57632 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -1799,11 +1799,27 @@ static int handle_quota_options(ext2_filsys fs)
return 1;
}
+ for (qtype = 0; qtype < MAXQUOTAS; qtype++) {
+ if (quota_enable[qtype] == QOPT_ENABLE &&
+ *quota_sb_inump(fs->super, qtype) == 0) {
+ /* Some work needed to match the configuration. */
+ break;
+ }
+ if (quota_enable[qtype] == QOPT_DISABLE &&
+ *quota_sb_inump(fs->super, qtype)) {
+ /* Some work needed to match the configuration. */
+ break;
+ }
+ }
+ if (qtype == MAXQUOTAS) {
+ /* Nothing to do. */
+ return 0;
+ }
+
for (qtype = 0; qtype < MAXQUOTAS; qtype++) {
if (quota_enable[qtype] == QOPT_ENABLE)
qtype_bits |= 1 << qtype;
}
-
retval = quota_init_context(&qctx, fs, qtype_bits);
if (retval) {
com_err(program_name, retval,
--
2.47.1.613.gc27f4b7a9f-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tune2fs: do not update quota when not needed
2025-01-03 23:50 ` [PATCH v2] " Gwendal Grignou
@ 2025-01-06 10:10 ` Jan Kara
2025-05-21 14:51 ` Theodore Ts'o
1 sibling, 0 replies; 7+ messages in thread
From: Jan Kara @ 2025-01-06 10:10 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: jack, linux-ext4, tytso, uekawa
On Fri 03-01-25 15:50:42, 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
> | tune2fs -Qusrquota,grpquota -Q^prjquota -O quota unused | N |
> No change from previous line.
> | tune2fs -Qusrquota,grpquota -O quota unused | N |
> No change from previous line.
> | tune2fs -Qusrquota,^grpquota -O quota unused | Y |
> Disabling a quota
> | tune2fs -Qusrquota -O quota unused | N |
> No change from previous line.
> | tune2fs -O ^quota unused | Y |
> Remove quota
> | tune2fs -O quota unused | Y |
> Re-enable quota, default values
> (-Qusrquota,grpquota) used.
> | tune2fs -O quota -Qusrquota unused | N |
> Already set just above
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
>
> ---
> Changes in v2:
> Logic has been simplified, based on jack@suse.cz feedback.
>
> misc/tune2fs.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index 2548a766..3db57632 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -1799,11 +1799,27 @@ static int handle_quota_options(ext2_filsys fs)
> return 1;
> }
>
> + for (qtype = 0; qtype < MAXQUOTAS; qtype++) {
> + if (quota_enable[qtype] == QOPT_ENABLE &&
> + *quota_sb_inump(fs->super, qtype) == 0) {
> + /* Some work needed to match the configuration. */
> + break;
> + }
> + if (quota_enable[qtype] == QOPT_DISABLE &&
> + *quota_sb_inump(fs->super, qtype)) {
> + /* Some work needed to match the configuration. */
> + break;
> + }
> + }
> + if (qtype == MAXQUOTAS) {
> + /* Nothing to do. */
> + return 0;
> + }
> +
> for (qtype = 0; qtype < MAXQUOTAS; qtype++) {
> if (quota_enable[qtype] == QOPT_ENABLE)
> qtype_bits |= 1 << qtype;
> }
> -
> retval = quota_init_context(&qctx, fs, qtype_bits);
> if (retval) {
> com_err(program_name, retval,
> --
> 2.47.1.613.gc27f4b7a9f-goog
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tune2fs: do not update quota when not needed
2025-01-03 23:50 ` [PATCH v2] " Gwendal Grignou
2025-01-06 10:10 ` Jan Kara
@ 2025-05-21 14:51 ` Theodore Ts'o
1 sibling, 0 replies; 7+ messages in thread
From: Theodore Ts'o @ 2025-05-21 14:51 UTC (permalink / raw)
To: jack, Gwendal Grignou; +Cc: Theodore Ts'o, linux-ext4, uekawa
On Fri, 03 Jan 2025 15:50:42 -0800, 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
>
> [...]
Applied, thanks!
[1/1] tune2fs: do not update quota when not needed
commit: 4ea1d7f202bb4350e8f517e604c6300a521a762c
Best regards,
--
Theodore Ts'o <tytso@mit.edu>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-05-21 14:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-30 18:59 [PATCH] tune2fs: do not update quota when not needed Gwendal Grignou
2024-09-12 9:15 ` Jan Kara
2024-10-25 2:53 ` Theodore Ts'o
2025-01-03 23:47 ` Gwendal Grignou
2025-01-03 23:50 ` [PATCH v2] " Gwendal Grignou
2025-01-06 10:10 ` Jan Kara
2025-05-21 14:51 ` Theodore Ts'o
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).