btrfs: Do super block verification before writing it to disk

v3:
Update commit message to show the corruption in details. Modify the kernel error message to show corruption is detected before transaction commitment.

Quoted 5 lines

v3:
   Update commit message to show the corruption in details. Modify the kernel error message to show corruption is detected before transaction commitment.

 Nice. Thanks. more below.

Quoted 10 lines

(quoted diff) +0 -0

@@ -3310,6 +3311,27 @@ static int write_dev_supers(struct btrfs_device
 *device,
             btrfs_set_super_bytenr(sb, bytenr);

  +        /* check the validation of the primary sb before writing */ +        if (i == 0) {
+            ret = btrfs_check_super_valid(device->fs_info, sb); +            if (ret) {
+                btrfs_err(device->fs_info,
+"superblock corruption detected before transaction commitment for device %llu",

Quoted 3 lines

+                      device->devid);
+                return -EUCLEAN;
+            }

 Why not move this entire check further below, after we have the ready crc and use btrfs_check_super_csum(), instead of btrfs_check_super_valid()? so that we verify only what is known to be corrupted that is ..

btrfs_super_block {
::
        __le64 incompat_flags;
        __le16 csum_type;
::
}

 And also can you dump contents of incompat_flags and csum_type at both fs_info->super_copy
 and
   fs_info->super_for_commit

 Because at each commit transaction we

btrfs_commit_transaction()
{
  ::
        memcpy(fs_info->super_for_commit, fs_info->super_copy, sizeof(*fs_info->super_copy));
  ::
        ret = write_all_supers(fs_info, 0);

}

 And also the sync log can write the

btrfs_sync_log()
{
::
        ret = write_all_supers(fs_info, 1);

 Finally locks between these two threads needs a review as well.

Thanks, Anand

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 17 lines · Anand Jain

Quoted 5 lines

v3:
   Update commit message to show the corruption in details. Modify the kernel error message to show corruption is detected before transaction commitment.

 Nice. Thanks. more below.

Quoted 10 lines

fs/btrfs/disk-io.c +5 -0

@@ -3310,6 +3311,27 @@ static int write_dev_supers(struct btrfs_device
 *device,
             btrfs_set_super_bytenr(sb, bytenr);
   +        /* check the validation of the primary sb before writing */
+        if (i == 0) {
+            ret = btrfs_check_super_valid(device->fs_info, sb);
+            if (ret) {
+                btrfs_err(device->fs_info,
+"superblock corruption detected before transaction commitment for
 device %llu",

Quoted 8 lines

fs/btrfs/disk-io.c static int write_dev_supers(struct btrfs_device *device, · line 3322

Quoted 3 lines

fs/btrfs/disk-io.c +3 -0

+                      device->devid);
+                return -EUCLEAN;
+            }

 Why not move this entire check further below, after we have the ready crc and use btrfs_check_super_csum(), instead of btrfs_check_super_valid()? so that we verify only what is known to be corrupted that is ..

The problem is, we don't know the cause yet, so we must check the whole superblock.

For example, if the corruption is caused by some wild pointer of other kernel module, and we're just unlucky that one day it corrupts nodesize, then we can't detect it if we only check certain members.

Quoted 7 lines

btrfs_super_block {
::
        __le64 incompat_flags;
        __le16 csum_type;
::
}

Quoted 4 lines

 And also can you dump contents of incompat_flags and csum_type at both fs_info->super_copy
 and
   fs_info->super_for_commit

Not really needed, as when corruption happens, it's super_copy corrupted, not something went wrong after we called memcpy()

Thanks,
Qu

Quoted 28 lines

 Because at each commit transaction we

btrfs_commit_transaction()
{
  ::
        memcpy(fs_info->super_for_commit, fs_info->super_copy, sizeof(*fs_info->super_copy));
  ::
        ret = write_all_supers(fs_info, 0);

}

 And also the sync log can write the

btrfs_sync_log()
{
::
        ret = write_all_supers(fs_info, 1);

 Finally locks between these two threads needs a review as well.

Thanks, Anand

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 30 lines · Qu Wenruo

Quoted 18 lines · Anand Jain

Quoted 6 lines

v3:
    Update commit message to show the corruption in details. Modify the kernel error message to show corruption is detected before
    transaction commitment.

  Nice. Thanks. more below.

Quoted 10 lines

(quoted diff) +0 -0

@@ -3310,6 +3311,27 @@ static int write_dev_supers(struct btrfs_device
 *device,
              btrfs_set_super_bytenr(sb, bytenr);

   +        /* check the validation of the primary sb before writing */ +        if (i == 0) {
+            ret = btrfs_check_super_valid(device->fs_info, sb); +            if (ret) {
+                btrfs_err(device->fs_info,
+"superblock corruption detected before transaction commitment for device %llu",

Quoted 8 lines

Quoted 3 lines

+                      device->devid);
+                return -EUCLEAN;
+            }

  Why not move this entire check further below, after we have the ready crc and use btrfs_check_super_csum(), instead of btrfs_check_super_valid()? so that we verify only what is known to be corrupted that is ..

Quoted 6 lines

The problem is, we don't know the cause yet, so we must check the whole superblock.

For example, if the corruption is caused by some wild pointer of other kernel module, and we're just unlucky that one day it corrupts nodesize, then we can't detect it if we only check certain members.

Right I notice that.

But without btrfs_check_super_csum(), it leaves out checking one of the member (csum_type) which is know to be corrupted at the two instances, so it can also include btrfs_check_super_csum().

There were two cases, both of them corrupted the same offset, its not just a coincidence that both of these reported corrupted the same offset.

Also the incompatible features flags (169) are still valid in both the cases. It looks as if we wrote u32 to a u64. I notice that we provide the options to write the incompatible flags through mount option, sysfs and ioctl.

Quoted 14 lines

Quoted 13 lines

btrfs_super_block {
::
         __le64 incompat_flags;
         __le16 csum_type;
::
}

  And also can you dump contents of incompat_flags and csum_type at both fs_info->super_copy
  and
    fs_info->super_for_commit

Quoted 2 lines

Not really needed, as when corruption happens, it's super_copy corrupted, not something went wrong after we called memcpy()

As shown below, we aren't memcpy()-ing in the btrfs_sync_log() thread, did you check if btrfs_sync_log() can not be the last person to write at umount?

Thanks, Anand

Quoted 33 lines

Thanks,
Qu

Quoted 29 lines

  Because at each commit transaction we

btrfs_commit_transaction()
{
   ::
         memcpy(fs_info->super_for_commit, fs_info->super_copy, sizeof(*fs_info->super_copy));
   ::
         ret = write_all_supers(fs_info, 0);

}

  And also the sync log can write the

btrfs_sync_log()
{
::
         ret = write_all_supers(fs_info, 1);

  Finally locks between these two threads needs a review as well.

Thanks, Anand
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

fs/btrfs/disk-io.c static int write_dev_supers(struct btrfs_device *device,

Quoted 10 lines · Qu Wenruo

fs/btrfs/disk-io.c +1 -1

@@ -2680,7 +2681,7 @@ int open_ctree(struct super_block *sb,
 
 	memcpy(fs_info->fsid, fs_info->super_copy->fsid, BTRFS_FSID_SIZE);
 
-	ret = btrfs_check_super_valid(fs_info);
+	ret = btrfs_check_super_valid(fs_info, fs_info->super_copy);
 	if (ret) {
 		btrfs_err(fs_info, "superblock contains fatal errors");
 		err = -EINVAL;

@@ -3310,6 +3311,27 @@ static int write_dev_supers(struct btrfs_device *device,

This is in write_dev_supers, so the superblock is checked number-of-devices times. The caller write_all_supers rewrites the device item so it matches the device it's going to write to. But, btrfs_check_super_valid does not validate the dev_item so all the validation does not bring much benefit, as it repeatedly checks the same data.

So, what if the validation is done only once in write_all_supers? Lock the devices, validate, if it fails, report that and unlock devices and go readonly.

There's a differnce to what you implemented: if the in-memory superblock corruption happens between writing to the devices, there are some left with the new superblock and some with the old.

Although this sounds quite improbable, I think that doing the check in advance would save some trouble if that happens. The superblocks on all devices will match.