super block scalabilit patches V3
Subsystems (6)
cover super block scalabilit patches V3
Here are the cleaned up versions of Dave Chinners super block scalability patches. I've been testing them locally for a while and they are pretty solid. They fix a few big issues, such as the global inode list and soft lockups on boxes on unmount that have lots of inodes in cache. Al if you would consider pulling these in that would be great, you can pull from here
git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.git superblock-scaling
Thanks,
Josef
PATCH 1/8 writeback: plug writeback at a high level
From: Dave Chinner <[email protected]>
Doing writeback on lots of little files causes terrible IOPS storms because of the per-mapping writeback plugging we do. This essentially causes imeediate dispatch of IO for each mapping, regardless of the context in which writeback is occurring.
IOWs, running a concurrent write-lots-of-small 4k files using fsmark on XFS results in a huge number of IOPS being issued for data writes. Metadata writes are sorted and plugged at a high level by XFS, so aggregate nicely into large IOs. However, data writeback IOs are dispatched in individual 4k IOs, even when the blocks of two consecutively written files are adjacent.
Test VM: 8p, 8GB RAM, 4xSSD in RAID0, 100TB sparse XFS filesystem, metadata CRCs enabled.
Kernel: 3.10-rc5 + xfsdev + my 3.11 xfs queue (~70 patches)
Test:
$ ./fs_mark -D 10000 -S0 -n 10000 -s 4096 -L 120 -d /mnt/scratch/0 -d /mnt/scratch/1 -d /mnt/scratch/2 -d /mnt/scratch/3 -d /mnt/scratch/4 -d /mnt/scratch/5 -d /mnt/scratch/6 -d /mnt/scratch/7
Result:
wall sys create rate Physical write IO
time CPU (avg files/s) IOPS Bandwidth
----- ----- ------------ ------ ---------
unpatched 6m56s 15m47s 24,000+/-500 26,000 130MB/s
patched 5m06s 13m28s 32,800+/-600 1,500 180MB/s
improvement -26.44% -14.68% +36.67% -94.23% +38.46%
If I use zero length files, this workload at about 500 IOPS, so plugging drops the data IOs from roughly 25,500/s to 1000/s. 3 lines of code, 35% better throughput for 15% less CPU.
The benefits of plugging at this layer are likely to be higher for spinning media as the IO patterns for this workload are going make a much bigger difference on high IO latency devices.....
--- fs/fs-writeback.c | 3 +++ 1 file changed, 3 insertions(+)
fs/fs-writeback.c +3 -0
@@ -677,7 +677,9 @@ static long writeback_sb_inodes(struct super_block *sb,unsigned long start_time = jiffies;long write_chunk;long wrote =0 ;/* count both pages and inodes */ +struct blk_plug plug; +blk_start_plug (&plug);while (!list_empty (&wb->b_io)) {struct inode *inode =wb_inode (wb->b_io.prev);
@@ -774,6 +776,7 @@ static long writeback_sb_inodes(struct super_block *sb,break ; } } +blk_finish_plug (&plug);return wrote; }
Signature · 1 line
2.1.0
Re: [PATCH 1/8] writeback: plug writeback at a high level
Looks good,
PATCH 2/8 inode: add hlist_fake to avoid the inode hash lock in evict
In reply to Josef BacikSome filesystems don't use the VFS inode hash and fake the fact they are hashed so that all the writeback code works correctly. However, this means the evict() path still tries to remove the inode from the hash, meaning that the inode_hash_lock() needs to be taken unnecessarily. Hence under certain workloads the inode_hash_lock can be contended even if the inode is never actually hashed.
To avoid this add hlist_fake to test if the inode isn't actually hashed to avoid taking the hash lock on inodes that have never been hashed. Based on Dave Chinner's
inode: add IOP_NOTHASHED to avoid inode hash lock in evict
basd on Al's suggestions. Thanks,
--- include/linux/fs.h | 2 +- include/linux/list.h | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
include/linux/fs.h +1 -1
@@ -2563,7 +2563,7 @@ static inline void insert_inode_hash(struct inode *inode)extern void __remove_inode_hash (struct inode *);static inline void remove_inode_hash (struct inode *inode) { -if (!inode_unhashed (inode)) +if (!inode_unhashed (inode) && !hlist_fake (&inode->i_hash))__remove_inode_hash (inode); }
include/linux/list.h +5 -0
@@ -672,6 +672,11 @@ static inline void hlist_add_fake(struct hlist_node *n) n->pprev = &n->next; } +static inline bool hlist_fake (struct hlist_node *h) +{ +return h->pprev == &h->next; +} +/* * Move a list from one list head to another. Fixup the pprev * reference of the first entry if it exists.
Signature · 1 line
2.1.0
Re: [PATCH 2/8] inode: add hlist_fake to avoid the inode hash lock in evict
Looks good,
PATCH 3/8 inode: convert inode_sb_list_lock to per-sb
In reply to Josef BacikFrom: Dave Chinner <[email protected]>
The process of reducing contention on per-superblock inode lists starts with moving the locking to match the per-superblock inode list. This takes the global lock out of the picture and reduces the contention problems to within a single filesystem. This doesn't get rid of contention as the locks still have global CPU scope, but it does isolate operations on different superblocks form each other.
--- fs/block_dev.c | 12 ++++++------ fs/drop_caches.c | 10 ++++++---- fs/fs-writeback.c | 12 ++++++------ fs/inode.c | 28 +++++++++++++--------------- fs/internal.h | 1 - fs/notify/inode_mark.c | 20 ++++++++++---------- fs/quota/dquot.c | 16 ++++++++-------- fs/super.c | 3 ++- include/linux/fs.h | 5 ++++- include/linux/fsnotify_backend.h | 4 ++-- 10 files changed, 57 insertions(+), 54 deletions(-)
fs/block_dev.c +6 -6
@@ -1762,7 +1762,7 @@ void iterate_bdevs(void (*func)(struct block_device *, void *), void *arg) {struct inode *inode, *old_inode =NULL ; -spin_lock (&inode_sb_list_lock); +spin_lock (&blockdev_superblock->s_inode_list_lock);list_for_each_entry (inode, &blockdev_superblock->s_inodes, i_sb_list) {struct address_space *mapping = inode->i_mapping;
@@ -1774,13 +1774,13 @@ void iterate_bdevs(void (*func)(struct block_device *, void *), void *arg) }__iget (inode);spin_unlock (&inode->i_lock); -spin_unlock (&inode_sb_list_lock); +spin_unlock (&blockdev_superblock->s_inode_list_lock);/* * We hold a reference to 'inode' so it couldn't have been * removed from s_inodes list while we dropped the -* inode_sb_list_lock. We cannot iput the inode now as we can +* s_inode_list_lock We cannot iput the inode now as we can * be holding the last reference and we cannot iput it under -* inode_sb_list_lock. So we keep the reference and iput it +* s_inode_list_lock. So we keep the reference and iput it * later. */ iput (old_inode);
@@ -1788,8 +1788,8 @@ void iterate_bdevs(void (*func)(struct block_device *, void *), void *arg)func (I_BDEV (inode), arg); -spin_lock (&inode_sb_list_lock); +spin_lock (&blockdev_superblock->s_inode_list_lock); } -spin_unlock (&inode_sb_list_lock); +spin_unlock (&blockdev_superblock->s_inode_list_lock);iput (old_inode); }
fs/drop_caches.c +6 -4
@@ -17,7 +17,7 @@ static void drop_pagecache_sb(struct super_block *sb, void *unused) {struct inode *inode, *toput_inode =NULL ; -spin_lock (&inode_sb_list_lock); +spin_lock (&sb->s_inode_list_lock);list_for_each_entry (inode, &sb->s_inodes, i_sb_list) {spin_lock (&inode->i_lock); if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) ||
@@ -27,13 +27,15 @@ static void drop_pagecache_sb(struct super_block *sb, void *unused) }__iget (inode);spin_unlock (&inode->i_lock); -spin_unlock (&inode_sb_list_lock); +spin_unlock (&sb->s_inode_list_lock); +invalidate_mapping_pages (inode->i_mapping,0 ,-1 );iput (toput_inode); toput_inode = inode; -spin_lock (&inode_sb_list_lock); + +spin_lock (&sb->s_inode_list_lock); } -spin_unlock (&inode_sb_list_lock); +spin_unlock (&sb->s_inode_list_lock);iput (toput_inode); }
fs/fs-writeback.c +6 -6
@@ -1371,7 +1371,7 @@ static void wait_sb_inodes(struct super_block *sb)*/ WARN_ON (!rwsem_is_locked (&sb->s_umount)); -spin_lock (&inode_sb_list_lock); +spin_lock (&sb->s_inode_list_lock);/* * Data integrity sync. Must wait for all pages under writeback,
@@ -1391,14 +1391,14 @@ static void wait_sb_inodes(struct super_block *sb) }__iget (inode);spin_unlock (&inode->i_lock); -spin_unlock (&inode_sb_list_lock); +spin_unlock (&sb->s_inode_list_lock);/* * We hold a reference to 'inode' so it couldn't have been * removed from s_inodes list while we dropped the -* inode_sb_list_lock. We cannot iput the inode now as we can +* s_inode_list_lock. We cannot iput the inode now as we can * be holding the last reference and we cannot iput it under -* inode_sb_list_lock. So we keep the reference and iput it +* s_inode_list_lock. So we keep the reference and iput it * later. */ iput (old_inode);
@@ -1408,9 +1408,9 @@ static void wait_sb_inodes(struct super_block *sb)cond_resched (); -spin_lock (&inode_sb_list_lock); +spin_lock (&sb->s_inode_list_lock); } -spin_unlock (&inode_sb_list_lock); +spin_unlock (&sb->s_inode_list_lock);iput (old_inode); }
fs/inode.c +13 -15
@@ -28,8 +28,8 @@* inode->i_state, inode->i_hash, __iget() * Inode LRU list locks protect: * inode->i_sb->s_inode_lru, inode->i_lru -* inode_sb_list_lock protects: -* sb->s_inodes, inode->i_sb_list +* inode->i_sb->s_inode_list_lock protects: +* inode->i_sb->s_inodes, inode->i_sb_list * bdi->wb.list_lock protects: * bdi->wb.b_{dirty,io,more_io,dirty_time}, inode->i_wb_list * inode_hash_lock protects:
@@ -37,7 +37,7 @@* * Lock ordering: * -* inode_sb_list_lock +* inode->i_sb->s_inode_list_lock * inode->i_lock * Inode LRU list locks *
@@ -45,7 +45,7 @@* inode->i_lock * * inode_hash_lock -* inode_sb_list_lock +* inode->i_sb->s_inode_list_lock * inode->i_lock * * iunique_lock
@@ -57,8 +57,6 @@ static unsigned int i_hash_shift __read_mostly;static struct hlist_head *inode_hashtable__read_mostly ;static __cacheline_aligned_in_smp DEFINE_SPINLOCK (inode_hash_lock ); -__cacheline_aligned_in_smp DEFINE_SPINLOCK (inode_sb_list_lock ); -/* * Empty aops. Can be used for the cases where the user does not * define any of the address_space operations.
@@ -424,18 +422,18 @@ static void inode_lru_list_del(struct inode *inode)*/ void inode_sb_list_add (struct inode *inode) { -spin_lock (&inode_sb_list_lock); +spin_lock (&inode->i_sb->s_inode_list_lock);list_add (&inode->i_sb_list, &inode->i_sb->s_inodes); -spin_unlock (&inode_sb_list_lock); +spin_unlock (&inode->i_sb->s_inode_list_lock); }EXPORT_SYMBOL_GPL (inode_sb_list_add);static inline void inode_sb_list_del (struct inode *inode) {if (!list_empty (&inode->i_sb_list)) { -spin_lock (&inode_sb_list_lock); +spin_lock (&inode->i_sb->s_inode_list_lock);list_del_init (&inode->i_sb_list); -spin_unlock (&inode_sb_list_lock); +spin_unlock (&inode->i_sb->s_inode_list_lock); } }
@@ -592,7 +590,7 @@ void evict_inodes(struct super_block *sb)struct inode *inode, *next;LIST_HEAD (dispose); -spin_lock (&inode_sb_list_lock); +spin_lock (&sb->s_inode_list_lock);list_for_each_entry_safe (inode, next, &sb->s_inodes, i_sb_list) {if (atomic_read (&inode->i_count))continue ;
@@ -608,7 +606,7 @@ void evict_inodes(struct super_block *sb)spin_unlock (&inode->i_lock);list_add (&inode->i_lru, &dispose); } -spin_unlock (&inode_sb_list_lock); +spin_unlock (&sb->s_inode_list_lock);dispose_list (&dispose); }
@@ -629,7 +627,7 @@ int invalidate_inodes(struct super_block *sb, bool kill_dirty)struct inode *inode, *next;LIST_HEAD (dispose); -spin_lock (&inode_sb_list_lock); +spin_lock (&sb->s_inode_list_lock);list_for_each_entry_safe (inode, next, &sb->s_inodes, i_sb_list) {spin_lock (&inode->i_lock);if (inode->i_state & (I_NEW |I_FREEING |I_WILL_FREE )) {
@@ -652,7 +650,7 @@ int invalidate_inodes(struct super_block *sb, bool kill_dirty)spin_unlock (&inode->i_lock);list_add (&inode->i_lru, &dispose); } -spin_unlock (&inode_sb_list_lock); +spin_unlock (&sb->s_inode_list_lock);dispose_list (&dispose);
@@ -884,7 +882,7 @@ struct inode *new_inode(struct super_block *sb) {struct inode *inode; -spin_lock_prefetch (&inode_sb_list_lock); +spin_lock_prefetch (&sb->s_inode_list_lock); inode =new_inode_pseudo (sb);if (inode)
fs/internal.h +0 -1
@@ -111,7 +111,6 @@ extern int open_check_o_direct(struct file *f);/* * inode.c */ -extern spinlock_t inode_sb_list_lock;extern long prune_icache_sb (struct super_block *sb,struct shrink_control *sc);extern void inode_add_lru (struct inode *inode);
fs/notify/inode_mark.c +10 -10
@@ -163,17 +163,17 @@ int fsnotify_add_inode_mark(struct fsnotify_mark *mark,/** * fsnotify_unmount_inodes - an sb is unmounting. handle any watched inodes. -* @list: list of inodes being unmounted (sb->s_inodes) +* @sb: superblock being unmounted. * * Called during unmount with no locks held, so needs to be safe against -* concurrent modifiers. We temporarily drop inode_sb_list_lock and CAN block. +* concurrent modifiers. We temporarily drop sb->s_inode_list_lock and CAN block. */ -void fsnotify_unmount_inodes (struct list_head *list) +void fsnotify_unmount_inodes (struct super_block *sb) {struct inode *inode, *next_i, *need_iput =NULL ; -spin_lock (&inode_sb_list_lock); -list_for_each_entry_safe (inode, next_i, list, i_sb_list) { +spin_lock (&sb->s_inode_list_lock); +list_for_each_entry_safe (inode, next_i, &sb->s_inodes, i_sb_list) {struct inode *need_iput_tmp;/*
@@ -209,7 +209,7 @@ void fsnotify_unmount_inodes(struct list_head *list)spin_unlock (&inode->i_lock);/* In case the dropping of a reference would nuke next_i. */ -while (&next_i->i_sb_list != list) { +while (&next_i->i_sb_list != &sb->s_inodes) {spin_lock (&next_i->i_lock);if (!(next_i->i_state & (I_FREEING |I_WILL_FREE )) &&atomic_read (&next_i->i_count)) {
@@ -224,12 +224,12 @@ void fsnotify_unmount_inodes(struct list_head *list) }/* -* We can safely drop inode_sb_list_lock here because either +* We can safely drop s_inode_list_lock here because either * we actually hold references on both inode and next_i or * end of list. Also no new inodes will be added since the * umount has begun. */ -spin_unlock (&inode_sb_list_lock); +spin_unlock (&sb->s_inode_list_lock);if (need_iput_tmp)iput (need_iput_tmp);
@@ -241,7 +241,7 @@ void fsnotify_unmount_inodes(struct list_head *list)iput (inode); -spin_lock (&inode_sb_list_lock); +spin_lock (&sb->s_inode_list_lock); } -spin_unlock (&inode_sb_list_lock); +spin_unlock (&sb->s_inode_list_lock); }
fs/quota/dquot.c +8 -8
@@ -923,7 +923,7 @@ static void add_dquot_ref(struct super_block *sb, int type)int reserved =0 ;#endif -spin_lock (&inode_sb_list_lock); +spin_lock (&sb->s_inode_list_lock);list_for_each_entry (inode, &sb->s_inodes, i_sb_list) {spin_lock (&inode->i_lock); if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) ||
@@ -934,7 +934,7 @@ static void add_dquot_ref(struct super_block *sb, int type) }__iget (inode);spin_unlock (&inode->i_lock); -spin_unlock (&inode_sb_list_lock); +spin_unlock (&sb->s_inode_list_lock);#ifdef CONFIG_QUOTA_DEBUG if (unlikely (inode_get_rsv_space (inode) >0 ))
@@ -946,15 +946,15 @@ static void add_dquot_ref(struct super_block *sb, int type)/* * We hold a reference to 'inode' so it couldn't have been * removed from s_inodes list while we dropped the -* inode_sb_list_lock We cannot iput the inode now as we can be +* s_inode_list_lock. We cannot iput the inode now as we can be * holding the last reference and we cannot iput it under -* inode_sb_list_lock. So we keep the reference and iput it +* s_inode_list_lock. So we keep the reference and iput it * later. */ old_inode = inode; -spin_lock (&inode_sb_list_lock); +spin_lock (&sb->s_inode_list_lock); } -spin_unlock (&inode_sb_list_lock); +spin_unlock (&sb->s_inode_list_lock);iput (old_inode);#ifdef CONFIG_QUOTA_DEBUG
@@ -1023,7 +1023,7 @@ static void remove_dquot_ref(struct super_block *sb, int type,struct inode *inode;int reserved =0 ; -spin_lock (&inode_sb_list_lock); +spin_lock (&sb->s_inode_list_lock);list_for_each_entry (inode, &sb->s_inodes, i_sb_list) {/* * We have to scan also I_NEW inodes because they can already
@@ -1039,7 +1039,7 @@ static void remove_dquot_ref(struct super_block *sb, int type, }spin_unlock (&dq_data_lock); } -spin_unlock (&inode_sb_list_lock); +spin_unlock (&sb->s_inode_list_lock);#ifdef CONFIG_QUOTA_DEBUG if (reserved) { printk(KERN_WARNING "VFS (%s): Writes happened after quota"
fs/super.c +2 -1
@@ -191,6 +191,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags)INIT_HLIST_NODE (&s->s_instances);INIT_HLIST_BL_HEAD (&s->s_anon);INIT_LIST_HEAD (&s->s_inodes); +spin_lock_init (&s->s_inode_list_lock);if (list_lru_init_memcg (&s->s_dentry_lru)) goto fail;
@@ -399,7 +400,7 @@ void generic_shutdown_super(struct super_block *sb)sync_filesystem (sb); sb->s_flags &= ~MS_ACTIVE ; -fsnotify_unmount_inodes (&sb->s_inodes); +fsnotify_unmount_inodes (sb);evict_inodes (sb);
include/linux/fs.h +4 -1
@@ -1280,7 +1280,6 @@ struct super_block {#endif const struct xattr_handler **s_xattr; -struct list_head s_inodes;/* all inodes */ struct hlist_bl_head s_anon;/* anonymous dentries for (nfs) exporting */ struct list_head s_mounts;/* list of mounts; _not_ for fs use */ struct block_device *s_bdev;
@@ -1351,6 +1350,10 @@ struct super_block {* Indicates how deep in a filesystem stack this SB is */ int s_stack_depth; + +/* s_inode_list_lock protects s_inodes */ +spinlock_t s_inode_list_lock____cacheline_aligned_in_smp ; +struct list_head s_inodes;/* all inodes */ };extern struct timespec current_fs_time (struct super_block *sb);
include/linux/fsnotify_backend.h +2 -2
@@ -359,7 +359,7 @@ extern void fsnotify_clear_marks_by_group_flags(struct fsnotify_group *group, unextern void fsnotify_clear_marks_by_group (struct fsnotify_group *group);extern void fsnotify_get_mark (struct fsnotify_mark *mark);extern void fsnotify_put_mark (struct fsnotify_mark *mark); -extern void fsnotify_unmount_inodes (struct list_head *list); +extern void fsnotify_unmount_inodes (struct super_block *sb);/* put here because inotify does some weird stuff when destroying watches */ extern void fsnotify_init_event(struct fsnotify_event *event,
@@ -395,7 +395,7 @@ static inline u32 fsnotify_get_cookie(void) return 0; } -static inline void fsnotify_unmount_inodes (struct list_head *list) +static inline void fsnotify_unmount_inodes (struct super_block *sb) {}#endif /* CONFIG_FSNOTIFY */
Signature · 1 line
2.1.0
Re: [PATCH 3/8] inode: convert inode_sb_list_lock to per-sb
Quoted 8 lines · Josef Bacik
From: Dave Chinner <[email protected]>
The process of reducing contention on per-superblock inode lists starts with moving the locking to match the per-superblock inode list. This takes the global lock out of the picture and reduces the contention problems to within a single filesystem. This doesn't get rid of contention as the locks still have global CPU scope, but it does isolate operations on different superblocks form each other.
Looks good,
PATCH 4/8 sync: serialise per-superblock sync operations
In reply to Josef BacikFrom: Dave Chinner <[email protected]>
When competing sync(2) calls walk the same filesystem, they need to walk the list of inodes on the superblock to find all the inodes that we need to wait for IO completion on. However, when multiple wait_sb_inodes() calls do this at the same time, they contend on the the inode_sb_list_lock and the contention causes system wide slowdowns. In effect, concurrent sync(2) calls can take longer and burn more CPU than if they were serialised.
Stop the worst of the contention by adding a per-sb mutex to wrap around wait_sb_inodes() so that we only execute one sync(2) IO completion walk per superblock superblock at a time and hence avoid contention being triggered by concurrent sync(2) calls.
--- fs/fs-writeback.c | 11 +++++++++++ fs/super.c | 1 + include/linux/fs.h | 2 ++ 3 files changed, 14 insertions(+)
fs/fs-writeback.c +11 -0
@@ -1361,6 +1361,15 @@ out_unlock_inode: }EXPORT_SYMBOL (__mark_inode_dirty); +/* +* The @s_sync_lock is used to serialise concurrent sync operations +* to avoid lock contention problems with concurrent wait_sb_inodes() calls. +* Concurrent callers will block on the s_sync_lock rather than doing contending +* walks. The queueing maintains sync(2) required behaviour as all the IO that +* has been issued up to the time this function is enter is guaranteed to be +* completed by the time we have gained the lock and waited for all IO that is +* in progress regardless of the order callers are granted the lock. +*/ static void wait_sb_inodes (struct super_block *sb) {struct inode *inode, *old_inode =NULL ;
@@ -1371,6 +1380,7 @@ static void wait_sb_inodes(struct super_block *sb)*/ WARN_ON (!rwsem_is_locked (&sb->s_umount)); +mutex_lock (&sb->s_sync_lock);spin_lock (&sb->s_inode_list_lock);/*
@@ -1412,6 +1422,7 @@ static void wait_sb_inodes(struct super_block *sb) }spin_unlock (&sb->s_inode_list_lock);iput (old_inode); +mutex_unlock (&sb->s_sync_lock); }/**
fs/super.c +1 -0
@@ -190,6 +190,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags) s->s_flags = flags;INIT_HLIST_NODE (&s->s_instances);INIT_HLIST_BL_HEAD (&s->s_anon); +mutex_init (&s->s_sync_lock);INIT_LIST_HEAD (&s->s_inodes);spin_lock_init (&s->s_inode_list_lock);
include/linux/fs.h +2 -0
@@ -1346,6 +1346,8 @@ struct super_block {struct list_lru s_inode_lru____cacheline_aligned_in_smp ;struct rcu_head rcu; +struct mutex s_sync_lock;/* sync serialisation lock */ +/* * Indicates how deep in a filesystem stack this SB is */
Signature · 1 line
2.1.0
Re: [PATCH 4/8] sync: serialise per-superblock sync operations
Looks good,
PATCH 5/8 inode: rename i_wb_list to i_io_list
In reply to Josef BacikFrom: Dave Chinner <[email protected]>
There's a small consistency problem between the inode and writeback naming. Writeback calls the "for IO" inode queues b_io and b_more_io, but the inode calls these the "writeback list" or i_wb_list. This makes it hard to an new "under writeback" list to the inode, or call it an "under IO" list on the bdi because either way we'll have writeback on IO and IO on writeback and it'll just be confusing. I'm getting confused just writing this!
So, rename the inode "for IO" list variable to i_io_list so we can add a new "writeback list" in a subsequent patch.
--- fs/fs-writeback.c | 22 +++++++++++----------- fs/inode.c | 6 +++--- include/linux/fs.h | 2 +- mm/backing-dev.c | 8 ++++---- 4 files changed, 19 insertions(+), 19 deletions(-)
fs/fs-writeback.c +11 -11
@@ -96,7 +96,7 @@ EXPORT_SYMBOL_GPL(inode_to_bdi);static inline struct inode *wb_inode (struct list_head *head) { - return list_entry(head, struct inode, i_wb_list); + return list_entry(head, struct inode, i_io_list); }/*
@@ -205,7 +205,7 @@ void inode_wb_list_del(struct inode *inode)struct backing_dev_info *bdi =inode_to_bdi (inode);spin_lock (&bdi->wb.list_lock); -list_del_init (&inode->i_wb_list); +list_del_init (&inode->i_io_list);spin_unlock (&bdi->wb.list_lock); }
@@ -228,7 +228,7 @@ static void redirty_tail(struct inode *inode, struct bdi_writeback *wb)if (time_before (inode->dirtied_when, tail->dirtied_when)) inode->dirtied_when = jiffies; } -list_move (&inode->i_wb_list, &wb->b_dirty); +list_move (&inode->i_io_list, &wb->b_dirty); }/*
@@ -237,7 +237,7 @@ static void redirty_tail(struct inode *inode, struct bdi_writeback *wb)static void requeue_io (struct inode *inode,struct bdi_writeback *wb) {assert_spin_locked (&wb->list_lock); -list_move (&inode->i_wb_list, &wb->b_more_io); +list_move (&inode->i_io_list, &wb->b_more_io); }static void inode_sync_complete (struct inode *inode)
@@ -296,7 +296,7 @@ static int move_expired_inodes(struct list_head *delaying_queue,if (older_than_this &&inode_dirtied_after (inode, *older_than_this))break ; -list_move (&inode->i_wb_list, &tmp); +list_move (&inode->i_io_list, &tmp); moved++;if (flags &EXPIRE_DIRTY_ATIME )set_bit (__I_DIRTY_TIME_EXPIRED, &inode->i_state);
@@ -319,7 +319,7 @@ static int move_expired_inodes(struct list_head *delaying_queue,list_for_each_prev_safe (pos, node, &tmp) { inode =wb_inode (pos);if (inode->i_sb == sb) -list_move (&inode->i_wb_list, dispatch_queue); +list_move (&inode->i_io_list, dispatch_queue); } } out:
@@ -471,10 +471,10 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb,redirty_tail (inode, wb); }else if (inode->i_state &I_DIRTY_TIME ) { inode->dirtied_when = jiffies; -list_move (&inode->i_wb_list, &wb->b_dirty_time); +list_move (&inode->i_io_list, &wb->b_dirty_time); }else {/* The inode is clean. Remove from writeback lists. */ -list_del_init (&inode->i_wb_list); +list_del_init (&inode->i_io_list); } }
@@ -616,7 +616,7 @@ writeback_single_inode(struct inode *inode, struct bdi_writeback *wb,* touch it. See comment above for explanation. */ if (!(inode->i_state &I_DIRTY_ALL )) -list_del_init (&inode->i_wb_list); +list_del_init (&inode->i_io_list);spin_unlock (&wb->list_lock);inode_sync_complete (inode); out:
@@ -1343,9 +1343,9 @@ void __mark_inode_dirty(struct inode *inode, int flags)if (dirtytime) inode->dirtied_time_when = jiffies;if (inode->i_state & (I_DIRTY_INODE |I_DIRTY_PAGES )) -list_move (&inode->i_wb_list, &bdi->wb.b_dirty); +list_move (&inode->i_io_list, &bdi->wb.b_dirty);else -list_move (&inode->i_wb_list, +list_move (&inode->i_io_list, &bdi->wb.b_dirty_time);spin_unlock (&bdi->wb.list_lock);trace_writeback_dirty_inode_enqueue (inode);
fs/inode.c +3 -3
@@ -31,7 +31,7 @@* inode->i_sb->s_inode_list_lock protects: * inode->i_sb->s_inodes, inode->i_sb_list * bdi->wb.list_lock protects: -* bdi->wb.b_{dirty,io,more_io,dirty_time}, inode->i_wb_list +* bdi->wb.b_{dirty,io,more_io,dirty_time}, inode->i_io_list * inode_hash_lock protects: * inode_hashtable, inode->i_hash *
@@ -355,7 +355,7 @@ void inode_init_once(struct inode *inode)memset (inode,0 ,sizeof (*inode));INIT_HLIST_NODE (&inode->i_hash);INIT_LIST_HEAD (&inode->i_devices); -INIT_LIST_HEAD (&inode->i_wb_list); +INIT_LIST_HEAD (&inode->i_io_list);INIT_LIST_HEAD (&inode->i_lru);address_space_init_once (&inode->i_data);i_size_ordered_init (inode);
@@ -523,7 +523,7 @@ static void evict(struct inode *inode)BUG_ON (!(inode->i_state &I_FREEING ));BUG_ON (!list_empty (&inode->i_lru)); -if (!list_empty (&inode->i_wb_list)) +if (!list_empty (&inode->i_io_list))inode_wb_list_del (inode);inode_sb_list_del (inode);
include/linux/fs.h +1 -1
@@ -634,7 +634,7 @@ struct inode {unsigned long dirtied_time_when;struct hlist_node i_hash; -struct list_head i_wb_list;/* backing dev IO list */ +struct list_head i_io_list;/* backing dev IO list */ struct list_head i_lru;/* inode LRU list */ struct list_head i_sb_list;union {
mm/backing-dev.c +4 -4
@@ -54,13 +54,13 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v) nr_dirty = nr_io = nr_more_io = nr_dirty_time =0 ;spin_lock (&wb->list_lock); -list_for_each_entry (inode, &wb->b_dirty, i_wb_list) +list_for_each_entry (inode, &wb->b_dirty, i_io_list) nr_dirty++; -list_for_each_entry (inode, &wb->b_io, i_wb_list) +list_for_each_entry (inode, &wb->b_io, i_io_list) nr_io++; -list_for_each_entry (inode, &wb->b_more_io, i_wb_list) +list_for_each_entry (inode, &wb->b_more_io, i_io_list) nr_more_io++; -list_for_each_entry (inode, &wb->b_dirty_time, i_wb_list) +list_for_each_entry (inode, &wb->b_dirty_time, i_io_list)if (inode->i_state &I_DIRTY_TIME ) nr_dirty_time++;spin_unlock (&wb->list_lock);
Signature · 1 line
2.1.0
Re: [PATCH 5/8] inode: rename i_wb_list to i_io_list
Quoted 10 lines · Josef Bacik
From: Dave Chinner <[email protected]>
There's a small consistency problem between the inode and writeback naming. Writeback calls the "for IO" inode queues
b_ioandb_more_io, but the inode calls these the "writeback list" ori_wb_list. This makes it hard to an new "under writeback" list to the inode, or call it an "under IO" list on the bdi because either way we'll have writeback on IO and IO on writeback and it'll just be confusing. I'm getting confused just writing this!
Quoted 2 lines
So, rename the inode "for IO" list variable to
i_io_listso we can add a new "writeback list" in a subsequent patch.
Looks good,
PATCH 6/8 bdi: add a new writeback list for sync
In reply to Josef BacikFrom: Dave Chinner <[email protected]>
wait_sb_inodes() current does a walk of all inodes in the filesystem to find dirty one to wait on during sync. This is highly inefficient and wastes a lot of CPU when there are lots of clean cached inodes that we don't need to wait on.
To avoid this "all inode" walk, we need to track inodes that are currently under writeback that we need to wait for. We do this by adding inodes to a writeback list on the bdi when the mapping is first tagged as having pages under writeback. wait_sb_inodes() can then walk this list of "inodes under IO" and wait specifically just for the inodes that the current sync(2) needs to wait for.
To avoid needing all the realted locking to be safe against interrupts, Jan Kara suggested that we be lazy about removal from the writeback list. That is, we don't remove inodes from the writeback list on IO completion, but do it directly during a wait_sb_inodes() walk.
This means that the a rare sync(2) call will have some work to do skipping clean inodes However, in the current problem case of concurrent sync workloads, concurrent wait_sb_inodes() calls only walk the very recently dispatched inodes and hence should have very little work to do.
This also means that we have to remove the inodes from the writeback list during eviction. Do this in inode_wait_for_writeback() once all writeback on the inode is complete.
--- fs/fs-writeback.c | 131 ++++++++++++++++++++++++++++++++++---------- fs/inode.c | 1 + include/linux/backing-dev.h | 3 + include/linux/fs.h | 1 + mm/backing-dev.c | 1 + mm/page-writeback.c | 14 +++++ 6 files changed, 122 insertions(+), 29 deletions(-)
fs/fs-writeback.c +102 -29
@@ -210,6 +210,34 @@ void inode_wb_list_del(struct inode *inode) }/* +* mark an inode as under writeback on the given bdi +*/ +void bdi_mark_inode_writeback (struct backing_dev_info *bdi,struct inode *inode) +{ +WARN_ON_ONCE (bdi !=inode_to_bdi (inode)); +if (list_empty (&inode->i_wb_list)) { +spin_lock (&bdi->wb.list_lock); +if (list_empty (&inode->i_wb_list)) +list_add_tail (&inode->i_wb_list, &bdi->wb.b_writeback); +spin_unlock (&bdi->wb.list_lock); + } +} + +/* +* clear an inode as under writeback on the given bdi +*/ +static void bdi_clear_inode_writeback (struct backing_dev_info *bdi, +struct inode *inode) +{ +WARN_ON_ONCE (bdi !=inode_to_bdi (inode)); +if (!list_empty (&inode->i_wb_list)) { +spin_lock (&bdi->wb.list_lock); +list_del_init (&inode->i_wb_list); +spin_unlock (&bdi->wb.list_lock); + } +} + +/* * Redirty an inode: set its when-it-was dirtied timestamp and move it to the * furthest end of its superblock's dirty-inode list. *
@@ -383,13 +411,23 @@ static void __inode_wait_for_writeback(struct inode *inode) }/* -* Wait for writeback on an inode to complete. Caller must have inode pinned. +* Wait for writeback on an inode to complete during eviction. Caller must have +* inode pinned. */ void inode_wait_for_writeback (struct inode *inode) { +BUG_ON (!(inode->i_state &I_FREEING )); +spin_lock (&inode->i_lock);__inode_wait_for_writeback (inode);spin_unlock (&inode->i_lock); + +/* +* bd_inode's will have already had the bdev free'd so don't bother +* doing the bdi_clear_inode_writeback. +*/ +if (!sb_is_blkdev_sb (inode->i_sb)) +bdi_clear_inode_writeback (inode_to_bdi (inode), inode); }/*
@@ -1372,7 +1410,8 @@ EXPORT_SYMBOL(__mark_inode_dirty);*/ static void wait_sb_inodes (struct super_block *sb) { -struct inode *inode, *old_inode =NULL ; +struct backing_dev_info *bdi = sb->s_bdi; +LIST_HEAD (sync_list);/* * We need to be protected against the filesystem going from
@@ -1380,48 +1419,82 @@ static void wait_sb_inodes(struct super_block *sb)*/ WARN_ON (!rwsem_is_locked (&sb->s_umount)); -mutex_lock (&sb->s_sync_lock); -spin_lock (&sb->s_inode_list_lock); -/* -* Data integrity sync. Must wait for all pages under writeback, -* because there may have been pages dirtied before our sync -* call, but which had writeout started before we write it out. -* In which case, the inode may not be on the dirty list, but -* we still have to wait for that writeout. +* Data integrity sync. Must wait for all pages under writeback, because +* there may have been pages dirtied before our sync call, but which had +* writeout started before we write it out. In which case, the inode +* may not be on the dirty list, but we still have to wait for that +* writeout. +* +* To avoid syncing inodes put under IO after we have started here, +* splice the io list to a temporary list head and walk that. Newly +* dirtied inodes will go onto the primary list so we won't wait for +* them. This is safe to do as we are serialised by the s_sync_lock, +* so we'll complete processing the complete list before the next +* sync operation repeats the splice-and-walk process. +* +* Inodes that have pages under writeback after we've finished the wait +* may or may not be on the primary list. They had pages under put IO +* after we started our wait, so we need to make sure the next sync or +* IO completion treats them correctly, Move them back to the primary +* list and restart the walk. +* +* Inodes that are clean after we have waited for them don't belong on +* any list, so simply remove them from the sync list and move onwards. */ -list_for_each_entry (inode, &sb->s_inodes, i_sb_list) { +mutex_lock (&sb->s_sync_lock); +spin_lock (&bdi->wb.list_lock); +list_splice_init (&bdi->wb.b_writeback, &sync_list); + +while (!list_empty (&sync_list)) { + struct inode *inode = list_first_entry(&sync_list, struct inode, + i_wb_list);struct address_space *mapping = inode->i_mapping; +/* +* We are lazy on IO completion and don't remove inodes from the +* list when they are clean. Detect that immediately and skip +* inodes we don't ahve to wait on. +*/ +if (!mapping_tagged (mapping,PAGECACHE_TAG_WRITEBACK )) { +list_del_init (&inode->i_wb_list); +cond_resched_lock (&bdi->wb.list_lock); +continue ; + } +spin_lock (&inode->i_lock); -if ((inode->i_state & (I_FREEING |I_WILL_FREE |I_NEW )) || - (mapping->nrpages ==0 )) { +if (inode->i_state & (I_FREEING |I_WILL_FREE |I_NEW )) { +list_move (&inode->i_wb_list, &bdi->wb.b_writeback);spin_unlock (&inode->i_lock); +cond_resched_lock (&bdi->wb.list_lock);continue ; }__iget (inode);spin_unlock (&inode->i_lock); -spin_unlock (&sb->s_inode_list_lock); - -/* -* We hold a reference to 'inode' so it couldn't have been -* removed from s_inodes list while we dropped the -* s_inode_list_lock. We cannot iput the inode now as we can -* be holding the last reference and we cannot iput it under -* s_inode_list_lock. So we keep the reference and iput it -* later. -*/ -iput (old_inode); - old_inode = inode; +spin_unlock (&bdi->wb.list_lock);filemap_fdatawait (mapping); -cond_resched (); -spin_lock (&sb->s_inode_list_lock); +/* +* the inode has been written back now, so check whether we +* still have pages under IO and move it back to the primary +* list if necessary. We really need the mapping->tree_lock +* here because bdi_mark_inode_writeback may have not done +* anything because we were on the spliced list and we need to +* check TAG_WRITEBACK. +*/ +spin_lock_irq (&mapping->tree_lock); +spin_lock (&bdi->wb.list_lock); +if (mapping_tagged (mapping,PAGECACHE_TAG_WRITEBACK )) { +WARN_ON (list_empty (&inode->i_wb_list)); +list_move (&inode->i_wb_list, &bdi->wb.b_writeback); + }else +list_del_init (&inode->i_wb_list); +spin_unlock_irq (&mapping->tree_lock); +iput (inode); } -spin_unlock (&sb->s_inode_list_lock); -iput (old_inode); +spin_unlock (&bdi->wb.list_lock);mutex_unlock (&sb->s_sync_lock); }
fs/inode.c +1 -0
@@ -356,6 +356,7 @@ void inode_init_once(struct inode *inode)INIT_HLIST_NODE (&inode->i_hash);INIT_LIST_HEAD (&inode->i_devices);INIT_LIST_HEAD (&inode->i_io_list); +INIT_LIST_HEAD (&inode->i_wb_list);INIT_LIST_HEAD (&inode->i_lru);address_space_init_once (&inode->i_data);i_size_ordered_init (inode);
include/linux/backing-dev.h +3 -0
@@ -56,6 +56,7 @@ struct bdi_writeback {struct list_head b_io;/* parked for writeback */ struct list_head b_more_io;/* parked for more writeback */ struct list_head b_dirty_time;/* time stamps are dirty */ +struct list_head b_writeback;/* inodes under writeback */ spinlock_t list_lock;/* protects the b_* lists */ };
@@ -124,6 +125,8 @@ void bdi_start_background_writeback(struct backing_dev_info *bdi);void bdi_writeback_workfn (struct work_struct *work);int bdi_has_dirty_io (struct backing_dev_info *bdi);void bdi_wakeup_thread_delayed (struct backing_dev_info *bdi); +void bdi_mark_inode_writeback (struct backing_dev_info *bdi, +struct inode *inode);extern spinlock_t bdi_lock;extern struct list_head bdi_list;
include/linux/fs.h +1 -0
@@ -637,6 +637,7 @@ struct inode {struct list_head i_io_list;/* backing dev IO list */ struct list_head i_lru;/* inode LRU list */ struct list_head i_sb_list; +struct list_head i_wb_list;/* backing dev writeback list */ union {struct hlist_head i_dentry;struct rcu_head i_rcu;
mm/backing-dev.c +1 -0
@@ -386,6 +386,7 @@ static void bdi_wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi)INIT_LIST_HEAD (&wb->b_io);INIT_LIST_HEAD (&wb->b_more_io);INIT_LIST_HEAD (&wb->b_dirty_time); +INIT_LIST_HEAD (&wb->b_writeback);spin_lock_init (&wb->list_lock);INIT_DELAYED_WORK (&wb->dwork, bdi_writeback_workfn); }
mm/page-writeback.c +14 -0
@@ -2382,11 +2382,25 @@ int __test_set_page_writeback(struct page *page, bool keep_write)spin_lock_irqsave (&mapping->tree_lock, flags); ret =TestSetPageWriteback (page);if (!ret) { +bool on_wblist; + + on_wblist =mapping_tagged (mapping, +PAGECACHE_TAG_WRITEBACK ); +radix_tree_tag_set (&mapping->page_tree,page_index (page),PAGECACHE_TAG_WRITEBACK );if (bdi_cap_account_writeback (bdi))__inc_bdi_stat (bdi,BDI_WRITEBACK ); + +/* +* we can come through here when swapping anonymous +* pages, so we don't necessarily have an inode to +* track for sync. Inodes are remove lazily, so there is +* no equivalent in test_clear_page_writeback(). +*/ +if (!on_wblist && mapping->host) +bdi_mark_inode_writeback (bdi, mapping->host); }if (!PageDirty (page)) radix_tree_tag_clear(&mapping->page_tree,
Signature · 1 line
2.1.0
Jan Kara
maintainerRe: [PATCH 6/8] bdi: add a new writeback list for sync
Quoted 26 lines · Josef Bacik
From: Dave Chinner <[email protected]>
wait_sb_inodes()current does a walk of all inodes in the filesystem to find dirty one to wait on during sync. This is highly inefficient and wastes a lot of CPU when there are lots of clean cached inodes that we don't need to wait on.To avoid this "all inode" walk, we need to track inodes that are currently under writeback that we need to wait for. We do this by adding inodes to a writeback list on the bdi when the mapping is first tagged as having pages under writeback.
wait_sb_inodes()can then walk this list of "inodes under IO" and wait specifically just for the inodes that the current sync(2) needs to wait for.To avoid needing all the realted locking to be safe against interrupts, Jan Kara suggested that we be lazy about removal from the writeback list. That is, we don't remove inodes from the writeback list on IO completion, but do it directly during a
wait_sb_inodes()walk.This means that the a rare sync(2) call will have some work to do skipping clean inodes However, in the current problem case of concurrent sync workloads, concurrent
wait_sb_inodes()calls only walk the very recently dispatched inodes and hence should have very little work to do.
Quoted 6 lines
This also means that we have to remove the inodes from the writeback list during eviction. Do this in
inode_wait_for_writeback()once all writeback on the inode is complete.Signed-off-by Dave Chinner <[email protected]>Signed-off-by Josef Bacik <[email protected]>
I don't think you noticed comments I did when you last posted this series:
Quoted 10 lines
/*
- * Wait for writeback on an inode to complete. Caller must have inode pinned.
+ * Wait for writeback on an inode to complete during eviction. Caller must have
+ * inode pinned.
*/
voidinode_wait_for_writeback(struct inode *inode)
{fs/fs-writeback.c +2
-0+BUG_ON (!(inode->i_state &I_FREEING )); +spin_lock (&inode->i_lock);
fs/fs-writeback.c static void __inode_wait_for_writeback(struct inode *inode) · line 431
Quoted 10 lines
fs/fs-writeback.c +7
-0__inode_wait_for_writeback (inode);spin_unlock (&inode->i_lock); + +/* +* bd_inode's will have already had the bdev free'd so don't bother +* doing the bdi_clear_inode_writeback. +*/ +if (!sb_is_blkdev_sb (inode->i_sb)) +bdi_clear_inode_writeback (inode_to_bdi (inode), inode); }
Why do we bother with not putting bdev inode back?
...
fs/fs-writeback.c static void wait_sb_inodes(struct super_block *sb) · line 1467
Quoted 5 lines
fs/fs-writeback.c +2
-2spin_lock (&inode->i_lock); -if ((inode->i_state & (I_FREEING |I_WILL_FREE |I_NEW )) || - (mapping->nrpages ==0 )) { +if (inode->i_state & (I_FREEING |I_WILL_FREE |I_NEW )) { +list_move (&inode->i_wb_list, &bdi->wb.b_writeback);
Indenting is broken here...
fs/fs-writeback.c static void wait_sb_inodes(struct super_block *sb) · line 1473
Quoted 6 lines
fs/fs-writeback.c +1
-0spin_unlock (&inode->i_lock); +cond_resched_lock (&bdi->wb.list_lock);continue ; }__iget (inode);spin_unlock (&inode->i_lock);
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 6/8] bdi: add a new writeback list for sync
Quoted 31 lines · Jan Kara
Quoted 30 lines · Josef Bacik
From: Dave Chinner <[email protected]>
wait_sb_inodes()current does a walk of all inodes in the filesystem to find dirty one to wait on during sync. This is highly inefficient and wastes a lot of CPU when there are lots of clean cached inodes that we don't need to wait on.To avoid this "all inode" walk, we need to track inodes that are currently under writeback that we need to wait for. We do this by adding inodes to a writeback list on the bdi when the mapping is first tagged as having pages under writeback.
wait_sb_inodes()can then walk this list of "inodes under IO" and wait specifically just for the inodes that the current sync(2) needs to wait for.To avoid needing all the realted locking to be safe against interrupts, Jan Kara suggested that we be lazy about removal from the writeback list. That is, we don't remove inodes from the writeback list on IO completion, but do it directly during a
wait_sb_inodes()walk.This means that the a rare sync(2) call will have some work to do skipping clean inodes However, in the current problem case of concurrent sync workloads, concurrent
wait_sb_inodes()calls only walk the very recently dispatched inodes and hence should have very little work to do.This also means that we have to remove the inodes from the writeback list during eviction. Do this in
inode_wait_for_writeback()once all writeback on the inode is complete.
Quoted 5 lines
Quoted 2 lines
Signed-off-by Dave Chinner <[email protected]>Signed-off-by Josef Bacik <[email protected]>I don't think you noticed comments I did when you last posted this series:
You're right I missed them completely, sorry.
Quoted 15 lines
Quoted 15 lines
/*
- * Wait for writeback on an inode to complete. Caller must have inode pinned.
+ * Wait for writeback on an inode to complete during eviction. Caller must have
+ * inode pinned.
*/
voidinode_wait_for_writeback(struct inode *inode)
{fs/fs-writeback.c +4
-0+BUG_ON (!(inode->i_state &I_FREEING )); +spin_lock (&inode->i_lock);__inode_wait_for_writeback (inode);spin_unlock (&inode->i_lock); + +/*
Quoted 9 lines
fs/fs-writeback.c static void __inode_wait_for_writeback(struct inode *inode) · line 431
Quoted 6 lines
fs/fs-writeback.c +5
-0+* bd_inode's will have already had the bdev free'd so don't bother +* doing the bdi_clear_inode_writeback. +*/ +if (!sb_is_blkdev_sb (inode->i_sb)) +bdi_clear_inode_writeback (inode_to_bdi (inode), inode); }Why do we bother with not putting bdev inode back?
My memory is rusty on this, but if the inode is the inode for a bdev we will have already free'd the bdev at this point and we get a null pointer deref, so this just skips that bit. I'll fix up the indenting.
Thanks,
Josef
Jan Kara
maintainerRe: [PATCH 6/8] bdi: add a new writeback list for sync
Quoted 24 lines · Josef Bacik
Quoted 14 lines
Quoted 14 lines
/*
- * Wait for writeback on an inode to complete. Caller must have inode pinned.
+ * Wait for writeback on an inode to complete during eviction. Caller must have
+ * inode pinned.
*/
voidinode_wait_for_writeback(struct inode *inode)
{fs/fs-writeback.c +4
-0+BUG_ON (!(inode->i_state &I_FREEING )); +spin_lock (&inode->i_lock);__inode_wait_for_writeback (inode);spin_unlock (&inode->i_lock); + +/* Quoted 9 lines
fs/fs-writeback.c static void __inode_wait_for_writeback(struct inode *inode) · line 431
Quoted 6 lines
fs/fs-writeback.c +5
-0+* bd_inode's will have already had the bdev free'd so don't bother +* doing the bdi_clear_inode_writeback. +*/ +if (!sb_is_blkdev_sb (inode->i_sb)) +bdi_clear_inode_writeback (inode_to_bdi (inode), inode); }Why do we bother with not putting bdev inode back?
Quoted 3 lines
My memory is rusty on this, but if the inode is the inode for a bdev we will have already free'd the bdev at this point and we get a null pointer deref, so this just skips that bit.
Ah, the reason likely is that bdev->bd_disk is NULL (already cleaned up in __blkdev_put()) at this moment and thus bdev_get_queue() called from inode_to_bdi() will oops. Can you please add these details to the comment? It's a bit subtle...
Also we shouldn't have any pages in the block device mapping anymore because of the work done in __blkdev_put() (and thus inode shouldn't be in the writeback list) but I'd be calmer if we asserted list_empty(&inode->i_wb_list). Can you please add that? Thanks!
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 6/8] bdi: add a new writeback list for sync
Quoted 34 lines · Jan Kara
Quoted 24 lines · Josef Bacik
Quoted 14 lines
Quoted 14 lines
/*
- * Wait for writeback on an inode to complete. Caller must have inode pinned.
+ * Wait for writeback on an inode to complete during eviction. Caller must have
+ * inode pinned.
*/
voidinode_wait_for_writeback(struct inode *inode)
{fs/fs-writeback.c +4
-0+BUG_ON (!(inode->i_state &I_FREEING )); +spin_lock (&inode->i_lock);__inode_wait_for_writeback (inode);spin_unlock (&inode->i_lock); + +/* Quoted 9 lines
fs/fs-writeback.c static void __inode_wait_for_writeback(struct inode *inode) · line 431
Quoted 6 lines
fs/fs-writeback.c +5
-0+* bd_inode's will have already had the bdev free'd so don't bother +* doing the bdi_clear_inode_writeback. +*/ +if (!sb_is_blkdev_sb (inode->i_sb)) +bdi_clear_inode_writeback (inode_to_bdi (inode), inode); }Why do we bother with not putting bdev inode back?
Quoted 3 lines
My memory is rusty on this, but if the inode is the inode for a bdev we will have already free'd the bdev at this point and we get a null pointer deref, so this just skips that bit.
Ah, the reason likely is that
bdev->bd_diskis NULL (already cleaned up in__blkdev_put()) at this moment and thusbdev_get_queue()called frominode_to_bdi()will oops. Can you please add these details to the comment? It's a bit subtle...
Quoted 4 lines
Also we shouldn't have any pages in the block device mapping anymore because of the work done in
__blkdev_put()(and thus inode shouldn't be in the writeback list) but I'd be calmer if we assertedlist_empty(&inode->i_wb_list). Can you please add that? Thanks!
Won't it trip if we never sync before we drop the device tho? So we write some stuff to the block device, it gets written out, we then drop the device for whatever reason and boom, hit BUG_ON(&inode->i_wb_list) because we're still on the writeback list even though it doesn't matter because this disk is going away. Just an untested theory, what do you think? If it is possible I suppose I could just add the clear'ing bit for the bd inode to before we drop bd_disk if that would make you happy.
Thanks,
Josef
Jan Kara
maintainerRe: [PATCH 6/8] bdi: add a new writeback list for sync
Quoted 40 lines · Josef Bacik
Quoted 34 lines · Jan Kara
Quoted 24 lines · Josef Bacik
Quoted 14 lines
Quoted 14 lines
/*
- * Wait for writeback on an inode to complete. Caller must have inode pinned.
+ * Wait for writeback on an inode to complete during eviction. Caller must have
+ * inode pinned.
*/
voidinode_wait_for_writeback(struct inode *inode)
{fs/fs-writeback.c +4
-0+BUG_ON (!(inode->i_state &I_FREEING )); +spin_lock (&inode->i_lock);__inode_wait_for_writeback (inode);spin_unlock (&inode->i_lock); + +/* Quoted 9 lines
fs/fs-writeback.c static void __inode_wait_for_writeback(struct inode *inode) · line 431
Quoted 6 lines
fs/fs-writeback.c +5
-0+* bd_inode's will have already had the bdev free'd so don't bother +* doing the bdi_clear_inode_writeback. +*/ +if (!sb_is_blkdev_sb (inode->i_sb)) +bdi_clear_inode_writeback (inode_to_bdi (inode), inode); }Why do we bother with not putting bdev inode back?
Quoted 3 lines
My memory is rusty on this, but if the inode is the inode for a bdev we will have already free'd the bdev at this point and we get a null pointer deref, so this just skips that bit.
Ah, the reason likely is that
bdev->bd_diskis NULL (already cleaned up in__blkdev_put()) at this moment and thusbdev_get_queue()called frominode_to_bdi()will oops. Can you please add these details to the comment? It's a bit subtle...Quoted 4 lines
Also we shouldn't have any pages in the block device mapping anymore because of the work done in
__blkdev_put()(and thus inode shouldn't be in the writeback list) but I'd be calmer if we assertedlist_empty(&inode->i_wb_list). Can you please add that? Thanks!
Quoted 6 lines
Won't it trip if we never sync before we drop the device tho? So we write some stuff to the block device, it gets written out, we then drop the device for whatever reason and boom, hit
BUG_ON(&inode->i_wb_list)because we're still on the writeback list even though it doesn't matter because this disk is going away. Just an untested theory, what do you think?
Well, we are going to free the inode. If it is still linked into the writeback list, we are in trouble as the list will now contain freed element. So better BUG_ON earlier than chase memory corruption later.
Calling bdi_clear_inode_writeback() in kill_bdev() is probably a good idea and we can then just have the assertion in evict() to make sure nothing went wrong. OK?
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v4 6/8 bdi: add a new writeback list for sync
In reply to Josef BacikFrom: Dave Chinner <[email protected]>
wait_sb_inodes() current does a walk of all inodes in the filesystem to find dirty one to wait on during sync. This is highly inefficient and wastes a lot of CPU when there are lots of clean cached inodes that we don't need to wait on.
To avoid this "all inode" walk, we need to track inodes that are currently under writeback that we need to wait for. We do this by adding inodes to a writeback list on the bdi when the mapping is first tagged as having pages under writeback. wait_sb_inodes() can then walk this list of "inodes under IO" and wait specifically just for the inodes that the current sync(2) needs to wait for.
To avoid needing all the realted locking to be safe against interrupts, Jan Kara suggested that we be lazy about removal from the writeback list. That is, we don't remove inodes from the writeback list on IO completion, but do it directly during a wait_sb_inodes() walk.
This means that the a rare sync(2) call will have some work to do skipping clean inodes However, in the current problem case of concurrent sync workloads, concurrent wait_sb_inodes() calls only walk the very recently dispatched inodes and hence should have very little work to do.
This also means that we have to remove the inodes from the writeback list during eviction. Do this in inode_wait_for_writeback() once all writeback on the inode is complete.
---V3->V4:
-added bdi_clear_inode_writeback to kill_bdev.
-added BUG_ON(!list_emtpy(&inode->i_wb_list)) to inode_wait_writeback -clarified the comment in inode_wait_writeback
-fixed some indenting
fs/block_dev.c | 2 + fs/fs-writeback.c | 136 ++++++++++++++++++++++++++++++++++---------- fs/inode.c | 1 + include/linux/backing-dev.h | 5 ++ include/linux/fs.h | 1 + mm/backing-dev.c | 1 + mm/page-writeback.c | 14 +++++ 7 files changed, 131 insertions(+), 29 deletions(-)
fs/block_dev.c +2 -0
@@ -64,6 +64,8 @@ void kill_bdev(struct block_device *bdev) {struct address_space *mapping = bdev->bd_inode->i_mapping; +bdi_clear_inode_writeback (inode_to_bdi (bdev->bd_inode), + bdev->bd_inode);if (mapping->nrpages ==0 && mapping->nrshadows ==0 )return ;
fs/fs-writeback.c +107 -29
181 lines not on this page · raw message
fs/inode.c +1 -0
@@ -356,6 +356,7 @@ void inode_init_once(struct inode *inode)INIT_HLIST_NODE (&inode->i_hash);INIT_LIST_HEAD (&inode->i_devices);INIT_LIST_HEAD (&inode->i_io_list); +INIT_LIST_HEAD (&inode->i_wb_list);INIT_LIST_HEAD (&inode->i_lru);address_space_init_once (&inode->i_data);i_size_ordered_init (inode);
include/linux/backing-dev.h +5 -0
@@ -56,6 +56,7 @@ struct bdi_writeback {struct list_head b_io;/* parked for writeback */ struct list_head b_more_io;/* parked for more writeback */ struct list_head b_dirty_time;/* time stamps are dirty */ +struct list_head b_writeback;/* inodes under writeback */ spinlock_t list_lock;/* protects the b_* lists */ };
@@ -124,6 +125,10 @@ void bdi_start_background_writeback(struct backing_dev_info *bdi);void bdi_writeback_workfn (struct work_struct *work);int bdi_has_dirty_io (struct backing_dev_info *bdi);void bdi_wakeup_thread_delayed (struct backing_dev_info *bdi); +void bdi_mark_inode_writeback (struct backing_dev_info *bdi, +struct inode *inode); +void bdi_clear_inode_writeback (struct backing_dev_info *bdi, +struct inode *inode);extern spinlock_t bdi_lock;extern struct list_head bdi_list;
include/linux/fs.h +1 -0
@@ -637,6 +637,7 @@ struct inode {struct list_head i_io_list;/* backing dev IO list */ struct list_head i_lru;/* inode LRU list */ struct list_head i_sb_list; +struct list_head i_wb_list;/* backing dev writeback list */ union {struct hlist_head i_dentry;struct rcu_head i_rcu;
mm/backing-dev.c +1 -0
@@ -386,6 +386,7 @@ static void bdi_wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi)INIT_LIST_HEAD (&wb->b_io);INIT_LIST_HEAD (&wb->b_more_io);INIT_LIST_HEAD (&wb->b_dirty_time); +INIT_LIST_HEAD (&wb->b_writeback);spin_lock_init (&wb->list_lock);INIT_DELAYED_WORK (&wb->dwork, bdi_writeback_workfn); }
mm/page-writeback.c +14 -0
@@ -2382,11 +2382,25 @@ int __test_set_page_writeback(struct page *page, bool keep_write)spin_lock_irqsave (&mapping->tree_lock, flags); ret =TestSetPageWriteback (page);if (!ret) { +bool on_wblist; + + on_wblist =mapping_tagged (mapping, +PAGECACHE_TAG_WRITEBACK ); +radix_tree_tag_set (&mapping->page_tree,page_index (page),PAGECACHE_TAG_WRITEBACK );if (bdi_cap_account_writeback (bdi))__inc_bdi_stat (bdi,BDI_WRITEBACK ); + +/* +* we can come through here when swapping anonymous +* pages, so we don't necessarily have an inode to +* track for sync. Inodes are remove lazily, so there is +* no equivalent in test_clear_page_writeback(). +*/ +if (!on_wblist && mapping->host) +bdi_mark_inode_writeback (bdi, mapping->host); }if (!PageDirty (page)) radix_tree_tag_clear(&mapping->page_tree,
Signature · 1 line
2.1.0
Re: [PATCH 6/8 V4] bdi: add a new writeback list for sync
Quoted 26 lines · Josef Bacik
From: Dave Chinner <[email protected]>
wait_sb_inodes()current does a walk of all inodes in the filesystem to find dirty one to wait on during sync. This is highly inefficient and wastes a lot of CPU when there are lots of clean cached inodes that we don't need to wait on.To avoid this "all inode" walk, we need to track inodes that are currently under writeback that we need to wait for. We do this by adding inodes to a writeback list on the bdi when the mapping is first tagged as having pages under writeback.
wait_sb_inodes()can then walk this list of "inodes under IO" and wait specifically just for the inodes that the current sync(2) needs to wait for.To avoid needing all the realted locking to be safe against interrupts, Jan Kara suggested that we be lazy about removal from the writeback list. That is, we don't remove inodes from the writeback list on IO completion, but do it directly during a
wait_sb_inodes()walk.This means that the a rare sync(2) call will have some work to do skipping clean inodes However, in the current problem case of concurrent sync workloads, concurrent
wait_sb_inodes()calls only walk the very recently dispatched inodes and hence should have very little work to do.
Quoted 7 lines
This also means that we have to remove the inodes from the writeback list during eviction. Do this in
inode_wait_for_writeback()once all writeback on the inode is complete.Signed-off-by Dave Chinner <[email protected]>Signed-off-by Josef Bacik <[email protected]>Tested-by Dave Chinner <[email protected]>
Looks good. You can add:
Honza
Quoted 310 lines
---
V3->V4:(quoted diff) +0
-4-added bdi_clear_inode_writeback to kill_bdev. -added BUG_ON(!list_emtpy(&inode->i_wb_list)) to inode_wait_writeback -clarified the comment in inode_wait_writeback -fixed some indenting fs/block_dev.c | 2 + fs/fs-writeback.c | 136 ++++++++++++++++++++++++++++++++++---------- fs/inode.c | 1 + include/linux/backing-dev.h | 5 ++ include/linux/fs.h | 1 + mm/backing-dev.c | 1 + mm/page-writeback.c | 14 +++++ 7 files changed, 131 insertions(+), 29 deletions(-)fs/block_dev.c +2
-0@@ -64,6 +64,8 @@ void kill_bdev(struct block_device *bdev) {struct address_space *mapping = bdev->bd_inode->i_mapping; +bdi_clear_inode_writeback (inode_to_bdi (bdev->bd_inode), + bdev->bd_inode);if (mapping->nrpages ==0 && mapping->nrshadows ==0 )return ;fs/fs-writeback.c +107
-29@@ -210,6 +210,34 @@ void inode_wb_list_del(struct inode *inode) }/* +* mark an inode as under writeback on the given bdi +*/ +void bdi_mark_inode_writeback (struct backing_dev_info *bdi,struct inode *inode) +{ +WARN_ON_ONCE (bdi !=inode_to_bdi (inode)); +if (list_empty (&inode->i_wb_list)) { +spin_lock (&bdi->wb.list_lock); +if (list_empty (&inode->i_wb_list)) +list_add_tail (&inode->i_wb_list, &bdi->wb.b_writeback); +spin_unlock (&bdi->wb.list_lock); + } +} + +/* +* clear an inode as under writeback on the given bdi +*/ +void bdi_clear_inode_writeback (struct backing_dev_info *bdi, +struct inode *inode) +{ +WARN_ON_ONCE (bdi !=inode_to_bdi (inode)); +if (!list_empty (&inode->i_wb_list)) { +spin_lock (&bdi->wb.list_lock); +list_del_init (&inode->i_wb_list); +spin_unlock (&bdi->wb.list_lock); + } +} + +/* * Redirty an inode: set its when-it-was dirtied timestamp and move it to the * furthest end of its superblock's dirty-inode list. * @@ -383,13 +411,28 @@ static void __inode_wait_for_writeback(struct inode *inode) }/* -* Wait for writeback on an inode to complete. Caller must have inode pinned. +* Wait for writeback on an inode to complete during eviction. Caller must have +* inode pinned. */ void inode_wait_for_writeback (struct inode *inode) { +BUG_ON (!(inode->i_state &I_FREEING )); +spin_lock (&inode->i_lock);__inode_wait_for_writeback (inode);spin_unlock (&inode->i_lock); + +/* +* For a bd_inode when we do inode_to_bdi we'll want to get the bdev for +* the inode and then deref bdev->bd_disk, which at this point has been +* set to NULL, so we would panic. At the point we are dropping our +* bd_inode we won't have any pages under writeback on the device so +* this is safe. But just in case we'll assert to make sure we don't +* screw this up. +*/ +if (!sb_is_blkdev_sb (inode->i_sb)) +bdi_clear_inode_writeback (inode_to_bdi (inode), inode); +BUG_ON (!list_empty (&inode->i_wb_list)); }/* @@ -1372,7 +1415,8 @@ EXPORT_SYMBOL(__mark_inode_dirty);*/ static void wait_sb_inodes (struct super_block *sb) { -struct inode *inode, *old_inode =NULL ; +struct backing_dev_info *bdi = sb->s_bdi; +LIST_HEAD (sync_list);/* * We need to be protected against the filesystem going from @@ -1380,48 +1424,82 @@ static void wait_sb_inodes(struct super_block *sb)*/ WARN_ON (!rwsem_is_locked (&sb->s_umount)); -mutex_lock (&sb->s_sync_lock); -spin_lock (&sb->s_inode_list_lock); -/* -* Data integrity sync. Must wait for all pages under writeback, -* because there may have been pages dirtied before our sync -* call, but which had writeout started before we write it out. -* In which case, the inode may not be on the dirty list, but -* we still have to wait for that writeout. +* Data integrity sync. Must wait for all pages under writeback, because +* there may have been pages dirtied before our sync call, but which had +* writeout started before we write it out. In which case, the inode +* may not be on the dirty list, but we still have to wait for that +* writeout. +* +* To avoid syncing inodes put under IO after we have started here, +* splice the io list to a temporary list head and walk that. Newly +* dirtied inodes will go onto the primary list so we won't wait for +* them. This is safe to do as we are serialised by the s_sync_lock, +* so we'll complete processing the complete list before the next +* sync operation repeats the splice-and-walk process. +* +* Inodes that have pages under writeback after we've finished the wait +* may or may not be on the primary list. They had pages under put IO +* after we started our wait, so we need to make sure the next sync or +* IO completion treats them correctly, Move them back to the primary +* list and restart the walk. +* +* Inodes that are clean after we have waited for them don't belong on +* any list, so simply remove them from the sync list and move onwards. */ -list_for_each_entry (inode, &sb->s_inodes, i_sb_list) { +mutex_lock (&sb->s_sync_lock); +spin_lock (&bdi->wb.list_lock); +list_splice_init (&bdi->wb.b_writeback, &sync_list); + +while (!list_empty (&sync_list)) { + struct inode *inode = list_first_entry(&sync_list, struct inode, + i_wb_list);struct address_space *mapping = inode->i_mapping; +/* +* We are lazy on IO completion and don't remove inodes from the +* list when they are clean. Detect that immediately and skip +* inodes we don't ahve to wait on. +*/ +if (!mapping_tagged (mapping,PAGECACHE_TAG_WRITEBACK )) { +list_del_init (&inode->i_wb_list); +cond_resched_lock (&bdi->wb.list_lock); +continue ; + } +spin_lock (&inode->i_lock); -if ((inode->i_state & (I_FREEING |I_WILL_FREE |I_NEW )) || - (mapping->nrpages ==0 )) { +if (inode->i_state & (I_FREEING |I_WILL_FREE |I_NEW )) { +list_move (&inode->i_wb_list, &bdi->wb.b_writeback);spin_unlock (&inode->i_lock); +cond_resched_lock (&bdi->wb.list_lock);continue ; }__iget (inode);spin_unlock (&inode->i_lock); -spin_unlock (&sb->s_inode_list_lock); - -/* -* We hold a reference to 'inode' so it couldn't have been -* removed from s_inodes list while we dropped the -* s_inode_list_lock. We cannot iput the inode now as we can -* be holding the last reference and we cannot iput it under -* s_inode_list_lock. So we keep the reference and iput it -* later. -*/ -iput (old_inode); - old_inode = inode; +spin_unlock (&bdi->wb.list_lock);filemap_fdatawait (mapping); -cond_resched (); -spin_lock (&sb->s_inode_list_lock); +/* +* the inode has been written back now, so check whether we +* still have pages under IO and move it back to the primary +* list if necessary. We really need the mapping->tree_lock +* here because bdi_mark_inode_writeback may have not done +* anything because we were on the spliced list and we need to +* check TAG_WRITEBACK. +*/ +spin_lock_irq (&mapping->tree_lock); +spin_lock (&bdi->wb.list_lock); +if (mapping_tagged (mapping,PAGECACHE_TAG_WRITEBACK )) { +WARN_ON (list_empty (&inode->i_wb_list)); +list_move (&inode->i_wb_list, &bdi->wb.b_writeback); + }else +list_del_init (&inode->i_wb_list); +spin_unlock_irq (&mapping->tree_lock); +iput (inode); } -spin_unlock (&sb->s_inode_list_lock); -iput (old_inode); +spin_unlock (&bdi->wb.list_lock);mutex_unlock (&sb->s_sync_lock); }fs/inode.c +1
-0@@ -356,6 +356,7 @@ void inode_init_once(struct inode *inode)INIT_HLIST_NODE (&inode->i_hash);INIT_LIST_HEAD (&inode->i_devices);INIT_LIST_HEAD (&inode->i_io_list); +INIT_LIST_HEAD (&inode->i_wb_list);INIT_LIST_HEAD (&inode->i_lru);address_space_init_once (&inode->i_data);i_size_ordered_init (inode);include/linux/backing-dev.h +5
-0@@ -56,6 +56,7 @@ struct bdi_writeback {struct list_head b_io;/* parked for writeback */ struct list_head b_more_io;/* parked for more writeback */ struct list_head b_dirty_time;/* time stamps are dirty */ +struct list_head b_writeback;/* inodes under writeback */ spinlock_t list_lock;/* protects the b_* lists */ };@@ -124,6 +125,10 @@ void bdi_start_background_writeback(struct backing_dev_info *bdi);void bdi_writeback_workfn (struct work_struct *work);int bdi_has_dirty_io (struct backing_dev_info *bdi);void bdi_wakeup_thread_delayed (struct backing_dev_info *bdi); +void bdi_mark_inode_writeback (struct backing_dev_info *bdi, +struct inode *inode); +void bdi_clear_inode_writeback (struct backing_dev_info *bdi, +struct inode *inode);extern spinlock_t bdi_lock;extern struct list_head bdi_list;include/linux/fs.h +1
-0@@ -637,6 +637,7 @@ struct inode {struct list_head i_io_list;/* backing dev IO list */ struct list_head i_lru;/* inode LRU list */ struct list_head i_sb_list; +struct list_head i_wb_list;/* backing dev writeback list */ union {struct hlist_head i_dentry;struct rcu_head i_rcu;mm/backing-dev.c +1
-0@@ -386,6 +386,7 @@ static void bdi_wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi)INIT_LIST_HEAD (&wb->b_io);INIT_LIST_HEAD (&wb->b_more_io);INIT_LIST_HEAD (&wb->b_dirty_time); +INIT_LIST_HEAD (&wb->b_writeback);spin_lock_init (&wb->list_lock);INIT_DELAYED_WORK (&wb->dwork, bdi_writeback_workfn); }mm/page-writeback.c +14
-0@@ -2382,11 +2382,25 @@ int __test_set_page_writeback(struct page *page, bool keep_write)spin_lock_irqsave (&mapping->tree_lock, flags); ret =TestSetPageWriteback (page);if (!ret) { +bool on_wblist; + + on_wblist =mapping_tagged (mapping, +PAGECACHE_TAG_WRITEBACK ); +radix_tree_tag_set (&mapping->page_tree,page_index (page),PAGECACHE_TAG_WRITEBACK );if (bdi_cap_account_writeback (bdi))__inc_bdi_stat (bdi,BDI_WRITEBACK ); + +/* +* we can come through here when swapping anonymous +* pages, so we don't necessarily have an inode to +* track for sync. Inodes are remove lazily, so there is +* no equivalent in test_clear_page_writeback(). +*/ +if (!on_wblist && mapping->host) +bdi_mark_inode_writeback (bdi, mapping->host); }if (!PageDirty (page)) radix_tree_tag_clear(&mapping->page_tree,Signature · 1 line
2.1.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH 7/8 writeback: periodically trim the writeback list
In reply to Josef BacikFrom: Dave Chinner <[email protected]>
Inodes are removed lazily from the bdi writeback list, so in the absence of sync(2) work inodes will build up on the bdi writback list even though they are no longer under IO. Use the periodic kupdate work check to remove inodes no longer under IO from the writeback list.
--- fs/fs-writeback.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
fs/fs-writeback.c +19 -0
@@ -1055,6 +1055,23 @@ static long wb_check_background_flush(struct bdi_writeback *wb)return 0 ; } +/* +* clean out writeback list for all inodes that don't have IO in progress +*/ +static void wb_trim_writeback_list (struct bdi_writeback *wb) +{ +struct inode *inode; +struct inode *tmp; + +spin_lock (&wb->list_lock); +list_for_each_entry_safe (inode, tmp, &wb->b_writeback, i_wb_list) { +if (!mapping_tagged (inode->i_mapping,PAGECACHE_TAG_WRITEBACK )) +list_del_init (&inode->i_wb_list); + } +spin_unlock (&wb->list_lock); + +} +static long wb_check_old_data_flush (struct bdi_writeback *wb) {unsigned long expired;
@@ -1071,6 +1088,8 @@ static long wb_check_old_data_flush(struct bdi_writeback *wb)if (time_before (jiffies, expired))return 0 ; +wb_trim_writeback_list (wb); + wb->last_old_flush = jiffies; nr_pages =get_nr_dirty_pages ();
Signature · 1 line
2.1.0
PATCH 8/8 inode: don't softlockup when evicting inodes
In reply to Josef BacikOn a box with a lot of ram (148gb) I can make the box softlockup after running an fs_mark job that creates hundreds of millions of empty files. This is because we never generate enough memory pressure to keep the number of inodes on our unused list low, so when we go to unmount we have to evict ~100 million inodes. This makes one processor a very unhappy person, so add a cond_resched() in dispose_list() and if we need a resched when processing the s_inodes list do that and run dispose_list() on what we've currently culled. Thanks,
--- fs/inode.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
fs/inode.c +14 -0
@@ -574,6 +574,7 @@ static void dispose_list(struct list_head *head)list_del_init (&inode->i_lru);evict (inode); +cond_resched (); } }
@@ -591,6 +592,7 @@ void evict_inodes(struct super_block *sb)struct inode *inode, *next;LIST_HEAD (dispose); +again:spin_lock (&sb->s_inode_list_lock);list_for_each_entry_safe (inode, next, &sb->s_inodes, i_sb_list) {if (atomic_read (&inode->i_count))
@@ -606,6 +608,18 @@ void evict_inodes(struct super_block *sb)inode_lru_list_del (inode);spin_unlock (&inode->i_lock);list_add (&inode->i_lru, &dispose); + +/* +* We can have a ton of inodes to evict at unmount time given +* enough memory, check to see if we need to go to sleep for a +* bit so we don't livelock. +*/ +if (need_resched ()) { +spin_unlock (&sb->s_inode_list_lock); +cond_resched (); +dispose_list (&dispose); + goto again; + } }spin_unlock (&sb->s_inode_list_lock);
Signature · 1 line
2.1.0
Re: [PATCH 8/8] inode: don't softlockup when evicting inodes
Quoted 9 lines · Josef Bacik
On a box with a lot of ram (148gb) I can make the box softlockup after running an
fs_markjob that creates hundreds of millions of empty files. This is because we never generate enough memory pressure to keep the number of inodes on our unused list low, so when we go to unmount we have to evict ~100 million inodes. This makes one processor a very unhappy person, so add acond_resched()indispose_list()and if we need a resched when processing thes_inodeslist do that and rundispose_list()on what we've currently culled. Thanks,Signed-off-by Josef Bacik <[email protected]>
Looks good. You can add:
Honza
Quoted 46 lines
--- fs/inode.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)fs/inode.c +14
-0@@ -574,6 +574,7 @@ static void dispose_list(struct list_head *head)list_del_init (&inode->i_lru);evict (inode); +cond_resched (); } }@@ -591,6 +592,7 @@ void evict_inodes(struct super_block *sb)struct inode *inode, *next;LIST_HEAD (dispose); +again:spin_lock (&sb->s_inode_list_lock);list_for_each_entry_safe (inode, next, &sb->s_inodes, i_sb_list) {if (atomic_read (&inode->i_count))@@ -606,6 +608,18 @@ void evict_inodes(struct super_block *sb)inode_lru_list_del (inode);spin_unlock (&inode->i_lock);list_add (&inode->i_lru, &dispose); + +/* +* We can have a ton of inodes to evict at unmount time given +* enough memory, check to see if we need to go to sleep for a +* bit so we don't livelock. +*/ +if (need_resched ()) { +spin_unlock (&sb->s_inode_list_lock); +cond_resched (); +dispose_list (&dispose); + goto again; + } }spin_unlock (&sb->s_inode_list_lock);Signature · 1 line
2.1.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Hello,
(cc'ing Jens)
Quoted 5 lines · Josef Bacik
Here are the cleaned up versions of Dave Chinners super block scalability patches. I've been testing them locally for a while and they are pretty solid. They fix a few big issues, such as the global inode list and soft lockups on boxes on unmount that have lots of inodes in cache. Al if you would consider pulling these in that would be great, you can pull from here
FYI, the bdi / writeback changes will conflict with the cgroup writeback support queued in block/for-4.2/writeback. I think updating the patchset should be fairly straight forward tho.
Thanks.
Signature · 1 line
tejun
Quoted 7 lines · Josef Bacik
Here are the cleaned up versions of Dave Chinners super block scalability patches. I've been testing them locally for a while and they are pretty solid. They fix a few big issues, such as the global inode list and soft lockups on boxes on unmount that have lots of inodes in cache. Al if you would consider pulling these in that would be great, you can pull from here
git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.gitsuperblock-scaling
Passes all my smoke tests.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
PATCH 1/1 sync: wait_sb_inodes() calls iput() with spinlock held (was Re: [PATCH 0/7] super block scalabilit patches V3)
Quoted 7 lines · Dave Chinner
Quoted 6 lines · Josef Bacik
Here are the cleaned up versions of Dave Chinners super block scalability patches. I've been testing them locally for a while and they are pretty solid. They fix a few big issues, such as the global inode list and soft lockups on boxes on unmount that have lots of inodes in cache. Al if you would consider pulling these in that would be great, you can pull from here
Quoted 5 lines
Quoted 1 line
git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.gitsuperblock-scalingPasses all my smoke tests.
Tested-by Dave Chinner <[email protected]>
FWIW, I just updated my trees to whatever is in the above branch and v4.1-rc8, and now I'm seeing problems with wb.list_lock recursion and "sleeping in atomic" scehduling issues. generic/269 produced this:
BUG: spinlock cpu recursion on CPU#1, fsstress/3852 lock: 0xffff88042a650c28, .magic: dead4ead, .owner: fsstress/3804, .owner_cpu: 1 CPU: 1 PID: 3852 Comm: fsstress Tainted: G W 4.1.0-rc8-dgc+ #263 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 ffff88042a650c28 ffff88039898b8e8 ffffffff81e18ffd ffff88042f250fb0 ffff880428f6b8e0 ffff88039898b908 ffffffff81e12f09 ffff88042a650c28 ffffffff8221337b ffff88039898b928 ffffffff81e12f34 ffff88042a650c28 Call Trace: [<ffffffff81e18ffd>] dump_stack+0x4c/0x6e [<ffffffff81e12f09>] spin_dump+0x90/0x95 [<ffffffff81e12f34>] spin_bug+0x26/0x2b [<ffffffff810e762d>] do_raw_spin_lock+0x10d/0x150 [<ffffffff81e24975>] _raw_spin_lock+0x15/0x20 [<ffffffff811f8ba0>] __mark_inode_dirty+0x2b0/0x450 [<ffffffff812003b8>] __set_page_dirty+0x78/0xd0 [<ffffffff81200531>] mark_buffer_dirty+0x61/0xf0 [<ffffffff81200d91>] __block_commit_write.isra.24+0x81/0xb0 [<ffffffff81202406>] block_write_end+0x36/0x70 [<ffffffff814fa110>] ? __xfs_get_blocks+0x8a0/0x8a0 [<ffffffff81202474>] generic_write_end+0x34/0xb0 [<ffffffff8118af3d>] ? wait_for_stable_page+0x1d/0x50 [<ffffffff814fa317>] xfs_vm_write_end+0x67/0xc0 [<ffffffff811813af>] pagecache_write_end+0x1f/0x30 [<ffffffff815060dd>] xfs_iozero+0x10d/0x190 [<ffffffff8150666b>] xfs_zero_last_block+0xdb/0x110 [<ffffffff815067ba>] xfs_zero_eof+0x11a/0x290 [<ffffffff811d69e0>] ? complete_walk+0x60/0x100 [<ffffffff811da25f>] ? path_lookupat+0x5f/0x660 [<ffffffff81506a6e>] xfs_file_aio_write_checks+0x13e/0x160 [<ffffffff81506f15>] xfs_file_buffered_aio_write+0x75/0x250 [<ffffffff811ddb0f>] ? user_path_at_empty+0x5f/0xa0 [<ffffffff810c601d>] ? __might_sleep+0x4d/0x90 [<ffffffff815071f5>] xfs_file_write_iter+0x105/0x120 [<ffffffff811cc5ce>] __vfs_write+0xae/0xf0 [<ffffffff811ccc01>] vfs_write+0xa1/0x190 [<ffffffff811cd999>] SyS_write+0x49/0xb0 [<ffffffff811cc781>] ? SyS_lseek+0x91/0xb0 [<ffffffff81e24fee>] system_call_fastpath+0x12/0x71
And there are a few tests (including generic/269) producing in_atomic/"scheduling while atomic" bugs in the evict() path such as:
in_atomic(): 1, irqs_disabled(): 0, pid: 3852, name: fsstress
CPU: 12 PID: 3852 Comm: fsstress Not tainted 4.1.0-rc8-dgc+ #263 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 000000000000015d ffff88039898b6d8 ffffffff81e18ffd 0000000000000000 ffff880398865550 ffff88039898b6f8 ffffffff810c5f89 ffff8803f15c45c0 ffffffff8227a3bf ffff88039898b728 ffffffff810c601d ffff88039898b758 Call Trace: [<ffffffff81e18ffd>] dump_stack+0x4c/0x6e [<ffffffff810c5f89>] ___might_sleep+0xf9/0x140 [<ffffffff810c601d>] __might_sleep+0x4d/0x90 [<ffffffff81201e8b>] block_invalidatepage+0xab/0x140 [<ffffffff814f7579>] xfs_vm_invalidatepage+0x39/0xb0 [<ffffffff8118fa77>] truncate_inode_page+0x67/0xa0 [<ffffffff8118fc92>] truncate_inode_pages_range+0x1a2/0x6f0 [<ffffffff811828d1>] ? find_get_pages_tag+0xf1/0x1b0 [<ffffffff8104a663>] ? __switch_to+0x1e3/0x5a0 [<ffffffff8118dd05>] ? pagevec_lookup_tag+0x25/0x40 [<ffffffff811f620d>] ? __inode_wait_for_writeback+0x6d/0xc0 [<ffffffff8119024c>] truncate_inode_pages_final+0x4c/0x60 [<ffffffff8151c47f>] xfs_fs_evict_inode+0x4f/0x100 [<ffffffff811e8330>] evict+0xc0/0x1a0 [<ffffffff811e8d7b>] iput+0x1bb/0x220 [<ffffffff811f68b3>] sync_inodes_sb+0x353/0x3d0 [<ffffffff8151def8>] xfs_flush_inodes+0x28/0x40 [<ffffffff81514648>] xfs_create+0x638/0x770 [<ffffffff814e9049>] ? xfs_dir2_sf_lookup+0x199/0x330 [<ffffffff81511091>] xfs_generic_create+0xd1/0x300 [<ffffffff817a059c>] ? security_inode_permission+0x1c/0x30 [<ffffffff815112f6>] xfs_vn_create+0x16/0x20 [<ffffffff811d8665>] vfs_create+0xd5/0x140 [<ffffffff811dbea3>] do_last+0xff3/0x1200 [<ffffffff811d9f36>] ? path_init+0x186/0x450 [<ffffffff811dc130>] path_openat+0x80/0x610 [<ffffffff81512a24>] ? xfs_iunlock+0xc4/0x210 [<ffffffff811ddbfa>] do_filp_open+0x3a/0x90 [<ffffffff811dc8bf>] ? getname_flags+0x4f/0x200 [<ffffffff81e249ce>] ? _raw_spin_unlock+0xe/0x30 [<ffffffff811eab17>] ? __alloc_fd+0xa7/0x130 [<ffffffff811cbcf8>] do_sys_open+0x128/0x220 [<ffffffff811cbe4e>] SyS_creat+0x1e/0x20 [<ffffffff81e24fee>] system_call_fastpath+0x12/0x71
It looks to me like iput() is being called with the wb.list_lock held in wait_sb_inodes(), and everything is going downhill from there. Patch below fixes the problem for me.
Cheers,
Dave.
Signature · 18 lines
Dave Chinner [email protected] sync: wait_sb_inodes() calls iput() with spinlock held. From: Dave Chinner <[email protected]> wait_sb_inodes() is triggering "sleeping in atomic" problems with blocking operations in iput() processing when wait_sb_inodes() releases the last reference to an inode. Fix it by delaying the iput() until the next loop pass when we aren't holding any spinlocks. Signed-off-by: Dave Chinner <[email protected]> --- fs/fs-writeback.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
fs/fs-writeback.c +15 -1
@@ -1436,6 +1436,7 @@ static void wait_sb_inodes(struct super_block *sb) {struct backing_dev_info *bdi = sb->s_bdi;LIST_HEAD (sync_list); +struct inode *iput_inode =NULL ;/* * We need to be protected against the filesystem going from
@@ -1497,6 +1498,9 @@ static void wait_sb_inodes(struct super_block *sb)spin_unlock (&inode->i_lock);spin_unlock (&bdi->wb.list_lock); +if (iput_inode) +iput (iput_inode); +filemap_fdatawait (mapping);cond_resched ();
@@ -1516,9 +1520,19 @@ static void wait_sb_inodes(struct super_block *sb) }else list_del_init (&inode->i_wb_list);spin_unlock_irq (&mapping->tree_lock); -iput (inode); + +/* +* can't iput inode while holding the wb.list_lock. Save it for +* the next time through the loop when we drop all our spin +* locks. +*/ + iput_inode = inode; }spin_unlock (&bdi->wb.list_lock); + +if (iput_inode) +iput (iput_inode); +mutex_unlock (&sb->s_sync_lock); }
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
Re: [PATCH] sync: wait_sb_inodes() calls iput() with spinlock held (was Re: [PATCH 0/7] super block scalabilit patches V3)
Quoted 103 lines · Dave Chinner
Quoted 7 lines · Dave Chinner
Quoted 6 lines · Josef Bacik
Here are the cleaned up versions of Dave Chinners super block scalability patches. I've been testing them locally for a while and they are pretty solid. They fix a few big issues, such as the global inode list and soft lockups on boxes on unmount that have lots of inodes in cache. Al if you would consider pulling these in that would be great, you can pull from here
Quoted 5 lines
Quoted 1 line
git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.gitsuperblock-scalingPasses all my smoke tests.
Tested-by Dave Chinner <[email protected]>FWIW, I just updated my trees to whatever is in the above branch and v4.1-rc8, and now I'm seeing problems with wb.list_lock recursion and "sleeping in atomic" scehduling issues. generic/269 produced this:
BUG: spinlock cpu recursion on CPU#1, fsstress/3852 lock: 0xffff88042a650c28, .magic: dead4ead, .owner: fsstress/3804, .owner_cpu: 1 CPU: 1 PID: 3852 Comm: fsstress Tainted: G W 4.1.0-rc8-dgc+ #263 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 ffff88042a650c28 ffff88039898b8e8 ffffffff81e18ffd ffff88042f250fb0 ffff880428f6b8e0 ffff88039898b908 ffffffff81e12f09 ffff88042a650c28 ffffffff8221337b ffff88039898b928 ffffffff81e12f34 ffff88042a650c28 Call Trace: [<ffffffff81e18ffd>] dump_stack+0x4c/0x6e [<ffffffff81e12f09>] spin_dump+0x90/0x95 [<ffffffff81e12f34>] spin_bug+0x26/0x2b [<ffffffff810e762d>] do_raw_spin_lock+0x10d/0x150 [<ffffffff81e24975>] _raw_spin_lock+0x15/0x20 [<ffffffff811f8ba0>] __mark_inode_dirty+0x2b0/0x450 [<ffffffff812003b8>] __set_page_dirty+0x78/0xd0 [<ffffffff81200531>] mark_buffer_dirty+0x61/0xf0 [<ffffffff81200d91>] __block_commit_write.isra.24+0x81/0xb0 [<ffffffff81202406>] block_write_end+0x36/0x70 [<ffffffff814fa110>] ? __xfs_get_blocks+0x8a0/0x8a0 [<ffffffff81202474>] generic_write_end+0x34/0xb0 [<ffffffff8118af3d>] ? wait_for_stable_page+0x1d/0x50 [<ffffffff814fa317>] xfs_vm_write_end+0x67/0xc0 [<ffffffff811813af>] pagecache_write_end+0x1f/0x30 [<ffffffff815060dd>] xfs_iozero+0x10d/0x190 [<ffffffff8150666b>] xfs_zero_last_block+0xdb/0x110 [<ffffffff815067ba>] xfs_zero_eof+0x11a/0x290 [<ffffffff811d69e0>] ? complete_walk+0x60/0x100 [<ffffffff811da25f>] ? path_lookupat+0x5f/0x660 [<ffffffff81506a6e>] xfs_file_aio_write_checks+0x13e/0x160 [<ffffffff81506f15>] xfs_file_buffered_aio_write+0x75/0x250 [<ffffffff811ddb0f>] ? user_path_at_empty+0x5f/0xa0 [<ffffffff810c601d>] ? __might_sleep+0x4d/0x90 [<ffffffff815071f5>] xfs_file_write_iter+0x105/0x120 [<ffffffff811cc5ce>] __vfs_write+0xae/0xf0 [<ffffffff811ccc01>] vfs_write+0xa1/0x190 [<ffffffff811cd999>] SyS_write+0x49/0xb0 [<ffffffff811cc781>] ? SyS_lseek+0x91/0xb0 [<ffffffff81e24fee>] system_call_fastpath+0x12/0x71And there are a few tests (including generic/269) producing in_atomic/"scheduling while atomic" bugs in the
evict()path such as:
in_atomic(): 1,irqs_disabled(): 0, pid: 3852, name: fsstressCPU: 12 PID: 3852 Comm: fsstress Not tainted 4.1.0-rc8-dgc+ #263 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 000000000000015d ffff88039898b6d8 ffffffff81e18ffd 0000000000000000 ffff880398865550 ffff88039898b6f8 ffffffff810c5f89 ffff8803f15c45c0 ffffffff8227a3bf ffff88039898b728 ffffffff810c601d ffff88039898b758 Call Trace: [<ffffffff81e18ffd>] dump_stack+0x4c/0x6e [<ffffffff810c5f89>] ___might_sleep+0xf9/0x140 [<ffffffff810c601d>] __might_sleep+0x4d/0x90 [<ffffffff81201e8b>] block_invalidatepage+0xab/0x140 [<ffffffff814f7579>] xfs_vm_invalidatepage+0x39/0xb0 [<ffffffff8118fa77>] truncate_inode_page+0x67/0xa0 [<ffffffff8118fc92>] truncate_inode_pages_range+0x1a2/0x6f0 [<ffffffff811828d1>] ? find_get_pages_tag+0xf1/0x1b0 [<ffffffff8104a663>] ? __switch_to+0x1e3/0x5a0 [<ffffffff8118dd05>] ? pagevec_lookup_tag+0x25/0x40 [<ffffffff811f620d>] ? __inode_wait_for_writeback+0x6d/0xc0 [<ffffffff8119024c>] truncate_inode_pages_final+0x4c/0x60 [<ffffffff8151c47f>] xfs_fs_evict_inode+0x4f/0x100 [<ffffffff811e8330>] evict+0xc0/0x1a0 [<ffffffff811e8d7b>] iput+0x1bb/0x220 [<ffffffff811f68b3>] sync_inodes_sb+0x353/0x3d0 [<ffffffff8151def8>] xfs_flush_inodes+0x28/0x40 [<ffffffff81514648>] xfs_create+0x638/0x770 [<ffffffff814e9049>] ? xfs_dir2_sf_lookup+0x199/0x330 [<ffffffff81511091>] xfs_generic_create+0xd1/0x300 [<ffffffff817a059c>] ? security_inode_permission+0x1c/0x30 [<ffffffff815112f6>] xfs_vn_create+0x16/0x20 [<ffffffff811d8665>] vfs_create+0xd5/0x140 [<ffffffff811dbea3>] do_last+0xff3/0x1200 [<ffffffff811d9f36>] ? path_init+0x186/0x450 [<ffffffff811dc130>] path_openat+0x80/0x610 [<ffffffff81512a24>] ? xfs_iunlock+0xc4/0x210 [<ffffffff811ddbfa>] do_filp_open+0x3a/0x90 [<ffffffff811dc8bf>] ? getname_flags+0x4f/0x200 [<ffffffff81e249ce>] ? _raw_spin_unlock+0xe/0x30 [<ffffffff811eab17>] ? __alloc_fd+0xa7/0x130 [<ffffffff811cbcf8>] do_sys_open+0x128/0x220 [<ffffffff811cbe4e>] SyS_creat+0x1e/0x20 [<ffffffff81e24fee>] system_call_fastpath+0x12/0x71
Quoted 8 lines
It looks to me like
iput()is being called with the wb.list_lock held inwait_sb_inodes(), and everything is going downhill from there. Patch below fixes the problem for me.Cheers,
Dave.
Thanks Dave I'll add it. I think this is what we were doing at first but then I changed it, didn't notice the wb.list_lock. Thanks,
Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
Re: [PATCH] sync: wait_sb_inodes() calls iput() with spinlock held (was Re: [PATCH 0/7] super block scalabilit patches V3)
In reply to Dave ChinnerQuoted 103 lines · Dave Chinner
Quoted 7 lines · Dave Chinner
Quoted 6 lines · Josef Bacik
Here are the cleaned up versions of Dave Chinners super block scalability patches. I've been testing them locally for a while and they are pretty solid. They fix a few big issues, such as the global inode list and soft lockups on boxes on unmount that have lots of inodes in cache. Al if you would consider pulling these in that would be great, you can pull from here
Quoted 5 lines
Quoted 1 line
git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.gitsuperblock-scalingPasses all my smoke tests.
Tested-by Dave Chinner <[email protected]>FWIW, I just updated my trees to whatever is in the above branch and v4.1-rc8, and now I'm seeing problems with wb.list_lock recursion and "sleeping in atomic" scehduling issues. generic/269 produced this:
BUG: spinlock cpu recursion on CPU#1, fsstress/3852 lock: 0xffff88042a650c28, .magic: dead4ead, .owner: fsstress/3804, .owner_cpu: 1 CPU: 1 PID: 3852 Comm: fsstress Tainted: G W 4.1.0-rc8-dgc+ #263 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 ffff88042a650c28 ffff88039898b8e8 ffffffff81e18ffd ffff88042f250fb0 ffff880428f6b8e0 ffff88039898b908 ffffffff81e12f09 ffff88042a650c28 ffffffff8221337b ffff88039898b928 ffffffff81e12f34 ffff88042a650c28 Call Trace: [<ffffffff81e18ffd>] dump_stack+0x4c/0x6e [<ffffffff81e12f09>] spin_dump+0x90/0x95 [<ffffffff81e12f34>] spin_bug+0x26/0x2b [<ffffffff810e762d>] do_raw_spin_lock+0x10d/0x150 [<ffffffff81e24975>] _raw_spin_lock+0x15/0x20 [<ffffffff811f8ba0>] __mark_inode_dirty+0x2b0/0x450 [<ffffffff812003b8>] __set_page_dirty+0x78/0xd0 [<ffffffff81200531>] mark_buffer_dirty+0x61/0xf0 [<ffffffff81200d91>] __block_commit_write.isra.24+0x81/0xb0 [<ffffffff81202406>] block_write_end+0x36/0x70 [<ffffffff814fa110>] ? __xfs_get_blocks+0x8a0/0x8a0 [<ffffffff81202474>] generic_write_end+0x34/0xb0 [<ffffffff8118af3d>] ? wait_for_stable_page+0x1d/0x50 [<ffffffff814fa317>] xfs_vm_write_end+0x67/0xc0 [<ffffffff811813af>] pagecache_write_end+0x1f/0x30 [<ffffffff815060dd>] xfs_iozero+0x10d/0x190 [<ffffffff8150666b>] xfs_zero_last_block+0xdb/0x110 [<ffffffff815067ba>] xfs_zero_eof+0x11a/0x290 [<ffffffff811d69e0>] ? complete_walk+0x60/0x100 [<ffffffff811da25f>] ? path_lookupat+0x5f/0x660 [<ffffffff81506a6e>] xfs_file_aio_write_checks+0x13e/0x160 [<ffffffff81506f15>] xfs_file_buffered_aio_write+0x75/0x250 [<ffffffff811ddb0f>] ? user_path_at_empty+0x5f/0xa0 [<ffffffff810c601d>] ? __might_sleep+0x4d/0x90 [<ffffffff815071f5>] xfs_file_write_iter+0x105/0x120 [<ffffffff811cc5ce>] __vfs_write+0xae/0xf0 [<ffffffff811ccc01>] vfs_write+0xa1/0x190 [<ffffffff811cd999>] SyS_write+0x49/0xb0 [<ffffffff811cc781>] ? SyS_lseek+0x91/0xb0 [<ffffffff81e24fee>] system_call_fastpath+0x12/0x71And there are a few tests (including generic/269) producing in_atomic/"scheduling while atomic" bugs in the
evict()path such as:
in_atomic(): 1,irqs_disabled(): 0, pid: 3852, name: fsstressCPU: 12 PID: 3852 Comm: fsstress Not tainted 4.1.0-rc8-dgc+ #263 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 000000000000015d ffff88039898b6d8 ffffffff81e18ffd 0000000000000000 ffff880398865550 ffff88039898b6f8 ffffffff810c5f89 ffff8803f15c45c0 ffffffff8227a3bf ffff88039898b728 ffffffff810c601d ffff88039898b758 Call Trace: [<ffffffff81e18ffd>] dump_stack+0x4c/0x6e [<ffffffff810c5f89>] ___might_sleep+0xf9/0x140 [<ffffffff810c601d>] __might_sleep+0x4d/0x90 [<ffffffff81201e8b>] block_invalidatepage+0xab/0x140 [<ffffffff814f7579>] xfs_vm_invalidatepage+0x39/0xb0 [<ffffffff8118fa77>] truncate_inode_page+0x67/0xa0 [<ffffffff8118fc92>] truncate_inode_pages_range+0x1a2/0x6f0 [<ffffffff811828d1>] ? find_get_pages_tag+0xf1/0x1b0 [<ffffffff8104a663>] ? __switch_to+0x1e3/0x5a0 [<ffffffff8118dd05>] ? pagevec_lookup_tag+0x25/0x40 [<ffffffff811f620d>] ? __inode_wait_for_writeback+0x6d/0xc0 [<ffffffff8119024c>] truncate_inode_pages_final+0x4c/0x60 [<ffffffff8151c47f>] xfs_fs_evict_inode+0x4f/0x100 [<ffffffff811e8330>] evict+0xc0/0x1a0 [<ffffffff811e8d7b>] iput+0x1bb/0x220 [<ffffffff811f68b3>] sync_inodes_sb+0x353/0x3d0 [<ffffffff8151def8>] xfs_flush_inodes+0x28/0x40 [<ffffffff81514648>] xfs_create+0x638/0x770 [<ffffffff814e9049>] ? xfs_dir2_sf_lookup+0x199/0x330 [<ffffffff81511091>] xfs_generic_create+0xd1/0x300 [<ffffffff817a059c>] ? security_inode_permission+0x1c/0x30 [<ffffffff815112f6>] xfs_vn_create+0x16/0x20 [<ffffffff811d8665>] vfs_create+0xd5/0x140 [<ffffffff811dbea3>] do_last+0xff3/0x1200 [<ffffffff811d9f36>] ? path_init+0x186/0x450 [<ffffffff811dc130>] path_openat+0x80/0x610 [<ffffffff81512a24>] ? xfs_iunlock+0xc4/0x210 [<ffffffff811ddbfa>] do_filp_open+0x3a/0x90 [<ffffffff811dc8bf>] ? getname_flags+0x4f/0x200 [<ffffffff81e249ce>] ? _raw_spin_unlock+0xe/0x30 [<ffffffff811eab17>] ? __alloc_fd+0xa7/0x130 [<ffffffff811cbcf8>] do_sys_open+0x128/0x220 [<ffffffff811cbe4e>] SyS_creat+0x1e/0x20 [<ffffffff81e24fee>] system_call_fastpath+0x12/0x71
Quoted 4 lines
It looks to me like
iput()is being called with the wb.list_lock held inwait_sb_inodes(), and everything is going downhill from there. Patch below fixes the problem for me.
I folded this into "bdi: add a new writeback list for sync" since it was there before and to be more bisect friendly. Let me know if this isn't ok with you and I'll undo it. Thanks,
Josef
Re: [PATCH] sync: wait_sb_inodes() calls iput() with spinlock held (was Re: [PATCH 0/7] super block scalabilit patches V3)
Quoted 19 lines · Josef Bacik
Quoted 18 lines · Dave Chinner
Quoted 7 lines · Dave Chinner
Quoted 6 lines · Josef Bacik
Here are the cleaned up versions of Dave Chinners super block scalability patches. I've been testing them locally for a while and they are pretty solid. They fix a few big issues, such as the global inode list and soft lockups on boxes on unmount that have lots of inodes in cache. Al if you would consider pulling these in that would be great, you can pull from here
Quoted 5 lines
Quoted 1 line
git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.gitsuperblock-scalingPasses all my smoke tests.
Tested-by Dave Chinner <[email protected]>FWIW, I just updated my trees to whatever is in the above branch and v4.1-rc8, and now I'm seeing problems with wb.list_lock recursion and "sleeping in atomic" scehduling issues. generic/269 produced this:
....
Quoted 8 lines
Quoted 4 lines
It looks to me like
iput()is being called with the wb.list_lock held inwait_sb_inodes(), and everything is going downhill from there. Patch below fixes the problem for me.I folded this into "bdi: add a new writeback list for sync" since it was there before and to be more bisect friendly. Let me know if this isn't ok with you and I'll undo it. Thanks,
That's fine - I was expecting you would fold it back in... ;)
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]