On Sep 25, 2023, at 9:35 PM, Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx> wrote:
>
> Andreas Dilger <adilger@xxxxxxxxx> writes:
>
>> On Sep 19, 2023, at 11:39 PM, Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx> wrote:
>>>
>>> Bobi Jam <bobijam@xxxxxxxxxxx> writes:
>>>
>>>> Non-metadata block allocation does not allocate from the IOPS groups
>>>> if non-IOPS groups are not used up.
>>>
>>>> Add for mke2fs an option to mark which blocks are in the IOPS region
>>>> of storage at format time:
>>>>
>>>> -E iops=0-1024G,4096-8192G
>>>>
>>>> so the ext4 mballoc code can then use the EXT4_BG_IOPS flag in the
>>>> group descriptors to decide which groups to allocate dynamic
>>>> filesystem metadata.
>>>>
>>>> Signed-off-by: Bobi Jam <bobijam@xxxxxxxxxxx
>>>>
>>>> --
>>>> v2->v3: add sysfs mb_enable_iops_data to enable data block allocation
>>>> from IOPS groups.
>>>> v1->v2: for metadata block allocation, search in IOPS list then normal
>>>> list.
>>>> ---
>>>>
>
>>>> @@ -1009,11 +1108,37 @@ static void ext4_mb_choose_next_group(struct ext4_allocation_context *ac,
>>>> return;
>>>> }
>>>>
>>>> + if (alloc_metadata && sbi->s_es->s_flags & EXT2_FLAGS_HAS_IOPS) {
>>>> + if (*new_cr == 0)
>>>> + ret = ext4_mb_choose_next_iops_group_cr0(ac, group);
>>>> + if (!ret && *new_cr < 2)
>>>> + ret = ext4_mb_choose_next_iops_group_cr1(ac, group);
>>
>> It looks like this patch is a bit stale since Ojaswin's renaming of the
>> cr0/cr1 phases to "p2_aligned" and "goal_fast" and "best_avail" names.
>>
>
> Yup. We should rebase our development effort to latest tree.
>
>>> This is a bit confusing here. Say if *new_cr = 0 fails, then we return
>>> ret = false and fallback to choosing xx_iops_group_cr1(). And say if we
>>> were able to find a group which satisfies this allocation request we
>>> return. But the caller never knows that we allocated using cr1 and not
>>> cr0. Because we never updated *new_cr inside xx_iops_group_crX()
>>
>> I guess it is a bit messy, since updating new_cr might interfere with the
>> use of new_cr in the fallthrough to the non-IOPS groups below? In the
>> "1% IOPS groups" case, doing this extra scan for metadata blocks should
>> be very fast, since the metadata allocations are almost always one block
>> (excluding large xattrs), so the only time this would fail is if no IOPS
>> blocks are free, in which case it is fast since the group lists are empty.
>>
>
> What I was suggesting was we never update *new_cr value when we were
> able to find a suitable group. That will confuse the two for loops we
> have in the caller. We might as well just update the *new_cr value once
> we have identified a suitable group in cr0 or cr1 before returning.
>
>> We _could_ have a separate (in effect) cr0_3 and cr0_6 phases before the
>> non-IOPS group allocation starts to be able to distinguish these cases
>> (i.e. skip IOPS group scans if they are full) and the fallthrough search
>> is also having trouble to find a single free block for the metadata, but
>> I think that is pretty unlikely.
I'm not clear which option you prefer here?
- update *new_cr based on the scan in the IOPS groups (in which case the
fallback to non-IOPS groups would start at a higher CR than necessary)
- add new phases *before* CR_POWER2_ALIGNED, e.g. "CR_IOPS_POWER2_ALIGNED"
"CR_IOPS_CR_GOAL_LEN_FAST" and "CR_IOPS_CR_GOAL_LEN_SLOW" to do either
a fast scan or a slow scan on the IOPS groups, and then fall back to
non-IOPS groups? They would be skipped if no IOPS groups exist.
The second option allows preserving the CR value across the loops, in case
the group returned is not suitable for some reason, without confusing whether
the lookup is being done for IOPS groups or not. Also, it makes sense to
have a "SLOW" pass on the IOPS groups, instead of just "FAST", to ensure
that all of the IOPS groups have been tried. This should be very rare
since most allocations (excluding xattrs) are only one block long.
Ojaswin, do you have any input here? You've been doing somework on the
mballoc code recently, and it would be good to get this aligned with what
you are doing/planning.
>>>> if (*new_cr == 0) {
>>>> ext4_mb_choose_next_group_cr0(ac, new_cr, group, ngroups);
>>>> } else if (*new_cr == 1) {
>>>> ext4_mb_choose_next_group_cr1(ac, new_cr, group, ngroups);
>>>> } else {
>>>> + /*
>>>> + * Cannot get data group from slow storage, try IOPS storage
>>>> + */
>>>> + if (sbi->s_es->s_flags & EXT2_FLAGS_HAS_IOPS &&
>>>> + !alloc_metadata && sbi->s_mb_enable_iops_data &&
>>>> + *new_cr == 3) {
>>>> + if (ac->ac_2order)
>>>> + ret = ext4_mb_choose_next_iops_group_cr0(ac,
>>>> + group);
>>>> + if (!ret)
>>>> + ext4_mb_choose_next_iops_group_cr1(ac, group);
>>>> + }
>>>
>>> We might never come here in this else case because
>>> should_optimize_scan() which we check in the beginning of this function
>>> will return 0 and we will chose a next linear group for CR >= 2.
>>
>> Hmm, you are right. Just off the bottom of this hunk is a "WARN_ON(1)"
>> that this code block should never be entered.
>
> right.
>
>>
>> Really, the fallback to IOPS groups for regular files should only happen
>> in case of if *new_cr >= CR_GOAL_ANY_FREE. We don't want "normal" block
>> allocation to fill up the IOPS groups just because the filesystem is
>> fragmented and low on space, but only if out of non-IOPS space.
>>
>
> Sure, I have added some comments later on this policy part...
>
>>>>
>>>> @@ -2498,6 +2629,10 @@ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac,
>>>> goto out;
>>>> if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
>>>> goto out;
>>>> + if (sbi->s_es->s_flags & EXT2_FLAGS_HAS_IOPS &&
>>>> + (ac->ac_flags & EXT4_MB_HINT_DATA) && EXT4_MB_GRP_TEST_IOPS(grp) &&
>>>> + !sbi->s_mb_enable_iops_data)
>>>> + goto out;
>>>
>>> since we already have a grp information here. Then checking for s_flags
>>> and is redundant here right?
>>
>> This is intended to stop regular data allocations in IOPS groups that are
>> found by next_linear_group().
>
> What I meant is EXT4_MB_GRP_TEST_IOPS(grp), will only be true when we
> have sbi->s_es->s_flags & EXT2_FLAGS_HAS_IOPS as true right?
> So do we still need to keep both conditions here?
Well, EXT2_FLAGS_HAS_IOPS determines whether this functionality is enabled
or not, while the GRP_TEST_IOPS check is for the individual group. So if
the feature is totally disabled (no EXT2_FLAGS_HAS_IOPS) then the per-group
bit should be ignored.
>> With the change to allow regular data to be allocated in IOPS groups,
>> there might need to be an extra check added here to see what allocation
>> phase this is. Should we add *higher* CR_ phases above CR_ANY_FREE to
>> allow distinguishing between IOPS->regular fallback and regular->IOPS
>> fallback?
>>
>>
>> It seems like most of the complexity/issues here have crept in since the
>> addition of the fallback for regular data allocations in IOPS groups...
>> I'm not sure if we want to defer that functionality to a separate patch,
>> or if you have any suggestions on how to clarify this without adding a
>> lot of complexity?
>
> I agree that the separation is not clear. I think it would have been
> better if we would have split that functionality in 2 separate patches.
> The 1st patch just adds the functionality that you intended i.e.
>
> 1. metadata allocations should happen via IOPS group and only if there
> is no space left in IOPS group it will fallback to non-IOPS group.
> This 1st patch also have data allocations coming only from non-iops group.
>
> and the second patch can have details of...
> 2. adding a knob which can allow users to fallback data allocations to IOPS group too.
Sure, I would be happy with that. My main goal is to reserve these flags
and get this feature working in a basic fashion, and then more elaborate
policy decisions can be added once there is a demand for it.
> If you think you would like to defer the second patch to later to avoid
> the complexity, I am fine with that too. The reason is we should still
> think upon what should be the fallback critera for that. Should we do it
> when we absolutely have no space in non-IOPS group (cr >= CR_ANY_FREE)
> or is it ok to fallback even earlier. I guess it will also depend upon
> the information of how many groups are IOPS v/s non-IOPS.
>
> I don't think we are keeping that information anywhere on disk right?
> (no. of IOPS v/s non-IOPS groups). That means we might have to do that
> at runtime. Once we have that information, the filesystem can better
> decide on when should the fallback happen.
Mount already scans all of the groups at mount to set the IOPS flags in
the in-memory group_info, so the count of IOPS groups vs. non-IOPS could
be easily determined, if there is a use for this.
> So I agree, we need to more discussion and think it through. I guess Ted
> was also suggesting the same on the call. Feel free to defer the
> fallback of data allocations to non-IOPS group for a later time (If
> we don't have a strong objection from others on this).
Great, thanks for your review and feedback.
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
