Do not hash unlinked inodes in zfs_znode_alloc

[PaulZ-98]

Motivation and Context

Fix panic in zfs_znode_alloc

When zfs_suspend_fs is doing zrele (iput) in an async fashion, and zfs_resume_fs unlinked drain processing tries to hash an inode that is still hashed, then VERIFY3S(insert_inode_locked(ip), ==, 0 will fail, and we panic.

#9741
#11223
#11648

Description

In zfs_znode_alloc we always hash inodes. If the znode is unlinked, we do not need to hash it. This fixes the problem where zfs_suspend_fs is doing zrele (iput) in an async fashion, and zfs_resume_fs unlinked drain processing tries to hash an inode that could still be hashed, resulting in a panic.

How Has This Been Tested?

See #9741 for the reproducer code change and script. With the fix in place, the reproducer no longer produces the panic.

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)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[behlendorf]

Since you already have a test case it would be really nice to adapt it to the ZTS test suite. Aside from that, and determining what (if anything) should be done for FreeBSD, this looks good.

[PaulZ-98]

Since you already have a test case it would be really nice to adapt it to the ZTS test suite. Aside from that, and determining what (if anything) should be done for FreeBSD, this looks good.

To reproduce this I had to exaggerate the time it takes to do iput. Should I include that too?

--- a/module/os/linux/zfs/zfs_vnops_os.c
+++ b/module/os/linux/zfs/zfs_vnops_os.c
@@ -175,6 +175,8 @@
  *     return (error);                 // done, report error
  */
 
+unsigned long zfs_iput_delay = 0;
+
 /* ARGSUSED */
 int
 zfs_open(struct inode *ip, int mode, int flag, cred_t *cr)
@@ -367,6 +369,16 @@ zfs_write_simple(znode_t *zp, const void *data, size_t len,
        return (error);
 }
 
+static void
+iput_special(struct inode *ip)
+{
+       if (zfs_iput_delay) {
+               zfs_dbgmsg("delayed iput for %lld", ITOZ(ip)->z_id);
+               delay(4 * hz);
+       }
+       iput(ip);
+}
+
 void
 zfs_zrele_async(znode_t *zp)
 {
@@ -384,9 +396,10 @@ zfs_zrele_async(znode_t *zp)
         * For more information on the dangers of a synchronous iput, see the
         * header comment of this file.
         */
+       zfs_dbgmsg("async rele for %lld", (longlong_t)zp->z_id);
        if (!atomic_add_unless(&ip->i_count, -1, 1)) {
                VERIFY(taskq_dispatch(dsl_pool_zrele_taskq(dmu_objset_pool(os)),
-                   (task_func_t *)iput, ip, TQ_SLEEP) != TASKQID_INVALID);
+                   (task_func_t *)iput_special, ip, TQ_SLEEP) != TASKQID_INVALID);
        }
 }
 
@@ -3988,6 +4001,9 @@ EXPORT_SYMBOL(zfs_map);
 /* BEGIN CSTYLED */
 module_param(zfs_delete_blocks, ulong, 0644);
 MODULE_PARM_DESC(zfs_delete_blocks, "Delete files larger than N blocks async");
+
+module_param(zfs_iput_delay, ulong, 0644);
+MODULE_PARM_DESC(zfs_iput_delay, "Delay async zrele (iput)");
 /* END CSTYLED */
 
 #endif

[behlendorf]

Ahh, I see. Well in that case I understand why you opted not to include the test case. Given that, I'm fine with not adding the test case for this.