* Re: [vfs] 8bb3c61baf: vm-scalability.median -23.7% regression [not found] <20190903084122.GH15734@shao2-debian> @ 2019-09-08 21:46 ` Al Viro 2019-09-08 23:47 ` Al Viro 0 siblings, 1 reply; 6+ messages in thread From: Al Viro @ 2019-09-08 21:46 UTC (permalink / raw) To: kernel test robot; +Cc: David Howells, Hugh Dickins, LKML, linux-fsdevel, lkp On Tue, Sep 03, 2019 at 04:41:22PM +0800, kernel test robot wrote: > Greeting, > > FYI, we noticed a -23.7% regression of vm-scalability.median due to commit: > > > commit: 8bb3c61bafa8c1cd222ada602bb94ff23119e738 ("vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API") > https://kernel.googlesource.com/pub/scm/linux/kernel/git/viro/vfs.git work.mount > > in testcase: vm-scalability > on test machine: 88 threads Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz with 128G memory > with following parameters: > > runtime: 300s > size: 16G > test: shm-pread-rand > cpufreq_governor: performance > ucode: 0xb000036 That thing loses size=... option. Both size= and nr_blocks= affect the same thing (->max_blocks), but the parser keeps track of the options it has seen and applying the parsed data to superblock checks only whether nr_blocks= had been there. IOW, size= gets parsed, but the result goes nowhere. I'm not sure whether it's better to fix the patch up or redo it from scratch - it needs to be carved up anyway and it's highly non-transparent, so I'm probably going to replace the damn thing entirely with something that would be easier to follow. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [vfs] 8bb3c61baf: vm-scalability.median -23.7% regression 2019-09-08 21:46 ` [vfs] 8bb3c61baf: vm-scalability.median -23.7% regression Al Viro @ 2019-09-08 23:47 ` Al Viro 2019-09-09 3:10 ` Hugh Dickins 0 siblings, 1 reply; 6+ messages in thread From: Al Viro @ 2019-09-08 23:47 UTC (permalink / raw) To: kernel test robot; +Cc: David Howells, Hugh Dickins, LKML, linux-fsdevel, lkp On Sun, Sep 08, 2019 at 10:46:01PM +0100, Al Viro wrote: > On Tue, Sep 03, 2019 at 04:41:22PM +0800, kernel test robot wrote: > > Greeting, > > > > FYI, we noticed a -23.7% regression of vm-scalability.median due to commit: > > > > > > commit: 8bb3c61bafa8c1cd222ada602bb94ff23119e738 ("vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API") > > https://kernel.googlesource.com/pub/scm/linux/kernel/git/viro/vfs.git work.mount > > > > in testcase: vm-scalability > > on test machine: 88 threads Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz with 128G memory > > with following parameters: > > > > runtime: 300s > > size: 16G > > test: shm-pread-rand > > cpufreq_governor: performance > > ucode: 0xb000036 > > That thing loses size=... option. Both size= and nr_blocks= affect the > same thing (->max_blocks), but the parser keeps track of the options > it has seen and applying the parsed data to superblock checks only > whether nr_blocks= had been there. IOW, size= gets parsed, but the > result goes nowhere. > > I'm not sure whether it's better to fix the patch up or redo it from > scratch - it needs to be carved up anyway and it's highly non-transparent, > so I'm probably going to replace the damn thing entirely with something > that would be easier to follow. ... and this + { Opt_huge, "deny", SHMEM_HUGE_DENY }, + { Opt_huge, "force", SHMEM_HUGE_FORCE }, had been wrong - huge=deny and huge=force should not be accepted _and_ fs_parameter_enum is not suitable for negative constants right now anyway. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [vfs] 8bb3c61baf: vm-scalability.median -23.7% regression 2019-09-08 23:47 ` Al Viro @ 2019-09-09 3:10 ` Hugh Dickins 2019-09-09 3:56 ` Al Viro 0 siblings, 1 reply; 6+ messages in thread From: Hugh Dickins @ 2019-09-09 3:10 UTC (permalink / raw) To: Al Viro Cc: kernel test robot, David Howells, Hugh Dickins, Andrew Morton, LKML, linux-fsdevel, lkp On Mon, 9 Sep 2019, Al Viro wrote: > On Sun, Sep 08, 2019 at 10:46:01PM +0100, Al Viro wrote: > > On Tue, Sep 03, 2019 at 04:41:22PM +0800, kernel test robot wrote: > > > Greeting, > > > > > > FYI, we noticed a -23.7% regression of vm-scalability.median due to commit: > > > > > > > > > commit: 8bb3c61bafa8c1cd222ada602bb94ff23119e738 ("vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API") > > > https://kernel.googlesource.com/pub/scm/linux/kernel/git/viro/vfs.git work.mount > > > > > > in testcase: vm-scalability > > > on test machine: 88 threads Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz with 128G memory > > > with following parameters: > > > > > > runtime: 300s > > > size: 16G > > > test: shm-pread-rand > > > cpufreq_governor: performance > > > ucode: 0xb000036 > > > > That thing loses size=... option. Both size= and nr_blocks= affect the > > same thing (->max_blocks), but the parser keeps track of the options > > it has seen and applying the parsed data to superblock checks only > > whether nr_blocks= had been there. IOW, size= gets parsed, but the > > result goes nowhere. > > > > I'm not sure whether it's better to fix the patch up or redo it from > > scratch - it needs to be carved up anyway and it's highly non-transparent, > > so I'm probably going to replace the damn thing entirely with something > > that would be easier to follow. > > ... and this > + { Opt_huge, "deny", SHMEM_HUGE_DENY }, > + { Opt_huge, "force", SHMEM_HUGE_FORCE }, > had been wrong - huge=deny and huge=force should not be accepted _and_ > fs_parameter_enum is not suitable for negative constants right now > anyway. Sorry you've been spending time redisovering these, Al: I sent David the tmpfs fixes (Cc'ing you and Andrew and lists) a couple of weeks ago - but had no idea until your mail that the "loss of size" was behind this vm-scalability regression report. Ah, not for the first time, I missed saying "[PATCH]" in the subject: sorry, that may have rendered it invisible to many eyes. Here's what Andrew has been carrying in the mmotm tree since I sent it: I'm sure we'd both be happy for you to take it into your tree. I had expected it to percolate through from mmotm to linux-next by now, but apparently not. From: Hugh Dickins <hughd@google.com> Subject: tmpfs: fixups to use of the new mount API Several fixups to shmem_parse_param() and tmpfs use of new mount API: mm/shmem.c manages filesystem named "tmpfs": revert "shmem" to "tmpfs" in its mount error messages. /sys/kernel/mm/transparent_hugepage/shmem_enabled has valid options "deny" and "force", but they are not valid as tmpfs "huge" options. The "size" param is an alternative to "nr_blocks", and needs to be recognized as changing max_blocks. And where there's ambiguity, it's better to mention "size" than "nr_blocks" in messages, since "size" is the variant shown in /proc/mounts. shmem_apply_options() left ctx->mpol as the new mpol, so then it was freed in shmem_free_fc(), and the filesystem went on to use-after-free. shmem_parse_param() issue "tmpfs: Bad value for '%s'" messages just like fs_parse() would, instead of a different wording. Where config disables "mpol" or "huge", say "tmpfs: Unsupported parameter '%s'". Link: http://lkml.kernel.org/r/alpine.LSU.2.11.1908191503290.1253@eggly.anvils Fixes: 144df3b288c41 ("vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API") Signed-off-by: Hugh Dickins <hughd@google.com> Cc: David Howells <dhowells@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- mm/shmem.c | 80 ++++++++++++++++++++++++++------------------------- 1 file changed, 42 insertions(+), 38 deletions(-) --- a/mm/shmem.c~tmpfs-fixups-to-use-of-the-new-mount-api +++ a/mm/shmem.c @@ -3432,13 +3432,11 @@ static const struct fs_parameter_enum sh { Opt_huge, "always", SHMEM_HUGE_ALWAYS }, { Opt_huge, "within_size", SHMEM_HUGE_WITHIN_SIZE }, { Opt_huge, "advise", SHMEM_HUGE_ADVISE }, - { Opt_huge, "deny", SHMEM_HUGE_DENY }, - { Opt_huge, "force", SHMEM_HUGE_FORCE }, {} }; const struct fs_parameter_description shmem_fs_parameters = { - .name = "shmem", + .name = "tmpfs", .specs = shmem_param_specs, .enums = shmem_param_enums, }; @@ -3448,9 +3446,9 @@ static void shmem_apply_options(struct s unsigned long inodes_in_use) { struct shmem_fs_context *ctx = fc->fs_private; - struct mempolicy *old = NULL; - if (test_bit(Opt_nr_blocks, &ctx->changes)) + if (test_bit(Opt_nr_blocks, &ctx->changes) || + test_bit(Opt_size, &ctx->changes)) sbinfo->max_blocks = ctx->max_blocks; if (test_bit(Opt_nr_inodes, &ctx->changes)) { sbinfo->max_inodes = ctx->max_inodes; @@ -3459,8 +3457,11 @@ static void shmem_apply_options(struct s if (test_bit(Opt_huge, &ctx->changes)) sbinfo->huge = ctx->huge; if (test_bit(Opt_mpol, &ctx->changes)) { - old = sbinfo->mpol; - sbinfo->mpol = ctx->mpol; + /* + * Update sbinfo->mpol now while stat_lock is held. + * Leave shmem_free_fc() to free the old mpol if any. + */ + swap(sbinfo->mpol, ctx->mpol); } if (fc->purpose != FS_CONTEXT_FOR_RECONFIGURE) { @@ -3471,8 +3472,6 @@ static void shmem_apply_options(struct s if (test_bit(Opt_mode, &ctx->changes)) sbinfo->mode = ctx->mode; } - - mpol_put(old); } static int shmem_parse_param(struct fs_context *fc, struct fs_parameter *param) @@ -3498,7 +3497,7 @@ static int shmem_parse_param(struct fs_c rest++; } if (*rest) - return invalf(fc, "shmem: Invalid size"); + goto bad_value; ctx->max_blocks = DIV_ROUND_UP(size, PAGE_SIZE); break; @@ -3506,55 +3505,59 @@ static int shmem_parse_param(struct fs_c rest = param->string; ctx->max_blocks = memparse(param->string, &rest); if (*rest) - return invalf(fc, "shmem: Invalid nr_blocks"); + goto bad_value; break; + case Opt_nr_inodes: rest = param->string; ctx->max_inodes = memparse(param->string, &rest); if (*rest) - return invalf(fc, "shmem: Invalid nr_inodes"); + goto bad_value; break; + case Opt_mode: ctx->mode = result.uint_32 & 07777; break; + case Opt_uid: ctx->uid = make_kuid(current_user_ns(), result.uint_32); if (!uid_valid(ctx->uid)) - return invalf(fc, "shmem: Invalid uid"); + goto bad_value; break; case Opt_gid: ctx->gid = make_kgid(current_user_ns(), result.uint_32); if (!gid_valid(ctx->gid)) - return invalf(fc, "shmem: Invalid gid"); + goto bad_value; break; case Opt_huge: -#ifdef CONFIG_TRANSPARENT_HUGE_PAGECACHE - if (!has_transparent_hugepage() && - result.uint_32 != SHMEM_HUGE_NEVER) - return invalf(fc, "shmem: Huge pages disabled"); - ctx->huge = result.uint_32; + if (ctx->huge != SHMEM_HUGE_NEVER && + !(IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE) && + has_transparent_hugepage())) + goto unsupported_parameter; break; -#else - return invalf(fc, "shmem: huge= option disabled"); -#endif - - case Opt_mpol: { -#ifdef CONFIG_NUMA - struct mempolicy *mpol; - if (mpol_parse_str(param->string, &mpol)) - return invalf(fc, "shmem: Invalid mpol="); - mpol_put(ctx->mpol); - ctx->mpol = mpol; -#endif - break; - } + + case Opt_mpol: + if (IS_ENABLED(CONFIG_NUMA)) { + struct mempolicy *mpol; + if (mpol_parse_str(param->string, &mpol)) + goto bad_value; + mpol_put(ctx->mpol); + ctx->mpol = mpol; + break; + } + goto unsupported_parameter; } __set_bit(opt, &ctx->changes); return 0; + +unsupported_parameter: + return invalf(fc, "tmpfs: Unsupported parameter '%s'", param->key); +bad_value: + return invalf(fc, "tmpfs: Bad value for '%s'", param->key); } /* @@ -3572,14 +3575,15 @@ static int shmem_reconfigure(struct fs_c unsigned long inodes_in_use; spin_lock(&sbinfo->stat_lock); - if (test_bit(Opt_nr_blocks, &ctx->changes)) { + if (test_bit(Opt_nr_blocks, &ctx->changes) || + test_bit(Opt_size, &ctx->changes)) { if (ctx->max_blocks && !sbinfo->max_blocks) { spin_unlock(&sbinfo->stat_lock); - return invalf(fc, "shmem: Can't retroactively limit nr_blocks"); + return invalf(fc, "tmpfs: Cannot retroactively limit size"); } if (percpu_counter_compare(&sbinfo->used_blocks, ctx->max_blocks) > 0) { spin_unlock(&sbinfo->stat_lock); - return invalf(fc, "shmem: Too few blocks for current use"); + return invalf(fc, "tmpfs: Too small a size for current use"); } } @@ -3587,11 +3591,11 @@ static int shmem_reconfigure(struct fs_c if (test_bit(Opt_nr_inodes, &ctx->changes)) { if (ctx->max_inodes && !sbinfo->max_inodes) { spin_unlock(&sbinfo->stat_lock); - return invalf(fc, "shmem: Can't retroactively limit nr_inodes"); + return invalf(fc, "tmpfs: Cannot retroactively limit inodes"); } if (ctx->max_inodes < inodes_in_use) { spin_unlock(&sbinfo->stat_lock); - return invalf(fc, "shmem: Too few inodes for current use"); + return invalf(fc, "tmpfs: Too few inodes for current use"); } } _ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [vfs] 8bb3c61baf: vm-scalability.median -23.7% regression 2019-09-09 3:10 ` Hugh Dickins @ 2019-09-09 3:56 ` Al Viro 2019-09-10 6:27 ` Hugh Dickins 0 siblings, 1 reply; 6+ messages in thread From: Al Viro @ 2019-09-09 3:56 UTC (permalink / raw) To: Hugh Dickins Cc: kernel test robot, David Howells, Andrew Morton, LKML, linux-fsdevel, lkp On Sun, Sep 08, 2019 at 08:10:17PM -0700, Hugh Dickins wrote: Hmm... FWIW, I'd ended up redoing most of the thing, with hopefully sane carve-up. Differences: * we really care only about three things having been set - ->max_blocks, ->max_inodes and ->huge. This __set_bit() hack is cute, but asking for trouble (and getting it). Explicit ctx->seen & SHMEM_SEEN_BLOCKS, etc. is cleaner. * > const struct fs_parameter_description shmem_fs_parameters = { > - .name = "shmem", > + .name = "tmpfs", > .specs = shmem_param_specs, > .enums = shmem_param_enums, > }; Missed that one, will fold. * > @@ -3448,9 +3446,9 @@ static void shmem_apply_options(struct s The whole "apply" thing is useless - in remount we need to copy max_inode/max_blocks/huge/mpol under the lock after checks, and we can do that manually just fine. Other options (uid/gid/mode) get ignored. There very little overlap with fill_super case, really. > - old = sbinfo->mpol; > - sbinfo->mpol = ctx->mpol; > + /* > + * Update sbinfo->mpol now while stat_lock is held. > + * Leave shmem_free_fc() to free the old mpol if any. > + */ > + swap(sbinfo->mpol, ctx->mpol); Umm... Missed that use-after-free due to destructor, TBH (in remount, that is). Fixed (in a slightly different way). > } > if (*rest) > - return invalf(fc, "shmem: Invalid size"); > + goto bad_value; > ctx->max_blocks = DIV_ROUND_UP(size, PAGE_SIZE); > break; FWIW, I had those with s/shmem/tmpfs/, no problem with merging like that. Will fold. [snip] > case Opt_huge: > -#ifdef CONFIG_TRANSPARENT_HUGE_PAGECACHE > - if (!has_transparent_hugepage() && > - result.uint_32 != SHMEM_HUGE_NEVER) > - return invalf(fc, "shmem: Huge pages disabled"); > - > ctx->huge = result.uint_32; > + if (ctx->huge != SHMEM_HUGE_NEVER && > + !(IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE) && > + has_transparent_hugepage())) > + goto unsupported_parameter; > break; > -#else > - return invalf(fc, "shmem: huge= option disabled"); > -#endif > - > - case Opt_mpol: { > -#ifdef CONFIG_NUMA > - struct mempolicy *mpol; > - if (mpol_parse_str(param->string, &mpol)) > - return invalf(fc, "shmem: Invalid mpol="); > - mpol_put(ctx->mpol); > - ctx->mpol = mpol; > -#endif > - break; > - } OK... > + case Opt_mpol: > + if (IS_ENABLED(CONFIG_NUMA)) { > + struct mempolicy *mpol; > + if (mpol_parse_str(param->string, &mpol)) > + goto bad_value; > + mpol_put(ctx->mpol); > + ctx->mpol = mpol; > + break; > + } > + goto unsupported_parameter; Slightly different here - I'd done that bit as mpol_put(ctx->mpol); ctx->mpol = NULL; if (mpol_parse_str(param->string, &ctx->mpol)) return invalf (goto bad_value now) > +unsupported_parameter: > + return invalf(fc, "tmpfs: Unsupported parameter '%s'", param->key); > +bad_value: > + return invalf(fc, "tmpfs: Bad value for '%s'", param->key); > - return invalf(fc, "shmem: Can't retroactively limit nr_blocks"); > + return invalf(fc, "tmpfs: Cannot retroactively limit size"); > } > if (percpu_counter_compare(&sbinfo->used_blocks, ctx->max_blocks) > 0) { > spin_unlock(&sbinfo->stat_lock); > - return invalf(fc, "shmem: Too few blocks for current use"); > + return invalf(fc, "tmpfs: Too small a size for current use"); > } > } > > @@ -3587,11 +3591,11 @@ static int shmem_reconfigure(struct fs_c > if (test_bit(Opt_nr_inodes, &ctx->changes)) { > if (ctx->max_inodes && !sbinfo->max_inodes) { > spin_unlock(&sbinfo->stat_lock); > - return invalf(fc, "shmem: Can't retroactively limit nr_inodes"); > + return invalf(fc, "tmpfs: Cannot retroactively limit inodes"); > } > if (ctx->max_inodes < inodes_in_use) { > spin_unlock(&sbinfo->stat_lock); > - return invalf(fc, "shmem: Too few inodes for current use"); > + return invalf(fc, "tmpfs: Too few inodes for current use"); > } > } s/Can't/Cannot/ and s/few blocks/small a size/? No problem, except that I'd done err = "Too few inodes for current use"; goto out; ... out: return invalf(fc, "tmpfs: %s", err); Anyway, see vfs.git#uncertain.shmem for what I've got with those folded in. Do you see any problems with that one? That's the last 5 commits in there... ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [vfs] 8bb3c61baf: vm-scalability.median -23.7% regression 2019-09-09 3:56 ` Al Viro @ 2019-09-10 6:27 ` Hugh Dickins 2019-09-11 3:05 ` Hugh Dickins 0 siblings, 1 reply; 6+ messages in thread From: Hugh Dickins @ 2019-09-10 6:27 UTC (permalink / raw) To: Al Viro Cc: Hugh Dickins, kernel test robot, David Howells, Andrew Morton, LKML, linux-fsdevel, lkp On Mon, 9 Sep 2019, Al Viro wrote: > > Anyway, see vfs.git#uncertain.shmem for what I've got with those folded in. > Do you see any problems with that one? That's the last 5 commits in there... It's mostly fine, I've no problem with going your way instead of what we had in mmotm; but I have seen some problems with it, and had been intending to send you a fixup patch tonight (shmem_reconfigure() missing unlock on error is the main problem, but there are other fixes needed). But I'm growing tired. I've a feeling my "swap" of the mpols, instead of immediate mpol_put(), was necessary to protect against a race with shmem_get_sbmpol(), but I'm not clear-headed enough to trust myself on that now. And I've a mystery to solve, that shmem_reconfigure() gets stuck into showing the wrong error message. Tomorrow.... Oh, and my first attempt to build and boot that series over 5.3-rc5 wouldn't boot. Luckily there was a tell-tale "i915" in the stacktrace, which reminded me of the drivers/gpu/drm/i915/gem/i915_gemfs.c fix we discussed earlier in the cycle. That is of course in linux-next by now, but I wonder if your branch ought to contain a duplicate of that fix, so that people with i915 doing bisections on 5.4-rc do not fall into an unbootable hole between vfs and gpu merges. Hugh ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [vfs] 8bb3c61baf: vm-scalability.median -23.7% regression 2019-09-10 6:27 ` Hugh Dickins @ 2019-09-11 3:05 ` Hugh Dickins 0 siblings, 0 replies; 6+ messages in thread From: Hugh Dickins @ 2019-09-11 3:05 UTC (permalink / raw) To: Al Viro Cc: Hugh Dickins, kernel test robot, David Howells, Andrew Morton, LKML, linux-fsdevel, lkp On Mon, 9 Sep 2019, Hugh Dickins wrote: > On Mon, 9 Sep 2019, Al Viro wrote: > > > > Anyway, see vfs.git#uncertain.shmem for what I've got with those folded in. > > Do you see any problems with that one? That's the last 5 commits in there... > > It's mostly fine, I've no problem with going your way instead of what > we had in mmotm; but I have seen some problems with it, and had been > intending to send you a fixup patch tonight (shmem_reconfigure() missing > unlock on error is the main problem, but there are other fixes needed). > > But I'm growing tired. I've a feeling my "swap" of the mpols, instead > of immediate mpol_put(), was necessary to protect against a race with > shmem_get_sbmpol(), but I'm not clear-headed enough to trust myself on > that now. And I've a mystery to solve, that shmem_reconfigure() gets > stuck into showing the wrong error message. On my "swap" for the mpol_put(): no, the race against shmem_get_sbmpol() is safe enough without that, and what you have matches what was always done before. I rather like my "swap", which the previous double-free had led me to, but it's fine if you prefer the ordinary way. I was probably coming down from some over-exposure to iput() under spinlock, but there's no such complications here. > > Tomorrow.... > > Oh, and my first attempt to build and boot that series over 5.3-rc5 > wouldn't boot. Luckily there was a tell-tale "i915" in the stacktrace, > which reminded me of the drivers/gpu/drm/i915/gem/i915_gemfs.c fix > we discussed earlier in the cycle. That is of course in linux-next > by now, but I wonder if your branch ought to contain a duplicate of > that fix, so that people with i915 doing bisections on 5.4-rc do not > fall into an unbootable hole between vfs and gpu merges. Below are the fixups I arrived at last night (I've not rechecked your tree today, to see if you made any changes since). But they're not enough: I now understand why shmem_reconfigure() got stuck showing the wrong error message, but I'll have to leave it to you to decide what to do about it, because I don't know whether it's just a mistake, or different filesystem types have different needs there. My /etc/fstab has a line in for one of my test mounts: tmpfs /tlo tmpfs size=4G 0 0 and that "size=4G" is what causes the problem: because each time shmem_parse_options(fc, data) is called for a remount, data (that is, options) points to a string starting with "size=4G,", followed by what's actually been asked for in the remount options. So if I try mount -o remount,size=0 /tlo that succeeds, setting the filesystem size to 0 meaning unlimited. So if then as a test I try mount -o remount,size=1M /tlo that correctly fails with "Cannot retroactively limit size". But then when I try mount -o remount,nr_inodes=0 /tlo I again get "Cannot retroactively limit size", when it should have succeeded (again, 0 here meaning unlimited). That's because the options in shmem_parse_options() are "size=4G,nr_inodes=0", which indeed looks like an attempt to retroactively limit size; but the user never asked "size=4G" there. I think this problem, and some of what's fixed below, predate your rework, and would equally affect the version in mmotm: I just didn't discover these issues when I was testing that before. Hugh --- aviro/mm/shmem.c 2019-09-09 14:10:34.379832855 -0700 +++ hughd/mm/shmem.c 2019-09-09 23:29:28.467037895 -0700 @@ -3456,7 +3456,7 @@ static int shmem_parse_one(struct fs_con ctx->huge = result.uint_32; if (ctx->huge != SHMEM_HUGE_NEVER && !(IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE) && - has_transparent_hugepage())) + has_transparent_hugepage())) goto unsupported_parameter; ctx->seen |= SHMEM_SEEN_HUGE; break; @@ -3532,26 +3532,26 @@ static int shmem_reconfigure(struct fs_c spin_lock(&sbinfo->stat_lock); inodes = sbinfo->max_inodes - sbinfo->free_inodes; - if (ctx->seen & SHMEM_SEEN_BLOCKS) { + if ((ctx->seen & SHMEM_SEEN_BLOCKS) && ctx->blocks) { + if (!sbinfo->max_blocks) { + err = "Cannot retroactively limit size"; + goto out; + } if (percpu_counter_compare(&sbinfo->used_blocks, ctx->blocks) > 0) { err = "Too small a size for current use"; goto out; } - if (ctx->blocks && !sbinfo->max_blocks) { - err = "Cannot retroactively limit nr_blocks"; + } + if ((ctx->seen & SHMEM_SEEN_INODES) && ctx->inodes) { + if (!sbinfo->max_inodes) { + err = "Cannot retroactively limit inodes"; goto out; } - } - if (ctx->seen & SHMEM_SEEN_INODES) { if (ctx->inodes < inodes) { err = "Too few inodes for current use"; goto out; } - if (ctx->inodes && !sbinfo->max_inodes) { - err = "Cannot retroactively limit nr_inodes"; - goto out; - } } if (ctx->seen & SHMEM_SEEN_HUGE) @@ -3574,6 +3574,7 @@ static int shmem_reconfigure(struct fs_c spin_unlock(&sbinfo->stat_lock); return 0; out: + spin_unlock(&sbinfo->stat_lock); return invalf(fc, "tmpfs: %s", err); } ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-11 3:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20190903084122.GH15734@shao2-debian> 2019-09-08 21:46 ` [vfs] 8bb3c61baf: vm-scalability.median -23.7% regression Al Viro 2019-09-08 23:47 ` Al Viro 2019-09-09 3:10 ` Hugh Dickins 2019-09-09 3:56 ` Al Viro 2019-09-10 6:27 ` Hugh Dickins 2019-09-11 3:05 ` Hugh Dickins
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).