Re: [PATCH] Btrfs: use self-explaining variable

[David Sterba <dsterba@suse.cz>]

On Sat, Sep 23, 2017 at 09:09:24AM +0800, Qu Wenruo wrote:
> 
> 
> On 2017年09月23日 08:48, Liu Bo wrote:
> > On Sat, Sep 23, 2017 at 08:46:55AM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2017年09月23日 07:36, Liu Bo wrote:
> >>> This uses a bool 'do_backup' to help understand this piece of code.
> >>>
> >>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> >>> ---
> >>> This is based on a patch "Btrfs: do not backup tree roots when fsync".
> >>>
> >>>    fs/btrfs/disk-io.c | 3 ++-
> >>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >>> index cdb7043..9811b9d 100644
> >>> --- a/fs/btrfs/disk-io.c
> >>> +++ b/fs/btrfs/disk-io.c
> >>> @@ -3691,6 +3691,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
> >>>    	int max_errors;
> >>>    	int total_errors = 0;
> >>>    	u64 flags;
> >>> +	bool do_backup = (max_mirrors == 0);
> >>
> >> Why not replacing @max_mirrors with @do_backup as parameter?
> > 
> > If I read the code correctly, max_mirrors is not just for deciding
> > backup.
> 
> That's strange.
> 
> write_all_supers() only uses @max_mirrors by passing it to 
> write_dev_supers() and wait_dev_supers().
> 
> Both of the write/wait_dev_supers() will replace @max_mirrors to 
> BTRFS_SUPER_MIRROR_MAX if it's zero.
> 
> Further more, all write_all_supers() callers just pass @max_mirrors as 
> bool (either 0 or 1).
> 
> So I don't see any point not replacing the parameter as bool.

Agreed, the idea was to replace the parameter by the bool.