Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3.12 compat, 4.0 compat for super_operations shrinker callbacks #3308

Conversation

dweeezil
Copy link
Contributor

3.12 API change - A node ID argument was added to the nr_cached_objects
and free_cached_objects callbacks in struct super_operations.

4.0 API change - The node ID and nr_to_scan arguments to the
nr_cached_objects and free_cached_objects callbacks in struct
super_operations were replaced with a single struct shrink_control
argument.

Also, the return value from each callback should be long.

I've not tested this yet, however, it means we've not had the nr_cached_objects callback enabled for quite some time, if ever. I found this while looking into #3303.

@dweeezil dweeezil force-pushed the super-operations-shrink-callback-args branch 5 times, most recently from fe3de22 to 445ba57 Compare April 19, 2015 15:11
@dweeezil
Copy link
Contributor Author

I'm going to leave this as-is until it gets through the buildbots.

Although it does "fix" an error, it may actually make matters worse because the value returned by the nr_cached_objects callback is a lie and will cause the kernel's prune_super() to put less pressure on the the dentry and inode caches rather than more.

Unless we can do something useful in the free_cached_objects callback, I'd suggest we remove both of them from ZoL. These callbacks appear to have been added mainly to support xfs and the manner in which it stores per-allocation group data.

@dweeezil
Copy link
Contributor Author

Following on to @chrisrd's comment regarding my note above, #3350, which is running a kernel on which the autoconf test will actually work, provides a bit more evidence we should either get rid of the callback or simply have it return zero for the time being. I'd suggest having it return zero to leave the callbacks in place mainly for documentation. In fact, I'll update the pull request accordingly.

@behlendorf
Copy link
Contributor

@dweeezil I agree. We should update these callbacks so that they simply return zero but leave them in place. I strongly suspect we're going to have a use for them once the ARC is integrated with the page cache. At that point we should be able to abandon out existing overly broad shrinker hook in favor of something per-filesystem like this.

@dweeezil dweeezil force-pushed the super-operations-shrink-callback-args branch from 445ba57 to 90463f4 Compare April 27, 2015 21:13
@dweeezil
Copy link
Contributor Author

Updated pull request with commentary as to potential future use.

@behlendorf behlendorf added this to the 0.6.5 milestone Apr 27, 2015
@behlendorf
Copy link
Contributor

@dweeezil it looks like this is going to need a second look to resolve the build failures.

@dweeezil
Copy link
Contributor Author

I'll look into the failures. I clearly need to try it with a handful of intermediate kernels.

3.12 API change - A node ID argument was added to the nr_cached_objects
and free_cached_objects callbacks in struct super_operations.

4.0 API change - The node ID and nr_to_scan arguments to the
nr_cached_objects and free_cached_objects callbacks in struct
super_operations were replaced with a single struct shrink_control
argument.

Also, the return value from each callback should be long.

Since this will effectively enable the callbacks on post-3.12 kernels and
there is currently no free_cached_objects callback, the nr_cached_objects
has been disabled until additional page cache integration is done at
which time it's likely to be necessary to have a proper implementation
of both callbacks.
@behlendorf
Copy link
Contributor

@dweeezil since #3495 takes care of the 3.12 compat issues (and will hopefully get merged tomorrow) we'll want to refresh this patch to just cover the minor 4.0 scan control issue.

@behlendorf
Copy link
Contributor

Closing as stale. #3495 addressed the Linux 3.12 compatibility issue and we can open a new pull request for the remaining less critical 4.0 issue.

@behlendorf behlendorf closed this Jun 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants