| Message ID | 20221027032435.27374-1-guoqing.jiang@linux.dev |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: make ext4_mb_initialize_context return void | expand |
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. Thanks, Jason > > ac->ac_op = EXT4_MB_HISTORY_PREALLOC; > seq = this_cpu_read(discard_pa_seq); >
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 Is there any place where don't set it accordfingly? If so, the below should be kept. *errp = ext4_mb_initialize_context(ac, ar); Thanks, Guoqing
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; } ... } So it seems like this cleanup won't alter the behavior. Feel free to add: Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> Regards, ojaswin
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
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); ac->ac_op = EXT4_MB_HISTORY_PREALLOC; seq = this_cpu_read(discard_pa_seq);
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(-)