[pnfs] [PATCH 19/37] pnfs: client layout cache: allow waiting for alloc_layout

[Dean Hildebrand]

I think we need to talk about all these waits and locks on the conf 
call.   This code has almost no comments, so understanding their purpose 
is really difficult.

There seems to be multiple nested locks and waits, with the locks and unlocks happening in different functions.  This could easily lead to deadlocks, maybe not by you, but future maintainers will have a heck of a time ensuring they don't accidentally change things so a deadlock occurs. 


My personal view is that each function should grab and release its locks 
at the same level in the call hierarchy.  For example,  
pnfs_update_layout grabs locks in lower level function calls 
(get_alloc_layout), and then goes on to release and reacquire these 
locks at the high level.  This is bad.   Not only that, but it locks 
nfsi->lo_lock in the get_lock_current_layout function but also in random 
other parts of the code (end of get_alloc_layout).  At the minimum, a 
single helper function should lock and unlock lo_lock.

goto's have their purpose, but C has lots of constructs that allow code 
simplification at the expense of readability.  Programming style in 1974 
(the year of that paper) applies little to programming in 2007.  For 
example, recursion is useful and cool, but most programmers these days 
will not understand what it is doing.  And even for those who do 
understand recursion, it will take longer to review the code.  Let's 
just stick with regular loops and not sacrifice readability.

more below.

> Well, the allocation function might block so a spinlock is inappropriate,
> a mutex could have been used instead.  
Do you mean the layout driver will block? Why?
> However, on a different thread,
> Trond expressed his desire to get rid of locks where possible and use
> waits instead.
waits are ok, but pnfs_inject_layout, get_lock_current_layout, 
put_lock_current_layout, get_alloc_layout, pnfs_update_layout, 
pnfs_getboundary,  seem to mix waits with locks in scattered places.  I 
think these are the functions that need to be reorg'd.

>  The idea here is to solve the potential race between
> multiple threads wanting to allocate nfsi->current_layout while
> allowing signals to interrupt and wake up any waiting thread.
>   
I like the idea, we just need to get the code to do this in an 
appropriate way.  Would it be possible to re-org the code so that
get_lock_current_layout, put_lock_current_layout are always called in 
the same function?  In addition, get_lock_current_layout, 
put_lock_current_layout would be the only 2 functions locking the 
nfsi->lo_lock? 
Dean


>   
>> 	res = wait_on_bit_lock(&nfsi->pnfs_layout_state, NFS_INO_LAYOUT_ALLOC,
>> 			       pnfs_wait_schedule, TASK_INTERRUPTIBLE);
>>
>> 	if (res) {
>> 		lo = ERR_PTR(res);
>> 		goto err;
>> 	}
>>
>> 	if (nfsi->current_layout) {
>> 		clear_bit_unlock(NFS_INO_LAYOUT_ALLOC,
>> 				 &nfsi->pnfs_layout_state);
>> 		wake_up_bit(&nfsi->pnfs_layout_state, NFS_INO_LAYOUT_ALLOC);
>>
>> 		goto out;
>> 	}
>>
>>  	lo = nfsi->current_layout = alloc_init_layout(ino, io_ops);
>> 	clear_bit_unlock(NFS_INO_LAYOUT_ALLOC, &nfsi->pnfs_layout_state);
>> 	wake_up_bit(&nfsi->pnfs_layout_state, NFS_INO_LAYOUT_ALLOC);
>>     
>
> This patch represents an intermediate version of this function before the
> introduction of {get_lock,put_unlock}_current_layout
> (see http://git.linux-nfs.org/?p=bhalevy/linux-pnfs.git;a=commitdiff;h=5220589afa99bdc8f5301e26db8d5c5870c285c8)
> I hope the backward goto makes more sense over there when an extra step (atomic locking 
> and referencing of nfsi->current_layout) is needed.
> This can be changed to an infinite loop with explicit continue and break points
> like the following code
> (if it looks simpler, I'm not sure... see http://pplab.snu.ac.kr/courses/adv_pl05/papers/p261-knuth.pdf)
>
> static struct pnfs_layout_type *
> get_alloc_layout(struct inode *ino,
> 		   struct layoutdriver_io_operations *io_ops)
> {
> 	struct nfs_inode *nfsi = NFS_I(ino);
> 	struct pnfs_layout_type *lo;
> 	int res;
>
> 	dprintk("%s Begin\n", __FUNCTION__);
>
>   
> 	while ((lo = get_lock_current_layout(nfsi)) == NULL) {
> 		/* Compete against other threads on who's doing the allocation,
> 		 * wait until bit is cleared if we lost this race.
> 		 */
> 		res = wait_on_bit_lock(&nfsi->pnfs_layout_state, NFS_INO_LAYOUT_ALLOC,
> 				       pnfs_wait_schedule, TASK_INTERRUPTIBLE);
> 		if (res) {
> 			lo = ERR_PTR(res);
> 			goto err;
> 		}
>
> 		/* Was current_layout already allocated while we slept?
> 		 * If so, try to get it again using get_lock_current_layout.
> 		 */
> 		if (nfsi->current_layout) {
> 			clear_bit_unlock(NFS_INO_LAYOUT_ALLOC,
> 					 &nfsi->pnfs_layout_state);
> 			continue;
>   
                                                     ^^^^^^^^^^

> 		}
>
> 		/* Try to allocate current_layout and grab the lo_lock */
> 		lo = nfsi->current_layout = alloc_init_layout(ino, io_ops);
> 		spin_lock(&nfsi->lo_lock);
>
> 		/* release the NFS_INO_LAYOUT_ALLOC bit and wake up any waiter */
> 		clear_bit_unlock(NFS_INO_LAYOUT_ALLOC, &nfsi->pnfs_layout_state);
> 		wake_up_bit(&nfsi->pnfs_layout_state, NFS_INO_LAYOUT_ALLOC);
>
> 		/* we're done here. just check whether alloc_init_layout succeeded. */
> 		if (lo)
> 			break;
>
> 		spin_unlock(&nfsi->lo_lock);
> 		lo = ERR_PTR(-ENOMEM);
> 		goto err;
> 	}
>
> 	dprintk("%s Return %p\n", __FUNCTION__, lo);
> 	return lo;
>
> err:
> 	dprintk("%s Return error %ld\n", __FUNCTION__, PTR_ERR(lo));
> 	return lo;
> }
>
>   
>> Dean
>>     
>
>