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

[Christian Brauner <brauner@kernel.org>]

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>