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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Linux: s_op: use .free_inode #16788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions config/kernel-inode-free.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
AC_DEFUN([ZFS_AC_KERNEL_SRC_INODE_FREE], [
ZFS_LINUX_TEST_SRC([inode_free], [
#include <linux/fs.h>

static void inode_free(struct inode *ip)
{ return; }

static const struct super_operations
iops __attribute__ ((unused)) = {
.free_inode = inode_free,
};
],[])
])

AC_DEFUN([ZFS_AC_KERNEL_INODE_FREE], [
AC_MSG_CHECKING([whether inode_free() is available])
ZFS_LINUX_TEST_RESULT([inode_free], [
AC_MSG_RESULT(yes)
AC_DEFINE(HAVE_INODE_FREE, 1,
[.inode_free() i_op exists])
],[
AC_MSG_RESULT(no)
])
])

2 changes: 2 additions & 0 deletions config/kernel.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ AC_DEFUN([ZFS_AC_KERNEL_TEST_SRC], [
ZFS_AC_KERNEL_SRC_INSERT_INODE_LOCKED
ZFS_AC_KERNEL_SRC_TRUNCATE_SETSIZE
ZFS_AC_KERNEL_SRC_SECURITY_INODE
ZFS_AC_KERNEL_SRC_INODE_FREE
ZFS_AC_KERNEL_SRC_FST_MOUNT
ZFS_AC_KERNEL_SRC_SET_NLINK
ZFS_AC_KERNEL_SRC_SGET
Expand Down Expand Up @@ -183,6 +184,7 @@ AC_DEFUN([ZFS_AC_KERNEL_TEST_RESULT], [
ZFS_AC_KERNEL_INSERT_INODE_LOCKED
ZFS_AC_KERNEL_TRUNCATE_SETSIZE
ZFS_AC_KERNEL_SECURITY_INODE
ZFS_AC_KERNEL_INODE_FREE
ZFS_AC_KERNEL_FST_MOUNT
ZFS_AC_KERNEL_SET_NLINK
ZFS_AC_KERNEL_SGET
Expand Down
1 change: 1 addition & 0 deletions include/os/linux/zfs/sys/zfs_znode_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ struct znode;
extern int zfs_sync(struct super_block *, int, cred_t *);
extern int zfs_inode_alloc(struct super_block *, struct inode **ip);
extern void zfs_inode_destroy(struct inode *);
extern void zfs_inode_free(struct inode *);
extern void zfs_mark_inode_dirty(struct inode *);
extern boolean_t zfs_relatime_need_update(const struct inode *);
extern zil_replay_func_t *const zfs_replay_vector[TX_MAX_TYPE];
Expand Down
15 changes: 11 additions & 4 deletions module/os/linux/zfs/zfs_znode_os.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,9 +370,14 @@ zfs_inode_alloc(struct super_block *sb, struct inode **ip)
return (0);
}

/*
* Called in multiple places when an inode should be destroyed.
*/
void
zfs_inode_free(struct inode *ip)
{
znode_t *zp = ITOZ(ip);

kmem_cache_free(znode_cache, zp);
}

void
zfs_inode_destroy(struct inode *ip)
{
Expand All @@ -395,7 +400,9 @@ zfs_inode_destroy(struct inode *ip)
zp->z_xattr_cached = NULL;
}

kmem_cache_free(znode_cache, zp);
#ifndef HAVE_INODE_FREE
zfs_inode_free(ip);
#endif
}

static void
Expand Down
15 changes: 14 additions & 1 deletion module/os/linux/zfs/zpl_super.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,13 @@ zpl_inode_alloc(struct super_block *sb)
return (ip);
}

static void
static void __maybe_unused
zpl_inode_free(struct inode *ip)
{
zfs_inode_free(ip);
}

static void __maybe_unused
Copy link
Contributor

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.

zpl_inode_destroy(struct inode *ip)
{
ASSERT(atomic_read(&ip->i_count) == 0);
Expand Down Expand Up @@ -89,6 +95,9 @@ zpl_evict_inode(struct inode *ip)
truncate_setsize(ip, 0);
clear_inode(ip);
zfs_inactive(ip);
#ifdef HAVE_INODE_FREE
zfs_inode_destroy(ip);
#endif
spl_fstrans_unmark(cookie);
}

Expand Down Expand Up @@ -380,7 +389,11 @@ zpl_prune_sb(uint64_t nr_to_scan, void *arg)

const struct super_operations zpl_super_operations = {
.alloc_inode = zpl_inode_alloc,
#ifdef HAVE_INODE_FREE
.free_inode = zpl_inode_free,
#else
.destroy_inode = zpl_inode_destroy,
#endif
Copy link
Contributor

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.

Copy link
Contributor Author

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"

Copy link
Contributor Author

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?

Copy link
Contributor Author

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!

.dirty_inode = zpl_dirty_inode,
.write_inode = NULL,
.evict_inode = zpl_evict_inode,
Expand Down