Re: [PATCH 6/8] btrfs: verify checksum when superblock is read for scan

[Nikolay Borisov <nborisov@suse.com>]

On 28.03.2018 02:01, Anand Jain wrote:
> 
> 
>>>> Test script:
>>>>
>>>>   Corrupt primary superblock and check if device scan and mount
>>>>   fails:
>>>>   mkfs.btrfs -fq /dev/sdc
>>>>   dd if=/dev/urandom of=/dev/sdc ibs=1 obs=1 count=1 seek=64K
>>>>   btrfs dev scan
>>>>   mount /dev/sdb /btrfs
>>>>
>>>>   Corrupt secondary superblock and check if device scan and mount
>>>>   is succcessful, check for the dmesg for errors.
>>>>   mkfs.btrfs -fq /dev/sdc
>>>>   dd if=/dev/urandom of=/dev/sdc ibs=1 obs=1 count=1 seek=64K
>>>>   btrfs dev scan
>>>>   mount /dev/sdb /btrfs
>>>
>>> Have you considered adding fstests, it will be very easy to test for
>>> this behavior?
> 
>  This is one off kind of bug, not sure if it would value add
>  for checking it all the time in xfstests?
> 

It's some btrfs-specific behavior which in order to not regress in the
future it will be best to have tests for. IMO whatever we can test
should be tested to ensure maintainability. I think a btrfs-specific
test wouldn't hurt here.

> 
>>> Same comment as before regarding string literals splitting across lines.
> 
> accepted.
> 
> 
>>>> +        else
>>>> +            pr_err("BTRFS error (device %pg): "\
>>>> +                "superblock checksum failed, bytenr=%llu",
>>>> +                bdev, bytenr);
>>>> +        btrfs_release_disk_super(*page);
>>>> +        return err;
>>>> +    }
>>
>> Also it will be better to have the checksum check after we have verified
>> some basic invariants - that bytenr and magic have sane values.
> 
>  accepted.
> 
> Thanks,
> Anand
>