[sheepdog] [PATCH 1/3] md: make vdisk management dynamic

[Liu Yuan]

On Tue, Sep 03, 2013 at 04:34:04PM +0900, MORITA Kazutaka wrote:
> At Tue,  3 Sep 2013 11:06:53 +0800,
> Liu Yuan wrote:
> > 
> > diff --git a/sheep/md.c b/sheep/md.c
> > index d51a7c6..f94fd29 100644
> > --- a/sheep/md.c
> > +++ b/sheep/md.c
> > @@ -13,98 +13,116 @@
> >  
> >  #include "sheep_priv.h"
> >  
> > -#define MD_DEFAULT_VDISKS 128
> > -#define MD_MAX_VDISK (MD_MAX_DISK * MD_DEFAULT_VDISKS)
> > +#define MD_VDISK_SIZE ((uint64_t)1*1024*1024*1024) /* 1G */
> > +
> > +#define NONE_EXIST_PATH "/all/disks/are/broken/,ps/əʌo7/!"
> 
> The path may exist, no?  How about using "" as a non-existent path?

I think we can't use "", eg when it is used in "%s%lx" for oid, we'll
oid for the working directory.

This path isn't magic enough?

> > +static inline uint64_t vnode_hash(struct rb_node *node)
> > +{
> > +	struct vdisk *v;
> >  
> > -		nr++;
> > -	}
> > +	v = rb_entry(node, struct vdisk, rb);
> > +	return v->hash;
> > +}
> >  
> > -	return nr;
> > +static inline struct vdisk *vnode_vdisk(struct rb_node *node)
> > +{
> > +	return rb_entry(node, struct vdisk, rb);
> >  }
> >  
> > -static inline int disks_to_vdisks(const struct disk *ds, int nmds, struct vdisk *vds)
> > +static struct vdisk *find_right_close(struct rb_node *node, uint64_t hash)
> >  {
> > -	int nr_vdisks = 0;
> > +	struct vdisk *v;
> >  
> > -	for (int i = 0; i < nmds; i++)
> > -		nr_vdisks += disk_to_vdisks(ds, i, vds + nr_vdisks);
> > +	if (hash < vnode_hash(node)) {
> > +		if (!node->rb_left)
> > +			return vnode_vdisk(node);
> >  
> > -	xqsort(vds, nr_vdisks, vdisk_cmp);
> > +		v = find_right_close(node->rb_left, hash);
> > +		return v->hash - hash > vnode_hash(node) - hash ?
> > +			vnode_vdisk(node) : v;
> > +	} else if (hash > vnode_hash(node)) {
> > +		if (!node->rb_right)
> > +			return vnode_vdisk(rb_first(&md.vroot));
> > +		return find_right_close(node->rb_right, hash);
> > +	} else {
> > +		return vnode_vdisk(node);
> > +	}
> > +}
> >  
> > -	return nr_vdisks;
> > +/* If v1_hash < oid <= v2_hash, then oid is resident in v2 */
> > +static struct vdisk *oid_to_vdisk(uint64_t oid)
> > +{
> > +	uint64_t hash = sd_hash_oid(oid);
> > +
> > +	return find_right_close(md.vroot.rb_node, hash);
> >  }
> 
> I think we will implement a similar function when we change node
> management from a fixed array to rbtree.  How about adding a helper
> function in rbtree.h like as follows?  Then, the above code will be
> much cleaner.
> 
> /*
>  * Search for a value in the rbtree.  When the key is not found in the rbtree,
>  * this returns the next greater node if one exists.
>  */
> #define rb_nsearch(root, key, member, compar)				\
> ({									\
> 	struct rb_node *__n = (root)->rb_node;				\
> 	typeof(key) __ret = NULL, __data;				\
> 									\
> 	while (__n) {							\
> 		__data = rb_entry(__n, typeof(*key), member);		\
> 		int __cmp = compar(key, __data);			\
> 									\
> 		if (__cmp < 0) {					\
> 			__ret = __data;					\
> 			__n = __n->rb_left;				\
> 		} else if (__cmp > 0)					\
> 			__n = __n->rb_right;				\
> 		else {							\
> 			__ret = __data;					\
> 			break;						\
> 		}							\
> 	}								\
> 	__ret;								\
> })
> 

This macro can't return the right closest element, but yes, I can write a helper
for it.

> 
> >  
> > -static inline struct vdisk *oid_to_vdisk(uint64_t oid)
> > +static void create_vdisks(struct disk *disk)
> >  {
> > -	return oid_to_vdisk_from(md_vds, md_nr_vds, oid);
> > +	uint64_t hval = sd_hash(disk->path, strlen(disk->path));
> > +
> > +	for (int i = 0; i < vdisk_number(disk); i++) {
> 
> GCC can know that vdisk_number(disk) returns a constant number while
> looping?  I think it's better to save the number of vdisks to a
> temporary variable:
> 
> 	nr = vdisk_number(disk);
> 	for (int i = 0; i < nr; i++) {
> 

I'm not quite sure, but fine for me to modify as you suggested.

Thanks
Yuan