Page MenuHomeFreeBSD

Break recursion involving getnewvnode and zfs_rmnode
ClosedPublic

Authored by benno on May 7 2018, 10:21 PM.

Details

Summary

When we're at our vnode limit, getnewvnode will call into the vnode LRU cache to free up vnodes. If the vnode we try to recycle is a ZFS vnode we end up, eventually, in zfs_rmnode. If the ZFS vnode we're recycling represents something with extended attributes, zfs_rmnode will call zfs_zget which will attempt to allocate another vnode. If the next vnode we try to recycle is also a ZFS vnode representing something with extended attributes we can recurse further. This ends up being unbounded and can end up overflowing the stack.

In order to avoid this, restructure zfs_rmnode to simply add the extended attribute directory's object ID to the unlinked set, thus not requiring the allocation of a vnode. We then schedule a task that calls zfs_unlinked_drain which will do the work of properly marking the vnodes for unlinking. zfs_unlinked_drain is also called on mount so these will be cleaned up there.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

The change looks good to me. A couple of small comments inline.
Thank you!

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_dir.c
475 ↗(On Diff #42248)

Here I would place the new code under ifdef FreeBSD and keep the old code.
This also seems like a good place to add a comment describing why we defer the attribute directory removal to the taskqueue.

486 ↗(On Diff #42248)

I think that we still need to reset z_links in the drain path.
Or amend the assert about z_links at the start of this function...
Have you tested the change with INVARIANTS ?

benno marked 2 inline comments as done.

Update based on review from avg:

  • Ensure link count is set to 0 in zfs_unlinked_drain
  • Mark larger changes with #if defined(FreeBSD)

I am not the right person to review the ZFS-specific parts of this change. But the approach of avoiding recursive invocations of getnewvnode is needed and this change accomplishes that requirement.

This revision is now accepted and ready to land.Jun 2 2018, 6:25 PM

LGTM.
Thank you!

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
975 ↗(On Diff #43252)

I would just move the function here and dropped the forward declaration.

Looks good to me. Thanks you. I agree with avg@ that forward declaration of the function of few lines is not very useful.

Move unlinked drain task function.
Mark a bunch more stuff as FreeBSD-specific.

This revision now requires review to proceed.Jun 7 2018, 6:52 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 7 2018, 6:59 PM
This revision was automatically updated to reflect the committed changes.