Re: [PATCH v4 0/7] Superblock read and verify cleanups

2018-04-12 Thread Anand Jain



On 04/10/2018 10:48 PM, David Sterba wrote:

On Fri, Mar 30, 2018 at 08:09:16AM +0800, Anand Jain wrote:

v3->v4:
  Update changelog and signoff.
  Reintroduce explicit check for '-EUCLEAN'
   at Patch 2/8 and 5/8.

v2->v3:
  Squash
   4/8 btrfs: make btrfs_check_super_csum() non static
  to
   6/8 btrfs: verify superblock checksum during scan
  As in the individual patch mentioned
  
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.


This is IMO too strict and the need of workaround puts the burden on the
user while it's not necessary and scan or mount can continue as long as
the first superblock is valid.


 IMO there is indirect loss for not being too strict here. Offline
 disk-corruptions will be reported as btrfs corruption at a later stage,
 by that time we would have overwritten the superblocks on all the
 disks. Its just about doing the right thing and IMO is pays well to
 itself in the long term. I am ok to drop this if any disagreements.


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


This was the main intention, as I read the discussion in the thread
under the RFC patch.

The semantics of scanning is not exactly defined, in the current code
it's "pick any device that has the right magic and sb offset", so adding
the csum check would add some validation but otherwise it's fine. All
the work is left to mount and all reported errors can be examined
immediatelly.

Wiping the magic either manually or by wipefs will not touch the csum,
so we don't change the behaviour.


 Will fix progs patch. Just noticed that wipefs wipes the magic only,
 I could do that in the progs patch here.


Reporting errors on each scan is a usability no-go.


 Ok.


Patches 1-4 look ok,
I'll have to think more about 5.


 Sure.

Thanks, Anand


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


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


Re: [PATCH v4 0/7] Superblock read and verify cleanups

2018-04-10 Thread David Sterba
On Fri, Mar 30, 2018 at 08:09:16AM +0800, Anand Jain wrote:
> v3->v4:
>  Update changelog and signoff.
>  Reintroduce explicit check for '-EUCLEAN'
>   at Patch 2/8 and 5/8.
> 
> v2->v3:
>  Squash
>   4/8 btrfs: make btrfs_check_super_csum() non static
>  to
>   6/8 btrfs: verify superblock checksum during scan
>  As in the individual patch mentioned
>  
> 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.

This is IMO too strict and the need of workaround puts the burden on the
user while it's not necessary and scan or mount can continue as long as
the first superblock is valid.

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

This was the main intention, as I read the discussion in the thread
under the RFC patch.

The semantics of scanning is not exactly defined, in the current code
it's "pick any device that has the right magic and sb offset", so adding
the csum check would add some validation but otherwise it's fine. All
the work is left to mount and all reported errors can be examined
immediatelly.

Wiping the magic either manually or by wipefs will not touch the csum,
so we don't change the behaviour.

Reporting errors on each scan is a usability no-go. Patches 1-4 look ok,
I'll have to think more about 5.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v4 0/7] Superblock read and verify cleanups

2018-03-29 Thread Anand Jain
v3->v4:
 Update changelog and signoff.
 Reintroduce explicit check for '-EUCLEAN'
  at Patch 2/8 and 5/8.

v2->v3:
 Squash
  4/8 btrfs: make btrfs_check_super_csum() non static
 to
  6/8 btrfs: verify superblock checksum during scan
 As in the individual patch mentioned
 
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= 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, );
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 (7):
  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: check if the fsid in the primary sb and copy sb are same
  btrfs: verify superblock checksum during 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 | 72 ++
 3 files changed, 92 insertions(+), 53 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

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