linux-ext4 - Re: [PATCH] tune2fs: do not update quota when not needed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPUE2usUprLQboD74x+_137fVmsYVmFULdqKSXG9wM79dXLE-Q@mail.gmail.com>
Date: Fri, 3 Jan 2025 15:47:55 -0800
From: Gwendal Grignou <gwendal@...omium.org>
To: "Theodore Ts'o" <tytso@....edu>
Cc: Jan Kara <jack@...e.cz>, uekawa@...omium.org, linux-ext4@...r.kernel.org
Subject: Re: [PATCH] tune2fs: do not update quota when not needed

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