On Wed, Jul 31, 2013 at 03:50:50PM -0700, Zach Brown wrote:
>
> > +#define BTRFS_DEDUP_HASH_SIZE 32 /* 256bit = 32 * 8bit */
> > +#define BTRFS_DEDUP_HASH_LEN 4
> > +
> > +struct btrfs_dedup_hash_item {
> > + /* FIXME: put a hash type field here */
> > +
> > + __le64 hash[BTRFS_DEDUP_HASH_LEN];
> > +} __attribute__ ((__packed__));
>
> The handling of hashes in this patch is a mess.
>
> The inconsistent use of _SIZE, _LEN, and literal 4 and the u64 *s being
> passed around is asking for mistakes to be made in the future. And I
> don't think it's endian safe.
Yeah, you're right, I missed the endian part for hash.
>
> I think I'd have a struct that represents the native representation of
> the tree item. Something like:
>
> struct btrfs_dedup_hash {
> u64 key_value;
> u8 algo;
> u8 len;
> u8 bytes[0];
> }
>
> You can then have helpers that generate that from either the cryptolib
> transformation of dedup regions or to and from the tree items. The
> bytes (and the tree item payload) wouldn't need to have the hash bytes
> that are stored up in the key.
I agree on merging the two structs, btrfs_dedup_item and btrfs_dedup_hash_item,
into one.
So do you mean that our whole hash value will be (key.objectid + bytes)
because key.objectid is a part of hash value?
Thanks for the comments!
-liubo
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
