Re: [JFFS2 & UMOUNT/VFS] is it safe to iget() during unmount?

["Artem B. Bityuckiy" <dedekind@infradead.org>]

Hello, no suggestion? Al, could you please comment?

On Thu, 16 Dec 2004, Artem B. Bityuckiy wrote:

> Hello.
> 
> I've hit on one problem with JFFS2. In JFFS2 there is a kernel thread 
> (called GC thread)
> which just performs the background auxiliary job. This thread may issue 
> the iget() call to obtain inodes.
> 
> On JFFS2 unmount, the GC thread is killed in put_super() call. The 
> following are some extracts from fs/jffs2/super.c code:
> 
> /* The put_super VFS callback */
> static void jffs2_put_super (struct super_block *sb)
> {
>   ... bla ... bla ...
>   jffs2_stop_garbage_collect_thread(c); /* KILL GC thread */
>   ... other bla bla bla ...
> }
> 
> /* The kill_sb VFS callback */
> static void jffs2_kill_sb(struct super_block *sb)
> {
>   .. bla ...
>   generic_shutdown_super(sb);
>   .. bla ...
> }
> 
> The situation when my problem arises is:
> 1. JFFS2 umount call is issued from the user space.
> 2. The generic_shutdown_super() VFS function is called. The code of 
> the generic_shutdown_super() function is at the end of this letter, for 
> reference (kernel 2.6.10-rcX).
> 3. The generic_shutdown_super() function is preempted  somewhere in the 
> middle, the sop->put_super(sb) is not called yet. I do not know where 
> exactly is it preempted, guess somewhere between sop->write_super(sb) and 
> sop->put_super(sb).
> 4. The GC thread wakes up and starts working (it was not killed yet since 
> the put_super was not called yet). GC thread calls iget for inode which is 
> for sure in the inode cache since there was not clear_inode call yet. But 
> VFS calls read_inode for that inode. And nasty things start from this 
> point.
> 
> I've offered patch to JFFS2 which changes it to kill the GC thread 
> *before* calling the generic_shutdown_super(), in kill_sb, like this:
> 
> /* The put_super VFS callback */
> static void jffs2_put_super (struct super_block *sb)
> {
>   ... bla ... bla ...
>   jffs2_stop_garbage_collect_thread(c); /* KILL GC thread */
>   generic_shutdown_super(sb);
>   ... other bla bla bla ...
> }
> 
> This solves problems at least in my case. But I was referred to the VFS 
> developers list and asked to ask  Al Viro's comments.
> 
> So, Al or somebody else, could you please coment this? Is it allowed to 
> call iget() during unmount()? Why?
> 
> P.S: CC me since I'm not in the list.
> 
> Thanks.
> 
> 
> 
> void generic_shutdown_super(struct super_block *sb)
> {
>         struct dentry *root = sb->s_root;
>         struct super_operations *sop = sb->s_op;
>                                                                                                                                              
>         if (root) {
>                 sb->s_root = NULL;
>                 shrink_dcache_parent(root);
>                 shrink_dcache_anon(&sb->s_anon);
>                 dput(root);
>                 fsync_super(sb);
>                 lock_super(sb);
>                 lock_kernel();
>                 sb->s_flags &= ~MS_ACTIVE;
>                 /* bad name - it should be evict_inodes() */
>                 invalidate_inodes(sb);
>                                                                                                                                              
>                 if (sop->write_super && sb->s_dirt)
>                         sop->write_super(sb);
>                 if (sop->put_super)
>                         sop->put_super(sb);
>                                                                                                                                              
>                 /* Forget any remaining inodes */
>                 if (invalidate_inodes(sb)) {
>                         printk("VFS: Busy inodes after unmount. "
>                            "Self-destruct in 5 seconds.  Have a nice 
> day...\n");
>                 }
>                                                                                                                                              
>                 unlock_kernel();
>                 unlock_super(sb);
>         }
>         spin_lock(&sb_lock);
>         /* should be initialized for __put_super_and_need_restart() */
>         list_del_init(&sb->s_list);
>         list_del(&sb->s_instances);
>         spin_unlock(&sb_lock);
>         up_write(&sb->s_umount);
> }
> 
> --
> Best Regards,
> Artem B. Bityuckiy,
> St.-Petersburg, Russia.
> 

--
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.