On Thu, Oct 24, 2024 at 7:53 PM Theodore Ts'o <tytso@xxxxxxx> 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
