A few minor comments below
On Mon, May 02, 2016 at 11:15:51AM -0700, Liu Bo wrote:
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6206,27 +6206,23 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
> return dev;
> }
>
> -static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
> - struct extent_buffer *leaf,
> - struct btrfs_chunk *chunk)
> +/* Return -EIO if any error, otherwise return 0. */
> +static int btrfs_check_chunk_valid(struct btrfs_root *root,
> + struct extent_buffer *leaf,
> + struct btrfs_chunk *chunk, u64 logical)
> {
> - struct btrfs_mapping_tree *map_tree = &root->fs_info->mapping_tree;
> - struct map_lookup *map;
> - struct extent_map *em;
> - u64 logical;
> u64 length;
> u64 stripe_len;
> - u64 devid;
> - u8 uuid[BTRFS_UUID_SIZE];
> - int num_stripes;
> - int ret;
> - int i;
> + u16 num_stripes;
> + u16 sub_stripes;
> + u64 type;
>
> - logical = key->offset;
> length = btrfs_chunk_length(leaf, chunk);
> stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
> num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
> - /* Validation check */
> + sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
> + type = btrfs_chunk_type(leaf, chunk);
> +
> if (!num_stripes) {
> btrfs_err(root->fs_info, "invalid chunk num_stripes: %u",
> num_stripes);
> @@ -6237,24 +6233,70 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
> "invalid chunk logical %llu", logical);
> return -EIO;
> }
> + if (btrfs_chunk_sector_size(leaf, chunk) != root->sectorsize) {
> + btrfs_err(root->fs_info, "invalid chunk sectorsize %llu",
> + (unsigned long long)btrfs_chunk_sector_size(leaf,
type cast not necessry
> + chunk));
> + return -EIO;
> + }
> if (!length || !IS_ALIGNED(length, root->sectorsize)) {
> btrfs_err(root->fs_info,
> "invalid chunk length %llu", length);
> return -EIO;
> }
> - if (!is_power_of_2(stripe_len)) {
> + if (stripe_len != BTRFS_STRIPE_LEN) {
Again too strict. As mentined elsewhere, add a helper to validate
stripe_len and use it so we don't open-code it.
> btrfs_err(root->fs_info, "invalid chunk stripe length: %llu",
> stripe_len);
> return -EIO;
> }
> if (~(BTRFS_BLOCK_GROUP_TYPE_MASK | BTRFS_BLOCK_GROUP_PROFILE_MASK) &
> - btrfs_chunk_type(leaf, chunk)) {
> + type) {
> btrfs_err(root->fs_info, "unrecognized chunk type: %llu",
> ~(BTRFS_BLOCK_GROUP_TYPE_MASK |
> BTRFS_BLOCK_GROUP_PROFILE_MASK) &
> btrfs_chunk_type(leaf, chunk));
> return -EIO;
> }
> + if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes == 0) ||
> + (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
> + (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
> + (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 3) ||
> + (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) ||
> + ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&
I was looking if we could turn that into some generic checks using the
btrfs_raid_array but seems that the tests do not make a uniform pattern,
eg the DUP and SINGLE disguised as "mask == 0". As we don't add new
profiles too often I'm ok with that version.
> + num_stripes != 1)) {
> + btrfs_err(root->fs_info, "Invalid num_stripes:sub_stripes %u:%u for profile %llu",
"invalid..." (no initial capital letter) and put the string on the next
line so it does not exceed 80 cols
> + num_stripes, sub_stripes,
> + type & BTRFS_BLOCK_GROUP_PROFILE_MASK);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
> + struct extent_buffer *leaf,
> + struct btrfs_chunk *chunk)
> +{
> + struct btrfs_mapping_tree *map_tree = &root->fs_info->mapping_tree;
> + struct map_lookup *map;
> + struct extent_map *em;
> + u64 logical;
> + u64 length;
> + u64 stripe_len;
> + u64 devid;
> + u8 uuid[BTRFS_UUID_SIZE];
> + int num_stripes;
> + int ret;
> + int i;
> +
> + logical = key->offset;
> + length = btrfs_chunk_length(leaf, chunk);
> + stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
> + num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
> + /* Validation check */
Redundant comment (from the time when the validation was not in a
wrapper)
> + ret = btrfs_check_chunk_valid(root, leaf, chunk, logical);
> + if (ret)
> + return ret;
>
> read_lock(&map_tree->map_tree.lock);
> em = lookup_extent_mapping(&map_tree->map_tree, logical, 1);
> @@ -6502,6 +6544,7 @@ int btrfs_read_sys_array(struct btrfs_root *root)
> u32 array_size;
> u32 len = 0;
> u32 cur_offset;
> + u64 type;
> struct btrfs_key key;
>
> ASSERT(BTRFS_SUPER_INFO_SIZE <= root->nodesize);
> @@ -6568,6 +6611,15 @@ int btrfs_read_sys_array(struct btrfs_root *root)
> break;
> }
>
> + type = btrfs_chunk_type(sb, chunk);
> + if ((type & BTRFS_BLOCK_GROUP_SYSTEM) == 0) {
> + printk(KERN_ERR
> + "BTRFS: invalid chunk type %llu in sys_array at offset %u\n",
> + type, cur_offset);
> + ret = -EIO;
> + break;
> + }
> +
> len = btrfs_chunk_item_size(num_stripes);
> if (cur_offset + len > array_size)
> goto out_short_read;
> --
> 2.5.5
>
> --
> 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
--
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
