Re: [PATCH 1/1] nfsd: Add support for btime

[Chuck Lever III <chuck.lever@oracle.com>]

Hi Ondrej-

> On Jan 10, 2022, at 4:53 AM, Ondrej Valousek <ondrej.valousek.xm@renesas.com> wrote:
> 
> Hi Bruce/kernel maintainers,
> 
> Please help to submit the following patch into kernel.

This version is better, but there are still some issues (all
fixable). I'm guessing you will eventually want to submit more
than just this simple fix, so I'm going to give you feedback
instead of just fixing it myself. If I'm wrong about that,
let me know and I will apply this by hand and we can be done.


> Legal stuff: It is OK for Renesas to submit this code into kernel

Thanks for checking.

Regular contributors generally don't have to check each time
they submit, but rather they have "blanket permission" from
their legal team to submit to the Linux kernel... So if you'd
like to continue submitting, do check in with your management
and legal teams and make sure they understand what's what.


> - and besides, it Bruce's patch in fact as he told me what to do.

To indicate this, you might add:

Suggested-by: Bruce Fields <bfields@fieldses.org>


> Signed-off-by: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
> ---
> Short description:
> Adds support for "btime" fattr into nfsd

Our tools use the content of the Subject: line of the email
item as the short description. You don't need to add it
separately. The Subject: for this email is just right.


> Long Description:
> For filesystems that supports "btime" timestamp (i.e. most modern filesystems do) we share it via kernel nfsd. Btime support for NFS client has already been added by Trond recently

When submitting, this goes before the Signed-off-by above,
and it's typically wrapped at ~72 characters.


> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 5a93a5db4fb0..876b317a4cae 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2865,6 +2865,9 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>        err = vfs_getattr(&path, &stat, STATX_BASIC_STATS, AT_STATX_SYNC_AS_STAT);
>        if (err)
>                goto out_nfserr;
> +       if (!(stat.result_mask & STATX_BTIME))
> +               /* underlying FS does not offer btime so we can't share it */
> +               bmval1 &= ~FATTR4_WORD1_TIME_CREATE;
>        if ((bmval0 & (FATTR4_WORD0_FILES_AVAIL | FATTR4_WORD0_FILES_FREE |
>                        FATTR4_WORD0_FILES_TOTAL | FATTR4_WORD0_MAXNAME)) ||
>            (bmval1 & (FATTR4_WORD1_SPACE_AVAIL | FATTR4_WORD1_SPACE_FREE |

Your e-mail system changed the tabs to spaces, so this
diff doesn't apply using "patch". Have you tried sending
the change using "git send-email" ?

Again, if that's more than you feel like dealing with
right now, let me know and I'll apply by hand. Otherwise,
please try again using our preferred tooling. That makes
life for maintainers a little easier.


> @@ -3265,6 +3268,14 @@ 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);
>        }
> +       /* support for btime here */

I'd prefer if you removed this comment because it states
the obvious.


> +        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) {
>                struct kstat parent_stat;
>                u64 ino = stat.ino;
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 498e5a489826..5ef056ce7591 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -364,7 +364,7 @@ void                nfsd_lockd_shutdown(void);
>  | FATTR4_WORD1_OWNER          | FATTR4_WORD1_OWNER_GROUP  | FATTR4_WORD1_RAWDEV           \
>  | FATTR4_WORD1_SPACE_AVAIL     | FATTR4_WORD1_SPACE_FREE   | FATTR4_WORD1_SPACE_TOTAL      \
>  | FATTR4_WORD1_SPACE_USED      | FATTR4_WORD1_TIME_ACCESS  | FATTR4_WORD1_TIME_ACCESS_SET  \
> - | FATTR4_WORD1_TIME_DELTA   | FATTR4_WORD1_TIME_METADATA    \
> + | FATTR4_WORD1_TIME_DELTA   | FATTR4_WORD1_TIME_METADATA   | FATTR4_WORD1_TIME_CREATE      \
>  | FATTR4_WORD1_TIME_MODIFY     | FATTR4_WORD1_TIME_MODIFY_SET | FATTR4_WORD1_MOUNTED_ON_FILEID)
> 
> #define NFSD4_SUPPORTED_ATTRS_WORD2 0

--
Chuck Lever