* [PATCH 1/4] fs: do not drop inode_lock in dispose_list
@ 2010-10-24 17:40 Christoph Hellwig
2010-10-24 17:40 ` [PATCH 2/4] fs: fold invalidate_list into invalidate_inodes Christoph Hellwig
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Christoph Hellwig @ 2010-10-24 17:40 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel
Despite the comment above it we can not safely drop the lock here.
invalidate_list is called from many other places that just umount.
Also switch to proper list macros now that we never drop the lock.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c 2010-10-24 13:12:18.873003757 +0200
+++ linux-2.6/fs/inode.c 2010-10-24 14:35:29.924253651 +0200
@@ -482,26 +482,10 @@ static void dispose_list(struct list_hea
*/
static int invalidate_list(struct list_head *head, struct list_head *dispose)
{
- struct list_head *next;
+ struct inode *inode, *next;
int busy = 0;
- next = head->next;
- for (;;) {
- struct list_head *tmp = next;
- struct inode *inode;
-
- /*
- * We can reschedule here without worrying about the list's
- * consistency because the per-sb list of inodes must not
- * change during umount anymore, and because iprune_sem keeps
- * shrink_icache_memory() away.
- */
- cond_resched_lock(&inode_lock);
-
- next = next->next;
- if (tmp == head)
- break;
- inode = list_entry(tmp, struct inode, i_sb_list);
+ list_for_each_entry_safe(inode, next, head, i_sb_list) {
if (inode->i_state & I_NEW)
continue;
if (atomic_read(&inode->i_count)) {
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/4] fs: fold invalidate_list into invalidate_inodes
2010-10-24 17:40 [PATCH 1/4] fs: do not drop inode_lock in dispose_list Christoph Hellwig
@ 2010-10-24 17:40 ` Christoph Hellwig
2010-10-24 21:45 ` Christian Stroetmann
2010-10-24 17:40 ` [PATCH 3/4] fs: skip I_FREEING inodes in writeback_sb_inodes Christoph Hellwig
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2010-10-24 17:40 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c 2010-10-24 16:31:49.000000000 +0200
+++ linux-2.6/fs/inode.c 2010-10-24 16:32:20.609254420 +0200
@@ -477,15 +477,24 @@ static void dispose_list(struct list_hea
}
}
-/*
- * Invalidate all inodes for a device.
+/**
+ * invalidate_inodes - attempt to free all inodes on a superblock
+ * @sb: superblock to operate on
+ *
+ * Attempts to free all inodes for a given superblock. If there were any
+ * busy inodes return a non-zero value, else zero.
*/
-static int invalidate_list(struct list_head *head, struct list_head *dispose)
+int invalidate_inodes(struct super_block *sb)
{
- struct inode *inode, *next;
int busy = 0;
+ struct inode *inode, *next;
+ LIST_HEAD(dispose);
- list_for_each_entry_safe(inode, next, head, i_sb_list) {
+ down_write(&iprune_sem);
+
+ spin_lock(&inode_lock);
+ fsnotify_unmount_inodes(&sb->s_inodes);
+ list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
if (inode->i_state & I_NEW)
continue;
if (atomic_read(&inode->i_count)) {
@@ -499,34 +508,14 @@ static int invalidate_list(struct list_h
* Move the inode off the IO lists and LRU once I_FREEING is
* set so that it won't get moved back on there if it is dirty.
*/
- list_move(&inode->i_lru, dispose);
+ list_move(&inode->i_lru, &dispose);
list_del_init(&inode->i_wb_list);
if (!(inode->i_state & (I_DIRTY | I_SYNC)))
percpu_counter_dec(&nr_inodes_unused);
}
- return busy;
-}
-
-/**
- * invalidate_inodes - discard the inodes on a device
- * @sb: superblock
- *
- * Discard all of the inodes for a given superblock. If the discard
- * fails because there are busy inodes then a non zero value is returned.
- * If the discard is successful all the inodes have been discarded.
- */
-int invalidate_inodes(struct super_block *sb)
-{
- int busy;
- LIST_HEAD(throw_away);
-
- down_write(&iprune_sem);
- spin_lock(&inode_lock);
- fsnotify_unmount_inodes(&sb->s_inodes);
- busy = invalidate_list(&sb->s_inodes, &throw_away);
spin_unlock(&inode_lock);
- dispose_list(&throw_away);
+ dispose_list(&dispose);
up_write(&iprune_sem);
return busy;
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/4] fs: skip I_FREEING inodes in writeback_sb_inodes
2010-10-24 17:40 [PATCH 1/4] fs: do not drop inode_lock in dispose_list Christoph Hellwig
2010-10-24 17:40 ` [PATCH 2/4] fs: fold invalidate_list into invalidate_inodes Christoph Hellwig
@ 2010-10-24 17:40 ` Christoph Hellwig
2010-10-24 21:46 ` Christian Stroetmann
2010-10-24 17:40 ` [PATCH 4/4] fs: kill I_WILL_FREE Christoph Hellwig
2010-10-25 5:33 ` [PATCH 1/4] fs: do not drop inode_lock in dispose_list Dave Chinner
3 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2010-10-24 17:40 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel
Skip I_FREEING inodes just like I_WILL_FREE and I_NEW when walking the
writeback lists. Currenly this can't happen, but once we move from
inode_lock to more fine grained locking we can have an inode that's
still on the writeback lists but has I_FREEING set, and we absolutely
need to skip it here, just like we do for all other inode list walks.
Based on a patch from Dave Chinner.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: linux-2.6/fs/fs-writeback.c
===================================================================
--- linux-2.6.orig/fs/fs-writeback.c 2010-10-24 13:21:18.282010741 +0200
+++ linux-2.6/fs/fs-writeback.c 2010-10-24 13:21:35.456004735 +0200
@@ -487,10 +487,16 @@ static int writeback_sb_inodes(struct su
return 0;
}
- if (inode->i_state & (I_NEW | I_WILL_FREE)) {
+ /*
+ * Don't bother with new inodes or inodes beeing freed, first
+ * kind does not need peridic writeout yet, and for the latter
+ * kind writeout is handled by the freer.
+ */
+ if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) {
requeue_io(inode);
continue;
}
+
/*
* Was this inode dirtied after sync_sb_inodes was called?
* This keeps sync from extra jobs and livelock.
@@ -498,7 +504,6 @@ static int writeback_sb_inodes(struct su
if (inode_dirtied_after(inode, wbc->wb_start))
return 1;
- BUG_ON(inode->i_state & I_FREEING);
__iget(inode);
pages_skipped = wbc->pages_skipped;
writeback_single_inode(inode, wbc);
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/4] fs: kill I_WILL_FREE
2010-10-24 17:40 [PATCH 1/4] fs: do not drop inode_lock in dispose_list Christoph Hellwig
2010-10-24 17:40 ` [PATCH 2/4] fs: fold invalidate_list into invalidate_inodes Christoph Hellwig
2010-10-24 17:40 ` [PATCH 3/4] fs: skip I_FREEING inodes in writeback_sb_inodes Christoph Hellwig
@ 2010-10-24 17:40 ` Christoph Hellwig
2010-10-24 21:46 ` Christian Stroetmann
` (2 more replies)
2010-10-25 5:33 ` [PATCH 1/4] fs: do not drop inode_lock in dispose_list Dave Chinner
3 siblings, 3 replies; 15+ messages in thread
From: Christoph Hellwig @ 2010-10-24 17:40 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel
The I_WILL_FREE is currently set for inodes that we write out during
umount after dropping their last reference. It is handled equally
to I_FREEING in most places. The two execptions are:
- writeback_single_inode skips all list manipulations for I_FREEING,
but not for I_WILL_FREE. We don't care about which list an
I_WILL_FREE inode is on, because we will remove it from the list
a little bit later.
- __mark_inode_dirty skips I_FREEING inodes but not I_WILL_FREE
inodes. This only matters for filesystem that re-dirty the inode
during writeback and then use the I_DIRTY flags inside ->evict_inode.
The formers is done by XFS, but it uses it's internal state to flush
the inode. I could not find any filesystem that looks at I_DIRTY
inside ->evict_inode either.
Besides cleaning up the code removing I_WILL_FREE will allow us to
avoid one i_lock roundtrip once inode_lock is split and keep iput_final
more logic. This includes removing the __remove_inode_hash call in
iput_final, given that we never drop the protection from lookups now
that I_FREEING is set earlier.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: linux-2.6/fs/btrfs/inode.c
===================================================================
--- linux-2.6.orig/fs/btrfs/inode.c 2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/btrfs/inode.c 2010-10-24 16:32:43.647253791 +0200
@@ -3862,8 +3862,7 @@ again:
else if (inode->i_ino > entry->vfs_inode.i_ino)
p = &parent->rb_right;
else {
- WARN_ON(!(entry->vfs_inode.i_state &
- (I_WILL_FREE | I_FREEING)));
+ WARN_ON(!(entry->vfs_inode.i_state & I_FREEING));
rb_erase(parent, &root->inode_tree);
RB_CLEAR_NODE(parent);
spin_unlock(&root->inode_lock);
Index: linux-2.6/fs/drop_caches.c
===================================================================
--- linux-2.6.orig/fs/drop_caches.c 2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/drop_caches.c 2010-10-24 16:32:43.647253791 +0200
@@ -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_NEW))
continue;
if (inode->i_mapping->nrpages == 0)
continue;
Index: linux-2.6/fs/gfs2/inode.c
===================================================================
--- linux-2.6.orig/fs/gfs2/inode.c 2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/gfs2/inode.c 2010-10-24 16:32:43.650253651 +0200
@@ -84,7 +84,7 @@ static int iget_skip_test(struct inode *
struct gfs2_skip_data *data = opaque;
if (ip->i_no_addr == data->no_addr) {
- if (inode->i_state & (I_FREEING|I_WILL_FREE)){
+ if (inode->i_state & I_FREEING){
data->skipped = 1;
return 0;
}
Index: linux-2.6/fs/fs-writeback.c
===================================================================
--- linux-2.6.orig/fs/fs-writeback.c 2010-10-24 16:32:39.000000000 +0200
+++ linux-2.6/fs/fs-writeback.c 2010-10-24 16:32:43.654254629 +0200
@@ -301,8 +301,7 @@ static void inode_wait_for_writeback(str
/*
* Write out an inode's dirty pages. Called under inode_lock. Either the
- * caller has ref on the inode (either via __iget or via syscall against an fd)
- * or the inode has I_WILL_FREE set (via generic_forget_inode)
+ * caller has ref on the inode or the inode is beeing freed.
*
* If `wait' is set, wait on the writeout.
*
@@ -320,9 +319,7 @@ writeback_single_inode(struct inode *ino
int ret;
if (!atomic_read(&inode->i_count))
- WARN_ON(!(inode->i_state & (I_WILL_FREE|I_FREEING)));
- else
- WARN_ON(inode->i_state & I_WILL_FREE);
+ WARN_ON(!(inode->i_state & I_FREEING));
if (inode->i_state & I_SYNC) {
/*
@@ -492,7 +489,7 @@ static int writeback_sb_inodes(struct su
* kind does not need peridic writeout yet, and for the latter
* kind writeout is handled by the freer.
*/
- if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) {
+ if (inode->i_state & (I_NEW | I_FREEING)) {
requeue_io(inode);
continue;
}
@@ -1043,7 +1040,7 @@ static void wait_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_NEW))
continue;
mapping = inode->i_mapping;
if (mapping->nrpages == 0)
@@ -1154,7 +1151,7 @@ EXPORT_SYMBOL(sync_inodes_sb);
* This function commits an inode to disk immediately if it is dirty. This is
* primarily needed by knfsd.
*
- * The caller must either have a ref on the inode or must have set I_WILL_FREE.
+ * The caller must either have a ref on the inode or must have set I_FREEING.
*/
int write_inode_now(struct inode *inode, int sync)
{
Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c 2010-10-24 16:32:20.000000000 +0200
+++ linux-2.6/fs/inode.c 2010-10-24 16:33:46.912003547 +0200
@@ -667,7 +667,7 @@ repeat:
continue;
if (!test(inode, data))
continue;
- if (inode->i_state & (I_FREEING|I_WILL_FREE)) {
+ if (inode->i_state & I_FREEING) {
__wait_on_freeing_inode(inode);
goto repeat;
}
@@ -693,7 +693,7 @@ repeat:
continue;
if (inode->i_sb != sb)
continue;
- if (inode->i_state & (I_FREEING|I_WILL_FREE)) {
+ if (inode->i_state & I_FREEING) {
__wait_on_freeing_inode(inode);
goto repeat;
}
@@ -964,7 +964,7 @@ EXPORT_SYMBOL(iunique);
struct inode *igrab(struct inode *inode)
{
spin_lock(&inode_lock);
- if (!(inode->i_state & (I_FREEING|I_WILL_FREE)))
+ if (!(inode->i_state & I_FREEING))
__iget(inode);
else
/*
@@ -1211,7 +1211,7 @@ int insert_inode_locked(struct inode *in
continue;
if (old->i_sb != sb)
continue;
- if (old->i_state & (I_FREEING|I_WILL_FREE))
+ if (old->i_state & I_FREEING)
continue;
break;
}
@@ -1250,7 +1250,7 @@ int insert_inode_locked4(struct inode *i
continue;
if (!test(old, data))
continue;
- if (old->i_state & (I_FREEING|I_WILL_FREE))
+ if (old->i_state & I_FREEING)
continue;
break;
}
@@ -1305,6 +1305,8 @@ static void iput_final(struct inode *ino
const struct super_operations *op = inode->i_sb->s_op;
int drop;
+ WARN_ON(inode->i_state & I_NEW);
+
if (op && op->drop_inode)
drop = op->drop_inode(inode);
else
@@ -1313,25 +1315,19 @@ static void iput_final(struct inode *ino
if (!drop) {
if (sb->s_flags & MS_ACTIVE) {
inode->i_state |= I_REFERENCED;
- if (!(inode->i_state & (I_DIRTY|I_SYNC))) {
+ if (!(inode->i_state & (I_DIRTY|I_SYNC)))
inode_lru_list_add(inode);
- }
spin_unlock(&inode_lock);
return;
}
- WARN_ON(inode->i_state & I_NEW);
- inode->i_state |= I_WILL_FREE;
+ inode->i_state |= I_FREEING;
spin_unlock(&inode_lock);
write_inode_now(inode, 1);
spin_lock(&inode_lock);
- WARN_ON(inode->i_state & I_NEW);
- inode->i_state &= ~I_WILL_FREE;
- __remove_inode_hash(inode);
+ } else {
+ inode->i_state |= I_FREEING;
}
- WARN_ON(inode->i_state & I_NEW);
- inode->i_state |= I_FREEING;
-
/*
* Move the inode off the IO lists and LRU once I_FREEING is
* set so that it won't get moved back on there if it is dirty.
Index: linux-2.6/fs/notify/inode_mark.c
===================================================================
--- linux-2.6.orig/fs/notify/inode_mark.c 2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/notify/inode_mark.c 2010-10-24 16:32:43.661255957 +0200
@@ -244,11 +244,11 @@ void fsnotify_unmount_inodes(struct list
struct inode *need_iput_tmp;
/*
- * We cannot __iget() an inode in state I_FREEING,
- * I_WILL_FREE, or I_NEW which is fine because by that point
- * the inode cannot have any associated watches.
+ * We cannot __iget() an inode in state I_FREEING or I_NEW,
+ * which is fine because by that point the inode cannot
+ * have any associated watches.
*/
- if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW))
+ if (inode->i_state & (I_FREEING | I_NEW))
continue;
/*
@@ -272,7 +272,7 @@ void fsnotify_unmount_inodes(struct list
/* In case the dropping of a reference would nuke next_i. */
if ((&next_i->i_sb_list != list) &&
atomic_read(&next_i->i_count) &&
- !(next_i->i_state & (I_FREEING | I_WILL_FREE))) {
+ !(next_i->i_state & I_FREEING)) {
__iget(next_i);
need_iput = next_i;
}
Index: linux-2.6/fs/quota/dquot.c
===================================================================
--- linux-2.6.orig/fs/quota/dquot.c 2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/quota/dquot.c 2010-10-24 16:32:43.665253722 +0200
@@ -898,7 +898,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_NEW))
continue;
#ifdef CONFIG_QUOTA_DEBUG
if (unlikely(inode_get_rsv_space(inode) > 0))
Index: linux-2.6/fs/xfs/linux-2.6/xfs_iops.c
===================================================================
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_iops.c 2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/xfs/linux-2.6/xfs_iops.c 2010-10-24 16:32:43.665253722 +0200
@@ -80,7 +80,7 @@ xfs_mark_inode_dirty_sync(
{
struct inode *inode = VFS_I(ip);
- if (!(inode->i_state & (I_WILL_FREE|I_FREEING)))
+ if (!(inode->i_state & I_FREEING))
mark_inode_dirty_sync(inode);
}
@@ -90,7 +90,7 @@ xfs_mark_inode_dirty(
{
struct inode *inode = VFS_I(ip);
- if (!(inode->i_state & (I_WILL_FREE|I_FREEING)))
+ if (!(inode->i_state & I_FREEING))
mark_inode_dirty(inode);
}
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h 2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/include/linux/fs.h 2010-10-24 16:32:43.673253652 +0200
@@ -1601,8 +1601,8 @@ struct super_operations {
* I_DIRTY_DATASYNC and I_DIRTY_PAGES.
*
* Four bits define the lifetime of an inode. Initially, inodes are I_NEW,
- * until that flag is cleared. I_WILL_FREE, I_FREEING and I_CLEAR are set at
- * various stages of removing an inode.
+ * until that flag is cleared. I_FREEING and I_CLEAR are set at various
+ * stages of removing an inode.
*
* Two bits are used for locking and completion notification, I_NEW and I_SYNC.
*
@@ -1613,46 +1613,39 @@ struct super_operations {
* don't have to write inode on fdatasync() when only
* mtime has changed in it.
* I_DIRTY_PAGES Inode has dirty pages. Inode itself may be clean.
+ * I_FREEING Set when inode is about to be freed but still has dirty
+ * pages or buffers attached or the inode itself is still
+ * dirty.
+ * I_CLEAR Added by end_writeback(). In this state the inode is
+ * clean and can be destroyed. Inode keeps I_FREEING.
+ * I_REFERENCED Inode was recently referenced on the LRU and got another
+ * pass before eviction.
* I_NEW Serves as both a mutex and completion notification.
* New inodes set I_NEW. If two processes both create
* the same inode, one of them will release its inode and
* wait for I_NEW to be released before returning.
- * Inodes in I_WILL_FREE, I_FREEING or I_CLEAR state can
- * also cause waiting on I_NEW, without I_NEW actually
- * being set. find_inode() uses this to prevent returning
- * nearly-dead inodes.
- * I_WILL_FREE Must be set when calling write_inode_now() if i_count
- * is zero. I_FREEING must be set when I_WILL_FREE is
- * cleared.
- * I_FREEING Set when inode is about to be freed but still has dirty
- * pages or buffers attached or the inode itself is still
- * dirty.
- * I_CLEAR Added by end_writeback(). In this state the inode is clean
- * and can be destroyed. Inode keeps I_FREEING.
- *
- * Inodes that are I_WILL_FREE, I_FREEING or I_CLEAR are
- * prohibited for many purposes. iget() must wait for
- * the inode to be completely released, then create it
- * anew. Other functions will just ignore such inodes,
- * if appropriate. I_NEW is used for waiting.
- *
+ * Inodes in I_FREEING state can also cause waiting on
+ * I_NEW, without I_NEW actually being set. find_inode()
+ * uses this to prevent returning nearly-dead inodes.
* I_SYNC Synchonized write of dirty inode data. The bits is
* set during data writeback, and cleared with a wakeup
* on the bit address once it is done.
*
- * Q: What is the difference between I_WILL_FREE and I_FREEING?
+ * Inodes that have I_FREEING set are prohibited for many purposes.
+ * iget*_locked() must wait for the inode to be completely released, then
+ * create it anew. Other functions will just ignore such inodes, if
+ * appropriate. I_NEW is used for waiting.
*/
#define I_DIRTY_SYNC (1 << 0)
#define I_DIRTY_DATASYNC (1 << 1)
#define I_DIRTY_PAGES (1 << 2)
-#define __I_NEW 3
+#define I_FREEING (1 << 3)
+#define I_CLEAR (1 << 4)
+#define I_REFERENCED (1 << 5)
+#define __I_NEW 6
#define I_NEW (1 << __I_NEW)
-#define I_WILL_FREE (1 << 4)
-#define I_FREEING (1 << 5)
-#define I_CLEAR (1 << 6)
#define __I_SYNC 7
#define I_SYNC (1 << __I_SYNC)
-#define I_REFERENCED (1 << 8)
#define I_DIRTY (I_DIRTY_SYNC | I_DIRTY_DATASYNC | I_DIRTY_PAGES)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] fs: fold invalidate_list into invalidate_inodes
2010-10-24 17:40 ` [PATCH 2/4] fs: fold invalidate_list into invalidate_inodes Christoph Hellwig
@ 2010-10-24 21:45 ` Christian Stroetmann
0 siblings, 0 replies; 15+ messages in thread
From: Christian Stroetmann @ 2010-10-24 21:45 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-fsdevel
Aloha;
Just only a typo, maybe
On the 24.10.2010 19:40, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig<hch@lst.de>
>
> Index: linux-2.6/fs/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/inode.c 2010-10-24 16:31:49.000000000 +0200
> +++ linux-2.6/fs/inode.c 2010-10-24 16:32:20.609254420 +0200
> @@ -477,15 +477,24 @@ static void dispose_list(struct list_hea
> }
> }
>
> -/*
> - * Invalidate all inodes for a device.
> +/**
> + * invalidate_inodes - attempt to free all inodes on a superblock
> + * @sb: superblock to operate on
> + *
> + * Attempts to free all inodes for a given superblock. If there were any
> + * busy inodes return a non-zero value, else zero.
maybe:
, then return a
[...]
Have fun
Christian Stroetmann
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] fs: skip I_FREEING inodes in writeback_sb_inodes
2010-10-24 17:40 ` [PATCH 3/4] fs: skip I_FREEING inodes in writeback_sb_inodes Christoph Hellwig
@ 2010-10-24 21:46 ` Christian Stroetmann
0 siblings, 0 replies; 15+ messages in thread
From: Christian Stroetmann @ 2010-10-24 21:46 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-fsdevel
little typos
On the 24.10.2010 19:40, Christoph Hellwig wrote:
> Skip I_FREEING inodes just like I_WILL_FREE and I_NEW when walking the
> writeback lists. Currenly this can't happen, but once we move from
> inode_lock to more fine grained locking we can have an inode that's
> still on the writeback lists but has I_FREEING set, and we absolutely
> need to skip it here, just like we do for all other inode list walks.
>
> Based on a patch from Dave Chinner.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
>
> Index: linux-2.6/fs/fs-writeback.c
> ===================================================================
> --- linux-2.6.orig/fs/fs-writeback.c 2010-10-24 13:21:18.282010741 +0200
> +++ linux-2.6/fs/fs-writeback.c 2010-10-24 13:21:35.456004735 +0200
> @@ -487,10 +487,16 @@ static int writeback_sb_inodes(struct su
> return 0;
> }
>
> - if (inode->i_state& (I_NEW | I_WILL_FREE)) {
> + /*
> + * Don't bother with new inodes or inodes beeing freed, first
being
> + * kind does not need peridic writeout yet, and for the latter
> + * kind writeout is handled by the freer.
don't know:
freer inode
or
releaser
[...]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] fs: kill I_WILL_FREE
2010-10-24 17:40 ` [PATCH 4/4] fs: kill I_WILL_FREE Christoph Hellwig
@ 2010-10-24 21:46 ` Christian Stroetmann
2010-10-24 21:50 ` Christian Stroetmann
2010-10-26 1:28 ` Al Viro
2010-10-26 19:18 ` Al Viro
2 siblings, 1 reply; 15+ messages in thread
From: Christian Stroetmann @ 2010-10-24 21:46 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-fsdevel
little typos
On the 24.10.2010 19:40, Christoph Hellwig wrote:
> The I_WILL_FREE is currently set for inodes that we write out during
> umount after dropping their last reference. It is handled equally
> to I_FREEING in most places. The two execptions are:
>
> - writeback_single_inode skips all list manipulations for I_FREEING,
> but not for I_WILL_FREE. We don't care about which list an
> I_WILL_FREE inode is on, because we will remove it from the list
> a little bit later.
> - __mark_inode_dirty skips I_FREEING inodes but not I_WILL_FREE
> inodes. This only matters for filesystem that re-dirty the inode
> during writeback and then use the I_DIRTY flags inside ->evict_inode.
> The formers is done by XFS, but it uses it's internal state to flush
> the inode. I could not find any filesystem that looks at I_DIRTY
> inside ->evict_inode either.
>
> Besides cleaning up the code removing I_WILL_FREE will allow us to
> avoid one i_lock roundtrip once inode_lock is split and keep iput_final
> more logic. This includes removing the __remove_inode_hash call in
> iput_final, given that we never drop the protection from lookups now
> that I_FREEING is set earlier.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
>
> Index: linux-2.6/fs/btrfs/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/btrfs/inode.c 2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/btrfs/inode.c 2010-10-24 16:32:43.647253791 +0200
> @@ -3862,8 +3862,7 @@ again:
> else if (inode->i_ino> entry->vfs_inode.i_ino)
> p =&parent->rb_right;
> else {
> - WARN_ON(!(entry->vfs_inode.i_state&
> - (I_WILL_FREE | I_FREEING)));
> + WARN_ON(!(entry->vfs_inode.i_state& I_FREEING));
> rb_erase(parent,&root->inode_tree);
> RB_CLEAR_NODE(parent);
> spin_unlock(&root->inode_lock);
> Index: linux-2.6/fs/drop_caches.c
> ===================================================================
> --- linux-2.6.orig/fs/drop_caches.c 2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/drop_caches.c 2010-10-24 16:32:43.647253791 +0200
> @@ -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_NEW))
> continue;
> if (inode->i_mapping->nrpages == 0)
> continue;
> Index: linux-2.6/fs/gfs2/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/gfs2/inode.c 2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/gfs2/inode.c 2010-10-24 16:32:43.650253651 +0200
> @@ -84,7 +84,7 @@ static int iget_skip_test(struct inode *
> struct gfs2_skip_data *data = opaque;
>
> if (ip->i_no_addr == data->no_addr) {
> - if (inode->i_state& (I_FREEING|I_WILL_FREE)){
> + if (inode->i_state& I_FREEING){
> data->skipped = 1;
> return 0;
> }
> Index: linux-2.6/fs/fs-writeback.c
> ===================================================================
> --- linux-2.6.orig/fs/fs-writeback.c 2010-10-24 16:32:39.000000000 +0200
> +++ linux-2.6/fs/fs-writeback.c 2010-10-24 16:32:43.654254629 +0200
> @@ -301,8 +301,7 @@ static void inode_wait_for_writeback(str
>
> /*
> * Write out an inode's dirty pages. Called under inode_lock. Either the
> - * caller has ref on the inode (either via __iget or via syscall against an fd)
> - * or the inode has I_WILL_FREE set (via generic_forget_inode)
> + * caller has ref on the inode or the inode is beeing freed.
> *
> * If `wait' is set, wait on the writeout.
> *
> @@ -320,9 +319,7 @@ writeback_single_inode(struct inode *ino
> int ret;
>
> if (!atomic_read(&inode->i_count))
> - WARN_ON(!(inode->i_state& (I_WILL_FREE|I_FREEING)));
> - else
> - WARN_ON(inode->i_state& I_WILL_FREE);
> + WARN_ON(!(inode->i_state& I_FREEING));
>
> if (inode->i_state& I_SYNC) {
> /*
> @@ -492,7 +489,7 @@ static int writeback_sb_inodes(struct su
> * kind does not need peridic writeout yet, and for the latter
> * kind writeout is handled by the freer.
don't know again:
freer inode
or
releaser
> */
> - if (inode->i_state& (I_NEW | I_FREEING | I_WILL_FREE)) {
> + if (inode->i_state& (I_NEW | I_FREEING)) {
> requeue_io(inode);
> continue;
> }
> @@ -1043,7 +1040,7 @@ static void wait_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_NEW))
> continue;
> mapping = inode->i_mapping;
> if (mapping->nrpages == 0)
> @@ -1154,7 +1151,7 @@ EXPORT_SYMBOL(sync_inodes_sb);
> * This function commits an inode to disk immediately if it is dirty. This is
> * primarily needed by knfsd.
> *
> - * The caller must either have a ref on the inode or must have set I_WILL_FREE.
> + * The caller must either have a ref on the inode or must have set I_FREEING.
> */
> int write_inode_now(struct inode *inode, int sync)
> {
> Index: linux-2.6/fs/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/inode.c 2010-10-24 16:32:20.000000000 +0200
> +++ linux-2.6/fs/inode.c 2010-10-24 16:33:46.912003547 +0200
> @@ -667,7 +667,7 @@ repeat:
> continue;
> if (!test(inode, data))
> continue;
> - if (inode->i_state& (I_FREEING|I_WILL_FREE)) {
> + if (inode->i_state& I_FREEING) {
> __wait_on_freeing_inode(inode);
> goto repeat;
> }
> @@ -693,7 +693,7 @@ repeat:
> continue;
> if (inode->i_sb != sb)
> continue;
> - if (inode->i_state& (I_FREEING|I_WILL_FREE)) {
> + if (inode->i_state& I_FREEING) {
> __wait_on_freeing_inode(inode);
> goto repeat;
> }
> @@ -964,7 +964,7 @@ EXPORT_SYMBOL(iunique);
> struct inode *igrab(struct inode *inode)
> {
> spin_lock(&inode_lock);
> - if (!(inode->i_state& (I_FREEING|I_WILL_FREE)))
> + if (!(inode->i_state& I_FREEING))
> __iget(inode);
> else
> /*
> @@ -1211,7 +1211,7 @@ int insert_inode_locked(struct inode *in
> continue;
> if (old->i_sb != sb)
> continue;
> - if (old->i_state& (I_FREEING|I_WILL_FREE))
> + if (old->i_state& I_FREEING)
> continue;
> break;
> }
> @@ -1250,7 +1250,7 @@ int insert_inode_locked4(struct inode *i
> continue;
> if (!test(old, data))
> continue;
> - if (old->i_state& (I_FREEING|I_WILL_FREE))
> + if (old->i_state& I_FREEING)
> continue;
> break;
> }
> @@ -1305,6 +1305,8 @@ static void iput_final(struct inode *ino
> const struct super_operations *op = inode->i_sb->s_op;
> int drop;
>
> + WARN_ON(inode->i_state& I_NEW);
> +
> if (op&& op->drop_inode)
> drop = op->drop_inode(inode);
> else
> @@ -1313,25 +1315,19 @@ static void iput_final(struct inode *ino
> if (!drop) {
> if (sb->s_flags& MS_ACTIVE) {
> inode->i_state |= I_REFERENCED;
> - if (!(inode->i_state& (I_DIRTY|I_SYNC))) {
> + if (!(inode->i_state& (I_DIRTY|I_SYNC)))
> inode_lru_list_add(inode);
> - }
> spin_unlock(&inode_lock);
> return;
> }
> - WARN_ON(inode->i_state& I_NEW);
> - inode->i_state |= I_WILL_FREE;
> + inode->i_state |= I_FREEING;
> spin_unlock(&inode_lock);
> write_inode_now(inode, 1);
> spin_lock(&inode_lock);
> - WARN_ON(inode->i_state& I_NEW);
> - inode->i_state&= ~I_WILL_FREE;
> - __remove_inode_hash(inode);
> + } else {
> + inode->i_state |= I_FREEING;
> }
>
> - WARN_ON(inode->i_state& I_NEW);
> - inode->i_state |= I_FREEING;
> -
> /*
> * Move the inode off the IO lists and LRU once I_FREEING is
> * set so that it won't get moved back on there if it is dirty.
> Index: linux-2.6/fs/notify/inode_mark.c
> ===================================================================
> --- linux-2.6.orig/fs/notify/inode_mark.c 2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/notify/inode_mark.c 2010-10-24 16:32:43.661255957 +0200
> @@ -244,11 +244,11 @@ void fsnotify_unmount_inodes(struct list
> struct inode *need_iput_tmp;
>
> /*
> - * We cannot __iget() an inode in state I_FREEING,
> - * I_WILL_FREE, or I_NEW which is fine because by that point
> - * the inode cannot have any associated watches.
> + * We cannot __iget() an inode in state I_FREEING or I_NEW,
> + * which is fine because by that point the inode cannot
> + * have any associated watches.
> */
> - if (inode->i_state& (I_FREEING|I_WILL_FREE|I_NEW))
> + if (inode->i_state& (I_FREEING | I_NEW))
> continue;
>
> /*
> @@ -272,7 +272,7 @@ void fsnotify_unmount_inodes(struct list
> /* In case the dropping of a reference would nuke next_i. */
> if ((&next_i->i_sb_list != list)&&
> atomic_read(&next_i->i_count)&&
> - !(next_i->i_state& (I_FREEING | I_WILL_FREE))) {
> + !(next_i->i_state& I_FREEING)) {
> __iget(next_i);
> need_iput = next_i;
> }
> Index: linux-2.6/fs/quota/dquot.c
> ===================================================================
> --- linux-2.6.orig/fs/quota/dquot.c 2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/quota/dquot.c 2010-10-24 16:32:43.665253722 +0200
> @@ -898,7 +898,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_NEW))
> continue;
> #ifdef CONFIG_QUOTA_DEBUG
> if (unlikely(inode_get_rsv_space(inode)> 0))
> Index: linux-2.6/fs/xfs/linux-2.6/xfs_iops.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_iops.c 2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_iops.c 2010-10-24 16:32:43.665253722 +0200
> @@ -80,7 +80,7 @@ xfs_mark_inode_dirty_sync(
> {
> struct inode *inode = VFS_I(ip);
>
> - if (!(inode->i_state& (I_WILL_FREE|I_FREEING)))
> + if (!(inode->i_state& I_FREEING))
> mark_inode_dirty_sync(inode);
> }
>
> @@ -90,7 +90,7 @@ xfs_mark_inode_dirty(
> {
> struct inode *inode = VFS_I(ip);
>
> - if (!(inode->i_state& (I_WILL_FREE|I_FREEING)))
> + if (!(inode->i_state& I_FREEING))
> mark_inode_dirty(inode);
> }
>
> Index: linux-2.6/include/linux/fs.h
> ===================================================================
> --- linux-2.6.orig/include/linux/fs.h 2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/include/linux/fs.h 2010-10-24 16:32:43.673253652 +0200
> @@ -1601,8 +1601,8 @@ struct super_operations {
> * I_DIRTY_DATASYNC and I_DIRTY_PAGES.
> *
> * Four bits define the lifetime of an inode. Initially, inodes are I_NEW,
comma too much?!
> - * until that flag is cleared. I_WILL_FREE, I_FREEING and I_CLEAR are set at
> - * various stages of removing an inode.
> + * until that flag is cleared. I_FREEING and I_CLEAR are set at various
> + * stages of removing an inode.
> *
> * Two bits are used for locking and completion notification, I_NEW and I_SYNC.
> *
> @@ -1613,46 +1613,39 @@ struct super_operations {
> * don't have to write inode on fdatasync() when only
> * mtime has changed in it.
> * I_DIRTY_PAGES Inode has dirty pages. Inode itself may be clean.
> + * I_FREEING Set when inode is about to be freed but still has dirty
> + * pages or buffers attached or the inode itself is still
maybe a comma:
, or the inode itself is
> + * dirty.
> + * I_CLEAR Added by end_writeback(). In this state the inode is
> + * clean and can be destroyed. Inode keeps I_FREEING.
> + * I_REFERENCED Inode was recently referenced on the LRU and got another
> + * pass before eviction.
> * I_NEW Serves as both a mutex and completion notification.
maybe:
Serves as both: a mutex and a completion notification.
> * New inodes set I_NEW. If two processes both create
> * the same inode, one of them will release its inode and
, then one of them
> * wait for I_NEW to be released before returning.
> - * Inodes in I_WILL_FREE, I_FREEING or I_CLEAR state can
> - * also cause waiting on I_NEW, without I_NEW actually
> - * being set. find_inode() uses this to prevent returning
> - * nearly-dead inodes.
> - * I_WILL_FREE Must be set when calling write_inode_now() if i_count
> - * is zero. I_FREEING must be set when I_WILL_FREE is
> - * cleared.
> - * I_FREEING Set when inode is about to be freed but still has dirty
> - * pages or buffers attached or the inode itself is still
> - * dirty.
> - * I_CLEAR Added by end_writeback(). In this state the inode is clean
> - * and can be destroyed. Inode keeps I_FREEING.
> - *
> - * Inodes that are I_WILL_FREE, I_FREEING or I_CLEAR are
> - * prohibited for many purposes. iget() must wait for
> - * the inode to be completely released, then create it
> - * anew. Other functions will just ignore such inodes,
> - * if appropriate. I_NEW is used for waiting.
> - *
> + * Inodes in I_FREEING state can also cause waiting on
> + * I_NEW, without I_NEW actually being set. find_inode()
> + * uses this to prevent returning nearly-dead inodes.
> * I_SYNC Synchonized write of dirty inode data. The bits is
The bit is
> * set during data writeback, and cleared with a wakeup
> * on the bit address once it is done.
> *
> - * Q: What is the difference between I_WILL_FREE and I_FREEING?
> + * Inodes that have I_FREEING set are prohibited for many purposes.
> + * iget*_locked() must wait for the inode to be completely released, then
> + * create it anew. Other functions will just ignore such inodes, if
> + * appropriate. I_NEW is used for waiting.
appropriated
> */
> #define I_DIRTY_SYNC (1<< 0)
> #define I_DIRTY_DATASYNC (1<< 1)
> #define I_DIRTY_PAGES (1<< 2)
> -#define __I_NEW 3
> +#define I_FREEING (1<< 3)
> +#define I_CLEAR (1<< 4)
> +#define I_REFERENCED (1<< 5)
> +#define __I_NEW 6
> #define I_NEW (1<< __I_NEW)
> -#define I_WILL_FREE (1<< 4)
> -#define I_FREEING (1<< 5)
> -#define I_CLEAR (1<< 6)
> #define __I_SYNC 7
> #define I_SYNC (1<< __I_SYNC)
> -#define I_REFERENCED (1<< 8)
>
> #define I_DIRTY (I_DIRTY_SYNC | I_DIRTY_DATASYNC | I_DIRTY_PAGES)
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] fs: kill I_WILL_FREE
2010-10-24 21:46 ` Christian Stroetmann
@ 2010-10-24 21:50 ` Christian Stroetmann
0 siblings, 0 replies; 15+ messages in thread
From: Christian Stroetmann @ 2010-10-24 21:50 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-fsdevel
and one again
On the 24.10.2010 23:46, Christian Stroetmann wrote:
> little typos
>
> On the 24.10.2010 19:40, Christoph Hellwig wrote:
>> The I_WILL_FREE is currently set for inodes that we write out during
>> umount after dropping their last reference. It is handled equally
>> to I_FREEING in most places. The two execptions are:
>>
>> - writeback_single_inode skips all list manipulations for I_FREEING,
>> but not for I_WILL_FREE. We don't care about which list an
>> I_WILL_FREE inode is on, because we will remove it from the list
>> a little bit later.
>> - __mark_inode_dirty skips I_FREEING inodes but not I_WILL_FREE
>> inodes. This only matters for filesystem that re-dirty the inode
>> during writeback and then use the I_DIRTY flags inside
>> ->evict_inode.
>> The formers is done by XFS, but it uses it's internal state to flush
>> the inode. I could not find any filesystem that looks at I_DIRTY
>> inside ->evict_inode either.
>>
>> Besides cleaning up the code removing I_WILL_FREE will allow us to
>> avoid one i_lock roundtrip once inode_lock is split and keep iput_final
>> more logic. This includes removing the __remove_inode_hash call in
>> iput_final, given that we never drop the protection from lookups now
>> that I_FREEING is set earlier.
>>
>> Signed-off-by: Christoph Hellwig<hch@lst.de>
>>
>> Index: linux-2.6/fs/btrfs/inode.c
>> ===================================================================
>> --- linux-2.6.orig/fs/btrfs/inode.c 2010-10-24 16:30:05.000000000
>> +0200
>> +++ linux-2.6/fs/btrfs/inode.c 2010-10-24 16:32:43.647253791 +0200
>> @@ -3862,8 +3862,7 @@ again:
>> else if (inode->i_ino> entry->vfs_inode.i_ino)
>> p =&parent->rb_right;
>> else {
>> - WARN_ON(!(entry->vfs_inode.i_state&
>> - (I_WILL_FREE | I_FREEING)));
>> + WARN_ON(!(entry->vfs_inode.i_state& I_FREEING));
>> rb_erase(parent,&root->inode_tree);
>> RB_CLEAR_NODE(parent);
>> spin_unlock(&root->inode_lock);
>> Index: linux-2.6/fs/drop_caches.c
>> ===================================================================
>> --- linux-2.6.orig/fs/drop_caches.c 2010-10-24 16:30:05.000000000
>> +0200
>> +++ linux-2.6/fs/drop_caches.c 2010-10-24 16:32:43.647253791 +0200
>> @@ -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_NEW))
>> continue;
>> if (inode->i_mapping->nrpages == 0)
>> continue;
>> Index: linux-2.6/fs/gfs2/inode.c
>> ===================================================================
>> --- linux-2.6.orig/fs/gfs2/inode.c 2010-10-24 16:30:05.000000000
>> +0200
>> +++ linux-2.6/fs/gfs2/inode.c 2010-10-24 16:32:43.650253651 +0200
>> @@ -84,7 +84,7 @@ static int iget_skip_test(struct inode *
>> struct gfs2_skip_data *data = opaque;
>>
>> if (ip->i_no_addr == data->no_addr) {
>> - if (inode->i_state& (I_FREEING|I_WILL_FREE)){
>> + if (inode->i_state& I_FREEING){
>> data->skipped = 1;
>> return 0;
>> }
>> Index: linux-2.6/fs/fs-writeback.c
>> ===================================================================
>> --- linux-2.6.orig/fs/fs-writeback.c 2010-10-24 16:32:39.000000000
>> +0200
>> +++ linux-2.6/fs/fs-writeback.c 2010-10-24 16:32:43.654254629 +0200
>> @@ -301,8 +301,7 @@ static void inode_wait_for_writeback(str
>>
>> /*
>> * Write out an inode's dirty pages. Called under inode_lock.
>> Either the
>> - * caller has ref on the inode (either via __iget or via syscall
>> against an fd)
>> - * or the inode has I_WILL_FREE set (via generic_forget_inode)
>> + * caller has ref on the inode or the inode is beeing freed.
being
[...]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] fs: do not drop inode_lock in dispose_list
2010-10-24 17:40 [PATCH 1/4] fs: do not drop inode_lock in dispose_list Christoph Hellwig
` (2 preceding siblings ...)
2010-10-24 17:40 ` [PATCH 4/4] fs: kill I_WILL_FREE Christoph Hellwig
@ 2010-10-25 5:33 ` Dave Chinner
2010-10-25 5:46 ` Dave Chinner
3 siblings, 1 reply; 15+ messages in thread
From: Dave Chinner @ 2010-10-25 5:33 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: viro, linux-fsdevel
On Sun, Oct 24, 2010 at 07:40:24PM +0200, Christoph Hellwig wrote:
>
> Despite the comment above it we can not safely drop the lock here.
> invalidate_list is called from many other places that just umount.
> Also switch to proper list macros now that we never drop the lock.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
What tree are these patches against - a couple of them don't quite
apply to Al's merge-stem tree...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] fs: do not drop inode_lock in dispose_list
2010-10-25 5:33 ` [PATCH 1/4] fs: do not drop inode_lock in dispose_list Dave Chinner
@ 2010-10-25 5:46 ` Dave Chinner
2010-10-25 9:20 ` Dave Chinner
2010-10-25 10:07 ` Christoph Hellwig
0 siblings, 2 replies; 15+ messages in thread
From: Dave Chinner @ 2010-10-25 5:46 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: viro, linux-fsdevel
On Mon, Oct 25, 2010 at 04:33:53PM +1100, Dave Chinner wrote:
> On Sun, Oct 24, 2010 at 07:40:24PM +0200, Christoph Hellwig wrote:
> >
> > Despite the comment above it we can not safely drop the lock here.
> > invalidate_list is called from many other places that just umount.
> > Also switch to proper list macros now that we never drop the lock.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> What tree are these patches against - a couple of them don't quite
> apply to Al's merge-stem tree...
Not sure whether it's my merge, something in Al's tree series or
something brought in from the post-2.6.36 tree, but it goes splat
pretty quickly (xfstest 013) with:
[ 302.233778] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
[ 302.235129] IP: [<ffffffff8114cc3a>] __put_super+0x2a/0x80
[ 302.235997] PGD 7a98a067 PUD 7a98b067 PMD 0
[ 302.236024] Oops: 0002 [#1] SMP
[ 302.236024] last sysfs file: /sys/devices/virtio-pci/virtio1/block/vda/removable
[ 302.236024] CPU 1
[ 302.236024] Modules linked in:
[ 302.236024]
[ 302.236024] Pid: 2523, comm: flush-253:0 Not tainted 2.6.36-dgc+ #577 /Bochs
[ 302.236024] RIP: 0010:[<ffffffff8114cc3a>] [<ffffffff8114cc3a>] __put_super+0x2a/0x80
[ 302.236024] RSP: 0018:ffff88007b6a3c60 EFLAGS: 00010246
[ 302.236024] RAX: ffff88007cf3c820 RBX: ffff88007c3e1000 RCX: ffff88007b6a3c60
[ 302.236024] RDX: 0000000000000000 RSI: ffff88007b1f5220 RDI: ffff88007c3e1000
[ 302.236024] RBP: ffff88007b6a3c70 R08: ffff88007b6a3c60 R09: ffff88007c1eb800
[ 302.236024] R10: ffff88007b6a3c60 R11: 0000000000000001 R12: ffff88007c199778
[ 302.236024] R13: ffff88007619b1d0 R14: ffff88007c199808 R15: ffffffff81d427e0
[ 302.236024] FS: 0000000000000000(0000) GS:ffff88007fd00000(0000) knlGS:0000000000000000
[ 302.236024] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 302.236024] CR2: 0000000000000008 CR3: 000000007a989000 CR4: 00000000000006e0
[ 302.236024] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 302.236024] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 302.236024] Process flush-253:0 (pid: 2523, threadinfo ffff88007b6a2000, task ffff88007c91b040)
[ 302.236024] Stack:
[ 302.236024] ffff88007b6a3c70 ffff88007c3e1000 ffff88007b6a3c90 ffffffff8114d135
[ 302.236024] <0> ffff88007b6a3c90 ffff88007c3e1000 ffff88007b6a3ce0 ffffffff8116e010
[ 302.236024] <0> ffff88007b6a3d30 ffff88007c3e1068 0000000000000000 ffff88007b461ec0
[ 302.236024] Call Trace:
[ 302.236024] [<ffffffff8114d135>] put_super+0x25/0x40
[ 302.236024] [<ffffffff8116e010>] writeback_inodes_wb+0x130/0x180
[ 302.236024] [<ffffffff8116e2fb>] wb_writeback+0x29b/0x410
[ 302.236024] [<ffffffff8105fb89>] ? default_spin_lock_flags+0x9/0x10
[ 302.236024] [<ffffffff817f6a4f>] ? _raw_spin_lock_irqsave+0x2f/0x40
[ 302.236024] [<ffffffff8116e507>] wb_do_writeback+0x97/0x1e0
[ 302.236024] [<ffffffff8116e702>] bdi_writeback_thread+0xb2/0x270
[ 302.236024] [<ffffffff8116e650>] ? bdi_writeback_thread+0x0/0x270
[ 302.236024] [<ffffffff8109f976>] kthread+0x96/0xa0
[ 302.236024] [<ffffffff81036de4>] kernel_thread_helper+0x4/0x10
[ 302.236024] [<ffffffff8109f8e0>] ? kthread+0x0/0xa0
[ 302.236024] [<ffffffff81036de0>] ? kernel_thread_helper+0x0/0x10
[ 302.236024] Code: 90 55 48 89 e5 53 48 83 ec 08 0f 1f 44 00 00 8b 87 a8 00 00 00 48 89 fb ff c8 89 87 a8 00 00 00 85 c0 75
[ 302.236024] RIP [<ffffffff8114cc3a>] __put_super+0x2a/0x80
[ 302.236024] RSP <ffff88007b6a3c60>
[ 302.236024] CR2: 0000000000000008
[ 302.265338] ---[ end trace 64118e3d92267d36 ]---
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] fs: do not drop inode_lock in dispose_list
2010-10-25 5:46 ` Dave Chinner
@ 2010-10-25 9:20 ` Dave Chinner
2010-10-25 10:07 ` Christoph Hellwig
1 sibling, 0 replies; 15+ messages in thread
From: Dave Chinner @ 2010-10-25 9:20 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: viro, linux-fsdevel
On Mon, Oct 25, 2010 at 04:46:11PM +1100, Dave Chinner wrote:
> On Mon, Oct 25, 2010 at 04:33:53PM +1100, Dave Chinner wrote:
> > On Sun, Oct 24, 2010 at 07:40:24PM +0200, Christoph Hellwig wrote:
> > >
> > > Despite the comment above it we can not safely drop the lock here.
> > > invalidate_list is called from many other places that just umount.
> > > Also switch to proper list macros now that we never drop the lock.
> > >
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >
> > What tree are these patches against - a couple of them don't quite
> > apply to Al's merge-stem tree...
>
> Not sure whether it's my merge, something in Al's tree series or
> something brought in from the post-2.6.36 tree, but it goes splat
> pretty quickly (xfstest 013) with:
>
> [ 302.233778] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> [ 302.235129] IP: [<ffffffff8114cc3a>] __put_super+0x2a/0x80
> [ 302.235997] PGD 7a98a067 PUD 7a98b067 PMD 0
> [ 302.236024] Oops: 0002 [#1] SMP
> [ 302.236024] last sysfs file: /sys/devices/virtio-pci/virtio1/block/vda/removable
> [ 302.236024] CPU 1
> [ 302.236024] Modules linked in:
> [ 302.236024]
> [ 302.236024] Pid: 2523, comm: flush-253:0 Not tainted 2.6.36-dgc+ #577 /Bochs
> [ 302.236024] RIP: 0010:[<ffffffff8114cc3a>] [<ffffffff8114cc3a>] __put_super+0x2a/0x80
> [ 302.236024] RSP: 0018:ffff88007b6a3c60 EFLAGS: 00010246
> [ 302.236024] RAX: ffff88007cf3c820 RBX: ffff88007c3e1000 RCX: ffff88007b6a3c60
> [ 302.236024] RDX: 0000000000000000 RSI: ffff88007b1f5220 RDI: ffff88007c3e1000
> [ 302.236024] RBP: ffff88007b6a3c70 R08: ffff88007b6a3c60 R09: ffff88007c1eb800
> [ 302.236024] R10: ffff88007b6a3c60 R11: 0000000000000001 R12: ffff88007c199778
> [ 302.236024] R13: ffff88007619b1d0 R14: ffff88007c199808 R15: ffffffff81d427e0
> [ 302.236024] FS: 0000000000000000(0000) GS:ffff88007fd00000(0000) knlGS:0000000000000000
> [ 302.236024] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [ 302.236024] CR2: 0000000000000008 CR3: 000000007a989000 CR4: 00000000000006e0
> [ 302.236024] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 302.236024] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [ 302.236024] Process flush-253:0 (pid: 2523, threadinfo ffff88007b6a2000, task ffff88007c91b040)
> [ 302.236024] Stack:
> [ 302.236024] ffff88007b6a3c70 ffff88007c3e1000 ffff88007b6a3c90 ffffffff8114d135
> [ 302.236024] <0> ffff88007b6a3c90 ffff88007c3e1000 ffff88007b6a3ce0 ffffffff8116e010
> [ 302.236024] <0> ffff88007b6a3d30 ffff88007c3e1068 0000000000000000 ffff88007b461ec0
> [ 302.236024] Call Trace:
> [ 302.236024] [<ffffffff8114d135>] put_super+0x25/0x40
> [ 302.236024] [<ffffffff8116e010>] writeback_inodes_wb+0x130/0x180
> [ 302.236024] [<ffffffff8116e2fb>] wb_writeback+0x29b/0x410
> [ 302.236024] [<ffffffff8105fb89>] ? default_spin_lock_flags+0x9/0x10
> [ 302.236024] [<ffffffff817f6a4f>] ? _raw_spin_lock_irqsave+0x2f/0x40
> [ 302.236024] [<ffffffff8116e507>] wb_do_writeback+0x97/0x1e0
> [ 302.236024] [<ffffffff8116e702>] bdi_writeback_thread+0xb2/0x270
> [ 302.236024] [<ffffffff8116e650>] ? bdi_writeback_thread+0x0/0x270
> [ 302.236024] [<ffffffff8109f976>] kthread+0x96/0xa0
> [ 302.236024] [<ffffffff81036de4>] kernel_thread_helper+0x4/0x10
> [ 302.236024] [<ffffffff8109f8e0>] ? kthread+0x0/0xa0
> [ 302.236024] [<ffffffff81036de0>] ? kernel_thread_helper+0x0/0x10
> [ 302.236024] Code: 90 55 48 89 e5 53 48 83 ec 08 0f 1f 44 00 00 8b 87 a8 00 00 00 48 89 fb ff c8 89 87 a8 00 00 00 85 c0 75
> [ 302.236024] RIP [<ffffffff8114cc3a>] __put_super+0x2a/0x80
> [ 302.236024] RSP <ffff88007b6a3c60>
> [ 302.236024] CR2: 0000000000000008
> [ 302.265338] ---[ end trace 64118e3d92267d36 ]---
This is coming form Al's tree, so it's not these specific patches
that are triggering it.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] fs: do not drop inode_lock in dispose_list
2010-10-25 5:46 ` Dave Chinner
2010-10-25 9:20 ` Dave Chinner
@ 2010-10-25 10:07 ` Christoph Hellwig
2010-10-25 23:07 ` Dave Chinner
1 sibling, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2010-10-25 10:07 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, viro, linux-fsdevel
On Mon, Oct 25, 2010 at 04:46:11PM +1100, Dave Chinner wrote:
> Not sure whether it's my merge, something in Al's tree series or
> something brought in from the post-2.6.36 tree, but it goes splat
> pretty quickly (xfstest 013) with:
That's because the version of the separate wb and lru lists patch that
Al took from your tree was missing various deletions from the wb list.
Below is the fix I sent him, not sure why it didn't make it to the list:
Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c 2010-10-24 16:30:07.592253652 +0200
+++ linux-2.6/fs/inode.c 2010-10-24 16:31:31.132003756 +0200
@@ -509,8 +509,14 @@ static int invalidate_list(struct list_h
continue;
}
- list_move(&inode->i_lru, dispose);
inode->i_state |= I_FREEING;
+
+ /*
+ * Move the inode off the IO lists and LRU once I_FREEING is
+ * set so that it won't get moved back on there if it is dirty.
+ */
+ list_move(&inode->i_lru, dispose);
+ list_del_init(&inode->i_wb_list);
if (!(inode->i_state & (I_DIRTY | I_SYNC)))
percpu_counter_dec(&nr_inodes_unused);
}
@@ -620,9 +626,15 @@ static void prune_icache(int nr_to_scan)
if (!can_unuse(inode))
continue;
}
- list_move(&inode->i_lru, &freeable);
WARN_ON(inode->i_state & I_NEW);
inode->i_state |= I_FREEING;
+
+ /*
+ * Move the inode off the IO lists and LRU once I_FREEING is
+ * set so that it won't get moved back on there if it is dirty.
+ */
+ list_move(&inode->i_lru, &freeable);
+ list_del_init(&inode->i_wb_list);
percpu_counter_dec(&nr_inodes_unused);
}
if (current_is_kswapd())
@@ -1343,16 +1355,16 @@ static void iput_final(struct inode *ino
inode->i_state &= ~I_WILL_FREE;
__remove_inode_hash(inode);
}
- list_del_init(&inode->i_wb_list);
+
WARN_ON(inode->i_state & I_NEW);
inode->i_state |= I_FREEING;
/*
- * After we delete the inode from the LRU here, we avoid moving dirty
- * inodes back onto the LRU now because I_FREEING is set and hence
- * writeback_single_inode() won't move the inode around.
+ * Move the inode off the IO lists and LRU once I_FREEING is
+ * set so that it won't get moved back on there if it is dirty.
*/
inode_lru_list_del(inode);
+ list_del_init(&inode->i_wb_list);
__inode_sb_list_del(inode);
spin_unlock(&inode_lock);
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] fs: do not drop inode_lock in dispose_list
2010-10-25 10:07 ` Christoph Hellwig
@ 2010-10-25 23:07 ` Dave Chinner
0 siblings, 0 replies; 15+ messages in thread
From: Dave Chinner @ 2010-10-25 23:07 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: viro, linux-fsdevel
On Mon, Oct 25, 2010 at 12:07:58PM +0200, Christoph Hellwig wrote:
> On Mon, Oct 25, 2010 at 04:46:11PM +1100, Dave Chinner wrote:
> > Not sure whether it's my merge, something in Al's tree series or
> > something brought in from the post-2.6.36 tree, but it goes splat
> > pretty quickly (xfstest 013) with:
>
> That's because the version of the separate wb and lru lists patch that
> Al took from your tree was missing various deletions from the wb list.
>
> Below is the fix I sent him, not sure why it didn't make it to the list:
Fix works just fine. Thanks.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] fs: kill I_WILL_FREE
2010-10-24 17:40 ` [PATCH 4/4] fs: kill I_WILL_FREE Christoph Hellwig
2010-10-24 21:46 ` Christian Stroetmann
@ 2010-10-26 1:28 ` Al Viro
2010-10-26 19:18 ` Al Viro
2 siblings, 0 replies; 15+ messages in thread
From: Al Viro @ 2010-10-26 1:28 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-fsdevel
On Sun, Oct 24, 2010 at 07:40:58PM +0200, Christoph Hellwig wrote:
> The I_WILL_FREE is currently set for inodes that we write out during
> umount after dropping their last reference. It is handled equally
> to I_FREEING in most places. The two execptions are:
>
> - writeback_single_inode skips all list manipulations for I_FREEING,
> but not for I_WILL_FREE. We don't care about which list an
> I_WILL_FREE inode is on, because we will remove it from the list
> a little bit later.
> - __mark_inode_dirty skips I_FREEING inodes but not I_WILL_FREE
> inodes. This only matters for filesystem that re-dirty the inode
> during writeback and then use the I_DIRTY flags inside ->evict_inode.
> The formers is done by XFS, but it uses it's internal state to flush
> the inode. I could not find any filesystem that looks at I_DIRTY
> inside ->evict_inode either.
>
> Besides cleaning up the code removing I_WILL_FREE will allow us to
> avoid one i_lock roundtrip once inode_lock is split and keep iput_final
> more logic. This includes removing the __remove_inode_hash call in
> iput_final, given that we never drop the protection from lookups now
> that I_FREEING is set earlier.
All except that one applied, exofs patch dropped, fixes folded, etc.
Branch pushed in the same place...
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] fs: kill I_WILL_FREE
2010-10-24 17:40 ` [PATCH 4/4] fs: kill I_WILL_FREE Christoph Hellwig
2010-10-24 21:46 ` Christian Stroetmann
2010-10-26 1:28 ` Al Viro
@ 2010-10-26 19:18 ` Al Viro
2 siblings, 0 replies; 15+ messages in thread
From: Al Viro @ 2010-10-26 19:18 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-fsdevel
On Sun, Oct 24, 2010 at 07:40:58PM +0200, Christoph Hellwig wrote:
> - writeback_single_inode skips all list manipulations for I_FREEING,
> but not for I_WILL_FREE. We don't care about which list an
> I_WILL_FREE inode is on, because we will remove it from the list
> a little bit later.
> - __mark_inode_dirty skips I_FREEING inodes but not I_WILL_FREE
> inodes. This only matters for filesystem that re-dirty the inode
> during writeback and then use the I_DIRTY flags inside ->evict_inode.
> The formers is done by XFS, but it uses it's internal state to flush
> the inode. I could not find any filesystem that looks at I_DIRTY
> inside ->evict_inode either.
>
> Besides cleaning up the code removing I_WILL_FREE will allow us to
> avoid one i_lock roundtrip once inode_lock is split and keep iput_final
> more logic. This includes removing the __remove_inode_hash call in
> iput_final, given that we never drop the protection from lookups now
> that I_FREEING is set earlier.
The thing is, the code is really brittle in that area. What we rely upon
is
* due to sync_filesystem() done just before there won't be anything
dirty during invalidate_inodes() (OK, evict_inodes() now) run.
* for final iput done during ->put_super() we won't redirty the
inode. Note that we used to loop in write_one_inode() until the sucker
got clean. Not anymore (since 2002 or so).
* XFS does redirty, but then it does explicit write of those guys
itself (from ->put_super()).
Let's leave that alone for now; I remember quite well how much PITA has
that area caused us in the past (my fuckup in 2.4.15, etc.) and I'd rather
not mix revisiting that fun place with other non-trivial work.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2010-10-26 19:18 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-24 17:40 [PATCH 1/4] fs: do not drop inode_lock in dispose_list Christoph Hellwig
2010-10-24 17:40 ` [PATCH 2/4] fs: fold invalidate_list into invalidate_inodes Christoph Hellwig
2010-10-24 21:45 ` Christian Stroetmann
2010-10-24 17:40 ` [PATCH 3/4] fs: skip I_FREEING inodes in writeback_sb_inodes Christoph Hellwig
2010-10-24 21:46 ` Christian Stroetmann
2010-10-24 17:40 ` [PATCH 4/4] fs: kill I_WILL_FREE Christoph Hellwig
2010-10-24 21:46 ` Christian Stroetmann
2010-10-24 21:50 ` Christian Stroetmann
2010-10-26 1:28 ` Al Viro
2010-10-26 19:18 ` Al Viro
2010-10-25 5:33 ` [PATCH 1/4] fs: do not drop inode_lock in dispose_list Dave Chinner
2010-10-25 5:46 ` Dave Chinner
2010-10-25 9:20 ` Dave Chinner
2010-10-25 10:07 ` Christoph Hellwig
2010-10-25 23:07 ` Dave Chinner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.