ext4: make ext4_mb_initialize_context return void
PATCH 1/1 ext4: make ext4_mb_initialize_context return void
Change the return type to void since it always return 0, and no need to do the checking in ext4_mb_new_blocks.
--- fs/ext4/mballoc.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
fs/ext4/mballoc.c +2 -8
@@ -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 voidext4_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);
Signature · 1 line
2.31.1
Quoted 31 lines · Guoqing Jiang
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 <[email protected]>--- fs/ext4/mballoc.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)fs/ext4/mballoc.c +1
-3@@ -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 voidext4_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
fs/ext4/mballoc.c ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle, · line 5592
Quoted 9 lines
fs/ext4/mballoc.c +1
-5@@ -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
Quoted 4 lines
fs/ext4/mballoc.c +0
-0ac->ac_op =EXT4_MB_HISTORY_PREALLOC ; seq =this_cpu_read (discard_pa_seq);
Quoted 35 lines · Jason Yan
Quoted 33 lines · Guoqing Jiang
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 <[email protected]>---
fs/ext4/mballoc.c| 10 ++--------1 file changed, 2 insertions(+), 8 deletions(-)fs/ext4/mballoc.c +0
-0@@ -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_stackint
+staticnoinline_for_stackvoidext4_mb_initialize_context(struct ext4_allocation_context *ac, struct ext4_allocation_request *ar){fs/ext4/mballoc.c +0
-2@@ -5253,8 +5253,6 @@ ext4_mb_initialize_context(structext4_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_stackvoidfs/ext4/mballoc.c +0
-0@@ -5591,11 +5589,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle, goto out; }
Quoted 8 lines
fs/ext4/mballoc.c ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle, · line 5592
Quoted 6 lines
fs/ext4/mballoc.c +1
-4- *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
Quoted 49 lines · Guoqing Jiang
Quoted 37 lines · Jason Yan
Quoted 35 lines · Guoqing Jiang
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 <[email protected]>---
fs/ext4/mballoc.c| 10 ++--------1 file changed, 2 insertions(+), 8 deletions(-)fs/ext4/mballoc.c +0
-0@@ -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_stackint
+staticnoinline_for_stackvoidext4_mb_initialize_context(struct ext4_allocation_context *ac, struct ext4_allocation_request *ar){fs/ext4/mballoc.c +0
-2@@ -5253,8 +5253,6 @@ ext4_mb_initialize_context(structext4_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_stackvoidfs/ext4/mballoc.c +0
-1@@ -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) {Quoted 8 lines
fs/ext4/mballoc.c ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle, · line 5592
Quoted 4 lines
fs/ext4/mballoc.c +1
-3- 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.
Quoted 6 lines
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:
Regards,
ojaswin
Hi Ojaswin,
Quoted 76 lines · Ojaswin Mujoo
Quoted 45 lines · Guoqing Jiang
Quoted 35 lines · Jason Yan
Quoted 34 lines · Guoqing Jiang
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 <[email protected]>--- fs/ext4/mballoc.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)fs/ext4/mballoc.c +1
-2@@ -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(structext4_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);Quoted 8 lines
fs/ext4/mballoc.c ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle, · line 5592
Quoted 5 lines
fs/ext4/mballoc.c +1
-4- 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.
Quoted 6 lines
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#L5646Hi 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; } ...
Quoted 1 line
}
Yes, thanks for the above.
Quoted 4 lines
So it seems like this cleanup won't alter the behavior. Feel free to, add:
Reviewed-by Ojaswin Mujoo <[email protected]>
Appreciate for your review!
Thanks,
Guoqing