| Message ID | 20240628211105.54736-8-snitzer@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfs/nfsd: add support for localio | expand |
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 >
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 >
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 --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; }
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(-)