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

[Benny Halevy]

On Jan. 03, 2008, 1:54 +0200, Dean Hildebrand <seattleplus at gmail.com> wrote:
>>  /*
>>   * get, possibly allocate current_layout
>>   */
>> @@ -442,14 +450,32 @@ get_alloc_layout(struct inode *ino,
>>  {
>>  	struct nfs_inode *nfsi = NFS_I(ino);
>>  	struct pnfs_layout_type *lo;
>> +	int res;
>>  
>>  	dprintk("%s Begin\n", __FUNCTION__);
>>  
>> +retry:
>>  	lo = nfsi->current_layout;
>>  	if (lo)
>>  		goto out;
>>  
>> +	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);
>> +		goto retry;
>> +	}
>> +
>>  	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 function seems a little muddled (or maybe some comments would 
> help).  I've never been a fan of backward gotos.  Would something like 
> the following be any less correct or inefficient?  Actually, if the 
> following code works, could we just use a spinlock instead?  Are we just 
> protecting nfsi->current_layout?

Well, the allocation function might block so a spinlock is inappropriate,
a mutex could have been used instead.  However, on a different thread,
Trond expressed his desire to get rid of locks where possible and use
waits instead. 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.

> 
> 	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