[sheepdog] [PATCH 1/2] object cache: introduce background pusher to boost flush performance

[MORITA Kazutaka]

> +
> +/*
> + * Try to push dirty objects in the background to boost VM flush request such as
> + * fsync().
> + *
> + * This doesn't voliate the flush semantics because the flush request will
> + * still be served synchronously and guaranteed to drain all the dirty objects.
> + */
> +static void do_async_push(struct work *work)
> +{
> +	struct push_work *pw = container_of(work, struct push_work, work);
> +	struct object_cache *oc = pw->oc;
> +	struct object_cache_entry *entry;
> +
> +	sd_dprintf("%"PRIx32"\n", oc->vid);
> +next:
> +	pthread_rwlock_wrlock(&oc->lock);
> +	if (list_empty(&oc->dirty_head)) {
> +	    pthread_rwlock_unlock(&oc->lock);
> +	    return;
> +	}
> +	entry = list_first_entry(&oc->dirty_head, typeof(*entry), dirty_list);
> +	uatomic_inc(&entry->refcnt);
> +	uatomic_dec(&oc->dirty_count);
> +	pthread_rwlock_unlock(&oc->lock);
> +
> +	pthread_rwlock_rdlock(&entry->lock);
> +
> +	assert(push_cache_object(oc->vid, entry_idx(entry), entry->bmap,
> +				 !!(entry->idx & CACHE_CREATE_BIT))
> +	       == SD_RES_SUCCESS);

We must not call functions with side effects inside assert.  The code
does not compiled if we specify -NDEBUG.


>  /* Push back all the dirty objects to sheep cluster storage */
> -static int object_cache_push(struct object_cache *oc)
> +static int object_cache_sync_push(struct object_cache *oc)
>  {
>  	struct object_cache_entry *entry, *t;
> -
>  	int ret = SD_RES_SUCCESS;
>  
>  	pthread_rwlock_wrlock(&oc->lock);
>  	list_for_each_entry_safe(entry, t, &oc->dirty_head, dirty_list) {
> -		ret = push_cache_object(oc->vid, entry_idx(entry), entry->bmap,
> -					!!(entry->idx & CACHE_CREATE_BIT));
> -		if (ret != SD_RES_SUCCESS)
> -			goto push_failed;
> +		if (uatomic_read(&entry->refcnt) > 0)
> +			/* async pusher has been handling it */
> +			continue;
> +
> +		assert(push_cache_object(oc->vid, entry_idx(entry), entry->bmap,
> +					!!(entry->idx & CACHE_CREATE_BIT))
> +		       == SD_RES_SUCCESS);

Same here.

> diff --git a/sheep/sheep.c b/sheep/sheep.c
> index 61e166c..06ed685 100644
> --- a/sheep/sheep.c
> +++ b/sheep/sheep.c
> @@ -375,7 +375,8 @@ static int init_work_queues(void)
>  	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->push_wqueue = init_work_queue("push", false);

Shouldn't we use a more descriptive name?  We now have many work
queues and it is becoming harder to understand each role.

Thanks,

Kazutaka