All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] Superblock read and verify cleanups
@ 2018-03-26  8:27 Anand Jain
  2018-03-26  8:27 ` [PATCH 1/8] btrfs: cleanup btrfs_check_super_csum() for better code flow Anand Jain
                   ` (7 more replies)
  0 siblings, 8 replies; 27+ messages in thread
From: Anand Jain @ 2018-03-26  8:27 UTC (permalink / raw)
  To: linux-btrfs

Patch 1-4/8 are preparatory patches adds cleanups and nonstatic requisites.

Patch 5/8 makes sure that all copies of the superblock have the same fsid
when we scan the device.

Patch 6/8 verifies superblock csum when we read it in the scan context.

Patch 7/8 fixes a bug that we weren't verifying the superblock csum for
the non-latest_bdev.

And 8/8 patch drops the redundant invalidate_bdev() call during mount.

There is a btrfs-progs patch which is a kind of related, as its found that
we weren't wiping the non-overwritten superblock, so it could cause
confusion during the superblock recovery process. So the patch btrfs-progs
1/1 adds code to wipe superblock if we aren't overwriting it.

Now since kernel patch 5/8 checks if all the superblock copies are
pointing to the same fsid on the disk, so the scan will fail if without
the above 1/1 btrfs-progs, as in the example below [1]. However the simple
workaround is to wipe the superblock manually [2] or apply the btrfs-progs
patch below.

[1]
 mkfs.btrfs -qf /dev/sdb <-- 1T disk
 mkfs.btrfs -b 256M  /dev/sdb
 ERROR: device scan failed on /dev/sdb

[2]
 dd if=/dev/zero of=<dev> seek=274877906944 ibs=1 obs=1 count4K

Unfortunately, the error messages should have been failed to register
[3] device into the kernel to be more appropriate to the error.

[3]
        ret = ioctl(fd, BTRFS_IOC_SCAN_DEV, &args);
        if (ret < 0) {
                error("device scan failed on '%s': %m", fname);
                ret = -errno;
        }


Patches 1-7/8 were sent independently before. And  I found few more things
to fix alongs the line, and since they are related, so I am sending these
all together. Also, as there are minor changes, like in pr_err strings,
and splitting the unrelated changes into a separate patch, so though I am
thankful for the received reviewed-by, I couldn't include them here. Sorry.

Finally, here I am including the function relations [4] so that it will help
to review the code. And this flow is before these patches were applied.

[4]
In the long term, I suggest deprecating ioctl args which pass device path
(where possible), like in delete-device/replace. And
btrfs_read_dev_one_super() should replace btrfs_read_disk_super()

delete-device/replace:
btrfs_rm_device() || btrfs_dev_replace_by_ioctl()
|_btrfs_find_device_by_devspec()
  |_btrfs_find_device_missing_or_by_path()
    |_btrfs_find_device_by_path()
      |_btrfs_get_bdev_and_sb()
        |_btrfs_read_dev_super()
          |_btrfs_read_dev_one_super()
            |___bread()

btrfs_mount_root()
 |
 |_btrfs_parse_early_options (-o device only)
 | |_btrfs_scan_one_device
 |   |_btrfs_read_disk_super()
 |     |_read_cache_page_gfp()
 |
 |_btrfs_scan_one_device(mount-arg-dev only)
 | |_btrfs_read_disk_super()
 |   |_read_cache_page_gfp()
 |
 |
 |_btrfs_open_devices(fsid:all)
 |  |_btrfs_open_one_device()
 |    |_btrfs_get_bdev_and_sb()  <--- invalidate_bdev(fsid:all)
 |      |_btrfs_read_dev_super()
 |        |_btrfs_read_dev_one_super()
 |          |___bread()
 |
 |_btrfs_fill_super()
   |_btrfs_open_ctree()   <-- invalidate_bdev(latest_bdev) <-- redundant
     |_btrfs_read_dev_super(latest_bdev only)
     | |_btrfs_read_dev_one_super(latest_bdev only)
     |   |___bread(latest_bdev)
     |
     |_btrfs_check_super_csum(latest_bdev only) [*]
     |
     |_btrfs_read_chunk_tree
     | |_read_one_dev()
     |   |_open_seed_devices()
     |     |_btrfs_open_devices(fs_devices->seed only)

scan/ready
|
|_btrfs_scan_one_device(ioctl-arg-dev only)
   |_btrfs_read_disk_super()
     |_read_cache_page_gfp()


Anand Jain (8):
  btrfs: cleanup btrfs_check_super_csum() for better code flow
  btrfs: return required error from btrfs_check_super_csum
  btrfs: cleanup btrfs_read_disk_super() to return std error
  btrfs: make btrfs_check_super_csum() non static
  btrfs: check if the fsid in the primary sb and copy sb are same
  btrfs: verify checksum when superblock is read for scan
  btrfs: verify checksum for all devices in mount context
  btrfs: drop the redundant invalidate_bdev()

 fs/btrfs/disk-io.c | 72 +++++++++++++++++++++++-----------------------
 fs/btrfs/disk-io.h |  1 +
 fs/btrfs/volumes.c | 84 ++++++++++++++++++++++++++++++++++++++++++------------
 3 files changed, 102 insertions(+), 55 deletions(-)

Anand Jain (1):
  btrfs-progs: wipe copies of the stale superblock beyond -b size

 utils.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

-- 
2.7.0


^ permalink raw reply	[flat|nested] 27+ messages in thread
* [PATCH v2 0/9] Superblock read and verify cleanups
@ 2018-03-27 23:39 Anand Jain
  2018-03-27 23:39 ` [PATCH 1/8] btrfs: cleanup btrfs_check_super_csum() for better code flow Anand Jain
  0 siblings, 1 reply; 27+ messages in thread
From: Anand Jain @ 2018-03-27 23:39 UTC (permalink / raw)
  To: linux-btrfs

v1->v2:
 Various changes suggested by Nicokay. Thanks. For specific changes
 pls ref to the patch.

Patch 1-4/8 are preparatory patches adds cleanups and nonstatic requisites.

Patch 5/8 makes sure that all copies of the superblock have the same fsid
when we scan the device.

Patch 6/8 verifies superblock csum when we read it in the scan context.

Patch 7/8 fixes a bug that we weren't verifying the superblock csum for
the non-latest_bdev.

And 8/8 patch drops the redundant invalidate_bdev() call during mount.

There is a btrfs-progs patch which is a kind of related, as its found that
we weren't wiping the non-overwritten superblock, so it could cause
confusion during the superblock recovery process. So the patch btrfs-progs
1/1 adds code to wipe superblock if we aren't overwriting it.

Now since kernel patch 5/8 checks if all the superblock copies are
pointing to the same fsid on the disk, so the scan will fail if without
the above 1/1 btrfs-progs, as in the example below [1]. However the simple
workaround is to wipe the superblock manually [2] or apply the btrfs-progs
patch below.

[1]
 mkfs.btrfs -qf /dev/sdb <-- 1T disk
 mkfs.btrfs -b 256M  /dev/sdb
 ERROR: device scan failed on /dev/sdb

[2]
 dd if=/dev/zero of=<dev> seek=274877906944 ibs=1 obs=1 count4K

Unfortunately, the error messages should have been failed to register
[3] device into the kernel to be more appropriate to the error.

[3]
        ret = ioctl(fd, BTRFS_IOC_SCAN_DEV, &args);
        if (ret < 0) {
                error("device scan failed on '%s': %m", fname);
                ret = -errno;
        }


Patches 1-7/8 were sent independently before. And  I found few more things
to fix alongs the line, and since they are related, so I am sending these
all together. Also, as there are minor changes, like in pr_err strings,
and splitting the unrelated changes into a separate patch, so though I am
thankful for the received reviewed-by, I couldn't include them here. Sorry.

Finally, here I am including the function relations [4] so that it will help
to review the code. And this flow is before these patches were applied.

[4]
In the long term, I suggest deprecating ioctl args which pass device path
(where possible), like in delete-device/replace. And
btrfs_read_dev_one_super() should replace btrfs_read_disk_super()

delete-device/replace:
btrfs_rm_device() || btrfs_dev_replace_by_ioctl()
|_btrfs_find_device_by_devspec()
  |_btrfs_find_device_missing_or_by_path()
    |_btrfs_find_device_by_path()
      |_btrfs_get_bdev_and_sb()
        |_btrfs_read_dev_super()
          |_btrfs_read_dev_one_super()
            |___bread()

btrfs_mount_root()
 |
 |_btrfs_parse_early_options (-o device only)
 | |_btrfs_scan_one_device
 |   |_btrfs_read_disk_super()
 |     |_read_cache_page_gfp()
 |
 |_btrfs_scan_one_device(mount-arg-dev only)
 | |_btrfs_read_disk_super()
 |   |_read_cache_page_gfp()
 |
 |
 |_btrfs_open_devices(fsid:all)
 |  |_btrfs_open_one_device()
 |    |_btrfs_get_bdev_and_sb()  <--- invalidate_bdev(fsid:all)
 |      |_btrfs_read_dev_super()
 |        |_btrfs_read_dev_one_super()
 |          |___bread()
 |
 |_btrfs_fill_super()
   |_btrfs_open_ctree()   <-- invalidate_bdev(latest_bdev) <-- redundant
     |_btrfs_read_dev_super(latest_bdev only)
     | |_btrfs_read_dev_one_super(latest_bdev only)
     |   |___bread(latest_bdev)
     |
     |_btrfs_check_super_csum(latest_bdev only) [*]
     |
     |_btrfs_read_chunk_tree
     | |_read_one_dev()
     |   |_open_seed_devices()
     |     |_btrfs_open_devices(fs_devices->seed only)

scan/ready
|
|_btrfs_scan_one_device(ioctl-arg-dev only)
   |_btrfs_read_disk_super()
     |_read_cache_page_gfp()


Anand Jain (8):
  btrfs: cleanup btrfs_check_super_csum() for better code flow
  btrfs: return required error from btrfs_check_super_csum
  btrfs: cleanup btrfs_read_disk_super() to return std error
  btrfs: make btrfs_check_super_csum() non static
  btrfs: check if the fsid in the primary sb and copy sb are same
  btrfs: verify checksum when superblock is read for scan
  btrfs: verify checksum for all devices in mount context
  btrfs: drop the redundant invalidate_bdev()

 fs/btrfs/disk-io.c | 72 +++++++++++++++++++++++-----------------------
 fs/btrfs/disk-io.h |  1 +
 fs/btrfs/volumes.c | 84 ++++++++++++++++++++++++++++++++++++++++++------------
 3 files changed, 102 insertions(+), 55 deletions(-)

Anand Jain (1):
  btrfs-progs: wipe copies of the stale superblock beyond -b size

 utils.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

-- 
2.7.0


^ permalink raw reply	[flat|nested] 27+ messages in thread
end of thread, other threads:[~2018-04-05 14:51 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-26  8:27 [PATCH 0/8] Superblock read and verify cleanups Anand Jain
2018-03-26  8:27 ` [PATCH 1/8] btrfs: cleanup btrfs_check_super_csum() for better code flow Anand Jain
2018-03-27 12:10   ` Nikolay Borisov
2018-03-26  8:27 ` [PATCH 2/8] btrfs: return required error from btrfs_check_super_csum Anand Jain
2018-03-27  8:05   ` Nikolay Borisov
2018-03-27 19:16     ` David Sterba
2018-03-27 20:43       ` Anand Jain
2018-04-05 14:48         ` David Sterba
2018-03-26  8:27 ` [PATCH 3/8] btrfs: cleanup btrfs_read_disk_super() to return std error Anand Jain
2018-03-27  8:07   ` Nikolay Borisov
2018-03-26  8:27 ` [PATCH 4/8] btrfs: make btrfs_check_super_csum() non-static Anand Jain
2018-03-27 12:10   ` Nikolay Borisov
2018-03-26  8:27 ` [PATCH 5/8] btrfs: check if the fsid in the primary sb and copy sb are same Anand Jain
2018-03-27  8:49   ` Nikolay Borisov
2018-03-27 22:06     ` Anand Jain
2018-03-28  6:08       ` Nikolay Borisov
2018-03-28  7:05         ` Anand Jain
2018-03-26  8:27 ` [PATCH 6/8] btrfs: verify checksum when superblock is read for scan Anand Jain
2018-03-27 11:30   ` Nikolay Borisov
2018-03-27 12:09     ` Nikolay Borisov
2018-03-27 23:01       ` Anand Jain
2018-03-28  6:12         ` Nikolay Borisov
2018-03-26  8:27 ` [PATCH 7/8] btrfs: verify checksum for all devices in mount context Anand Jain
2018-03-27 12:21   ` Nikolay Borisov
2018-03-27 22:48     ` Anand Jain
2018-03-26  8:27 ` [PATCH 8/8] btrfs: drop the redundant invalidate_bdev() Anand Jain
2018-03-27 23:39 [PATCH v2 0/9] Superblock read and verify cleanups Anand Jain
2018-03-27 23:39 ` [PATCH 1/8] btrfs: cleanup btrfs_check_super_csum() for better code flow 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.