From: Jeff Layton <jlayton@redhat.com>
To: "J. Bruce Fields" <bfields@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>,
David Howells <dhowells@redhat.com>,
Tyler Hicks <tyhicks@canonical.com>,
Dustin Kirkland <dustin.kirkland@gazzang.com>
Subject: Re: [PATCH 12/12] locks: break delegations on any attribute modification
Date: Tue, 9 Jul 2013 19:57:54 -0400 [thread overview]
Message-ID: <20130709195754.4574ee06@corrin.poochiereds.net> (raw)
In-Reply-To: <20130709205101.GK32574@pad.fieldses.org>
On Tue, 9 Jul 2013 16:51:01 -0400
"J. Bruce Fields" <bfields@redhat.com> 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.
> > >
> > > Cc: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>
> > > Cc: David Howells <dhowells@redhat.com>
> > > Cc: Tyler Hicks <tyhicks@canonical.com>
> > > Cc: Dustin Kirkland <dustin.kirkland@gazzang.com>
> > > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > > ---
> > > drivers/base/devtmpfs.c | 4 ++--
> > > fs/attr.c | 5 ++++-
> > > fs/cachefiles/interface.c | 4 ++--
> > > fs/ecryptfs/inode.c | 2 +-
> > > fs/hpfs/namei.c | 2 +-
> > > fs/inode.c | 6 +++++-
> > > fs/nfsd/vfs.c | 8 ++++++--
> > > fs/open.c | 21 +++++++++++++++++----
> > > fs/utimes.c | 9 ++++++++-
> > > include/linux/fs.h | 2 +-
> > > 10 files changed, 47 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
> > > index 1b8490e..0f38201 100644
> > > --- a/drivers/base/devtmpfs.c
> > > +++ b/drivers/base/devtmpfs.c
> > > @@ -216,7 +216,7 @@ static int handle_create(const char *nodename, umode_t mode, kuid_t uid,
> > > newattrs.ia_gid = gid;
> > > newattrs.ia_valid = ATTR_MODE|ATTR_UID|ATTR_GID;
> > > mutex_lock(&dentry->d_inode->i_mutex);
> > > - notify_change(dentry, &newattrs);
> > > + notify_change(dentry, &newattrs, NULL);
> > > mutex_unlock(&dentry->d_inode->i_mutex);
> > >
> > > /* mark as kernel-created inode */
> > > @@ -322,7 +322,7 @@ static int handle_remove(const char *nodename, struct device *dev)
> > > newattrs.ia_valid =
> > > ATTR_UID|ATTR_GID|ATTR_MODE;
> > > mutex_lock(&dentry->d_inode->i_mutex);
> > > - notify_change(dentry, &newattrs);
> > > + notify_change(dentry, &newattrs, NULL);
> > > mutex_unlock(&dentry->d_inode->i_mutex);
> > > err = vfs_unlink(parent.dentry->d_inode, dentry, NULL);
> > > if (!err || err == -ENOENT)
> > > diff --git a/fs/attr.c b/fs/attr.c
> > > index 1449adb..261f5c9 100644
> > > --- a/fs/attr.c
> > > +++ b/fs/attr.c
> > > @@ -167,7 +167,7 @@ void setattr_copy(struct inode *inode, const struct iattr *attr)
> > > }
> > > EXPORT_SYMBOL(setattr_copy);
> > >
> > > -int notify_change(struct dentry * dentry, struct iattr * attr)
> > > +int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **delegated_inode)
> > > {
> > > struct inode *inode = dentry->d_inode;
> > > umode_t mode = inode->i_mode;
> > > @@ -243,6 +243,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
> > > error = security_inode_setattr(dentry, attr);
> > > if (error)
> > > return error;
> > > + error = try_break_deleg(inode, delegated_inode);
> > > + if (error)
> > > + return error;
> > >
> > > if (inode->i_op->setattr)
> > > error = inode->i_op->setattr(dentry, attr);
> > > diff --git a/fs/cachefiles/interface.c b/fs/cachefiles/interface.c
> > > index 746ce53..40f5917 100644
> > > --- a/fs/cachefiles/interface.c
> > > +++ b/fs/cachefiles/interface.c
> > > @@ -417,14 +417,14 @@ static int cachefiles_attr_changed(struct fscache_object *_object)
> > > _debug("discard tail %llx", oi_size);
> > > newattrs.ia_valid = ATTR_SIZE;
> > > newattrs.ia_size = oi_size & PAGE_MASK;
> > > - ret = notify_change(object->backer, &newattrs);
> > > + ret = notify_change(object->backer, &newattrs, NULL);
> > > if (ret < 0)
> > > goto truncate_failed;
> > > }
> > >
> > > newattrs.ia_valid = ATTR_SIZE;
> > > newattrs.ia_size = ni_size;
> > > - ret = notify_change(object->backer, &newattrs);
> > > + ret = notify_change(object->backer, &newattrs, NULL);
> > >
> > > truncate_failed:
> > > mutex_unlock(&object->backer->d_inode->i_mutex);
> > > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> > > index 19e4435..bd54575 100644
> > > --- a/fs/ecryptfs/inode.c
> > > +++ b/fs/ecryptfs/inode.c
> > > @@ -992,7 +992,7 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia)
> > > lower_ia.ia_valid &= ~ATTR_MODE;
> > >
> > > mutex_lock(&lower_dentry->d_inode->i_mutex);
> > > - rc = notify_change(lower_dentry, &lower_ia);
> > > + rc = notify_change(lower_dentry, &lower_ia, NULL);
> > > mutex_unlock(&lower_dentry->d_inode->i_mutex);
> > > out:
> > > fsstack_copy_attr_all(inode, lower_inode);
> > > diff --git a/fs/hpfs/namei.c b/fs/hpfs/namei.c
> > > index 345713d..1b39afd 100644
> > > --- a/fs/hpfs/namei.c
> > > +++ b/fs/hpfs/namei.c
> > > @@ -407,7 +407,7 @@ again:
> > > /*printk("HPFS: truncating file before delete.\n");*/
> > > newattrs.ia_size = 0;
> > > newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
> > > - err = notify_change(dentry, &newattrs);
> > > + err = notify_change(dentry, &newattrs, NULL);
> > > put_write_access(inode);
> > > if (!err)
> > > goto again;
> > > diff --git a/fs/inode.c b/fs/inode.c
> > > index 304db4c..664d631 100644
> > > --- a/fs/inode.c
> > > +++ b/fs/inode.c
> > > @@ -1633,7 +1633,11 @@ static int __remove_suid(struct dentry *dentry, int kill)
> > > struct iattr newattrs;
> > >
> > > newattrs.ia_valid = ATTR_FORCE | kill;
> > > - return notify_change(dentry, &newattrs);
> > > + /*
> > > + * Note we call this on write, so notify_change will not
> > > + * encounter any conflicting delegations:
> > > + */
> > > + return notify_change(dentry, &newattrs, NULL);
> > > }
> > >
> > > int file_remove_suid(struct file *file)
> > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > > index b9740cb..e781901 100644
> > > --- a/fs/nfsd/vfs.c
> > > +++ b/fs/nfsd/vfs.c
> > > @@ -426,7 +426,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> > > goto out_nfserr;
> > > fh_lock(fhp);
> > >
> > > - host_err = notify_change(dentry, iap);
> > > + host_err = notify_change(dentry, iap, NULL);
> > > err = nfserrno(host_err);
> > > fh_unlock(fhp);
> > > }
> > > @@ -959,7 +959,11 @@ static void kill_suid(struct dentry *dentry)
> > > ia.ia_valid = ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
> > >
> > > mutex_lock(&dentry->d_inode->i_mutex);
> > > - notify_change(dentry, &ia);
> > > + /*
> > > + * Note we call this on write, so notify_change will not
> > > + * encounter any conflicting delegations:
> > > + */
> > > + notify_change(dentry, &ia, NULL);
> > > mutex_unlock(&dentry->d_inode->i_mutex);
> > > }
> > >
> > > diff --git a/fs/open.c b/fs/open.c
> > > index 8c74100..1a39d29 100644
> > > --- a/fs/open.c
> > > +++ b/fs/open.c
> > > @@ -57,7 +57,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
> > > newattrs.ia_valid |= ret | ATTR_FORCE;
> > >
> > > mutex_lock(&dentry->d_inode->i_mutex);
> > > - ret = notify_change(dentry, &newattrs);
> > > + ret = notify_change(dentry, &newattrs, NULL);
> > > mutex_unlock(&dentry->d_inode->i_mutex);
> > > return ret;
> > > }
> >
> > 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."
>
Ahh ok, makes sense. Both comments sound like good additions.
Thanks,
--
Jeff Layton <jlayton@redhat.com>
next prev parent reply other threads:[~2013-07-09 23:57 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-03 20:12 [PATCH 00/12] Implement NFSv4 delegations, take 8 J. Bruce Fields
2013-07-03 20:12 ` [PATCH 01/12] vfs: pull ext4's double-i_mutex-locking into common code J. Bruce Fields
2013-07-09 10:49 ` Jeff Layton
2013-07-09 15:48 ` Theodore Ts'o
2013-07-09 22:04 ` Dave Chinner
2013-07-10 0:21 ` J. Bruce Fields
2013-07-10 2:09 ` Dave Chinner
2013-07-10 2:40 ` J. Bruce Fields
2013-07-10 3:38 ` Dave Chinner
2013-07-10 21:26 ` J. Bruce Fields
2013-07-11 14:04 ` Jeff Layton
2013-07-12 22:07 ` J. Bruce Fields
2013-07-03 20:12 ` [PATCH 02/12] vfs: don't use PARENT/CHILD lock classes for non-directories J. Bruce Fields
2013-07-09 10:50 ` Jeff Layton
2013-07-03 20:12 ` [PATCH 03/12] vfs: rename I_MUTEX_QUOTA now that it's not used for quotas J. Bruce Fields
2013-07-09 10:54 ` Jeff Layton
2013-07-09 14:26 ` J. Bruce Fields
2013-07-09 14:31 ` Jeff Layton
2013-07-03 20:12 ` [PATCH 04/12] vfs: take i_mutex on renamed file J. Bruce Fields
2013-07-09 10:59 ` Jeff Layton
2013-07-03 20:12 ` [PATCH 05/12] locks: introduce new FL_DELEG lock flag J. Bruce Fields
2013-07-09 11:00 ` Jeff Layton
2013-07-03 20:12 ` [PATCH 06/12] locks: implement delegations J. Bruce Fields
2013-07-09 12:23 ` Jeff Layton
2013-07-09 14:41 ` J. Bruce Fields
2013-07-03 20:12 ` [PATCH 07/12] namei: minor vfs_unlink cleanup J. Bruce Fields
2013-07-09 12:50 ` Jeff Layton
2013-07-03 20:12 ` [PATCH 08/12] locks: break delegations on unlink J. Bruce Fields
2013-07-09 13:05 ` Jeff Layton
2013-07-09 13:07 ` Jeff Layton
2013-07-09 15:58 ` J. Bruce Fields
2013-07-09 16:02 ` Jeff Layton
2013-07-09 19:29 ` J. Bruce Fields
2013-07-03 20:12 ` [PATCH 09/12] locks: helper functions for delegation breaking J. Bruce Fields
2013-07-09 13:09 ` Jeff Layton
2013-07-09 19:31 ` J. Bruce Fields
2013-07-09 19:37 ` Jeff Layton
2013-07-09 13:23 ` Jeff Layton
2013-07-09 19:38 ` J. Bruce Fields
2013-07-09 20:28 ` Jeff Layton
2013-07-03 20:12 ` [PATCH 10/12] locks: break delegations on rename J. Bruce Fields
2013-07-09 13:14 ` Jeff Layton
2013-07-03 20:12 ` [PATCH 11/12] locks: break delegations on link J. Bruce Fields
2013-07-09 13:16 ` Jeff Layton
2013-07-09 20:41 ` J. Bruce Fields
2013-07-03 20:12 ` [PATCH 12/12] locks: break delegations on any attribute modification J. Bruce Fields
2013-07-09 13:30 ` Jeff Layton
2013-07-09 20:51 ` J. Bruce Fields
2013-07-09 21:19 ` J. Bruce Fields
2013-07-10 1:26 ` Jeff Layton
2013-07-10 19:33 ` J. Bruce Fields
2013-07-09 23:57 ` Jeff Layton [this message]
-- strict thread matches above, loose matches on Subject: below --
2013-09-05 16:30 [PATCH 00/12] Implement NFSv4 delegations, take 10 J. Bruce Fields
2013-09-05 16:30 ` [PATCH 12/12] locks: break delegations on any attribute modification J. Bruce Fields
2013-04-17 1:46 [PATCH 00/12] Implement NFSv4 delegations, take 7 J. Bruce Fields
2013-04-17 1:46 ` [PATCH 12/12] locks: break delegations on any attribute modification J. Bruce Fields
2013-02-03 16:31 [PATCH 00/12] Implement NFSv4 delegations, take 6 J. Bruce Fields
2013-02-03 16:31 ` [PATCH 12/12] locks: break delegations on any attribute modification J. Bruce Fields
2012-10-16 22:01 [PATCH 00/12] Implement NFSv4 delegations, take 5 J. Bruce Fields
2012-10-16 22:01 ` [PATCH 12/12] locks: break delegations on any attribute modification J. Bruce Fields
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130709195754.4574ee06@corrin.poochiereds.net \
--to=jlayton@redhat.com \
--cc=bfields@redhat.com \
--cc=dhowells@redhat.com \
--cc=dustin.kirkland@gazzang.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=mikulas@artax.karlin.mff.cuni.cz \
--cc=tyhicks@canonical.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).