Project

General

Profile

Actions

Bug #3980

closed

sdev vfs refcount assertion violation

Added by Robert Mustacchi almost 8 years ago. Updated over 7 years ago.

Status:
Resolved
Priority:
Normal
Category:
kernel
Start date:
2013-08-04
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

We're doing a vn_renameat. We have picked up the various vnodes that we
care about and are moving forward with them. When we call into
fop_rename, we have two references on the vnode in question. We never
add additional references from this point on.

This brings us to sdev_rename(), which takes us to sdev_rnmnode(). At
this point we still have to references on the original /dev/null vnode.
We end up calling into sdev_dirdelete() for the original sdev /dev/null
node.

sdev_dirdelete causes us to decrement a reference on the vnode and mark
it as a zombie. As a result we decrement the vnode reference count. At
this point we have a reference count of one.

For some other reason, unknown at this time, the call to sdev_rename()
fails, specifically with ENOENT.

sdev_rename checks to see if sdev_rnmnode failed or not. If it does, it
always will lower the reference count on the corresponding vnode. This
lowers the reference count to zero.

We then return vn_renameat and as part of exiting we VN_RELE the vnode.
However, that vnode now has a reference count of zero, so we blow the
call to VERIFY.

The next questions to ask are, why did the sdev_rename fail and should
we have reconstituted the removed node in that case? Who owned those
holds and thus was supposed to remove them? My gut is suggesting that
the combination of having the underlying node turned into a zommbie
which decrements the reference count here is potentially incorrect given
what may or may not be taking holds.
vn_renameat calls lookuppnat, which is how vn_renameat gets its hold on
the vnode corresponding to /dev/null. It does try to properly release
its hold. However, vn_renameat does not pass that vnode down to the
lower levels. At this point we have a reference of at least 1.

So, let's look at how sdev_rename gets the vnode and what it does with
it. It gets it through a call to vop_lookup. When we return from the
call to fop_lookup, we now have a vnode reference count of at least 2.
This means that the problem is squarely inside of sdev and specifically
the semantics of deleting that reference in sdev_dirdelete – perhaps
the bug itself, and the general use of that reference.

When we go to create the node, we actually try to add it to the
directory cache. Here things get a bit more complicated. At this point
we try and look up the old sdev_node in our vnode pointer. We end up
actually finding it and as a result of finding it, place a hold on it.
By placing a hold on it, we end up incrementing the reference count back
to two. That meanst that when we subsequently call into sdev_dirdelete
then we come back through here, set it to be a zombie again, decrement
the referencee count, and then end up getting ebusy again. This causes
us to fail the creation of the node. As we go back up, we end up leaving
this world with a reference count of 1.

Finally, as we leave sdev_rnmnode, we decrement the reference count to
zero. At which point when we get back to vn_renameat and do the vn_rele,
we panic on the blown assertion.

The solution to this is a bit involved. sdev_dirdelete is just buggy in general and does some things wrong, for example it has this notion of an EBUSY which doesn't make sense. The solution was to have sdev_dirdelete remove the node from the directory that it is, but the vnode remains valid as long as there are references on it. Solving this issue in particular, sdev_dirdelete no longer decrements references on the vnode itself. This makes it much clearer what the semantics are and what the expectations are for consumers of the sdev vnodes. If you get a reference, you should remove it.

Actions #1

Updated by Robert Mustacchi over 7 years ago

  • Status changed from New to Resolved

Resolved in 9e5aa9d8a21f8efa8ba9c9e4a0aa6edc66d07eb2.

Actions

Also available in: Atom PDF