On 12/29/2011 01:23 AM, Christoph Hellwig wrote:
>> +struct trunk_entry {
>> + uint64_t oid;
>> + unsigned char sha1[SHA1_LEN];
>> +};
>> +
>> +struct __trunk_entry {
>> + struct trunk_entry raw;
>> + int flags;
>> + struct list_head active_list;
>> + struct hlist_node hash;
>> +};
>
> The naming seems a bit unfortunate. It seems like the first one is
> the on-disk variant, and the second the incore one. Maybe all on-disk
> structure should get a _disk postfix? Also what about making the disk
> format endian clean and always using __le or __be types like in the
> Linux kernel? The same also applies to the on the wire format.
>
Looks good to me.
>> +#define trunk_entry_size() sizeof(struct __trunk_entry)
>
> Does this helper really buy you anything?
>
Ah, this is leftover for previous dev, which I had use a zero length
array inside structure. Since now no that thing, I'll remove it in next
release.
>> +
>> +#define HASH_BITS 10
>> +#define HASH_SIZE (1 << HASH_BITS)
>> +
>> +static LIST_HEAD(trunk_active_list); /* no lock protection */
>> +static struct hlist_head trunk_hashtable[HASH_SIZE];
>> +static pthread_mutex_t hashtable_lock[HASH_SIZE] = { [0 ... HASH_SIZE - 1] = PTHREAD_MUTEX_INITIALIZER };
>> +static unsigned int trunk_entry_active_nr;
>
> Is a hash really a good data structure here? A tree would scale much
> better with the number of OIDs, which will be very different for
> different deployments. For example the xfsprogs code has a nice btree
> library to borrow.
>
>> +
>> +static inline struct __trunk_entry *alloc_trunk_entry(void)
>> +{
>> + return (struct __trunk_entry *)malloc(trunk_entry_size());
>> +}
>
>
> No need for the case here.
>
This is leftover for previous slab allocator. I'll remove this redundant
call in next release.
>> + struct dirent *d;
>> + uint64_t oid;
>> +
>> + dir = opendir(obj_path);
>> + if (!dir)
>> + return -1;
>> +
>> + while ((d = readdir(dir))) {
>> + if (!strncmp(d->d_name, ".", 1))
>> + continue;
>> + oid = strtoull(d->d_name, NULL, 16);
>> + if (oid == 0)
>> + continue;
>
> You should also check for ULLONG_MAX in case we ended up with a
> non-numerical entry in the directory for some reason.
>
Okay.
Thanks,
Yuan
|
|