[sheepdog] [PATCH] object cache: optimize push phase of dirty objects

[MORITA Kazutaka]

Yuan,

This patch looks correct, but I've added some comments which I'd like
you to address from the point of view of our patch contribution
guideline.  You are the maintainer, so you (and I) should strictly
follow our rules so that we can be a guide for other developers.

> 
> From: Liu Yuan <tailai.ly at taobao.com>
> 
> - Don't grab cache lock tight so we can serve RW requests while pushing.
>   It is okay for allow subsequent RW after FLUSH because we only need to
>   garantee the dirty objects before FLUSH to be pushed.
> - Use threaded AIO to hugely boost push performance, such as fsync(2) from VM.
> - Fix a nasty long bug that didn't destory pthread rwlock after VDI deletion.
> - Clean up {get,put}_cache_entry helpers and introduce lock helpers.
> - Prefix work queues of object cache as 'oc_'

Could you please split this patch into smaller ones?  It will make
this patch much easier to review, and in future, it'll be easier to
debug with bisect when this patch has problem.

> @@ -60,15 +61,18 @@ struct object_cache_entry {
>  	struct rb_node node;
>  	struct list_head dirty_list;
>  	struct list_head lru_list;
> +
> +	pthread_rwlock_t lock;
>  };
>  
>  struct object_cache {
>  	uint32_t vid;
> +	uint32_t push_count;
>  	struct hlist_node hash;
> -
>  	struct rb_root lru_tree;
>  	struct list_head lru_head; /* Per VDI LRU list for reclaimer */
>  	struct list_head dirty_head;
> +	int push_efd;
>  
>  	pthread_rwlock_t lock;
>  };

Please add comments to fields you newly added.  I can understand them,
but it will help newbies.

> @@ -158,7 +219,7 @@ lru_tree_insert(struct rb_root *root, struct object_cache_entry *new)
>  }
>  
>  static struct object_cache_entry *lru_tree_search(struct rb_root *root,
> -						     uint32_t idx)
> +						  uint32_t idx)

This patch contains many conding style fixes like this.  Please split
them to another patch.

> -static int write_cache_object(struct object_cache_entry *entry, void *buf,
> -			      size_t count, off_t offset, bool create,
> -			      bool writeback)
> -{

Any reason you've moved this function?  It makes this patch complex.

> @@ -505,18 +567,16 @@ static void reclaim_done(struct work *work)
>  static int create_dir_for(uint32_t vid)
>  {
>  	int ret = 0;
> -	struct strbuf buf = STRBUF_INIT;
> +	char p[PATH_MAX];
>  
> -	strbuf_addstr(&buf, object_cache_dir);
> -	strbuf_addf(&buf, "/%06"PRIx32, vid);
> -	if (mkdir(buf.buf, def_dmode) < 0)
> +	sprintf(p, "%s/%06"PRIx32, object_cache_dir, vid);

Although you fix this in the succeeding patch, I think there is no
reason we need to use sprintf here now.  Please fix it if it's not
trouble for you to do it.

> diff --git a/sheep/sheep.c b/sheep/sheep.c
> index 61e166c..58b93f8 100644
> --- a/sheep/sheep.c
> +++ b/sheep/sheep.c
> @@ -374,8 +374,9 @@ static int init_work_queues(void)
>  	sys->block_wqueue = init_work_queue("block", true);
>  	sys->sockfd_wqueue = init_work_queue("sockfd", true);
>  	if (is_object_cache_enabled()) {
> -		sys->reclaim_wqueue = init_work_queue("reclaim", true);
> -		if (!sys->reclaim_wqueue)
> +		sys->oc_reclaim_wqueue = init_work_queue("reclaim", true);
> +		sys->oc_push_wqueue = init_work_queue("push", false);
> +		if (!sys->oc_reclaim_wqueue || !sys->oc_push_wqueue)
>  			return -1;

I think thread names (the first arguments of init_work_queue) also
should be modified.

Thanks,

Kazutaka