| Message ID | 20200110223455.528471-8-Anna.Schumaker@Netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFS: Add support for the v4.2 READ_PLUS operation | expand |
Hi Anna- > On Jan 10, 2020, at 5:34 PM, schumaker.anna@gmail.com wrote: > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > There are some workloads where READ_PLUS might end up hurting > performance, Can you say more about this? Have you seen workloads that are hurt by READ_PLUS? Nothing jumps out at me from the tables in the cover letter. > so let's be nice to users and provide a way to disable this > operation similar to how READDIR_PLUS can be disabled. Does it make sense to hold off on a mount option until there is evidence that there is no other way to work around such a performance regression? - Attempt to address the regression directly - Improve the heuristics about when READ_PLUS is used - Document that dropping back to vers=4.1 will disable it Any experience with NFS/RDMA? > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > --- > fs/nfs/fs_context.c | 14 ++++++++++++++ > fs/nfs/nfs4client.c | 3 +++ > include/linux/nfs_fs_sb.h | 1 + > 3 files changed, 18 insertions(+) > > diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c > index 0247dcb7b316..82ba07c7c1ce 100644 > --- a/fs/nfs/fs_context.c > +++ b/fs/nfs/fs_context.c > @@ -64,6 +64,7 @@ enum nfs_param { > Opt_proto, > Opt_rdirplus, > Opt_rdma, > + Opt_readplus, > Opt_resvport, > Opt_retrans, > Opt_retry, > @@ -120,6 +121,7 @@ static const struct fs_parameter_spec nfs_param_specs[] = { > fsparam_string("proto", Opt_proto), > fsparam_flag_no("rdirplus", Opt_rdirplus), > fsparam_flag ("rdma", Opt_rdma), > + fsparam_flag_no("readplus", Opt_readplus), > fsparam_flag_no("resvport", Opt_resvport), > fsparam_u32 ("retrans", Opt_retrans), > fsparam_string("retry", Opt_retry), > @@ -555,6 +557,12 @@ static int nfs_fs_context_parse_param(struct fs_context *fc, > else > ctx->options |= NFS_OPTION_MIGRATION; > break; > + case Opt_readplus: > + if (result.negated) > + ctx->options |= NFS_OPTION_NO_READ_PLUS; > + else > + ctx->options &= ~NFS_OPTION_NO_READ_PLUS; > + break; > > /* > * options that take numeric values > @@ -1176,6 +1184,10 @@ static int nfs_fs_context_validate(struct fs_context *fc) > (ctx->version != 4 || ctx->minorversion != 0)) > goto out_migration_misuse; > > + if (ctx->options & NFS_OPTION_NO_READ_PLUS && > + (ctx->version != 4 || ctx->minorversion < 2)) > + goto out_noreadplus_misuse; > + > /* Verify that any proto=/mountproto= options match the address > * families in the addr=/mountaddr= options. > */ > @@ -1254,6 +1266,8 @@ static int nfs_fs_context_validate(struct fs_context *fc) > ctx->version, ctx->minorversion); > out_migration_misuse: > return nfs_invalf(fc, "NFS: 'Migration' not supported for this NFS version"); > +out_noreadplus_misuse: > + return nfs_invalf(fc, "NFS: 'noreadplus' not supported for this NFS version\n"); > out_version_unavailable: > nfs_errorf(fc, "NFS: Version unavailable"); > return ret; > diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c > index 0cd767e5c977..868dc3c36ba1 100644 > --- a/fs/nfs/nfs4client.c > +++ b/fs/nfs/nfs4client.c > @@ -1016,6 +1016,9 @@ static int nfs4_server_common_setup(struct nfs_server *server, > server->caps |= server->nfs_client->cl_mvops->init_caps; > if (server->flags & NFS_MOUNT_NORDIRPLUS) > server->caps &= ~NFS_CAP_READDIRPLUS; > + if (server->options & NFS_OPTION_NO_READ_PLUS) > + server->caps &= ~NFS_CAP_READ_PLUS; > + > /* > * Don't use NFS uid/gid mapping if we're using AUTH_SYS or lower > * authentication. > diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h > index 11248c5a7b24..360e70c7bbb6 100644 > --- a/include/linux/nfs_fs_sb.h > +++ b/include/linux/nfs_fs_sb.h > @@ -172,6 +172,7 @@ struct nfs_server { > unsigned int clone_blksize; /* granularity of a CLONE operation */ > #define NFS_OPTION_FSCACHE 0x00000001 /* - local caching enabled */ > #define NFS_OPTION_MIGRATION 0x00000002 /* - NFSv4 migration enabled */ > +#define NFS_OPTION_NO_READ_PLUS 0x00000004 /* - NFSv4.2 READ_PLUS enabled */ > > struct nfs_fsid fsid; > __u64 maxfilesize; /* maximum file size */ > -- > 2.24.1 > -- Chuck Lever
Hi Chuck, On Fri, 2020-01-10 at 17:41 -0500, Chuck Lever wrote: > NetApp Security WARNING: This is an external email. Do not click links or open > attachments unless you recognize the sender and know the content is safe. > > > > > Hi Anna- > > > On Jan 10, 2020, at 5:34 PM, schumaker.anna@gmail.com wrote: > > > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > There are some workloads where READ_PLUS might end up hurting > > performance, > > Can you say more about this? Have you seen workloads that are > hurt by READ_PLUS? Nothing jumps out at me from the tables in > the cover letter. It's mostly something I've seen when using btrfs in a virtual machine (so probably not a wide use case, and it's possible I need to change something in my setup to get it working better). > > > > so let's be nice to users and provide a way to disable this > > operation similar to how READDIR_PLUS can be disabled. > > Does it make sense to hold off on a mount option until there > is evidence that there is no other way to work around such a > performance regression? > > - Attempt to address the regression directly > - Improve the heuristics about when READ_PLUS is used > - Document that dropping back to vers=4.1 will disable it Yeah, we could hold off on the patch for now and see if anybody has any issues first. > > Any experience with NFS/RDMA? Not yet, but I can try to test that next week. Anna > > > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > --- > > fs/nfs/fs_context.c | 14 ++++++++++++++ > > fs/nfs/nfs4client.c | 3 +++ > > include/linux/nfs_fs_sb.h | 1 + > > 3 files changed, 18 insertions(+) > > > > diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c > > index 0247dcb7b316..82ba07c7c1ce 100644 > > --- a/fs/nfs/fs_context.c > > +++ b/fs/nfs/fs_context.c > > @@ -64,6 +64,7 @@ enum nfs_param { > > Opt_proto, > > Opt_rdirplus, > > Opt_rdma, > > + Opt_readplus, > > Opt_resvport, > > Opt_retrans, > > Opt_retry, > > @@ -120,6 +121,7 @@ static const struct fs_parameter_spec nfs_param_specs[] > > = { > > fsparam_string("proto", Opt_proto), > > fsparam_flag_no("rdirplus", Opt_rdirplus), > > fsparam_flag ("rdma", Opt_rdma), > > + fsparam_flag_no("readplus", Opt_readplus), > > fsparam_flag_no("resvport", Opt_resvport), > > fsparam_u32 ("retrans", Opt_retrans), > > fsparam_string("retry", Opt_retry), > > @@ -555,6 +557,12 @@ static int nfs_fs_context_parse_param(struct fs_context > > *fc, > > else > > ctx->options |= NFS_OPTION_MIGRATION; > > break; > > + case Opt_readplus: > > + if (result.negated) > > + ctx->options |= NFS_OPTION_NO_READ_PLUS; > > + else > > + ctx->options &= ~NFS_OPTION_NO_READ_PLUS; > > + break; > > > > /* > > * options that take numeric values > > @@ -1176,6 +1184,10 @@ static int nfs_fs_context_validate(struct fs_context > > *fc) > > (ctx->version != 4 || ctx->minorversion != 0)) > > goto out_migration_misuse; > > > > + if (ctx->options & NFS_OPTION_NO_READ_PLUS && > > + (ctx->version != 4 || ctx->minorversion < 2)) > > + goto out_noreadplus_misuse; > > + > > /* Verify that any proto=/mountproto= options match the address > > * families in the addr=/mountaddr= options. > > */ > > @@ -1254,6 +1266,8 @@ static int nfs_fs_context_validate(struct fs_context > > *fc) > > ctx->version, ctx->minorversion); > > out_migration_misuse: > > return nfs_invalf(fc, "NFS: 'Migration' not supported for this NFS > > version"); > > +out_noreadplus_misuse: > > + return nfs_invalf(fc, "NFS: 'noreadplus' not supported for this NFS > > version\n"); > > out_version_unavailable: > > nfs_errorf(fc, "NFS: Version unavailable"); > > return ret; > > diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c > > index 0cd767e5c977..868dc3c36ba1 100644 > > --- a/fs/nfs/nfs4client.c > > +++ b/fs/nfs/nfs4client.c > > @@ -1016,6 +1016,9 @@ static int nfs4_server_common_setup(struct nfs_server > > *server, > > server->caps |= server->nfs_client->cl_mvops->init_caps; > > if (server->flags & NFS_MOUNT_NORDIRPLUS) > > server->caps &= ~NFS_CAP_READDIRPLUS; > > + if (server->options & NFS_OPTION_NO_READ_PLUS) > > + server->caps &= ~NFS_CAP_READ_PLUS; > > + > > /* > > * Don't use NFS uid/gid mapping if we're using AUTH_SYS or lower > > * authentication. > > diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h > > index 11248c5a7b24..360e70c7bbb6 100644 > > --- a/include/linux/nfs_fs_sb.h > > +++ b/include/linux/nfs_fs_sb.h > > @@ -172,6 +172,7 @@ struct nfs_server { > > unsigned int clone_blksize; /* granularity of a CLONE > > operation */ > > #define NFS_OPTION_FSCACHE 0x00000001 /* - local caching enabled */ > > #define NFS_OPTION_MIGRATION 0x00000002 /* - NFSv4 migration enabled > > */ > > +#define NFS_OPTION_NO_READ_PLUS 0x00000004 /* - NFSv4.2 READ_PLUS > > enabled */ > > > > struct nfs_fsid fsid; > > __u64 maxfilesize; /* maximum file size */ > > -- > > 2.24.1 > > > > -- > Chuck Lever > > >
> On Jan 10, 2020, at 5:43 PM, Schumaker, Anna <Anna.Schumaker@netapp.com> wrote: > > Hi Chuck, > > On Fri, 2020-01-10 at 17:41 -0500, Chuck Lever wrote: >> NetApp Security WARNING: This is an external email. Do not click links or open >> attachments unless you recognize the sender and know the content is safe. >> >> >> >> >> Hi Anna- >> >>> On Jan 10, 2020, at 5:34 PM, schumaker.anna@gmail.com wrote: >>> >>> From: Anna Schumaker <Anna.Schumaker@Netapp.com> >>> >>> There are some workloads where READ_PLUS might end up hurting >>> performance, >> >> Can you say more about this? Have you seen workloads that are >> hurt by READ_PLUS? Nothing jumps out at me from the tables in >> the cover letter. > > It's mostly something I've seen when using btrfs in a virtual machine (so > probably not a wide use case, and it's possible I need to change something in my > setup to get it working better). > >> >> >>> so let's be nice to users and provide a way to disable this >>> operation similar to how READDIR_PLUS can be disabled. >> >> Does it make sense to hold off on a mount option until there >> is evidence that there is no other way to work around such a >> performance regression? >> >> - Attempt to address the regression directly >> - Improve the heuristics about when READ_PLUS is used >> - Document that dropping back to vers=4.1 will disable it > > Yeah, we could hold off on the patch for now and see if anybody has any issues > first. > >> >> Any experience with NFS/RDMA? > > Not yet, but I can try to test that next week. Thanks for your response! > Anna > >> >> >>> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> >>> --- >>> fs/nfs/fs_context.c | 14 ++++++++++++++ >>> fs/nfs/nfs4client.c | 3 +++ >>> include/linux/nfs_fs_sb.h | 1 + >>> 3 files changed, 18 insertions(+) >>> >>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c >>> index 0247dcb7b316..82ba07c7c1ce 100644 >>> --- a/fs/nfs/fs_context.c >>> +++ b/fs/nfs/fs_context.c >>> @@ -64,6 +64,7 @@ enum nfs_param { >>> Opt_proto, >>> Opt_rdirplus, >>> Opt_rdma, >>> + Opt_readplus, >>> Opt_resvport, >>> Opt_retrans, >>> Opt_retry, >>> @@ -120,6 +121,7 @@ static const struct fs_parameter_spec nfs_param_specs[] >>> = { >>> fsparam_string("proto", Opt_proto), >>> fsparam_flag_no("rdirplus", Opt_rdirplus), >>> fsparam_flag ("rdma", Opt_rdma), >>> + fsparam_flag_no("readplus", Opt_readplus), >>> fsparam_flag_no("resvport", Opt_resvport), >>> fsparam_u32 ("retrans", Opt_retrans), >>> fsparam_string("retry", Opt_retry), >>> @@ -555,6 +557,12 @@ static int nfs_fs_context_parse_param(struct fs_context >>> *fc, >>> else >>> ctx->options |= NFS_OPTION_MIGRATION; >>> break; >>> + case Opt_readplus: >>> + if (result.negated) >>> + ctx->options |= NFS_OPTION_NO_READ_PLUS; >>> + else >>> + ctx->options &= ~NFS_OPTION_NO_READ_PLUS; >>> + break; >>> >>> /* >>> * options that take numeric values >>> @@ -1176,6 +1184,10 @@ static int nfs_fs_context_validate(struct fs_context >>> *fc) >>> (ctx->version != 4 || ctx->minorversion != 0)) >>> goto out_migration_misuse; >>> >>> + if (ctx->options & NFS_OPTION_NO_READ_PLUS && >>> + (ctx->version != 4 || ctx->minorversion < 2)) >>> + goto out_noreadplus_misuse; >>> + >>> /* Verify that any proto=/mountproto= options match the address >>> * families in the addr=/mountaddr= options. >>> */ >>> @@ -1254,6 +1266,8 @@ static int nfs_fs_context_validate(struct fs_context >>> *fc) >>> ctx->version, ctx->minorversion); >>> out_migration_misuse: >>> return nfs_invalf(fc, "NFS: 'Migration' not supported for this NFS >>> version"); >>> +out_noreadplus_misuse: >>> + return nfs_invalf(fc, "NFS: 'noreadplus' not supported for this NFS >>> version\n"); >>> out_version_unavailable: >>> nfs_errorf(fc, "NFS: Version unavailable"); >>> return ret; >>> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c >>> index 0cd767e5c977..868dc3c36ba1 100644 >>> --- a/fs/nfs/nfs4client.c >>> +++ b/fs/nfs/nfs4client.c >>> @@ -1016,6 +1016,9 @@ static int nfs4_server_common_setup(struct nfs_server >>> *server, >>> server->caps |= server->nfs_client->cl_mvops->init_caps; >>> if (server->flags & NFS_MOUNT_NORDIRPLUS) >>> server->caps &= ~NFS_CAP_READDIRPLUS; >>> + if (server->options & NFS_OPTION_NO_READ_PLUS) >>> + server->caps &= ~NFS_CAP_READ_PLUS; >>> + >>> /* >>> * Don't use NFS uid/gid mapping if we're using AUTH_SYS or lower >>> * authentication. >>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h >>> index 11248c5a7b24..360e70c7bbb6 100644 >>> --- a/include/linux/nfs_fs_sb.h >>> +++ b/include/linux/nfs_fs_sb.h >>> @@ -172,6 +172,7 @@ struct nfs_server { >>> unsigned int clone_blksize; /* granularity of a CLONE >>> operation */ >>> #define NFS_OPTION_FSCACHE 0x00000001 /* - local caching enabled */ >>> #define NFS_OPTION_MIGRATION 0x00000002 /* - NFSv4 migration enabled >>> */ >>> +#define NFS_OPTION_NO_READ_PLUS 0x00000004 /* - NFSv4.2 READ_PLUS >>> enabled */ >>> >>> struct nfs_fsid fsid; >>> __u64 maxfilesize; /* maximum file size */ >>> -- >>> 2.24.1 >>> >> >> -- >> Chuck Lever >> >> >> -- Chuck Lever
diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c index 0247dcb7b316..82ba07c7c1ce 100644 --- a/fs/nfs/fs_context.c +++ b/fs/nfs/fs_context.c @@ -64,6 +64,7 @@ enum nfs_param { Opt_proto, Opt_rdirplus, Opt_rdma, + Opt_readplus, Opt_resvport, Opt_retrans, Opt_retry, @@ -120,6 +121,7 @@ static const struct fs_parameter_spec nfs_param_specs[] = { fsparam_string("proto", Opt_proto), fsparam_flag_no("rdirplus", Opt_rdirplus), fsparam_flag ("rdma", Opt_rdma), + fsparam_flag_no("readplus", Opt_readplus), fsparam_flag_no("resvport", Opt_resvport), fsparam_u32 ("retrans", Opt_retrans), fsparam_string("retry", Opt_retry), @@ -555,6 +557,12 @@ static int nfs_fs_context_parse_param(struct fs_context *fc, else ctx->options |= NFS_OPTION_MIGRATION; break; + case Opt_readplus: + if (result.negated) + ctx->options |= NFS_OPTION_NO_READ_PLUS; + else + ctx->options &= ~NFS_OPTION_NO_READ_PLUS; + break; /* * options that take numeric values @@ -1176,6 +1184,10 @@ static int nfs_fs_context_validate(struct fs_context *fc) (ctx->version != 4 || ctx->minorversion != 0)) goto out_migration_misuse; + if (ctx->options & NFS_OPTION_NO_READ_PLUS && + (ctx->version != 4 || ctx->minorversion < 2)) + goto out_noreadplus_misuse; + /* Verify that any proto=/mountproto= options match the address * families in the addr=/mountaddr= options. */ @@ -1254,6 +1266,8 @@ static int nfs_fs_context_validate(struct fs_context *fc) ctx->version, ctx->minorversion); out_migration_misuse: return nfs_invalf(fc, "NFS: 'Migration' not supported for this NFS version"); +out_noreadplus_misuse: + return nfs_invalf(fc, "NFS: 'noreadplus' not supported for this NFS version\n"); out_version_unavailable: nfs_errorf(fc, "NFS: Version unavailable"); return ret; diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c index 0cd767e5c977..868dc3c36ba1 100644 --- a/fs/nfs/nfs4client.c +++ b/fs/nfs/nfs4client.c @@ -1016,6 +1016,9 @@ static int nfs4_server_common_setup(struct nfs_server *server, server->caps |= server->nfs_client->cl_mvops->init_caps; if (server->flags & NFS_MOUNT_NORDIRPLUS) server->caps &= ~NFS_CAP_READDIRPLUS; + if (server->options & NFS_OPTION_NO_READ_PLUS) + server->caps &= ~NFS_CAP_READ_PLUS; + /* * Don't use NFS uid/gid mapping if we're using AUTH_SYS or lower * authentication. diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h index 11248c5a7b24..360e70c7bbb6 100644 --- a/include/linux/nfs_fs_sb.h +++ b/include/linux/nfs_fs_sb.h @@ -172,6 +172,7 @@ struct nfs_server { unsigned int clone_blksize; /* granularity of a CLONE operation */ #define NFS_OPTION_FSCACHE 0x00000001 /* - local caching enabled */ #define NFS_OPTION_MIGRATION 0x00000002 /* - NFSv4 migration enabled */ +#define NFS_OPTION_NO_READ_PLUS 0x00000004 /* - NFSv4.2 READ_PLUS enabled */ struct nfs_fsid fsid; __u64 maxfilesize; /* maximum file size */