Re: [PATCH][BUG] Lack of mutex_lock in drop_pagecache_sb()

[Wu Fengguang <fengguang.wu@intel.com>]

Masasyoshi, 

On Wed, Mar 18, 2009 at 05:13:35PM +0900, Masasyoshi MIZUMA wrote:
> I create the patch which fixes lack of mutex_lock in drop_pagecache_sb().
> Please check the bug and the patch (below).

Is this a real producible bug or a theory one?
IMHO the I_FREEING flag should avoid the race.

> ----------------------------------------------------------------------
> 
> When drop_pagecache_sb() frees inodes, it doesn't get mutex_lock of 
> iprune_mutex. Therefore, if it races the process which frees inodes 
> (ex. prune_icache()), OS panic may happen.
> 
> An example of the panic flow is the following:
> ----------------------------------------------------------------------
>             [process A]               |         [process B]
>  |                                    |
>  |  shrink_icache_memory()            |
>  |      |                             |
>  |      V                             |
>  |    prune_icache()                  |  drop_pagecache()
>  |      mutex_lock(&iprune_mutex)     |      |
>  |      spin_lock(&inode_lock)        |      |
>  |          |                         |      V
>  |          |                         |    drop_pagecache_sb()
>  |          |                         |        |

          inode->i_state |= I_FREEING;

>  |          V                         |        V
>  |      spin_unlock(&inode_lock)      |      spin_lock(&inode_lock)
>  |          |                         |          |

                                                if (inode->i_state & (I_FREEING|I_WILL_FREE))
                                                        continue;

>  |          |                         |          |
>  |          V                         |          V
>  |      dispose_list()                |        __iget()
>  |        list_del()                  |            |
>  |            |                       |            |
>  |            V                       |            V
>  |        spin_lock(&inode_lock)      |          list_move() <----- PANIC !!
>  |                                    |
>  V                                    |
> (time)
> ----------------------------------------------------------------------
> If the inode which Process B do list_move() with is the same as the one which
> Process A did list_del() with, OS may panic.

Thanks,
Fengguang