On 2019/8/27 下午9:37, Jeff Mahoney wrote: > On 8/27/19 9:22 AM, Qu Wenruo wrote: >> Btrfs doesn't reuse devid, thus if we add and delete device in a loop, >> we can increase devid to higher value, triggering tree checker to give a >> false alert. >> >> But we still don't want to give up the devid check, so here we >> compromise by setting a larger devid upper limit, 1<<32. > > Is this really a useful check? There's no actual definition of what a > devid can be, only what the kernel/tools does right now when it adds new > devices. There's nothing in the format that requires it to be monotonic > increments, which makes any check on read unreliable.
Right, that check makes no sense.
> Once we do read
> all the dev items, we can check for corruption on write, though.
That could be too time consuming (we need to lookup each devid in
fs_devices list) to be done at write time.
So I'd prefer just to remove the devid check.
We already have dev_extent verification, so even we have corrupted
devid, we can detect it at mount time.
Thus even we have a devid corrupted by bitflip, we can still detect it,
although not by tree-checker.
Thanks,
Qu
>
> -Jeff
>
>> So crazy scripts can't bump devid to that high value easily, while we can
>> still detect obviously wrong devid.
>>
>> Reported-by: Anand Jain <anand.j...@oracle.com>
>> Fixes: ab4ba2e13346 ("btrfs: tree-checker: Verify dev item")
>> Signed-off-by: Qu Wenruo <w...@suse.com>
>> ---
>> fs/btrfs/tree-checker.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index 43e488f5d063..f9d24f01801e 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -686,9 +686,14 @@ static void dev_item_err(const struct extent_buffer
>> *eb, int slot,
>> static int check_dev_item(struct extent_buffer *leaf,
>> struct btrfs_key *key, int slot)
>> {
>> - struct btrfs_fs_info *fs_info = leaf->fs_info;
>> struct btrfs_dev_item *ditem;
>> - u64 max_devid = max(BTRFS_MAX_DEVS(fs_info), BTRFS_MAX_DEVS_SYS_CHUNK);
>> + /*
>> + * Btrfs doesn't really reuse devid, thus devid can increase to any
>> + * value, but we don't believe a devid higher than (1<<32) is really
>> + * valid. This could at least detect bitflip at the higher
>> + * 32 bits while still consider high devid valid.
>> + */
>> + u64 max_devid = (1ULL << 32);
>>
>> if (key->objectid != BTRFS_DEV_ITEMS_OBJECTID) {
>> dev_item_err(leaf, slot,
>>
>
>
signature.asc
Description: OpenPGP digital signature
