| Message ID | 156652198391.2607.14772471190581142304.stgit@fedora-28 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: mount API patch series | expand |
On 8/22/19 7:59 PM, Ian Kent wrote: > Make xfs_parse_param() take arguments of the fs context operation > .parse_param() in preperation 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 | 187 ++++++++++++++++++++++++++++++---------------------- > 1 file changed, 108 insertions(+), 79 deletions(-) > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 3ae29938dd89..754d2ccfd960 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -184,64 +184,62 @@ 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) > + return opt; > + > + switch (opt) { > case Opt_logbufs: > - if (match_int(args, &mp->m_logbufs)) > - return -EINVAL; > + mp->m_logbufs = result.uint_32; > break; I'm not all that excited about how xfs_fs_parameters defines Opt_logbufs as a u32 result, but we still have to hard-code the grabbing of the u32 result after fs_parse(). I know that's a core API thing but I wonder if there's any way to connect it more strongly so they don't have to stay in sync. Perhaps a new function, mp->m_logbufs = fs_parse_result(&xfs_fs_parameters, &result); and let fs_parse_result() pick the right part of the union? Would be an api enhancement not for this patch I guess. > 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: > case Opt_biosize: > - 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: > + if (result.negated) > + mp->m_flags &= ~XFS_MOUNT_GRPID; > + else > + mp->m_flags |= XFS_MOUNT_GRPID; > + break; Is there any real advantage to this "fsparam_flag_no" / negated stuff? I don't see any other filesystem using it (yet) and I'm not totally convinced that this is any better, more readable, or more efficient than just keeping the "Opt_nogrpid" stuff around. Not a dealbreaker but just thinking out loud... seems like this interface was a solution in search of a problem? > case Opt_bsdgroups: > mp->m_flags |= XFS_MOUNT_GRPID; > break; > - case Opt_nogrpid: > case Opt_sysvgroups: > mp->m_flags &= ~XFS_MOUNT_GRPID; > break; > @@ -258,12 +256,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; > @@ -275,33 +271,38 @@ xfs_parse_param( > mp->m_flags |= XFS_MOUNT_NOUUID; > break; > case Opt_ikeep: > - mp->m_flags |= XFS_MOUNT_IKEEP; > - break; > - case Opt_noikeep: > - mp->m_flags &= ~XFS_MOUNT_IKEEP; > + if (result.negated) > + mp->m_flags &= ~XFS_MOUNT_IKEEP; > + else > + mp->m_flags |= XFS_MOUNT_IKEEP; > break; > case Opt_largeio: > - mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE; > - break; > - case Opt_nolargeio: > - mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE; > + if (result.negated) > + mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE; > + else > + mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE; > break; > case Opt_attr2: > - mp->m_flags |= XFS_MOUNT_ATTR2; > - break; > - case Opt_noattr2: > - mp->m_flags &= ~XFS_MOUNT_ATTR2; > - mp->m_flags |= XFS_MOUNT_NOATTR2; > + if (!result.negated) { > + mp->m_flags |= XFS_MOUNT_ATTR2; > + } else { > + mp->m_flags &= ~XFS_MOUNT_ATTR2; > + mp->m_flags |= XFS_MOUNT_NOATTR2; > + } > break > case Opt_filestreams: > mp->m_flags |= XFS_MOUNT_FILESTREAMS; > break; > - case Opt_noquota: > - mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT; > - mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD; > - mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE; > - break; > case Opt_quota: > + if (!result.negated) { > + mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE | > + XFS_UQUOTA_ENFD); > + } else { > + mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT; > + mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD; > + mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE; > + } > + break; > case Opt_uquota: > case Opt_usrquota: > mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE | > @@ -331,10 +332,10 @@ xfs_parse_param( > mp->m_qflags &= ~XFS_GQUOTA_ENFD; > break; > case Opt_discard: > - mp->m_flags |= XFS_MOUNT_DISCARD; > - break; > - case Opt_nodiscard: > - mp->m_flags &= ~XFS_MOUNT_DISCARD; > + if (result.negated) > + mp->m_flags &= ~XFS_MOUNT_DISCARD; > + else > + mp->m_flags |= XFS_MOUNT_DISCARD; > break; > #ifdef CONFIG_FS_DAX > case Opt_dax: > @@ -342,7 +343,7 @@ xfs_parse_param( > break; > #endif > default: > - xfs_warn(mp, "unknown mount option [%s].", p); > + xfs_warn(mp, "unknown mount option [%s].", param->key); Hm, we don't ever seem to get here: # mount -o fweeeep /dev/pmem0p1 /mnt/test mount: mount /dev/pmem0p1 on /mnt/test failed: Unknown error 519 # and nothing in dmesg. we never get a default because fs_parse() returns an error and we never drop into the case statement at all. (again maybe this all goes away by the last patch...) <and now after testing this, the kernel paniced.... :( > > return -EINVAL; > } > > @@ -367,10 +368,10 @@ 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 = &context; > > /* > * set up the mount name first so all the errors will refer to the > @@ -406,17 +407,41 @@ xfs_parseargs( > if (!options) > goto done; > > + memset(&fc, 0, sizeof(fc)); > + memset(&ctx, 0, sizeof(ctx)); I think you mean: + memset(ctx, 0, sizeof(*ctx)); or maybe better, just: + memset(&context, 0, sizeof(context)); here? Otherwise you're essentially just doing ctx = NULL (thanks djwong for looking over my shoulder there) which then means fs_private is NULL, and ... Well, I was going to demonstrate that it probably oopses with certain mount options, but actually I couldn't boot with patches applied up to this point: [ 18.491901] SGI XFS with ACLs, security attributes, realtime, scrub, repair, debug enabled [ 18.502444] XFS (dm-0): invalid log iosize: 184 [not 12-30] Fixing up the problem above (which I thought would lead to a null ptr deref but did not ...) I still fail to boot with: [ 18.191504] XFS (dm-0): alignment check failed: sunit/swidth vs. blocksize(4096) This is because if you goto done on no options you do it before you've initialized the context structure, so it grabs uninitialized values for all the context fields. Probably best to move the structure init right to the top for clarity. I think this all goes away by the last patch but we still want to make it bisectable... > + fc.fs_private = ctx; > + fc.s_fs_info = mp; > + > while ((p = strsep(&options, ",")) != NULL) { > - int token; > - int ret; > + struct fs_parameter param; > + char *value; > + int ret; > > 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.size = 0; > + > + value = strchr(p, '='); > + if (value) { > + if (value == p) > + continue; > + *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; > } > > @@ -429,7 +454,7 @@ xfs_parseargs( > return -EINVAL; > } > > - if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || dswidth)) { > + if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (ctx->dsunit || ctx->dswidth)) { it'd be nice to reflow this to 80 cols or less (maybe just drop the last space, or break the line at the &&) > xfs_warn(mp, > "sunit and swidth options incompatible with the noalign option"); > return -EINVAL; > @@ -442,28 +467,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 && > @@ -485,18 +510,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; > @@ -1914,6 +1939,10 @@ static const struct super_operations xfs_super_operations = { > .free_cached_objects = xfs_fs_free_cached_objects, > }; > > +static const struct fs_context_operations xfs_context_ops = { > + .parse_param = xfs_parse_param, > +}; This seems weird in this patch, it's not used until "xfs: mount-api - switch to new mount-api" so might want to move it there? ok after testing a little I paniced the box, gonna go off and see what that's all about now. -Eric > + > static struct file_system_type xfs_fs_type = { > .owner = THIS_MODULE, > .name = "xfs", >
On 8/26/19 2:19 PM, Eric Sandeen wrote: >> case Opt_biosize: >> - 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: >> + if (result.negated) >> + mp->m_flags &= ~XFS_MOUNT_GRPID; >> + else >> + mp->m_flags |= XFS_MOUNT_GRPID; >> + break; > Is there any real advantage to this "fsparam_flag_no" / negated stuff? > I don't see any other filesystem using it (yet) and I'm not totally convinced > that this is any better, more readable, or more efficient than just keeping > the "Opt_nogrpid" stuff around. Not a dealbreaker but just thinking out > loud... seems like this interface was a solution in search of a problem? Also, at least as of this patch, it seems broken: [xfstests-dev]# mount -o noikeep /dev/pmem0p1 /mnt/test mount: mount /dev/pmem0p1 on /mnt/test failed: Unknown error 519 <dmesg shows nothing> [xfstests-dev]# mount -o ikeep /dev/pmem0p1 /mnt/test mount: wrong fs type, bad option, bad superblock on /dev/pmem0p1, missing codepage or helper program, or other error In some cases useful info is found in syslog - try dmesg | tail or so. [xfstests-dev]# dmesg | tail -n 1 [ 282.281557] XFS: Unexpected value for 'ikeep'
On 8/26/19 2:31 PM, Eric Sandeen wrote: > On 8/26/19 2:19 PM, Eric Sandeen wrote: >>> case Opt_biosize: >>> - 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: >>> + if (result.negated) >>> + mp->m_flags &= ~XFS_MOUNT_GRPID; >>> + else >>> + mp->m_flags |= XFS_MOUNT_GRPID; >>> + break; >> Is there any real advantage to this "fsparam_flag_no" / negated stuff? >> I don't see any other filesystem using it (yet) and I'm not totally convinced >> that this is any better, more readable, or more efficient than just keeping >> the "Opt_nogrpid" stuff around. Not a dealbreaker but just thinking out >> loud... seems like this interface was a solution in search of a problem? > > Also, at least as of this patch, it seems broken: > > [xfstests-dev]# mount -o noikeep /dev/pmem0p1 /mnt/test > mount: mount /dev/pmem0p1 on /mnt/test failed: Unknown error 519 > > <dmesg shows nothing> > > [xfstests-dev]# mount -o ikeep /dev/pmem0p1 /mnt/test > mount: wrong fs type, bad option, bad superblock on /dev/pmem0p1, > missing codepage or helper program, or other error > > In some cases useful info is found in syslog - try > dmesg | tail or so. > [xfstests-dev]# dmesg | tail -n 1 > [ 282.281557] XFS: Unexpected value for 'ikeep' and it panics shortly after these failures. [ 378.761698] WARNING: CPU: 8 PID: 12477 at kernel/rcu/tree.c:2498 __call_rcu+0x1e9/0x290 [ 378.770633] Modules linked in: macsec tcp_diag udp_diag inet_diag unix_diag af_packet_diag netlink_diag xt_CHECKSUM iptable_mangle xt_MASQUERADE iptable_nat nf_nat xt_conntrack nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ipt_REJECT nf_reject_ipv4 tun bridge stp llc ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter intel_rapl_msr intel_rapl_common sunrpc x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel nd_pmem nd_btt iTCO_wdt intel_cstate ipmi_si iTCO_vendor_support mei_me ipmi_devintf intel_uncore intel_rapl_perf mei ipmi_msghandler wmi nd_e820 libnvdimm pcspkr sg i2c_i801 ioatdma lpc_ich binfmt_misc ip_tables xfs(O) libcrc32c sd_mod mgag200 drm_vram_helper ttm drm_kms_helper syscopyarea igb crc32c_intel sysfillrect sysimgblt isci fb_sys_fops ptp libsas ahci pps_core drm scsi_transport_sas libahci dca i2c_algo_bit libata i2c_core dm_mirror dm_region_hash dm_log dm_mod [ 378.863955] CPU: 8 PID: 12477 Comm: bash Kdump: loaded Tainted: G O 5.3.0-rc2+ #12 [ 378.873860] Hardware name: Intel Corporation LH Pass/SVRBD-ROW_P, BIOS SE5C600.86B.02.01.SP06.050920141054 05/09/2014 [ 378.885696] RIP: 0010:__call_rcu+0x1e9/0x290 [ 378.890460] Code: 0f 84 8d 00 00 00 48 89 83 b0 00 00 00 48 8b 83 98 00 00 00 48 89 83 a8 00 00 00 e9 b4 fe ff ff e8 2c c8 ff ff e9 5f ff ff ff <0f> 0b 0f 1f 44 00 00 e9 31 fe ff ff 83 fa ff 0f 1f 84 00 00 00 00 [ 378.911418] RSP: 0018:ffffc900070a3de0 EFLAGS: 00010206 [ 378.917247] RAX: 0000000080000000 RBX: ffff888ffc6eade5 RCX: 0000000000000000 [ 378.925210] RDX: 00000000ffffffff RSI: ffffffff81747450 RDI: ffff888ffc6eafbd [ 378.933174] RBP: ffff888ffc6eafbd R08: 0000000000000000 R09: ffff888ffc6eaec5 [ 378.941135] R10: ffff88901567c880 R11: ffff889000844a10 R12: ffff888ffc6eafb5 [ 378.949098] R13: ffff888103e20750 R14: ffffffff81747450 R15: 0000000000000000 [ 378.957062] FS: 00007f08a102b740(0000) GS:ffff88901ea00000(0000) knlGS:0000000000000000 [ 378.966092] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 378.972505] CR2: 00007f08a1037000 CR3: 0000000ffc77a004 CR4: 00000000001606e0 [ 378.980469] Call Trace: [ 378.983208] netlink_release+0x2b1/0x540 [ 378.987586] __sock_release+0x3d/0xc0 [ 378.991673] sock_close+0x11/0x20 [ 378.995373] __fput+0xbe/0x250 [ 378.998784] task_work_run+0x88/0xa0 [ 379.002775] exit_to_usermode_loop+0x77/0xc7 [ 379.007537] do_syscall_64+0x1a1/0x1d0 [ 379.011721] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 379.017361] RIP: 0033:0x7f08a0714620 [ 379.021348] Code: 00 64 c7 00 0d 00 00 00 b8 ff ff ff ff eb 90 b8 ff ff ff ff eb 89 0f 1f 40 00 83 3d 7d c9 2d 00 00 75 10 b8 03 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 9e c5 01 00 48 89 04 24 [ 379.042303] RSP: 002b:00007ffdef07c748 EFLAGS: 00000246 ORIG_RAX: 0000000000000003 [ 379.050752] RAX: 0000000000000000 RBX: 0000000001e0cc70 RCX: 00007f08a0714620 [ 379.058715] RDX: 0000000000000000 RSI: 00007ffdef07c790 RDI: 0000000000000003 [ 379.066679] RBP: 0000000000000003 R08: 00007ffdef07c6a0 R09: 00007ffdef07c5f0 [ 379.074640] R10: 0000000000000008 R11: 0000000000000246 R12: 000000000000001c [ 379.082602] R13: 0000000000000001 R14: 0000000000000002 R15: 00007ffdef07c930 [ 379.090565] ---[ end trace 0146f578893eb498 ]---
On Fri, Aug 23, 2019 at 08:59:43AM +0800, Ian Kent wrote: > Make xfs_parse_param() take arguments of the fs context operation > .parse_param() in preperation 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> > --- I see Eric had some feedback on this patch already. Some additional notes (which may overlap)... > fs/xfs/xfs_super.c | 187 ++++++++++++++++++++++++++++++---------------------- > 1 file changed, 108 insertions(+), 79 deletions(-) > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 3ae29938dd89..754d2ccfd960 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c ... > @@ -275,33 +271,38 @@ xfs_parse_param( > mp->m_flags |= XFS_MOUNT_NOUUID; > break; > case Opt_ikeep: > - mp->m_flags |= XFS_MOUNT_IKEEP; > - break; > - case Opt_noikeep: > - mp->m_flags &= ~XFS_MOUNT_IKEEP; > + if (result.negated) > + mp->m_flags &= ~XFS_MOUNT_IKEEP; > + else > + mp->m_flags |= XFS_MOUNT_IKEEP; > break; > case Opt_largeio: > - mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE; > - break; > - case Opt_nolargeio: > - mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE; > + if (result.negated) > + mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE; > + else > + mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE; > break; > case Opt_attr2: > - mp->m_flags |= XFS_MOUNT_ATTR2; > - break; > - case Opt_noattr2: > - mp->m_flags &= ~XFS_MOUNT_ATTR2; > - mp->m_flags |= XFS_MOUNT_NOATTR2; > + if (!result.negated) { > + mp->m_flags |= XFS_MOUNT_ATTR2; > + } else { > + mp->m_flags &= ~XFS_MOUNT_ATTR2; > + mp->m_flags |= XFS_MOUNT_NOATTR2; > + } Eric's comments aside, it would be nice to have some consistency between the various result.negated checks (i.e. 'if (negated)' vs 'if (!negated)'). > break; > case Opt_filestreams: > mp->m_flags |= XFS_MOUNT_FILESTREAMS; > break; > - case Opt_noquota: > - mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT; > - mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD; > - mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE; > - break; > case Opt_quota: > + if (!result.negated) { > + mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE | > + XFS_UQUOTA_ENFD); > + } else { > + mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT; > + mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD; > + mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE; > + } > + break; > case Opt_uquota: > case Opt_usrquota: > mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE | ... > @@ -367,10 +368,10 @@ 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 = &context; I don't really see the point for having a separate pointer variable based on the code so far. Why not just do: struct xfs_fs_context ctx = {0,}; ... and pass by reference where necessary? > > /* > * set up the mount name first so all the errors will refer to the > @@ -406,17 +407,41 @@ xfs_parseargs( > if (!options) > goto done; > > + memset(&fc, 0, sizeof(fc)); > + memset(&ctx, 0, sizeof(ctx)); > + fc.fs_private = ctx; > + fc.s_fs_info = mp; > + > while ((p = strsep(&options, ",")) != NULL) { > - int token; > - int ret; > + struct fs_parameter param; > + char *value; > + int ret; > > 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.size = 0; > + > + value = strchr(p, '='); > + if (value) { > + if (value == p) > + continue; What's the purpose of the above check? Why do we skip the param as opposed to return an error or something? > + *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; > } > ... > @@ -1914,6 +1939,10 @@ static const struct super_operations xfs_super_operations = { > .free_cached_objects = xfs_fs_free_cached_objects, > }; > > +static const struct fs_context_operations xfs_context_ops = { > + .parse_param = xfs_parse_param, > +}; > + It's probably better to introduce this in the first patch where it's used. Brian > static struct file_system_type xfs_fs_type = { > .owner = THIS_MODULE, > .name = "xfs", >
On Tue, Aug 27, 2019 at 08:41:20AM -0400, Brian Foster wrote: > On Fri, Aug 23, 2019 at 08:59:43AM +0800, Ian Kent wrote: > > Make xfs_parse_param() take arguments of the fs context operation > > .parse_param() in preperation 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> > > --- > > I see Eric had some feedback on this patch already. Some additional > notes (which may overlap)... > > > fs/xfs/xfs_super.c | 187 ++++++++++++++++++++++++++++++---------------------- > > 1 file changed, 108 insertions(+), 79 deletions(-) > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > index 3ae29938dd89..754d2ccfd960 100644 > > --- a/fs/xfs/xfs_super.c > > +++ b/fs/xfs/xfs_super.c > ... > > @@ -275,33 +271,38 @@ xfs_parse_param( > > mp->m_flags |= XFS_MOUNT_NOUUID; > > break; > > case Opt_ikeep: > > - mp->m_flags |= XFS_MOUNT_IKEEP; > > - break; > > - case Opt_noikeep: > > - mp->m_flags &= ~XFS_MOUNT_IKEEP; > > + if (result.negated) > > + mp->m_flags &= ~XFS_MOUNT_IKEEP; > > + else > > + mp->m_flags |= XFS_MOUNT_IKEEP; > > break; > > case Opt_largeio: > > - mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE; > > - break; > > - case Opt_nolargeio: > > - mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE; > > + if (result.negated) > > + mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE; > > + else > > + mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE; > > break; > > case Opt_attr2: > > - mp->m_flags |= XFS_MOUNT_ATTR2; > > - break; > > - case Opt_noattr2: > > - mp->m_flags &= ~XFS_MOUNT_ATTR2; > > - mp->m_flags |= XFS_MOUNT_NOATTR2; > > + if (!result.negated) { > > + mp->m_flags |= XFS_MOUNT_ATTR2; > > + } else { > > + mp->m_flags &= ~XFS_MOUNT_ATTR2; > > + mp->m_flags |= XFS_MOUNT_NOATTR2; > > + } > > Eric's comments aside, it would be nice to have some consistency between > the various result.negated checks (i.e. 'if (negated)' vs 'if > (!negated)'). Frankly I'd prefer "if (result.enabled)" or something affirmative so I don't have to twist my brain around !negated == MAIN SCREEN TURN ON. --D > > break; > > case Opt_filestreams: > > mp->m_flags |= XFS_MOUNT_FILESTREAMS; > > break; > > - case Opt_noquota: > > - mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT; > > - mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD; > > - mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE; > > - break; > > case Opt_quota: > > + if (!result.negated) { > > + mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE | > > + XFS_UQUOTA_ENFD); > > + } else { > > + mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT; > > + mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD; > > + mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE; > > + } > > + break; > > case Opt_uquota: > > case Opt_usrquota: > > mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE | > ... > > @@ -367,10 +368,10 @@ 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 = &context; > > I don't really see the point for having a separate pointer variable > based on the code so far. Why not just do: > > struct xfs_fs_context ctx = {0,}; > > ... and pass by reference where necessary? > > > > > /* > > * set up the mount name first so all the errors will refer to the > > @@ -406,17 +407,41 @@ xfs_parseargs( > > if (!options) > > goto done; > > > > + memset(&fc, 0, sizeof(fc)); > > + memset(&ctx, 0, sizeof(ctx)); > > + fc.fs_private = ctx; > > + fc.s_fs_info = mp; > > + > > while ((p = strsep(&options, ",")) != NULL) { > > - int token; > > - int ret; > > + struct fs_parameter param; > > + char *value; > > + int ret; > > > > 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.size = 0; > > + > > + value = strchr(p, '='); > > + if (value) { > > + if (value == p) > > + continue; > > What's the purpose of the above check? Why do we skip the param as > opposed to return an error or something? > > > + *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; > > } > > > ... > > @@ -1914,6 +1939,10 @@ static const struct super_operations xfs_super_operations = { > > .free_cached_objects = xfs_fs_free_cached_objects, > > }; > > > > +static const struct fs_context_operations xfs_context_ops = { > > + .parse_param = xfs_parse_param, > > +}; > > + > > It's probably better to introduce this in the first patch where it's used. > > Brian > > > static struct file_system_type xfs_fs_type = { > > .owner = THIS_MODULE, > > .name = "xfs", > >
On 8/27/19 10:10 AM, Darrick J. Wong wrote: >> Eric's comments aside, it would be nice to have some consistency between >> the various result.negated checks (i.e. 'if (negated)' vs 'if >> (!negated)'). > Frankly I'd prefer "if (result.enabled)" or something affirmative so I > don't have to twist my brain around !negated == MAIN SCREEN TURN ON. > > --D I think it's probably possible to just keep explicit Opt_ikeep / Opt_noikeep and dispense with the whole negated/enabled parsing, unless there's a downside to doing that. Not quite sure what's best, but it also exposes the inconsistency we have with flag-ish options - some have negation of defaults as well as ability to restate defaults, others don't. (i.e. we have nouuid but not "uuid," norecovery but not "recovery," but we have ikeep/noikeep, attr2/noattr2...) -Eric
On Tue, 2019-08-27 at 10:15 -0500, Eric Sandeen wrote: > On 8/27/19 10:10 AM, Darrick J. Wong wrote: > > > Eric's comments aside, it would be nice to have some consistency > > > between > > > the various result.negated checks (i.e. 'if (negated)' vs 'if > > > (!negated)'). > > Frankly I'd prefer "if (result.enabled)" or something affirmative > > so I > > don't have to twist my brain around !negated == MAIN SCREEN TURN > > ON. > > > > --D > > I think it's probably possible to just keep explicit Opt_ikeep / > Opt_noikeep > and dispense with the whole negated/enabled parsing, unless there's a > downside > to doing that. Not quite sure what's best, but it also exposes the > inconsistency > we have with flag-ish options - some have negation of defaults as > well as ability > to restate defaults, others don't. (i.e. we have nouuid but not > "uuid," > norecovery but not "recovery," but we have ikeep/noikeep, > attr2/noattr2...) As far as I know there's nothing that mandates using the type that marks these as having a corresponding "no" option. Not using it would also eliminate my compultion to always have the short block in "if () {} else {}" first (at least mostly). I'll do that when I'm fixing up the problems Eric has described. Thanks Eric and Brian (and for that matter, Darrick too) for spending time looking at the patches. btw, I will get around to replying to the comments that have been made, just not straight away, since I want to have a look at the option parsing problem that Eric has seen first. Ian
On Mon, 2019-08-26 at 14:31 -0500, Eric Sandeen wrote: Finally got time to start looking at this. > On 8/26/19 2:19 PM, Eric Sandeen wrote: > > > case Opt_biosize: > > > - 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: > > > + if (result.negated) > > > + mp->m_flags &= ~XFS_MOUNT_GRPID; > > > + else > > > + mp->m_flags |= XFS_MOUNT_GRPID; > > > + break; > > Is there any real advantage to this "fsparam_flag_no" / negated > > stuff? > > I don't see any other filesystem using it (yet) and I'm not totally > > convinced > > that this is any better, more readable, or more efficient than just > > keeping > > the "Opt_nogrpid" stuff around. Not a dealbreaker but just > > thinking out > > loud... seems like this interface was a solution in search of a > > problem? > > Also, at least as of this patch, it seems broken: > > [xfstests-dev]# mount -o noikeep /dev/pmem0p1 /mnt/test > mount: mount /dev/pmem0p1 on /mnt/test failed: Unknown error 519 > > <dmesg shows nothing> > > [xfstests-dev]# mount -o ikeep /dev/pmem0p1 /mnt/test > mount: wrong fs type, bad option, bad superblock on /dev/pmem0p1, > missing codepage or helper program, or other error > > In some cases useful info is found in syslog - try > dmesg | tail or so. > [xfstests-dev]# dmesg | tail -n 1 > [ 282.281557] XFS: Unexpected value for 'ikeep' Bizare, everything I'm looking at says this case shouldn't trigger! Think I'm going to need to burn some grey cells on this, ;) Ian
On Tue, 2019-08-27 at 08:41 -0400, Brian Foster wrote: > On Fri, Aug 23, 2019 at 08:59:43AM +0800, Ian Kent wrote: > > Make xfs_parse_param() take arguments of the fs context operation > > .parse_param() in preperation 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> > > --- > > I see Eric had some feedback on this patch already. Some additional > notes (which may overlap)... > > > fs/xfs/xfs_super.c | 187 ++++++++++++++++++++++++++++++-------- > > -------------- > > 1 file changed, 108 insertions(+), 79 deletions(-) > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > index 3ae29938dd89..754d2ccfd960 100644 > > --- a/fs/xfs/xfs_super.c > > +++ b/fs/xfs/xfs_super.c > ... > > @@ -275,33 +271,38 @@ xfs_parse_param( > > mp->m_flags |= XFS_MOUNT_NOUUID; > > break; > > case Opt_ikeep: > > - mp->m_flags |= XFS_MOUNT_IKEEP; > > - break; > > - case Opt_noikeep: > > - mp->m_flags &= ~XFS_MOUNT_IKEEP; > > + if (result.negated) > > + mp->m_flags &= ~XFS_MOUNT_IKEEP; > > + else > > + mp->m_flags |= XFS_MOUNT_IKEEP; > > break; > > case Opt_largeio: > > - mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE; > > - break; > > - case Opt_nolargeio: > > - mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE; > > + if (result.negated) > > + mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE; > > + else > > + mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE; > > break; > > case Opt_attr2: > > - mp->m_flags |= XFS_MOUNT_ATTR2; > > - break; > > - case Opt_noattr2: > > - mp->m_flags &= ~XFS_MOUNT_ATTR2; > > - mp->m_flags |= XFS_MOUNT_NOATTR2; > > + if (!result.negated) { > > + mp->m_flags |= XFS_MOUNT_ATTR2; > > + } else { > > + mp->m_flags &= ~XFS_MOUNT_ATTR2; > > + mp->m_flags |= XFS_MOUNT_NOATTR2; > > + } > > Eric's comments aside, it would be nice to have some consistency > between > the various result.negated checks (i.e. 'if (negated)' vs 'if > (!negated)'). Right, that's my "the smallest block must always be first 'ism" cutting in there. I always feel that a larger block before a smaller block makes the later (partially) invisible in reading the code. But the fact is that seeing this annoys me so I always see the hanging code so what I'm saying is kind-off nonsence! I'll try and overcome my 'ism and make this consistent, ;) > > > break; > > case Opt_filestreams: > > mp->m_flags |= XFS_MOUNT_FILESTREAMS; > > break; > > - case Opt_noquota: > > - mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT; > > - mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD; > > - mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE; > > - break; > > case Opt_quota: > > + if (!result.negated) { > > + mp->m_qflags |= (XFS_UQUOTA_ACCT | > > XFS_UQUOTA_ACTIVE | > > + XFS_UQUOTA_ENFD); > > + } else { > > + mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT; > > + mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD; > > + mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE; > > + } > > + break; > > case Opt_uquota: > > case Opt_usrquota: > > mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE | > ... > > @@ -367,10 +368,10 @@ 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 = &context; > > I don't really see the point for having a separate pointer variable > based on the code so far. Why not just do: > > struct xfs_fs_context ctx = {0,}; > > ... and pass by reference where necessary? Yep, it was a stupid mistake to begin with. I'm using only the context variable now. > > > > > /* > > * set up the mount name first so all the errors will refer to > > the > > @@ -406,17 +407,41 @@ xfs_parseargs( > > if (!options) > > goto done; > > > > + memset(&fc, 0, sizeof(fc)); > > + memset(&ctx, 0, sizeof(ctx)); > > + fc.fs_private = ctx; > > + fc.s_fs_info = mp; > > + > > while ((p = strsep(&options, ",")) != NULL) { > > - int token; > > - int ret; > > + struct fs_parameter param; > > + char *value; > > + int ret; > > > > 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.size = 0; > > + > > + value = strchr(p, '='); > > + if (value) { > > + if (value == p) > > + continue; > > What's the purpose of the above check? Why do we skip the param as > opposed to return an error or something? Well, that's a good question, there's a thought in the back of my mind that if the char isn't found the whole string is returned but that's not the way strchr(3) works so I was thinking of something different. Maybe I'm not the only one to think this way because it looks like the VFS code does the same thing (and that's where this came from altough I think it was slightly different when I did that). But I'm probably miss-reading the VFS code ... I'll need to look closer at it. > > > + *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; > > } > > > ... > > @@ -1914,6 +1939,10 @@ static const struct super_operations > > xfs_super_operations = { > > .free_cached_objects = xfs_fs_free_cached_objects, > > }; > > > > +static const struct fs_context_operations xfs_context_ops = { > > + .parse_param = xfs_parse_param, > > +}; > > + > > It's probably better to introduce this in the first patch where it's > used. Either or, I thought that building up the operations structure as we go gave insight into the context of where the change is headed. But if doing this irritates your sensibilities I can change it as long as no-one else has strong preferences against it. > > Brian > > > static struct file_system_type xfs_fs_type = { > > .owner = THIS_MODULE, > > .name = "xfs", > >
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 3ae29938dd89..754d2ccfd960 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -184,64 +184,62 @@ 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) + 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: case Opt_biosize: - 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: + if (result.negated) + mp->m_flags &= ~XFS_MOUNT_GRPID; + else + mp->m_flags |= XFS_MOUNT_GRPID; + break; case Opt_bsdgroups: mp->m_flags |= XFS_MOUNT_GRPID; break; - case Opt_nogrpid: case Opt_sysvgroups: mp->m_flags &= ~XFS_MOUNT_GRPID; break; @@ -258,12 +256,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; @@ -275,33 +271,38 @@ xfs_parse_param( mp->m_flags |= XFS_MOUNT_NOUUID; break; case Opt_ikeep: - mp->m_flags |= XFS_MOUNT_IKEEP; - break; - case Opt_noikeep: - mp->m_flags &= ~XFS_MOUNT_IKEEP; + if (result.negated) + mp->m_flags &= ~XFS_MOUNT_IKEEP; + else + mp->m_flags |= XFS_MOUNT_IKEEP; break; case Opt_largeio: - mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE; - break; - case Opt_nolargeio: - mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE; + if (result.negated) + mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE; + else + mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE; break; case Opt_attr2: - mp->m_flags |= XFS_MOUNT_ATTR2; - break; - case Opt_noattr2: - mp->m_flags &= ~XFS_MOUNT_ATTR2; - mp->m_flags |= XFS_MOUNT_NOATTR2; + if (!result.negated) { + mp->m_flags |= XFS_MOUNT_ATTR2; + } else { + mp->m_flags &= ~XFS_MOUNT_ATTR2; + mp->m_flags |= XFS_MOUNT_NOATTR2; + } break; case Opt_filestreams: mp->m_flags |= XFS_MOUNT_FILESTREAMS; break; - case Opt_noquota: - mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT; - mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD; - mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE; - break; case Opt_quota: + if (!result.negated) { + mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE | + XFS_UQUOTA_ENFD); + } else { + mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT; + mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD; + mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE; + } + break; case Opt_uquota: case Opt_usrquota: mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE | @@ -331,10 +332,10 @@ xfs_parse_param( mp->m_qflags &= ~XFS_GQUOTA_ENFD; break; case Opt_discard: - mp->m_flags |= XFS_MOUNT_DISCARD; - break; - case Opt_nodiscard: - mp->m_flags &= ~XFS_MOUNT_DISCARD; + if (result.negated) + mp->m_flags &= ~XFS_MOUNT_DISCARD; + else + mp->m_flags |= XFS_MOUNT_DISCARD; break; #ifdef CONFIG_FS_DAX case Opt_dax: @@ -342,7 +343,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; } @@ -367,10 +368,10 @@ 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 = &context; /* * set up the mount name first so all the errors will refer to the @@ -406,17 +407,41 @@ xfs_parseargs( if (!options) goto done; + memset(&fc, 0, sizeof(fc)); + memset(&ctx, 0, sizeof(ctx)); + fc.fs_private = ctx; + fc.s_fs_info = mp; + while ((p = strsep(&options, ",")) != NULL) { - int token; - int ret; + struct fs_parameter param; + char *value; + int ret; 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.size = 0; + + value = strchr(p, '='); + if (value) { + if (value == p) + continue; + *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; } @@ -429,7 +454,7 @@ 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; @@ -442,28 +467,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 && @@ -485,18 +510,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; @@ -1914,6 +1939,10 @@ static const struct super_operations xfs_super_operations = { .free_cached_objects = xfs_fs_free_cached_objects, }; +static const struct fs_context_operations xfs_context_ops = { + .parse_param = xfs_parse_param, +}; + static struct file_system_type xfs_fs_type = { .owner = THIS_MODULE, .name = "xfs",
Make xfs_parse_param() take arguments of the fs context operation .parse_param() in preperation 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 | 187 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 108 insertions(+), 79 deletions(-)