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" <bfields@xxxxxxxxxx> 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" <bfields@xxxxxxxxxx> wrote: > > > > > > > > > From: "J. Bruce Fields" <bfields@xxxxxxxxxx> > > > > > > > > > > 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 <bfields@xxxxxxxxxx> > > 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 <bfields@xxxxxxxxxx> > > > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
