[PATCH] fs/nfsd: fix update of inode attrs in CB_GETATTR

[PATCH] fs/nfsd: fix update of inode attrs in CB_GETATTR

[Jeff Layton]

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(-)

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,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] fs/nfsd: fix update of inode attrs in CB_GETATTR

[Chuck Lever]

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

-- 
Chuck Lever

Re: [PATCH] fs/nfsd: fix update of inode attrs in CB_GETATTR

[Christian Brauner]

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>

Re: [PATCH] fs/nfsd: fix update of inode attrs in CB_GETATTR

[Jeff Layton]

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.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] fs/nfsd: fix update of inode attrs in CB_GETATTR

[Dai Ngo]

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,

Re: [PATCH] fs/nfsd: fix update of inode attrs in CB_GETATTR

[Jeff Layton]

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,

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] fs/nfsd: fix update of inode attrs in CB_GETATTR

[Dai Ngo]

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,