Approval request of some additions to src/sys/kern/vfs_subr.c and src/sys/sys/vnode.h

[Daichi GOTO]

Thanks for your response and explanation :)

Kostik Belousov wrote:
> On Mon, Apr 28, 2008 at 06:37:09PM +0900, Daichi GOTO wrote:
>> Kostik Belousov wrote:
>>> On Fri, Apr 25, 2008 at 07:21:20PM +0900, Daichi GOTO wrote:
>>>> Hi Konstantin :)
>>>>
>>>> To fix a unionfs issue of 
>>>> http://www.freebsd.org/cgi/query-pr.cgi?pr=109377,
>>>> we need to add new functions
>>>>
>>>>   void vkernrele(struct vnode *vp);
>>>>   void vkernref(struct vnode *vp);
>>>>
>>>> and one value
>>>>
>>>>   int	v_kernusecount; /* i ref count of kernel */
>>>>
>>>> to src/sys/sys/vnode.h and rc/sys/kern/vfs_subr.c.
>>>>
>>>> Unionfs will be panic when lower fs layer is forced umounted by
>>>> "umount -f".  So to avoid this issue, we've added
>>>> "v_kernusecount" value that means "a vnode count that kernel are
>>>> using".  vkernrele() and vkernref() are functions that manage
>>>> "v_kernusecount" value.
>>>>
>>>> Please check those and give us an approve or some comments!
>>> There is already the vnode reference count. From your description, I cannot
>>> understand how the kernusecount would prevent the panic when forced unmount
>>> is performed. Could you, please, show the actual code ? PR mentioned
>>> does not contain any patch.
>> Our patch realizes avoiding kernel panic by "umount -f" operation using with
>> EBUSY process.
>>
>> On current implementation (not applied our patch), "umount -f" tries to
>> release vnode at any vnode reference count value. Since that, unionfs
>> and nullfs access invalid vnode and lead kernel panic. To prevent this
>> issue, we need a some kind of not-umount-accept-mechanism in invalid case
>> (e.x. fs in unionfsed stack, it must be umounted in correct order) and
>> to realize that, current vnode reference count is not enough we are 
>> thinking.
>>
>> If you have any ideas to realize the same solution with current vnode
>> reference, would you please tell us your idea :)
>>
>>> The problem you described is common for the kernel code, and right way
>>> to handle it, for now, is to keep refcount _and_ check for the forced
>>> reclaim.
> 
> Your patch in essence disables the forced unmount. I would object against
> such decision.

Oooooo....   OK. We understand.

> Even if taking this direction, I believe more cleaner solution would be
> to introduce a counter that disables the (forced) unmount into the
> struct mount, instead of the struct vnode. Having the counter in the
> vnode, the unmount -f behaviour is non-deterministic and depended on
> the presence of the cached vnodes of the upper layer. The mount counter
> would be incremented by unionfs cover mount. But, as I said above, this
> looks like a wrong solution.
> 
> The right way to handle the forced reclaim with the current VFS is to
> add the explicit checks for the reclaimed vnodes where it is needed. The
> vnode cannot be reclaimed while the vnode lock is held. When obtaining
> the vnode lock, the reclamation can be detected. For instance, the
> vget() without LK_RETRY shall be checked for ENOENT.

At last, we want to check that vnode is released or not where
unionfs does not know. If we can do that check, our patch is
not needed for solving that issue.

Would you please give us the way to check that target vnode is
released or not before accessing it.


> You said that that nullfs is vulnerable to the problem. Could you,
> please, point me to the corresponding stack trace ? At least, the nullfs
> vop_lock() seems to carefully check the possible problems.

-- 
   Daichi GOTO, http://people.freebsd.org/~daichi