[pnfs] [PATCH 21/37] pnfs: client layout cache: unlock layout around call to get_layout

[Benny Halevy]

On Jan. 03, 2008, 1:51 +0200, Dean Hildebrand <seattleplus at gmail.com> wrote:
> 
> Benny Halevy wrote:
>> get_layout sends an rpc to the server so the layout spinlock should be
>> released around the call.
>>
>> Signed-off-by: Benny Halevy <bhalevy at panasas.com>
>> ---
>>  fs/nfs/pnfs.c |   19 ++++++++++++-------
>>  1 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index d11145c..edc087c 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -578,8 +578,6 @@ pnfs_update_layout(struct inode *ino,
>>  		return 0; /* Already have layout information */
>>  	}
>>  
>> -	res.layout.buf = NULL;
>> -
>>  	/* if get layout already failed once goto out */
>>  	if (nfsi->pnfs_layout_state & NFS_INO_LAYOUT_FAILED) {
>>  		if (unlikely(nfsi->pnfs_layout_suspend &&
>> @@ -593,7 +591,14 @@ pnfs_update_layout(struct inode *ino,
>>  		}
>>  	}
>>  
>> +	res.layout.buf = NULL;
>> +	spin_unlock(&nfsi->lo_lock);
>>  	result = get_layout(ino, ctx, &arg, &res);
>> +	spin_lock(&nfsi->lo_lock);
>> +
>> +	/* we got a reference on current_layout */
>> +	BUG_ON(nfsi->current_layout != layout_new);
>> +
>>   
> Couldn't something have happened while the lock was released?  Does this 
> need to be checked?  If not, maybe some comments could explain the 
> situation.

I hoped that the comment I added explains enough, but you proved me wrong :)

Since we got a reference on the current_layout it must never ever be released
or change (in the current implementation) even when nfsi->lo_lock is being unlocked
therefore the BUG_ON must never happened.  If it does trigger it means that
something nasty and insane happened which violated the design and we're not
playing a fair game anymore :)

How about the following, extended comment?
/* we got a reference on nfsi->current_layout hence it must never change
 * even while nfsi->lo_lock was not held.
 */

> Dean