-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Linux: s_op: use .free_inode #16788
base: master
Are you sure you want to change the base?
Linux: s_op: use .free_inode #16788
Conversation
as per Documentation/filesystems/porting.rst: quote: **strongly recommended** take the RCU-delayed parts of ->destroy_inode() into a new method - ->free_inode(). If ->destroy_inode() becomes empty - all the better, just get rid of it. endquote. Signed-off-by: Pavel Snajdr <[email protected]>
f1dfc9d
to
1c6da1a
Compare
.destroy_inode = zpl_inode_destroy, | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we still want to register .destroy_inode
even when .free_inode
is available. It's just that the RCU-delayed parts have been moved in to .free_inode
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I tried to adhere to the note in porting.rst
to the fullest extent possible, as it doesn't even say that .free_inode is now required if one doesn't want to run into unexplainable behavior (perhaps only with some kernel releases along the way to full folio conversion); I was wondering what more in terms of problems can we expect if I try to go against that note, in a sense of leaving .destroy_inode
as it was.
Do you see any consequences of merging destroy_inode
into evict_inode
from the ZFS point of view? I couldn't think of anything so that's why I decided to do away with destroy_inode
by merging it, just an abundance of caution, aka "what else aren't they telling me in the note, when they didn't even say it's mandatory but just recommended"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, if the inode gets as far as evict_inode
being called on it, it's already done and zfs_zget
is going to keep looping until it disappears completely, isn't it so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@behlendorf ping, what do you think, do we keep it as I propose? I'd like to push this forward but this seems like it needs further input from you :) Thanks!
zfs_inode_free(ip); | ||
} | ||
|
||
static void __maybe_unused |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__maybe_unused
shouldn't be needed here or above. We can use the guard macro's here to build the required functions, or not.
Motivation and Context
#16608
Description
How Has This Been Tested?
Types of changes
Checklist:
Signed-off-by
.