Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Apr 2023 12:02:44 +0530
From:   Ojaswin Mujoo <ojaswin@linux.ibm.com>
To:     Jan Kara <jack@suse.cz>
Cc:     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: <ZEDcjKUG3OjK9hg9@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
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>
 <20230323105537.rrecw5xqqzmw567d@quack3>
 <ZB8IB14yLaoY4+19@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: <ZB8IB14yLaoY4+19@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Precedence: bulk

On Sat, Mar 25, 2023 at 08:12:36PM +0530, Ojaswin Mujoo wrote:
> On Thu, Mar 23, 2023 at 11:55:37AM +0100, Jan Kara wrote:
> > 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.
> Alright, got it.
> > 
> > > > 
> > > >                 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 :)
> haha that's true. Anyways, I'll try a few things and see what looks
> good. Thanks for the suggestions.
Hey Jan,

So I was playing around with this and I prepare a patch to convert CR
numbers to symbolic names and it looks good as far as things like these
are concerned:

  if (cr < CR_POWER2_ALIGNED)
		...

However there's one problem that this numeric naming scheme is used in
several places like struct member names, function names, traces and
comments. The issue is that replacing it everywhere is making some of
the names very long for example:

	atomic_read(&sbi->s_bal_cr0_bad_suggestions));

becomes:

	atomic_read(&sbi->s_bal_cr_power2_aligned_bad_suggestions));

And this is kind of making the code look messy at a lot of places. So
right now there are a few options we can consider:

1. Use symbolic names everywhere at the cost of readability

2. Keep function names/members as is but change criterias enums to symbolic
names. This again is not ideal as it makes things ambiguous.

3. Keep the enums as is i.e. CR0, CR1.. etc and add the documentation in the enum
declaration on what these enum means. (we can still use CR_FAST_SCAN).

Let me know you thoughts on this, or incase you'd like to look at the
patch I can attach that as well.

Regards,
ojaswin
> 
> Regards,
> ojaswin
> > 
> > 								Honza
> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR