| Message ID | 1399512409-661-1-git-send-email-wangsl.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 8/5/2014 4:26 ??, Wang Shilong wrote: > This patch adds an option '--check-data-csum' to verify data csums. > fsck won't check data csums unless users specify this option explictly. Can this option be added to btrfs restore as well? i think it would be a good thing if users can tell restore to only recover non-corrupt files. > > Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> > --- > Documentation/btrfs-check.txt | 2 + > cmds-check.c | 122 ++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 120 insertions(+), 4 deletions(-) > > diff --git a/Documentation/btrfs-check.txt b/Documentation/btrfs-check.txt > index 485a49c..bc10755 100644 > --- a/Documentation/btrfs-check.txt > +++ b/Documentation/btrfs-check.txt > @@ -30,6 +30,8 @@ try to repair the filesystem. > create a new CRC tree. > --init-extent-tree:: > create a new extent tree. > +--check-data-csum:: > +check data csums. > > EXIT STATUS > ----------- > diff --git a/cmds-check.c b/cmds-check.c > index 103efc5..b53d49c 100644 > --- a/cmds-check.c > +++ b/cmds-check.c > @@ -53,6 +53,7 @@ static LIST_HEAD(delete_items); > static int repair = 0; > static int no_holes = 0; > static int init_extent_tree = 0; > +static int check_data_csum = 0; > > struct extent_backref { > struct list_head list; > @@ -3634,6 +3635,106 @@ static int check_space_cache(struct btrfs_root *root) > return error ? -EINVAL : 0; > } > > +static int read_extent_data(struct btrfs_root *root, char *data, > + u64 logical, u64 len, int mirror) > +{ > + u64 offset = 0; > + struct btrfs_multi_bio *multi = NULL; > + struct btrfs_fs_info *info = root->fs_info; > + struct btrfs_device *device; > + int ret = 0; > + u64 read_len; > + unsigned long bytes_left = len; > + > + while (bytes_left) { > + read_len = bytes_left; > + device = NULL; > + ret = btrfs_map_block(&info->mapping_tree, READ, > + logical + offset, &read_len, &multi, > + mirror, NULL); > + if (ret) { > + fprintf(stderr, "Couldn't map the block %llu\n", > + logical + offset); > + goto error; > + } > + device = multi->stripes[0].dev; > + > + if (device->fd == 0) > + goto error; > + > + if (read_len > root->sectorsize) > + read_len = root->sectorsize; > + if (read_len > bytes_left) > + read_len = bytes_left; > + > + ret = pread64(device->fd, data + offset, read_len, > + multi->stripes[0].physical); > + if (ret != read_len) > + goto error; > + offset += read_len; > + bytes_left -= read_len; > + kfree(multi); > + multi = NULL; > + } > + return 0; > +error: > + kfree(multi); > + return -EIO; > +} > + > +static int check_extent_csums(struct btrfs_root *root, u64 bytenr, > + u64 num_bytes, unsigned long leaf_offset, > + struct extent_buffer *eb) { > + > + u64 offset = 0; > + u16 csum_size = btrfs_super_csum_size(root->fs_info->super_copy); > + char *data; > + u32 crc; > + unsigned long tmp; > + char result[csum_size]; > + char out[csum_size]; > + int ret = 0; > + __s64 cmp; > + int mirror; > + int num_copies = btrfs_num_copies(&root->fs_info->mapping_tree, > + bytenr, num_bytes); > + > + BUG_ON(num_bytes % root->sectorsize); > + data = malloc(root->sectorsize); > + if (!data) > + return -ENOMEM; > + > + while (offset < num_bytes) { > + mirror = 0; > +again: > + ret = read_extent_data(root, data, bytenr + offset, > + root->sectorsize, mirror); > + if (ret) > + goto out; > + > + crc = ~(u32)0; > + crc = btrfs_csum_data(NULL, (char *)data, crc, > + root->sectorsize); > + btrfs_csum_final(crc, result); > + > + tmp = leaf_offset + offset / root->sectorsize * csum_size; > + read_extent_buffer(eb, out, tmp, csum_size); > + cmp = memcmp(out, result, csum_size); > + if (cmp) { > + fprintf(stderr, "mirror: %d range bytenr: %llu, len: %d checksum mismatch\n", > + mirror, bytenr + offset, root->sectorsize); > + if (mirror < num_copies - 1) { > + mirror += 1; > + goto again; > + } > + } > + offset += root->sectorsize; > + } > +out: > + free(data); > + return ret; > +} > + > static int check_extent_exists(struct btrfs_root *root, u64 bytenr, > u64 num_bytes) > { > @@ -3771,6 +3872,8 @@ static int check_csums(struct btrfs_root *root) > u16 csum_size = btrfs_super_csum_size(root->fs_info->super_copy); > int errors = 0; > int ret; > + u64 data_len; > + unsigned long leaf_offset; > > root = root->fs_info->csum_root; > > @@ -3812,6 +3915,16 @@ static int check_csums(struct btrfs_root *root) > continue; > } > > + data_len = (btrfs_item_size_nr(leaf, path->slots[0]) / > + csum_size) * root->sectorsize; > + if (!check_data_csum) > + goto skip_csum_check; > + leaf_offset = btrfs_item_ptr_offset(leaf, path->slots[0]); > + ret = check_extent_csums(root, key.offset, data_len, > + leaf_offset, leaf); > + if (ret) > + break; > +skip_csum_check: > if (!num_bytes) { > offset = key.offset; > } else if (key.offset != offset + num_bytes) { > @@ -3825,9 +3938,7 @@ static int check_csums(struct btrfs_root *root) > offset = key.offset; > num_bytes = 0; > } > - > - num_bytes += (btrfs_item_size_nr(leaf, path->slots[0]) / > - csum_size) * root->sectorsize; > + num_bytes += data_len; > path->slots[0]++; > } > > @@ -6665,6 +6776,7 @@ static struct option long_options[] = { > { "repair", 0, NULL, 0 }, > { "init-csum-tree", 0, NULL, 0 }, > { "init-extent-tree", 0, NULL, 0 }, > + { "check-data-csum", 0, NULL, 0 }, > { "backup", 0, NULL, 0 }, > { NULL, 0, NULL, 0} > }; > @@ -6678,6 +6790,7 @@ const char * const cmd_check_usage[] = { > "--repair try to repair the filesystem", > "--init-csum-tree create a new CRC tree", > "--init-extent-tree create a new extent tree", > + "--check-data-csum check data csums", > NULL > }; > > @@ -6736,8 +6849,9 @@ int cmd_check(int argc, char **argv) > ctree_flags |= (OPEN_CTREE_WRITES | > OPEN_CTREE_NO_BLOCK_GROUPS); > repair = 1; > + } else if (option_index == 4) { > + check_data_csum = 1; > } > - > } > argc = argc - optind; > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 08, 2014 at 09:26:49AM +0800, Wang Shilong wrote: > This patch adds an option '--check-data-csum' to verify data csums. > fsck won't check data csums unless users specify this option explictly. Nice. > +static int read_extent_data(struct btrfs_root *root, char *data, > + u64 logical, u64 len, int mirror) > +{ > + if (read_len > root->sectorsize) > + read_len = root->sectorsize; > + if (read_len > bytes_left) > + read_len = bytes_left; > + > + ret = pread64(device->fd, data + offset, read_len, > + multi->stripes[0].physical); You can od kfree(multi) here and make the error/exit block simpler. kfree(multi); multi = NULL; > + if (ret != read_len) ret = -EIO; > + goto error; > + offset += read_len; > + bytes_left -= read_len; > + kfree(multi); > + multi = NULL; > + } > + return 0; return ret; Remove the rest. > +error: > + kfree(multi); > + return -EIO; > +} > + > +static int check_extent_csums(struct btrfs_root *root, u64 bytenr, > + u64 num_bytes, unsigned long leaf_offset, > + struct extent_buffer *eb) { > + > + u64 offset = 0; > + u16 csum_size = btrfs_super_csum_size(root->fs_info->super_copy); > + char *data; > + u32 crc; > + unsigned long tmp; > + char result[csum_size]; > + char out[csum_size]; A local variable of dynamic size, though we know it will not be larger than BTRFS_CSUM_SIZE. Please use that. csum_size should be validated during superblock read, so we can assume it's correct at this point. > + int ret = 0; > + __s64 cmp; This is used as a return value of memcmp, but both kernel and userspace libraries use simple int which is IMO enough. > + int mirror; > + int num_copies = btrfs_num_copies(&root->fs_info->mapping_tree, > + bytenr, num_bytes); > + > + BUG_ON(num_bytes % root->sectorsize); Can you do this check in the caller and avoid a BUG_ON here? > + data = malloc(root->sectorsize); > + if (!data) > + return -ENOMEM; > + > + while (offset < num_bytes) { > + mirror = 0; > +again: > + ret = read_extent_data(root, data, bytenr + offset, > + root->sectorsize, mirror); > + if (ret) > + goto out; > + > + crc = ~(u32)0; > + crc = btrfs_csum_data(NULL, (char *)data, crc, > + root->sectorsize); > + btrfs_csum_final(crc, result); > + > + tmp = leaf_offset + offset / root->sectorsize * csum_size; > + read_extent_buffer(eb, out, tmp, csum_size); > + cmp = memcmp(out, result, csum_size); > + if (cmp) { > + fprintf(stderr, "mirror: %d range bytenr: %llu, len: %d checksum mismatch\n", > + mirror, bytenr + offset, root->sectorsize); It has proved useful to print the checksums here, please enhance the output. > + if (mirror < num_copies - 1) { > + mirror += 1; mirror++; > + goto again; > + } > + } > + offset += root->sectorsize; > + } > +out: > + free(data); > + return ret; > +} > + > static int check_extent_exists(struct btrfs_root *root, u64 bytenr, > u64 num_bytes) > { > @@ -6678,6 +6790,7 @@ const char * const cmd_check_usage[] = { > "--repair try to repair the filesystem", > "--init-csum-tree create a new CRC tree", > "--init-extent-tree create a new extent tree", > + "--check-data-csum check data csums", The help text is not very useful, just repeats the option name without the dashes. Something like "verify checkums of data blocks" looks more readable to me. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 08, 2014 at 01:48:43PM +0300, Konstantinos Skarlatos wrote: > On 8/5/2014 4:26 ??, Wang Shilong wrote: > >This patch adds an option '--check-data-csum' to verify data csums. > >fsck won't check data csums unless users specify this option explictly. > Can this option be added to btrfs restore as well? i think it would be a > good thing if users can tell restore to only recover non-corrupt files. Good idea. From the UI side, I think about adding 3 option variants: * restore all (default) * restore only files with completely valid checksums * dtto invalid checksums The point is to efficiently retrieve files based on their validity, so without the 3rd option, one would have to "subtract" 2 from 1. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/16/2014 11:40 PM, David Sterba wrote: > On Thu, May 08, 2014 at 09:26:49AM +0800, Wang Shilong wrote: >> This patch adds an option '--check-data-csum' to verify data csums. >> fsck won't check data csums unless users specify this option explictly. > Nice. > >> +static int read_extent_data(struct btrfs_root *root, char *data, >> + u64 logical, u64 len, int mirror) >> +{ >> + if (read_len > root->sectorsize) >> + read_len = root->sectorsize; >> + if (read_len > bytes_left) >> + read_len = bytes_left; >> + >> + ret = pread64(device->fd, data + offset, read_len, >> + multi->stripes[0].physical); > You can od kfree(multi) here and make the error/exit block simpler. > > kfree(multi); > multi = NULL; > >> + if (ret != read_len) > ret = -EIO; > >> + goto error; >> + offset += read_len; >> + bytes_left -= read_len; >> + kfree(multi); >> + multi = NULL; >> + } >> + return 0; > return ret; > > Remove the rest. > >> +error: >> + kfree(multi); >> + return -EIO; >> +} >> + >> +static int check_extent_csums(struct btrfs_root *root, u64 bytenr, >> + u64 num_bytes, unsigned long leaf_offset, >> + struct extent_buffer *eb) { >> + >> + u64 offset = 0; >> + u16 csum_size = btrfs_super_csum_size(root->fs_info->super_copy); >> + char *data; >> + u32 crc; >> + unsigned long tmp; >> + char result[csum_size]; >> + char out[csum_size]; > A local variable of dynamic size, though we know it will not be larger > than BTRFS_CSUM_SIZE. Please use that. > > csum_size should be validated during superblock read, so we can assume > it's correct at this point. > >> + int ret = 0; >> + __s64 cmp; > This is used as a return value of memcmp, but both kernel and userspace > libraries use simple int which is IMO enough. > >> + int mirror; >> + int num_copies = btrfs_num_copies(&root->fs_info->mapping_tree, >> + bytenr, num_bytes); >> + >> + BUG_ON(num_bytes % root->sectorsize); > Can you do this check in the caller and avoid a BUG_ON here? > >> + data = malloc(root->sectorsize); >> + if (!data) >> + return -ENOMEM; >> + >> + while (offset < num_bytes) { >> + mirror = 0; >> +again: >> + ret = read_extent_data(root, data, bytenr + offset, >> + root->sectorsize, mirror); >> + if (ret) >> + goto out; >> + >> + crc = ~(u32)0; >> + crc = btrfs_csum_data(NULL, (char *)data, crc, >> + root->sectorsize); >> + btrfs_csum_final(crc, result); >> + >> + tmp = leaf_offset + offset / root->sectorsize * csum_size; >> + read_extent_buffer(eb, out, tmp, csum_size); >> + cmp = memcmp(out, result, csum_size); >> + if (cmp) { >> + fprintf(stderr, "mirror: %d range bytenr: %llu, len: %d checksum mismatch\n", >> + mirror, bytenr + offset, root->sectorsize); > It has proved useful to print the checksums here, please enhance the > output. > >> + if (mirror < num_copies - 1) { >> + mirror += 1; > mirror++; > >> + goto again; >> + } >> + } >> + offset += root->sectorsize; >> + } >> +out: >> + free(data); >> + return ret; >> +} >> + >> static int check_extent_exists(struct btrfs_root *root, u64 bytenr, >> u64 num_bytes) >> { >> @@ -6678,6 +6790,7 @@ const char * const cmd_check_usage[] = { >> "--repair try to repair the filesystem", >> "--init-csum-tree create a new CRC tree", >> "--init-extent-tree create a new extent tree", >> + "--check-data-csum check data csums", > The help text is not very useful, just repeats the option name without > the dashes. Something like "verify checkums of data blocks" looks more > readable to me. Sorry for late reply, i will address your comments, and send a v2 later. > . > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/btrfs-check.txt b/Documentation/btrfs-check.txt index 485a49c..bc10755 100644 --- a/Documentation/btrfs-check.txt +++ b/Documentation/btrfs-check.txt @@ -30,6 +30,8 @@ try to repair the filesystem. create a new CRC tree. --init-extent-tree:: create a new extent tree. +--check-data-csum:: +check data csums. EXIT STATUS ----------- diff --git a/cmds-check.c b/cmds-check.c index 103efc5..b53d49c 100644 --- a/cmds-check.c +++ b/cmds-check.c @@ -53,6 +53,7 @@ static LIST_HEAD(delete_items); static int repair = 0; static int no_holes = 0; static int init_extent_tree = 0; +static int check_data_csum = 0; struct extent_backref { struct list_head list; @@ -3634,6 +3635,106 @@ static int check_space_cache(struct btrfs_root *root) return error ? -EINVAL : 0; } +static int read_extent_data(struct btrfs_root *root, char *data, + u64 logical, u64 len, int mirror) +{ + u64 offset = 0; + struct btrfs_multi_bio *multi = NULL; + struct btrfs_fs_info *info = root->fs_info; + struct btrfs_device *device; + int ret = 0; + u64 read_len; + unsigned long bytes_left = len; + + while (bytes_left) { + read_len = bytes_left; + device = NULL; + ret = btrfs_map_block(&info->mapping_tree, READ, + logical + offset, &read_len, &multi, + mirror, NULL); + if (ret) { + fprintf(stderr, "Couldn't map the block %llu\n", + logical + offset); + goto error; + } + device = multi->stripes[0].dev; + + if (device->fd == 0) + goto error; + + if (read_len > root->sectorsize) + read_len = root->sectorsize; + if (read_len > bytes_left) + read_len = bytes_left; + + ret = pread64(device->fd, data + offset, read_len, + multi->stripes[0].physical); + if (ret != read_len) + goto error; + offset += read_len; + bytes_left -= read_len; + kfree(multi); + multi = NULL; + } + return 0; +error: + kfree(multi); + return -EIO; +} + +static int check_extent_csums(struct btrfs_root *root, u64 bytenr, + u64 num_bytes, unsigned long leaf_offset, + struct extent_buffer *eb) { + + u64 offset = 0; + u16 csum_size = btrfs_super_csum_size(root->fs_info->super_copy); + char *data; + u32 crc; + unsigned long tmp; + char result[csum_size]; + char out[csum_size]; + int ret = 0; + __s64 cmp; + int mirror; + int num_copies = btrfs_num_copies(&root->fs_info->mapping_tree, + bytenr, num_bytes); + + BUG_ON(num_bytes % root->sectorsize); + data = malloc(root->sectorsize); + if (!data) + return -ENOMEM; + + while (offset < num_bytes) { + mirror = 0; +again: + ret = read_extent_data(root, data, bytenr + offset, + root->sectorsize, mirror); + if (ret) + goto out; + + crc = ~(u32)0; + crc = btrfs_csum_data(NULL, (char *)data, crc, + root->sectorsize); + btrfs_csum_final(crc, result); + + tmp = leaf_offset + offset / root->sectorsize * csum_size; + read_extent_buffer(eb, out, tmp, csum_size); + cmp = memcmp(out, result, csum_size); + if (cmp) { + fprintf(stderr, "mirror: %d range bytenr: %llu, len: %d checksum mismatch\n", + mirror, bytenr + offset, root->sectorsize); + if (mirror < num_copies - 1) { + mirror += 1; + goto again; + } + } + offset += root->sectorsize; + } +out: + free(data); + return ret; +} + static int check_extent_exists(struct btrfs_root *root, u64 bytenr, u64 num_bytes) { @@ -3771,6 +3872,8 @@ static int check_csums(struct btrfs_root *root) u16 csum_size = btrfs_super_csum_size(root->fs_info->super_copy); int errors = 0; int ret; + u64 data_len; + unsigned long leaf_offset; root = root->fs_info->csum_root; @@ -3812,6 +3915,16 @@ static int check_csums(struct btrfs_root *root) continue; } + data_len = (btrfs_item_size_nr(leaf, path->slots[0]) / + csum_size) * root->sectorsize; + if (!check_data_csum) + goto skip_csum_check; + leaf_offset = btrfs_item_ptr_offset(leaf, path->slots[0]); + ret = check_extent_csums(root, key.offset, data_len, + leaf_offset, leaf); + if (ret) + break; +skip_csum_check: if (!num_bytes) { offset = key.offset; } else if (key.offset != offset + num_bytes) { @@ -3825,9 +3938,7 @@ static int check_csums(struct btrfs_root *root) offset = key.offset; num_bytes = 0; } - - num_bytes += (btrfs_item_size_nr(leaf, path->slots[0]) / - csum_size) * root->sectorsize; + num_bytes += data_len; path->slots[0]++; } @@ -6665,6 +6776,7 @@ static struct option long_options[] = { { "repair", 0, NULL, 0 }, { "init-csum-tree", 0, NULL, 0 }, { "init-extent-tree", 0, NULL, 0 }, + { "check-data-csum", 0, NULL, 0 }, { "backup", 0, NULL, 0 }, { NULL, 0, NULL, 0} }; @@ -6678,6 +6790,7 @@ const char * const cmd_check_usage[] = { "--repair try to repair the filesystem", "--init-csum-tree create a new CRC tree", "--init-extent-tree create a new extent tree", + "--check-data-csum check data csums", NULL }; @@ -6736,8 +6849,9 @@ int cmd_check(int argc, char **argv) ctree_flags |= (OPEN_CTREE_WRITES | OPEN_CTREE_NO_BLOCK_GROUPS); repair = 1; + } else if (option_index == 4) { + check_data_csum = 1; } - } argc = argc - optind;
This patch adds an option '--check-data-csum' to verify data csums. fsck won't check data csums unless users specify this option explictly. Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> --- Documentation/btrfs-check.txt | 2 + cmds-check.c | 122 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 120 insertions(+), 4 deletions(-)