From: Chuck Lever Subject: Re: [PATCH 1/5] NFS: combine nfs_kill_super() and nfs4_kill_super() Date: Sun, 30 Aug 2009 13:04:06 -0400 Message-ID: <948B2DB2-411B-4054-9851-2339E625901E@oracle.com> References: <20090830162211.3652.14638.stgit@matisse.1015granger.net> <20090830163432.3652.437.stgit@matisse.1015granger.net> <20090830164229.GA32025@infradead.org> <1251651593.12486.14.camel@heimdal.trondhjem.org> Mime-Version: 1.0 (Apple Message framework v936) Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes Cc: Christoph Hellwig , linux-nfs@vger.kernel.org To: Trond Myklebust Return-path: Received: from rcsinet12.oracle.com ([148.87.113.124]:62453 "EHLO rgminet12.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753579AbZH3REW (ORCPT ); Sun, 30 Aug 2009 13:04:22 -0400 In-Reply-To: <1251651593.12486.14.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Aug 30, 2009, at 12:59 PM, Trond Myklebust wrote: > On Sun, 2009-08-30 at 12:42 -0400, Christoph Hellwig wrote: >> On Sun, Aug 30, 2009 at 12:34:33PM -0400, Chuck Lever wrote: >>> +static void nfs_kill_super(struct super_block *sb) >>> { >>> + struct nfs_server *server = NFS_SB(sb); >>> + >>> + dprintk("--> %s\n", __func__); >>> + >>> +#ifdef CONFIG_NFS_V4 >>> + if (server->nfs_client->rpc_ops->version == 4) { >>> + nfs_super_return_all_delegations(sb); >>> + nfs4_renewd_prepare_shutdown(server); >>> + } >>> +#endif /* CONFIG_NFS_V4 */ >>> >>> bdi_unregister(&server->backing_dev_info); >> >> This was previously not done for nfs4. If it is a bug-fixed that >> should be documented in the patch description, and if not it needs >> to be changed. > > It has always been done, but it was in a separate function > (nfs4_kill_super()). I don't really see what we gain by inlining it > into > nfs_kill_super.. Actually nfs4_kill_super didn't unregister the bdi. > Also, I'm concerned about the growth of "if (version == X)" type > constructs. We shouldn't be looking at the version number in order to > figure out whether or not we're holding delegations, or whether or > not a > particular daemon thread is running. > AFAICS, in this case it should in any case be safe to call > nfs_super_return_all_delegations() (as long as CONFIG_NFS_V4 is > defined > - that we might want to fix). Ditto for > nfs4_renewd_prepare_shutdown(). That's all fine, but as I said in the cover letter, this patch may be dropped since the file system is still nfs4 under the covers, so I think nfs4_kill_super would be invoked anyway for "-t nfs -o vers=4". > > Cheers > Trond > -- Chuck Lever chuck[dot]lever[at]oracle[dot]com