linux-kernel - Re: [RFC 04/11] ext4: Convert mballoc cr (criteria) to enum

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: <20230323105537.rrecw5xqqzmw567d@quack3>
Date:   Thu, 23 Mar 2023 11:55:37 +0100
From:   Jan Kara <jack@...e.cz>
To:     Ojaswin Mujoo <ojaswin@...ux.ibm.com>
Cc:     Jan Kara <jack@...e.cz>, linux-ext4@...r.kernel.org,
        Theodore Ts'o <tytso@....edu>,
        Ritesh Harjani <riteshh@...ux.ibm.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ritesh Harjani <ritesh.list@...il.com>
Subject: Re: [RFC 04/11] ext4: Convert mballoc cr (criteria) to enum

On Fri 17-03-23 15:56:46, Ojaswin Mujoo wrote:
> On Thu, Mar 09, 2023 at 01:11:22PM +0100, Jan Kara wrote:
> > Also when going for symbolic allocator scan names maybe we could actually
> > make names sensible instead of CR[0-4]? Perhaps like CR_ORDER2_ALIGNED,
> > CR_BEST_LENGHT_FAST, CR_BEST_LENGTH_ALL, CR_ANY_FREE. And probably we could
> > deal with ordered comparisons like in:
> I like this idea, it should make the code a bit more easier to
> understand. However just wondering if I should do it as a part of this
> series or a separate patch since we'll be touching code all around and 
> I don't want to confuse people with the noise :) 

I guess a mechanical rename should not be really confusing. It just has to
be a separate patch.

> > 
> >                 if (cr < 2 &&
> >                     (!sbi->s_log_groups_per_flex ||
> >                      ((group & ((1 << sbi->s_log_groups_per_flex) - 1)) != 0)) &
> >                     !(ext4_has_group_desc_csum(sb) &&
> >                       (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT))))
> >                         return 0;
> > 
> > to declare CR_FAST_SCAN = 2, or something like that. What do you think?
> About this, wont it be better to just use something like
> 
> cr < CR_BEST_LENGTH_ALL 
> 
> instead of defining a new CR_FAST_SCAN = 2.

Yeah, that works as well.

> The only concern is that if we add a new "fast" CR (say between
> CR_BEST_LENGTH_FAST and CR_BEST_LENGTH_ALL) then we'll need to make
> sure we also update CR_FAST_SCAN to 3 which is easy to miss.

Well, you have that problem with any naming scheme (and even with numbers).
So as long as names are all defined together, there's reasonable chance
you'll remember to verify the limits still hold :)

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR