* [PATCH 0/3] Preparatory to add the csum check in the scan context
@ 2018-03-20 9:53 Anand Jain
2018-03-20 9:53 ` [PATCH 1/3] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type Anand Jain
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Anand Jain @ 2018-03-20 9:53 UTC (permalink / raw)
To: linux-btrfs
Anand Jain (3):
btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the
type
btrfs: return required error from btrfs_check_super_csum
btrfs: refactor btrfs_check_super_csum() for scan
fs/btrfs/disk-io.c | 50 ++++++++++++++++++++++++--------------------------
fs/btrfs/disk-io.h | 1 +
2 files changed, 25 insertions(+), 26 deletions(-)
--
2.15.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type
2018-03-20 9:53 [PATCH 0/3] Preparatory to add the csum check in the scan context Anand Jain
@ 2018-03-20 9:53 ` Anand Jain
2018-03-20 10:11 ` Nikolay Borisov
2018-03-20 9:53 ` [PATCH 2/3] btrfs: return required error from btrfs_check_super_csum Anand Jain
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Anand Jain @ 2018-03-20 9:53 UTC (permalink / raw)
To: linux-btrfs
%csum_type is being checked to check the number of csum types we support,
which is 1 as of now. Instead just check if the type matches with the
only type we support, that is BTRFS_CSUM_TYPE_CRC32. And further adds
cleanup.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/disk-io.c | 41 +++++++++++++++++++----------------------
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 1657d6aa4fa6..582ed6af3c50 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -399,32 +399,29 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
struct btrfs_super_block *disk_sb =
(struct btrfs_super_block *)raw_disk_sb;
u16 csum_type = btrfs_super_csum_type(disk_sb);
- int ret = 0;
-
- if (csum_type == BTRFS_CSUM_TYPE_CRC32) {
- u32 crc = ~(u32)0;
- char result[sizeof(crc)];
-
- /*
- * The super_block structure does not span the whole
- * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space
- * is filled with zeros and is included in the checksum.
- */
- crc = btrfs_csum_data(raw_disk_sb + BTRFS_CSUM_SIZE,
- crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
- btrfs_csum_final(crc, result);
-
- if (memcmp(raw_disk_sb, result, sizeof(result)))
- ret = 1;
- }
+ u32 crc = ~(u32)0;
+ char result[sizeof(crc)];
- if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) {
+ /* We support csum type crc32 only as of now */
+ if (csum_type != BTRFS_CSUM_TYPE_CRC32) {
btrfs_err(fs_info, "unsupported checksum algorithm %u",
- csum_type);
- ret = 1;
+ csum_type);
+ return 1;
}
- return ret;
+ /*
+ * The super_block structure does not span the whole
+ * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space
+ * is filled with zeros and is included in the checksum.
+ */
+ crc = btrfs_csum_data(raw_disk_sb + BTRFS_CSUM_SIZE,
+ crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
+ btrfs_csum_final(crc, result);
+
+ if (memcmp(raw_disk_sb, result, sizeof(result)))
+ return 1;
+
+ return 0;
}
/*
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] btrfs: return required error from btrfs_check_super_csum
2018-03-20 9:53 [PATCH 0/3] Preparatory to add the csum check in the scan context Anand Jain
2018-03-20 9:53 ` [PATCH 1/3] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type Anand Jain
@ 2018-03-20 9:53 ` Anand Jain
2018-03-20 10:13 ` Nikolay Borisov
2018-03-20 9:53 ` [PATCH 3/3] btrfs: refactor btrfs_check_super_csum() for scan Anand Jain
2018-03-21 8:14 ` [PATCH v3 0/2] Preparatory to add the csum check in the scan context Anand Jain
3 siblings, 1 reply; 16+ messages in thread
From: Anand Jain @ 2018-03-20 9:53 UTC (permalink / raw)
To: linux-btrfs
Return the required -EINVAL from the function btrfs_check_super_csum()
and move the error logging into the btrfs_check_super_csum() itself.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/disk-io.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 582ed6af3c50..aafd836af61d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -406,7 +406,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
if (csum_type != BTRFS_CSUM_TYPE_CRC32) {
btrfs_err(fs_info, "unsupported checksum algorithm %u",
csum_type);
- return 1;
+ return -EINVAL;
}
/*
@@ -418,8 +418,10 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
btrfs_csum_final(crc, result);
- if (memcmp(raw_disk_sb, result, sizeof(result)))
- return 1;
+ if (memcmp(raw_disk_sb, result, sizeof(result))) {
+ btrfs_err(fs_info, "superblock checksum mismatch");
+ return -EINVAL;
+ }
return 0;
}
@@ -2571,9 +2573,8 @@ int open_ctree(struct super_block *sb,
* We want to check superblock checksum, the type is stored inside.
* Pass the whole disk block of size BTRFS_SUPER_INFO_SIZE (4k).
*/
- if (btrfs_check_super_csum(fs_info, bh->b_data)) {
- btrfs_err(fs_info, "superblock checksum mismatch");
- err = -EINVAL;
+ err = btrfs_check_super_csum(fs_info, bh->b_data);
+ if (err) {
brelse(bh);
goto fail_alloc;
}
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] btrfs: refactor btrfs_check_super_csum() for scan
2018-03-20 9:53 [PATCH 0/3] Preparatory to add the csum check in the scan context Anand Jain
2018-03-20 9:53 ` [PATCH 1/3] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type Anand Jain
2018-03-20 9:53 ` [PATCH 2/3] btrfs: return required error from btrfs_check_super_csum Anand Jain
@ 2018-03-20 9:53 ` Anand Jain
2018-03-20 11:10 ` Nikolay Borisov
2018-03-21 8:14 ` [PATCH v3 0/2] Preparatory to add the csum check in the scan context Anand Jain
3 siblings, 1 reply; 16+ messages in thread
From: Anand Jain @ 2018-03-20 9:53 UTC (permalink / raw)
To: linux-btrfs
In preparation to use the function btrfs_check_super_csum() for
the device scan context, make it nonstatic and pass the
struct block_device instead of the struct fs_info.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/disk-io.c | 12 ++++++------
fs/btrfs/disk-io.h | 1 +
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index aafd836af61d..d2ace2dca9de 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -393,8 +393,7 @@ static int verify_parent_transid(struct extent_io_tree *io_tree,
* Return 0 if the superblock checksum type matches the checksum value of that
* algorithm. Pass the raw disk superblock data.
*/
-static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
- char *raw_disk_sb)
+int btrfs_check_super_csum(struct block_device *bdev, char *raw_disk_sb)
{
struct btrfs_super_block *disk_sb =
(struct btrfs_super_block *)raw_disk_sb;
@@ -404,8 +403,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
/* We support csum type crc32 only as of now */
if (csum_type != BTRFS_CSUM_TYPE_CRC32) {
- btrfs_err(fs_info, "unsupported checksum algorithm %u",
- csum_type);
+ pr_err("BTRFS error (device %pg): unsupported checksum algorithm %u",
+ bdev, csum_type);
return -EINVAL;
}
@@ -419,7 +418,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
btrfs_csum_final(crc, result);
if (memcmp(raw_disk_sb, result, sizeof(result))) {
- btrfs_err(fs_info, "superblock checksum mismatch");
+ pr_err("BTRFS error (device %pg): superblock checksum mismatch",
+ bdev);
return -EINVAL;
}
@@ -2573,7 +2573,7 @@ int open_ctree(struct super_block *sb,
* We want to check superblock checksum, the type is stored inside.
* Pass the whole disk block of size BTRFS_SUPER_INFO_SIZE (4k).
*/
- err = btrfs_check_super_csum(fs_info, bh->b_data);
+ err = btrfs_check_super_csum(fs_devices->latest_bdev, bh->b_data);
if (err) {
brelse(bh);
goto fail_alloc;
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 70a88d61b547..1427fa1181d4 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -69,6 +69,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors);
struct buffer_head *btrfs_read_dev_super(struct block_device *bdev);
int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
struct buffer_head **bh_ret);
+int btrfs_check_super_csum(struct block_device *bdev, char *raw_disk_sb);
int btrfs_commit_super(struct btrfs_fs_info *fs_info);
struct btrfs_root *btrfs_read_fs_root(struct btrfs_root *tree_root,
struct btrfs_key *location);
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type
2018-03-20 9:53 ` [PATCH 1/3] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type Anand Jain
@ 2018-03-20 10:11 ` Nikolay Borisov
2018-03-20 10:12 ` Nikolay Borisov
0 siblings, 1 reply; 16+ messages in thread
From: Nikolay Borisov @ 2018-03-20 10:11 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 20.03.2018 11:53, Anand Jain wrote:
> %csum_type is being checked to check the number of csum types we support,
> which is 1 as of now. Instead just check if the type matches with the
> only type we support, that is BTRFS_CSUM_TYPE_CRC32. And further adds
> cleanup.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> fs/btrfs/disk-io.c | 41 +++++++++++++++++++----------------------
> 1 file changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 1657d6aa4fa6..582ed6af3c50 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -399,32 +399,29 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
> struct btrfs_super_block *disk_sb =
> (struct btrfs_super_block *)raw_disk_sb;
> u16 csum_type = btrfs_super_csum_type(disk_sb);
> - int ret = 0;
> -
> - if (csum_type == BTRFS_CSUM_TYPE_CRC32) {
> - u32 crc = ~(u32)0;
> - char result[sizeof(crc)];
> -
> - /*
> - * The super_block structure does not span the whole
> - * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space
> - * is filled with zeros and is included in the checksum.
> - */
> - crc = btrfs_csum_data(raw_disk_sb + BTRFS_CSUM_SIZE,
> - crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
> - btrfs_csum_final(crc, result);
> -
> - if (memcmp(raw_disk_sb, result, sizeof(result)))
> - ret = 1;
> - }
> + u32 crc = ~(u32)0;
> + char result[sizeof(crc)];
>
> - if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) {
> + /* We support csum type crc32 only as of now */
> + if (csum_type != BTRFS_CSUM_TYPE_CRC32) {
> btrfs_err(fs_info, "unsupported checksum algorithm %u",
> - csum_type);
> - ret = 1;
> + csum_type);
> + return 1;
I think it would make sense to return EINVAL here. Then the sole caller
of this function (open_ctree) can use the code a bit more idiomatically:
ret = btrfs_check_super_csum()
if (ret < 0) {
handle failure
}
Right now we return 1 on both when the function fails due to invalid CRC
algorithm and if the checksum doesn't match. And we are going to print 2
things: unsupported checksum algorithm and the
btrfs_err(fs_info, "superblock checksum mismatch"); from open_ctree.
Let's implemented proper error returning strategy for this function.
> }
>
> - return ret;
> + /*
> + * The super_block structure does not span the whole
> + * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space
> + * is filled with zeros and is included in the checksum.
> + */
> + crc = btrfs_csum_data(raw_disk_sb + BTRFS_CSUM_SIZE,
> + crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
> + btrfs_csum_final(crc, result);
> +
> + if (memcmp(raw_disk_sb, result, sizeof(result)))
> + return 1;
Perhaps return EUCLEAN i.e something is corrupted hence the checksum
didn't pan out as we expected.
> +
> + return 0;
> }
>
> /*
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type
2018-03-20 10:11 ` Nikolay Borisov
@ 2018-03-20 10:12 ` Nikolay Borisov
0 siblings, 0 replies; 16+ messages in thread
From: Nikolay Borisov @ 2018-03-20 10:12 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 20.03.2018 12:11, Nikolay Borisov wrote:
>
>
> On 20.03.2018 11:53, Anand Jain wrote:
>> %csum_type is being checked to check the number of csum types we support,
>> which is 1 as of now. Instead just check if the type matches with the
>> only type we support, that is BTRFS_CSUM_TYPE_CRC32. And further adds
>> cleanup.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> fs/btrfs/disk-io.c | 41 +++++++++++++++++++----------------------
>> 1 file changed, 19 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 1657d6aa4fa6..582ed6af3c50 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -399,32 +399,29 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>> struct btrfs_super_block *disk_sb =
>> (struct btrfs_super_block *)raw_disk_sb;
>> u16 csum_type = btrfs_super_csum_type(disk_sb);
>> - int ret = 0;
>> -
>> - if (csum_type == BTRFS_CSUM_TYPE_CRC32) {
>> - u32 crc = ~(u32)0;
>> - char result[sizeof(crc)];
>> -
>> - /*
>> - * The super_block structure does not span the whole
>> - * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space
>> - * is filled with zeros and is included in the checksum.
>> - */
>> - crc = btrfs_csum_data(raw_disk_sb + BTRFS_CSUM_SIZE,
>> - crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
>> - btrfs_csum_final(crc, result);
>> -
>> - if (memcmp(raw_disk_sb, result, sizeof(result)))
>> - ret = 1;
>> - }
>> + u32 crc = ~(u32)0;
>> + char result[sizeof(crc)];
>>
>> - if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) {
>> + /* We support csum type crc32 only as of now */
>> + if (csum_type != BTRFS_CSUM_TYPE_CRC32) {
>> btrfs_err(fs_info, "unsupported checksum algorithm %u",
>> - csum_type);
>> - ret = 1;
>> + csum_type);
>> + return 1;
>
> I think it would make sense to return EINVAL here. Then the sole caller
> of this function (open_ctree) can use the code a bit more idiomatically:
>
> ret = btrfs_check_super_csum()
>
> if (ret < 0) {
> handle failure
> }
>
> Right now we return 1 on both when the function fails due to invalid CRC
> algorithm and if the checksum doesn't match. And we are going to print 2
> things: unsupported checksum algorithm and the
> btrfs_err(fs_info, "superblock checksum mismatch"); from open_ctree.
>
> Let's implemented proper error returning strategy for this function.
>
Disregard, you have already implemented something like that in 2/3. I
will comment there how it can be improved.
>
>
>> }
>>
>> - return ret;
>> + /*
>> + * The super_block structure does not span the whole
>> + * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space
>> + * is filled with zeros and is included in the checksum.
>> + */
>> + crc = btrfs_csum_data(raw_disk_sb + BTRFS_CSUM_SIZE,
>> + crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
>> + btrfs_csum_final(crc, result);
>> +
>> + if (memcmp(raw_disk_sb, result, sizeof(result)))
>> + return 1;
>
> Perhaps return EUCLEAN i.e something is corrupted hence the checksum
> didn't pan out as we expected.
>
>> +
>> + return 0;
>> }
>>
>> /*
>>
> --
> 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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] btrfs: return required error from btrfs_check_super_csum
2018-03-20 9:53 ` [PATCH 2/3] btrfs: return required error from btrfs_check_super_csum Anand Jain
@ 2018-03-20 10:13 ` Nikolay Borisov
2018-03-20 22:23 ` Anand Jain
2018-03-21 2:37 ` Anand Jain
0 siblings, 2 replies; 16+ messages in thread
From: Nikolay Borisov @ 2018-03-20 10:13 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 20.03.2018 11:53, Anand Jain wrote:
> Return the required -EINVAL from the function btrfs_check_super_csum()
> and move the error logging into the btrfs_check_super_csum() itself.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> fs/btrfs/disk-io.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 582ed6af3c50..aafd836af61d 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -406,7 +406,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
> if (csum_type != BTRFS_CSUM_TYPE_CRC32) {
> btrfs_err(fs_info, "unsupported checksum algorithm %u",
> csum_type);
> - return 1;
> + return -EINVAL;
> }
>
> /*
> @@ -418,8 +418,10 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
> crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
> btrfs_csum_final(crc, result);
>
> - if (memcmp(raw_disk_sb, result, sizeof(result)))
> - return 1;
> + if (memcmp(raw_disk_sb, result, sizeof(result))) {
> + btrfs_err(fs_info, "superblock checksum mismatch");
> + return -EINVAL;
Don't we want EUCLEAN here, since an error in the checksum indicates
corruption?
> + }
>
> return 0;
> }
> @@ -2571,9 +2573,8 @@ int open_ctree(struct super_block *sb,
> * We want to check superblock checksum, the type is stored inside.
> * Pass the whole disk block of size BTRFS_SUPER_INFO_SIZE (4k).
> */
> - if (btrfs_check_super_csum(fs_info, bh->b_data)) {
> - btrfs_err(fs_info, "superblock checksum mismatch");
> - err = -EINVAL;
> + err = btrfs_check_super_csum(fs_info, bh->b_data);
> + if (err) {
> brelse(bh);
> goto fail_alloc;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] btrfs: refactor btrfs_check_super_csum() for scan
2018-03-20 9:53 ` [PATCH 3/3] btrfs: refactor btrfs_check_super_csum() for scan Anand Jain
@ 2018-03-20 11:10 ` Nikolay Borisov
2018-03-20 22:32 ` Anand Jain
0 siblings, 1 reply; 16+ messages in thread
From: Nikolay Borisov @ 2018-03-20 11:10 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 20.03.2018 11:53, Anand Jain wrote:
> In preparation to use the function btrfs_check_super_csum() for
> the device scan context, make it nonstatic and pass the
> struct block_device instead of the struct fs_info.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> fs/btrfs/disk-io.c | 12 ++++++------
> fs/btrfs/disk-io.h | 1 +
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index aafd836af61d..d2ace2dca9de 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -393,8 +393,7 @@ static int verify_parent_transid(struct extent_io_tree *io_tree,
> * Return 0 if the superblock checksum type matches the checksum value of that
> * algorithm. Pass the raw disk superblock data.
> */
> -static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
> - char *raw_disk_sb)
> +int btrfs_check_super_csum(struct block_device *bdev, char *raw_disk_sb)
> {
Since this has become a public function and you've changed the fs_info
parameter to taking a bdev, which is not used for anything else than
printing the error I think it's appropriate to document which block
device should be passed to this function.
Also passing the block device only for printing purposes seems a bit odd.
> struct btrfs_super_block *disk_sb =
> (struct btrfs_super_block *)raw_disk_sb;
> @@ -404,8 +403,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>
> /* We support csum type crc32 only as of now */
> if (csum_type != BTRFS_CSUM_TYPE_CRC32) {
> - btrfs_err(fs_info, "unsupported checksum algorithm %u",
> - csum_type);
> + pr_err("BTRFS error (device %pg): unsupported checksum algorithm %u",
> + bdev, csum_type);
> return -EINVAL;
> }
>
> @@ -419,7 +418,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
> btrfs_csum_final(crc, result);
>
> if (memcmp(raw_disk_sb, result, sizeof(result))) {
> - btrfs_err(fs_info, "superblock checksum mismatch");
> + pr_err("BTRFS error (device %pg): superblock checksum mismatch",
> + bdev);
> return -EINVAL;
> }
>
> @@ -2573,7 +2573,7 @@ int open_ctree(struct super_block *sb,
> * We want to check superblock checksum, the type is stored inside.
> * Pass the whole disk block of size BTRFS_SUPER_INFO_SIZE (4k).
> */
> - err = btrfs_check_super_csum(fs_info, bh->b_data);
> + err = btrfs_check_super_csum(fs_devices->latest_bdev, bh->b_data);
> if (err) {
> brelse(bh);
> goto fail_alloc;
> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
> index 70a88d61b547..1427fa1181d4 100644
> --- a/fs/btrfs/disk-io.h
> +++ b/fs/btrfs/disk-io.h
> @@ -69,6 +69,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors);
> struct buffer_head *btrfs_read_dev_super(struct block_device *bdev);
> int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
> struct buffer_head **bh_ret);
> +int btrfs_check_super_csum(struct block_device *bdev, char *raw_disk_sb);
> int btrfs_commit_super(struct btrfs_fs_info *fs_info);
> struct btrfs_root *btrfs_read_fs_root(struct btrfs_root *tree_root,
> struct btrfs_key *location);
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] btrfs: return required error from btrfs_check_super_csum
2018-03-20 10:13 ` Nikolay Borisov
@ 2018-03-20 22:23 ` Anand Jain
2018-03-21 2:37 ` Anand Jain
1 sibling, 0 replies; 16+ messages in thread
From: Anand Jain @ 2018-03-20 22:23 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
>> @@ -418,8 +418,10 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>> crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
>> btrfs_csum_final(crc, result);
>>
>> - if (memcmp(raw_disk_sb, result, sizeof(result)))
>> - return 1;
>> + if (memcmp(raw_disk_sb, result, sizeof(result))) {
>> + btrfs_err(fs_info, "superblock checksum mismatch");
>> + return -EINVAL;
>
> Don't we want EUCLEAN here, since an error in the checksum indicates
> corruption?
yeah, I struggled with it before.
Strangely there isn't an error code for the csum error
when csum being widely used in the computer systems.
Our closest are:
#define EFAULT 14 /* Bad address */
#define EUCLEAN 135 /* Structure needs cleaning */
Will use the suggested EUCLEAN.
Thanks, Anand
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] btrfs: refactor btrfs_check_super_csum() for scan
2018-03-20 11:10 ` Nikolay Borisov
@ 2018-03-20 22:32 ` Anand Jain
0 siblings, 0 replies; 16+ messages in thread
From: Anand Jain @ 2018-03-20 22:32 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 03/20/2018 07:10 PM, Nikolay Borisov wrote:
>
>
> On 20.03.2018 11:53, Anand Jain wrote:
>> In preparation to use the function btrfs_check_super_csum() for
>> the device scan context, make it nonstatic and pass the
>> struct block_device instead of the struct fs_info.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> fs/btrfs/disk-io.c | 12 ++++++------
>> fs/btrfs/disk-io.h | 1 +
>> 2 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index aafd836af61d..d2ace2dca9de 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -393,8 +393,7 @@ static int verify_parent_transid(struct extent_io_tree *io_tree,
>> * Return 0 if the superblock checksum type matches the checksum value of that
>> * algorithm. Pass the raw disk superblock data.
>> */
>> -static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>> - char *raw_disk_sb)
>> +int btrfs_check_super_csum(struct block_device *bdev, char *raw_disk_sb)
>> {
>
> Since this has become a public function and you've changed the fs_info
> parameter to taking a bdev, which is not used for anything else than
> printing the error I think it's appropriate to document which block
> device should be passed to this function.
>
> Also passing the block device only for printing purposes seems a bit odd.
Its the device on which we have read the superblock. I find it
odd too. Will do that pr_err on the parent function. Will add
comments to the public function.
Thanks, Anand
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] btrfs: return required error from btrfs_check_super_csum
2018-03-20 10:13 ` Nikolay Borisov
2018-03-20 22:23 ` Anand Jain
@ 2018-03-21 2:37 ` Anand Jain
2018-03-21 3:01 ` Anand Jain
1 sibling, 1 reply; 16+ messages in thread
From: Anand Jain @ 2018-03-21 2:37 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
>> @@ -418,8 +418,10 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>> crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
>> btrfs_csum_final(crc, result);
>>
>> - if (memcmp(raw_disk_sb, result, sizeof(result)))
>> - return 1;
>> + if (memcmp(raw_disk_sb, result, sizeof(result))) {
>> + btrfs_err(fs_info, "superblock checksum mismatch");
>> + return -EINVAL;
>
> Don't we want EUCLEAN here, since an error in the checksum indicates
> corruption?
With the v2 patch sentout here is the error str that is seen on the cli.
mount: mount /dev/sdb on /btrfs failed: Structure needs cleaning
I am ok. However would have much better if it says checksum error.
Do you/anyone suggest I should update the errno.h?
Thanks, Anand
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] btrfs: return required error from btrfs_check_super_csum
2018-03-21 2:37 ` Anand Jain
@ 2018-03-21 3:01 ` Anand Jain
0 siblings, 0 replies; 16+ messages in thread
From: Anand Jain @ 2018-03-21 3:01 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 03/21/2018 10:37 AM, Anand Jain wrote:
>
>
>>> @@ -418,8 +418,10 @@ static int btrfs_check_super_csum(struct
>>> btrfs_fs_info *fs_info,
>>> crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
>>> btrfs_csum_final(crc, result);
>>> - if (memcmp(raw_disk_sb, result, sizeof(result)))
>>> - return 1;
>>> + if (memcmp(raw_disk_sb, result, sizeof(result))) {
>>> + btrfs_err(fs_info, "superblock checksum mismatch");
>>> + return -EINVAL;
>>
>> Don't we want EUCLEAN here, since an error in the checksum indicates
>> corruption?
>
> With the v2 patch sentout here is the error str that is seen on the cli.
>
> mount: mount /dev/sdb on /btrfs failed: Structure needs cleaning
And since we return -EINVAL for the error csum_type not found we get..
mount: wrong fs type, bad option, bad superblock on /dev/sdb,
missing codepage or helper program, or other error
In some cases useful info is found in syslog - try
dmesg | tail or so.
Looks like mount syscall interprets -EINVAL error.
> I am ok. However would have much better if it says checksum error.
Also we may need an error code to say incompatible disk format/version.
> Do you/anyone suggest I should update the errno.h?
Thanks, Anand
>
> Thanks, Anand
> --
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 0/2] Preparatory to add the csum check in the scan context
@ 2018-03-21 8:14 ` Anand Jain
2018-03-21 8:14 ` [PATCH v3 1/2] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type Anand Jain
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Anand Jain @ 2018-03-21 8:14 UTC (permalink / raw)
To: linux-btrfs
v2->v3:
1/2: Keep the check %(csum_type >= ARRAY_SIZE(btrfs_csum_sizes))
because it is inline to support future expansion, further
btrfs-progs is already copied it.
2/2: Drop pr_err for default error which won't be possible.
v1->v2:
Merge 2/3 and 3/3
Use -EUCLEAN for csum mismatch
Move the error logging to the parent function
Drop the struct btrfs_fsinfo as the argument for
btrfs_check_super_csum()
Do not make btrfs_check_super_csum() nonstatic here, it will be part
of the patchset which will use the csum in the scan context
Anand Jain (2):
btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the
type
btrfs: return required error from btrfs_check_super_csum
fs/btrfs/disk-io.c | 53 ++++++++++++++++++++++++++---------------------------
1 file changed, 26 insertions(+), 27 deletions(-)
--
2.15.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 1/2] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type
2018-03-21 8:14 ` [PATCH v3 0/2] Preparatory to add the csum check in the scan context Anand Jain
@ 2018-03-21 8:14 ` Anand Jain
2018-03-21 8:14 ` [PATCH v3 2/2] btrfs: return required error from btrfs_check_super_csum Anand Jain
2018-03-26 6:40 ` [PATCH v3 0/2] Preparatory to add the csum check in the scan context Anand Jain
2 siblings, 0 replies; 16+ messages in thread
From: Anand Jain @ 2018-03-21 8:14 UTC (permalink / raw)
To: linux-btrfs
%csum_type is being checked to check the number of csum types we support,
which is 1 as of now. Instead just check if the type matches with the
only type we support, that is BTRFS_CSUM_TYPE_CRC32. And further adds
cleanup.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/disk-io.c | 38 +++++++++++++++++---------------------
1 file changed, 17 insertions(+), 21 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 1657d6aa4fa6..b9b435579527 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -399,32 +399,28 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
struct btrfs_super_block *disk_sb =
(struct btrfs_super_block *)raw_disk_sb;
u16 csum_type = btrfs_super_csum_type(disk_sb);
- int ret = 0;
-
- if (csum_type == BTRFS_CSUM_TYPE_CRC32) {
- u32 crc = ~(u32)0;
- char result[sizeof(crc)];
-
- /*
- * The super_block structure does not span the whole
- * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space
- * is filled with zeros and is included in the checksum.
- */
- crc = btrfs_csum_data(raw_disk_sb + BTRFS_CSUM_SIZE,
- crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
- btrfs_csum_final(crc, result);
-
- if (memcmp(raw_disk_sb, result, sizeof(result)))
- ret = 1;
- }
+ u32 crc = ~(u32)0;
+ char result[sizeof(crc)];
if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) {
btrfs_err(fs_info, "unsupported checksum algorithm %u",
- csum_type);
- ret = 1;
+ csum_type);
+ return 1;
}
- return ret;
+ /*
+ * The super_block structure does not span the whole
+ * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space
+ * is filled with zeros and is included in the checksum.
+ */
+ crc = btrfs_csum_data(raw_disk_sb + BTRFS_CSUM_SIZE,
+ crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
+ btrfs_csum_final(crc, result);
+
+ if (memcmp(raw_disk_sb, result, sizeof(result)))
+ return 1;
+
+ return 0;
}
/*
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] btrfs: return required error from btrfs_check_super_csum
2018-03-21 8:14 ` [PATCH v3 0/2] Preparatory to add the csum check in the scan context Anand Jain
2018-03-21 8:14 ` [PATCH v3 1/2] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type Anand Jain
@ 2018-03-21 8:14 ` Anand Jain
2018-03-26 6:40 ` [PATCH v3 0/2] Preparatory to add the csum check in the scan context Anand Jain
2 siblings, 0 replies; 16+ messages in thread
From: Anand Jain @ 2018-03-21 8:14 UTC (permalink / raw)
To: linux-btrfs
Return the required -EINVAL and -EUCLEAN from the function
btrfs_check_super_csum(). And more the error log into the
parent function.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/disk-io.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b9b435579527..062b3e0c7fd1 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -392,9 +392,10 @@ static int verify_parent_transid(struct extent_io_tree *io_tree,
/*
* Return 0 if the superblock checksum type matches the checksum value of that
* algorithm. Pass the raw disk superblock data.
+ * Otherwise: -EINVAL if csum type is not found
+ * -EUCLEAN if csum does not match
*/
-static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
- char *raw_disk_sb)
+static int btrfs_check_super_csum(char *raw_disk_sb)
{
struct btrfs_super_block *disk_sb =
(struct btrfs_super_block *)raw_disk_sb;
@@ -402,11 +403,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
u32 crc = ~(u32)0;
char result[sizeof(crc)];
- if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) {
- btrfs_err(fs_info, "unsupported checksum algorithm %u",
- csum_type);
- return 1;
- }
+ if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes))
+ return -EINVAL;
/*
* The super_block structure does not span the whole
@@ -418,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
btrfs_csum_final(crc, result);
if (memcmp(raw_disk_sb, result, sizeof(result)))
- return 1;
+ return -EUCLEAN;
return 0;
}
@@ -2570,9 +2568,14 @@ int open_ctree(struct super_block *sb,
* We want to check superblock checksum, the type is stored inside.
* Pass the whole disk block of size BTRFS_SUPER_INFO_SIZE (4k).
*/
- if (btrfs_check_super_csum(fs_info, bh->b_data)) {
- btrfs_err(fs_info, "superblock checksum mismatch");
- err = -EINVAL;
+ err = btrfs_check_super_csum(bh->b_data);
+ if (err) {
+ if (err == -EINVAL)
+ pr_err("BTRFS error (device %pg): unsupported checksum algorithm",
+ fs_devices->latest_bdev);
+ else if (err == -EUCLEAN)
+ pr_err("BTRFS error (device %pg): superblock checksum mismatch",
+ fs_devices->latest_bdev);
brelse(bh);
goto fail_alloc;
}
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] Preparatory to add the csum check in the scan context
2018-03-21 8:14 ` [PATCH v3 0/2] Preparatory to add the csum check in the scan context Anand Jain
2018-03-21 8:14 ` [PATCH v3 1/2] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type Anand Jain
2018-03-21 8:14 ` [PATCH v3 2/2] btrfs: return required error from btrfs_check_super_csum Anand Jain
@ 2018-03-26 6:40 ` Anand Jain
2 siblings, 0 replies; 16+ messages in thread
From: Anand Jain @ 2018-03-26 6:40 UTC (permalink / raw)
To: linux-btrfs
Pls, ignore this bunch as I have included this with other cleanups.
Thanks, Anand
On 03/21/2018 04:14 PM, Anand Jain wrote:
> v2->v3:
> 1/2: Keep the check %(csum_type >= ARRAY_SIZE(btrfs_csum_sizes))
> because it is inline to support future expansion, further
> btrfs-progs is already copied it.
> 2/2: Drop pr_err for default error which won't be possible.
>
> v1->v2:
> Merge 2/3 and 3/3
> Use -EUCLEAN for csum mismatch
> Move the error logging to the parent function
> Drop the struct btrfs_fsinfo as the argument for
> btrfs_check_super_csum()
> Do not make btrfs_check_super_csum() nonstatic here, it will be part
> of the patchset which will use the csum in the scan context
>
> Anand Jain (2):
> btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the
> type
> btrfs: return required error from btrfs_check_super_csum
>
> fs/btrfs/disk-io.c | 53 ++++++++++++++++++++++++++---------------------------
> 1 file changed, 26 insertions(+), 27 deletions(-)
>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-03-26 6:38 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-20 9:53 [PATCH 0/3] Preparatory to add the csum check in the scan context Anand Jain
2018-03-20 9:53 ` [PATCH 1/3] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type Anand Jain
2018-03-20 10:11 ` Nikolay Borisov
2018-03-20 10:12 ` Nikolay Borisov
2018-03-20 9:53 ` [PATCH 2/3] btrfs: return required error from btrfs_check_super_csum Anand Jain
2018-03-20 10:13 ` Nikolay Borisov
2018-03-20 22:23 ` Anand Jain
2018-03-21 2:37 ` Anand Jain
2018-03-21 3:01 ` Anand Jain
2018-03-20 9:53 ` [PATCH 3/3] btrfs: refactor btrfs_check_super_csum() for scan Anand Jain
2018-03-20 11:10 ` Nikolay Borisov
2018-03-20 22:32 ` Anand Jain
2018-03-21 8:14 ` [PATCH v3 0/2] Preparatory to add the csum check in the scan context Anand Jain
2018-03-21 8:14 ` [PATCH v3 1/2] btrfs: cleanup btrfs_check_super_csum() to check the csum_type to the type Anand Jain
2018-03-21 8:14 ` [PATCH v3 2/2] btrfs: return required error from btrfs_check_super_csum Anand Jain
2018-03-26 6:40 ` [PATCH v3 0/2] Preparatory to add the csum check in the scan context Anand Jain
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.