In fact I am not sure about this revision, as well as with the VNODE_IS_DOOMED(). My main concern is that we allowed to drop the vnode lock in the course of vgone(l)(). With this in mind, I do not see anything wrong, and rather it is useful to clean pages of the vnode if writes are still working, i.e. as far as we did not called RECLAIM.

For VNODE_IS_DOOMED(), you changed the logic significantly if the vnode unlock is possible, before it code relied on the fact that started vgonel() is noted, with VNODE_IS_DOOMED() it would not notice until the vnode is completely dismantled up to the point of resetting v_ops vector.

If you are worried about catching code doing such an unlock, we can add an assertion to unlock that if VI_DOOMED is set, v_op is dead_vnodeops. If want such an unlock to remain legitimate, I can move v_iflag to the beginning of the struct. Right now there is a 4 byte hole in there on amd64 (after v_tag got moved). I did not want to do it because it adds an avoidable constraint on reshuffling of the layout for later.

That said, I'm fine with leaving LK_RETRY in place and removing the error check instead. However, in the current codebase I don't see a case where VI_DOOMED vnode would get returned and v_object would still be set.

Regardless of the above, the current approach to dooming is buggy. Consider a filesystem which uses custom locking routines not based around lockmgr.

1. the thread which locked the vnode and called vgonel ends up calling lockmgr's unlock fetched from dead_vnodeops. meaning the actual lock may not get proper rollback
2. a concurrently executing thread doing _vn_lock can get itself into the fs-specific locking routine. it either never gets released, thus the thread gets stuck or it gets released and the thread gets the wrong lock. if LK_RETRY was passed, it proceeds when it should not. if it is not set, it performs the mismatched unlock like the thread above

That being said, I think a dedicated VOP should be provided for filesystems to take care of this (and the rest of vgonel).

In the meantime we can just go with that v_iflag move as least contentious.

Still, I think that proposed assert would be very useful and either removes FUD and unblocks some of your patches, or proves the point.

In D22689#497216, @kib wrote:

Still, I think that proposed assert would be very useful and either removes FUD and unblocks some of your patches, or proves the point.

I will add the assert later. Note I created a new review for the vn_lock problem which does not depend on the asserted property. D22715