diff mbox series

[v9,07/19] nfs/localio: fix nfs_localio_vfs_getattr() to properly support v4

Message ID 20240628211105.54736-8-snitzer@kernel.org (mailing list archive)
State New
Headers show
Series nfs/nfsd: add support for localio | expand

Commit Message

Mike Snitzer June 28, 2024, 9:10 p.m. UTC
This is nfs-localio code which blurs the boundary between server and
client...

The change_attr is used by NFS to detect if a file might have changed.
This code is used to get the attributes after a write request.  NFS
uses a GETATTR request to the server at other times.  The change_attr
should be consistent between the two else comparisons will be
meaningless.

So nfs_localio_vfs_getattr() should use the same change_attr as the
one that would be used if the NFS GETATTR request were made.  For
NFSv3, that is nfs_timespec_to_change_attr() as was already
implemented.  For NFSv4 it is something different (as implemented in
this commit).

[above header derived from linux-nfs message Neil sent on this topic]
Suggested-by: NeilBrown <neil@brown.name>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfs/localio.c | 48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

Comments

Chuck Lever III June 29, 2024, 3:50 p.m. UTC | #1
On Fri, Jun 28, 2024 at 05:10:53PM -0400, Mike Snitzer wrote:
> This is nfs-localio code which blurs the boundary between server and
> client...
> 
> The change_attr is used by NFS to detect if a file might have changed.
> This code is used to get the attributes after a write request.  NFS
> uses a GETATTR request to the server at other times.  The change_attr
> should be consistent between the two else comparisons will be
> meaningless.
> 
> So nfs_localio_vfs_getattr() should use the same change_attr as the
> one that would be used if the NFS GETATTR request were made.  For
> NFSv3, that is nfs_timespec_to_change_attr() as was already
> implemented.  For NFSv4 it is something different (as implemented in
> this commit).
> 
> [above header derived from linux-nfs message Neil sent on this topic]

Instead of this note, I recommend:

Message-Id: <171918165963.14261.959545364150864599@noble.neil.brown.name>


> Suggested-by: NeilBrown <neil@brown.name>
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> ---
>  fs/nfs/localio.c | 48 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> index 0f7d6d55087b..fe96f05ba8ca 100644
> --- a/fs/nfs/localio.c
> +++ b/fs/nfs/localio.c
> @@ -364,21 +364,47 @@ nfs_set_local_verifier(struct inode *inode,
>  	verf->committed = how;
>  }
>  
> +/* Factored out from fs/nfsd/vfs.h:fh_getattr() */
> +static int __vfs_getattr(struct path *p, struct kstat *stat, int version)
> +{
> +	u32 request_mask = STATX_BASIC_STATS;
> +
> +	if (version == 4)
> +		request_mask |= (STATX_BTIME | STATX_CHANGE_COOKIE);
> +	return vfs_getattr(p, stat, request_mask, AT_STATX_SYNC_AS_STAT);
> +}
> +
> +/*
> + * Copied from fs/nfsd/nfsfh.c:nfsd4_change_attribute(),
> + * FIXME: factor out to common code.
> + */
> +static u64 __nfsd4_change_attribute(const struct kstat *stat,
> +				    const struct inode *inode)
> +{
> +	u64 chattr;
> +
> +	if (stat->result_mask & STATX_CHANGE_COOKIE) {
> +		chattr = stat->change_cookie;
> +		if (S_ISREG(inode->i_mode) &&
> +		    !(stat->attributes & STATX_ATTR_CHANGE_MONOTONIC)) {
> +			chattr += (u64)stat->ctime.tv_sec << 30;
> +			chattr += stat->ctime.tv_nsec;
> +		}
> +	} else {
> +		chattr = time_to_chattr(&stat->ctime);
> +	}
> +	return chattr;
> +}
> +
>  static void nfs_local_vfs_getattr(struct nfs_local_kiocb *iocb)
>  {
>  	struct kstat stat;
>  	struct file *filp = iocb->kiocb.ki_filp;
>  	struct nfs_pgio_header *hdr = iocb->hdr;
>  	struct nfs_fattr *fattr = hdr->res.fattr;
> +	int version = NFS_PROTO(hdr->inode)->version;
>  
> -	if (unlikely(!fattr) || vfs_getattr(&filp->f_path, &stat,
> -					    STATX_INO |
> -					    STATX_ATIME |
> -					    STATX_MTIME |
> -					    STATX_CTIME |
> -					    STATX_SIZE |
> -					    STATX_BLOCKS,
> -					    AT_STATX_SYNC_AS_STAT))
> +	if (unlikely(!fattr) || __vfs_getattr(&filp->f_path, &stat, version))
>  		return;
>  
>  	fattr->valid = (NFS_ATTR_FATTR_FILEID |
> @@ -394,7 +420,11 @@ static void nfs_local_vfs_getattr(struct nfs_local_kiocb *iocb)
>  	fattr->atime = stat.atime;
>  	fattr->mtime = stat.mtime;
>  	fattr->ctime = stat.ctime;
> -	fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime);
> +	if (version == 4) {
> +		fattr->change_attr =
> +			__nfsd4_change_attribute(&stat, file_inode(filp));
> +	} else
> +		fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime);
>  	fattr->du.nfs3.used = stat.blocks << 9;
>  }
>  
> -- 
> 2.44.0
>
NeilBrown June 30, 2024, 10:01 p.m. UTC | #2
On Sun, 30 Jun 2024, Chuck Lever wrote:
> On Fri, Jun 28, 2024 at 05:10:53PM -0400, Mike Snitzer wrote:
> > This is nfs-localio code which blurs the boundary between server and
> > client...
> > 
> > The change_attr is used by NFS to detect if a file might have changed.
> > This code is used to get the attributes after a write request.  NFS
> > uses a GETATTR request to the server at other times.  The change_attr
> > should be consistent between the two else comparisons will be
> > meaningless.
> > 
> > So nfs_localio_vfs_getattr() should use the same change_attr as the
> > one that would be used if the NFS GETATTR request were made.  For
> > NFSv3, that is nfs_timespec_to_change_attr() as was already
> > implemented.  For NFSv4 it is something different (as implemented in
> > this commit).
> > 
> > [above header derived from linux-nfs message Neil sent on this topic]
> 
> Instead of this note, I recommend:
> 
> Message-Id: <171918165963.14261.959545364150864599@noble.neil.brown.name>

Linus would not be impressed.  He likes links that you can click on and
follow.
So
   Link: https://lore.kernel.org/171918165963.14261.959545364150864599@noble.neil.brown.name

is preferred (at least I think that is the current state of the
conversation

see https://lore.kernel.org/all/CAHk-=wiD9du3fBHuLYzwUSdNgY+hxMZEWNZpqJXy-=wD2wafdg@mail.gmail.com/

NeilBrown

> 
> 
> > Suggested-by: NeilBrown <neil@brown.name>
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  fs/nfs/localio.c | 48 +++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 39 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> > index 0f7d6d55087b..fe96f05ba8ca 100644
> > --- a/fs/nfs/localio.c
> > +++ b/fs/nfs/localio.c
> > @@ -364,21 +364,47 @@ nfs_set_local_verifier(struct inode *inode,
> >  	verf->committed = how;
> >  }
> >  
> > +/* Factored out from fs/nfsd/vfs.h:fh_getattr() */
> > +static int __vfs_getattr(struct path *p, struct kstat *stat, int version)
> > +{
> > +	u32 request_mask = STATX_BASIC_STATS;
> > +
> > +	if (version == 4)
> > +		request_mask |= (STATX_BTIME | STATX_CHANGE_COOKIE);
> > +	return vfs_getattr(p, stat, request_mask, AT_STATX_SYNC_AS_STAT);
> > +}
> > +
> > +/*
> > + * Copied from fs/nfsd/nfsfh.c:nfsd4_change_attribute(),
> > + * FIXME: factor out to common code.
> > + */
> > +static u64 __nfsd4_change_attribute(const struct kstat *stat,
> > +				    const struct inode *inode)
> > +{
> > +	u64 chattr;
> > +
> > +	if (stat->result_mask & STATX_CHANGE_COOKIE) {
> > +		chattr = stat->change_cookie;
> > +		if (S_ISREG(inode->i_mode) &&
> > +		    !(stat->attributes & STATX_ATTR_CHANGE_MONOTONIC)) {
> > +			chattr += (u64)stat->ctime.tv_sec << 30;
> > +			chattr += stat->ctime.tv_nsec;
> > +		}
> > +	} else {
> > +		chattr = time_to_chattr(&stat->ctime);
> > +	}
> > +	return chattr;
> > +}
> > +
> >  static void nfs_local_vfs_getattr(struct nfs_local_kiocb *iocb)
> >  {
> >  	struct kstat stat;
> >  	struct file *filp = iocb->kiocb.ki_filp;
> >  	struct nfs_pgio_header *hdr = iocb->hdr;
> >  	struct nfs_fattr *fattr = hdr->res.fattr;
> > +	int version = NFS_PROTO(hdr->inode)->version;
> >  
> > -	if (unlikely(!fattr) || vfs_getattr(&filp->f_path, &stat,
> > -					    STATX_INO |
> > -					    STATX_ATIME |
> > -					    STATX_MTIME |
> > -					    STATX_CTIME |
> > -					    STATX_SIZE |
> > -					    STATX_BLOCKS,
> > -					    AT_STATX_SYNC_AS_STAT))
> > +	if (unlikely(!fattr) || __vfs_getattr(&filp->f_path, &stat, version))
> >  		return;
> >  
> >  	fattr->valid = (NFS_ATTR_FATTR_FILEID |
> > @@ -394,7 +420,11 @@ static void nfs_local_vfs_getattr(struct nfs_local_kiocb *iocb)
> >  	fattr->atime = stat.atime;
> >  	fattr->mtime = stat.mtime;
> >  	fattr->ctime = stat.ctime;
> > -	fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime);
> > +	if (version == 4) {
> > +		fattr->change_attr =
> > +			__nfsd4_change_attribute(&stat, file_inode(filp));
> > +	} else
> > +		fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime);
> >  	fattr->du.nfs3.used = stat.blocks << 9;
> >  }
> >  
> > -- 
> > 2.44.0
> > 
> 
> -- 
> Chuck Lever
>
Chuck Lever III June 30, 2024, 10:23 p.m. UTC | #3
On Mon, Jul 01, 2024 at 08:01:30AM +1000, NeilBrown wrote:
> On Sun, 30 Jun 2024, Chuck Lever wrote:
> > On Fri, Jun 28, 2024 at 05:10:53PM -0400, Mike Snitzer wrote:
> > > This is nfs-localio code which blurs the boundary between server and
> > > client...
> > > 
> > > The change_attr is used by NFS to detect if a file might have changed.
> > > This code is used to get the attributes after a write request.  NFS
> > > uses a GETATTR request to the server at other times.  The change_attr
> > > should be consistent between the two else comparisons will be
> > > meaningless.
> > > 
> > > So nfs_localio_vfs_getattr() should use the same change_attr as the
> > > one that would be used if the NFS GETATTR request were made.  For
> > > NFSv3, that is nfs_timespec_to_change_attr() as was already
> > > implemented.  For NFSv4 it is something different (as implemented in
> > > this commit).
> > > 
> > > [above header derived from linux-nfs message Neil sent on this topic]
> > 
> > Instead of this note, I recommend:
> > 
> > Message-Id: <171918165963.14261.959545364150864599@noble.neil.brown.name>
> 
> Linus would not be impressed.  He likes links that you can click on and
> follow.

I've read email that suggests he doesn't like those either. Another
day ending in "y", I guess.


> So
>    Link: https://lore.kernel.org/171918165963.14261.959545364150864599@noble.neil.brown.name
> 
> is preferred (at least I think that is the current state of the
> conversation
> 
> see https://lore.kernel.org/all/CAHk-=wiD9du3fBHuLYzwUSdNgY+hxMZEWNZpqJXy-=wD2wafdg@mail.gmail.com/

As I read it, this refers to using Message-Id: to link to a patch
submission. I'm linking to a discussion thread, not to a patch.
Just to be clear.


> NeilBrown
> 
> > 
> > 
> > > Suggested-by: NeilBrown <neil@brown.name>
> > > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > > ---
> > >  fs/nfs/localio.c | 48 +++++++++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 39 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> > > index 0f7d6d55087b..fe96f05ba8ca 100644
> > > --- a/fs/nfs/localio.c
> > > +++ b/fs/nfs/localio.c
> > > @@ -364,21 +364,47 @@ nfs_set_local_verifier(struct inode *inode,
> > >  	verf->committed = how;
> > >  }
> > >  
> > > +/* Factored out from fs/nfsd/vfs.h:fh_getattr() */
> > > +static int __vfs_getattr(struct path *p, struct kstat *stat, int version)
> > > +{
> > > +	u32 request_mask = STATX_BASIC_STATS;
> > > +
> > > +	if (version == 4)
> > > +		request_mask |= (STATX_BTIME | STATX_CHANGE_COOKIE);
> > > +	return vfs_getattr(p, stat, request_mask, AT_STATX_SYNC_AS_STAT);
> > > +}
> > > +
> > > +/*
> > > + * Copied from fs/nfsd/nfsfh.c:nfsd4_change_attribute(),
> > > + * FIXME: factor out to common code.
> > > + */
> > > +static u64 __nfsd4_change_attribute(const struct kstat *stat,
> > > +				    const struct inode *inode)
> > > +{
> > > +	u64 chattr;
> > > +
> > > +	if (stat->result_mask & STATX_CHANGE_COOKIE) {
> > > +		chattr = stat->change_cookie;
> > > +		if (S_ISREG(inode->i_mode) &&
> > > +		    !(stat->attributes & STATX_ATTR_CHANGE_MONOTONIC)) {
> > > +			chattr += (u64)stat->ctime.tv_sec << 30;
> > > +			chattr += stat->ctime.tv_nsec;
> > > +		}
> > > +	} else {
> > > +		chattr = time_to_chattr(&stat->ctime);
> > > +	}
> > > +	return chattr;
> > > +}
> > > +
> > >  static void nfs_local_vfs_getattr(struct nfs_local_kiocb *iocb)
> > >  {
> > >  	struct kstat stat;
> > >  	struct file *filp = iocb->kiocb.ki_filp;
> > >  	struct nfs_pgio_header *hdr = iocb->hdr;
> > >  	struct nfs_fattr *fattr = hdr->res.fattr;
> > > +	int version = NFS_PROTO(hdr->inode)->version;
> > >  
> > > -	if (unlikely(!fattr) || vfs_getattr(&filp->f_path, &stat,
> > > -					    STATX_INO |
> > > -					    STATX_ATIME |
> > > -					    STATX_MTIME |
> > > -					    STATX_CTIME |
> > > -					    STATX_SIZE |
> > > -					    STATX_BLOCKS,
> > > -					    AT_STATX_SYNC_AS_STAT))
> > > +	if (unlikely(!fattr) || __vfs_getattr(&filp->f_path, &stat, version))
> > >  		return;
> > >  
> > >  	fattr->valid = (NFS_ATTR_FATTR_FILEID |
> > > @@ -394,7 +420,11 @@ static void nfs_local_vfs_getattr(struct nfs_local_kiocb *iocb)
> > >  	fattr->atime = stat.atime;
> > >  	fattr->mtime = stat.mtime;
> > >  	fattr->ctime = stat.ctime;
> > > -	fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime);
> > > +	if (version == 4) {
> > > +		fattr->change_attr =
> > > +			__nfsd4_change_attribute(&stat, file_inode(filp));
> > > +	} else
> > > +		fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime);
> > >  	fattr->du.nfs3.used = stat.blocks << 9;
> > >  }
> > >  
> > > -- 
> > > 2.44.0
> > > 
> > 
> > -- 
> > Chuck Lever
> > 
>
diff mbox series

Patch

diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
index 0f7d6d55087b..fe96f05ba8ca 100644
--- a/fs/nfs/localio.c
+++ b/fs/nfs/localio.c
@@ -364,21 +364,47 @@  nfs_set_local_verifier(struct inode *inode,
 	verf->committed = how;
 }
 
+/* Factored out from fs/nfsd/vfs.h:fh_getattr() */
+static int __vfs_getattr(struct path *p, struct kstat *stat, int version)
+{
+	u32 request_mask = STATX_BASIC_STATS;
+
+	if (version == 4)
+		request_mask |= (STATX_BTIME | STATX_CHANGE_COOKIE);
+	return vfs_getattr(p, stat, request_mask, AT_STATX_SYNC_AS_STAT);
+}
+
+/*
+ * Copied from fs/nfsd/nfsfh.c:nfsd4_change_attribute(),
+ * FIXME: factor out to common code.
+ */
+static u64 __nfsd4_change_attribute(const struct kstat *stat,
+				    const struct inode *inode)
+{
+	u64 chattr;
+
+	if (stat->result_mask & STATX_CHANGE_COOKIE) {
+		chattr = stat->change_cookie;
+		if (S_ISREG(inode->i_mode) &&
+		    !(stat->attributes & STATX_ATTR_CHANGE_MONOTONIC)) {
+			chattr += (u64)stat->ctime.tv_sec << 30;
+			chattr += stat->ctime.tv_nsec;
+		}
+	} else {
+		chattr = time_to_chattr(&stat->ctime);
+	}
+	return chattr;
+}
+
 static void nfs_local_vfs_getattr(struct nfs_local_kiocb *iocb)
 {
 	struct kstat stat;
 	struct file *filp = iocb->kiocb.ki_filp;
 	struct nfs_pgio_header *hdr = iocb->hdr;
 	struct nfs_fattr *fattr = hdr->res.fattr;
+	int version = NFS_PROTO(hdr->inode)->version;
 
-	if (unlikely(!fattr) || vfs_getattr(&filp->f_path, &stat,
-					    STATX_INO |
-					    STATX_ATIME |
-					    STATX_MTIME |
-					    STATX_CTIME |
-					    STATX_SIZE |
-					    STATX_BLOCKS,
-					    AT_STATX_SYNC_AS_STAT))
+	if (unlikely(!fattr) || __vfs_getattr(&filp->f_path, &stat, version))
 		return;
 
 	fattr->valid = (NFS_ATTR_FATTR_FILEID |
@@ -394,7 +420,11 @@  static void nfs_local_vfs_getattr(struct nfs_local_kiocb *iocb)
 	fattr->atime = stat.atime;
 	fattr->mtime = stat.mtime;
 	fattr->ctime = stat.ctime;
-	fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime);
+	if (version == 4) {
+		fattr->change_attr =
+			__nfsd4_change_attribute(&stat, file_inode(filp));
+	} else
+		fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime);
 	fattr->du.nfs3.used = stat.blocks << 9;
 }