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
