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@xxxxxxxxxx>
> > > > > ---
> > > > > 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;
> > > > >
