| Message ID | 165708109259.1940.685583862810495747.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: clean up locking. | expand |
On Wed, 2022-07-06 at 14:18 +1000, NeilBrown wrote: > When creating or unlinking a name in a directory use explicit > inode_lock_nested() instead of fh_lock(), and explicit calls to > fh_fill_pre_attrs() and fh_fill_post_attrs(). This is already done for > renames. > > Also move the 'fill' calls closer to the operation that might change the > attributes. This way they are avoided on some error paths. > > Having the locking explicit will simplify proposed future changes to > locking for directories. It also makes it easily visible exactly where > pre/post attributes are used - not all callers of fh_lock() actually > need the pre/post attributes. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs3proc.c | 6 ++++-- > fs/nfsd/nfs4proc.c | 6 ++++-- > fs/nfsd/nfsproc.c | 7 ++++--- > fs/nfsd/vfs.c | 30 +++++++++++++++++++----------- > 4 files changed, 31 insertions(+), 18 deletions(-) > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > index 3a67d0afb885..9629517344ff 100644 > --- a/fs/nfsd/nfs3proc.c > +++ b/fs/nfsd/nfs3proc.c > @@ -254,7 +254,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > return nfserrno(host_err); > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > + inode_lock_nested(inode, I_MUTEX_PARENT); > > child = lookup_one_len(argp->name, parent, argp->len); > if (IS_ERR(child)) { > @@ -312,11 +312,13 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (!IS_POSIXACL(inode)) > iap->ia_mode &= ~current_umask(); > > + fh_fill_pre_attrs(fhp); > host_err = vfs_create(&init_user_ns, inode, child, iap->ia_mode, true); > if (host_err < 0) { > status = nfserrno(host_err); > goto out; > } > + fh_fill_post_attrs(fhp); > > /* A newly created file already has a file size of zero. */ > if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) > @@ -334,7 +336,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); > > out: > - fh_unlock(fhp); > + inode_unlock(inode); > if (child && !IS_ERR(child)) > dput(child); > fh_drop_write(fhp); > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index 6ec22c69cbec..242f059e6788 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -306,7 +306,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > return nfserrno(host_err); > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > + inode_lock_nested(inode, I_MUTEX_PARENT); > > child = lookup_one_len(open->op_fname, parent, open->op_fnamelen); > if (IS_ERR(child)) { > @@ -385,10 +385,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (!IS_POSIXACL(inode)) > iap->ia_mode &= ~current_umask(); > > + fh_fill_pre_attrs(fhp); > status = nfsd4_vfs_create(fhp, child, open); > if (status != nfs_ok) > goto out; > open->op_created = true; > + fh_fill_post_attrs(fhp); > > /* A newly created file already has a file size of zero. */ > if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) > @@ -406,7 +408,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); > > out: > - fh_unlock(fhp); > + inode_unlock(inode); > if (child && !IS_ERR(child)) > dput(child); > fh_drop_write(fhp); > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > index ed24fae09517..427c404bc52b 100644 > --- a/fs/nfsd/nfsproc.c > +++ b/fs/nfsd/nfsproc.c > @@ -285,7 +285,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) > goto done; > } > > - fh_lock_nested(dirfhp, I_MUTEX_PARENT); > + inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT); > dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len); > if (IS_ERR(dchild)) { > resp->status = nfserrno(PTR_ERR(dchild)); > @@ -382,6 +382,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) > } > > resp->status = nfs_ok; > + fh_fill_pre_attrs(dirfhp); > if (!inode) { > /* File doesn't exist. Create it and set attrs */ > resp->status = nfsd_create_locked(rqstp, dirfhp, argp->name, > @@ -399,10 +400,10 @@ nfsd_proc_create(struct svc_rqst *rqstp) > resp->status = nfsd_setattr(rqstp, newfhp, attr, 0, > (time64_t)0); > } > + fh_fill_post_attrs(dirfhp); > > out_unlock: > - /* We don't really need to unlock, as fh_put does it. */ > - fh_unlock(dirfhp); > + inode_unlock(dirfhp->fh_dentry->d_inode); > fh_drop_write(dirfhp); > done: > fh_put(dirfhp); > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 8e050c6d112a..2ca748aa83bb 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1412,7 +1412,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > return nfserrno(host_err); > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT); > dchild = lookup_one_len(fname, dentry, flen); > host_err = PTR_ERR(dchild); > if (IS_ERR(dchild)) { > @@ -1427,12 +1427,14 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > dput(dchild); > if (err) > goto out_unlock; > + fh_fill_pre_attrs(fhp); > err = nfsd_create_locked(rqstp, fhp, fname, flen, iap, type, > rdev, resfhp); > if (!err && post_create) > post_create(resfhp, data); > + fh_fill_post_attrs(fhp); > out_unlock: > - fh_unlock(fhp); > + inode_unlock(dentry->d_inode); > return err; > } > > @@ -1505,14 +1507,15 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > goto out_nfserr; > > - fh_lock(fhp); > dentry = fhp->fh_dentry; > + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT); > dnew = lookup_one_len(fname, dentry, flen); > host_err = PTR_ERR(dnew); > if (IS_ERR(dnew)) { > - fh_unlock(fhp); > + inode_unlock(dentry->d_inode); > goto out_nfserr; > } > + fh_fill_pre_attrs(fhp); > host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); > err = nfserrno(host_err); > if (!err) > @@ -1525,7 +1528,8 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (err==0) err = cerr; > if (!err && post_create) > post_create(resfhp, data); > - fh_unlock(fhp); > + fh_fill_post_attrs(fhp); > + inode_unlock(dentry->d_inode); > out: > return err; > > @@ -1569,9 +1573,9 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > goto out; > } > > - fh_lock_nested(ffhp, I_MUTEX_PARENT); > ddir = ffhp->fh_dentry; > dirp = d_inode(ddir); > + inode_lock_nested(dirp, I_MUTEX_PARENT); > > dnew = lookup_one_len(name, ddir, len); > host_err = PTR_ERR(dnew); > @@ -1585,8 +1589,10 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > err = nfserr_noent; > if (d_really_is_negative(dold)) > goto out_dput; > + fh_fill_pre_attrs(ffhp); > host_err = vfs_link(dold, &init_user_ns, dirp, dnew, NULL); > - fh_unlock(ffhp); > + fh_fill_post_attrs(ffhp); > + inode_unlock(dirp); > if (!host_err) { > err = nfserrno(commit_metadata(ffhp)); > if (!err) > @@ -1606,7 +1612,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > out_dput: > dput(dnew); > out_unlock: > - fh_unlock(ffhp); > + inode_unlock(dirp); > goto out_drop_write; > } > > @@ -1781,9 +1787,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > if (host_err) > goto out_nfserr; > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > dentry = fhp->fh_dentry; > dirp = d_inode(dentry); > + inode_lock_nested(dirp, I_MUTEX_PARENT); > > rdentry = lookup_one_len(fname, dentry, flen); > host_err = PTR_ERR(rdentry); > @@ -1801,6 +1807,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > if (!type) > type = d_inode(rdentry)->i_mode & S_IFMT; > > + fh_fill_pre_attrs(fhp); > if (type != S_IFDIR) { > if (rdentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK) > nfsd_close_cached_files(rdentry); > @@ -1808,8 +1815,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > } else { > host_err = vfs_rmdir(&init_user_ns, dirp, rdentry); > } > + fh_fill_post_attrs(fhp); > > - fh_unlock(fhp); > + inode_unlock(dirp); > if (!host_err) > host_err = commit_metadata(fhp); > dput(rdentry); > @@ -1832,7 +1840,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > out: > return err; > out_unlock: > - fh_unlock(fhp); > + inode_unlock(dirp); > goto out_drop_write; > } > > > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> On Jul 6, 2022, at 12:18 AM, NeilBrown <neilb@suse.de> wrote: > > When creating or unlinking a name in a directory use explicit > inode_lock_nested() instead of fh_lock(), and explicit calls to > fh_fill_pre_attrs() and fh_fill_post_attrs(). This is already done for > renames. > > Also move the 'fill' calls closer to the operation that might change the > attributes. This way they are avoided on some error paths. > > Having the locking explicit will simplify proposed future changes to > locking for directories. It also makes it easily visible exactly where > pre/post attributes are used - not all callers of fh_lock() actually > need the pre/post attributes. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs3proc.c | 6 ++++-- > fs/nfsd/nfs4proc.c | 6 ++++-- > fs/nfsd/nfsproc.c | 7 ++++--- > fs/nfsd/vfs.c | 30 +++++++++++++++++++----------- > 4 files changed, 31 insertions(+), 18 deletions(-) > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > index 3a67d0afb885..9629517344ff 100644 > --- a/fs/nfsd/nfs3proc.c > +++ b/fs/nfsd/nfs3proc.c > @@ -254,7 +254,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > return nfserrno(host_err); > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > + inode_lock_nested(inode, I_MUTEX_PARENT); > > child = lookup_one_len(argp->name, parent, argp->len); > if (IS_ERR(child)) { > @@ -312,11 +312,13 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (!IS_POSIXACL(inode)) > iap->ia_mode &= ~current_umask(); > > + fh_fill_pre_attrs(fhp); > host_err = vfs_create(&init_user_ns, inode, child, iap->ia_mode, true); > if (host_err < 0) { > status = nfserrno(host_err); > goto out; > } > + fh_fill_post_attrs(fhp); > > /* A newly created file already has a file size of zero. */ > if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) > @@ -334,7 +336,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); > > out: > - fh_unlock(fhp); > + inode_unlock(inode); > if (child && !IS_ERR(child)) > dput(child); > fh_drop_write(fhp); > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index 6ec22c69cbec..242f059e6788 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -306,7 +306,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > return nfserrno(host_err); > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > + inode_lock_nested(inode, I_MUTEX_PARENT); > > child = lookup_one_len(open->op_fname, parent, open->op_fnamelen); > if (IS_ERR(child)) { > @@ -385,10 +385,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (!IS_POSIXACL(inode)) > iap->ia_mode &= ~current_umask(); > > + fh_fill_pre_attrs(fhp); > status = nfsd4_vfs_create(fhp, child, open); > if (status != nfs_ok) > goto out; > open->op_created = true; > + fh_fill_post_attrs(fhp); > > /* A newly created file already has a file size of zero. */ > if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) > @@ -406,7 +408,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); > > out: > - fh_unlock(fhp); > + inode_unlock(inode); > if (child && !IS_ERR(child)) > dput(child); > fh_drop_write(fhp); > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > index ed24fae09517..427c404bc52b 100644 > --- a/fs/nfsd/nfsproc.c > +++ b/fs/nfsd/nfsproc.c > @@ -285,7 +285,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) > goto done; > } > > - fh_lock_nested(dirfhp, I_MUTEX_PARENT); > + inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT); > dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len); > if (IS_ERR(dchild)) { > resp->status = nfserrno(PTR_ERR(dchild)); > @@ -382,6 +382,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) > } > > resp->status = nfs_ok; > + fh_fill_pre_attrs(dirfhp); > if (!inode) { > /* File doesn't exist. Create it and set attrs */ > resp->status = nfsd_create_locked(rqstp, dirfhp, argp->name, > @@ -399,10 +400,10 @@ nfsd_proc_create(struct svc_rqst *rqstp) > resp->status = nfsd_setattr(rqstp, newfhp, attr, 0, > (time64_t)0); > } > + fh_fill_post_attrs(dirfhp); Are the fh_fill_* twins necessary for NFSv2 CREATE? > out_unlock: > - /* We don't really need to unlock, as fh_put does it. */ > - fh_unlock(dirfhp); > + inode_unlock(dirfhp->fh_dentry->d_inode); > fh_drop_write(dirfhp); > done: > fh_put(dirfhp); > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 8e050c6d112a..2ca748aa83bb 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1412,7 +1412,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > return nfserrno(host_err); > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT); > dchild = lookup_one_len(fname, dentry, flen); > host_err = PTR_ERR(dchild); > if (IS_ERR(dchild)) { > @@ -1427,12 +1427,14 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > dput(dchild); > if (err) > goto out_unlock; > + fh_fill_pre_attrs(fhp); > err = nfsd_create_locked(rqstp, fhp, fname, flen, iap, type, > rdev, resfhp); > if (!err && post_create) > post_create(resfhp, data); > + fh_fill_post_attrs(fhp); > out_unlock: > - fh_unlock(fhp); > + inode_unlock(dentry->d_inode); > return err; > } > > @@ -1505,14 +1507,15 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > goto out_nfserr; > > - fh_lock(fhp); > dentry = fhp->fh_dentry; > + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT); > dnew = lookup_one_len(fname, dentry, flen); > host_err = PTR_ERR(dnew); > if (IS_ERR(dnew)) { > - fh_unlock(fhp); > + inode_unlock(dentry->d_inode); > goto out_nfserr; > } > + fh_fill_pre_attrs(fhp); > host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); > err = nfserrno(host_err); > if (!err) > @@ -1525,7 +1528,8 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (err==0) err = cerr; > if (!err && post_create) > post_create(resfhp, data); > - fh_unlock(fhp); > + fh_fill_post_attrs(fhp); > + inode_unlock(dentry->d_inode); > out: > return err; > > @@ -1569,9 +1573,9 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > goto out; > } > > - fh_lock_nested(ffhp, I_MUTEX_PARENT); > ddir = ffhp->fh_dentry; > dirp = d_inode(ddir); > + inode_lock_nested(dirp, I_MUTEX_PARENT); > > dnew = lookup_one_len(name, ddir, len); > host_err = PTR_ERR(dnew); > @@ -1585,8 +1589,10 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > err = nfserr_noent; > if (d_really_is_negative(dold)) > goto out_dput; > + fh_fill_pre_attrs(ffhp); > host_err = vfs_link(dold, &init_user_ns, dirp, dnew, NULL); > - fh_unlock(ffhp); > + fh_fill_post_attrs(ffhp); > + inode_unlock(dirp); > if (!host_err) { > err = nfserrno(commit_metadata(ffhp)); > if (!err) > @@ -1606,7 +1612,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > out_dput: > dput(dnew); > out_unlock: > - fh_unlock(ffhp); > + inode_unlock(dirp); > goto out_drop_write; > } > > @@ -1781,9 +1787,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > if (host_err) > goto out_nfserr; > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > dentry = fhp->fh_dentry; > dirp = d_inode(dentry); > + inode_lock_nested(dirp, I_MUTEX_PARENT); > > rdentry = lookup_one_len(fname, dentry, flen); > host_err = PTR_ERR(rdentry); > @@ -1801,6 +1807,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > if (!type) > type = d_inode(rdentry)->i_mode & S_IFMT; > > + fh_fill_pre_attrs(fhp); > if (type != S_IFDIR) { > if (rdentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK) > nfsd_close_cached_files(rdentry); > @@ -1808,8 +1815,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > } else { > host_err = vfs_rmdir(&init_user_ns, dirp, rdentry); > } > + fh_fill_post_attrs(fhp); > > - fh_unlock(fhp); > + inode_unlock(dirp); > if (!host_err) > host_err = commit_metadata(fhp); > dput(rdentry); > @@ -1832,7 +1840,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > out: > return err; > out_unlock: > - fh_unlock(fhp); > + inode_unlock(dirp); > goto out_drop_write; > } > > > -- Chuck Lever
On Wed, 2022-07-06 at 14:18 +1000, NeilBrown wrote: > When creating or unlinking a name in a directory use explicit > inode_lock_nested() instead of fh_lock(), and explicit calls to > fh_fill_pre_attrs() and fh_fill_post_attrs(). This is already done for > renames. > > Also move the 'fill' calls closer to the operation that might change the > attributes. This way they are avoided on some error paths. > > Having the locking explicit will simplify proposed future changes to > locking for directories. It also makes it easily visible exactly where > pre/post attributes are used - not all callers of fh_lock() actually > need the pre/post attributes. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs3proc.c | 6 ++++-- > fs/nfsd/nfs4proc.c | 6 ++++-- > fs/nfsd/nfsproc.c | 7 ++++--- > fs/nfsd/vfs.c | 30 +++++++++++++++++++----------- > 4 files changed, 31 insertions(+), 18 deletions(-) > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > index 3a67d0afb885..9629517344ff 100644 > --- a/fs/nfsd/nfs3proc.c > +++ b/fs/nfsd/nfs3proc.c > @@ -254,7 +254,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > return nfserrno(host_err); > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > + inode_lock_nested(inode, I_MUTEX_PARENT); > > child = lookup_one_len(argp->name, parent, argp->len); > if (IS_ERR(child)) { > @@ -312,11 +312,13 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (!IS_POSIXACL(inode)) > iap->ia_mode &= ~current_umask(); > > + fh_fill_pre_attrs(fhp); > host_err = vfs_create(&init_user_ns, inode, child, iap->ia_mode, true); > if (host_err < 0) { > status = nfserrno(host_err); > goto out; > } > + fh_fill_post_attrs(fhp); > > /* A newly created file already has a file size of zero. */ > if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) > @@ -334,7 +336,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); > > out: > - fh_unlock(fhp); > + inode_unlock(inode); > if (child && !IS_ERR(child)) > dput(child); > fh_drop_write(fhp); > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index 6ec22c69cbec..242f059e6788 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -306,7 +306,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > return nfserrno(host_err); > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > + inode_lock_nested(inode, I_MUTEX_PARENT); > > child = lookup_one_len(open->op_fname, parent, open->op_fnamelen); > if (IS_ERR(child)) { > @@ -385,10 +385,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (!IS_POSIXACL(inode)) > iap->ia_mode &= ~current_umask(); > > + fh_fill_pre_attrs(fhp); > status = nfsd4_vfs_create(fhp, child, open); > if (status != nfs_ok) > goto out; > open->op_created = true; > + fh_fill_post_attrs(fhp); > > /* A newly created file already has a file size of zero. */ > if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) Should the fh_fill_post_attrs call be done after nfsd_create_setattr instead in this function? It seems like we're filling out the post-op attr here before we're actually done changing things... > @@ -406,7 +408,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); > > out: > - fh_unlock(fhp); > + inode_unlock(inode); > if (child && !IS_ERR(child)) > dput(child); > fh_drop_write(fhp); > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > index ed24fae09517..427c404bc52b 100644 > --- a/fs/nfsd/nfsproc.c > +++ b/fs/nfsd/nfsproc.c > @@ -285,7 +285,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) > goto done; > } > > - fh_lock_nested(dirfhp, I_MUTEX_PARENT); > + inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT); > dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len); > if (IS_ERR(dchild)) { > resp->status = nfserrno(PTR_ERR(dchild)); > @@ -382,6 +382,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) > } > > resp->status = nfs_ok; > + fh_fill_pre_attrs(dirfhp); > if (!inode) { > /* File doesn't exist. Create it and set attrs */ > resp->status = nfsd_create_locked(rqstp, dirfhp, argp->name, > @@ -399,10 +400,10 @@ nfsd_proc_create(struct svc_rqst *rqstp) > resp->status = nfsd_setattr(rqstp, newfhp, attr, 0, > (time64_t)0); > } > + fh_fill_post_attrs(dirfhp); > > out_unlock: > - /* We don't really need to unlock, as fh_put does it. */ > - fh_unlock(dirfhp); > + inode_unlock(dirfhp->fh_dentry->d_inode); > fh_drop_write(dirfhp); > done: > fh_put(dirfhp); > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 8e050c6d112a..2ca748aa83bb 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1412,7 +1412,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > return nfserrno(host_err); > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT); > dchild = lookup_one_len(fname, dentry, flen); > host_err = PTR_ERR(dchild); > if (IS_ERR(dchild)) { > @@ -1427,12 +1427,14 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > dput(dchild); > if (err) > goto out_unlock; > + fh_fill_pre_attrs(fhp); > err = nfsd_create_locked(rqstp, fhp, fname, flen, iap, type, > rdev, resfhp); > if (!err && post_create) > post_create(resfhp, data); > + fh_fill_post_attrs(fhp); > out_unlock: > - fh_unlock(fhp); > + inode_unlock(dentry->d_inode); > return err; > } > > @@ -1505,14 +1507,15 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (host_err) > goto out_nfserr; > > - fh_lock(fhp); > dentry = fhp->fh_dentry; > + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT); > dnew = lookup_one_len(fname, dentry, flen); > host_err = PTR_ERR(dnew); > if (IS_ERR(dnew)) { > - fh_unlock(fhp); > + inode_unlock(dentry->d_inode); > goto out_nfserr; > } > + fh_fill_pre_attrs(fhp); > host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); > err = nfserrno(host_err); > if (!err) > @@ -1525,7 +1528,8 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (err==0) err = cerr; > if (!err && post_create) > post_create(resfhp, data); > - fh_unlock(fhp); > + fh_fill_post_attrs(fhp); > + inode_unlock(dentry->d_inode); > out: > return err; > > @@ -1569,9 +1573,9 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > goto out; > } > > - fh_lock_nested(ffhp, I_MUTEX_PARENT); > ddir = ffhp->fh_dentry; > dirp = d_inode(ddir); > + inode_lock_nested(dirp, I_MUTEX_PARENT); > > dnew = lookup_one_len(name, ddir, len); > host_err = PTR_ERR(dnew); > @@ -1585,8 +1589,10 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > err = nfserr_noent; > if (d_really_is_negative(dold)) > goto out_dput; > + fh_fill_pre_attrs(ffhp); > host_err = vfs_link(dold, &init_user_ns, dirp, dnew, NULL); > - fh_unlock(ffhp); > + fh_fill_post_attrs(ffhp); > + inode_unlock(dirp); > if (!host_err) { > err = nfserrno(commit_metadata(ffhp)); > if (!err) > @@ -1606,7 +1612,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > out_dput: > dput(dnew); > out_unlock: > - fh_unlock(ffhp); > + inode_unlock(dirp); > goto out_drop_write; > } > > @@ -1781,9 +1787,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > if (host_err) > goto out_nfserr; > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > dentry = fhp->fh_dentry; > dirp = d_inode(dentry); > + inode_lock_nested(dirp, I_MUTEX_PARENT); > > rdentry = lookup_one_len(fname, dentry, flen); > host_err = PTR_ERR(rdentry); > @@ -1801,6 +1807,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > if (!type) > type = d_inode(rdentry)->i_mode & S_IFMT; > > + fh_fill_pre_attrs(fhp); > if (type != S_IFDIR) { > if (rdentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK) > nfsd_close_cached_files(rdentry); > @@ -1808,8 +1815,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > } else { > host_err = vfs_rmdir(&init_user_ns, dirp, rdentry); > } > + fh_fill_post_attrs(fhp); > > - fh_unlock(fhp); > + inode_unlock(dirp); > if (!host_err) > host_err = commit_metadata(fhp); > dput(rdentry); > @@ -1832,7 +1840,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > out: > return err; > out_unlock: > - fh_unlock(fhp); > + inode_unlock(dirp); > goto out_drop_write; > } > > >
On Fri, 2022-07-15 at 12:11 -0400, Jeff Layton wrote: > On Wed, 2022-07-06 at 14:18 +1000, NeilBrown wrote: > > When creating or unlinking a name in a directory use explicit > > inode_lock_nested() instead of fh_lock(), and explicit calls to > > fh_fill_pre_attrs() and fh_fill_post_attrs(). This is already done for > > renames. > > > > Also move the 'fill' calls closer to the operation that might change the > > attributes. This way they are avoided on some error paths. > > > > Having the locking explicit will simplify proposed future changes to > > locking for directories. It also makes it easily visible exactly where > > pre/post attributes are used - not all callers of fh_lock() actually > > need the pre/post attributes. > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > fs/nfsd/nfs3proc.c | 6 ++++-- > > fs/nfsd/nfs4proc.c | 6 ++++-- > > fs/nfsd/nfsproc.c | 7 ++++--- > > fs/nfsd/vfs.c | 30 +++++++++++++++++++----------- > > 4 files changed, 31 insertions(+), 18 deletions(-) > > > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > > index 3a67d0afb885..9629517344ff 100644 > > --- a/fs/nfsd/nfs3proc.c > > +++ b/fs/nfsd/nfs3proc.c > > @@ -254,7 +254,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > if (host_err) > > return nfserrno(host_err); > > > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > > + inode_lock_nested(inode, I_MUTEX_PARENT); > > > > child = lookup_one_len(argp->name, parent, argp->len); > > if (IS_ERR(child)) { > > @@ -312,11 +312,13 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > if (!IS_POSIXACL(inode)) > > iap->ia_mode &= ~current_umask(); > > > > + fh_fill_pre_attrs(fhp); > > host_err = vfs_create(&init_user_ns, inode, child, iap->ia_mode, true); > > if (host_err < 0) { > > status = nfserrno(host_err); > > goto out; > > } > > + fh_fill_post_attrs(fhp); > > > > /* A newly created file already has a file size of zero. */ > > if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) > > @@ -334,7 +336,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); > > > > out: > > - fh_unlock(fhp); > > + inode_unlock(inode); > > if (child && !IS_ERR(child)) > > dput(child); > > fh_drop_write(fhp); > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > index 6ec22c69cbec..242f059e6788 100644 > > --- a/fs/nfsd/nfs4proc.c > > +++ b/fs/nfsd/nfs4proc.c > > @@ -306,7 +306,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > if (host_err) > > return nfserrno(host_err); > > > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > > + inode_lock_nested(inode, I_MUTEX_PARENT); > > > > child = lookup_one_len(open->op_fname, parent, open->op_fnamelen); > > if (IS_ERR(child)) { > > @@ -385,10 +385,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > if (!IS_POSIXACL(inode)) > > iap->ia_mode &= ~current_umask(); > > > > + fh_fill_pre_attrs(fhp); > > status = nfsd4_vfs_create(fhp, child, open); > > if (status != nfs_ok) > > goto out; > > open->op_created = true; > > + fh_fill_post_attrs(fhp); > > > > /* A newly created file already has a file size of zero. */ > > if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) > > Should the fh_fill_post_attrs call be done after nfsd_create_setattr > instead in this function? It seems like we're filling out the post-op > attr here before we're actually done changing things... > > > @@ -406,7 +408,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); > > > > out: > > - fh_unlock(fhp); > > + inode_unlock(inode); > > if (child && !IS_ERR(child)) > > dput(child); > > fh_drop_write(fhp); > > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > > index ed24fae09517..427c404bc52b 100644 > > --- a/fs/nfsd/nfsproc.c > > +++ b/fs/nfsd/nfsproc.c > > @@ -285,7 +285,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) > > goto done; > > } > > > > - fh_lock_nested(dirfhp, I_MUTEX_PARENT); > > + inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT); > > dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len); > > if (IS_ERR(dchild)) { > > resp->status = nfserrno(PTR_ERR(dchild)); > > @@ -382,6 +382,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) > > } > > > > resp->status = nfs_ok; > > + fh_fill_pre_attrs(dirfhp); > > if (!inode) { > > /* File doesn't exist. Create it and set attrs */ > > resp->status = nfsd_create_locked(rqstp, dirfhp, argp->name, > > @@ -399,10 +400,10 @@ nfsd_proc_create(struct svc_rqst *rqstp) > > resp->status = nfsd_setattr(rqstp, newfhp, attr, 0, > > (time64_t)0); > > } > > + fh_fill_post_attrs(dirfhp); > > > > out_unlock: > > - /* We don't really need to unlock, as fh_put does it. */ > > - fh_unlock(dirfhp); > > + inode_unlock(dirfhp->fh_dentry->d_inode); > > fh_drop_write(dirfhp); > > done: > > fh_put(dirfhp); > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > > index 8e050c6d112a..2ca748aa83bb 100644 > > --- a/fs/nfsd/vfs.c > > +++ b/fs/nfsd/vfs.c > > @@ -1412,7 +1412,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > > if (host_err) > > return nfserrno(host_err); > > > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > > + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT); > > dchild = lookup_one_len(fname, dentry, flen); > > host_err = PTR_ERR(dchild); > > if (IS_ERR(dchild)) { > > @@ -1427,12 +1427,14 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > > dput(dchild); > > if (err) > > goto out_unlock; > > + fh_fill_pre_attrs(fhp); > > err = nfsd_create_locked(rqstp, fhp, fname, flen, iap, type, > > rdev, resfhp); > > if (!err && post_create) > > post_create(resfhp, data); > > + fh_fill_post_attrs(fhp); > > out_unlock: > > - fh_unlock(fhp); > > + inode_unlock(dentry->d_inode); > > return err; > > } > > > > @@ -1505,14 +1507,15 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > > if (host_err) > > goto out_nfserr; > > > > - fh_lock(fhp); > > dentry = fhp->fh_dentry; > > + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT); > > dnew = lookup_one_len(fname, dentry, flen); > > host_err = PTR_ERR(dnew); > > if (IS_ERR(dnew)) { > > - fh_unlock(fhp); > > + inode_unlock(dentry->d_inode); > > goto out_nfserr; > > } > > + fh_fill_pre_attrs(fhp); > > host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); > > err = nfserrno(host_err); > > if (!err) > > @@ -1525,7 +1528,8 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > > if (err==0) err = cerr; > > if (!err && post_create) > > post_create(resfhp, data); > > - fh_unlock(fhp); > > + fh_fill_post_attrs(fhp); > > + inode_unlock(dentry->d_inode); > > out: > > return err; > > > > @@ -1569,9 +1573,9 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > > goto out; > > } > > > > - fh_lock_nested(ffhp, I_MUTEX_PARENT); > > ddir = ffhp->fh_dentry; > > dirp = d_inode(ddir); > > + inode_lock_nested(dirp, I_MUTEX_PARENT); > > > > dnew = lookup_one_len(name, ddir, len); > > host_err = PTR_ERR(dnew); > > @@ -1585,8 +1589,10 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > > err = nfserr_noent; > > if (d_really_is_negative(dold)) > > goto out_dput; > > + fh_fill_pre_attrs(ffhp); > > host_err = vfs_link(dold, &init_user_ns, dirp, dnew, NULL); > > - fh_unlock(ffhp); > > + fh_fill_post_attrs(ffhp); > > + inode_unlock(dirp); > > if (!host_err) { > > err = nfserrno(commit_metadata(ffhp)); > > if (!err) > > @@ -1606,7 +1612,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, > > out_dput: > > dput(dnew); > > out_unlock: > > - fh_unlock(ffhp); > > + inode_unlock(dirp); > > goto out_drop_write; > > } > > > > @@ -1781,9 +1787,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > > if (host_err) > > goto out_nfserr; > > > > - fh_lock_nested(fhp, I_MUTEX_PARENT); > > dentry = fhp->fh_dentry; > > dirp = d_inode(dentry); > > + inode_lock_nested(dirp, I_MUTEX_PARENT); > > > > rdentry = lookup_one_len(fname, dentry, flen); > > host_err = PTR_ERR(rdentry); > > @@ -1801,6 +1807,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > > if (!type) > > type = d_inode(rdentry)->i_mode & S_IFMT; > > > > + fh_fill_pre_attrs(fhp); > > if (type != S_IFDIR) { > > if (rdentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK) > > nfsd_close_cached_files(rdentry); > > @@ -1808,8 +1815,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > > } else { > > host_err = vfs_rmdir(&init_user_ns, dirp, rdentry); > > } > > + fh_fill_post_attrs(fhp); > > > > - fh_unlock(fhp); > > + inode_unlock(dirp); > > if (!host_err) > > host_err = commit_metadata(fhp); > > dput(rdentry); > > @@ -1832,7 +1840,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, > > out: > > return err; > > out_unlock: > > - fh_unlock(fhp); > > + inode_unlock(dirp); > > goto out_drop_write; > > } > > > > > > > [PATCH] SQUASH: nfsd: ensure we fill in pre-op-attrs in nfsd4_create_file In some cases, they're left uninitialized. This also ensures that the post_op attrs are properly filled in all cases too. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4proc.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index 242f059e6788..05652a7dabe8 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -346,6 +346,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, switch (open->op_createmode) { case NFS4_CREATE_UNCHECKED: + fh_fill_pre_attrs(fhp); if (!d_is_reg(child)) break; @@ -365,6 +366,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, if (d_inode(child)->i_mtime.tv_sec == v_mtime && d_inode(child)->i_atime.tv_sec == v_atime && d_inode(child)->i_size == 0) { + fh_fill_pre_attrs(fhp); open->op_created = true; break; /* subtle */ } @@ -374,6 +376,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, if (d_inode(child)->i_mtime.tv_sec == v_mtime && d_inode(child)->i_atime.tv_sec == v_atime && d_inode(child)->i_size == 0) { + fh_fill_pre_attrs(fhp); open->op_created = true; goto set_attr; /* subtle */ } @@ -385,12 +388,10 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, if (!IS_POSIXACL(inode)) iap->ia_mode &= ~current_umask(); - fh_fill_pre_attrs(fhp); status = nfsd4_vfs_create(fhp, child, open); if (status != nfs_ok) goto out; open->op_created = true; - fh_fill_post_attrs(fhp); /* A newly created file already has a file size of zero. */ if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) @@ -408,6 +409,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); out: + if (status == nfs_ok) + fh_fill_post_attrs(fhp); inode_unlock(inode); if (child && !IS_ERR(child)) dput(child);
On Sat, 16 Jul 2022, Jeff Layton wrote: > On Wed, 2022-07-06 at 14:18 +1000, NeilBrown wrote: > > > > + fh_fill_pre_attrs(fhp); > > status = nfsd4_vfs_create(fhp, child, open); > > if (status != nfs_ok) > > goto out; > > open->op_created = true; > > + fh_fill_post_attrs(fhp); > > > > /* A newly created file already has a file size of zero. */ > > if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) > > Should the fh_fill_post_attrs call be done after nfsd_create_setattr > instead in this function? It seems like we're filling out the post-op > attr here before we're actually done changing things... nfsd_create_setattr() only affects the newly created thing, so it should not be changing any attributes of the directory that it was created in. So it should not matter for correctness where fh_fill_post_attrs() is called, as long as it is between nfsd4_vfs_create() and inode_unlock(). I preferred closer to the former. Thanks, NeilBrown
On Sat, 16 Jul 2022, Jeff Layton wrote: > On Fri, 2022-07-15 at 12:11 -0400, Jeff Layton wrote: > > > [PATCH] SQUASH: nfsd: ensure we fill in pre-op-attrs in > nfsd4_create_file > > In some cases, they're left uninitialized. This also ensures that the > post_op attrs are properly filled in all cases too. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> Thanks Jeff, but I think this is more noisy than necessary. The problem is that the d_really_is_positive() doesn't actually change the directory (obviously) but can succeed - so pre/post attributes are needed by NFSv4 even though they aren't really relevant. I would rather use the same approach as in the !open->op_create branch in d_open_lookup() : fh_fill_pre_attrs(current_fh); fh_fill_post_attrs(current_fh); with a comment explaining that as the directory is locked, and as it isn't being changed, this makes sense. I'll fold that in. Thanks, NeilBrown > --- > fs/nfsd/nfs4proc.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index 242f059e6788..05652a7dabe8 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -346,6 +346,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > switch (open->op_createmode) { > case NFS4_CREATE_UNCHECKED: > + fh_fill_pre_attrs(fhp); > if (!d_is_reg(child)) > break; > > @@ -365,6 +366,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (d_inode(child)->i_mtime.tv_sec == v_mtime && > d_inode(child)->i_atime.tv_sec == v_atime && > d_inode(child)->i_size == 0) { > + fh_fill_pre_attrs(fhp); > open->op_created = true; > break; /* subtle */ > } > @@ -374,6 +376,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (d_inode(child)->i_mtime.tv_sec == v_mtime && > d_inode(child)->i_atime.tv_sec == v_atime && > d_inode(child)->i_size == 0) { > + fh_fill_pre_attrs(fhp); > open->op_created = true; > goto set_attr; /* subtle */ > } > @@ -385,12 +388,10 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (!IS_POSIXACL(inode)) > iap->ia_mode &= ~current_umask(); > > - fh_fill_pre_attrs(fhp); > status = nfsd4_vfs_create(fhp, child, open); > if (status != nfs_ok) > goto out; > open->op_created = true; > - fh_fill_post_attrs(fhp); > > /* A newly created file already has a file size of zero. */ > if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) > @@ -408,6 +409,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); > > out: > + if (status == nfs_ok) > + fh_fill_post_attrs(fhp); > inode_unlock(inode); > if (child && !IS_ERR(child)) > dput(child); > -- > 2.36.1 > > >
diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c index 3a67d0afb885..9629517344ff 100644 --- a/fs/nfsd/nfs3proc.c +++ b/fs/nfsd/nfs3proc.c @@ -254,7 +254,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, if (host_err) return nfserrno(host_err); - fh_lock_nested(fhp, I_MUTEX_PARENT); + inode_lock_nested(inode, I_MUTEX_PARENT); child = lookup_one_len(argp->name, parent, argp->len); if (IS_ERR(child)) { @@ -312,11 +312,13 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, if (!IS_POSIXACL(inode)) iap->ia_mode &= ~current_umask(); + fh_fill_pre_attrs(fhp); host_err = vfs_create(&init_user_ns, inode, child, iap->ia_mode, true); if (host_err < 0) { status = nfserrno(host_err); goto out; } + fh_fill_post_attrs(fhp); /* A newly created file already has a file size of zero. */ if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) @@ -334,7 +336,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); out: - fh_unlock(fhp); + inode_unlock(inode); if (child && !IS_ERR(child)) dput(child); fh_drop_write(fhp); diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index 6ec22c69cbec..242f059e6788 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -306,7 +306,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, if (host_err) return nfserrno(host_err); - fh_lock_nested(fhp, I_MUTEX_PARENT); + inode_lock_nested(inode, I_MUTEX_PARENT); child = lookup_one_len(open->op_fname, parent, open->op_fnamelen); if (IS_ERR(child)) { @@ -385,10 +385,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, if (!IS_POSIXACL(inode)) iap->ia_mode &= ~current_umask(); + fh_fill_pre_attrs(fhp); status = nfsd4_vfs_create(fhp, child, open); if (status != nfs_ok) goto out; open->op_created = true; + fh_fill_post_attrs(fhp); /* A newly created file already has a file size of zero. */ if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0)) @@ -406,7 +408,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, status = nfsd_create_setattr(rqstp, fhp, resfhp, iap); out: - fh_unlock(fhp); + inode_unlock(inode); if (child && !IS_ERR(child)) dput(child); fh_drop_write(fhp); diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c index ed24fae09517..427c404bc52b 100644 --- a/fs/nfsd/nfsproc.c +++ b/fs/nfsd/nfsproc.c @@ -285,7 +285,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) goto done; } - fh_lock_nested(dirfhp, I_MUTEX_PARENT); + inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT); dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len); if (IS_ERR(dchild)) { resp->status = nfserrno(PTR_ERR(dchild)); @@ -382,6 +382,7 @@ nfsd_proc_create(struct svc_rqst *rqstp) } resp->status = nfs_ok; + fh_fill_pre_attrs(dirfhp); if (!inode) { /* File doesn't exist. Create it and set attrs */ resp->status = nfsd_create_locked(rqstp, dirfhp, argp->name, @@ -399,10 +400,10 @@ nfsd_proc_create(struct svc_rqst *rqstp) resp->status = nfsd_setattr(rqstp, newfhp, attr, 0, (time64_t)0); } + fh_fill_post_attrs(dirfhp); out_unlock: - /* We don't really need to unlock, as fh_put does it. */ - fh_unlock(dirfhp); + inode_unlock(dirfhp->fh_dentry->d_inode); fh_drop_write(dirfhp); done: fh_put(dirfhp); diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 8e050c6d112a..2ca748aa83bb 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1412,7 +1412,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, if (host_err) return nfserrno(host_err); - fh_lock_nested(fhp, I_MUTEX_PARENT); + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT); dchild = lookup_one_len(fname, dentry, flen); host_err = PTR_ERR(dchild); if (IS_ERR(dchild)) { @@ -1427,12 +1427,14 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, dput(dchild); if (err) goto out_unlock; + fh_fill_pre_attrs(fhp); err = nfsd_create_locked(rqstp, fhp, fname, flen, iap, type, rdev, resfhp); if (!err && post_create) post_create(resfhp, data); + fh_fill_post_attrs(fhp); out_unlock: - fh_unlock(fhp); + inode_unlock(dentry->d_inode); return err; } @@ -1505,14 +1507,15 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, if (host_err) goto out_nfserr; - fh_lock(fhp); dentry = fhp->fh_dentry; + inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT); dnew = lookup_one_len(fname, dentry, flen); host_err = PTR_ERR(dnew); if (IS_ERR(dnew)) { - fh_unlock(fhp); + inode_unlock(dentry->d_inode); goto out_nfserr; } + fh_fill_pre_attrs(fhp); host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); err = nfserrno(host_err); if (!err) @@ -1525,7 +1528,8 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, if (err==0) err = cerr; if (!err && post_create) post_create(resfhp, data); - fh_unlock(fhp); + fh_fill_post_attrs(fhp); + inode_unlock(dentry->d_inode); out: return err; @@ -1569,9 +1573,9 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, goto out; } - fh_lock_nested(ffhp, I_MUTEX_PARENT); ddir = ffhp->fh_dentry; dirp = d_inode(ddir); + inode_lock_nested(dirp, I_MUTEX_PARENT); dnew = lookup_one_len(name, ddir, len); host_err = PTR_ERR(dnew); @@ -1585,8 +1589,10 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, err = nfserr_noent; if (d_really_is_negative(dold)) goto out_dput; + fh_fill_pre_attrs(ffhp); host_err = vfs_link(dold, &init_user_ns, dirp, dnew, NULL); - fh_unlock(ffhp); + fh_fill_post_attrs(ffhp); + inode_unlock(dirp); if (!host_err) { err = nfserrno(commit_metadata(ffhp)); if (!err) @@ -1606,7 +1612,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, out_dput: dput(dnew); out_unlock: - fh_unlock(ffhp); + inode_unlock(dirp); goto out_drop_write; } @@ -1781,9 +1787,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, if (host_err) goto out_nfserr; - fh_lock_nested(fhp, I_MUTEX_PARENT); dentry = fhp->fh_dentry; dirp = d_inode(dentry); + inode_lock_nested(dirp, I_MUTEX_PARENT); rdentry = lookup_one_len(fname, dentry, flen); host_err = PTR_ERR(rdentry); @@ -1801,6 +1807,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, if (!type) type = d_inode(rdentry)->i_mode & S_IFMT; + fh_fill_pre_attrs(fhp); if (type != S_IFDIR) { if (rdentry->d_sb->s_export_op->flags & EXPORT_OP_CLOSE_BEFORE_UNLINK) nfsd_close_cached_files(rdentry); @@ -1808,8 +1815,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, } else { host_err = vfs_rmdir(&init_user_ns, dirp, rdentry); } + fh_fill_post_attrs(fhp); - fh_unlock(fhp); + inode_unlock(dirp); if (!host_err) host_err = commit_metadata(fhp); dput(rdentry); @@ -1832,7 +1840,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, out: return err; out_unlock: - fh_unlock(fhp); + inode_unlock(dirp); goto out_drop_write; }
When creating or unlinking a name in a directory use explicit inode_lock_nested() instead of fh_lock(), and explicit calls to fh_fill_pre_attrs() and fh_fill_post_attrs(). This is already done for renames. Also move the 'fill' calls closer to the operation that might change the attributes. This way they are avoided on some error paths. Having the locking explicit will simplify proposed future changes to locking for directories. It also makes it easily visible exactly where pre/post attributes are used - not all callers of fh_lock() actually need the pre/post attributes. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfs3proc.c | 6 ++++-- fs/nfsd/nfs4proc.c | 6 ++++-- fs/nfsd/nfsproc.c | 7 ++++--- fs/nfsd/vfs.c | 30 +++++++++++++++++++----------- 4 files changed, 31 insertions(+), 18 deletions(-)