[pnfs] [PATCH 10/10] pnfs: Enable O_DIRECT write path.

[Dean Hildebrand]

Benny b wrote:
> On Jul. 09, 2008, 0:54 +0300, Dean Hildebrand <seattleplus at gmail.com> wrote:
>   
>> Signed-off-by: Dean Hildebrand <dhildeb at us.ibm.com>
>> ---
>>  fs/nfs/direct.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 42 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
>> index a397a9c..fe291ad 100644
>> --- a/fs/nfs/direct.c
>> +++ b/fs/nfs/direct.c
>> @@ -515,6 +515,7 @@ static void nfs_direct_write_reschedule(struct nfs_direct_req *dreq)
>>  		.workqueue = nfsiod_workqueue,
>>  		.flags = RPC_TASK_ASYNC,
>>  	};
>> +	int result;
>>  
>>  	dreq->count = 0;
>>  	get_dreq(dreq);
>> @@ -538,6 +539,13 @@ static void nfs_direct_write_reschedule(struct nfs_direct_req *dreq)
>>  		 * Reuse data->task; data->args should not have changed
>>  		 * since the original request was sent.
>>  		 */
>> +		result = pnfs_try_to_write_data(data, &nfs_write_direct_ops,
>> +						NFS_FILE_SYNC);
>> +#if defined(CONFIG_PNFS)
>> +		if (result == PNFS_ATTEMPTED && !data->pdata.pnfs_error)
>> +			continue;
>> +#endif /* CONFIG_PNFS */
>> +
>>  		nfs_direct_write_execute(data, &task_setup_data, &msg);
>>  	}
>>  
>> @@ -617,6 +625,7 @@ static void nfs_direct_commit_schedule(struct nfs_direct_req *dreq)
>>  		.workqueue = nfsiod_workqueue,
>>  		.flags = RPC_TASK_ASYNC,
>>  	};
>> +	int result;
>>  
>>  	data->inode = dreq->inode;
>>  	data->cred = msg.rpc_cred;
>> @@ -629,6 +638,12 @@ static void nfs_direct_commit_schedule(struct nfs_direct_req *dreq)
>>  	data->res.fattr = &data->fattr;
>>  	data->res.verf = &data->verf;
>>  
>> +	result = pnfs_try_to_commit(data, &nfs_commit_direct_ops,
>> +				    RPC_TASK_ASYNC);
>> +	dprintk("%s: pnfs commit result %d\n", __func__, result);
>> +	if (result <= PNFS_ATTEMPTED)
>>     
>
> I don't like the fact that some pnfs_try_to methods may return
> an (negative) error status and some may not.  If the return type
> is enum pnfs_try_status then that should be it.  In this case,
> pnfs_try_to_commit should just return PNFS_ATTEMPTED and the
> error status, if needed, can be extracted from data->pdata.pnfs_error;
>   
Yes, I totally agree.  I guess we all need to agree on how ifdef and the 
try_to_xxx functions relate (from earlier email).  Maybe we can talk 
about this tomorrow.
> Benny
>
>   
>> +		return;
>> +
>>  	nfs_direct_commit_execute(dreq, data, &task_setup_data, &msg);
>>  }
>>  
>> @@ -677,6 +692,9 @@ static void nfs_direct_write_result(struct rpc_task *task, void *calldata)
>>  {
>>  	struct nfs_write_data *data = calldata;
>>  
>> +	dprintk("%s: verf: %d stable %d\n", __func__,
>> +		data->res.verf->committed, data->args.stable);
>> +
>>  	if (nfs_writeback_done(task, data) != 0)
>>  		return;
>>  }
>> @@ -794,6 +812,16 @@ static ssize_t nfs_direct_write_schedule_segment(struct nfs_direct_req *dreq,
>>  	unsigned int pgbase;
>>  	int result;
>>  	ssize_t started = 0;
>> +	size_t pnfs_stripe_rem = count;
>> +
>> +	/* pnfs_stripe_rem will be set to the remaining bytes in
>> +	 * the first stripe_unit (which for standard nfs is count)
>> +	 */
>> +	pnfs_direct_init_io(inode, ctx, count, pos, 1,
>> +			    &wsize, &pnfs_stripe_rem);
>> +
>> +	dprintk("%s: pos %llu count %Zu wsize %Zu\n",
>> +		__func__, pos, count, wsize);
>>  
>>  	do {
>>  		struct nfs_write_data *data;
>> @@ -802,6 +830,10 @@ static ssize_t nfs_direct_write_schedule_segment(struct nfs_direct_req *dreq,
>>  		pgbase = user_addr & ~PAGE_MASK;
>>  		bytes = min(wsize,count);
>>  
>> +#if defined(CONFIG_PNFS)
>> +		bytes = min(bytes, pnfs_stripe_rem);
>> +		pnfs_stripe_rem = wsize;
>> +#endif /* CONFIG_PNFS */
>>  		result = -ENOMEM;
>>  		data = nfs_writedata_alloc(nfs_page_array_len(pgbase, bytes));
>>  		if (unlikely(!data))
>> @@ -844,7 +876,16 @@ static ssize_t nfs_direct_write_schedule_segment(struct nfs_direct_req *dreq,
>>  		data->res.count = bytes;
>>  		data->res.verf = &data->verf;
>>  
>> -		if (nfs_direct_write_execute(data, &task_setup_data, &msg))
>> +		result = pnfs_try_to_write_data(data, &nfs_write_direct_ops,
>> +						sync);
>> +#if defined(CONFIG_PNFS)
>> +		if (result == PNFS_ATTEMPTED && data->pdata.pnfs_error) {
>> +			result = data->pdata.pnfs_error;
>>     
>
> I'm not sure if this is the right thing to do since in the non-pnfs
> path the error status is not returned.
>
> Benny
>
>   
>> +			break;
>> +		}
>> +#endif /* CONFIG_PNFS */
>> +		if (result == PNFS_NOT_ATTEMPTED &&
>> +		    nfs_direct_write_execute(data, &task_setup_data, &msg))
>>  			break;
>>  
>>  		started += bytes;
>>     
>
>