Re: [PATCH] SGI 882960: Busy inodes after unmount, oops

[Greg Banks <gnb@melbourne.sgi.com>]

Olaf Kirch wrote:
> 
> Hi Greg,
> 
> I've been looking at your analysis a little closer, and am
> trying to understand how the bug was triggered. Here's what
> I think happened:
> 
>  -      the unlink code keeps a reference to the dentry of
>         the parent directory, but not to the vfsmount

Yes.

>  -      this allows the umount to proceed, because there
>         don't seem to be any more references to the mount

Yes.

BTW (not directly related to this bug) I found by experiment that I
could umount an NFS mount when there were open file descriptors for
unlinked files in the mount, and even keep writing.  All the NFS
and RPC structures stay alive until the last file descriptor closes,
thanks to the magic of refcounts.  All this despite the vfsmount
reference taken in struct file, which I thought was supposed to
prevent umount.

>  -      rpc_shutdown_client calls rpc_killall_tasks, which
>         terminates the async unlink task.

Yes.

>  -      rpciod is woken up and schedules the async task,
>         calling __rpc_execute

Yes.

>  -      __rpc_execute notices the task is dead (no tk_action),
>         leaves the loop and invokes task->tk_exit == nfs_async_unlink_done

No.  In a crash dump taken after the umount has completed, the dir dentry has
1 leaked d_count for every async unlink present at umount, even though the
async unlink tasks have been cleaned up.  This indicates that task->tk_exit
is not being called but task->tk_release is, so the dput is not happening.

The change in unlink.c moves the dput so that it happens in task->tk_release.
After the change, the dir dentry d_count is decremented to zero reliably
(I walked data structures before during and after umount in the debugger).

It's not entirely clear to me how __rpc_execute can do that, but the evidence
is that it does so.

>  -      nfs_async_unlink_done calls dput() on the parent dentry,
>         but the dentry is not unhashed.




> 
> Now looking at kill_super(), the sequence of calls there looks
> like this:
> 
>         shrink_dcache_parent(root);
>         ...
>         sop->put_super(sb);
>         ...
>         if (invalidate_inodes(sb)) {
>                 printk(KERN_ERR "VFS: Busy inodes after unmount. "
>                         "Self-destruct in 5 seconds.  Have a nice day...\n");
>         }
> 
> So the real problem is that the dentry isn't unhashed.

Yes, this was precisely the problem I encountered when I had the unlink.c
change but not the dir.c change.

> The other
> part of the patch isn't required because the tk_exit() function
> is called always, even when rpc_killall_tasks triggers the demise
> of an async task.

Surprisingly, it isn't.

> Do you agree with this analysis?

Almost.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.


-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs