| Message ID | 4154eb6503988c2096f5d30af26d06863fb5d973.1687554467.git.tavianator@tavianator.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs/nfsd: Fix creation time serialization order | expand |
On Fri, Jun 23, 2023 at 05:09:06PM -0400, tavianator@tavianator.com wrote: > From: Tavian Barnes <tavianator@tavianator.com> > > In nfsd4_encode_fattr(), TIME_CREATE was being written out after all > other times. However, they should be written out in an order that > matches the bit flags in bmval1, which in this case are > > #define FATTR4_WORD1_TIME_ACCESS (1UL << 15) > #define FATTR4_WORD1_TIME_CREATE (1UL << 18) > #define FATTR4_WORD1_TIME_DELTA (1UL << 19) > #define FATTR4_WORD1_TIME_METADATA (1UL << 20) > #define FATTR4_WORD1_TIME_MODIFY (1UL << 21) > > so TIME_CREATE should come second. > > I noticed this on a FreeBSD NFSv4.2 client, which supports creation > times. On this client, file times were weirdly permuted. With this > patch applied on the server, times looked normal on the client. > > Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute") > Link: https://unix.stackexchange.com/q/749605/56202 > Signed-off-by: Tavian Barnes <tavianator@tavianator.com> I'm not especially familiar with this area of the protocol, but this looks correct at first glance. I've applied this to nfsd-fixes for v6.5. Out of interest, what type of filesystem does your server export? > --- > fs/nfsd/nfs4xdr.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index 76db2fe29624..3037c5b0623e 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -3354,6 +3354,13 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp, > p = xdr_encode_hyper(p, (s64)stat.atime.tv_sec); > *p++ = cpu_to_be32(stat.atime.tv_nsec); > } > + if (bmval1 & FATTR4_WORD1_TIME_CREATE) { > + p = xdr_reserve_space(xdr, 12); > + if (!p) > + goto out_resource; > + p = xdr_encode_hyper(p, (s64)stat.btime.tv_sec); > + *p++ = cpu_to_be32(stat.btime.tv_nsec); > + } > if (bmval1 & FATTR4_WORD1_TIME_DELTA) { > p = xdr_reserve_space(xdr, 12); > if (!p) > @@ -3374,13 +3381,6 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp, > p = xdr_encode_hyper(p, (s64)stat.mtime.tv_sec); > *p++ = cpu_to_be32(stat.mtime.tv_nsec); > } > - if (bmval1 & FATTR4_WORD1_TIME_CREATE) { > - p = xdr_reserve_space(xdr, 12); > - if (!p) > - goto out_resource; > - p = xdr_encode_hyper(p, (s64)stat.btime.tv_sec); > - *p++ = cpu_to_be32(stat.btime.tv_nsec); > - } > if (bmval1 & FATTR4_WORD1_MOUNTED_ON_FILEID) { > u64 ino = stat.ino; > > -- > 2.41.0 >
On Fri, Jun 23, 2023 at 5:48 PM Chuck Lever <cel@kernel.org> wrote: > On Fri, Jun 23, 2023 at 05:09:06PM -0400, tavianator@tavianator.com wrote: > > From: Tavian Barnes <tavianator@tavianator.com> > > > > In nfsd4_encode_fattr(), TIME_CREATE was being written out after all > > other times. However, they should be written out in an order that > > matches the bit flags in bmval1, which in this case are > > > > #define FATTR4_WORD1_TIME_ACCESS (1UL << 15) > > #define FATTR4_WORD1_TIME_CREATE (1UL << 18) > > #define FATTR4_WORD1_TIME_DELTA (1UL << 19) > > #define FATTR4_WORD1_TIME_METADATA (1UL << 20) > > #define FATTR4_WORD1_TIME_MODIFY (1UL << 21) > > > > so TIME_CREATE should come second. > > > > I noticed this on a FreeBSD NFSv4.2 client, which supports creation > > times. On this client, file times were weirdly permuted. With this > > patch applied on the server, times looked normal on the client. > > > > Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute") > > Link: https://unix.stackexchange.com/q/749605/56202 > > Signed-off-by: Tavian Barnes <tavianator@tavianator.com> > > I'm not especially familiar with this area of the protocol, but this > looks correct at first glance. I've applied this to nfsd-fixes for > v6.5. Great, thanks! > Out of interest, what type of filesystem does your server export? It's a btrfs filesystem.
On Fri, 2023-06-23 at 17:09 -0400, tavianator@tavianator.com wrote: > From: Tavian Barnes <tavianator@tavianator.com> > > In nfsd4_encode_fattr(), TIME_CREATE was being written out after all > other times. However, they should be written out in an order that > matches the bit flags in bmval1, which in this case are > > #define FATTR4_WORD1_TIME_ACCESS (1UL << 15) > #define FATTR4_WORD1_TIME_CREATE (1UL << 18) > #define FATTR4_WORD1_TIME_DELTA (1UL << 19) > #define FATTR4_WORD1_TIME_METADATA (1UL << 20) > #define FATTR4_WORD1_TIME_MODIFY (1UL << 21) > > so TIME_CREATE should come second. > > I noticed this on a FreeBSD NFSv4.2 client, which supports creation > times. On this client, file times were weirdly permuted. With this > patch applied on the server, times looked normal on the client. > > Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute") > Link: https://unix.stackexchange.com/q/749605/56202 > Signed-off-by: Tavian Barnes <tavianator@tavianator.com> > --- > fs/nfsd/nfs4xdr.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index 76db2fe29624..3037c5b0623e 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -3354,6 +3354,13 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp, > p = xdr_encode_hyper(p, (s64)stat.atime.tv_sec); > *p++ = cpu_to_be32(stat.atime.tv_nsec); > } > + if (bmval1 & FATTR4_WORD1_TIME_CREATE) { > + p = xdr_reserve_space(xdr, 12); > + if (!p) > + goto out_resource; > + p = xdr_encode_hyper(p, (s64)stat.btime.tv_sec); > + *p++ = cpu_to_be32(stat.btime.tv_nsec); > + } > if (bmval1 & FATTR4_WORD1_TIME_DELTA) { > p = xdr_reserve_space(xdr, 12); > if (!p) > @@ -3374,13 +3381,6 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp, > p = xdr_encode_hyper(p, (s64)stat.mtime.tv_sec); > *p++ = cpu_to_be32(stat.mtime.tv_nsec); > } > - if (bmval1 & FATTR4_WORD1_TIME_CREATE) { > - p = xdr_reserve_space(xdr, 12); > - if (!p) > - goto out_resource; > - p = xdr_encode_hyper(p, (s64)stat.btime.tv_sec); > - *p++ = cpu_to_be32(stat.btime.tv_nsec); > - } > if (bmval1 & FATTR4_WORD1_MOUNTED_ON_FILEID) { > u64 ino = stat.ino; > Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index 76db2fe29624..3037c5b0623e 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -3354,6 +3354,13 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp, p = xdr_encode_hyper(p, (s64)stat.atime.tv_sec); *p++ = cpu_to_be32(stat.atime.tv_nsec); } + if (bmval1 & FATTR4_WORD1_TIME_CREATE) { + p = xdr_reserve_space(xdr, 12); + if (!p) + goto out_resource; + p = xdr_encode_hyper(p, (s64)stat.btime.tv_sec); + *p++ = cpu_to_be32(stat.btime.tv_nsec); + } if (bmval1 & FATTR4_WORD1_TIME_DELTA) { p = xdr_reserve_space(xdr, 12); if (!p) @@ -3374,13 +3381,6 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp, p = xdr_encode_hyper(p, (s64)stat.mtime.tv_sec); *p++ = cpu_to_be32(stat.mtime.tv_nsec); } - if (bmval1 & FATTR4_WORD1_TIME_CREATE) { - p = xdr_reserve_space(xdr, 12); - if (!p) - goto out_resource; - p = xdr_encode_hyper(p, (s64)stat.btime.tv_sec); - *p++ = cpu_to_be32(stat.btime.tv_nsec); - } if (bmval1 & FATTR4_WORD1_MOUNTED_ON_FILEID) { u64 ino = stat.ino;