Re: [PATCH] ext4: make ext4_mb_initialize_context return void

[Guoqing Jiang <guoqing.jiang@linux.dev>]

Hi Ojaswin,

On 10/28/22 6:54 PM, Ojaswin Mujoo wrote:
> On Thu, Oct 27, 2022 at 04:12:45PM +0800, Guoqing Jiang wrote:
>>
>> On 10/27/22 2:29 PM, Jason Yan wrote:
>>> On 2022/10/27 11:24, Guoqing Jiang wrote:
>>>> Change the return type to void since it always return 0, and no need
>>>> to do the checking in ext4_mb_new_blocks.
>>>>
>>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
>>>> ---
>>>>    fs/ext4/mballoc.c | 10 ++--------
>>>>    1 file changed, 2 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>>>> index 9dad93059945..5b2ae37a8b80 100644
>>>> --- a/fs/ext4/mballoc.c
>>>> +++ b/fs/ext4/mballoc.c
>>>> @@ -5204,7 +5204,7 @@ static void ext4_mb_group_or_file(struct
>>>> ext4_allocation_context *ac)
>>>>        mutex_lock(&ac->ac_lg->lg_mutex);
>>>>    }
>>>>    -static noinline_for_stack int
>>>> +static noinline_for_stack void
>>>>    ext4_mb_initialize_context(struct ext4_allocation_context *ac,
>>>>                    struct ext4_allocation_request *ar)
>>>>    {
>>>> @@ -5253,8 +5253,6 @@ ext4_mb_initialize_context(struct
>>>> ext4_allocation_context *ac,
>>>>                (unsigned) ar->lleft, (unsigned) ar->pleft,
>>>>                (unsigned) ar->lright, (unsigned) ar->pright,
>>>>                inode_is_open_for_write(ar->inode) ? "" : "non-");
>>>> -    return 0;
>>>> -
>>>>    }
>>>>      static noinline_for_stack void
>>>> @@ -5591,11 +5589,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
>>>>            goto out;
>>>>        }
>>>>    -    *errp = ext4_mb_initialize_context(ac, ar);
>>>> -    if (*errp) {
>>>> -        ar->len = 0;
>>>> -        goto out;
>>>> -    }
>>>> +    ext4_mb_initialize_context(ac, ar);
>>> This changed the logic here slightly. *errp will not be intialized with
>>> zero after this change. So we need to carefully check whether this will
>>> cause any issues.
>> Yes, thanks for reminder. I think "*errp" is always set later with below.
>>
>> https://elixir.bootlin.com/linux/v6.1-rc2/source/fs/ext4/mballoc.c#L5606
>> https://elixir.bootlin.com/linux/v6.1-rc2/source/fs/ext4/mballoc.c#L5611
>> https://elixir.bootlin.com/linux/v6.1-rc2/source/fs/ext4/mballoc.c#L5629
>> https://elixir.bootlin.com/linux/v6.1-rc2/source/fs/ext4/mballoc.c#L5646
> Hi Guoqing,
>
> I agree, it seems to be intialized correctly later in the code. The
> flow is something like:
>
>    ext4_fsblk_t ext4_mb_new_blocks(...)
>    {
>        ...
>        ext4_mb_initialize_context(ac, ar);
>        ...
>        if (!ext4_mb_use_preallocated(ac)) {
>            *errp = ext4_mb_pa_alloc(ac);  // *errp init to 0 on success
>            ...
>        }
>
>        if (likely(ac->ac_status == AC_STATUS_FOUND)) {
>            // *errp init to 0 on success
>            *errp = ext4_mb_mark_diskspace_used(ac, handle, reserv_clstrs);
>            ...
>        } else {
>            ...
>            *errp = -ENOSPC;
>        }
>        ...
>    }

Yes, thanks for the above.

> So it seems like this cleanup won't alter the behavior. Feel free to,
> add:
>
> Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Appreciate for your review!

Thanks,
Guoqing