-
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: O_TMPFILE and insert inode hash rework #16772
base: master
Are you sure you want to change the base?
Conversation
This change: - reworks inode hash insert so that its in topmost layer and with inode unlock in the same order as other Linux FS implementations do it - reworks O_TMPFILE creation to use new linux specific zp->z_is_tmpfile, this fixes a potential unchecked error return value of zpl_xattr_security_init - changes igrabs to zhold where it can - exchanges truncate_setsize for truncate_inode_pages_final in zpl_evict_inode to follow vfs.rst's "the method has to use truncate_inode_pages_final()" Signed-off-by: Pavel Snajdr <[email protected]>
902d554
to
b0c12ed
Compare
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.
To make this easier to review can you break each of these 4 logical changes in to it's own commit and open a PR for each. Conceptually, these seems to make sense but I'd like to work through them one at a time.
@@ -1717,7 +1717,7 @@ zfs_vget(struct super_block *sb, struct inode **ipp, fid_t *fidp) | |||
* Must have an existing ref, so igrab() | |||
* cannot return NULL | |||
*/ | |||
VERIFY3P(igrab(*ipp), !=, NULL); | |||
zhold(ITOZ(*ipp)); |
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.
Let's leave this as igrab()
. It's in the Linux specific code already, and switching it means it does a pointless ITOZ(ZTOI())
. Plus the comment above references igrab()
.
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'd rather modify the comment if it works for you, then a grep for igrab on the codebase should return the zhold
macro and zfs_zget
; this use here is exactly what zhold
translates to, so I think it would make sense to convert it - and only keep that one igrab in zget as is, because there we actually need the result of the condition.
Motivation and Context
Byproduct of investigation done for #16608 - these are the more optional bits, cosmetics, in hope to simplify reasoning about inode lifetime
Description
This change:
reworks inode hash insert so that its in topmost layer and with inode unlock in the same order as other Linux FS implementations do it
reworks
O_TMPFILE
creation to use new linux specificzp->z_is_tmpfile
, this fixes a potential unchecked error return value ofzpl_xattr_security_init
changes igrabs to zhold where it can
exchanges
truncate_setsize
fortruncate_inode_pages_final
inzpl_evict_inode
to followvfs.rst
's"the method has to use truncate_inode_pages_final()"
notes + questions:
not sure about the change from "working with inode" to "working with znode" in
zpl_link
, can remove that, that's a bit of a leftoverI'd like to eliminate the
txg_wait_synced
inzfs_link
in the tmpfile case, I'd log it as a new file creation, is that ok? edit: this probably won't work, or will be too much work, I have another idea:log the tmpfile creation at creation, log all the changes, acl/xattr/writes, if it comes to replay then put it to unlinked set replay all the changes and then the unlinked set get processed before mount anyway, right?
How Has This Been Tested?
Localy during development and via the bots also
Types of changes
Checklist:
Signed-off-by
.