Re: [PATCH 12/12] locks: break delegations on any attribute modification

[Jeff Layton <jlayton@redhat.com>]

On Tue, 9 Jul 2013 17:19:12 -0400
"J. Bruce Fields" <bfields@redhat.com> 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@redhat.com> wrote:
> > > 
> > > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > > 
> > > > 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@redhat.com>
> 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@redhat.com>
> 
> 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.

-- 
Jeff Layton <jlayton@redhat.com>