From mboxrd@z Thu Jan 1 00:00:00 1970 From: "J. Bruce Fields" Subject: Re: [PATCH 6/6] nfs: disable leases over NFS Date: Thu, 5 Jul 2007 11:41:00 -0400 Message-ID: <20070705154100.GC29867@fieldses.org> References: <6e0beaf3e950494a6903571f0b5c9b61fc7bf650.1183143819.git.bfields@citi.umich.edu> <20070630092516.GD22050@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Andrew Morton , linux-fsdevel@vger.kernel.org, Peter Staubach , Trond Myklebust To: Christoph Hellwig Return-path: Received: from mail.fieldses.org ([66.93.2.214]:58716 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758032AbXGEPlI (ORCPT ); Thu, 5 Jul 2007 11:41:08 -0400 Content-Disposition: inline In-Reply-To: <20070630092516.GD22050@infradead.org> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Sat, Jun 30, 2007 at 10:25:16AM +0100, Christoph Hellwig wrote: > On Fri, Jun 29, 2007 at 03:21:30PM -0400, J. Bruce Fields wrote: > > Define an nfs setlease method that just returns -EOPNOTSUPP. > > > > If someone can demonstrate a real need, perhaps we could reenable > > them in the presence of the "nolock" mount option. > > I'm not a big fan of default methods that do the wrong thing instead > of just missing functionality. Would you mind just returning > -EOPNOTSUPP if ->setlease is not implemented and add it to all > the local filesystems while all the network/distributed filesystems > should not have it, not just nfs. OK, after looking at this a little more, I'm less happy about the idea of erroring out by default: - There are a ton of filesystems that probably should allow leases, and only a few (network filesystems) that shouldn't, so leaving leases on by default seems simpler. - We already fall back on the local method by default in the case of locks, and I don't see a reason to treat leases differently. - The patch to add .setlease = setlease, to all the file_operations is going to be a big patch that changes behavior in a way that might be easy to miss (because it changes behavior exactly on those filesystems it *doesn't* touch.) I think it'll be easier to get better review on the patch that adds a method just to those filesystems that we're disabling leases for. I agree about dealing with the other network filesystems, though, and not just NFS and GFS2; I'll look into that. --b.