zfsctl_umount_snapshots

[Will Andrews]

Hi,

Looking at the Illumos version here.

How can it work that zfsctl_umount_snapshots() removes a snapshot
entry prior to unmounting it?

Shouldn't this unmount (especially if MS_FORCE) release the last hold
on sep->se_root, causing zfsctl_snapshot_inactive() to get called on
it? In that function, there's an assert that it must have removed
(and subsequently freed) the snapshot entry corresponding to the vnode
being inactived.

Even if that's not how it works, how does the snapshot entry get
cleaned up from zfsctl_umount_snapshots() in the success case? Upon
completion, the entry is not visible in the snapdir's AVL tree
anymore, and zfsctl_umount_snapshots() doesn't free it.

Thanks...
--Will.

[Will Andrews]

I see it's freed by zfsctl_unmount_snap(). It seems a bit ugly that
there are two places to free the snapshot entry. That said, this
comment in zfsctl_unmount_snap() seems wrong:

547 /*
548 * We can't use VN_RELE(), as that will try to invoke
549 * zfsctl_snapdir_inactive(), which would cause us to
destroy
550 * the sd_lock mutex held by our caller.
551 */

zfsctl_snapdir_inactive(), the vop_inactive for svp's parent,
shouldn't be called by virtue of calling VN_RELE(svp) here, since it
was held by the caller in order to obtain the snapdir vnode, right?

Isn't the real issue with VN_RELE(svp) here the fact that
zfsctl_snapshot_inactive() requires looking up the snapdir vnode and
grabbing its lock (already held by zfsctl_umount_snapshots()) in order
to remove the snapshot entry?

--Will.

Post by Will Andrews
Hi,
Looking at the Illumos version here.
How can it work that zfsctl_umount_snapshots() removes a snapshot
entry prior to unmounting it?
Shouldn't this unmount (especially if MS_FORCE) release the last hold
on sep->se_root, causing zfsctl_snapshot_inactive() to get called on
it? In that function, there's an assert that it must have removed
(and subsequently freed) the snapshot entry corresponding to the vnode
being inactived.
Even if that's not how it works, how does the snapshot entry get
cleaned up from zfsctl_umount_snapshots() in the success case? Upon
completion, the entry is not visible in the snapdir's AVL tree
anymore, and zfsctl_umount_snapshots() doesn't free it.
Thanks...
--Will.

[Matthew Ahrens]

Post by Will Andrews
I see it's freed by zfsctl_unmount_snap(). It seems a bit ugly that
there are two places to free the snapshot entry. That said, this
547 /*
548 * We can't use VN_RELE(), as that will try to invoke
549 * zfsctl_snapdir_inactive(), which would cause us to
destroy
550 * the sd_lock mutex held by our caller.
551 */
zfsctl_snapdir_inactive(), the vop_inactive for svp's parent,
shouldn't be called by virtue of calling VN_RELE(svp) here, since it
was held by the caller in order to obtain the snapdir vnode, right?
Isn't the real issue with VN_RELE(svp) here the fact that
zfsctl_snapshot_inactive() requires looking up the snapdir vnode and
grabbing its lock (already held by zfsctl_umount_snapshots()) in order
to remove the snapshot entry?

I'm not that familiar with this code, but your explanation makes sense to
me. I guess if you want to be sure, before updating the comment, you could
try using VN_RELE() and see what breaks :)

--matt

Post by Will Andrews
--Will.

Post by Will Andrews
Hi,
Looking at the Illumos version here.
How can it work that zfsctl_umount_snapshots() removes a snapshot
entry prior to unmounting it?
Shouldn't this unmount (especially if MS_FORCE) release the last hold
on sep->se_root, causing zfsctl_snapshot_inactive() to get called on
it? In that function, there's an assert that it must have removed
(and subsequently freed) the snapshot entry corresponding to the vnode
being inactived.
Even if that's not how it works, how does the snapshot entry get
cleaned up from zfsctl_umount_snapshots() in the success case? Upon
completion, the entry is not visible in the snapdir's AVL tree
anymore, and zfsctl_umount_snapshots() doesn't free it.
Thanks...
--Will.

-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460
https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.com

-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com