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

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