Sysctls for ARC min/max ignored at runtime #9489

loli10K · 2019-10-20T18:05:00Z

Motivation and Context

Description

This change leverage module_param_call() to run arc_tuning_update()
immediately after the ARC tunable has been updated as suggested in
cffa837 code review.

Maked WIP for now: new test case needs to be improved for low memory systems (namely my raspberry)

How Has This Been Tested?

Added test case to the ZFS Test Suite as suggested in #8463 code review.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.

sempervictus · 2019-10-21T01:48:55Z

Thank you, currently digging through this trying to figure out how backport it to stable - most of the diff does not apply :).

loli10K · 2019-10-21T05:28:53Z

All changes and fixes are now more difficult to backport, also git blame and bisecting have been more tedious unfortunately.

@sempervictus you should be able to apply this to zfs-0.8.2:

diff --git a/module/zfs/arc.c b/module/zfs/arc.c
index 53a44bdaf..3678903e7 100644
--- a/module/zfs/arc.c
+++ b/module/zfs/arc.c
@@ -296,6 +296,7 @@
 #include <sys/vmsystm.h>
 #include <sys/zpl.h>
 #include <linux/page_compat.h>
+#include <linux/mod_compat.h>
 #endif
 #include <sys/callb.h>
 #include <sys/kstat.h>
@@ -9369,6 +9370,26 @@ l2arc_stop(void)
 }
 
 #if defined(_KERNEL)
+
+static int
+param_set_arc_value(const char *buf, zfs_kernel_param_t *kp)
+{
+       uint64_t val;
+       int error;
+
+       error = kstrtoull(buf, 0, &val);
+       if (error)
+               return (SET_ERROR(error));
+
+       error = param_set_long(buf, kp);
+       if (error < 0)
+               return (SET_ERROR(error));
+
+       arc_tuning_update();
+
+       return (0);
+}
+
 EXPORT_SYMBOL(arc_buf_size);
 EXPORT_SYMBOL(arc_write);
 EXPORT_SYMBOL(arc_read);
@@ -9378,10 +9399,12 @@ EXPORT_SYMBOL(arc_add_prune_callback);
 EXPORT_SYMBOL(arc_remove_prune_callback);
 
 /* BEGIN CSTYLED */
-module_param(zfs_arc_min, ulong, 0644);
+module_param_call(zfs_arc_min, param_set_arc_value, param_get_long,
+       &zfs_arc_min, 0644);
 MODULE_PARM_DESC(zfs_arc_min, "Min arc size");
 
-module_param(zfs_arc_max, ulong, 0644);
+module_param_call(zfs_arc_max, param_set_arc_value, param_get_long,
+       &zfs_arc_max, 0644);
 MODULE_PARM_DESC(zfs_arc_max, "Max arc size");
 
 module_param(zfs_arc_meta_limit, ulong, 0644);

It would be great if you could test it and verify it fixes your usecase. Thanks!

sempervictus · 2019-10-21T17:06:23Z

Thank you, applied in-tree, building grsec & hardened versions presently, should have results this afternoon.

behlendorf

Thanks! Looks very reasonable, just a little bit of initial feedback.

module/zfs/arc.c

behlendorf · 2019-10-21T17:17:08Z

module/zfs/arc.c

+
+	return (0);
+}
+#endif


This may need to be moved to ./module/os/linux/zfs/arc_os.c unless we declare portable wrappers to do this.

I am still a bit lost due to all these changes to the codebase, i am doing this locally to see what happens.

Moved to ./module/os/linux/zfs/arc_os.c, everything seems to be broken now in ./module/zfs/arc.c:

CC [M] /tmp/zfs-build-buildbot-czCOc8ZC/BUILD/zfs-0.8.0/module/spl/../os/linux/spl/spl-zlib.o LD [M] /tmp/zfs-build-buildbot-czCOc8ZC/BUILD/zfs-0.8.0/module/spl/spl.o CC [M] /tmp/zfs-build-buildbot-czCOc8ZC/BUILD/zfs-0.8.0/module/zfs/aggsum.o CC [M] /tmp/zfs-build-buildbot-czCOc8ZC/BUILD/zfs-0.8.0/module/zcommon/zfs_prop.o CC [M] /tmp/zfs-build-buildbot-czCOc8ZC/BUILD/zfs-0.8.0/module/zfs/arc.o In file included from include/linux/module.h:17:0, from /tmp/zfs-build-buildbot-czCOc8ZC/BUILD/zfs-0.8.0/include/os/linux/spl/sys/atomic.h:28, from /tmp/zfs-build-buildbot-czCOc8ZC/BUILD/zfs-0.8.0/include/sys/zfs_context.h:35, from /tmp/zfs-build-buildbot-czCOc8ZC/BUILD/zfs-0.8.0/include/sys/spa.h:37, from /tmp/zfs-build-buildbot-czCOc8ZC/BUILD/zfs-0.8.0/module/zfs/arc.c:278: /tmp/zfs-build-buildbot-czCOc8ZC/BUILD/zfs-0.8.0/module/zfs/arc.c:8699:47: error: 'param_set_arc_long' undeclared here (not in a function) ZFS_MODULE_PARAM_CALL(zfs_arc, zfs_arc_, min, param_set_arc_long, ^ include/linux/moduleparam.h:190:14: note: in definition of macro 'module_param_call' { (void *)set, (void *)get }; \ ^ /tmp/zfs-build-buildbot-czCOc8ZC/BUILD/zfs-0.8.0/module/zfs/arc.c:8699:1: note: in expansion of macro 'ZFS_MODULE_PARAM_CALL' ZFS_MODULE_PARAM_CALL(zfs_arc, zfs_arc_, min, param_set_arc_long,

I tried moving more code to ./module/os/linux/zfs/arc_os.c but made things even worse: i never thought i would miss #ifdef <platform>.

@loli10K I sympathize! Refactoring the code to accommodate other platforms is going to take a little adjusting too. Plus we're still working on getting all the bits in place and documented, and a lot of the details are still being worked through. So things may change.

It looks to me like the only thing missing from this change is to add the common prototypes to the private arc_impl.h header. The arc_os.c sources contain platform specific versions of functions as needed, but when possible we want to share prototypes. @mattmacy would have the best idea what the param_set_arc_long and param_set_arc_int counterparts will need to look like for FreeBSD.

This change resolves the build issues for me. It:

Adds the needed forward references to the private arc_impl.h header, and

Removes the kstrtoull and kstrtoul calls from the helpers. We should be able to rely on the kernel helpers to perform the needed checks. Though we can add them back as long as we cast the type, if there's a case here which isn't covered.

diff --git a/include/sys/arc_impl.h b/include/sys/arc_impl.h index 28d430f..8eeee54 100644 --- a/include/sys/arc_impl.h +++ b/include/sys/arc_impl.h @@ -611,6 +611,10 @@ extern void arc_prune_async(int64_t); extern int arc_memory_throttle(spa_t *spa, uint64_t reserve, uint64_t txg); extern uint64_t arc_free_memory(void); extern int64_t arc_available_memory(void); +extern void arc_tuning_update(void); + +extern int param_set_arc_long(const char *buf, zfs_kernel_param_t *kp); +extern int param_set_arc_int(const char *buf, zfs_kernel_param_t *kp); #ifdef __cplusplus } diff --git a/include/sys/zfs_context.h b/include/sys/zfs_context.h index 9780726..4ea3050 100644 --- a/include/sys/zfs_context.h +++ b/include/sys/zfs_context.h @@ -204,6 +204,10 @@ extern int aok; /* * Tunables. */ +typedef struct zfs_kernel_param { + const char *name; /* unused stub */ +} zfs_kernel_param_t; + #define ZFS_MODULE_PARAM(scope_prefix, name_prefix, name, type, perm, desc) #define ZFS_MODULE_PARAM_CALL(scope_prefix, name_prefix, name, setfunc, \ getfunc, perm, desc) diff --git a/module/os/linux/zfs/arc_os.c b/module/os/linux/zfs/arc_os.c index 25ce9dc..eecc9fc 100644 --- a/module/os/linux/zfs/arc_os.c +++ b/module/os/linux/zfs/arc_os.c @@ -358,16 +358,11 @@ arc_lowmem_fini(void) spl_unregister_shrinker(&arc_shrinker); } -static int +int param_set_arc_long(const char *buf, zfs_kernel_param_t *kp) { - uint64_t val; int error; - error = kstrtoull(buf, 0, &val); - if (error) - return (SET_ERROR(error)); - error = param_set_long(buf, kp); if (error < 0) return (SET_ERROR(error)); @@ -377,16 +372,11 @@ param_set_arc_long(const char *buf, zfs_kernel_param_t *kp) return (0); } -static int +int param_set_arc_int(const char *buf, zfs_kernel_param_t *kp) { - int64_t val; int error; - error = kstrtoul(buf, 0, &val); - if (error) - return (SET_ERROR(error)); - error = param_set_int(buf, kp); if (error < 0) return (SET_ERROR(error)); diff --git a/module/zfs/arc.c b/module/zfs/arc.c index 9bf3f18..d4651d8 100644 --- a/module/zfs/arc.c +++ b/module/zfs/arc.c @@ -814,7 +814,6 @@ static void arc_hdr_alloc_abd(arc_buf_hdr_t *, boolean_t); static void arc_access(arc_buf_hdr_t *, kmutex_t *); static boolean_t arc_is_overflowing(void); static void arc_buf_watch(arc_buf_t *); -static void arc_tuning_update(void); static arc_buf_contents_t arc_buf_type(arc_buf_hdr_t *); static uint32_t arc_bufc_to_flags(arc_buf_contents_t); @@ -6878,7 +6877,7 @@ arc_state_multilist_index_func(multilist_t *ml, void *obj) * updated manually. Non-zero zfs_* values which differ from the currently set * values will be applied. */ -static void +void arc_tuning_update(void) { uint64_t allmem = arc_all_memory();

behlendorf · 2019-10-21T17:17:56Z

include/os/linux/kernel/linux/mod_compat.h

+ *   spa_slop_shift
+ */
+/* BEGIN CSTYLED */
+#define ZFS_MODULE_PARAM_CALL(scope_prefix, name_prefix, name, setfunc, getfunc, perm, desc) \


@mattmacy could you comment on how/if we'd want to wire this up on FreeBSD?

On FreeBSD it looks pretty different:

static int sysctl_vfs_zfs_max_auto_ashift(SYSCTL_HANDLER_ARGS) { uint64_t val; int err; val = zfs_max_auto_ashift; err = sysctl_handle_64(oidp, &val, 0, req); if (err != 0 || req->newptr == NULL) return (err); if (val > ASHIFT_MAX || val < zfs_min_auto_ashift) return (EINVAL); zfs_max_auto_ashift = val; return (0); } SYSCTL_PROC(_vfs_zfs, OID_AUTO, max_auto_ashift, CTLTYPE_U64 | CTLFLAG_MPSAFE | CTLFLAG_RW, 0, sizeof (uint64_t), sysctl_vfs_zfs_max_auto_ashift, "QU", "Max ashift used when optimising for logical -> physical sectors size on " "new top-level vdevs.");

I say go ahead and commit and I'll adapt it when I get the chance.

sempervictus · 2019-10-21T18:47:37Z

Confirmed working on 0.8.2 with 4.14.150 builtin:

# arcstat ; echo $((1024*1024*1024))> /sys/module/zfs/parameters/zfs_arc_max ; arcstat
    time  read  miss  miss%  dmis  dm%  pmis  pm%  mmis  mm%  arcsz     c  
14:44:13     0     0      0     0    0     0    0     0    0      0  1.9G  
    time  read  miss  miss%  dmis  dm%  pmis  pm%  mmis  mm%  arcsz     c  
14:44:48     0     0      0     0    0     0    0     0    0      0  1024M

codecov · 2019-10-25T06:03:34Z

Codecov Report

Merging #9489 into master will increase coverage by 0.01%.
The diff coverage is 41.66%.

@@            Coverage Diff             @@
##           master    #9489      +/-   ##
==========================================
+ Coverage   79.09%    79.1%   +0.01%     
==========================================
  Files         416      416              
  Lines      123640   123652      +12     
==========================================
+ Hits        97796    97819      +23     
+ Misses      25844    25833      -11

Flag	Coverage Δ
#kernel	`79.74% <41.66%> (+0.02%)`	⬆️
#user	`66.97% <ø> (+0.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdf5593...b3f0a23. Read the comment docs.

behlendorf · 2019-10-25T19:43:17Z

@mattmacy would you mind reviewing this to ensure it's reasonable from a FreeBSD perspective. We can always tweak it after the fact, but it would be nice to catch anything major now.

This change leverage module_param_call() to run arc_tuning_update() immediately after the ARC tunable has been updated as suggested in cffa837 code review. A simple test case is added to the ZFS Test Suite to prevent future regressions in functionality. Signed-off-by: loli10K <[email protected]>

loli10K · 2019-10-26T12:50:03Z

Rebased on master and added missing copyright statements.

This change leverage module_param_call() to run arc_tuning_update() immediately after the ARC tunable has been updated as suggested in cffa837 code review. A simple test case is added to the ZFS Test Suite to prevent future regressions in functionality. This is a backport of openzfs#9489 provided from: openzfs#9776 (comment) Signed-off-by: loli10K <[email protected]>

This change leverage module_param_call() to run arc_tuning_update() immediately after the ARC tunable has been updated as suggested in cffa837 code review. A simple test case is added to the ZFS Test Suite to prevent future regressions in functionality. This is a backport of #9489 provided from: #9776 (comment) Signed-off-by: loli10K <[email protected]>

loli10K added the Status: Work in Progress Not yet ready for general review label Oct 20, 2019

behlendorf reviewed Oct 21, 2019

View reviewed changes

loli10K force-pushed the issue-9487 branch 2 times, most recently from 3da4cfb to a5e13da Compare October 24, 2019 20:55

loli10K added Status: Code Review Needed Ready for review and testing and removed Status: Work in Progress Not yet ready for general review labels Oct 24, 2019

loli10K force-pushed the issue-9487 branch from a5e13da to ec30952 Compare October 25, 2019 05:41

behlendorf approved these changes Oct 25, 2019

View reviewed changes

behlendorf requested a review from a user October 25, 2019 19:42

loli10K force-pushed the issue-9487 branch from ec30952 to b3f0a23 Compare October 26, 2019 12:48

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 26, 2019

behlendorf merged commit e357046 into openzfs:master Oct 26, 2019

loli10K mentioned this pull request Dec 28, 2019

zfs-0.8.3 patchset #9776

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sysctls for ARC min/max ignored at runtime #9489

Sysctls for ARC min/max ignored at runtime #9489

loli10K commented Oct 20, 2019 •

edited

Loading

sempervictus commented Oct 21, 2019

loli10K commented Oct 21, 2019 •

edited

Loading

sempervictus commented Oct 21, 2019

behlendorf left a comment

behlendorf Oct 21, 2019

loli10K Oct 22, 2019

loli10K Oct 23, 2019

behlendorf Oct 23, 2019

loli10K Oct 24, 2019

behlendorf Oct 21, 2019

mattmacy Oct 25, 2019

sempervictus commented Oct 21, 2019

codecov bot commented Oct 25, 2019 •

edited

Loading

behlendorf commented Oct 25, 2019

loli10K commented Oct 26, 2019

Sysctls for ARC min/max ignored at runtime #9489

Sysctls for ARC min/max ignored at runtime #9489

Conversation

loli10K commented Oct 20, 2019 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

sempervictus commented Oct 21, 2019

loli10K commented Oct 21, 2019 • edited Loading

sempervictus commented Oct 21, 2019

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf Oct 21, 2019

Choose a reason for hiding this comment

loli10K Oct 22, 2019

Choose a reason for hiding this comment

loli10K Oct 23, 2019

Choose a reason for hiding this comment

behlendorf Oct 23, 2019

Choose a reason for hiding this comment

loli10K Oct 24, 2019

Choose a reason for hiding this comment

behlendorf Oct 21, 2019

Choose a reason for hiding this comment

mattmacy Oct 25, 2019

Choose a reason for hiding this comment

sempervictus commented Oct 21, 2019

codecov bot commented Oct 25, 2019 • edited Loading

Codecov Report

behlendorf commented Oct 25, 2019

loli10K commented Oct 26, 2019

loli10K commented Oct 20, 2019 •

edited

Loading

loli10K commented Oct 21, 2019 •

edited

Loading

codecov bot commented Oct 25, 2019 •

edited

Loading