| Message ID | 20240824-nfsd-fixes-v1-1-c7208502492e@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs/nfsd: fix update of inode attrs in CB_GETATTR | expand |
On Sat, Aug 24, 2024 at 08:46:18AM -0400, Jeff Layton wrote: > Currently, we copy the mtime and ctime to the in-core inode and then > mark the inode dirty. This is fine for certain types of filesystems, but > not all. Some require a real setattr to properly change these values > (e.g. ceph or reexported NFS). > > Fix this code to call notify_change() instead, which is the proper way > to effect a setattr. There is one problem though: > > [...] Applied to nfsd-fixes for v6.11-rc, thanks! [1/1] fs/nfsd: fix update of inode attrs in CB_GETATTR commit: b0367a7cf033841257d6cc1ff5f9d383aad97b61
On Sat, Aug 24, 2024 at 08:46:18AM GMT, Jeff Layton wrote: > Currently, we copy the mtime and ctime to the in-core inode and then > mark the inode dirty. This is fine for certain types of filesystems, but > not all. Some require a real setattr to properly change these values > (e.g. ceph or reexported NFS). > > Fix this code to call notify_change() instead, which is the proper way > to effect a setattr. There is one problem though: > > In this case, the client is holding a write delegation and has sent us > attributes to update our cache. We don't want to break the delegation > for this since that would defeat the purpose. Add a new ATTR_DELEG flag > that makes notify_change bypass the try_break_deleg call. > > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > One more CB_GETATTR fix. This one involves a little change at the VFS > layer to avoid breaking the delegation. > > Christian, unless you have objections, this should probably go in > via Chuck's tree as this patch depends on a nfsd patch [1] that I sent > yesterday. An A-b or R-b would be welcome though. Fwiw, #define ATTR_DELEG (1 << 18) /* Delegated attrs (don't break) */ is a bit sparse of a comment for anyone not familiar with leases imo. So I would update this to say something similar to what what you say in the commit message: "Don't break write delegation while we're updating the cache because of the write." or something similar/less clunky. Reviewed-by: Christian Brauner <brauner@kernel.org>
On Mon, 2024-08-26 at 12:36 +0200, Christian Brauner wrote: > On Sat, Aug 24, 2024 at 08:46:18AM GMT, Jeff Layton wrote: > > Currently, we copy the mtime and ctime to the in-core inode and then > > mark the inode dirty. This is fine for certain types of filesystems, but > > not all. Some require a real setattr to properly change these values > > (e.g. ceph or reexported NFS). > > > > Fix this code to call notify_change() instead, which is the proper way > > to effect a setattr. There is one problem though: > > > > In this case, the client is holding a write delegation and has sent us > > attributes to update our cache. We don't want to break the delegation > > for this since that would defeat the purpose. Add a new ATTR_DELEG flag > > that makes notify_change bypass the try_break_deleg call. > > > > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > One more CB_GETATTR fix. This one involves a little change at the VFS > > layer to avoid breaking the delegation. > > > > Christian, unless you have objections, this should probably go in > > via Chuck's tree as this patch depends on a nfsd patch [1] that I sent > > yesterday. An A-b or R-b would be welcome though. > > Fwiw, > > #define ATTR_DELEG (1 << 18) /* Delegated attrs (don't break) */ > > is a bit sparse of a comment for anyone not familiar with leases imo. So > I would update this to say something similar to what what you say in the > commit message: "Don't break write delegation while we're updating the > cache because of the write." or something similar/less clunky. > > Reviewed-by: Christian Brauner <brauner@kernel.org> Thanks. I'll plan to send a v2 that adds better comments.
On 8/24/24 5:46 AM, Jeff Layton wrote: > Currently, we copy the mtime and ctime to the in-core inode and then > mark the inode dirty. This is fine for certain types of filesystems, but > not all. Some require a real setattr to properly change these values > (e.g. ceph or reexported NFS). > > Fix this code to call notify_change() instead, which is the proper way > to effect a setattr. There is one problem though: > > In this case, the client is holding a write delegation and has sent us > attributes to update our cache. We don't want to break the delegation > for this since that would defeat the purpose. I think this won't happen with NFS since nfsd_breaker_owns_lease detects its own lease and won't break the delegation. -Dai > Add a new ATTR_DELEG flag > that makes notify_change bypass the try_break_deleg call. > > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > One more CB_GETATTR fix. This one involves a little change at the VFS > layer to avoid breaking the delegation. > > Christian, unless you have objections, this should probably go in > via Chuck's tree as this patch depends on a nfsd patch [1] that I sent > yesterday. An A-b or R-b would be welcome though. > > [1]: https://lore.kernel.org/linux-nfs/20240823-nfsd-fixes-v1-1-fc99aa16f6a0@kernel.org/T/#u > --- > fs/attr.c | 9 ++++++--- > fs/nfsd/nfs4state.c | 18 +++++++++++++----- > fs/nfsd/nfs4xdr.c | 2 +- > fs/nfsd/state.h | 2 +- > include/linux/fs.h | 1 + > 5 files changed, 22 insertions(+), 10 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index 960a310581eb..a40a2fb406f0 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -489,9 +489,12 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry, > error = security_inode_setattr(idmap, dentry, attr); > if (error) > return error; > - error = try_break_deleg(inode, delegated_inode); > - if (error) > - return error; > + > + if (!(ia_valid & ATTR_DELEG)) { > + error = try_break_deleg(inode, delegated_inode); > + if (error) > + return error; > + } > > if (inode->i_op->setattr) > error = inode->i_op->setattr(idmap, dentry, attr); > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index dafff707e23a..e0e3d3ca0d45 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -8815,7 +8815,7 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate, > /** > * nfsd4_deleg_getattr_conflict - Recall if GETATTR causes conflict > * @rqstp: RPC transaction context > - * @inode: file to be checked for a conflict > + * @dentry: dentry of inode to be checked for a conflict > * @modified: return true if file was modified > * @size: new size of file if modified is true > * > @@ -8830,7 +8830,7 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate, > * code is returned. > */ > __be32 > -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry, > bool *modified, u64 *size) > { > __be32 status; > @@ -8840,6 +8840,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > struct nfs4_delegation *dp; > struct iattr attrs; > struct nfs4_cb_fattr *ncf; > + struct inode *inode = d_inode(dentry); > > *modified = false; > ctx = locks_inode_context(inode); > @@ -8887,15 +8888,22 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > ncf->ncf_cur_fsize != ncf->ncf_cb_fsize)) > ncf->ncf_file_modified = true; > if (ncf->ncf_file_modified) { > + int err; > + > /* > * Per section 10.4.3 of RFC 8881, the server would > * not update the file's metadata with the client's > * modified size > */ > attrs.ia_mtime = attrs.ia_ctime = current_time(inode); > - attrs.ia_valid = ATTR_MTIME | ATTR_CTIME; > - setattr_copy(&nop_mnt_idmap, inode, &attrs); > - mark_inode_dirty(inode); > + attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG; > + inode_lock(inode); > + err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL); > + inode_unlock(inode); > + if (err) { > + nfs4_put_stid(&dp->dl_stid); > + return nfserrno(err); > + } > ncf->ncf_cur_fsize = ncf->ncf_cb_fsize; > *size = ncf->ncf_cur_fsize; > *modified = true; > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index 43ccf6119cf1..97f583777972 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -3565,7 +3565,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr, > } > args.size = 0; > if (attrmask[0] & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) { > - status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry), > + status = nfsd4_deleg_getattr_conflict(rqstp, dentry, > &file_modified, &size); > if (status) > goto out; > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > index ffc217099d19..ec4559ecd193 100644 > --- a/fs/nfsd/state.h > +++ b/fs/nfsd/state.h > @@ -781,5 +781,5 @@ static inline bool try_to_expire_client(struct nfs4_client *clp) > } > > extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, > - struct inode *inode, bool *file_modified, u64 *size); > + struct dentry *dentry, bool *file_modified, u64 *size); > #endif /* NFSD4_STATE_H */ > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 0283cf366c2a..3fe289c74869 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -208,6 +208,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, > #define ATTR_OPEN (1 << 15) /* Truncating from open(O_TRUNC) */ > #define ATTR_TIMES_SET (1 << 16) > #define ATTR_TOUCH (1 << 17) > +#define ATTR_DELEG (1 << 18) /* Delegated attrs (don't break) */ > > /* > * Whiteout is represented by a char device. The following constants define the > > --- > base-commit: a204501e1743d695ca2930ed25a2be9f8ced96d3 > change-id: 20240823-nfsd-fixes-61f0c785d125 > > Best regards,
On Mon, 2024-08-26 at 08:31 -0700, Dai Ngo wrote: > > On 8/24/24 5:46 AM, Jeff Layton wrote: > > Currently, we copy the mtime and ctime to the in-core inode and then > > mark the inode dirty. This is fine for certain types of filesystems, but > > not all. Some require a real setattr to properly change these values > > (e.g. ceph or reexported NFS). > > > > Fix this code to call notify_change() instead, which is the proper way > > to effect a setattr. There is one problem though: > > > > In this case, the client is holding a write delegation and has sent us > > attributes to update our cache. We don't want to break the delegation > > for this since that would defeat the purpose. > > I think this won't happen with NFS since nfsd_breaker_owns_lease detects > its own lease and won't break the delegation. > I don't think that works here. In this case, we've gotten a GETATTR request from a different client than the lease holder. So the breaker in this case does _not_ own the lease. > > > Add a new ATTR_DELEG flag > > that makes notify_change bypass the try_break_deleg call. > > > > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > One more CB_GETATTR fix. This one involves a little change at the VFS > > layer to avoid breaking the delegation. > > > > Christian, unless you have objections, this should probably go in > > via Chuck's tree as this patch depends on a nfsd patch [1] that I sent > > yesterday. An A-b or R-b would be welcome though. > > > > [1]: https://lore.kernel.org/linux-nfs/20240823-nfsd-fixes-v1-1-fc99aa16f6a0@kernel.org/T/#u > > --- > > fs/attr.c | 9 ++++++--- > > fs/nfsd/nfs4state.c | 18 +++++++++++++----- > > fs/nfsd/nfs4xdr.c | 2 +- > > fs/nfsd/state.h | 2 +- > > include/linux/fs.h | 1 + > > 5 files changed, 22 insertions(+), 10 deletions(-) > > > > diff --git a/fs/attr.c b/fs/attr.c > > index 960a310581eb..a40a2fb406f0 100644 > > --- a/fs/attr.c > > +++ b/fs/attr.c > > @@ -489,9 +489,12 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry, > > error = security_inode_setattr(idmap, dentry, attr); > > if (error) > > return error; > > - error = try_break_deleg(inode, delegated_inode); > > - if (error) > > - return error; > > + > > + if (!(ia_valid & ATTR_DELEG)) { > > + error = try_break_deleg(inode, delegated_inode); > > + if (error) > > + return error; > > + } > > > > if (inode->i_op->setattr) > > error = inode->i_op->setattr(idmap, dentry, attr); > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index dafff707e23a..e0e3d3ca0d45 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -8815,7 +8815,7 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate, > > /** > > * nfsd4_deleg_getattr_conflict - Recall if GETATTR causes conflict > > * @rqstp: RPC transaction context > > - * @inode: file to be checked for a conflict > > + * @dentry: dentry of inode to be checked for a conflict > > * @modified: return true if file was modified > > * @size: new size of file if modified is true > > * > > @@ -8830,7 +8830,7 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate, > > * code is returned. > > */ > > __be32 > > -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry, > > bool *modified, u64 *size) > > { > > __be32 status; > > @@ -8840,6 +8840,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > > struct nfs4_delegation *dp; > > struct iattr attrs; > > struct nfs4_cb_fattr *ncf; > > + struct inode *inode = d_inode(dentry); > > > > *modified = false; > > ctx = locks_inode_context(inode); > > @@ -8887,15 +8888,22 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > > ncf->ncf_cur_fsize != ncf->ncf_cb_fsize)) > > ncf->ncf_file_modified = true; > > if (ncf->ncf_file_modified) { > > + int err; > > + > > /* > > * Per section 10.4.3 of RFC 8881, the server would > > * not update the file's metadata with the client's > > * modified size > > */ > > attrs.ia_mtime = attrs.ia_ctime = current_time(inode); > > - attrs.ia_valid = ATTR_MTIME | ATTR_CTIME; > > - setattr_copy(&nop_mnt_idmap, inode, &attrs); > > - mark_inode_dirty(inode); > > + attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG; > > + inode_lock(inode); > > + err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL); > > + inode_unlock(inode); > > + if (err) { > > + nfs4_put_stid(&dp->dl_stid); > > + return nfserrno(err); > > + } > > ncf->ncf_cur_fsize = ncf->ncf_cb_fsize; > > *size = ncf->ncf_cur_fsize; > > *modified = true; > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > index 43ccf6119cf1..97f583777972 100644 > > --- a/fs/nfsd/nfs4xdr.c > > +++ b/fs/nfsd/nfs4xdr.c > > @@ -3565,7 +3565,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr, > > } > > args.size = 0; > > if (attrmask[0] & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) { > > - status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry), > > + status = nfsd4_deleg_getattr_conflict(rqstp, dentry, > > &file_modified, &size); > > if (status) > > goto out; > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > > index ffc217099d19..ec4559ecd193 100644 > > --- a/fs/nfsd/state.h > > +++ b/fs/nfsd/state.h > > @@ -781,5 +781,5 @@ static inline bool try_to_expire_client(struct nfs4_client *clp) > > } > > > > extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, > > - struct inode *inode, bool *file_modified, u64 *size); > > + struct dentry *dentry, bool *file_modified, u64 *size); > > #endif /* NFSD4_STATE_H */ > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 0283cf366c2a..3fe289c74869 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -208,6 +208,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, > > #define ATTR_OPEN (1 << 15) /* Truncating from open(O_TRUNC) */ > > #define ATTR_TIMES_SET (1 << 16) > > #define ATTR_TOUCH (1 << 17) > > +#define ATTR_DELEG (1 << 18) /* Delegated attrs (don't break) */ > > > > /* > > * Whiteout is represented by a char device. The following constants define the > > > > --- > > base-commit: a204501e1743d695ca2930ed25a2be9f8ced96d3 > > change-id: 20240823-nfsd-fixes-61f0c785d125 > > > > Best regards,
On 8/26/24 9:13 AM, Jeff Layton wrote: > On Mon, 2024-08-26 at 08:31 -0700, Dai Ngo wrote: >> On 8/24/24 5:46 AM, Jeff Layton wrote: >>> Currently, we copy the mtime and ctime to the in-core inode and then >>> mark the inode dirty. This is fine for certain types of filesystems, but >>> not all. Some require a real setattr to properly change these values >>> (e.g. ceph or reexported NFS). >>> >>> Fix this code to call notify_change() instead, which is the proper way >>> to effect a setattr. There is one problem though: >>> >>> In this case, the client is holding a write delegation and has sent us >>> attributes to update our cache. We don't want to break the delegation >>> for this since that would defeat the purpose. >> I think this won't happen with NFS since nfsd_breaker_owns_lease detects >> its own lease and won't break the delegation. >> > I don't think that works here. In this case, we've gotten a GETATTR > request from a different client than the lease holder. So the breaker > in this case does _not_ own the lease. Oh yes, the lease breaker in nfsd_breaker_owns_lease is the client that sends the GETATTR so it is not the same as the lease's owner. Thanks, -Dai > >>> Add a new ATTR_DELEG flag >>> that makes notify_change bypass the try_break_deleg call. >>> >>> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>> --- >>> One more CB_GETATTR fix. This one involves a little change at the VFS >>> layer to avoid breaking the delegation. >>> >>> Christian, unless you have objections, this should probably go in >>> via Chuck's tree as this patch depends on a nfsd patch [1] that I sent >>> yesterday. An A-b or R-b would be welcome though. >>> >>> [1]: https://lore.kernel.org/linux-nfs/20240823-nfsd-fixes-v1-1-fc99aa16f6a0@kernel.org/T/#u >>> --- >>> fs/attr.c | 9 ++++++--- >>> fs/nfsd/nfs4state.c | 18 +++++++++++++----- >>> fs/nfsd/nfs4xdr.c | 2 +- >>> fs/nfsd/state.h | 2 +- >>> include/linux/fs.h | 1 + >>> 5 files changed, 22 insertions(+), 10 deletions(-) >>> >>> diff --git a/fs/attr.c b/fs/attr.c >>> index 960a310581eb..a40a2fb406f0 100644 >>> --- a/fs/attr.c >>> +++ b/fs/attr.c >>> @@ -489,9 +489,12 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry, >>> error = security_inode_setattr(idmap, dentry, attr); >>> if (error) >>> return error; >>> - error = try_break_deleg(inode, delegated_inode); >>> - if (error) >>> - return error; >>> + >>> + if (!(ia_valid & ATTR_DELEG)) { >>> + error = try_break_deleg(inode, delegated_inode); >>> + if (error) >>> + return error; >>> + } >>> >>> if (inode->i_op->setattr) >>> error = inode->i_op->setattr(idmap, dentry, attr); >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>> index dafff707e23a..e0e3d3ca0d45 100644 >>> --- a/fs/nfsd/nfs4state.c >>> +++ b/fs/nfsd/nfs4state.c >>> @@ -8815,7 +8815,7 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate, >>> /** >>> * nfsd4_deleg_getattr_conflict - Recall if GETATTR causes conflict >>> * @rqstp: RPC transaction context >>> - * @inode: file to be checked for a conflict >>> + * @dentry: dentry of inode to be checked for a conflict >>> * @modified: return true if file was modified >>> * @size: new size of file if modified is true >>> * >>> @@ -8830,7 +8830,7 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate, >>> * code is returned. >>> */ >>> __be32 >>> -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, >>> +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry, >>> bool *modified, u64 *size) >>> { >>> __be32 status; >>> @@ -8840,6 +8840,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, >>> struct nfs4_delegation *dp; >>> struct iattr attrs; >>> struct nfs4_cb_fattr *ncf; >>> + struct inode *inode = d_inode(dentry); >>> >>> *modified = false; >>> ctx = locks_inode_context(inode); >>> @@ -8887,15 +8888,22 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, >>> ncf->ncf_cur_fsize != ncf->ncf_cb_fsize)) >>> ncf->ncf_file_modified = true; >>> if (ncf->ncf_file_modified) { >>> + int err; >>> + >>> /* >>> * Per section 10.4.3 of RFC 8881, the server would >>> * not update the file's metadata with the client's >>> * modified size >>> */ >>> attrs.ia_mtime = attrs.ia_ctime = current_time(inode); >>> - attrs.ia_valid = ATTR_MTIME | ATTR_CTIME; >>> - setattr_copy(&nop_mnt_idmap, inode, &attrs); >>> - mark_inode_dirty(inode); >>> + attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG; >>> + inode_lock(inode); >>> + err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL); >>> + inode_unlock(inode); >>> + if (err) { >>> + nfs4_put_stid(&dp->dl_stid); >>> + return nfserrno(err); >>> + } >>> ncf->ncf_cur_fsize = ncf->ncf_cb_fsize; >>> *size = ncf->ncf_cur_fsize; >>> *modified = true; >>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c >>> index 43ccf6119cf1..97f583777972 100644 >>> --- a/fs/nfsd/nfs4xdr.c >>> +++ b/fs/nfsd/nfs4xdr.c >>> @@ -3565,7 +3565,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr, >>> } >>> args.size = 0; >>> if (attrmask[0] & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) { >>> - status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry), >>> + status = nfsd4_deleg_getattr_conflict(rqstp, dentry, >>> &file_modified, &size); >>> if (status) >>> goto out; >>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h >>> index ffc217099d19..ec4559ecd193 100644 >>> --- a/fs/nfsd/state.h >>> +++ b/fs/nfsd/state.h >>> @@ -781,5 +781,5 @@ static inline bool try_to_expire_client(struct nfs4_client *clp) >>> } >>> >>> extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, >>> - struct inode *inode, bool *file_modified, u64 *size); >>> + struct dentry *dentry, bool *file_modified, u64 *size); >>> #endif /* NFSD4_STATE_H */ >>> diff --git a/include/linux/fs.h b/include/linux/fs.h >>> index 0283cf366c2a..3fe289c74869 100644 >>> --- a/include/linux/fs.h >>> +++ b/include/linux/fs.h >>> @@ -208,6 +208,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, >>> #define ATTR_OPEN (1 << 15) /* Truncating from open(O_TRUNC) */ >>> #define ATTR_TIMES_SET (1 << 16) >>> #define ATTR_TOUCH (1 << 17) >>> +#define ATTR_DELEG (1 << 18) /* Delegated attrs (don't break) */ >>> >>> /* >>> * Whiteout is represented by a char device. The following constants define the >>> >>> --- >>> base-commit: a204501e1743d695ca2930ed25a2be9f8ced96d3 >>> change-id: 20240823-nfsd-fixes-61f0c785d125 >>> >>> Best regards,
diff --git a/fs/attr.c b/fs/attr.c index 960a310581eb..a40a2fb406f0 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -489,9 +489,12 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry, error = security_inode_setattr(idmap, dentry, attr); if (error) return error; - error = try_break_deleg(inode, delegated_inode); - if (error) - return error; + + if (!(ia_valid & ATTR_DELEG)) { + error = try_break_deleg(inode, delegated_inode); + if (error) + return error; + } if (inode->i_op->setattr) error = inode->i_op->setattr(idmap, dentry, attr); diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index dafff707e23a..e0e3d3ca0d45 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -8815,7 +8815,7 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate, /** * nfsd4_deleg_getattr_conflict - Recall if GETATTR causes conflict * @rqstp: RPC transaction context - * @inode: file to be checked for a conflict + * @dentry: dentry of inode to be checked for a conflict * @modified: return true if file was modified * @size: new size of file if modified is true * @@ -8830,7 +8830,7 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate, * code is returned. */ __be32 -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry, bool *modified, u64 *size) { __be32 status; @@ -8840,6 +8840,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, struct nfs4_delegation *dp; struct iattr attrs; struct nfs4_cb_fattr *ncf; + struct inode *inode = d_inode(dentry); *modified = false; ctx = locks_inode_context(inode); @@ -8887,15 +8888,22 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, ncf->ncf_cur_fsize != ncf->ncf_cb_fsize)) ncf->ncf_file_modified = true; if (ncf->ncf_file_modified) { + int err; + /* * Per section 10.4.3 of RFC 8881, the server would * not update the file's metadata with the client's * modified size */ attrs.ia_mtime = attrs.ia_ctime = current_time(inode); - attrs.ia_valid = ATTR_MTIME | ATTR_CTIME; - setattr_copy(&nop_mnt_idmap, inode, &attrs); - mark_inode_dirty(inode); + attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG; + inode_lock(inode); + err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL); + inode_unlock(inode); + if (err) { + nfs4_put_stid(&dp->dl_stid); + return nfserrno(err); + } ncf->ncf_cur_fsize = ncf->ncf_cb_fsize; *size = ncf->ncf_cur_fsize; *modified = true; diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index 43ccf6119cf1..97f583777972 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -3565,7 +3565,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr, } args.size = 0; if (attrmask[0] & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) { - status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry), + status = nfsd4_deleg_getattr_conflict(rqstp, dentry, &file_modified, &size); if (status) goto out; diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h index ffc217099d19..ec4559ecd193 100644 --- a/fs/nfsd/state.h +++ b/fs/nfsd/state.h @@ -781,5 +781,5 @@ static inline bool try_to_expire_client(struct nfs4_client *clp) } extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, - struct inode *inode, bool *file_modified, u64 *size); + struct dentry *dentry, bool *file_modified, u64 *size); #endif /* NFSD4_STATE_H */ diff --git a/include/linux/fs.h b/include/linux/fs.h index 0283cf366c2a..3fe289c74869 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -208,6 +208,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, #define ATTR_OPEN (1 << 15) /* Truncating from open(O_TRUNC) */ #define ATTR_TIMES_SET (1 << 16) #define ATTR_TOUCH (1 << 17) +#define ATTR_DELEG (1 << 18) /* Delegated attrs (don't break) */ /* * Whiteout is represented by a char device. The following constants define the
Currently, we copy the mtime and ctime to the in-core inode and then mark the inode dirty. This is fine for certain types of filesystems, but not all. Some require a real setattr to properly change these values (e.g. ceph or reexported NFS). Fix this code to call notify_change() instead, which is the proper way to effect a setattr. There is one problem though: In this case, the client is holding a write delegation and has sent us attributes to update our cache. We don't want to break the delegation for this since that would defeat the purpose. Add a new ATTR_DELEG flag that makes notify_change bypass the try_break_deleg call. Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") Signed-off-by: Jeff Layton <jlayton@kernel.org> --- One more CB_GETATTR fix. This one involves a little change at the VFS layer to avoid breaking the delegation. Christian, unless you have objections, this should probably go in via Chuck's tree as this patch depends on a nfsd patch [1] that I sent yesterday. An A-b or R-b would be welcome though. [1]: https://lore.kernel.org/linux-nfs/20240823-nfsd-fixes-v1-1-fc99aa16f6a0@kernel.org/T/#u --- fs/attr.c | 9 ++++++--- fs/nfsd/nfs4state.c | 18 +++++++++++++----- fs/nfsd/nfs4xdr.c | 2 +- fs/nfsd/state.h | 2 +- include/linux/fs.h | 1 + 5 files changed, 22 insertions(+), 10 deletions(-) --- base-commit: a204501e1743d695ca2930ed25a2be9f8ced96d3 change-id: 20240823-nfsd-fixes-61f0c785d125 Best regards,