Re: [PATCH v4 1/6] btrfs: Introduce a function to check if all chunks a OK for degraded rw mount

[David Sterba <dsterba@suse.cz>]

On Fri, Jul 14, 2017 at 04:19:07PM +0800, Qu Wenruo wrote:
> On 2017年07月14日 15:44, Nikolay Borisov wrote:
> > On 28.06.2017 08:43, Qu Wenruo wrote:
> >> Introduce a new function, btrfs_check_rw_degradable(), to check if all
> >> chunks in btrfs is OK for degraded rw mount.
> >>
> >> It provides the new basis for accurate btrfs mount/remount and even
> >> runtime degraded mount check other than old one-size-fit-all method.
> >>
> >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> >> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> >> ---
> >>   fs/btrfs/volumes.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   fs/btrfs/volumes.h |  1 +
> >>   2 files changed, 59 insertions(+)
> >>
> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >> index c95f018d4a1e..7a72fbdb8262 100644
> >> --- a/fs/btrfs/volumes.c
> >> +++ b/fs/btrfs/volumes.c
> >> @@ -6817,6 +6817,64 @@ int btrfs_read_sys_array(struct btrfs_fs_info *fs_info)
> >>   	return -EIO;
> >>   }
> >>   
> >> +/*
> >> + * Check if all chunks in the fs is OK for read-write degraded mount
> >> + *
> >> + * Return true if all chunks meet the minimal RW mount requirement.
> >> + * Return false if any chunk doesn't meet the minimal RW mount requirement.
> >> + */
> >> +bool btrfs_check_rw_degradable(struct btrfs_fs_info *fs_info)
> >> +{
> >> +	struct btrfs_mapping_tree *map_tree = &fs_info->mapping_tree;
> >> +	struct extent_map *em;
> >> +	u64 next_start = 0;
> >> +	bool ret = true;
> >> +
> >> +	read_lock(&map_tree->map_tree.lock);
> >> +	em = lookup_extent_mapping(&map_tree->map_tree, 0, (u64)-1);
> >> +	read_unlock(&map_tree->map_tree.lock);
> >> +	/* No chunk at all? Return false anyway */
> >> +	if (!em) {
> >> +		ret = false;
> >> +		goto out;
> >> +	}
> >> +	while (em) {
> >> +		struct map_lookup *map;
> >> +		int missing = 0;
> >> +		int max_tolerated;
> >> +		int i;
> >> +
> >> +		map = em->map_lookup;
> >> +		max_tolerated =
> >> +			btrfs_get_num_tolerated_disk_barrier_failures(
> >> +					map->type);
> >> +		for (i = 0; i < map->num_stripes; i++) {
> >> +			struct btrfs_device *dev = map->stripes[i].dev;
> >> +
> >> +			if (!dev || !dev->bdev || dev->missing ||
> >> +			    dev->last_flush_error)
> >> +				missing++;
> >> +		}
> >> +		if (missing > max_tolerated) {
> >> +			ret = false;
> >> +			btrfs_warn(fs_info,
> >> +	"chunk %llu missing %d devices, max tolerance is %d for writeble mount",
> >> +				   em->start, missing, max_tolerated);
> >> +			free_extent_map(em);
> >> +			goto out;
> >> +		}
> >> +		next_start = extent_map_end(em);
> >> +		free_extent_map(em);
> >> +
> >> +		read_lock(&map_tree->map_tree.lock);
> >> +		em = lookup_extent_mapping(&map_tree->map_tree, next_start,
> >> +					   (u64)(-1) - next_start);
> >> +		read_unlock(&map_tree->map_tree.lock);
> >> +	}
> >> +out:
> >> +	return ret;
> >> +}
> >> +
> > 
> > Nit but I think in this function it would be best to directly return
> > true/false based on context rather than having the superfluous goto.
> 
> Right, the goto out is not necessary as it's original design to handle 
> extent map unlock.
> But the final patch uses the current method to free extent map.
> 
> Indeed just returning true and false will be better, but goto out also 
> seems fine to me.

Yeah, it conforms to the pattern of single return point, though this
usually is best in functions with multiple jumps sources and some
non-trivial cleanup code.