Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 23 Mar 2023 11:55:37 +0100
From:   Jan Kara <jack@suse.cz>
To:     Ojaswin Mujoo <ojaswin@linux.ibm.com>
Cc:     Jan Kara <jack@suse.cz>, linux-ext4@vger.kernel.org,
        Theodore Ts'o <tytso@mit.edu>,
        Ritesh Harjani <riteshh@linux.ibm.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Ritesh Harjani <ritesh.list@gmail.com>
Subject: Re: [RFC 04/11] ext4: Convert mballoc cr (criteria) to enum
Message-ID: <20230323105537.rrecw5xqqzmw567d@quack3>
References: <cover.1674822311.git.ojaswin@linux.ibm.com>
 <9670431b31aa62e83509fa2802aad364910ee52e.1674822311.git.ojaswin@linux.ibm.com>
 <20230309121122.vzfswandgqqm4yk5@quack3>
 <ZBRAZsvbcSBNJ+Pl@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZBRAZsvbcSBNJ+Pl@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Precedence: bulk

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@suse.com>
SUSE Labs, CR