linux-ext4 - 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: <20230309121122.vzfswandgqqm4yk5@quack3>
Date:   Thu, 9 Mar 2023 13:11:22 +0100
From:   Jan Kara <jack@...e.cz>
To:     Ojaswin Mujoo <ojaswin@...ux.ibm.com>
Cc:     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,
        Jan Kara <jack@...e.cz>, Ritesh Harjani <ritesh.list@...il.com>
Subject: Re: [RFC 04/11] ext4: Convert mballoc cr (criteria) to enum

On Fri 27-01-23 18:07:31, Ojaswin Mujoo wrote:
> Convert criteria to be an enum so it easier to maintain. This change
> also makes it easier to insert new criterias in the future.
> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>

Just two small comments below:

> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index b8b00457da8d..6037b8e0af86 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -126,6 +126,14 @@ enum SHIFT_DIRECTION {
>  	SHIFT_RIGHT,
>  };
>  
> +/*
> + * Number of criterias defined. For each criteria, mballoc has slightly
> + * different way of finding the required blocks nad usually, higher the
						   ^^^ and

> + * criteria the slower the allocation. We start at lower criterias and keep
> + * falling back to higher ones if we are not able to find any blocks.
> + */
> +#define EXT4_MB_NUM_CRS 4
> +

So defining this in a different header than the enum itself is fragile. I
understand you need it in ext4_sb_info declaration so probably I'd move the
enum declaration to ext4.h. Alternatively I suppose we could move a lot of
mballoc stuff out of ext4_sb_info into a separate struct because there's a
lot of it. But that would be much larger undertaking.

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:

                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?

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