[sheepdog] [PATCH] use queue_request when resume waiting requests

[levin li]

On 05/30/2012 04:45 PM, Christoph Hellwig wrote:
> Give me a bit more time for a real review and testing, but here's
> a simple comment from a quick look over the code:
> 
>>  		list_del(&req->request_list);
>> +		put_vnode_info(req->vnodes);
>> +		queue_request(req);
> 

It's OK for me to add a helper function.

> This pattern happens so often that at least a requeue_request() helper
> bounding these three calls would help.
> 
>> @@ -313,10 +312,14 @@ void resume_wait_epoch_requests(void)
>>  			put_vnode_info(req->vnodes);
>>  			req->vnodes = get_vnode_info();
>>  			setup_access_to_local_objects(req);
>> +			list_del(&req->request_list);
>> +			process_io_request(req);
> 
> This also seems prettry equivalent to the (re)queue_request sequence.
> 

Here it's different from requeue_request(), we did not queue the request again,
but directly process it.

>>  			request_list) {
>> +		if (req->local_oid != oid)
>> +			continue;
>> +
>>  		/* the object requested by a pending request has been
>>  		 * recovered, notify the pending request. */
>> -		if (req->local_oid == oid) {
>> -			dprintf("retry %" PRIx64 "\n", req->local_oid);
>> -			list_del(&req->request_list);
>> -			process_io_request(req);
>> -		}
>> +		dprintf("retry %" PRIx64 "\n", req->local_oid);
>> +		list_del(&req->request_list);
>> +		put_vnode_info(req->vnodes);
>> +		queue_request(req);
>>  	}
> 
> I think all these need the splite to a local list, then process pattern.
> Without that we could add the request to the list currently walked and
> create a busy loop.
> 

Well, It's indeed a problem here, thanks for pointing it out, I'd send out
another version.

thanks,

levin