Re: [PATCH 2/5] SQUASHME: pnfs_submit: move pnfs_layout_state back to nfs_inode

[Boaz Harrosh <bharrosh@panasas.com>]

On 06/30/2010 12:05 PM, Boaz Harrosh wrote:
> On 06/29/2010 07:42 PM, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Prepare to embed struct pnfs_layout_tupe into layout driver private structs
>> and to make layout a pointer: state flags need to be accessible before
>> allocation.
>>
> 
> I don't get that. A pointer inspection operation is atomic. (gone are the
> days of 16-bit large/huge models where a pointer is two registers.)
> 
> So an: if (lo->pointer) is just as atomic as test_bit()
> 
> And I don't see, not here, and not in the next patch, where are these used before
> the allocation. if you throw away Fred's first patch and make:
>  layoutcommit_needed(struct nfs_inode *nfsi)
>  {
> -	return nfsi->layout.lo_cred != NULL;
> +	return nfsi->layout != NULL;
>  }
> 

Rrrr need my morning coffee, sorry. 

But still, Fred's patch can go both sites of the pointer assignment have an
MB and a pointer inspection is there-for atomic. Above can then be:

 layoutcommit_needed(struct nfs_inode *nfsi)
 {
-	return nfsi->layout.lo_cred != NULL;
+	return nfsi->layout && (nfsi->layout->lo_cred != NULL);
 }


> Then all three patches (first one thrown away) can just collapse to a single
> embedded-to-pointer conversion.
> 

This still applies

Boaz
> Boaz
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>>  fs/nfs/inode.c         |    2 +-
>>  fs/nfs/pnfs.c          |   16 ++++++++--------
>>  include/linux/nfs_fs.h |   10 +++++-----
>>  3 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>> index 7989dea..66d7be2 100644
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -1364,7 +1364,6 @@ void nfs4_clear_inode(struct inode *inode)
>>  static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
>>  {
>>  #ifdef CONFIG_NFS_V4_1
>> -	nfsi->layout.pnfs_layout_state = 0;
>>  	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
>>  	nfsi->layout.roc_iomode = 0;
>>  	nfsi->layout.lo_cred = NULL;
>> @@ -1420,6 +1419,7 @@ static void pnfs_init_once(struct nfs_inode *nfsi)
>>  {
>>  #ifdef CONFIG_NFS_V4_1
>>  	init_waitqueue_head(&nfsi->lo_waitq);
>> +	nfsi->pnfs_layout_state = 0;
>>  	seqlock_init(&nfsi->layout.seqlock);
>>  	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
>>  	INIT_LIST_HEAD(&nfsi->layout.segs);
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index bf15b5c..d05cb03 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -951,7 +951,7 @@ get_lock_alloc_layout(struct inode *ino)
>>  		 * wait until bit is cleared if we lost this race.
>>  		 */
>>  		res = wait_on_bit_lock(
>> -			&nfsi->layout.pnfs_layout_state,
>> +			&nfsi->pnfs_layout_state,
>>  			NFS_INO_LAYOUT_ALLOC,
>>  			pnfs_wait_schedule, TASK_KILLABLE);
>>  		if (res) {
>> @@ -975,8 +975,8 @@ get_lock_alloc_layout(struct inode *ino)
>>  
>>  		/* release the NFS_INO_LAYOUT_ALLOC bit and wake up waiters */
>>  		clear_bit_unlock(NFS_INO_LAYOUT_ALLOC,
>> -				 &nfsi->layout.pnfs_layout_state);
>> -		wake_up_bit(&nfsi->layout.pnfs_layout_state,
>> +				 &nfsi->pnfs_layout_state);
>> +		wake_up_bit(&nfsi->pnfs_layout_state,
>>  			    NFS_INO_LAYOUT_ALLOC);
>>  		break;
>>  	}
>> @@ -1104,12 +1104,12 @@ _pnfs_update_layout(struct inode *ino,
>>  	}
>>  
>>  	/* if get layout already failed once goto out */
>> -	if (test_bit(lo_fail_bit(iomode), &nfsi->layout.pnfs_layout_state)) {
>> +	if (test_bit(lo_fail_bit(iomode), &nfsi->pnfs_layout_state)) {
>>  		if (unlikely(nfsi->layout.pnfs_layout_suspend &&
>>  		    get_seconds() >= nfsi->layout.pnfs_layout_suspend)) {
>>  			dprintk("%s: layout_get resumed\n", __func__);
>>  			clear_bit(lo_fail_bit(iomode),
>> -				  &nfsi->layout.pnfs_layout_state);
>> +				  &nfsi->pnfs_layout_state);
>>  			nfsi->layout.pnfs_layout_suspend = 0;
>>  		} else
>>  			goto out_put;
>> @@ -1121,7 +1121,7 @@ _pnfs_update_layout(struct inode *ino,
>>  	send_layoutget(ino, ctx, &arg, lsegpp, lo);
>>  out:
>>  	dprintk("%s end, state 0x%lx lseg %p\n", __func__,
>> -		nfsi->layout.pnfs_layout_state, lseg);
>> +		nfsi->pnfs_layout_state, lseg);
>>  	return;
>>  out_put:
>>  	if (lsegpp)
>> @@ -1226,12 +1226,12 @@ pnfs_get_layout_done(struct nfs4_pnfs_layoutget *lgp, int rpc_status)
>>  	/* remember that get layout failed and suspend trying */
>>  	nfsi->layout.pnfs_layout_suspend = suspend;
>>  	set_bit(lo_fail_bit(lgp->args.lseg.iomode),
>> -		&nfsi->layout.pnfs_layout_state);
>> +		&nfsi->pnfs_layout_state);
>>  	dprintk("%s: layout_get suspended until %ld\n",
>>  		__func__, suspend);
>>  out:
>>  	dprintk("%s end (err:%d) state 0x%lx lseg %p\n",
>> -		__func__, lgp->status, nfsi->layout.pnfs_layout_state, lseg);
>> +		__func__, lgp->status, nfsi->pnfs_layout_state, lseg);
>>  	return;
>>  }
>>  
>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>> index a33e86e..e216e24 100644
>> --- a/include/linux/nfs_fs.h
>> +++ b/include/linux/nfs_fs.h
>> @@ -105,11 +105,6 @@ struct pnfs_layout_type {
>>  	seqlock_t seqlock;		/* Protects the stateid */
>>  	nfs4_stateid stateid;
>>  	void *ld_data;			/* layout driver private data */
>> -	unsigned long pnfs_layout_state;
>> -	#define NFS_INO_RO_LAYOUT_FAILED 0      /* get ro layout failed stop trying */
>> -	#define NFS_INO_RW_LAYOUT_FAILED 1      /* get rw layout failed stop trying */
>> -	#define NFS_INO_LAYOUT_ALLOC     2      /* bit lock for layout allocation */
>> -	#define NFS_INO_LAYOUTCOMMIT     3	/* LAYOUTCOMMIT needed */
>>  	time_t pnfs_layout_suspend;
>>  	struct rpc_cred         *lo_cred; /* layoutcommit credential */
>>  	/* DH: These vars keep track of the maximum write range
>> @@ -208,6 +203,11 @@ struct nfs_inode {
>>  #if defined(CONFIG_NFS_V4_1)
>>  	wait_queue_head_t lo_waitq;
>>  	struct pnfs_layout_type layout;
>> +	unsigned long pnfs_layout_state;
>> +	#define NFS_INO_RO_LAYOUT_FAILED 0 /* ro LAYOUTGET failed stop trying */
>> +	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop trying */
>> +	#define NFS_INO_LAYOUT_ALLOC     2 /* bit lock for layout allocation */
>> +	#define NFS_INO_LAYOUTCOMMIT     3 /* LAYOUTCOMMIT needed */
>>  #endif /* CONFIG_NFS_V4_1 */
>>  #endif /* CONFIG_NFS_V4*/
>>  #ifdef CONFIG_NFS_FSCACHE
>