From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: linux-nfs-owner@vger.kernel.org Received: from mx1.redhat.com ([209.132.183.28]:7834 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754600Ab3GJTdj (ORCPT ); Wed, 10 Jul 2013 15:33:39 -0400 Date: Wed, 10 Jul 2013 15:33:30 -0400 From: "J. Bruce Fields" To: Jeff Layton Cc: Al Viro , linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, Mikulas Patocka , David Howells , Tyler Hicks , Dustin Kirkland Subject: Re: [PATCH 12/12] locks: break delegations on any attribute modification Message-ID: <20130710193330.GA24548@pad.fieldses.org> References: <1372882356-14168-1-git-send-email-bfields@redhat.com> <1372882356-14168-13-git-send-email-bfields@redhat.com> <20130709093047.0096f061@tlielax.poochiereds.net> <20130709205101.GK32574@pad.fieldses.org> <20130709211911.GL32574@pad.fieldses.org> <20130709212625.7fdfc6e1@corrin.poochiereds.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <20130709212625.7fdfc6e1@corrin.poochiereds.net> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Tue, Jul 09, 2013 at 09:26:25PM -0400, Jeff Layton wrote: > On Tue, 9 Jul 2013 17:19:12 -0400 > "J. Bruce Fields" wrote: > > > On Tue, Jul 09, 2013 at 04:51:01PM -0400, J. Bruce Fields wrote: > > > On Tue, Jul 09, 2013 at 09:30:47AM -0400, Jeff Layton wrote: > > > > On Wed, 3 Jul 2013 16:12:36 -0400 > > > > "J. Bruce Fields" wrote: > > > > > > > > > From: "J. Bruce Fields" > > > > > > > > > > NFSv4 uses leases to guarantee that clients can cache metadata as well > > > > > as data. > > > > > > > ... > > > > Isn't it possible we'll need to break a delegation on truncate()? > > > > > > In the truncate case, the caller called break_lease, and in the > > > ftruncate case it's called with a write open, and the open already broke > > > any leases or delegations. > > > > > > Might need a comment--could I get away with just this?: > > > > > > mutex_lock(&dentry->d_inode->i_mutex); > > > + /* NULL is safe: any delegations have already been broken: */ > > > ret = notify_change(dentry, &newattrs, NULL); > > > mutex_unlock(&dentry->d_inode->i_mutex); > > > return ret; > > > > > > I also added something to the notify_change kerneldoc: "passing NULL is > > > fine for callers holding the file open for write, as there can be no > > > conflicting delegation in that case." > > > > Another question is whether it's really worth dropping locks and > > retrying in this case. > > > > We could instead do the following. > > > > --b. > > > > commit 40a4fd613034cd3f242ec11e5ecd44f9a83ab39d > > Author: J. Bruce Fields > > Date: Tue Sep 20 17:19:26 2011 -0400 > > > > locks: break delegations on any attribute modification > > > > NFSv4 uses leases to guarantee that clients can cache metadata as well > > as data. > > > > Note unlike link, unlink, and rename, we don't bother dropping locks and > > retrying. In the other cases we're holding a directory mutex, hence > > blocking operations (even lookups) on the same directory. In this case > > we're holding only the i_mutex on this file, so the impact of an > > unresponsive client is limited to this file. > > > > Signed-off-by: J. Bruce Fields > > > > diff --git a/fs/attr.c b/fs/attr.c > > index 1449adb..a2c1d04 100644 > > --- a/fs/attr.c > > +++ b/fs/attr.c > > @@ -243,6 +243,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr) > > error = security_inode_setattr(dentry, attr); > > if (error) > > return error; > > + error = break_deleg_wait(inode); > > + if (error) > > + return error; > > > > if (inode->i_op->setattr) > > error = inode->i_op->setattr(dentry, attr); > > > I guess the question is whether there are operations that require the > i_mutex but that don't require the delegation recall to have finished. Also if there's any risk that something on the delegation-return path might take the i_mutex then we'd risk blocking the client's attempt to return until the delegation timed out and got revoked. In fact a CLAIM_DELEG_CUR open needs a lookup so probably runs into exactly that problem. OK, back to the more complicated solution. --b.