Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230110184726.13380-1-mora@netapp.com> <525FDD70-D00A-40DD-9C2A-71048F4D612E@oracle.com>
In-Reply-To: <525FDD70-D00A-40DD-9C2A-71048F4D612E@oracle.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Wed, 11 Jan 2023 09:15:45 -0500
Message-ID: <CAN-5tyG_aefN8x1bzif7CTnv3_b3qf2V2mWk3c-Uu7MamFR2oA@mail.gmail.com>
Subject: Re: [PATCH v1] NFSD: add IO_ADVISE operation
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Jorge Mora <jmora1300@gmail.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Jeff Layton <jlayton@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jan 10, 2023 at 2:04 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Jan 10, 2023, at 1:47 PM, Jorge Mora <jmora1300@gmail.com> wrote:
> >
> > If multiple bits are set, select just one using a predetermined
> > order of precedence. If there are no bits which correspond to
> > any of the advice values (POSIX_FADV_*), the server simply
> > replies with NFS4ERR_UNION_NOTSUPP.
> >
> > If a client sends a bitmap mask with multiple words, ignore all
> > but the first word in the bitmap. The response is always the
> > same first word of the bitmap mask given in the request.
>
> Hi Jorge-
>
> I'd rather not add this operation just because it happens to be
> missing. Is there a reason you need it?

The motivation was to implement the not implement and hope to serve as
a foundation for others to expand on and do something. Also because
typically if we put in a client piece support there is supposed to be
server side as well.

> Does it provide some
> kind of performance benefit, for instance? The patch description
> really does need to provide this kind of rationale, and hopefully
> some performance measurements.
>
> Do the POSIX_FADV_* settings map to behavior that a client can
> expect in other server implementations?

I thought the purpose of IO_ADVISE is to advise but not expect. If the
server wants to do something with the knowledge it can.

> That is, do you expect
>  the proposed implementation below to interoperate properly?
>
>
> > Signed-off-by: Jorge Mora <mora@netapp.com>
> > ---
> > fs/nfsd/nfs4proc.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
> > fs/nfsd/nfs4xdr.c  | 27 +++++++++++++++++++--
> > fs/nfsd/xdr4.h     | 11 +++++++++
> > 3 files changed, 95 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 8beb2bc4c328..65179a3946f5 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -35,6 +35,7 @@
> > #include <linux/fs_struct.h>
> > #include <linux/file.h>
> > #include <linux/falloc.h>
> > +#include <linux/fadvise.h>
> > #include <linux/slab.h>
> > #include <linux/kthread.h>
> > #include <linux/namei.h>
> > @@ -1995,6 +1996,53 @@ nfsd4_deallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >                              FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE);
> > }
> >
> > +static __be32
> > +nfsd4_io_advise(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > +             union nfsd4_op_u *u)
> > +{
> > +     struct nfsd4_io_advise *io_advise = &u->io_advise;
> > +     struct nfsd_file *nf;
> > +     __be32 status;
> > +     int advice;
> > +     int ret;
> > +
> > +     status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> > +                                         &io_advise->stateid,
> > +                                         RD_STATE, &nf, NULL);
> > +     if (status) {
> > +             dprintk("NFSD: %s: couldn't process stateid!\n", __func__);
> > +             return status;
> > +     }
> > +
> > +     /*
> > +      * If multiple bits are set, select just one using the following
> > +      * order of precedence
> > +      */
> > +     if (io_advise->hints & NFS_IO_ADVISE4_NORMAL)
> > +             advice = POSIX_FADV_NORMAL;
> > +     else if (io_advise->hints & NFS_IO_ADVISE4_RANDOM)
> > +             advice = POSIX_FADV_RANDOM;
> > +     else if (io_advise->hints & NFS_IO_ADVISE4_SEQUENTIAL)
> > +             advice = POSIX_FADV_SEQUENTIAL;
> > +     else if (io_advise->hints & NFS_IO_ADVISE4_WILLNEED)
> > +             advice = POSIX_FADV_WILLNEED;
> > +     else if (io_advise->hints & NFS_IO_ADVISE4_DONTNEED)
> > +             advice = POSIX_FADV_DONTNEED;
> > +     else if (io_advise->hints & NFS_IO_ADVISE4_NOREUSE)
> > +             advice = POSIX_FADV_NOREUSE;
> > +     else {
> > +             status = nfserr_union_notsupp;
> > +             goto out;
> > +     }
> > +
> > +     ret = vfs_fadvise(nf->nf_file, io_advise->offset, io_advise->count, advice);
> > +     if (ret < 0)
> > +             status = nfserrno(ret);
> > +out:
> > +     nfsd_file_put(nf);
> > +     return status;
> > +}
> > +
> > static __be32
> > nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >          union nfsd4_op_u *u)
> > @@ -3096,6 +3144,12 @@ static u32 nfsd4_layoutreturn_rsize(const struct svc_rqst *rqstp,
> > #endif /* CONFIG_NFSD_PNFS */
> >
> >
> > +static u32 nfsd4_io_advise_rsize(const struct svc_rqst *rqstp,
> > +                              const struct nfsd4_op *op)
> > +{
> > +     return (op_encode_hdr_size + 2) * sizeof(__be32);
> > +}
> > +
> > static u32 nfsd4_seek_rsize(const struct svc_rqst *rqstp,
> >                           const struct nfsd4_op *op)
> > {
> > @@ -3479,6 +3533,11 @@ static const struct nfsd4_operation nfsd4_ops[] = {
> >               .op_name = "OP_DEALLOCATE",
> >               .op_rsize_bop = nfsd4_only_status_rsize,
> >       },
> > +     [OP_IO_ADVISE] = {
> > +             .op_func = nfsd4_io_advise,
> > +             .op_name = "OP_IO_ADVISE",
> > +             .op_rsize_bop = nfsd4_io_advise_rsize,
> > +     },
> >       [OP_CLONE] = {
> >               .op_func = nfsd4_clone,
> >               .op_flags = OP_MODIFIES_SOMETHING,
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index bcfeb1a922c0..8814c24047ff 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -1882,6 +1882,22 @@ nfsd4_decode_fallocate(struct nfsd4_compoundargs *argp,
> >       return nfs_ok;
> > }
> >
> > +static __be32
> > +nfsd4_decode_io_advise(struct nfsd4_compoundargs *argp,
> > +                    struct nfsd4_io_advise *io_advise)
> > +{
> > +     __be32 status;
> > +
> > +     status = nfsd4_decode_stateid4(argp, &io_advise->stateid);
> > +     if (status)
> > +             return status;
> > +     if (xdr_stream_decode_u64(argp->xdr, &io_advise->offset) < 0)
> > +             return nfserr_bad_xdr;
> > +     if (xdr_stream_decode_u64(argp->xdr, &io_advise->count) < 0)
> > +             return nfserr_bad_xdr;
> > +     return nfsd4_decode_bitmap4(argp, &io_advise->hints, 1);
> > +}
> > +
> > static __be32 nfsd4_decode_nl4_server(struct nfsd4_compoundargs *argp,
> >                                     struct nl4_server *ns)
> > {
> > @@ -2338,7 +2354,7 @@ static const nfsd4_dec nfsd4_dec_ops[] = {
> >       [OP_COPY]               = (nfsd4_dec)nfsd4_decode_copy,
> >       [OP_COPY_NOTIFY]        = (nfsd4_dec)nfsd4_decode_copy_notify,
> >       [OP_DEALLOCATE]         = (nfsd4_dec)nfsd4_decode_fallocate,
> > -     [OP_IO_ADVISE]          = (nfsd4_dec)nfsd4_decode_notsupp,
> > +     [OP_IO_ADVISE]          = (nfsd4_dec)nfsd4_decode_io_advise,
> >       [OP_LAYOUTERROR]        = (nfsd4_dec)nfsd4_decode_notsupp,
> >       [OP_LAYOUTSTATS]        = (nfsd4_dec)nfsd4_decode_notsupp,
> >       [OP_OFFLOAD_CANCEL]     = (nfsd4_dec)nfsd4_decode_offload_status,
> > @@ -4948,6 +4964,13 @@ nfsd4_encode_copy_notify(struct nfsd4_compoundres *resp, __be32 nfserr,
> >       return nfserr;
> > }
> >
> > +static __be32
> > +nfsd4_encode_io_advise(struct nfsd4_compoundres *resp, __be32 nfserr,
> > +                    struct nfsd4_io_advise *io_advise)
> > +{
> > +     return nfsd4_encode_bitmap(resp->xdr, io_advise->hints, 0, 0);
> > +}
> > +
> > static __be32
> > nfsd4_encode_seek(struct nfsd4_compoundres *resp, __be32 nfserr,
> >                 struct nfsd4_seek *seek)
> > @@ -5282,7 +5305,7 @@ static const nfsd4_enc nfsd4_enc_ops[] = {
> >       [OP_COPY]               = (nfsd4_enc)nfsd4_encode_copy,
> >       [OP_COPY_NOTIFY]        = (nfsd4_enc)nfsd4_encode_copy_notify,
> >       [OP_DEALLOCATE]         = (nfsd4_enc)nfsd4_encode_noop,
> > -     [OP_IO_ADVISE]          = (nfsd4_enc)nfsd4_encode_noop,
> > +     [OP_IO_ADVISE]          = (nfsd4_enc)nfsd4_encode_io_advise,
> >       [OP_LAYOUTERROR]        = (nfsd4_enc)nfsd4_encode_noop,
> >       [OP_LAYOUTSTATS]        = (nfsd4_enc)nfsd4_encode_noop,
> >       [OP_OFFLOAD_CANCEL]     = (nfsd4_enc)nfsd4_encode_noop,
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index 0eb00105d845..9b8ba4d6e3ab 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -518,6 +518,16 @@ struct nfsd4_fallocate {
> >       u64             falloc_length;
> > };
> >
> > +struct nfsd4_io_advise {
> > +     /* request */
> > +     stateid_t       stateid;
> > +     loff_t          offset;
> > +     u64             count;
> > +
> > +     /* both */
> > +     u32             hints;
> > +};
> > +
> > struct nfsd4_clone {
> >       /* request */
> >       stateid_t       cl_src_stateid;
> > @@ -688,6 +698,7 @@ struct nfsd4_op {
> >               /* NFSv4.2 */
> >               struct nfsd4_fallocate          allocate;
> >               struct nfsd4_fallocate          deallocate;
> > +             struct nfsd4_io_advise          io_advise;
> >               struct nfsd4_clone              clone;
> >               struct nfsd4_copy               copy;
> >               struct nfsd4_offload_status     offload_status;
> > --
> > 2.31.1
> >
>
> --
> Chuck Lever
>
>
>