[Sheepdog] [PATCH v4 04/12] farm: add trunk object

[Liu Yuan]

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