Re: [PATCH v6 1/8] Btrfs: introduce a tree for items that map UUIDs to something

[Eric Sandeen <sandeen@redhat.com>]

On 6/26/13 5:47 PM, Stefan Behrens wrote:
> On 06/26/2013 21:55, Zach Brown wrote:
>>> +    if (!uuid_root) {
>>> +        WARN_ON_ONCE(1);
>>> +        ret = -ENOENT;
>>> +        goto out;
>>> +    }
>>
>> WARN_ON_ONCE specifically returns the condition so that you can write:
>>
>>     if (WARN_ON_ONCE(!uuid_root)) {
>>         ret = -ENOENT;
>>         goto out;
>>     }
>>
>>> +    while (item_size) {
>>> +        u64 data;
>>> +
>>> +        read_extent_buffer(eb, &data, offset, sizeof(data));
>>> +        data = le64_to_cpu(data);
>>> +        if (data == subid) {
>>> +            ret = 0;
>>> +            break;
>>> +        }
>>> +        offset += sizeof(data);
>>> +        item_size -= sizeof(data);
>>> +    }
>>
>> fs/btrfs/uuid-tree.c:81 col 24 warning: cast to restricted __le64
>>
>> There are a few more instances of this.  The good news is that fixing
>> the sparse warning makes the code better, too.
>>
>>         __le64 data;
>>
>>         read_extent_buffer(eb, &data, offset, sizeof(data));
>>         if (le64_to_cpu(data) == subid) {
>>
>> Plese make sure the rest of the series doesn't add sparse warnings for
>> Josef to get email about a few seconds after he merges.
>>
>>> +int btrfs_insert_uuid_subvol_item(struct btrfs_trans_handle *trans,
>>> +                  struct btrfs_root *uuid_root, u8 *uuid,
>>> +                  u64 subvol_id)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = btrfs_uuid_tree_lookup(uuid_root, uuid,
>>> +                     BTRFS_UUID_KEY_SUBVOL, subvol_id);
>>> +    if (ret == -ENOENT)
>>> +        ret = btrfs_uuid_tree_add(trans, uuid_root, uuid,
>>> +                      BTRFS_UUID_KEY_SUBVOL, subvol_id);
>>> +    return ret;
>>> +}
>>
>>
>>> +int btrfs_insert_uuid_received_subvol_item(struct btrfs_trans_handle *trans,
>>> +                       struct btrfs_root *uuid_root,
>>> +                       u8 *uuid, u64 subvol_id)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = btrfs_uuid_tree_lookup(uuid_root, uuid,
>>> +                     BTRFS_UUID_KEY_RECEIVED_SUBVOL, subvol_id);
>>> +    if (ret == -ENOENT)
>>> +        ret = btrfs_uuid_tree_add(trans, uuid_root, uuid,
>>> +                      BTRFS_UUID_KEY_RECEIVED_SUBVOL,
>>> +                      subvol_id);
>>> +    return ret;
>>> +}
>>
>> Just have callers pass in the key type so we get slightly less enormous
>> function names and less cut-and-paste code.
> 
> Thanks for your comments, but this salami review procedure is not very efficient. Everything that you comment on now and before is there since V1.

I'm not sure that makes it any less relevant.   We'd all like complete & early reviews, but unfortunately it's a busy, messy world.  Sparse will keep complaining even at V7 w/o fixing it.  :)  So better late than never, no?

> Please tell me when you are done with the full review. And please also stop the bikeshedding.

Catching something new on the 2nd review pass isn't that unusual.  I tend to agree that not cutting & pasting 25 lines is a noble goal (not really bikeshedding) if all it takes is a key argument to avoid it... fs/btrfs is already plenty big.

Just my 2 cents.

-Eric