* [PATCH][BUG] Lack of mutex_lock in drop_pagecache_sb()
@ 2009-03-18 8:13 Masasyoshi MIZUMA
2009-03-23 10:38 ` Wu Fengguang
0 siblings, 1 reply; 28+ messages in thread
From: Masasyoshi MIZUMA @ 2009-03-18 8:13 UTC (permalink / raw)
To: linux-fsdevel
I create the patch which fixes lack of mutex_lock in drop_pagecache_sb().
Please check the bug and the patch (below).
----------------------------------------------------------------------
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()
| | | |
| V | V
| spin_unlock(&inode_lock) | spin_lock(&inode_lock)
| | | |
| | | |
| 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.
Fixing this bug requires the following:
- Adding mutex_lock of iprune_mutex to drop_pagecache_sb().
- Changing iprune_mutex from static to global.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
---
fs/drop_caches.c | 2 ++
fs/inode.c | 2 +-
include/linux/writeback.h | 1 +
3 files changed, 4 insertions(+), 1 deletion(-)
diff -Nurp linux-2.6.29-rc7.orig/fs/drop_caches.c linux-2.6.29-rc7/fs/drop_caches.c
--- linux-2.6.29-rc7.orig/fs/drop_caches.c 2009-03-10 06:38:17.137283010 +0900
+++ linux-2.6.29-rc7/fs/drop_caches.c 2009-03-11 04:04:29.705284864 +0900
@@ -16,6 +16,7 @@ static void drop_pagecache_sb(struct sup
{
struct inode *inode, *toput_inode = NULL;
+ mutex_lock(&iprune_mutex);
spin_lock(&inode_lock);
list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
if (inode->i_state & (I_FREEING|I_WILL_FREE))
@@ -31,6 +32,7 @@ static void drop_pagecache_sb(struct sup
}
spin_unlock(&inode_lock);
iput(toput_inode);
+ mutex_unlock(&iprune_mutex);
}
static void drop_pagecache(void)
diff -Nurp linux-2.6.29-rc7.orig/fs/inode.c linux-2.6.29-rc7/fs/inode.c
--- linux-2.6.29-rc7.orig/fs/inode.c 2009-03-10 06:38:22.404282773 +0900
+++ linux-2.6.29-rc7/fs/inode.c 2009-03-18 07:17:31.032284423 +0900
@@ -91,7 +91,7 @@ DEFINE_SPINLOCK(inode_lock);
* from its final dispose_list, the struct super_block they refer to
* (for inode->i_sb->s_op) may already have been freed and reused.
*/
-static DEFINE_MUTEX(iprune_mutex);
+DEFINE_MUTEX(iprune_mutex);
/*
* Statistics gathering..
diff -Nurp linux-2.6.29-rc7.orig/include/linux/writeback.h linux-2.6.29-rc7/include/linux/writeback.h
--- linux-2.6.29-rc7.orig/include/linux/writeback.h 2009-03-10 06:38:27.133285255 +0900
+++ linux-2.6.29-rc7/include/linux/writeback.h 2009-03-11 05:27:43.679283477 +0900
@@ -10,6 +10,7 @@
struct backing_dev_info;
extern spinlock_t inode_lock;
+extern struct mutex iprune_mutex;
extern struct list_head inode_in_use;
extern struct list_head inode_unused;
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH][BUG] Lack of mutex_lock in drop_pagecache_sb() 2009-03-18 8:13 [PATCH][BUG] Lack of mutex_lock in drop_pagecache_sb() Masasyoshi MIZUMA @ 2009-03-23 10:38 ` Wu Fengguang 2009-03-24 7:06 ` Masayoshi MIZUMA 0 siblings, 1 reply; 28+ messages in thread From: Wu Fengguang @ 2009-03-23 10:38 UTC (permalink / raw) To: Masasyoshi MIZUMA; +Cc: linux-fsdevel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][BUG] Lack of mutex_lock in drop_pagecache_sb() 2009-03-23 10:38 ` Wu Fengguang @ 2009-03-24 7:06 ` Masayoshi MIZUMA 2009-03-24 7:44 ` Wu Fengguang 0 siblings, 1 reply; 28+ messages in thread From: Masayoshi MIZUMA @ 2009-03-24 7:06 UTC (permalink / raw) To: Wu Fengguang; +Cc: linux-fsdevel, viro Hi, Fengguang On Mon, 23 Mar 2009 18:38:46 +0800 Wu Fengguang <fengguang.wu@intel.com> wrote: > 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). > Thank you for your comment, and I apologize to you for my lack of explanation. > Is this a real producible bug or a theory one? This is a real bug. > IMHO the I_FREEING flag should avoid the race. I supplement the explanation for this problem. clear_inode() is called by dispose_list(), and sets the inode's i_state to I_CLEAR. Therefore, the following conditional expression doesn't match for the inode: "if (inode->i_state & (I_FREEING|I_WILL_FREE)) continue;" As the result, this problem can happen. > > > ---------------------------------------------------------------------- > > > > 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. I applied your comment and then modified the panic flow figure. Please check below: ---------------------------------------------------------------------- [process A] | [process B] | | | shrink_icache_memory() | | | | | V | | prune_icache() | drop_pagecache() | mutex_lock(&iprune_mutex) | | | spin_lock(&inode_lock) | | | | | V | | | drop_pagecache_sb() | | | | | V | | | inode->i_state |= I_FREEING; | | | | | | | V | V | spin_unlock(&inode_lock) | spin_lock(&inode_lock) | | | | | | | | | V | | | dispose_list() | | | list_del() | | | | | | | V | | | clear_inode() | | | inode->i_state = I_CLEAR | | | | | | | | | V | | | if (inode->i_state & (I_FREEING|I_WILL_FREE)) | | | continue; <---- NOT MATCH | | | | | | | V | | | __iget() | | | | | V | V | spin_lock(&inode_lock) | list_move() <----- PANIC !! | | V | (time) ---------------------------------------------------------------------- Thanks, Masayoshi MIZUMA ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][BUG] Lack of mutex_lock in drop_pagecache_sb() 2009-03-24 7:06 ` Masayoshi MIZUMA @ 2009-03-24 7:44 ` Wu Fengguang 2009-03-24 12:05 ` Jan Kara 0 siblings, 1 reply; 28+ messages in thread From: Wu Fengguang @ 2009-03-24 7:44 UTC (permalink / raw) To: Masayoshi MIZUMA; +Cc: linux-fsdevel, viro, Jan Kara, Nick Piggin Hi Masayoshi, On Tue, Mar 24, 2009 at 03:06:45PM +0800, Masayoshi MIZUMA wrote: > Hi, Fengguang > > On Mon, 23 Mar 2009 18:38:46 +0800 > Wu Fengguang <fengguang.wu@intel.com> wrote: > > > 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). > > > Thank you for your comment, and I apologize to you for my lack > of explanation. > > > Is this a real producible bug or a theory one? > This is a real bug. > > > IMHO the I_FREEING flag should avoid the race. > I supplement the explanation for this problem. > > clear_inode() is called by dispose_list(), and sets the inode's > i_state to I_CLEAR. Therefore, the following conditional expression > doesn't match for the inode: > "if (inode->i_state & (I_FREEING|I_WILL_FREE)) continue;" > As the result, this problem can happen. > > > > > > ---------------------------------------------------------------------- > > > > > > 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. > > I applied your comment and then modified the panic flow figure. > Please check below: > ---------------------------------------------------------------------- > [process A] | [process B] > | | > | shrink_icache_memory() | > | | | > | V | > | prune_icache() | drop_pagecache() > | mutex_lock(&iprune_mutex) | | > | spin_lock(&inode_lock) | | > | | | V > | | | drop_pagecache_sb() > | | | | > | V | | > | inode->i_state |= I_FREEING; | | > | | | | > | V | V > | spin_unlock(&inode_lock) | spin_lock(&inode_lock) > | | | | > | | | | > | V | | > | dispose_list() | | > | list_del() | | > | | | | > | V | | > | clear_inode() | | > | inode->i_state = I_CLEAR | | > | | | | > | | | V > | | | if (inode->i_state & (I_FREEING|I_WILL_FREE)) > | | | continue; <---- NOT MATCH > | | | | > | | | V > | | | __iget() > | | | | > | V | V > | spin_lock(&inode_lock) | list_move() <----- PANIC !! > | | > V | > (time) > ---------------------------------------------------------------------- Ah thanks for the explanation! How about this lightweight fix? Since s_umount is already taken in drop_pagecache(), it's not necessary to take iprune_mutex again. Thanks, Fengguang --- fs/drop_caches.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- mm.orig/fs/drop_caches.c +++ mm/fs/drop_caches.c @@ -18,7 +18,7 @@ static void drop_pagecache_sb(struct sup spin_lock(&inode_lock); list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { - if (inode->i_state & (I_FREEING|I_WILL_FREE)) + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE)) continue; if (inode->i_mapping->nrpages == 0) continue; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][BUG] Lack of mutex_lock in drop_pagecache_sb() 2009-03-24 7:44 ` Wu Fengguang @ 2009-03-24 12:05 ` Jan Kara 2009-03-24 12:11 ` Wu Fengguang 2009-03-24 12:40 ` [PATCH] skip I_CLEAR state inodes Wu Fengguang 0 siblings, 2 replies; 28+ messages in thread From: Jan Kara @ 2009-03-24 12:05 UTC (permalink / raw) To: Wu Fengguang; +Cc: Masayoshi MIZUMA, linux-fsdevel, viro, Jan Kara, Nick Piggin On Tue 24-03-09 15:44:57, Wu Fengguang wrote: > Hi Masayoshi, > > On Tue, Mar 24, 2009 at 03:06:45PM +0800, Masayoshi MIZUMA wrote: > > Hi, Fengguang > > > > On Mon, 23 Mar 2009 18:38:46 +0800 > > Wu Fengguang <fengguang.wu@intel.com> wrote: > > > > > 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). > > > > > Thank you for your comment, and I apologize to you for my lack > > of explanation. > > > > > Is this a real producible bug or a theory one? > > This is a real bug. > > > > > IMHO the I_FREEING flag should avoid the race. > > I supplement the explanation for this problem. > > > > clear_inode() is called by dispose_list(), and sets the inode's > > i_state to I_CLEAR. Therefore, the following conditional expression > > doesn't match for the inode: > > "if (inode->i_state & (I_FREEING|I_WILL_FREE)) continue;" > > As the result, this problem can happen. > > > > > > > > > ---------------------------------------------------------------------- > > > > > > > > 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. > > > > I applied your comment and then modified the panic flow figure. > > Please check below: > > ---------------------------------------------------------------------- > > [process A] | [process B] > > | | > > | shrink_icache_memory() | > > | | | > > | V | > > | prune_icache() | drop_pagecache() > > | mutex_lock(&iprune_mutex) | | > > | spin_lock(&inode_lock) | | > > | | | V > > | | | drop_pagecache_sb() > > | | | | > > | V | | > > | inode->i_state |= I_FREEING; | | > > | | | | > > | V | V > > | spin_unlock(&inode_lock) | spin_lock(&inode_lock) > > | | | | > > | | | | > > | V | | > > | dispose_list() | | > > | list_del() | | > > | | | | > > | V | | > > | clear_inode() | | > > | inode->i_state = I_CLEAR | | > > | | | | > > | | | V > > | | | if (inode->i_state & (I_FREEING|I_WILL_FREE)) > > | | | continue; <---- NOT MATCH > > | | | | > > | | | V > > | | | __iget() > > | | | | > > | V | V > > | spin_lock(&inode_lock) | list_move() <----- PANIC !! > > | | > > V | > > (time) > > ---------------------------------------------------------------------- > > Ah thanks for the explanation! > > How about this lightweight fix? Since s_umount is already taken in > drop_pagecache(), it's not necessary to take iprune_mutex again. Yes, the patch looks fine like this. But I believe there are more places which might miss similar check. Probably dquot.c: add_dquot_ref() needs the same check and fs-writeback.c: generic_sync_sb_inodes() also needs adding I_CLEAR to the test, doesn't it? Honza > --- mm.orig/fs/drop_caches.c > +++ mm/fs/drop_caches.c > @@ -18,7 +18,7 @@ static void drop_pagecache_sb(struct sup > > spin_lock(&inode_lock); > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > - if (inode->i_state & (I_FREEING|I_WILL_FREE)) > + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE)) > continue; > if (inode->i_mapping->nrpages == 0) > continue; -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][BUG] Lack of mutex_lock in drop_pagecache_sb() 2009-03-24 12:05 ` Jan Kara @ 2009-03-24 12:11 ` Wu Fengguang 2009-03-24 12:40 ` [PATCH] skip I_CLEAR state inodes Wu Fengguang 1 sibling, 0 replies; 28+ messages in thread From: Wu Fengguang @ 2009-03-24 12:11 UTC (permalink / raw) To: Jan Kara; +Cc: Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin On Tue, Mar 24, 2009 at 08:05:02PM +0800, Jan Kara wrote: > On Tue 24-03-09 15:44:57, Wu Fengguang wrote: > > Hi Masayoshi, > > > > On Tue, Mar 24, 2009 at 03:06:45PM +0800, Masayoshi MIZUMA wrote: > > > Hi, Fengguang > > > > > > On Mon, 23 Mar 2009 18:38:46 +0800 > > > Wu Fengguang <fengguang.wu@intel.com> wrote: > > > > > > > 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). > > > > > > > Thank you for your comment, and I apologize to you for my lack > > > of explanation. > > > > > > > Is this a real producible bug or a theory one? > > > This is a real bug. > > > > > > > IMHO the I_FREEING flag should avoid the race. > > > I supplement the explanation for this problem. > > > > > > clear_inode() is called by dispose_list(), and sets the inode's > > > i_state to I_CLEAR. Therefore, the following conditional expression > > > doesn't match for the inode: > > > "if (inode->i_state & (I_FREEING|I_WILL_FREE)) continue;" > > > As the result, this problem can happen. > > > > > > > > > > > > ---------------------------------------------------------------------- > > > > > > > > > > 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. > > > > > > I applied your comment and then modified the panic flow figure. > > > Please check below: > > > ---------------------------------------------------------------------- > > > [process A] | [process B] > > > | | > > > | shrink_icache_memory() | > > > | | | > > > | V | > > > | prune_icache() | drop_pagecache() > > > | mutex_lock(&iprune_mutex) | | > > > | spin_lock(&inode_lock) | | > > > | | | V > > > | | | drop_pagecache_sb() > > > | | | | > > > | V | | > > > | inode->i_state |= I_FREEING; | | > > > | | | | > > > | V | V > > > | spin_unlock(&inode_lock) | spin_lock(&inode_lock) > > > | | | | > > > | | | | > > > | V | | > > > | dispose_list() | | > > > | list_del() | | > > > | | | | > > > | V | | > > > | clear_inode() | | > > > | inode->i_state = I_CLEAR | | > > > | | | | > > > | | | V > > > | | | if (inode->i_state & (I_FREEING|I_WILL_FREE)) > > > | | | continue; <---- NOT MATCH > > > | | | | > > > | | | V > > > | | | __iget() > > > | | | | > > > | V | V > > > | spin_lock(&inode_lock) | list_move() <----- PANIC !! > > > | | > > > V | > > > (time) > > > ---------------------------------------------------------------------- > > > > Ah thanks for the explanation! > > > > How about this lightweight fix? Since s_umount is already taken in > > drop_pagecache(), it's not necessary to take iprune_mutex again. > Yes, the patch looks fine like this. But I believe there are more places > which might miss similar check. Probably dquot.c: add_dquot_ref() needs > the same check and fs-writeback.c: generic_sync_sb_inodes() also needs > adding I_CLEAR to the test, doesn't it? Exactly! Basically any test on I_FREEING shall be coupled with I_CLEAR, because of the I_FREEING => I_CLEAR transition _outside_ of inode_lock. Thanks, Fengguang > > --- mm.orig/fs/drop_caches.c > > +++ mm/fs/drop_caches.c > > @@ -18,7 +18,7 @@ static void drop_pagecache_sb(struct sup > > > > spin_lock(&inode_lock); > > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > > - if (inode->i_state & (I_FREEING|I_WILL_FREE)) > > + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE)) > > continue; > > if (inode->i_mapping->nrpages == 0) > > continue; > -- > Jan Kara <jack@suse.cz> > SUSE Labs, CR ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] skip I_CLEAR state inodes 2009-03-24 12:05 ` Jan Kara 2009-03-24 12:11 ` Wu Fengguang @ 2009-03-24 12:40 ` Wu Fengguang 2009-03-30 7:18 ` [PATCH][RESEND for 2.6.29-rc8-mm1] " Wu Fengguang 2009-06-01 21:38 ` [PATCH] " Eric Sandeen 1 sibling, 2 replies; 28+ messages in thread From: Wu Fengguang @ 2009-03-24 12:40 UTC (permalink / raw) To: Andrew Morton Cc: stable, LKML, Jan Kara, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and add_dquot_ref(). clear_inode() will switch inode state from I_FREEING to I_CLEAR, and do so _outside_ of inode_lock. So any I_FREEING testing is incomplete without the testing of I_CLEAR. Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and Jan Kara reminds fixing the other two cases. Thanks! Reported-by: Masayoshi MIZUMA <m.mizuma@jp.fujitsu.com> Reviewed-by: Jan Kara <jack@suse.cz> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> --- fs/dquot.c | 2 +- fs/drop_caches.c | 2 +- fs/fs-writeback.c | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) --- mm.orig/fs/drop_caches.c +++ mm/fs/drop_caches.c @@ -18,7 +18,7 @@ static void drop_pagecache_sb(struct sup spin_lock(&inode_lock); list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { - if (inode->i_state & (I_FREEING|I_WILL_FREE)) + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE)) continue; if (inode->i_mapping->nrpages == 0) continue; --- mm.orig/fs/fs-writeback.c +++ mm/fs/fs-writeback.c @@ -538,7 +538,8 @@ void generic_sync_sb_inodes(struct super list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { struct address_space *mapping; - if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) + if (inode->i_state & + (I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW)) continue; mapping = inode->i_mapping; if (mapping->nrpages == 0) --- mm.orig/fs/dquot.c +++ mm/fs/dquot.c @@ -793,7 +793,7 @@ static void add_dquot_ref(struct super_b continue; if (!dqinit_needed(inode, type)) continue; - if (inode->i_state & (I_FREEING|I_WILL_FREE)) + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE)) continue; __iget(inode); ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH][RESEND for 2.6.29-rc8-mm1] skip I_CLEAR state inodes 2009-03-24 12:40 ` [PATCH] skip I_CLEAR state inodes Wu Fengguang @ 2009-03-30 7:18 ` Wu Fengguang 2009-03-31 23:43 ` Andrew Morton 2009-06-01 21:38 ` [PATCH] " Eric Sandeen 1 sibling, 1 reply; 28+ messages in thread From: Wu Fengguang @ 2009-03-30 7:18 UTC (permalink / raw) To: Andrew Morton Cc: stable, LKML, Jan Kara, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin clear_inode() will switch inode state from I_FREEING to I_CLEAR, and do so _outside_ of inode_lock. So any I_FREEING testing is incomplete without a coupled testing of I_CLEAR. So add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and add_dquot_ref(). Masayoshi MIZUMA discovered the bug in drop_pagecache_sb() and Jan Kara reminds fixing the other two cases. Reported-by: Masayoshi MIZUMA <m.mizuma@jp.fujitsu.com> Reviewed-by: Jan Kara <jack@suse.cz> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> --- fs/drop_caches.c | 2 +- fs/fs-writeback.c | 3 ++- fs/quota/dquot.c | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) --- mm.orig/fs/drop_caches.c +++ mm/fs/drop_caches.c @@ -18,7 +18,7 @@ static void drop_pagecache_sb(struct sup spin_lock(&inode_lock); list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { - if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW)) continue; if (inode->i_mapping->nrpages == 0) continue; --- mm.orig/fs/fs-writeback.c +++ mm/fs/fs-writeback.c @@ -538,7 +538,8 @@ void generic_sync_sb_inodes(struct super list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { struct address_space *mapping; - if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) + if (inode->i_state & + (I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW)) continue; mapping = inode->i_mapping; if (mapping->nrpages == 0) --- mm.orig/fs/quota/dquot.c +++ mm/fs/quota/dquot.c @@ -823,7 +823,7 @@ static void add_dquot_ref(struct super_b spin_lock(&inode_lock); list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { - if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW)) continue; if (!atomic_read(&inode->i_writecount)) continue; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RESEND for 2.6.29-rc8-mm1] skip I_CLEAR state inodes 2009-03-30 7:18 ` [PATCH][RESEND for 2.6.29-rc8-mm1] " Wu Fengguang @ 2009-03-31 23:43 ` Andrew Morton 2009-04-01 0:53 ` Wu Fengguang 0 siblings, 1 reply; 28+ messages in thread From: Andrew Morton @ 2009-03-31 23:43 UTC (permalink / raw) To: Wu Fengguang Cc: stable, linux-kernel, jack, m.mizuma, linux-fsdevel, viro, npiggin On Mon, 30 Mar 2009 15:18:24 +0800 Wu Fengguang <fengguang.wu@intel.com> wrote: > clear_inode() will switch inode state from I_FREEING to I_CLEAR, > and do so _outside_ of inode_lock. So any I_FREEING testing is > incomplete without a coupled testing of I_CLEAR. > > So add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > add_dquot_ref(). > > Masayoshi MIZUMA discovered the bug in drop_pagecache_sb() and Jan Kara > reminds fixing the other two cases. ok... But what is the user-visible consequence of this? You cc'ed stable@kernel.org so I assume it's serious. People will want to know what problem we're fixing! > > --- mm.orig/fs/drop_caches.c > +++ mm/fs/drop_caches.c > @@ -18,7 +18,7 @@ static void drop_pagecache_sb(struct sup > > spin_lock(&inode_lock); > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > - if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) > + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW)) > continue; > if (inode->i_mapping->nrpages == 0) > continue; > --- mm.orig/fs/fs-writeback.c > +++ mm/fs/fs-writeback.c > @@ -538,7 +538,8 @@ void generic_sync_sb_inodes(struct super > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > struct address_space *mapping; > > - if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) > + if (inode->i_state & > + (I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW)) > continue; > mapping = inode->i_mapping; > if (mapping->nrpages == 0) > --- mm.orig/fs/quota/dquot.c > +++ mm/fs/quota/dquot.c > @@ -823,7 +823,7 @@ static void add_dquot_ref(struct super_b > > spin_lock(&inode_lock); > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > - if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) > + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW)) > continue; > if (!atomic_read(&inode->i_writecount)) > continue; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RESEND for 2.6.29-rc8-mm1] skip I_CLEAR state inodes 2009-03-31 23:43 ` Andrew Morton @ 2009-04-01 0:53 ` Wu Fengguang 0 siblings, 0 replies; 28+ messages in thread From: Wu Fengguang @ 2009-04-01 0:53 UTC (permalink / raw) To: Andrew Morton Cc: stable, linux-kernel, jack, m.mizuma, linux-fsdevel, viro, npiggin On Wed, Apr 01, 2009 at 07:43:32AM +0800, Andrew Morton wrote: > On Mon, 30 Mar 2009 15:18:24 +0800 > Wu Fengguang <fengguang.wu@intel.com> wrote: > > > clear_inode() will switch inode state from I_FREEING to I_CLEAR, > > and do so _outside_ of inode_lock. So any I_FREEING testing is > > incomplete without a coupled testing of I_CLEAR. > > > > So add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > > add_dquot_ref(). > > > > Masayoshi MIZUMA discovered the bug in drop_pagecache_sb() and Jan Kara > > reminds fixing the other two cases. > > ok... > > But what is the user-visible consequence of this? You cc'ed > stable@kernel.org so I assume it's serious. People will want to know > what problem we're fixing! Sorry, the changelog could be expanded with the following paragraph: Fix real kernel panics. Masayoshi MIZUMA has a nice panic flow: ---------------------------------------------------------------------- [process A] | [process B] | | | prune_icache() | drop_pagecache() | spin_lock(&inode_lock) | drop_pagecache_sb() | inode->i_state |= I_FREEING; | | | spin_unlock(&inode_lock) | V | | | spin_lock(&inode_lock) | V | | | dispose_list() | | | list_del() | | | clear_inode() | | | inode->i_state = I_CLEAR | | | | | V | | | if (inode->i_state & (I_FREEING|I_WILL_FREE)) | | | continue; <==== NOT MATCH | | | | | | (DANGER from here on! Accessing disposing inode!) | | | | | | __iget() | | | list_move() <===== PANIC on poisoned list !! V V | (time) ---------------------------------------------------------------------- Thanks, Fengguang > > > > --- mm.orig/fs/drop_caches.c > > +++ mm/fs/drop_caches.c > > @@ -18,7 +18,7 @@ static void drop_pagecache_sb(struct sup > > > > spin_lock(&inode_lock); > > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > > - if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) > > + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW)) > > continue; > > if (inode->i_mapping->nrpages == 0) > > continue; > > --- mm.orig/fs/fs-writeback.c > > +++ mm/fs/fs-writeback.c > > @@ -538,7 +538,8 @@ void generic_sync_sb_inodes(struct super > > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > > struct address_space *mapping; > > > > - if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) > > + if (inode->i_state & > > + (I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW)) > > continue; > > mapping = inode->i_mapping; > > if (mapping->nrpages == 0) > > --- mm.orig/fs/quota/dquot.c > > +++ mm/fs/quota/dquot.c > > @@ -823,7 +823,7 @@ static void add_dquot_ref(struct super_b > > > > spin_lock(&inode_lock); > > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > > - if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) > > + if (inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW)) > > continue; > > if (!atomic_read(&inode->i_writecount)) > > continue; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] skip I_CLEAR state inodes 2009-03-24 12:40 ` [PATCH] skip I_CLEAR state inodes Wu Fengguang 2009-03-30 7:18 ` [PATCH][RESEND for 2.6.29-rc8-mm1] " Wu Fengguang @ 2009-06-01 21:38 ` Eric Sandeen 2009-06-02 8:55 ` Wu Fengguang 1 sibling, 1 reply; 28+ messages in thread From: Eric Sandeen @ 2009-06-01 21:38 UTC (permalink / raw) To: Wu Fengguang Cc: Andrew Morton, LKML, Jan Kara, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin Wu Fengguang wrote: > Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > add_dquot_ref(). > > clear_inode() will switch inode state from I_FREEING to I_CLEAR, > and do so _outside_ of inode_lock. So any I_FREEING testing is > incomplete without the testing of I_CLEAR. > > Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and > Jan Kara reminds fixing the other two cases. Thanks! Is there a reason it's not done for __sync_single_inode as well? Jeff Layton asked the question and I'm following it up :) __sync_single_inode currently only tests I_FREEING, but I think we are safe because __sync_single_inode sets I_SYNC, and clear_inode waits for I_SYNC to be cleared before it changes I_STATE. On the other hand, testing I_CLEAR here probably would be safe anyway, and it'd be bonus points for consistency? Same basic question for generic_sync_sb_inodes, which has a BUG_ON(inode->i_state & I_FREEING), seems like this could check I_CLWAR as well? Thanks, -Eric ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] skip I_CLEAR state inodes 2009-06-01 21:38 ` [PATCH] " Eric Sandeen @ 2009-06-02 8:55 ` Wu Fengguang 2009-06-02 10:27 ` Jeff Layton 2009-06-02 11:37 ` Jan Kara 0 siblings, 2 replies; 28+ messages in thread From: Wu Fengguang @ 2009-06-02 8:55 UTC (permalink / raw) To: Eric Sandeen Cc: Andrew Morton, LKML, Jan Kara, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin On Tue, Jun 02, 2009 at 05:38:35AM +0800, Eric Sandeen wrote: > Wu Fengguang wrote: > > Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > > add_dquot_ref(). > > > > clear_inode() will switch inode state from I_FREEING to I_CLEAR, > > and do so _outside_ of inode_lock. So any I_FREEING testing is > > incomplete without the testing of I_CLEAR. > > > > Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and > > Jan Kara reminds fixing the other two cases. Thanks! > > Is there a reason it's not done for __sync_single_inode as well? It missed the glance because it don't have an obvious '|' in the line ;) > Jeff Layton asked the question and I'm following it up :) > > __sync_single_inode currently only tests I_FREEING, but I think we are > safe because __sync_single_inode sets I_SYNC, and clear_inode waits for > I_SYNC to be cleared before it changes I_STATE. But I_SYNC is removed just before the I_FREEING test, so we still have a small race window? > On the other hand, testing I_CLEAR here probably would be safe anyway, > and it'd be bonus points for consistency? So let's add the I_CLEAR test? > Same basic question for generic_sync_sb_inodes, which has a > BUG_ON(inode->i_state & I_FREEING), seems like this could check I_CLWAR > as well? Yes, we can add I_CLEAR here to catch more error condition. Thanks, Fengguang --- skip I_CLEAR state inodes in writeback routines The I_FREEING test in __sync_single_inode() is racy because clear_inode() can set i_state to I_CLEAR between the clear of I_SYNC and the test of I_FREEING. Also extend the coverage of BUG_ON(I_FREEING) to I_CLEAR. Reported-by: Jeff Layton <jlayton@redhat.com> Reported-by: Eric Sandeen <sandeen@sandeen.net> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> --- fs/fs-writeback.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- linux.orig/fs/fs-writeback.c +++ linux/fs/fs-writeback.c @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, spin_lock(&inode_lock); WARN_ON(inode->i_state & I_NEW); inode->i_state &= ~I_SYNC; - if (!(inode->i_state & I_FREEING)) { + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { if (!(inode->i_state & I_DIRTY) && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { /* @@ -518,7 +518,7 @@ void generic_sync_sb_inodes(struct super if (current_is_pdflush() && !writeback_acquire(bdi)) break; - BUG_ON(inode->i_state & I_FREEING); + BUG_ON(inode->i_state & (I_FREEING | I_CLEAR)); __iget(inode); pages_skipped = wbc->pages_skipped; __writeback_single_inode(inode, wbc); ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] skip I_CLEAR state inodes 2009-06-02 8:55 ` Wu Fengguang @ 2009-06-02 10:27 ` Jeff Layton 2009-06-02 11:37 ` Jan Kara 1 sibling, 0 replies; 28+ messages in thread From: Jeff Layton @ 2009-06-02 10:27 UTC (permalink / raw) To: Wu Fengguang Cc: Eric Sandeen, Andrew Morton, LKML, Jan Kara, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin On Tue, 2 Jun 2009 16:55:23 +0800 Wu Fengguang <fengguang.wu@intel.com> wrote: > On Tue, Jun 02, 2009 at 05:38:35AM +0800, Eric Sandeen wrote: > > Wu Fengguang wrote: > > > Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > > > add_dquot_ref(). > > > > > > clear_inode() will switch inode state from I_FREEING to I_CLEAR, > > > and do so _outside_ of inode_lock. So any I_FREEING testing is > > > incomplete without the testing of I_CLEAR. > > > > > > Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and > > > Jan Kara reminds fixing the other two cases. Thanks! > > > > Is there a reason it's not done for __sync_single_inode as well? > > It missed the glance because it don't have an obvious '|' in the line ;) > > > Jeff Layton asked the question and I'm following it up :) > > > > __sync_single_inode currently only tests I_FREEING, but I think we are > > safe because __sync_single_inode sets I_SYNC, and clear_inode waits for > > I_SYNC to be cleared before it changes I_STATE. > > But I_SYNC is removed just before the I_FREEING test, so we still have > a small race window? > Yes, I think so. __sync_single_inode clears I_SYNC but doesn't wake up the wait queue until the end of the function. So I think it's possible (though unlikely) that another thread can race in and change the state to I_CLEAR before the I_FREEING check. > > On the other hand, testing I_CLEAR here probably would be safe anyway, > > and it'd be bonus points for consistency? > > So let's add the I_CLEAR test? > > > Same basic question for generic_sync_sb_inodes, which has a > > BUG_ON(inode->i_state & I_FREEING), seems like this could check I_CLWAR > > as well? > > Yes, we can add I_CLEAR here to catch more error condition. > > Thanks, > Fengguang > > --- > skip I_CLEAR state inodes in writeback routines > > The I_FREEING test in __sync_single_inode() is racy because > clear_inode() can set i_state to I_CLEAR between the clear of I_SYNC > and the test of I_FREEING. > > Also extend the coverage of BUG_ON(I_FREEING) to I_CLEAR. > > Reported-by: Jeff Layton <jlayton@redhat.com> > Reported-by: Eric Sandeen <sandeen@sandeen.net> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > --- > fs/fs-writeback.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > --- linux.orig/fs/fs-writeback.c > +++ linux/fs/fs-writeback.c > @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, > spin_lock(&inode_lock); > WARN_ON(inode->i_state & I_NEW); > inode->i_state &= ~I_SYNC; > - if (!(inode->i_state & I_FREEING)) { > + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { > if (!(inode->i_state & I_DIRTY) && > mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > /* > @@ -518,7 +518,7 @@ void generic_sync_sb_inodes(struct super > if (current_is_pdflush() && !writeback_acquire(bdi)) > break; > > - BUG_ON(inode->i_state & I_FREEING); > + BUG_ON(inode->i_state & (I_FREEING | I_CLEAR)); > __iget(inode); > pages_skipped = wbc->pages_skipped; > __writeback_single_inode(inode, wbc); Acked-by: Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] skip I_CLEAR state inodes 2009-06-02 8:55 ` Wu Fengguang 2009-06-02 10:27 ` Jeff Layton @ 2009-06-02 11:37 ` Jan Kara 2009-06-02 21:48 ` Eric Sandeen ` (2 more replies) 1 sibling, 3 replies; 28+ messages in thread From: Jan Kara @ 2009-06-02 11:37 UTC (permalink / raw) To: Wu Fengguang Cc: Eric Sandeen, Andrew Morton, LKML, Jan Kara, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin On Tue 02-06-09 16:55:23, Wu Fengguang wrote: > On Tue, Jun 02, 2009 at 05:38:35AM +0800, Eric Sandeen wrote: > > Wu Fengguang wrote: > > > Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > > > add_dquot_ref(). > > > > > > clear_inode() will switch inode state from I_FREEING to I_CLEAR, > > > and do so _outside_ of inode_lock. So any I_FREEING testing is > > > incomplete without the testing of I_CLEAR. > > > > > > Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and > > > Jan Kara reminds fixing the other two cases. Thanks! > > > > Is there a reason it's not done for __sync_single_inode as well? > > It missed the glance because it don't have an obvious '|' in the line ;) > > > Jeff Layton asked the question and I'm following it up :) > > > > __sync_single_inode currently only tests I_FREEING, but I think we are > > safe because __sync_single_inode sets I_SYNC, and clear_inode waits for > > I_SYNC to be cleared before it changes I_STATE. > > But I_SYNC is removed just before the I_FREEING test, so we still have > a small race window? > > > On the other hand, testing I_CLEAR here probably would be safe anyway, > > and it'd be bonus points for consistency? > > So let's add the I_CLEAR test? > > > Same basic question for generic_sync_sb_inodes, which has a > > BUG_ON(inode->i_state & I_FREEING), seems like this could check I_CLWAR > > as well? > > Yes, we can add I_CLEAR here to catch more error condition. > > Thanks, > Fengguang > > --- > skip I_CLEAR state inodes in writeback routines > > The I_FREEING test in __sync_single_inode() is racy because > clear_inode() can set i_state to I_CLEAR between the clear of I_SYNC > and the test of I_FREEING. > > Also extend the coverage of BUG_ON(I_FREEING) to I_CLEAR. > > Reported-by: Jeff Layton <jlayton@redhat.com> > Reported-by: Eric Sandeen <sandeen@sandeen.net> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > --- > fs/fs-writeback.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > --- linux.orig/fs/fs-writeback.c > +++ linux/fs/fs-writeback.c > @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, > spin_lock(&inode_lock); > WARN_ON(inode->i_state & I_NEW); > inode->i_state &= ~I_SYNC; > - if (!(inode->i_state & I_FREEING)) { > + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { > if (!(inode->i_state & I_DIRTY) && > mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { Is the whole if needed? I had an impression that everyone calling __sync_single_inode() should better take care it does not race with inode freeing... So WARN_ON would be more appropriate IMHO. > /* > @@ -518,7 +518,7 @@ void generic_sync_sb_inodes(struct super > if (current_is_pdflush() && !writeback_acquire(bdi)) > break; > > - BUG_ON(inode->i_state & I_FREEING); > + BUG_ON(inode->i_state & (I_FREEING | I_CLEAR)); > __iget(inode); > pages_skipped = wbc->pages_skipped; > __writeback_single_inode(inode, wbc); Looking at this code, it looks a bit suspicious. What prevents this s_io list scan to race with inode freeing? In particular generic_forget_inode() can drop inode_lock to write the inode and in the mean time generic_sync_sb_inodes() can come, get a reference to the inode and start it's writeback... Subsequent iput() would then call generic_forget_inode() on the inode again. So shouldn't we skip I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW inodes in this scan like we do for later in the function for another scan? Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] skip I_CLEAR state inodes 2009-06-02 11:37 ` Jan Kara @ 2009-06-02 21:48 ` Eric Sandeen 2009-06-03 10:45 ` Jeff Layton 2009-06-03 13:32 ` Wu Fengguang 2009-06-03 14:10 ` Wu Fengguang 2 siblings, 1 reply; 28+ messages in thread From: Eric Sandeen @ 2009-06-02 21:48 UTC (permalink / raw) To: Jan Kara Cc: Wu Fengguang, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin Jan Kara wrote: > On Tue 02-06-09 16:55:23, Wu Fengguang wrote: >> On Tue, Jun 02, 2009 at 05:38:35AM +0800, Eric Sandeen wrote: >>> Wu Fengguang wrote: >>>> Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and >>>> add_dquot_ref(). >>>> >>>> clear_inode() will switch inode state from I_FREEING to I_CLEAR, >>>> and do so _outside_ of inode_lock. So any I_FREEING testing is >>>> incomplete without the testing of I_CLEAR. >>>> >>>> Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and >>>> Jan Kara reminds fixing the other two cases. Thanks! >>> Is there a reason it's not done for __sync_single_inode as well? >> It missed the glance because it don't have an obvious '|' in the line ;) >> >>> Jeff Layton asked the question and I'm following it up :) >>> >>> __sync_single_inode currently only tests I_FREEING, but I think we are >>> safe because __sync_single_inode sets I_SYNC, and clear_inode waits for >>> I_SYNC to be cleared before it changes I_STATE. >> But I_SYNC is removed just before the I_FREEING test, so we still have >> a small race window? yep that's right. inode->i_state &= ~I_SYNC; >>> clear_inode->inode_sync_wait here and find it clear <<< if (!(inode->i_state & I_FREEING)) { ... >> --- linux.orig/fs/fs-writeback.c >> +++ linux/fs/fs-writeback.c >> @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, >> spin_lock(&inode_lock); >> WARN_ON(inode->i_state & I_NEW); >> inode->i_state &= ~I_SYNC; >> - if (!(inode->i_state & I_FREEING)) { >> + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { >> if (!(inode->i_state & I_DIRTY) && >> mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > Is the whole if needed? I had an impression that everyone calling > __sync_single_inode() should better take care it does not race with inode > freeing... So WARN_ON would be more appropriate IMHO. Maybe both then (both a WARN on and then the test (defensive here, I guess)) because if we continue we may wander into a poisoned list pointer and explode, right? -Eric ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] skip I_CLEAR state inodes 2009-06-02 21:48 ` Eric Sandeen @ 2009-06-03 10:45 ` Jeff Layton 0 siblings, 0 replies; 28+ messages in thread From: Jeff Layton @ 2009-06-03 10:45 UTC (permalink / raw) To: Eric Sandeen Cc: Jan Kara, Wu Fengguang, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin On Tue, 02 Jun 2009 16:48:57 -0500 Eric Sandeen <sandeen@sandeen.net> wrote: > Jan Kara wrote: > > On Tue 02-06-09 16:55:23, Wu Fengguang wrote: > >> On Tue, Jun 02, 2009 at 05:38:35AM +0800, Eric Sandeen wrote: > >>> Wu Fengguang wrote: > >>>> Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > >>>> add_dquot_ref(). > >>>> > >>>> clear_inode() will switch inode state from I_FREEING to I_CLEAR, > >>>> and do so _outside_ of inode_lock. So any I_FREEING testing is > >>>> incomplete without the testing of I_CLEAR. > >>>> > >>>> Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and > >>>> Jan Kara reminds fixing the other two cases. Thanks! > >>> Is there a reason it's not done for __sync_single_inode as well? > >> It missed the glance because it don't have an obvious '|' in the line ;) > >> > >>> Jeff Layton asked the question and I'm following it up :) > >>> > >>> __sync_single_inode currently only tests I_FREEING, but I think we are > >>> safe because __sync_single_inode sets I_SYNC, and clear_inode waits for > >>> I_SYNC to be cleared before it changes I_STATE. > >> But I_SYNC is removed just before the I_FREEING test, so we still have > >> a small race window? > > yep that's right. > > inode->i_state &= ~I_SYNC; > >>> clear_inode->inode_sync_wait here and find it clear <<< > if (!(inode->i_state & I_FREEING)) { > > ... > > >> --- linux.orig/fs/fs-writeback.c > >> +++ linux/fs/fs-writeback.c > >> @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, > >> spin_lock(&inode_lock); > >> WARN_ON(inode->i_state & I_NEW); > >> inode->i_state &= ~I_SYNC; > >> - if (!(inode->i_state & I_FREEING)) { > >> + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { > >> if (!(inode->i_state & I_DIRTY) && > >> mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > > > Is the whole if needed? I had an impression that everyone calling > > __sync_single_inode() should better take care it does not race with inode > > freeing... So WARN_ON would be more appropriate IMHO. > > Maybe both then (both a WARN on and then the test (defensive here, I > guess)) because if we continue we may wander into a poisoned list > pointer and explode, right? > Right. I think removing this check would be roughly equivalent to adding a BUG_ON. It just wouldn't reliably pop since it would depend on the timing of the race. Keeping the check and adding a WARN seems like a reasonable thing to do. Maybe after a few releases if no one has seen the WARN fire we can look at removing the check (or maybe turn it into a BUG)? -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] skip I_CLEAR state inodes 2009-06-02 11:37 ` Jan Kara 2009-06-02 21:48 ` Eric Sandeen @ 2009-06-03 13:32 ` Wu Fengguang 2009-06-03 14:00 ` Jan Kara 2009-06-03 14:10 ` Wu Fengguang 2 siblings, 1 reply; 28+ messages in thread From: Wu Fengguang @ 2009-06-03 13:32 UTC (permalink / raw) To: Jan Kara Cc: Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin [reply the easy part first] On Tue, Jun 02, 2009 at 07:37:36PM +0800, Jan Kara wrote: > On Tue 02-06-09 16:55:23, Wu Fengguang wrote: > > On Tue, Jun 02, 2009 at 05:38:35AM +0800, Eric Sandeen wrote: > > > Wu Fengguang wrote: > > > > Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > > > > add_dquot_ref(). > > > > > > > > clear_inode() will switch inode state from I_FREEING to I_CLEAR, > > > > and do so _outside_ of inode_lock. So any I_FREEING testing is > > > > incomplete without the testing of I_CLEAR. > > > > > > > > Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and > > > > Jan Kara reminds fixing the other two cases. Thanks! > > > > > > Is there a reason it's not done for __sync_single_inode as well? > > > > It missed the glance because it don't have an obvious '|' in the line ;) > > > > > Jeff Layton asked the question and I'm following it up :) > > > > > > __sync_single_inode currently only tests I_FREEING, but I think we are > > > safe because __sync_single_inode sets I_SYNC, and clear_inode waits for > > > I_SYNC to be cleared before it changes I_STATE. > > > > But I_SYNC is removed just before the I_FREEING test, so we still have > > a small race window? > > > > > On the other hand, testing I_CLEAR here probably would be safe anyway, > > > and it'd be bonus points for consistency? > > > > So let's add the I_CLEAR test? > > > > > Same basic question for generic_sync_sb_inodes, which has a > > > BUG_ON(inode->i_state & I_FREEING), seems like this could check I_CLWAR > > > as well? > > > > Yes, we can add I_CLEAR here to catch more error condition. > > > > Thanks, > > Fengguang > > > > --- > > skip I_CLEAR state inodes in writeback routines > > > > The I_FREEING test in __sync_single_inode() is racy because > > clear_inode() can set i_state to I_CLEAR between the clear of I_SYNC > > and the test of I_FREEING. > > > > Also extend the coverage of BUG_ON(I_FREEING) to I_CLEAR. > > > > Reported-by: Jeff Layton <jlayton@redhat.com> > > Reported-by: Eric Sandeen <sandeen@sandeen.net> > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > > --- > > fs/fs-writeback.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > --- linux.orig/fs/fs-writeback.c > > +++ linux/fs/fs-writeback.c > > @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, > > spin_lock(&inode_lock); > > WARN_ON(inode->i_state & I_NEW); > > inode->i_state &= ~I_SYNC; > > - if (!(inode->i_state & I_FREEING)) { > > + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { > > if (!(inode->i_state & I_DIRTY) && > > mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > Is the whole if needed? I had an impression that everyone calling > __sync_single_inode() should better take care it does not race with inode > freeing... So WARN_ON would be more appropriate IMHO. The caller doesn't matter here, because we temporarily dropped inode_lock in __sync_single_inode() ? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] skip I_CLEAR state inodes 2009-06-03 13:32 ` Wu Fengguang @ 2009-06-03 14:00 ` Jan Kara 0 siblings, 0 replies; 28+ messages in thread From: Jan Kara @ 2009-06-03 14:00 UTC (permalink / raw) To: Wu Fengguang Cc: Jan Kara, Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin On Wed 03-06-09 21:32:02, Wu Fengguang wrote: > [reply the easy part first] > On Tue, Jun 02, 2009 at 07:37:36PM +0800, Jan Kara wrote: > > On Tue 02-06-09 16:55:23, Wu Fengguang wrote: > > > On Tue, Jun 02, 2009 at 05:38:35AM +0800, Eric Sandeen wrote: > > > > Wu Fengguang wrote: > > > > > Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > > > > > add_dquot_ref(). > > > > > > > > > > clear_inode() will switch inode state from I_FREEING to I_CLEAR, > > > > > and do so _outside_ of inode_lock. So any I_FREEING testing is > > > > > incomplete without the testing of I_CLEAR. > > > > > > > > > > Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and > > > > > Jan Kara reminds fixing the other two cases. Thanks! > > > > > > > > Is there a reason it's not done for __sync_single_inode as well? > > > > > > It missed the glance because it don't have an obvious '|' in the line ;) > > > > > > > Jeff Layton asked the question and I'm following it up :) > > > > > > > > __sync_single_inode currently only tests I_FREEING, but I think we are > > > > safe because __sync_single_inode sets I_SYNC, and clear_inode waits for > > > > I_SYNC to be cleared before it changes I_STATE. > > > > > > But I_SYNC is removed just before the I_FREEING test, so we still have > > > a small race window? > > > > > > > On the other hand, testing I_CLEAR here probably would be safe anyway, > > > > and it'd be bonus points for consistency? > > > > > > So let's add the I_CLEAR test? > > > > > > > Same basic question for generic_sync_sb_inodes, which has a > > > > BUG_ON(inode->i_state & I_FREEING), seems like this could check I_CLWAR > > > > as well? > > > > > > Yes, we can add I_CLEAR here to catch more error condition. > > > > > > Thanks, > > > Fengguang > > > > > > --- > > > skip I_CLEAR state inodes in writeback routines > > > > > > The I_FREEING test in __sync_single_inode() is racy because > > > clear_inode() can set i_state to I_CLEAR between the clear of I_SYNC > > > and the test of I_FREEING. > > > > > > Also extend the coverage of BUG_ON(I_FREEING) to I_CLEAR. > > > > > > Reported-by: Jeff Layton <jlayton@redhat.com> > > > Reported-by: Eric Sandeen <sandeen@sandeen.net> > > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > > > --- > > > fs/fs-writeback.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > --- linux.orig/fs/fs-writeback.c > > > +++ linux/fs/fs-writeback.c > > > @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, > > > spin_lock(&inode_lock); > > > WARN_ON(inode->i_state & I_NEW); > > > inode->i_state &= ~I_SYNC; > > > - if (!(inode->i_state & I_FREEING)) { > > > + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { > > > if (!(inode->i_state & I_DIRTY) && > > > mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > > Is the whole if needed? I had an impression that everyone calling > > __sync_single_inode() should better take care it does not race with inode > > freeing... So WARN_ON would be more appropriate IMHO. > > The caller doesn't matter here, because we temporarily dropped inode_lock > in __sync_single_inode() ? But the problem is basically what I described in the part you cut from the email - if the caller of __sync_single_inode() does not have a reference to the inode or some other means of protecting inode from being released, then we have a problem because e.g. __writeback_single_inode() can sleep and before I_SYNC is set, all the clear_inode() can finish and we end up calling do_writepages() on invalidated mapping. So in my opinion everybody calling __writeback_single_inode() and thus __sync_single_inode() should do an equivalent of igrab() or be sure inode cannot be freed e.g. because we have a file open. Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] skip I_CLEAR state inodes 2009-06-02 11:37 ` Jan Kara 2009-06-02 21:48 ` Eric Sandeen 2009-06-03 13:32 ` Wu Fengguang @ 2009-06-03 14:10 ` Wu Fengguang 2009-06-03 14:16 ` Jan Kara 2 siblings, 1 reply; 28+ messages in thread From: Wu Fengguang @ 2009-06-03 14:10 UTC (permalink / raw) To: Jan Kara Cc: Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin, Jeff Layton On Tue, Jun 02, 2009 at 07:37:36PM +0800, Jan Kara wrote: > On Tue 02-06-09 16:55:23, Wu Fengguang wrote: > > On Tue, Jun 02, 2009 at 05:38:35AM +0800, Eric Sandeen wrote: > > > Wu Fengguang wrote: > > > > Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > > > > add_dquot_ref(). > > > > > > > > clear_inode() will switch inode state from I_FREEING to I_CLEAR, > > > > and do so _outside_ of inode_lock. So any I_FREEING testing is > > > > incomplete without the testing of I_CLEAR. > > > > > > > > Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and > > > > Jan Kara reminds fixing the other two cases. Thanks! > > > > > > Is there a reason it's not done for __sync_single_inode as well? > > > > It missed the glance because it don't have an obvious '|' in the line ;) > > > > > Jeff Layton asked the question and I'm following it up :) > > > > > > __sync_single_inode currently only tests I_FREEING, but I think we are > > > safe because __sync_single_inode sets I_SYNC, and clear_inode waits for > > > I_SYNC to be cleared before it changes I_STATE. > > > > But I_SYNC is removed just before the I_FREEING test, so we still have > > a small race window? > > > > > On the other hand, testing I_CLEAR here probably would be safe anyway, > > > and it'd be bonus points for consistency? > > > > So let's add the I_CLEAR test? > > > > > Same basic question for generic_sync_sb_inodes, which has a > > > BUG_ON(inode->i_state & I_FREEING), seems like this could check I_CLWAR > > > as well? > > > > Yes, we can add I_CLEAR here to catch more error condition. > > > > Thanks, > > Fengguang > > > > --- > > skip I_CLEAR state inodes in writeback routines > > > > The I_FREEING test in __sync_single_inode() is racy because > > clear_inode() can set i_state to I_CLEAR between the clear of I_SYNC > > and the test of I_FREEING. > > > > Also extend the coverage of BUG_ON(I_FREEING) to I_CLEAR. > > > > Reported-by: Jeff Layton <jlayton@redhat.com> > > Reported-by: Eric Sandeen <sandeen@sandeen.net> > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > > --- > > fs/fs-writeback.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > --- linux.orig/fs/fs-writeback.c > > +++ linux/fs/fs-writeback.c > > @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, > > spin_lock(&inode_lock); > > WARN_ON(inode->i_state & I_NEW); > > inode->i_state &= ~I_SYNC; > > - if (!(inode->i_state & I_FREEING)) { > > + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { > > if (!(inode->i_state & I_DIRTY) && > > mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > Is the whole if needed? I had an impression that everyone calling > __sync_single_inode() should better take care it does not race with inode > freeing... So WARN_ON would be more appropriate IMHO. > > > /* > > @@ -518,7 +518,7 @@ void generic_sync_sb_inodes(struct super > > if (current_is_pdflush() && !writeback_acquire(bdi)) > > break; > > > > - BUG_ON(inode->i_state & I_FREEING); > > + BUG_ON(inode->i_state & (I_FREEING | I_CLEAR)); > > __iget(inode); > > pages_skipped = wbc->pages_skipped; > > __writeback_single_inode(inode, wbc); > Looking at this code, it looks a bit suspicious. What prevents this s_io > list scan to race with inode freeing? In particular generic_forget_inode() Good catch. > can drop inode_lock to write the inode and in the mean time > generic_sync_sb_inodes() can come, get a reference to the inode and start > it's writeback... Subsequent iput() would then call generic_forget_inode() Another possibility: generic_forget_inode inode->i_state |= I_WILL_FREE; spin_unlock(&inode_lock); generic_sync_sb_inodes() spin_lock(&inode_lock); __iget(inode); __writeback_single_inode // see non zero i_count WARN_ON(inode->i_state & I_WILL_FREE); I'm wondering why didn't we saw reports on the last WARN_ON()? Did we missed something? > on the inode again. So shouldn't we skip I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW > inodes in this scan like we do for later in the function for another scan? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] skip I_CLEAR state inodes 2009-06-03 14:10 ` Wu Fengguang @ 2009-06-03 14:16 ` Jan Kara 2009-06-03 14:47 ` Wu Fengguang 0 siblings, 1 reply; 28+ messages in thread From: Jan Kara @ 2009-06-03 14:16 UTC (permalink / raw) To: Wu Fengguang Cc: Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin, Jeff Layton On Wed 03-06-09 22:10:21, Wu Fengguang wrote: > On Tue, Jun 02, 2009 at 07:37:36PM +0800, Jan Kara wrote: > > On Tue 02-06-09 16:55:23, Wu Fengguang wrote: > > > On Tue, Jun 02, 2009 at 05:38:35AM +0800, Eric Sandeen wrote: > > > > Wu Fengguang wrote: > > > > > Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > > > > > add_dquot_ref(). > > > > > > > > > > clear_inode() will switch inode state from I_FREEING to I_CLEAR, > > > > > and do so _outside_ of inode_lock. So any I_FREEING testing is > > > > > incomplete without the testing of I_CLEAR. > > > > > > > > > > Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and > > > > > Jan Kara reminds fixing the other two cases. Thanks! > > > > > > > > Is there a reason it's not done for __sync_single_inode as well? > > > > > > It missed the glance because it don't have an obvious '|' in the line ;) > > > > > > > Jeff Layton asked the question and I'm following it up :) > > > > > > > > __sync_single_inode currently only tests I_FREEING, but I think we are > > > > safe because __sync_single_inode sets I_SYNC, and clear_inode waits for > > > > I_SYNC to be cleared before it changes I_STATE. > > > > > > But I_SYNC is removed just before the I_FREEING test, so we still have > > > a small race window? > > > > > > > On the other hand, testing I_CLEAR here probably would be safe anyway, > > > > and it'd be bonus points for consistency? > > > > > > So let's add the I_CLEAR test? > > > > > > > Same basic question for generic_sync_sb_inodes, which has a > > > > BUG_ON(inode->i_state & I_FREEING), seems like this could check I_CLWAR > > > > as well? > > > > > > Yes, we can add I_CLEAR here to catch more error condition. > > > > > > Thanks, > > > Fengguang > > > > > > --- > > > skip I_CLEAR state inodes in writeback routines > > > > > > The I_FREEING test in __sync_single_inode() is racy because > > > clear_inode() can set i_state to I_CLEAR between the clear of I_SYNC > > > and the test of I_FREEING. > > > > > > Also extend the coverage of BUG_ON(I_FREEING) to I_CLEAR. > > > > > > Reported-by: Jeff Layton <jlayton@redhat.com> > > > Reported-by: Eric Sandeen <sandeen@sandeen.net> > > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > > > --- > > > fs/fs-writeback.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > --- linux.orig/fs/fs-writeback.c > > > +++ linux/fs/fs-writeback.c > > > @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, > > > spin_lock(&inode_lock); > > > WARN_ON(inode->i_state & I_NEW); > > > inode->i_state &= ~I_SYNC; > > > - if (!(inode->i_state & I_FREEING)) { > > > + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { > > > if (!(inode->i_state & I_DIRTY) && > > > mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > > Is the whole if needed? I had an impression that everyone calling > > __sync_single_inode() should better take care it does not race with inode > > freeing... So WARN_ON would be more appropriate IMHO. > > > > > /* > > > @@ -518,7 +518,7 @@ void generic_sync_sb_inodes(struct super > > > if (current_is_pdflush() && !writeback_acquire(bdi)) > > > break; > > > > > > - BUG_ON(inode->i_state & I_FREEING); > > > + BUG_ON(inode->i_state & (I_FREEING | I_CLEAR)); > > > __iget(inode); > > > pages_skipped = wbc->pages_skipped; > > > __writeback_single_inode(inode, wbc); > > Looking at this code, it looks a bit suspicious. What prevents this s_io > > list scan to race with inode freeing? In particular generic_forget_inode() > > Good catch. > > > can drop inode_lock to write the inode and in the mean time > > generic_sync_sb_inodes() can come, get a reference to the inode and start > > it's writeback... Subsequent iput() would then call generic_forget_inode() > > Another possibility: > > generic_forget_inode > inode->i_state |= I_WILL_FREE; > spin_unlock(&inode_lock); > generic_sync_sb_inodes() > spin_lock(&inode_lock); > __iget(inode); > __writeback_single_inode > // see non zero i_count > WARN_ON(inode->i_state & I_WILL_FREE); > > I'm wondering why didn't we saw reports on the last WARN_ON()? > Did we missed something? I meant the above race in my description ;-). Anyway, the race can happen only if we are unmounting the filesystem (normally, we bail out on sb->s_flags & MS_ACTIVE check - yes, it's a bit hidden and it also took me a while to understand why we weren't seeing tons of warnings...). > > on the inode again. So shouldn't we skip I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW > > inodes in this scan like we do for later in the function for another scan? Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] skip I_CLEAR state inodes 2009-06-03 14:16 ` Jan Kara @ 2009-06-03 14:47 ` Wu Fengguang 2009-06-06 3:07 ` [PATCH] writeback: skip new or to-be-freed inodes Wu Fengguang 0 siblings, 1 reply; 28+ messages in thread From: Wu Fengguang @ 2009-06-03 14:47 UTC (permalink / raw) To: Jan Kara Cc: Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin, Jeff Layton On Wed, Jun 03, 2009 at 10:16:36PM +0800, Jan Kara wrote: > On Wed 03-06-09 22:10:21, Wu Fengguang wrote: > > On Tue, Jun 02, 2009 at 07:37:36PM +0800, Jan Kara wrote: > > > On Tue 02-06-09 16:55:23, Wu Fengguang wrote: > > > > On Tue, Jun 02, 2009 at 05:38:35AM +0800, Eric Sandeen wrote: > > > > > Wu Fengguang wrote: > > > > > > Add I_CLEAR tests to drop_pagecache_sb(), generic_sync_sb_inodes() and > > > > > > add_dquot_ref(). > > > > > > > > > > > > clear_inode() will switch inode state from I_FREEING to I_CLEAR, > > > > > > and do so _outside_ of inode_lock. So any I_FREEING testing is > > > > > > incomplete without the testing of I_CLEAR. > > > > > > > > > > > > Masayoshi MIZUMA first discovered the bug in drop_pagecache_sb() and > > > > > > Jan Kara reminds fixing the other two cases. Thanks! > > > > > > > > > > Is there a reason it's not done for __sync_single_inode as well? > > > > > > > > It missed the glance because it don't have an obvious '|' in the line ;) > > > > > > > > > Jeff Layton asked the question and I'm following it up :) > > > > > > > > > > __sync_single_inode currently only tests I_FREEING, but I think we are > > > > > safe because __sync_single_inode sets I_SYNC, and clear_inode waits for > > > > > I_SYNC to be cleared before it changes I_STATE. > > > > > > > > But I_SYNC is removed just before the I_FREEING test, so we still have > > > > a small race window? > > > > > > > > > On the other hand, testing I_CLEAR here probably would be safe anyway, > > > > > and it'd be bonus points for consistency? > > > > > > > > So let's add the I_CLEAR test? > > > > > > > > > Same basic question for generic_sync_sb_inodes, which has a > > > > > BUG_ON(inode->i_state & I_FREEING), seems like this could check I_CLWAR > > > > > as well? > > > > > > > > Yes, we can add I_CLEAR here to catch more error condition. > > > > > > > > Thanks, > > > > Fengguang > > > > > > > > --- > > > > skip I_CLEAR state inodes in writeback routines > > > > > > > > The I_FREEING test in __sync_single_inode() is racy because > > > > clear_inode() can set i_state to I_CLEAR between the clear of I_SYNC > > > > and the test of I_FREEING. > > > > > > > > Also extend the coverage of BUG_ON(I_FREEING) to I_CLEAR. > > > > > > > > Reported-by: Jeff Layton <jlayton@redhat.com> > > > > Reported-by: Eric Sandeen <sandeen@sandeen.net> > > > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > > > > --- > > > > fs/fs-writeback.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > --- linux.orig/fs/fs-writeback.c > > > > +++ linux/fs/fs-writeback.c > > > > @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, > > > > spin_lock(&inode_lock); > > > > WARN_ON(inode->i_state & I_NEW); > > > > inode->i_state &= ~I_SYNC; > > > > - if (!(inode->i_state & I_FREEING)) { > > > > + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { > > > > if (!(inode->i_state & I_DIRTY) && > > > > mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > > > Is the whole if needed? I had an impression that everyone calling > > > __sync_single_inode() should better take care it does not race with inode > > > freeing... So WARN_ON would be more appropriate IMHO. > > > > > > > /* > > > > @@ -518,7 +518,7 @@ void generic_sync_sb_inodes(struct super > > > > if (current_is_pdflush() && !writeback_acquire(bdi)) > > > > break; > > > > > > > > - BUG_ON(inode->i_state & I_FREEING); > > > > + BUG_ON(inode->i_state & (I_FREEING | I_CLEAR)); > > > > __iget(inode); > > > > pages_skipped = wbc->pages_skipped; > > > > __writeback_single_inode(inode, wbc); > > > Looking at this code, it looks a bit suspicious. What prevents this s_io > > > list scan to race with inode freeing? In particular generic_forget_inode() > > > > Good catch. > > > > > can drop inode_lock to write the inode and in the mean time > > > generic_sync_sb_inodes() can come, get a reference to the inode and start > > > it's writeback... Subsequent iput() would then call generic_forget_inode() > > > > Another possibility: > > > > generic_forget_inode > > inode->i_state |= I_WILL_FREE; > > spin_unlock(&inode_lock); > > generic_sync_sb_inodes() > > spin_lock(&inode_lock); > > __iget(inode); > > __writeback_single_inode > > // see non zero i_count > > WARN_ON(inode->i_state & I_WILL_FREE); > > > > I'm wondering why didn't we saw reports on the last WARN_ON()? > > Did we missed something? > I meant the above race in my description ;-). Anyway, the race can happen > only if we are unmounting the filesystem (normally, we bail out on > sb->s_flags & MS_ACTIVE check - yes, it's a bit hidden and it also took me > a while to understand why we weren't seeing tons of warnings...). Ah OK. Just checked that all three callers of generic_sync_sb_inodes(): - writeback_inodes(): umount prevented - pohmelfs_kill_super(): just before umount - ubifs calls: too complex to be obvious.. At least the first two cases are safe, so we didn't see the error report ;) > > > on the inode again. So shouldn't we skip I_FREEING|I_CLEAR|I_WILL_FREE|I_NEW > > > inodes in this scan like we do for later in the function for another scan? Yes we should do this at least for safety. I_WILL_FREE means generic_forget_inode() is going to writeback the inode on its own, so generic_sync_sb_inodes() would better not to wade in. Thanks, Fengguang ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] writeback: skip new or to-be-freed inodes 2009-06-03 14:47 ` Wu Fengguang @ 2009-06-06 3:07 ` Wu Fengguang 2009-06-08 7:03 ` Artem Bityutskiy 2009-06-08 17:07 ` Jan Kara 0 siblings, 2 replies; 28+ messages in thread From: Wu Fengguang @ 2009-06-06 3:07 UTC (permalink / raw) To: Jan Kara Cc: Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin, Jeff Layton 1) I_FREEING tests should be coupled with I_CLEAR The two I_FREEING tests are racy because clear_inode() can set i_state to I_CLEAR between the clear of I_SYNC and the test of I_FREEING. 2) skip I_WILL_FREE inodes in generic_sync_sb_inodes() to avoid possible races with generic_forget_inode() generic_forget_inode() sets I_WILL_FREE call writeback on its own, so generic_sync_sb_inodes() shall not try to step in and create possible races: generic_forget_inode inode->i_state |= I_WILL_FREE; spin_unlock(&inode_lock); generic_sync_sb_inodes() spin_lock(&inode_lock); __iget(inode); __writeback_single_inode // see non zero i_count may WARN here ==> WARN_ON(inode->i_state & I_WILL_FREE); spin_unlock(&inode_lock); may call generic_forget_inode again ==> iput(inode); The above race and warning didn't turn up because writeback_inodes() holds the s_umount lock, so generic_forget_inode() finds MS_ACTIVE and returns early. But we are not sure the UBIFS calls and future callers will guarantee that. So skip I_WILL_FREE inodes for the sake of safety. CC: Jan Kara <jack@suse.cz> CC: Eric Sandeen <sandeen@sandeen.net> Acked-by: Jeff Layton <jlayton@redhat.com> CC: Masayoshi MIZUMA <m.mizuma@jp.fujitsu.com> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> --- fs/fs-writeback.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) --- linux.orig/fs/fs-writeback.c +++ linux/fs/fs-writeback.c @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, spin_lock(&inode_lock); WARN_ON(inode->i_state & I_NEW); inode->i_state &= ~I_SYNC; - if (!(inode->i_state & I_FREEING)) { + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { if (!(inode->i_state & I_DIRTY) && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { /* @@ -487,7 +487,7 @@ void generic_sync_sb_inodes(struct super break; } - if (inode->i_state & I_NEW) { + if (inode->i_state & (I_NEW | I_WILL_FREE)) { requeue_io(inode); continue; } @@ -518,7 +518,7 @@ void generic_sync_sb_inodes(struct super if (current_is_pdflush() && !writeback_acquire(bdi)) break; - BUG_ON(inode->i_state & I_FREEING); + BUG_ON(inode->i_state & (I_FREEING | I_CLEAR)); __iget(inode); pages_skipped = wbc->pages_skipped; __writeback_single_inode(inode, wbc); ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] writeback: skip new or to-be-freed inodes 2009-06-06 3:07 ` [PATCH] writeback: skip new or to-be-freed inodes Wu Fengguang @ 2009-06-08 7:03 ` Artem Bityutskiy 2009-06-08 9:29 ` Wu Fengguang 2009-06-08 17:07 ` Jan Kara 1 sibling, 1 reply; 28+ messages in thread From: Artem Bityutskiy @ 2009-06-08 7:03 UTC (permalink / raw) To: Wu Fengguang Cc: Jan Kara, Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin, Jeff Layton Wu Fengguang wrote: > The above race and warning didn't turn up because writeback_inodes() holds > the s_umount lock, so generic_forget_inode() finds MS_ACTIVE and returns > early. But we are not sure the UBIFS calls and future callers will guarantee > that. So skip I_WILL_FREE inodes for the sake of safety. The inode states are a bit vague for me, but vs. UBIFS - feel free to ask questions. -- Best Regards, Artem Bityutskiy (Артём Битюцкий) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] writeback: skip new or to-be-freed inodes 2009-06-08 7:03 ` Artem Bityutskiy @ 2009-06-08 9:29 ` Wu Fengguang 2009-06-08 10:45 ` Christoph Hellwig 2009-06-09 7:03 ` Artem Bityutskiy 0 siblings, 2 replies; 28+ messages in thread From: Wu Fengguang @ 2009-06-08 9:29 UTC (permalink / raw) To: Artem Bityutskiy Cc: Jan Kara, Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin, Jeff Layton Hi Artem, On Mon, Jun 08, 2009 at 03:03:10PM +0800, Artem Bityutskiy wrote: > Wu Fengguang wrote: > > The above race and warning didn't turn up because writeback_inodes() holds > > the s_umount lock, so generic_forget_inode() finds MS_ACTIVE and returns > > early. But we are not sure the UBIFS calls and future callers will guarantee > > that. So skip I_WILL_FREE inodes for the sake of safety. > > The inode states are a bit vague for me, but vs. UBIFS - feel > free to ask questions. Thank you. Basically I'm not sure if UBIFS guarantees it won't be unmounted (hence the MS_ACTIVE bit is on) when calling generic_sync_sb_inodes() in shrink_liability() and ubifs_sync_fs(). Thanks, Fengguang PS: our previous discussions > > Another possibility: > > > > generic_forget_inode > > inode->i_state |= I_WILL_FREE; > > spin_unlock(&inode_lock); > > generic_sync_sb_inodes() > > spin_lock(&inode_lock); > > __iget(inode); > > __writeback_single_inode > > // see non zero i_count > > WARN_ON(inode->i_state & I_WILL_FREE); > > > > I'm wondering why didn't we saw reports on the last WARN_ON()? > > Did we missed something? > I meant the above race in my description ;-). Anyway, the race can happen > only if we are unmounting the filesystem (normally, we bail out on > sb->s_flags & MS_ACTIVE check - yes, it's a bit hidden and it also took me > a while to understand why we weren't seeing tons of warnings...). Ah OK. Just checked that all three callers of generic_sync_sb_inodes(): - writeback_inodes(): umount prevented - pohmelfs_kill_super(): just before umount - ubifs calls: too complex to be obvious.. At least the first two cases are safe, so we didn't see the error report ;) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] writeback: skip new or to-be-freed inodes 2009-06-08 9:29 ` Wu Fengguang @ 2009-06-08 10:45 ` Christoph Hellwig 2009-06-09 7:24 ` Artem Bityutskiy 2009-06-09 7:03 ` Artem Bityutskiy 1 sibling, 1 reply; 28+ messages in thread From: Christoph Hellwig @ 2009-06-08 10:45 UTC (permalink / raw) To: Wu Fengguang Cc: Artem Bityutskiy, Jan Kara, Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin, Jeff Layton On Mon, Jun 08, 2009 at 05:29:30PM +0800, Wu Fengguang wrote: > Thank you. Basically I'm not sure if UBIFS guarantees it won't be > unmounted (hence the MS_ACTIVE bit is on) when calling > generic_sync_sb_inodes() in shrink_liability() and ubifs_sync_fs(). Btw, the call in ubifs_sync_fs should be superflous in the vfs-2.6#for-next tree. We now do make sure that all inodes are flushed before calling ->sync_fs with the wait parameter. shrink_liability is a more interesting case, I don't understand enough of ubifs to comment on it. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] writeback: skip new or to-be-freed inodes 2009-06-08 10:45 ` Christoph Hellwig @ 2009-06-09 7:24 ` Artem Bityutskiy 0 siblings, 0 replies; 28+ messages in thread From: Artem Bityutskiy @ 2009-06-09 7:24 UTC (permalink / raw) To: Christoph Hellwig Cc: Wu Fengguang, Jan Kara, Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin, Jeff Layton, Hunter Adrian (Nokia-D/Helsinki) [CCed Adiran Hunter] Christoph Hellwig wrote: > On Mon, Jun 08, 2009 at 05:29:30PM +0800, Wu Fengguang wrote: >> Thank you. Basically I'm not sure if UBIFS guarantees it won't be >> unmounted (hence the MS_ACTIVE bit is on) when calling >> generic_sync_sb_inodes() in shrink_liability() and ubifs_sync_fs(). > > Btw, the call in ubifs_sync_fs should be superflous in the > vfs-2.6#for-next tree. We now do make sure that all inodes are flushed > before calling ->sync_fs with the wait parameter. OK, thanks for letting know. Once this is merged upstream, I'll amend UBIFS. > shrink_liability is a more interesting case, I don't understand enough > of ubifs to comment on it. Well... I'm not sure if I can tell why we need this in few words. But I'll try. UBIFS supports on-the-flight compression. This, and other factors lead to a situation when UBIFS does not know how much the dirty data in the page/inode caches will take on the flash. Indeed, how do we know how well the data will be compressed? UBIFS has so called "budgeting" subsystem. This subsystem is responsible for accounting flash space. If user writes a new data page, which goes to the page cache and sits there, the budgeting sub-system decrements the free space counters by 4KiB. And so on. At some point the free space counters in the budgeting subsystem reache zero, which means we do not have more space. However, in 99% of the cases this is not true, because the budgeting subsystem's calculations are _very_ pessimistic, they always assume the worst case scenario like the data are uncompressible. So consider a situation when user writes a new data page. First of all, the ->write_begin function will call a budgeting sub-system function in order to reserve flash space for this new data page. The budgeting subsystem will see that space counters are zero. And what it will do it will call the 'shrink_liability()' function, which, among other things, may call the 'generic_sync_sb_inodes()' function, which will force write-back, and this will give us some space. Indeed, when we actually write the data back, we'll see how much flash space they really take. And in 99% of cases they will take less than we budgeted for, usually much less. This is the rough idea. In practice things are more complex, and there are factors like inability to know how much of dirty space may be reclaimed, what will be the index size after commit, etc. This all makes the budgeting subsystem complex and difficult to understand. Moreover, we still consider it as a work in progress, because we use too rough calculations, and there are too many heuristics. Here you may read some more information about UBIFS flash accounting issues: http://www.linux-mtd.infradead.org/doc/ubifs.html#L_spaceacc You may also find a lot of info here: http://www.linux-mtd.infradead.org/doc/ubifs.html#L_documentation Especially in this document: http://www.linux-mtd.infradead.org/doc/ubifs_whitepaper.pdf but it is not easy reading. You may search for "budget" in the doc. HTH. -- Best Regards, Artem Bityutskiy (Артём Битюцкий) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] writeback: skip new or to-be-freed inodes 2009-06-08 9:29 ` Wu Fengguang 2009-06-08 10:45 ` Christoph Hellwig @ 2009-06-09 7:03 ` Artem Bityutskiy 1 sibling, 0 replies; 28+ messages in thread From: Artem Bityutskiy @ 2009-06-09 7:03 UTC (permalink / raw) To: Wu Fengguang Cc: Jan Kara, Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin, Jeff Layton Wu Fengguang wrote: > Hi Artem, > > On Mon, Jun 08, 2009 at 03:03:10PM +0800, Artem Bityutskiy wrote: >> Wu Fengguang wrote: >>> The above race and warning didn't turn up because writeback_inodes() holds >>> the s_umount lock, so generic_forget_inode() finds MS_ACTIVE and returns >>> early. But we are not sure the UBIFS calls and future callers will guarantee >>> that. So skip I_WILL_FREE inodes for the sake of safety. >> The inode states are a bit vague for me, but vs. UBIFS - feel >> free to ask questions. > > Thank you. Basically I'm not sure if UBIFS guarantees it won't be > unmounted (hence the MS_ACTIVE bit is on) when calling > generic_sync_sb_inodes() in shrink_liability() and ubifs_sync_fs(). To be frank my VFS knowledge is not good enough to give you a good answer. MS_ACTIVE seems to be set by file-systems when they are mounted, and cleaned by VFS when unmounting. I guess MS_ACTIVE is used by FS-es to check whether unmounting is in progress or not. Anyway, UBIFS does not use it. The generic_sync_sb_inodes() is called only from within VFS operations, e.g., from ->create, ->rename, ->mknod, ->write_begin, ->setattr, etc. I mean, it is called from UBIFS implementations of the above calls. UBIFS never calls generic_sync_sb_inodes() function by itself, e.g., from the UBIFS background thread. Also, all calls to generic_sync_sb_inodes() are from within VFS operations which need writing, e.g., VFS called 'mnt_want_write()' for all of them. I think VFS must protect the FS from being unmunted while it is in the middle of an operation, right? I'm just not sure how this mechanism works. This would mean that yes, we cannot be unmounted while we are in generic_sync_sb_inodes() called by UBIFS. Did this help :-) ? -- Best Regards, Artem Bityutskiy (Артём Битюцкий) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] writeback: skip new or to-be-freed inodes 2009-06-06 3:07 ` [PATCH] writeback: skip new or to-be-freed inodes Wu Fengguang 2009-06-08 7:03 ` Artem Bityutskiy @ 2009-06-08 17:07 ` Jan Kara 1 sibling, 0 replies; 28+ messages in thread From: Jan Kara @ 2009-06-08 17:07 UTC (permalink / raw) To: Wu Fengguang Cc: Jan Kara, Eric Sandeen, Andrew Morton, LKML, Masayoshi MIZUMA, linux-fsdevel, viro, Nick Piggin, Jeff Layton On Sat 06-06-09 11:07:25, Wu Fengguang wrote: > 1) I_FREEING tests should be coupled with I_CLEAR > > The two I_FREEING tests are racy because clear_inode() can set i_state to > I_CLEAR between the clear of I_SYNC and the test of I_FREEING. > > 2) skip I_WILL_FREE inodes in generic_sync_sb_inodes() to avoid possible > races with generic_forget_inode() > > generic_forget_inode() sets I_WILL_FREE call writeback on its own, so > generic_sync_sb_inodes() shall not try to step in and create possible races: > > generic_forget_inode > inode->i_state |= I_WILL_FREE; > spin_unlock(&inode_lock); > generic_sync_sb_inodes() > spin_lock(&inode_lock); > __iget(inode); > __writeback_single_inode > // see non zero i_count > may WARN here ==> WARN_ON(inode->i_state & I_WILL_FREE); > spin_unlock(&inode_lock); > may call generic_forget_inode again ==> iput(inode); > > The above race and warning didn't turn up because writeback_inodes() holds > the s_umount lock, so generic_forget_inode() finds MS_ACTIVE and returns > early. But we are not sure the UBIFS calls and future callers will guarantee > that. So skip I_WILL_FREE inodes for the sake of safety. OK, the patch looks fine. Acked-by: Jan Kara <jack@suse.cz> Honza > CC: Jan Kara <jack@suse.cz> > CC: Eric Sandeen <sandeen@sandeen.net> > Acked-by: Jeff Layton <jlayton@redhat.com> > CC: Masayoshi MIZUMA <m.mizuma@jp.fujitsu.com> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > --- > fs/fs-writeback.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > --- linux.orig/fs/fs-writeback.c > +++ linux/fs/fs-writeback.c > @@ -316,7 +316,7 @@ __sync_single_inode(struct inode *inode, > spin_lock(&inode_lock); > WARN_ON(inode->i_state & I_NEW); > inode->i_state &= ~I_SYNC; > - if (!(inode->i_state & I_FREEING)) { > + if (!(inode->i_state & (I_FREEING | I_CLEAR))) { > if (!(inode->i_state & I_DIRTY) && > mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > /* > @@ -487,7 +487,7 @@ void generic_sync_sb_inodes(struct super > break; > } > > - if (inode->i_state & I_NEW) { > + if (inode->i_state & (I_NEW | I_WILL_FREE)) { > requeue_io(inode); > continue; > } > @@ -518,7 +518,7 @@ void generic_sync_sb_inodes(struct super > if (current_is_pdflush() && !writeback_acquire(bdi)) > break; > > - BUG_ON(inode->i_state & I_FREEING); > + BUG_ON(inode->i_state & (I_FREEING | I_CLEAR)); > __iget(inode); > pages_skipped = wbc->pages_skipped; > __writeback_single_inode(inode, wbc); -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2009-06-09 7:25 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-03-18 8:13 [PATCH][BUG] Lack of mutex_lock in drop_pagecache_sb() Masasyoshi MIZUMA 2009-03-23 10:38 ` Wu Fengguang 2009-03-24 7:06 ` Masayoshi MIZUMA 2009-03-24 7:44 ` Wu Fengguang 2009-03-24 12:05 ` Jan Kara 2009-03-24 12:11 ` Wu Fengguang 2009-03-24 12:40 ` [PATCH] skip I_CLEAR state inodes Wu Fengguang 2009-03-30 7:18 ` [PATCH][RESEND for 2.6.29-rc8-mm1] " Wu Fengguang 2009-03-31 23:43 ` Andrew Morton 2009-04-01 0:53 ` Wu Fengguang 2009-06-01 21:38 ` [PATCH] " Eric Sandeen 2009-06-02 8:55 ` Wu Fengguang 2009-06-02 10:27 ` Jeff Layton 2009-06-02 11:37 ` Jan Kara 2009-06-02 21:48 ` Eric Sandeen 2009-06-03 10:45 ` Jeff Layton 2009-06-03 13:32 ` Wu Fengguang 2009-06-03 14:00 ` Jan Kara 2009-06-03 14:10 ` Wu Fengguang 2009-06-03 14:16 ` Jan Kara 2009-06-03 14:47 ` Wu Fengguang 2009-06-06 3:07 ` [PATCH] writeback: skip new or to-be-freed inodes Wu Fengguang 2009-06-08 7:03 ` Artem Bityutskiy 2009-06-08 9:29 ` Wu Fengguang 2009-06-08 10:45 ` Christoph Hellwig 2009-06-09 7:24 ` Artem Bityutskiy 2009-06-09 7:03 ` Artem Bityutskiy 2009-06-08 17:07 ` Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).