Re: [PATCH 08/19] pnfs: add return_range method

From: Anna Schumaker <Anna.Schumaker@netapp.com>
To: Christoph Hellwig <hch@lst.de>
Cc: <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 08/19] pnfs: add return_range method
Date: Mon, 25 Aug 2014 10:17:24 -0400	[thread overview]
Message-ID: <53FB4574.9090205@Netapp.com> (raw)
In-Reply-To: <20140825140952.GA13911@lst.de>

On 08/25/2014 10:09 AM, Christoph Hellwig wrote:
> On Mon, Aug 25, 2014 at 09:50:34AM -0400, Anna Schumaker wrote:
>>> +	} else {
>>> +		if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) {
>>> +			NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo,
>>> +				&args->cbl_range);
>>> +		}
>>>  	}
>> Is there a reason you're nesting the else-if here?
> 
> To catch the intent - the first two clauses find excuses why we can't return
> quite yet, while this if is for an optional feature in the actual return
> path. If I wasn't updating but newly writing the function I'd actually
> do something like:

I'm a fan of nice looking code, and I like what you have below better.  Can you arrange things to end up in this state?  Or maybe send a cleanup patch after?

Anna

> 
> 	...
> 
> 	rv = NFS4ERR_DELAY;
> 	if (test_bit(NFS_LAYOUT_BULK_RECALL, &lo->plh_flags))
> 		goto out_set_stateid;
> 
> 	if (pnfs_mark_matching_lsegs_invalid(lo, &free_me_list,
> 			&args->cbl_range)) {
> 		need_commit = true;
> 		goto out_set_stateid;
> 	}
> 
> 	rv = NFS4ERR_NOMATCHING_LAYOUT;
> 	if (NFS_SERVER(ino)->pnfs_curr_ld->return_range) {
> 		NFS_SERVER(ino)->pnfs_curr_ld->return_range(lo,
> 				&args->cbl_range);
> 	}
> 
> out_set_stateid:
> 	...
> 