Konstantin Belousov wrote this message on Wed, Sep 25, 2013 at 22:40 +0300: > On Wed, Sep 25, 2013 at 09:19:54AM -0700, John-Mark Gurney wrote: > > Konstantin Belousov wrote this message on Wed, Sep 25, 2013 at 00:21 +0300: > > > On Tue, Sep 24, 2013 at 10:45:17AM -0700, John-Mark Gurney wrote: > > > > I'd like to understand why you think protecting these functions w/ > > > > the _DETACHED check is correct... In kern_event.c, all calls to > > > > f_detach are followed by knote_drop which will ensure that the knote > > > > is removed and free, so no more f_event calls will be called on that > > > > knote.. > > > > > > My current belief is that what happens is a glitch in the > > > kqueue_register(). After a new knote is created and attached, the kq > > > lock is dropped and then f_event() is called. If the vnode is reclaimed > > > or possible freed meantime, f_event() seems to dereference freed memory, > > > since kn_hook points to freed vnode. > > > > Well, if that happens, then the vnode isn't properly clearing up the > > knote before it gets reclaimed... It is the vnode's responsibility to > > make sure any knotes that are associated w/ it get cleaned up properly.. > See below. > > > > > > The issue as I see it is that vnode lifecycle is detached from the knote > > > lifecycle. Might be, only the second patch, which acquires a hold > > > reference > > > on the vnode for each knote, is really needed. But before going into any > > > conclusions, I want to see the testing results. > > > > The vnode lifecycle can't/shouldn't be detached from the knote lifecycle > > since the knote contains a pointer to the vnode... There is the function > > knlist_clear that can be used to clean up knotes when the object goes > > away.. > This is done from the vdropl() (null hold count) -> destroy_vpollinfo(). > But this is too late, IMO. vdropl() is only executing with the vnode > interlock locked, and knote lock is vnode lock. This way, you might > get far enough into vdropl in other thread, while trying to operate on > a vnode with zero hold count in some kqueue code path. > > We do not drain the vnode lock holders when destroying vnode, because > VFS interface require that anybody locking the vnode own a hold reference > on it. My short patch should fix exactly this issue, hopefully we will see.
Which clearly wasn't happening before... With the above, and rereading your patch, I understand how this patch should fix the issue and bring the life cycle of the two back into sync... > > I was looking at the code, is there a good reason why you do > > VI_LOCK/VI_UNLOCK to protect the knote fields instead of getting it in > > the vfs_knllock/vfs_knlunlock functions? Because kq code will modify > > the knote fields w/ only running the vfs_knllock/vfs_knlunlock functions, > > so either the VI_LOCK/VI_UNLOCK are unnecessary, or should be moved to > > vfs_knllock/vfs_knlunlock... > > vfs_knllock() is fine. The problematic case if the > VOP_{PRE,POST}->VFS_KNOTE->VN_KNOTE->KNOTE calls from the VOPs. If you > look at the vfs_knl_assert_locked(), you would note that the function > only asserts that vnode is locked, not that it is locked exclusively. > This is because some filesystems started require from VFS to do e.g. > VOP_WRITE() with the vnode only shared-locked, and KNOTE() is called > with shared-locked vnode lock. > > The vfs_knllock() obtain the exclusive lock on the vnode, so kqueue > callers are fine. Taking vnode interlock inside the filters provides > enough exclusion for the VOP callers. Ahh, ok, makes sense now.. Clearly I need to learn more about the VFS/vnope system.. :) Thanks for the explanations... -- John-Mark Gurney Voice: +1 415 225 5579 "All that I will do, has been done, All that I have, has not." _______________________________________________ freebsd-stable@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-stable To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"