| Message ID | b1eaaa193879d4ae920a76dfa3bc5f2e6c7f8a4d.1710857863.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trivial adjustments for return variable coding style | expand |
On Tue, Mar 19, 2024 at 08:25:09PM +0530, Anand Jain wrote: > Since err represents the function return value, rename it as ret, > and rename the original ret, which serves as a helper return value, > to ret2. > I hate to be that guy right out the gate, but since we're just using ret2 as the return value of radix_tree_gang_lookup, and that's just the number of fs_roots, lets instead a variation of unsigned int nr_found = 0; unsigned int nr = 0; unsigned int found = 0; since we know this isn't just a temporary ret value, it actually corresponds to something. Thanks, Josef
在 2024/3/20 01:25, Anand Jain 写道: > Since err represents the function return value, rename it as ret, > and rename the original ret, which serves as a helper return value, > to ret2. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/disk-io.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 3df5477d48a8..d28de2cfb7b4 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2918,21 +2918,21 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info) > u64 root_objectid = 0; > struct btrfs_root *gang[8]; > int i = 0; > - int err = 0; > - unsigned int ret = 0; > + int ret = 0; > + unsigned int ret2 = 0; I really hate the same @ret2. Since it's mostly the found number of radix tree gang lookup, can we change it to something like @found? > > while (1) { Another thing is, the @ret2 is only utilized inside the loop except the final cleanup. Can't we only declare @ret2 (or the new name) inside the loop and make the cleanup also happen inside the loop (or a dedicated helper?) Thanks, Qu > spin_lock(&fs_info->fs_roots_radix_lock); > - ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix, > + ret2 = radix_tree_gang_lookup(&fs_info->fs_roots_radix, > (void **)gang, root_objectid, > ARRAY_SIZE(gang)); > - if (!ret) { > + if (!ret2) { > spin_unlock(&fs_info->fs_roots_radix_lock); > break; > } > - root_objectid = gang[ret - 1]->root_key.objectid + 1; > + root_objectid = gang[ret2 - 1]->root_key.objectid + 1; > > - for (i = 0; i < ret; i++) { > + for (i = 0; i < ret2; i++) { > /* Avoid to grab roots in dead_roots. */ > if (btrfs_root_refs(&gang[i]->root_item) == 0) { > gang[i] = NULL; > @@ -2943,12 +2943,12 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info) > } > spin_unlock(&fs_info->fs_roots_radix_lock); > > - for (i = 0; i < ret; i++) { > + for (i = 0; i < ret2; i++) { > if (!gang[i]) > continue; > root_objectid = gang[i]->root_key.objectid; > - err = btrfs_orphan_cleanup(gang[i]); > - if (err) > + ret = btrfs_orphan_cleanup(gang[i]); > + if (ret) > goto out; > btrfs_put_root(gang[i]); > } > @@ -2956,11 +2956,11 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info) > } > out: > /* Release the uncleaned roots due to error. */ > - for (; i < ret; i++) { > + for (; i < ret2; i++) { > if (gang[i]) > btrfs_put_root(gang[i]); > } > - return err; > + return ret; > } > > /*
On 19/03/2024 15.55, Anand Jain wrote: > Since err represents the function return value, rename it as ret, > and rename the original ret, which serves as a helper return value, > to ret2. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/disk-io.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 3df5477d48a8..d28de2cfb7b4 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2918,21 +2918,21 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info) > u64 root_objectid = 0; > struct btrfs_root *gang[8]; > int i = 0; I suggest to change also the line above in unsigned int = 0; This to avoid a comparation signed with unsigned in the two for() loop. In this case this should be not a problem but in general is better to avoid mix signed and unsigned. > - int err = 0; > - unsigned int ret = 0; > + int ret = 0; > + unsigned int ret2 = 0; In this *specific* case, instead of renaming 'ret' in 'ret2', it would be better rename 'ret' in 'items_count' or something like that. This because radix_tree_gang_lookup() doesn't return a status (ok or an error), but the number of the items found. > > while (1) { > spin_lock(&fs_info->fs_roots_radix_lock); > - ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix, > + ret2 = radix_tree_gang_lookup(&fs_info->fs_roots_radix, > (void **)gang, root_objectid, > ARRAY_SIZE(gang)); > - if (!ret) { > + if (!ret2) { > spin_unlock(&fs_info->fs_roots_radix_lock); > break; > } > - root_objectid = gang[ret - 1]->root_key.objectid + 1; > + root_objectid = gang[ret2 - 1]->root_key.objectid + 1; > > - for (i = 0; i < ret; i++) { > + for (i = 0; i < ret2; i++) { > /* Avoid to grab roots in dead_roots. */ > if (btrfs_root_refs(&gang[i]->root_item) == 0) { > gang[i] = NULL; > @@ -2943,12 +2943,12 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info) > } > spin_unlock(&fs_info->fs_roots_radix_lock); > > - for (i = 0; i < ret; i++) { > + for (i = 0; i < ret2; i++) { Comparation signed (i) with unsigned (ret2). > if (!gang[i]) > continue; > root_objectid = gang[i]->root_key.objectid; > - err = btrfs_orphan_cleanup(gang[i]); > - if (err) > + ret = btrfs_orphan_cleanup(gang[i]); > + if (ret) > goto out; > btrfs_put_root(gang[i]); > } > @@ -2956,11 +2956,11 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info) > } > out: > /* Release the uncleaned roots due to error. */ > - for (; i < ret; i++) { > + for (; i < ret2; i++) { Comparation signed (i) with unsigned (ret2). > if (gang[i]) > btrfs_put_root(gang[i]); > } > - return err; > + return ret; > } > > /*
On 3/20/24 04:43, Qu Wenruo wrote: > > > 在 2024/3/20 01:25, Anand Jain 写道: >> Since err represents the function return value, rename it as ret, >> and rename the original ret, which serves as a helper return value, >> to ret2. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/disk-io.c | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index 3df5477d48a8..d28de2cfb7b4 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -2918,21 +2918,21 @@ static int btrfs_cleanup_fs_roots(struct >> btrfs_fs_info *fs_info) >> u64 root_objectid = 0; >> struct btrfs_root *gang[8]; >> int i = 0; >> - int err = 0; >> - unsigned int ret = 0; >> + int ret = 0; >> + unsigned int ret2 = 0; > > I really hate the same @ret2. > > Since it's mostly the found number of radix tree gang lookup, can we > change it to something like @found? > >> >> while (1) { > > Another thing is, the @ret2 is only utilized inside the loop except the > final cleanup. > > Can't we only declare @ret2 (or the new name) inside the loop and make > the cleanup also happen inside the loop (or a dedicated helper?) > Cleanup inside the loop is possible, but it would be something like below, what do you think? diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index c2dc88f909b0..d1d23736de3c 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2926,22 +2926,23 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info) { u64 root_objectid = 0; struct btrfs_root *gang[8]; - int i = 0; - int err = 0; - unsigned int ret = 0; + int ret = 0; while (1) { + unsigned int i; + unsigned int found; + spin_lock(&fs_info->fs_roots_radix_lock); - ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix, + found = radix_tree_gang_lookup(&fs_info->fs_roots_radix, (void **)gang, root_objectid, ARRAY_SIZE(gang)); - if (!ret) { + if (!found) { spin_unlock(&fs_info->fs_roots_radix_lock); break; } - root_objectid = btrfs_root_id(gang[ret - 1]) + 1; + root_objectid = btrfs_root_id(gang[found - 1]) + 1; - for (i = 0; i < ret; i++) { + for (i = 0; i < found; i++) { /* Avoid to grab roots in dead_roots. */ if (btrfs_root_refs(&gang[i]->root_item) == 0) { gang[i] = NULL; @@ -2952,24 +2953,20 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info) } spin_unlock(&fs_info->fs_roots_radix_lock); - for (i = 0; i < ret; i++) { + for (i = 0; i < found; i++) { if (!gang[i]) continue; root_objectid = btrfs_root_id(gang[i]); - err = btrfs_orphan_cleanup(gang[i]); - if (err) - goto out; + if (!ret) + ret = btrfs_orphan_cleanup(gang[i]); btrfs_put_root(gang[i]); } + if (ret) + break; + root_objectid++; } -out: - /* Release the uncleaned roots due to error. */ - for (; i < ret; i++) { - if (gang[i]) - btrfs_put_root(gang[i]); - } - return err; + return ret; } Thanks, Anand > Thanks, > Qu >> spin_lock(&fs_info->fs_roots_radix_lock); >> - ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix, >> + ret2 = radix_tree_gang_lookup(&fs_info->fs_roots_radix, >> (void **)gang, root_objectid, >> ARRAY_SIZE(gang)); >> - if (!ret) { >> + if (!ret2) { >> spin_unlock(&fs_info->fs_roots_radix_lock); >> break; >> } >> - root_objectid = gang[ret - 1]->root_key.objectid + 1; >> + root_objectid = gang[ret2 - 1]->root_key.objectid + 1; >> >> - for (i = 0; i < ret; i++) { >> + for (i = 0; i < ret2; i++) { >> /* Avoid to grab roots in dead_roots. */ >> if (btrfs_root_refs(&gang[i]->root_item) == 0) { >> gang[i] = NULL; >> @@ -2943,12 +2943,12 @@ static int btrfs_cleanup_fs_roots(struct >> btrfs_fs_info *fs_info) >> } >> spin_unlock(&fs_info->fs_roots_radix_lock); >> >> - for (i = 0; i < ret; i++) { >> + for (i = 0; i < ret2; i++) { >> if (!gang[i]) >> continue; >> root_objectid = gang[i]->root_key.objectid; >> - err = btrfs_orphan_cleanup(gang[i]); >> - if (err) >> + ret = btrfs_orphan_cleanup(gang[i]); >> + if (ret) >> goto out; >> btrfs_put_root(gang[i]); >> } >> @@ -2956,11 +2956,11 @@ static int btrfs_cleanup_fs_roots(struct >> btrfs_fs_info *fs_info) >> } >> out: >> /* Release the uncleaned roots due to error. */ >> - for (; i < ret; i++) { >> + for (; i < ret2; i++) { >> if (gang[i]) >> btrfs_put_root(gang[i]); >> } >> - return err; >> + return ret; >> } >> >> /*
在 2024/4/17 12:54, Anand Jain 写道: > On 3/20/24 04:43, Qu Wenruo wrote: >> >> >> 在 2024/3/20 01:25, Anand Jain 写道: >>> Since err represents the function return value, rename it as ret, >>> and rename the original ret, which serves as a helper return value, >>> to ret2. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> --- >>> fs/btrfs/disk-io.c | 22 +++++++++++----------- >>> 1 file changed, 11 insertions(+), 11 deletions(-) >>> >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >>> index 3df5477d48a8..d28de2cfb7b4 100644 >>> --- a/fs/btrfs/disk-io.c >>> +++ b/fs/btrfs/disk-io.c >>> @@ -2918,21 +2918,21 @@ static int btrfs_cleanup_fs_roots(struct >>> btrfs_fs_info *fs_info) >>> u64 root_objectid = 0; >>> struct btrfs_root *gang[8]; >>> int i = 0; >>> - int err = 0; >>> - unsigned int ret = 0; >>> + int ret = 0; >>> + unsigned int ret2 = 0; >> >> I really hate the same @ret2. >> >> Since it's mostly the found number of radix tree gang lookup, can we >> change it to something like @found? >> >>> >>> while (1) { >> >> Another thing is, the @ret2 is only utilized inside the loop except the >> final cleanup. >> >> Can't we only declare @ret2 (or the new name) inside the loop and make >> the cleanup also happen inside the loop (or a dedicated helper?) >> > > Cleanup inside the loop is possible, but it would be something like > below, what do you think? One less @ret, one less goto tag. Only the btrfs_orphan_cleanup() call part needs some extra check, but still looks good to me. Thanks, Qu > > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index c2dc88f909b0..d1d23736de3c 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2926,22 +2926,23 @@ static int btrfs_cleanup_fs_roots(struct > btrfs_fs_info *fs_info) > { > u64 root_objectid = 0; > struct btrfs_root *gang[8]; > - int i = 0; > - int err = 0; > - unsigned int ret = 0; > + int ret = 0; > > while (1) { > + unsigned int i; > + unsigned int found; > + > spin_lock(&fs_info->fs_roots_radix_lock); > - ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix, > + found = radix_tree_gang_lookup(&fs_info->fs_roots_radix, > (void **)gang, root_objectid, > ARRAY_SIZE(gang)); > - if (!ret) { > + if (!found) { > spin_unlock(&fs_info->fs_roots_radix_lock); > break; > } > - root_objectid = btrfs_root_id(gang[ret - 1]) + 1; > + root_objectid = btrfs_root_id(gang[found - 1]) + 1; > > - for (i = 0; i < ret; i++) { > + for (i = 0; i < found; i++) { > /* Avoid to grab roots in dead_roots. */ > if (btrfs_root_refs(&gang[i]->root_item) == 0) { > gang[i] = NULL; > > @@ -2952,24 +2953,20 @@ static int btrfs_cleanup_fs_roots(struct > btrfs_fs_info *fs_info) > } > spin_unlock(&fs_info->fs_roots_radix_lock); > > - for (i = 0; i < ret; i++) { > + for (i = 0; i < found; i++) { > if (!gang[i]) > continue; > root_objectid = btrfs_root_id(gang[i]); > - err = btrfs_orphan_cleanup(gang[i]); > - if (err) > - goto out; > + if (!ret) > + ret = btrfs_orphan_cleanup(gang[i]); > btrfs_put_root(gang[i]); > } > + if (ret) > + break; > + > root_objectid++; > } > -out: > - /* Release the uncleaned roots due to error. */ > - for (; i < ret; i++) { > - if (gang[i]) > - btrfs_put_root(gang[i]); > - } > - return err; > + return ret; > } > > Thanks, > > Anand > > > >> Thanks, >> Qu >>> spin_lock(&fs_info->fs_roots_radix_lock); >>> - ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix, >>> + ret2 = radix_tree_gang_lookup(&fs_info->fs_roots_radix, >>> (void **)gang, root_objectid, >>> ARRAY_SIZE(gang)); >>> - if (!ret) { >>> + if (!ret2) { >>> spin_unlock(&fs_info->fs_roots_radix_lock); >>> break; >>> } >>> - root_objectid = gang[ret - 1]->root_key.objectid + 1; >>> + root_objectid = gang[ret2 - 1]->root_key.objectid + 1; >>> >>> - for (i = 0; i < ret; i++) { >>> + for (i = 0; i < ret2; i++) { >>> /* Avoid to grab roots in dead_roots. */ >>> if (btrfs_root_refs(&gang[i]->root_item) == 0) { >>> gang[i] = NULL; >>> @@ -2943,12 +2943,12 @@ static int btrfs_cleanup_fs_roots(struct >>> btrfs_fs_info *fs_info) >>> } >>> spin_unlock(&fs_info->fs_roots_radix_lock); >>> >>> - for (i = 0; i < ret; i++) { >>> + for (i = 0; i < ret2; i++) { >>> if (!gang[i]) >>> continue; >>> root_objectid = gang[i]->root_key.objectid; >>> - err = btrfs_orphan_cleanup(gang[i]); >>> - if (err) >>> + ret = btrfs_orphan_cleanup(gang[i]); >>> + if (ret) >>> goto out; >>> btrfs_put_root(gang[i]); >>> } >>> @@ -2956,11 +2956,11 @@ static int btrfs_cleanup_fs_roots(struct >>> btrfs_fs_info *fs_info) >>> } >>> out: >>> /* Release the uncleaned roots due to error. */ >>> - for (; i < ret; i++) { >>> + for (; i < ret2; i++) { >>> if (gang[i]) >>> btrfs_put_root(gang[i]); >>> } >>> - return err; >>> + return ret; >>> } >>> >>> /* > >
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 3df5477d48a8..d28de2cfb7b4 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2918,21 +2918,21 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info) u64 root_objectid = 0; struct btrfs_root *gang[8]; int i = 0; - int err = 0; - unsigned int ret = 0; + int ret = 0; + unsigned int ret2 = 0; while (1) { spin_lock(&fs_info->fs_roots_radix_lock); - ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix, + ret2 = radix_tree_gang_lookup(&fs_info->fs_roots_radix, (void **)gang, root_objectid, ARRAY_SIZE(gang)); - if (!ret) { + if (!ret2) { spin_unlock(&fs_info->fs_roots_radix_lock); break; } - root_objectid = gang[ret - 1]->root_key.objectid + 1; + root_objectid = gang[ret2 - 1]->root_key.objectid + 1; - for (i = 0; i < ret; i++) { + for (i = 0; i < ret2; i++) { /* Avoid to grab roots in dead_roots. */ if (btrfs_root_refs(&gang[i]->root_item) == 0) { gang[i] = NULL; @@ -2943,12 +2943,12 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info) } spin_unlock(&fs_info->fs_roots_radix_lock); - for (i = 0; i < ret; i++) { + for (i = 0; i < ret2; i++) { if (!gang[i]) continue; root_objectid = gang[i]->root_key.objectid; - err = btrfs_orphan_cleanup(gang[i]); - if (err) + ret = btrfs_orphan_cleanup(gang[i]); + if (ret) goto out; btrfs_put_root(gang[i]); } @@ -2956,11 +2956,11 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info) } out: /* Release the uncleaned roots due to error. */ - for (; i < ret; i++) { + for (; i < ret2; i++) { if (gang[i]) btrfs_put_root(gang[i]); } - return err; + return ret; } /*
Since err represents the function return value, rename it as ret, and rename the original ret, which serves as a helper return value, to ret2. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/disk-io.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)