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] [day] [month] [year] [list]

Message-ID: <20241025025355.GR3204734@mit.edu>
Date: Thu, 24 Oct 2024 22:53:55 -0400
From: "Theodore Ts'o" <tytso@....edu>
To: Jan Kara <jack@...e.cz>
Cc: Gwendal Grignou <gwendal@...omium.org>, uekawa@...omium.org,
        linux-ext4@...r.kernel.org
Subject: Re: [PATCH] tune2fs: do not update quota when not needed

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