* [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* 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
* [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* 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 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 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 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 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 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
* [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