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@xxxxxxxxxxxxxx>
> >> Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
> >> ---
> >> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
