Return-Path: linux-nfs-owner@vger.kernel.org Received: from mx11.netapp.com ([216.240.18.76]:16010 "EHLO mx11.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753785AbaHYORa (ORCPT ); Mon, 25 Aug 2014 10:17:30 -0400 Message-ID: <53FB4574.9090205@Netapp.com> Date: Mon, 25 Aug 2014 10:17:24 -0400 From: Anna Schumaker MIME-Version: 1.0 To: Christoph Hellwig CC: Subject: Re: [PATCH 08/19] pnfs: add return_range method References: <1408637375-11343-1-git-send-email-hch@lst.de> <1408637375-11343-9-git-send-email-hch@lst.de> <53FB3F2A.409@Netapp.com> <20140825140952.GA13911@lst.de> In-Reply-To: <20140825140952.GA13911@lst.de> Content-Type: text/plain; charset="utf-8" Sender: linux-nfs-owner@vger.kernel.org List-ID: 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: > ... >