| Message ID | 156933135322.20933.2166438700224340142.stgit@fedora-28 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: mount API patch series | expand |
On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote: > Make xfs_parse_param() take arguments of the fs context operation > .parse_param() in preparation for switching to use the file system > mount context for mount. > > The function fc_parse() only uses the file system context (fc here) > when calling log macros warnf() and invalf() which in turn check > only the fc->log field to determine if the message should be saved > to a context buffer (for later retrival by userspace) or logged > using printk(). > > Also the temporary function match_kstrtoint() is now unused, remove it. > > Signed-off-by: Ian Kent <raven@themaw.net> > --- > fs/xfs/xfs_super.c | 137 +++++++++++++++++++++++++++++++--------------------- > 1 file changed, 81 insertions(+), 56 deletions(-) > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index b04aebab69ab..6792d46fa0be 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s, unsigned int base, int *res) > return ret; > } > ... > > STATIC int > xfs_parse_param( ... > - switch (token) { > + opt = fs_parse(fc, &xfs_fs_parameters, param, &result); > + if (opt < 0) { > + /* > + * If fs_parse() returns -ENOPARAM and the parameter > + * is "source" the VFS needs to handle this option > + * in order to boot otherwise use the default case > + * below to handle invalid options. > + */ > + if (opt != -ENOPARAM || > + strcmp(param->key, "source") == 0) > + return opt; Same question as before on this bit.. ... > @@ -373,10 +374,16 @@ xfs_parseargs( > { > const struct super_block *sb = mp->m_super; > char *p; > - substring_t args[MAX_OPT_ARGS]; > - int dsunit = 0; > - int dswidth = 0; > - uint8_t iosizelog = 0; > + struct fs_context fc; > + struct xfs_fs_context context; > + struct xfs_fs_context *ctx; > + int ret; > + > + memset(&fc, 0, sizeof(fc)); > + memset(&context, 0, sizeof(context)); > + fc.fs_private = &context; > + ctx = &context; I think you mentioned this ctx/context pattern would be removed from v2..? Brian > + fc.s_fs_info = mp; > > /* > * set up the mount name first so all the errors will refer to the > @@ -413,16 +420,33 @@ xfs_parseargs( > goto done; > > while ((p = strsep(&options, ",")) != NULL) { > - int token; > - int ret; > + struct fs_parameter param; > + char *value; > > if (!*p) > continue; > > - token = match_token(p, tokens, args); > - ret = xfs_parse_param(token, p, args, mp, > - &dsunit, &dswidth, &iosizelog); > - if (ret) > + param.key = p; > + param.type = fs_value_is_string; > + param.string = NULL; > + param.size = 0; > + > + value = strchr(p, '='); > + if (value) { > + *value++ = 0; > + param.size = strlen(value); > + if (param.size > 0) { > + param.string = kmemdup_nul(value, > + param.size, > + GFP_KERNEL); > + if (!param.string) > + return -ENOMEM; > + } > + } > + > + ret = xfs_parse_param(&fc, ¶m); > + kfree(param.string); > + if (ret < 0) > return ret; > } > > @@ -435,7 +459,8 @@ xfs_parseargs( > return -EINVAL; > } > > - if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || dswidth)) { > + if ((mp->m_flags & XFS_MOUNT_NOALIGN) && > + (ctx->dsunit || ctx->dswidth)) { > xfs_warn(mp, > "sunit and swidth options incompatible with the noalign option"); > return -EINVAL; > @@ -448,28 +473,28 @@ xfs_parseargs( > } > #endif > > - if ((dsunit && !dswidth) || (!dsunit && dswidth)) { > + if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx->dswidth)) { > xfs_warn(mp, "sunit and swidth must be specified together"); > return -EINVAL; > } > > - if (dsunit && (dswidth % dsunit != 0)) { > + if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) { > xfs_warn(mp, > "stripe width (%d) must be a multiple of the stripe unit (%d)", > - dswidth, dsunit); > + ctx->dswidth, ctx->dsunit); > return -EINVAL; > } > > done: > - if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) { > + if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) { > /* > * At this point the superblock has not been read > * in, therefore we do not know the block size. > * Before the mount call ends we will convert > * these to FSBs. > */ > - mp->m_dalign = dsunit; > - mp->m_swidth = dswidth; > + mp->m_dalign = ctx->dsunit; > + mp->m_swidth = ctx->dswidth; > } > > if (mp->m_logbufs != -1 && > @@ -491,18 +516,18 @@ xfs_parseargs( > return -EINVAL; > } > > - if (iosizelog) { > - if (iosizelog > XFS_MAX_IO_LOG || > - iosizelog < XFS_MIN_IO_LOG) { > + if (ctx->iosizelog) { > + if (ctx->iosizelog > XFS_MAX_IO_LOG || > + ctx->iosizelog < XFS_MIN_IO_LOG) { > xfs_warn(mp, "invalid log iosize: %d [not %d-%d]", > - iosizelog, XFS_MIN_IO_LOG, > + ctx->iosizelog, XFS_MIN_IO_LOG, > XFS_MAX_IO_LOG); > return -EINVAL; > } > > mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE; > - mp->m_readio_log = iosizelog; > - mp->m_writeio_log = iosizelog; > + mp->m_readio_log = ctx->iosizelog; > + mp->m_writeio_log = ctx->iosizelog; > } > > return 0; >
On Tue, 2019-09-24 at 10:37 -0400, Brian Foster wrote: > On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote: > > Make xfs_parse_param() take arguments of the fs context operation > > .parse_param() in preparation for switching to use the file system > > mount context for mount. > > > > The function fc_parse() only uses the file system context (fc here) > > when calling log macros warnf() and invalf() which in turn check > > only the fc->log field to determine if the message should be saved > > to a context buffer (for later retrival by userspace) or logged > > using printk(). > > > > Also the temporary function match_kstrtoint() is now unused, remove > > it. > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > --- > > fs/xfs/xfs_super.c | 137 +++++++++++++++++++++++++++++++--------- > > ------------ > > 1 file changed, 81 insertions(+), 56 deletions(-) > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > index b04aebab69ab..6792d46fa0be 100644 > > --- a/fs/xfs/xfs_super.c > > +++ b/fs/xfs/xfs_super.c > > @@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s, unsigned int > > base, int *res) > > return ret; > > } > > > ... > > > > STATIC int > > xfs_parse_param( > ... > > - switch (token) { > > + opt = fs_parse(fc, &xfs_fs_parameters, param, &result); > > + if (opt < 0) { > > + /* > > + * If fs_parse() returns -ENOPARAM and the parameter > > + * is "source" the VFS needs to handle this option > > + * in order to boot otherwise use the default case > > + * below to handle invalid options. > > + */ > > + if (opt != -ENOPARAM || > > + strcmp(param->key, "source") == 0) > > + return opt; > > Same question as before on this bit.. Your comment was: Why is this not something that is handled in core mount-api code? Every filesystem needs this logic in order to be a rootfs..? I looked at the VFS code and was tempted to change it but it's all too easy to prevent the system from booting. The way the VFS looks to me it needs to give the file system a chance to handle the "source" option, if the file system ->parse_param() doesn't handle the option it "must" return -ENOPARAM so the VFS will test for and handle the "source" option. Having returned -ENOPARAM either the option is "source" or it's a real unknown option. The choices are: 1) If it is the "source" option we will get a false positive unknown parameter message logged by our ->parse_param(). 2) if it isn't the "source" option we will get an unknown parameter message from our ->parse_param() and an additional inconsistent format unknown parameter message from the VFS. 3) Check for the "source" parameter in our ->parse_param() and return without issuing a message so the VFS can handle the option, no duplicate message and no inconsistent logging. Suggestions on how to handle this better, a VFS patch perhaps? Suggestions David, Al? > ... > > @@ -373,10 +374,16 @@ xfs_parseargs( > > { > > const struct super_block *sb = mp->m_super; > > char *p; > > - substring_t args[MAX_OPT_ARGS]; > > - int dsunit = 0; > > - int dswidth = 0; > > - uint8_t iosizelog = 0; > > + struct fs_context fc; > > + struct xfs_fs_context context; > > + struct xfs_fs_context *ctx; > > + int ret; > > + > > + memset(&fc, 0, sizeof(fc)); > > + memset(&context, 0, sizeof(context)); > > + fc.fs_private = &context; > > + ctx = &context; > > I think you mentioned this ctx/context pattern would be removed from > v2..? Except that, to minimise code churn the ctx variable is needed because it will be used in the end result. I don't much like prefixing those references with &context even if some happen to go away later on, the additional local variable is clearer to read and provides usage consistency for the reader over the changes. Ian > > Brian > > > + fc.s_fs_info = mp; > > > > /* > > * set up the mount name first so all the errors will refer to > > the > > @@ -413,16 +420,33 @@ xfs_parseargs( > > goto done; > > > > while ((p = strsep(&options, ",")) != NULL) { > > - int token; > > - int ret; > > + struct fs_parameter param; > > + char *value; > > > > if (!*p) > > continue; > > > > - token = match_token(p, tokens, args); > > - ret = xfs_parse_param(token, p, args, mp, > > - &dsunit, &dswidth, &iosizelog); > > - if (ret) > > + param.key = p; > > + param.type = fs_value_is_string; > > + param.string = NULL; > > + param.size = 0; > > + > > + value = strchr(p, '='); > > + if (value) { > > + *value++ = 0; > > + param.size = strlen(value); > > + if (param.size > 0) { > > + param.string = kmemdup_nul(value, > > + param.size, > > + GFP_KERNEL); > > + if (!param.string) > > + return -ENOMEM; > > + } > > + } > > + > > + ret = xfs_parse_param(&fc, ¶m); > > + kfree(param.string); > > + if (ret < 0) > > return ret; > > } > > > > @@ -435,7 +459,8 @@ xfs_parseargs( > > return -EINVAL; > > } > > > > - if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || dswidth)) { > > + if ((mp->m_flags & XFS_MOUNT_NOALIGN) && > > + (ctx->dsunit || ctx->dswidth)) { > > xfs_warn(mp, > > "sunit and swidth options incompatible with the noalign > > option"); > > return -EINVAL; > > @@ -448,28 +473,28 @@ xfs_parseargs( > > } > > #endif > > > > - if ((dsunit && !dswidth) || (!dsunit && dswidth)) { > > + if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx- > > >dswidth)) { > > xfs_warn(mp, "sunit and swidth must be specified > > together"); > > return -EINVAL; > > } > > > > - if (dsunit && (dswidth % dsunit != 0)) { > > + if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) { > > xfs_warn(mp, > > "stripe width (%d) must be a multiple of the stripe unit (%d)", > > - dswidth, dsunit); > > + ctx->dswidth, ctx->dsunit); > > return -EINVAL; > > } > > > > done: > > - if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) { > > + if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) { > > /* > > * At this point the superblock has not been read > > * in, therefore we do not know the block size. > > * Before the mount call ends we will convert > > * these to FSBs. > > */ > > - mp->m_dalign = dsunit; > > - mp->m_swidth = dswidth; > > + mp->m_dalign = ctx->dsunit; > > + mp->m_swidth = ctx->dswidth; > > } > > > > if (mp->m_logbufs != -1 && > > @@ -491,18 +516,18 @@ xfs_parseargs( > > return -EINVAL; > > } > > > > - if (iosizelog) { > > - if (iosizelog > XFS_MAX_IO_LOG || > > - iosizelog < XFS_MIN_IO_LOG) { > > + if (ctx->iosizelog) { > > + if (ctx->iosizelog > XFS_MAX_IO_LOG || > > + ctx->iosizelog < XFS_MIN_IO_LOG) { > > xfs_warn(mp, "invalid log iosize: %d [not %d- > > %d]", > > - iosizelog, XFS_MIN_IO_LOG, > > + ctx->iosizelog, XFS_MIN_IO_LOG, > > XFS_MAX_IO_LOG); > > return -EINVAL; > > } > > > > mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE; > > - mp->m_readio_log = iosizelog; > > - mp->m_writeio_log = iosizelog; > > + mp->m_readio_log = ctx->iosizelog; > > + mp->m_writeio_log = ctx->iosizelog; > > } > > > > return 0; > >
On Wed, Sep 25, 2019 at 08:20:25AM +0800, Ian Kent wrote: > On Tue, 2019-09-24 at 10:37 -0400, Brian Foster wrote: > > On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote: > > > Make xfs_parse_param() take arguments of the fs context operation > > > .parse_param() in preparation for switching to use the file system > > > mount context for mount. > > > > > > The function fc_parse() only uses the file system context (fc here) > > > when calling log macros warnf() and invalf() which in turn check > > > only the fc->log field to determine if the message should be saved > > > to a context buffer (for later retrival by userspace) or logged > > > using printk(). > > > > > > Also the temporary function match_kstrtoint() is now unused, remove > > > it. > > > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > > --- > > > fs/xfs/xfs_super.c | 137 +++++++++++++++++++++++++++++++--------- > > > ------------ > > > 1 file changed, 81 insertions(+), 56 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > > index b04aebab69ab..6792d46fa0be 100644 > > > --- a/fs/xfs/xfs_super.c > > > +++ b/fs/xfs/xfs_super.c > > > @@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s, unsigned int > > > base, int *res) > > > return ret; > > > } > > > > > ... > > > > > > STATIC int > > > xfs_parse_param( > > ... > > > - switch (token) { > > > + opt = fs_parse(fc, &xfs_fs_parameters, param, &result); > > > + if (opt < 0) { > > > + /* > > > + * If fs_parse() returns -ENOPARAM and the parameter > > > + * is "source" the VFS needs to handle this option > > > + * in order to boot otherwise use the default case > > > + * below to handle invalid options. > > > + */ > > > + if (opt != -ENOPARAM || > > > + strcmp(param->key, "source") == 0) > > > + return opt; > > > > Same question as before on this bit.. > > Your comment was: > Why is this not something that is handled in core mount-api code? Every > filesystem needs this logic in order to be a rootfs..? > > I looked at the VFS code and was tempted to change it but it's all too > easy to prevent the system from booting. > Ok, so I'm not terribly familiar with the core mount code in the first place. Can you elaborate a bit on the where the whole "source" thing comes from and why/how it's necessary to boot? > The way the VFS looks to me it needs to give the file system a chance > to handle the "source" option, if the file system ->parse_param() > doesn't handle the option it "must" return -ENOPARAM so the VFS will > test for and handle the "source" option. > Do any existing filesystems handle this option? By handle, I mean actually have to make some change, set some option, etc. > Having returned -ENOPARAM either the option is "source" or it's a > real unknown option. > > The choices are: > 1) If it is the "source" option we will get a false positive unknown > parameter message logged by our ->parse_param(). > 2) if it isn't the "source" option we will get an unknown parameter > message from our ->parse_param() and an additional inconsistent > format unknown parameter message from the VFS. > 3) Check for the "source" parameter in our ->parse_param() and > return without issuing a message so the VFS can handle the option, > no duplicate message and no inconsistent logging. > Hmm.. so we definitely don't want spurious unknown parameter messages, but I don't see how that leaves #3 as the only other option. Is param-key already filled in at that point? If so, couldn't we set a flag or something on the context structure to signal that we don't care about the source option, so let the vfs handle it however it needs to? If not, another option could be to define a helper function or something that the fs can call to determine whether an -ENOPARAM key is some global option to be handled by a higher layer and so to not throw a warning or whatever. That has the same logic as this patch, but is still better than open-coding "source" key checks all over the place IMO. BTW, this all implies there is some reason for an fs to handle the "source" option, so what happens if one actually does? ISTM the ->parse_param() callout would return 0 and vfs_parse_fs_param() would skip its own update of fc->source. Hm? Brian > Suggestions on how to handle this better, a VFS patch perhaps? > Suggestions David, Al? > > > ... > > > @@ -373,10 +374,16 @@ xfs_parseargs( > > > { > > > const struct super_block *sb = mp->m_super; > > > char *p; > > > - substring_t args[MAX_OPT_ARGS]; > > > - int dsunit = 0; > > > - int dswidth = 0; > > > - uint8_t iosizelog = 0; > > > + struct fs_context fc; > > > + struct xfs_fs_context context; > > > + struct xfs_fs_context *ctx; > > > + int ret; > > > + > > > + memset(&fc, 0, sizeof(fc)); > > > + memset(&context, 0, sizeof(context)); > > > + fc.fs_private = &context; > > > + ctx = &context; > > > > I think you mentioned this ctx/context pattern would be removed from > > v2..? > > Except that, to minimise code churn the ctx variable is needed > because it will be used in the end result. > > I don't much like prefixing those references with &context even > if some happen to go away later on, the additional local variable > is clearer to read and provides usage consistency for the reader > over the changes. > > Ian > > > > Brian > > > > > + fc.s_fs_info = mp; > > > > > > /* > > > * set up the mount name first so all the errors will refer to > > > the > > > @@ -413,16 +420,33 @@ xfs_parseargs( > > > goto done; > > > > > > while ((p = strsep(&options, ",")) != NULL) { > > > - int token; > > > - int ret; > > > + struct fs_parameter param; > > > + char *value; > > > > > > if (!*p) > > > continue; > > > > > > - token = match_token(p, tokens, args); > > > - ret = xfs_parse_param(token, p, args, mp, > > > - &dsunit, &dswidth, &iosizelog); > > > - if (ret) > > > + param.key = p; > > > + param.type = fs_value_is_string; > > > + param.string = NULL; > > > + param.size = 0; > > > + > > > + value = strchr(p, '='); > > > + if (value) { > > > + *value++ = 0; > > > + param.size = strlen(value); > > > + if (param.size > 0) { > > > + param.string = kmemdup_nul(value, > > > + param.size, > > > + GFP_KERNEL); > > > + if (!param.string) > > > + return -ENOMEM; > > > + } > > > + } > > > + > > > + ret = xfs_parse_param(&fc, ¶m); > > > + kfree(param.string); > > > + if (ret < 0) > > > return ret; > > > } > > > > > > @@ -435,7 +459,8 @@ xfs_parseargs( > > > return -EINVAL; > > > } > > > > > > - if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || dswidth)) { > > > + if ((mp->m_flags & XFS_MOUNT_NOALIGN) && > > > + (ctx->dsunit || ctx->dswidth)) { > > > xfs_warn(mp, > > > "sunit and swidth options incompatible with the noalign > > > option"); > > > return -EINVAL; > > > @@ -448,28 +473,28 @@ xfs_parseargs( > > > } > > > #endif > > > > > > - if ((dsunit && !dswidth) || (!dsunit && dswidth)) { > > > + if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx- > > > >dswidth)) { > > > xfs_warn(mp, "sunit and swidth must be specified > > > together"); > > > return -EINVAL; > > > } > > > > > > - if (dsunit && (dswidth % dsunit != 0)) { > > > + if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) { > > > xfs_warn(mp, > > > "stripe width (%d) must be a multiple of the stripe unit (%d)", > > > - dswidth, dsunit); > > > + ctx->dswidth, ctx->dsunit); > > > return -EINVAL; > > > } > > > > > > done: > > > - if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) { > > > + if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) { > > > /* > > > * At this point the superblock has not been read > > > * in, therefore we do not know the block size. > > > * Before the mount call ends we will convert > > > * these to FSBs. > > > */ > > > - mp->m_dalign = dsunit; > > > - mp->m_swidth = dswidth; > > > + mp->m_dalign = ctx->dsunit; > > > + mp->m_swidth = ctx->dswidth; > > > } > > > > > > if (mp->m_logbufs != -1 && > > > @@ -491,18 +516,18 @@ xfs_parseargs( > > > return -EINVAL; > > > } > > > > > > - if (iosizelog) { > > > - if (iosizelog > XFS_MAX_IO_LOG || > > > - iosizelog < XFS_MIN_IO_LOG) { > > > + if (ctx->iosizelog) { > > > + if (ctx->iosizelog > XFS_MAX_IO_LOG || > > > + ctx->iosizelog < XFS_MIN_IO_LOG) { > > > xfs_warn(mp, "invalid log iosize: %d [not %d- > > > %d]", > > > - iosizelog, XFS_MIN_IO_LOG, > > > + ctx->iosizelog, XFS_MIN_IO_LOG, > > > XFS_MAX_IO_LOG); > > > return -EINVAL; > > > } > > > > > > mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE; > > > - mp->m_readio_log = iosizelog; > > > - mp->m_writeio_log = iosizelog; > > > + mp->m_readio_log = ctx->iosizelog; > > > + mp->m_writeio_log = ctx->iosizelog; > > > } > > > > > > return 0; > > > >
On Wed, 2019-09-25 at 10:33 -0400, Brian Foster wrote: > On Wed, Sep 25, 2019 at 08:20:25AM +0800, Ian Kent wrote: > > On Tue, 2019-09-24 at 10:37 -0400, Brian Foster wrote: > > > On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote: > > > > Make xfs_parse_param() take arguments of the fs context > > > > operation > > > > .parse_param() in preparation for switching to use the file > > > > system > > > > mount context for mount. > > > > > > > > The function fc_parse() only uses the file system context (fc > > > > here) > > > > when calling log macros warnf() and invalf() which in turn > > > > check > > > > only the fc->log field to determine if the message should be > > > > saved > > > > to a context buffer (for later retrival by userspace) or logged > > > > using printk(). > > > > > > > > Also the temporary function match_kstrtoint() is now unused, > > > > remove > > > > it. > > > > > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > > > --- > > > > fs/xfs/xfs_super.c | 137 +++++++++++++++++++++++++++++++----- > > > > ---- > > > > ------------ > > > > 1 file changed, 81 insertions(+), 56 deletions(-) > > > > > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > > > index b04aebab69ab..6792d46fa0be 100644 > > > > --- a/fs/xfs/xfs_super.c > > > > +++ b/fs/xfs/xfs_super.c > > > > @@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s, unsigned > > > > int > > > > base, int *res) > > > > return ret; > > > > } > > > > > > > ... > > > > > > > > STATIC int > > > > xfs_parse_param( > > > ... > > > > - switch (token) { > > > > + opt = fs_parse(fc, &xfs_fs_parameters, param, &result); > > > > + if (opt < 0) { > > > > + /* > > > > + * If fs_parse() returns -ENOPARAM and the > > > > parameter > > > > + * is "source" the VFS needs to handle this > > > > option > > > > + * in order to boot otherwise use the default > > > > case > > > > + * below to handle invalid options. > > > > + */ > > > > + if (opt != -ENOPARAM || > > > > + strcmp(param->key, "source") == 0) > > > > + return opt; > > > > > > Same question as before on this bit.. > > > > Your comment was: > > Why is this not something that is handled in core mount-api code? > > Every > > filesystem needs this logic in order to be a rootfs..? > > > > I looked at the VFS code and was tempted to change it but it's all > > too > > easy to prevent the system from booting. > > > > Ok, so I'm not terribly familiar with the core mount code in the > first > place. Can you elaborate a bit on the where the whole "source" thing > comes from and why/how it's necessary to boot? Your not alone. I've pondered over the VFS mount code fairly often over the years and I've not seen it before either. About all I know is it's needed for rootfs, so I guess it's needed to resolve the boot device when no file system is yet mounted and a normal path walk can't be done. > > > The way the VFS looks to me it needs to give the file system a > > chance > > to handle the "source" option, if the file system ->parse_param() > > doesn't handle the option it "must" return -ENOPARAM so the VFS > > will > > test for and handle the "source" option. > > > > Do any existing filesystems handle this option? By handle, I mean > actually have to make some change, set some option, etc. AFAIK very few file systems handle the option (and I suspect virtually none until perhaps recently) as David mentioned a couple that do yesterday on IRC. Apparently there are a couple of file systems that want to take a non-standard mount source and resolve it to a kernel usable source for mounting. I'm really not familiar with the details either so I'm making assumptions which might not be correct. > > > Having returned -ENOPARAM either the option is "source" or it's a > > real unknown option. > > > > The choices are: > > 1) If it is the "source" option we will get a false positive > > unknown > > parameter message logged by our ->parse_param(). > > 2) if it isn't the "source" option we will get an unknown parameter > > message from our ->parse_param() and an additional inconsistent > > format unknown parameter message from the VFS. > > 3) Check for the "source" parameter in our ->parse_param() and > > return without issuing a message so the VFS can handle the option, > > no duplicate message and no inconsistent logging. > > > > Hmm.. so we definitely don't want spurious unknown parameter > messages, > but I don't see how that leaves #3 as the only other option. My reading of the the code (the mount-api code) is that if the .parse_param() method is defined it gives it a chance to handle the parameter whatever it is. Then .parase_param() must return -ENOPARAM for the VFS parameter handling function to then check for the "source" parameter, handle it or issue an unknown parameter message. So, in order to return something other than -ENOPARAM, thereby avoiding the extra message, that must be done only if the parameter is not "source". The problem is most file systems won't handle that parameter and can reasonably be expected to issue an error message when they encounter it but there are a couple that will want to handle it so .parse_param() must be given a chance to do so. So it's not a problem specific to xfs. > > Is param-key already filled in at that point? If so, couldn't we set > a > flag or something on the context structure to signal that we don't > care > about the source option, so let the vfs handle it however it needs > to? Maybe. > If not, another option could be to define a helper function or > something > that the fs can call to determine whether an -ENOPARAM key is some > global option to be handled by a higher layer and so to not throw a > warning or whatever. That has the same logic as this patch, but is > still > better than open-coding "source" key checks all over the place IMO. Maybe an additional fs_context_purpose needs to be defined, maybe FS_CONTEXT_FOR_ROOTFS for example. That's probably the cleanest way to handle it, not sure it would properly cover the cases though. That wouldn't be an entirely trivial change so David and Al would likely need to get involved and Linus would need to be willing to accept it. > > BTW, this all implies there is some reason for an fs to handle the > "source" option, so what happens if one actually does? ISTM the > ->parse_param() callout would return 0 and vfs_parse_fs_param() would > skip its own update of fc->source. Hm? As I understand it that's not a problem because the file system will need to have converted the parameter value to some "source" value usable by the kernel. > > Brian > > > Suggestions on how to handle this better, a VFS patch perhaps? > > Suggestions David, Al? > > > > > ... > > > > @@ -373,10 +374,16 @@ xfs_parseargs( > > > > { > > > > const struct super_block *sb = mp->m_super; > > > > char *p; > > > > - substring_t args[MAX_OPT_ARGS]; > > > > - int dsunit = 0; > > > > - int dswidth = 0; > > > > - uint8_t iosizelog = 0; > > > > + struct fs_context fc; > > > > + struct xfs_fs_context context; > > > > + struct xfs_fs_context *ctx; > > > > + int ret; > > > > + > > > > + memset(&fc, 0, sizeof(fc)); > > > > + memset(&context, 0, sizeof(context)); > > > > + fc.fs_private = &context; > > > > + ctx = &context; > > > > > > I think you mentioned this ctx/context pattern would be removed > > > from > > > v2..? > > > > Except that, to minimise code churn the ctx variable is needed > > because it will be used in the end result. > > > > I don't much like prefixing those references with &context even > > if some happen to go away later on, the additional local variable > > is clearer to read and provides usage consistency for the reader > > over the changes. > > > > Ian > > > Brian > > > > > > > + fc.s_fs_info = mp; > > > > > > > > /* > > > > * set up the mount name first so all the errors will > > > > refer to > > > > the > > > > @@ -413,16 +420,33 @@ xfs_parseargs( > > > > goto done; > > > > > > > > while ((p = strsep(&options, ",")) != NULL) { > > > > - int token; > > > > - int ret; > > > > + struct fs_parameter param; > > > > + char *value; > > > > > > > > if (!*p) > > > > continue; > > > > > > > > - token = match_token(p, tokens, args); > > > > - ret = xfs_parse_param(token, p, args, mp, > > > > - &dsunit, &dswidth, > > > > &iosizelog); > > > > - if (ret) > > > > + param.key = p; > > > > + param.type = fs_value_is_string; > > > > + param.string = NULL; > > > > + param.size = 0; > > > > + > > > > + value = strchr(p, '='); > > > > + if (value) { > > > > + *value++ = 0; > > > > + param.size = strlen(value); > > > > + if (param.size > 0) { > > > > + param.string = > > > > kmemdup_nul(value, > > > > + para > > > > m.size, > > > > + GFP_ > > > > KERNEL); > > > > + if (!param.string) > > > > + return -ENOMEM; > > > > + } > > > > + } > > > > + > > > > + ret = xfs_parse_param(&fc, ¶m); > > > > + kfree(param.string); > > > > + if (ret < 0) > > > > return ret; > > > > } > > > > > > > > @@ -435,7 +459,8 @@ xfs_parseargs( > > > > return -EINVAL; > > > > } > > > > > > > > - if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || > > > > dswidth)) { > > > > + if ((mp->m_flags & XFS_MOUNT_NOALIGN) && > > > > + (ctx->dsunit || ctx->dswidth)) { > > > > xfs_warn(mp, > > > > "sunit and swidth options incompatible with the noalign > > > > option"); > > > > return -EINVAL; > > > > @@ -448,28 +473,28 @@ xfs_parseargs( > > > > } > > > > #endif > > > > > > > > - if ((dsunit && !dswidth) || (!dsunit && dswidth)) { > > > > + if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && > > > > ctx- > > > > > dswidth)) { > > > > xfs_warn(mp, "sunit and swidth must be > > > > specified > > > > together"); > > > > return -EINVAL; > > > > } > > > > > > > > - if (dsunit && (dswidth % dsunit != 0)) { > > > > + if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) { > > > > xfs_warn(mp, > > > > "stripe width (%d) must be a multiple of the stripe > > > > unit (%d)", > > > > - dswidth, dsunit); > > > > + ctx->dswidth, ctx->dsunit); > > > > return -EINVAL; > > > > } > > > > > > > > done: > > > > - if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) { > > > > + if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) > > > > { > > > > /* > > > > * At this point the superblock has not been > > > > read > > > > * in, therefore we do not know the block size. > > > > * Before the mount call ends we will convert > > > > * these to FSBs. > > > > */ > > > > - mp->m_dalign = dsunit; > > > > - mp->m_swidth = dswidth; > > > > + mp->m_dalign = ctx->dsunit; > > > > + mp->m_swidth = ctx->dswidth; > > > > } > > > > > > > > if (mp->m_logbufs != -1 && > > > > @@ -491,18 +516,18 @@ xfs_parseargs( > > > > return -EINVAL; > > > > } > > > > > > > > - if (iosizelog) { > > > > - if (iosizelog > XFS_MAX_IO_LOG || > > > > - iosizelog < XFS_MIN_IO_LOG) { > > > > + if (ctx->iosizelog) { > > > > + if (ctx->iosizelog > XFS_MAX_IO_LOG || > > > > + ctx->iosizelog < XFS_MIN_IO_LOG) { > > > > xfs_warn(mp, "invalid log iosize: %d > > > > [not %d- > > > > %d]", > > > > - iosizelog, XFS_MIN_IO_LOG, > > > > + ctx->iosizelog, XFS_MIN_IO_LOG, > > > > XFS_MAX_IO_LOG); > > > > return -EINVAL; > > > > } > > > > > > > > mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE; > > > > - mp->m_readio_log = iosizelog; > > > > - mp->m_writeio_log = iosizelog; > > > > + mp->m_readio_log = ctx->iosizelog; > > > > + mp->m_writeio_log = ctx->iosizelog; > > > > } > > > > > > > > return 0; > > > >
On Thu, Sep 26, 2019 at 10:57:48AM +0800, Ian Kent wrote: > > Ok, so I'm not terribly familiar with the core mount code in the > > first > > place. Can you elaborate a bit on the where the whole "source" thing > > comes from and why/how it's necessary to boot? device name. And there's nothing special about boot. > Your not alone. > > I've pondered over the VFS mount code fairly often over the years > and I've not seen it before either. > > About all I know is it's needed for rootfs, so I guess it's needed > to resolve the boot device when no file system is yet mounted and > a normal path walk can't be done. Bollocks. Absolute root is ramfs or tmpfs and we do have mknod done there. Root filesystem is a complete red herring. > Maybe an additional fs_context_purpose needs to be defined, maybe > FS_CONTEXT_FOR_ROOTFS for example. NO. ->purpose should go away, not be extended. And again, that has fuck-all to do with rootfs. > That's probably the cleanest way to handle it, not sure it would > properly cover the cases though. > > That wouldn't be an entirely trivial change so David and Al would > likely need to get involved and Linus would need to be willing to > accept it. > > BTW, this all implies there is some reason for an fs to handle the > > "source" option, so what happens if one actually does? ISTM the > > ->parse_param() callout would return 0 and vfs_parse_fs_param() would > > skip its own update of fc->source. Hm? > > As I understand it that's not a problem because the file system > will need to have converted the parameter value to some "source" > value usable by the kernel. For fsck sake... It's where the first argument of mount(2) goes. As in, "/dev/sda4" when you say mount -t xfs /dev/sda4 /mnt Or "10.1.1.18:/srv/nfs/mirrors" in mount -t nfs4 10.1.1.18:/srv/nfs/mirrors /home/mirrors/public \ -o rsize=8192,wsize=8192,proto=tcp,ro Note that it's not in any real sense different from any other option - its interpretation is entirely up to fs type. The only reason why it's separate is historical - way, way back there had been no filesystem types and mount(2) got just a block device pathname + mountpoint + one flag (ro/rw). No (string) options either. There has been a long and nasty history, considerably older than Linux, including such things as magical binary structures (each for its own fs type - check how e.g. OSF is doing that). But accidents of calling conventions aside, device name is just another fs option. And device name is a misnomer - witness what e.g. NFS is doing. Keeping it special for new API was pointless, so we have this in vfs_kern_mount(): fc = fs_context_for_mount(type, flags); if (IS_ERR(fc)) return ERR_CAST(fc); if (name) ret = vfs_parse_fs_string(fc, "source", name, strlen(name)); if (!ret) ret = parse_monolithic_mount_data(fc, data); if (!ret) mnt = fc_mount(fc); else mnt = ERR_PTR(ret); put_fs_context(fc); and sys_mount() ends up passing the first argument as 'name' to vfs_kern_mount(). That's it - nothing more to it.
On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote: > + opt = fs_parse(fc, &xfs_fs_parameters, param, &result); > + if (opt < 0) { > + /* > + * If fs_parse() returns -ENOPARAM and the parameter > + * is "source" the VFS needs to handle this option > + * in order to boot otherwise use the default case > + * below to handle invalid options. > + */ > + if (opt != -ENOPARAM || > + strcmp(param->key, "source") == 0) > + return opt; Just return opt; here and be done with that. The comment is bloody misleading - for one thing, "in order to boot" is really "in order to mount anything", and the only reason for the kludge is that the default for "source" (in vfs_parse_fs_param(), triggered in case when -ENOPARAM had been returned by ->parse_param()) won't get triggered if you insist on reporting _all_ unknown options on your own. > + } > default: > - xfs_warn(mp, "unknown mount option [%s].", p); > + xfs_warn(mp, "unknown mount option [%s].", param->key); > return -EINVAL; ... here, instead of letting the same vfs_parse_fs_param() handle the warning. Or you could add Opt_source for handling that, with equivalent of that fallback (namely, if (param->type != fs_value_is_string) return invalf(fc, "VFS: Non-string source"); if (fc->source) return invalf(fc, "VFS: Multiple sources"); fc->source = param->string; param->string = NULL; return 0; ) done in your ->parse_param().
On Thu, 2019-09-26 at 10:57 +0800, Ian Kent wrote: > On Wed, 2019-09-25 at 10:33 -0400, Brian Foster wrote: > > On Wed, Sep 25, 2019 at 08:20:25AM +0800, Ian Kent wrote: > > > On Tue, 2019-09-24 at 10:37 -0400, Brian Foster wrote: > > > > On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote: > > > > > Make xfs_parse_param() take arguments of the fs context > > > > > operation > > > > > .parse_param() in preparation for switching to use the file > > > > > system > > > > > mount context for mount. > > > > > > > > > > The function fc_parse() only uses the file system context (fc > > > > > here) > > > > > when calling log macros warnf() and invalf() which in turn > > > > > check > > > > > only the fc->log field to determine if the message should be > > > > > saved > > > > > to a context buffer (for later retrival by userspace) or > > > > > logged > > > > > using printk(). > > > > > > > > > > Also the temporary function match_kstrtoint() is now unused, > > > > > remove > > > > > it. > > > > > > > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > > > > --- > > > > > fs/xfs/xfs_super.c | 137 +++++++++++++++++++++++++++++++--- > > > > > -- > > > > > ---- > > > > > ------------ > > > > > 1 file changed, 81 insertions(+), 56 deletions(-) > > > > > > > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > > > > index b04aebab69ab..6792d46fa0be 100644 > > > > > --- a/fs/xfs/xfs_super.c > > > > > +++ b/fs/xfs/xfs_super.c > > > > > @@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s, > > > > > unsigned > > > > > int > > > > > base, int *res) > > > > > return ret; > > > > > } > > > > > > > > > ... > > > > > > > > > > STATIC int > > > > > xfs_parse_param( > > > > ... > > > > > - switch (token) { > > > > > + opt = fs_parse(fc, &xfs_fs_parameters, param, &result); > > > > > + if (opt < 0) { > > > > > + /* > > > > > + * If fs_parse() returns -ENOPARAM and the > > > > > parameter > > > > > + * is "source" the VFS needs to handle this > > > > > option > > > > > + * in order to boot otherwise use the default > > > > > case > > > > > + * below to handle invalid options. > > > > > + */ > > > > > + if (opt != -ENOPARAM || > > > > > + strcmp(param->key, "source") == 0) > > > > > + return opt; > > > > > > > > Same question as before on this bit.. > > > > > > Your comment was: > > > Why is this not something that is handled in core mount-api code? > > > Every > > > filesystem needs this logic in order to be a rootfs..? > > > > > > I looked at the VFS code and was tempted to change it but it's > > > all > > > too > > > easy to prevent the system from booting. > > > > > > > Ok, so I'm not terribly familiar with the core mount code in the > > first > > place. Can you elaborate a bit on the where the whole "source" > > thing > > comes from and why/how it's necessary to boot? > > Your not alone. > > I've pondered over the VFS mount code fairly often over the years > and I've not seen it before either. > > About all I know is it's needed for rootfs, so I guess it's needed > to resolve the boot device when no file system is yet mounted and > a normal path walk can't be done. > > > > The way the VFS looks to me it needs to give the file system a > > > chance > > > to handle the "source" option, if the file system ->parse_param() > > > doesn't handle the option it "must" return -ENOPARAM so the VFS > > > will > > > test for and handle the "source" option. > > > > > > > Do any existing filesystems handle this option? By handle, I mean > > actually have to make some change, set some option, etc. > > AFAIK very few file systems handle the option (and I suspect > virtually none until perhaps recently) as David mentioned a > couple that do yesterday on IRC. > > Apparently there are a couple of file systems that want to > take a non-standard mount source and resolve it to a kernel > usable source for mounting. > > I'm really not familiar with the details either so I'm making > assumptions which might not be correct. > > > > Having returned -ENOPARAM either the option is "source" or it's a > > > real unknown option. > > > > > > The choices are: > > > 1) If it is the "source" option we will get a false positive > > > unknown > > > parameter message logged by our ->parse_param(). > > > 2) if it isn't the "source" option we will get an unknown > > > parameter > > > message from our ->parse_param() and an additional inconsistent > > > format unknown parameter message from the VFS. > > > 3) Check for the "source" parameter in our ->parse_param() and > > > return without issuing a message so the VFS can handle the > > > option, > > > no duplicate message and no inconsistent logging. > > > > > > > Hmm.. so we definitely don't want spurious unknown parameter > > messages, > > but I don't see how that leaves #3 as the only other option. > > My reading of the the code (the mount-api code) is that if the > .parse_param() method is defined it gives it a chance to handle > the parameter whatever it is. Then .parase_param() must return > -ENOPARAM for the VFS parameter handling function to then check > for the "source" parameter, handle it or issue an unknown parameter > message. > > So, in order to return something other than -ENOPARAM, thereby > avoiding the extra message, that must be done only if the parameter > is not "source". > > The problem is most file systems won't handle that parameter and > can reasonably be expected to issue an error message when they > encounter it but there are a couple that will want to handle it > so .parse_param() must be given a chance to do so. > > So it's not a problem specific to xfs. But let's not forget why this is being done. The reason is the "XFS: <message>" vs. "xfs: <message>" inconsistency and the lack of kernel log messages if the fsxxx() system calls are being used to perform the mount. The other possibility is to do: opt = fs_parse(fc, &xfs_fs_parameters, param, &result); if (opt < 0) return opt; and accept that the "default:" case that issues an error message for an unknown parameter will never be reached and unknown parameter messages will use fs_type ->name (lower case) for the log message. And also accept that if the fsxxx() system calls are being used to perform the mount parameter parsing messages won't be logged to the kernel log at all. The gfs2 mount-api code does this. > > > Is param-key already filled in at that point? If so, couldn't we > > set > > a > > flag or something on the context structure to signal that we don't > > care > > about the source option, so let the vfs handle it however it needs > > to? > > Maybe. > > > If not, another option could be to define a helper function or > > something > > that the fs can call to determine whether an -ENOPARAM key is some > > global option to be handled by a higher layer and so to not throw a > > warning or whatever. That has the same logic as this patch, but is > > still > > better than open-coding "source" key checks all over the place > > IMO. > > Maybe an additional fs_context_purpose needs to be defined, maybe > FS_CONTEXT_FOR_ROOTFS for example. > > That's probably the cleanest way to handle it, not sure it would > properly cover the cases though. > > That wouldn't be an entirely trivial change so David and Al would > likely need to get involved and Linus would need to be willing to > accept it. > > > BTW, this all implies there is some reason for an fs to handle the > > "source" option, so what happens if one actually does? ISTM the > > ->parse_param() callout would return 0 and vfs_parse_fs_param() > > would > > skip its own update of fc->source. Hm? > > As I understand it that's not a problem because the file system > will need to have converted the parameter value to some "source" > value usable by the kernel. > > > Brian > > > > > Suggestions on how to handle this better, a VFS patch perhaps? > > > Suggestions David, Al? > > > > > > > ... > > > > > @@ -373,10 +374,16 @@ xfs_parseargs( > > > > > { > > > > > const struct super_block *sb = mp->m_super; > > > > > char *p; > > > > > - substring_t args[MAX_OPT_ARGS]; > > > > > - int dsunit = 0; > > > > > - int dswidth = 0; > > > > > - uint8_t iosizelog = 0; > > > > > + struct fs_context fc; > > > > > + struct xfs_fs_context context; > > > > > + struct xfs_fs_context *ctx; > > > > > + int ret; > > > > > + > > > > > + memset(&fc, 0, sizeof(fc)); > > > > > + memset(&context, 0, sizeof(context)); > > > > > + fc.fs_private = &context; > > > > > + ctx = &context; > > > > > > > > I think you mentioned this ctx/context pattern would be removed > > > > from > > > > v2..? > > > > > > Except that, to minimise code churn the ctx variable is needed > > > because it will be used in the end result. > > > > > > I don't much like prefixing those references with &context even > > > if some happen to go away later on, the additional local variable > > > is clearer to read and provides usage consistency for the reader > > > over the changes. > > > > > > Ian > > > > Brian > > > > > > > > > + fc.s_fs_info = mp; > > > > > > > > > > /* > > > > > * set up the mount name first so all the errors will > > > > > refer to > > > > > the > > > > > @@ -413,16 +420,33 @@ xfs_parseargs( > > > > > goto done; > > > > > > > > > > while ((p = strsep(&options, ",")) != NULL) { > > > > > - int token; > > > > > - int ret; > > > > > + struct fs_parameter param; > > > > > + char *value; > > > > > > > > > > if (!*p) > > > > > continue; > > > > > > > > > > - token = match_token(p, tokens, args); > > > > > - ret = xfs_parse_param(token, p, args, mp, > > > > > - &dsunit, &dswidth, > > > > > &iosizelog); > > > > > - if (ret) > > > > > + param.key = p; > > > > > + param.type = fs_value_is_string; > > > > > + param.string = NULL; > > > > > + param.size = 0; > > > > > + > > > > > + value = strchr(p, '='); > > > > > + if (value) { > > > > > + *value++ = 0; > > > > > + param.size = strlen(value); > > > > > + if (param.size > 0) { > > > > > + param.string = > > > > > kmemdup_nul(value, > > > > > + para > > > > > m.size, > > > > > + GFP_ > > > > > KERNEL); > > > > > + if (!param.string) > > > > > + return -ENOMEM; > > > > > + } > > > > > + } > > > > > + > > > > > + ret = xfs_parse_param(&fc, ¶m); > > > > > + kfree(param.string); > > > > > + if (ret < 0) > > > > > return ret; > > > > > } > > > > > > > > > > @@ -435,7 +459,8 @@ xfs_parseargs( > > > > > return -EINVAL; > > > > > } > > > > > > > > > > - if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || > > > > > dswidth)) { > > > > > + if ((mp->m_flags & XFS_MOUNT_NOALIGN) && > > > > > + (ctx->dsunit || ctx->dswidth)) { > > > > > xfs_warn(mp, > > > > > "sunit and swidth options incompatible with the noalign > > > > > option"); > > > > > return -EINVAL; > > > > > @@ -448,28 +473,28 @@ xfs_parseargs( > > > > > } > > > > > #endif > > > > > > > > > > - if ((dsunit && !dswidth) || (!dsunit && dswidth)) { > > > > > + if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && > > > > > ctx- > > > > > > dswidth)) { > > > > > xfs_warn(mp, "sunit and swidth must be > > > > > specified > > > > > together"); > > > > > return -EINVAL; > > > > > } > > > > > > > > > > - if (dsunit && (dswidth % dsunit != 0)) { > > > > > + if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) { > > > > > xfs_warn(mp, > > > > > "stripe width (%d) must be a multiple of the stripe > > > > > unit (%d)", > > > > > - dswidth, dsunit); > > > > > + ctx->dswidth, ctx->dsunit); > > > > > return -EINVAL; > > > > > } > > > > > > > > > > done: > > > > > - if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) { > > > > > + if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) > > > > > { > > > > > /* > > > > > * At this point the superblock has not been > > > > > read > > > > > * in, therefore we do not know the block size. > > > > > * Before the mount call ends we will convert > > > > > * these to FSBs. > > > > > */ > > > > > - mp->m_dalign = dsunit; > > > > > - mp->m_swidth = dswidth; > > > > > + mp->m_dalign = ctx->dsunit; > > > > > + mp->m_swidth = ctx->dswidth; > > > > > } > > > > > > > > > > if (mp->m_logbufs != -1 && > > > > > @@ -491,18 +516,18 @@ xfs_parseargs( > > > > > return -EINVAL; > > > > > } > > > > > > > > > > - if (iosizelog) { > > > > > - if (iosizelog > XFS_MAX_IO_LOG || > > > > > - iosizelog < XFS_MIN_IO_LOG) { > > > > > + if (ctx->iosizelog) { > > > > > + if (ctx->iosizelog > XFS_MAX_IO_LOG || > > > > > + ctx->iosizelog < XFS_MIN_IO_LOG) { > > > > > xfs_warn(mp, "invalid log iosize: %d > > > > > [not %d- > > > > > %d]", > > > > > - iosizelog, XFS_MIN_IO_LOG, > > > > > + ctx->iosizelog, XFS_MIN_IO_LOG, > > > > > XFS_MAX_IO_LOG); > > > > > return -EINVAL; > > > > > } > > > > > > > > > > mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE; > > > > > - mp->m_readio_log = iosizelog; > > > > > - mp->m_writeio_log = iosizelog; > > > > > + mp->m_readio_log = ctx->iosizelog; > > > > > + mp->m_writeio_log = ctx->iosizelog; > > > > > } > > > > > > > > > > return 0; > > > > >
On Thu, 2019-09-26 at 05:14 +0100, Al Viro wrote: > On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote: > > > + opt = fs_parse(fc, &xfs_fs_parameters, param, &result); > > + if (opt < 0) { > > + /* > > + * If fs_parse() returns -ENOPARAM and the parameter > > + * is "source" the VFS needs to handle this option > > + * in order to boot otherwise use the default case > > + * below to handle invalid options. > > + */ > > + if (opt != -ENOPARAM || > > + strcmp(param->key, "source") == 0) > > + return opt; > > Just return opt; here and be done with that. The comment is bloody > misleading - for one thing, "in order to boot" is really "in order to > mount anything", and the only reason for the kludge is that the > default for "source" (in vfs_parse_fs_param(), triggered in case > when -ENOPARAM had been returned by ->parse_param()) won't get > triggered > if you insist on reporting _all_ unknown options on your own. > > > + } > > default: > > - xfs_warn(mp, "unknown mount option [%s].", p); > > + xfs_warn(mp, "unknown mount option [%s].", param->key); > > return -EINVAL; > > ... here, instead of letting the same vfs_parse_fs_param() handle > the warning. > > Or you could add Opt_source for handling that, with equivalent of > that > fallback (namely, > if (param->type != fs_value_is_string) > return invalf(fc, "VFS: Non-string source"); > if (fc->source) > return invalf(fc, "VFS: Multiple sources"); > fc->source = param->string; > param->string = NULL; > return 0; > ) done in your ->parse_param(). Either of those makes sense to me. The only other thing relevant to either case is messages not going to the kernel log if fsconfig() is being used which could make problem resolution more difficult. Any objection to changing logfc() to always log to the kernel log and save messages to the context if fc->log is non-null rather than the either or behaviour we have now? Ian
On Thu, 2019-09-26 at 15:06 +0800, Ian Kent wrote: > On Thu, 2019-09-26 at 05:14 +0100, Al Viro wrote: > > On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote: > > > > > + opt = fs_parse(fc, &xfs_fs_parameters, param, &result); > > > + if (opt < 0) { > > > + /* > > > + * If fs_parse() returns -ENOPARAM and the parameter > > > + * is "source" the VFS needs to handle this option > > > + * in order to boot otherwise use the default case > > > + * below to handle invalid options. > > > + */ > > > + if (opt != -ENOPARAM || > > > + strcmp(param->key, "source") == 0) > > > + return opt; > > > > Just return opt; here and be done with that. The comment is bloody > > misleading - for one thing, "in order to boot" is really "in order > > to > > mount anything", and the only reason for the kludge is that the > > default for "source" (in vfs_parse_fs_param(), triggered in case > > when -ENOPARAM had been returned by ->parse_param()) won't get > > triggered > > if you insist on reporting _all_ unknown options on your own. > > > > > + } > > > default: > > > - xfs_warn(mp, "unknown mount option [%s].", p); > > > + xfs_warn(mp, "unknown mount option [%s].", param->key); > > > return -EINVAL; > > > > ... here, instead of letting the same vfs_parse_fs_param() handle > > the warning. > > > > Or you could add Opt_source for handling that, with equivalent of > > that > > fallback (namely, > > if (param->type != fs_value_is_string) > > return invalf(fc, "VFS: Non-string > > source"); > > if (fc->source) > > return invalf(fc, "VFS: Multiple sources"); > > fc->source = param->string; > > param->string = NULL; > > return 0; > > ) done in your ->parse_param(). > > Either of those makes sense to me. > > The only other thing relevant to either case is messages not going > to the kernel log if fsconfig() is being used which could make > problem > resolution more difficult. > > Any objection to changing logfc() to always log to the kernel log > and save messages to the context if fc->log is non-null rather than > the either or behaviour we have now? Actually, forget about this. That "e", "w" and "i" will attract inconsistent log formatting comments. Probably simplest to just add an xfs log macro to log to the kernel log and also use the mount-api log macro if context ->log is non-null. > > Ian >
Ian Kent <raven@themaw.net> wrote: > The only other thing relevant to either case is messages not going > to the kernel log if fsconfig() is being used which could make problem > resolution more difficult. Maybe we should just remove this entirely. It's only partially capable since I wasn't allowed to add a per-task message buffer, so it can't help you with interior mounts - as done by NFS - or automounts. David
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index b04aebab69ab..6792d46fa0be 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s, unsigned int base, int *res) return ret; } -STATIC int -match_kstrtoint(const substring_t *s, unsigned int base, int *res) -{ - const char *value; - int ret; - - value = match_strdup(s); - if (!value) - return -ENOMEM; - ret = suffix_kstrtoint(value, base, res); - kfree(value); - return ret; -} +struct xfs_fs_context { + int dsunit; + int dswidth; + uint8_t iosizelog; +}; STATIC int xfs_parse_param( - int token, - char *p, - substring_t *args, - struct xfs_mount *mp, - int *dsunit, - int *dswidth, - uint8_t *iosizelog) + struct fs_context *fc, + struct fs_parameter *param) { + struct xfs_fs_context *ctx = fc->fs_private; + struct xfs_mount *mp = fc->s_fs_info; + struct fs_parse_result result; int iosize = 0; + int opt; - switch (token) { + opt = fs_parse(fc, &xfs_fs_parameters, param, &result); + if (opt < 0) { + /* + * If fs_parse() returns -ENOPARAM and the parameter + * is "source" the VFS needs to handle this option + * in order to boot otherwise use the default case + * below to handle invalid options. + */ + if (opt != -ENOPARAM || + strcmp(param->key, "source") == 0) + return opt; + } + + switch (opt) { case Opt_logbufs: - if (match_int(args, &mp->m_logbufs)) - return -EINVAL; + mp->m_logbufs = result.uint_32; break; case Opt_logbsize: - if (match_kstrtoint(args, 10, &mp->m_logbsize)) + if (suffix_kstrtoint(param->string, 10, &mp->m_logbsize)) return -EINVAL; break; case Opt_logdev: kfree(mp->m_logname); - mp->m_logname = match_strdup(args); + mp->m_logname = kstrdup(param->string, GFP_KERNEL); if (!mp->m_logname) return -ENOMEM; break; case Opt_rtdev: kfree(mp->m_rtname); - mp->m_rtname = match_strdup(args); + mp->m_rtname = kstrdup(param->string, GFP_KERNEL); if (!mp->m_rtname) return -ENOMEM; break; case Opt_allocsize: - if (match_kstrtoint(args, 10, &iosize)) + if (suffix_kstrtoint(param->string, 10, &iosize)) return -EINVAL; - *iosizelog = ffs(iosize) - 1; + ctx->iosizelog = ffs(iosize) - 1; break; case Opt_grpid: case Opt_bsdgroups: @@ -264,12 +267,10 @@ xfs_parse_param( mp->m_flags |= XFS_MOUNT_SWALLOC; break; case Opt_sunit: - if (match_int(args, dsunit)) - return -EINVAL; + ctx->dsunit = result.uint_32; break; case Opt_swidth: - if (match_int(args, dswidth)) - return -EINVAL; + ctx->dswidth = result.uint_32; break; case Opt_inode32: mp->m_flags |= XFS_MOUNT_SMALL_INUMS; @@ -348,7 +349,7 @@ xfs_parse_param( break; #endif default: - xfs_warn(mp, "unknown mount option [%s].", p); + xfs_warn(mp, "unknown mount option [%s].", param->key); return -EINVAL; } @@ -373,10 +374,16 @@ xfs_parseargs( { const struct super_block *sb = mp->m_super; char *p; - substring_t args[MAX_OPT_ARGS]; - int dsunit = 0; - int dswidth = 0; - uint8_t iosizelog = 0; + struct fs_context fc; + struct xfs_fs_context context; + struct xfs_fs_context *ctx; + int ret; + + memset(&fc, 0, sizeof(fc)); + memset(&context, 0, sizeof(context)); + fc.fs_private = &context; + ctx = &context; + fc.s_fs_info = mp; /* * set up the mount name first so all the errors will refer to the @@ -413,16 +420,33 @@ xfs_parseargs( goto done; while ((p = strsep(&options, ",")) != NULL) { - int token; - int ret; + struct fs_parameter param; + char *value; if (!*p) continue; - token = match_token(p, tokens, args); - ret = xfs_parse_param(token, p, args, mp, - &dsunit, &dswidth, &iosizelog); - if (ret) + param.key = p; + param.type = fs_value_is_string; + param.string = NULL; + param.size = 0; + + value = strchr(p, '='); + if (value) { + *value++ = 0; + param.size = strlen(value); + if (param.size > 0) { + param.string = kmemdup_nul(value, + param.size, + GFP_KERNEL); + if (!param.string) + return -ENOMEM; + } + } + + ret = xfs_parse_param(&fc, ¶m); + kfree(param.string); + if (ret < 0) return ret; } @@ -435,7 +459,8 @@ xfs_parseargs( return -EINVAL; } - if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || dswidth)) { + if ((mp->m_flags & XFS_MOUNT_NOALIGN) && + (ctx->dsunit || ctx->dswidth)) { xfs_warn(mp, "sunit and swidth options incompatible with the noalign option"); return -EINVAL; @@ -448,28 +473,28 @@ xfs_parseargs( } #endif - if ((dsunit && !dswidth) || (!dsunit && dswidth)) { + if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx->dswidth)) { xfs_warn(mp, "sunit and swidth must be specified together"); return -EINVAL; } - if (dsunit && (dswidth % dsunit != 0)) { + if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) { xfs_warn(mp, "stripe width (%d) must be a multiple of the stripe unit (%d)", - dswidth, dsunit); + ctx->dswidth, ctx->dsunit); return -EINVAL; } done: - if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) { + if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) { /* * At this point the superblock has not been read * in, therefore we do not know the block size. * Before the mount call ends we will convert * these to FSBs. */ - mp->m_dalign = dsunit; - mp->m_swidth = dswidth; + mp->m_dalign = ctx->dsunit; + mp->m_swidth = ctx->dswidth; } if (mp->m_logbufs != -1 && @@ -491,18 +516,18 @@ xfs_parseargs( return -EINVAL; } - if (iosizelog) { - if (iosizelog > XFS_MAX_IO_LOG || - iosizelog < XFS_MIN_IO_LOG) { + if (ctx->iosizelog) { + if (ctx->iosizelog > XFS_MAX_IO_LOG || + ctx->iosizelog < XFS_MIN_IO_LOG) { xfs_warn(mp, "invalid log iosize: %d [not %d-%d]", - iosizelog, XFS_MIN_IO_LOG, + ctx->iosizelog, XFS_MIN_IO_LOG, XFS_MAX_IO_LOG); return -EINVAL; } mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE; - mp->m_readio_log = iosizelog; - mp->m_writeio_log = iosizelog; + mp->m_readio_log = ctx->iosizelog; + mp->m_writeio_log = ctx->iosizelog; } return 0;
Make xfs_parse_param() take arguments of the fs context operation .parse_param() in preparation for switching to use the file system mount context for mount. The function fc_parse() only uses the file system context (fc here) when calling log macros warnf() and invalf() which in turn check only the fc->log field to determine if the message should be saved to a context buffer (for later retrival by userspace) or logged using printk(). Also the temporary function match_kstrtoint() is now unused, remove it. Signed-off-by: Ian Kent <raven@themaw.net> --- fs/xfs/xfs_super.c | 137 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 81 insertions(+), 56 deletions(-)