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

[Liu Yuan]

On 01/26/2013 05:23 PM, MORITA Kazutaka wrote:
>> +
>> +/*
>> + * 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
> 

I'll drop this patch set because background pusher doesn't help boost
performance but more complicate the code.

Thanks,
Yuan