On Wed 09-12-15 13:40:30, Brian Foster wrote:
> On Tue, Jun 23, 2015 at 08:24:00PM -0700, Josef Bacik wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > 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.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > Signed-off-by: Josef Bacik <jbacik@xxxxxx>
> > Tested-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > Reviewed-by: Jan Kara <jack@xxxxxxx>
> > ---
Hi,
> My understanding is this (and the subsequent) patch were never merged
> due to conflicts with the cgroup aware writeback stuff that had been
> merged.
Yes. Thanks for bringing this up. At that point I thought Josef will take
care of rebasing the patches and it fell out of my radar.
> I think the fundamental approach of this patch still solves the
> original problem, the writeback inodes are just always tracked on the
> root bdi_writeback rather than the per-cgroup bdi_writeback the inode
> was associated with before writeback.
I was thinking about this and realized that the original patch actually has
a flaw. In case there are several partitions on a bdi and sync_fs() would
be called for two filesystems on the same bdi, then one of those sync_fs()
calls would splice the whole b_writeback list into its private list in
wait_sb_inodes() and the other sync_fs() call would return too early because
it would find b_writeback list empty.
Given that we need the list of inodes under writeback only for sync(2) /
sync_fs(2) purposes it would work the best to track those inodes in the
per-superblock list. We would miss block device inode this way but that one
is waited-on in a special way anyway (in __sync_blockdev() /
fdatawait_one_bdev()). Also that way any strange interaction and confusion
with cgroup writeback is avoided.
> Any thoughts on that are appreciated. Otherwise, I'm trying to rebase
> this and I've hit a lockdep splat [1] that suggests the locking for the
> writeback list needs to be tweaked one way or another. Note that lockdep
> fires with this original version as well, so I don't think this is a new
> issue.
Yeah, it looks that the issue has been there forever. When we have per-sb
list then we need a different lock to protect the list anyway. So having a
dedicated lock for the list which would be irqsave would be the easiest
solution. But note that in that case the locking of i_lock in
wait_inodes_sb() has to be dealt with as well since making i_lock irqsafe
is not an option.
I think we could avoid having the dedicated lock irqsafe if we moved
bdi_mark_inode_writeback() call from under mapping->tree_lock (to happen
later in __test_set_page_writeback()). However that needs some careful
thinking about how the interactions of __test_set_page_writeback() with the
list handling in wait_inodes_sb() work out.
Honza
> [1] lockdep splat:
>
> [ INFO: possible irq lock inversion dependency detected ]
> 4.4.0-rc4+ #54 Not tainted
> ---------------------------------------------------------
> swapper/0/0 just changed the state of lock:
> (&(&mapping->tree_lock)->rlock){-.....}, at: [<ffffffff811d25ed>] test_clear_page_writeback+0x5d/0x210
> but this lock took another, HARDIRQ-unsafe lock in the past:
> (&(&wb->list_lock)->rlock){+.+...}#012#012and interrupts could create inverse lock ordering between them.
> #012other info that might help us debug this:
> Possible interrupt unsafe locking scenario:
> CPU0 CPU1
> ---- ----
> lock(&(&wb->list_lock)->rlock);
> local_irq_disable();
> lock(&(&mapping->tree_lock)->rlock);
> lock(&(&wb->list_lock)->rlock);
> <Interrupt>
> lock(&(&mapping->tree_lock)->rlock);
> #012 *** DEADLOCK ***
> no locks held by swapper/0/0.
> #012the shortest dependencies between 2nd lock and 1st lock:
> -> (&(&wb->list_lock)->rlock){+.+...} ops: 194 {
> HARDIRQ-ON-W at:
> [<ffffffff810f9959>] __lock_acquire+0x819/0x1a50
> [<ffffffff810fb54e>] lock_acquire+0xce/0x1c0
> [<ffffffff817e3df8>] _raw_spin_lock+0x38/0x50
> [<ffffffff8126b7a9>] __mark_inode_dirty+0x3d9/0x5e0
> [<ffffffff8125a859>] generic_update_time+0x79/0xd0
> [<ffffffff8125c638>] touch_atime+0xa8/0xd0
> [<ffffffff812519c3>] iterate_dir+0xe3/0x130
> [<ffffffff81251ecd>] SyS_getdents+0x9d/0x130
> [<ffffffff817e4a72>] entry_SYSCALL_64_fastpath+0x12/0x76
> SOFTIRQ-ON-W at:
> [<ffffffff810f998a>] __lock_acquire+0x84a/0x1a50
> [<ffffffff810fb54e>] lock_acquire+0xce/0x1c0
> [<ffffffff817e3df8>] _raw_spin_lock+0x38/0x50
> [<ffffffff8126b7a9>] __mark_inode_dirty+0x3d9/0x5e0
> [<ffffffff8125a859>] generic_update_time+0x79/0xd0
> [<ffffffff8125c638>] touch_atime+0xa8/0xd0
> [<ffffffff812519c3>] iterate_dir+0xe3/0x130
> [<ffffffff81251ecd>] SyS_getdents+0x9d/0x130
> [<ffffffff817e4a72>] entry_SYSCALL_64_fastpath+0x12/0x76
> INITIAL USE at:
> [<ffffffff810f9538>] __lock_acquire+0x3f8/0x1a50
> [<ffffffff810fb54e>] lock_acquire+0xce/0x1c0
> [<ffffffff817e3df8>] _raw_spin_lock+0x38/0x50
> [<ffffffff8126b7a9>] __mark_inode_dirty+0x3d9/0x5e0
> [<ffffffff8125a859>] generic_update_time+0x79/0xd0
> [<ffffffff8125c638>] touch_atime+0xa8/0xd0
> [<ffffffff812519c3>] iterate_dir+0xe3/0x130
> [<ffffffff81251ecd>] SyS_getdents+0x9d/0x130
> [<ffffffff817e4a72>] entry_SYSCALL_64_fastpath+0x12/0x76
> }
> ... key at: [<ffffffff82c7faa0>] __key.37388+0x0/0x8
> ... acquired at:
> [<ffffffff810fb54e>] lock_acquire+0xce/0x1c0
> [<ffffffff817e3df8>] _raw_spin_lock+0x38/0x50
> [<ffffffff812703a7>] bdi_mark_inode_writeback+0x67/0xd0
> [<ffffffff811d19b2>] __test_set_page_writeback+0x112/0x200
> [<ffffffff8127bcdc>] __block_write_full_page.constprop.44+0xec/0x400
> [<ffffffff8127c0ee>] block_write_full_page+0xfe/0x190
> [<ffffffff8127cec8>] blkdev_writepage+0x18/0x20
> [<ffffffff811cede6>] __writepage+0x16/0x40
> [<ffffffff811cfc65>] write_cache_pages+0x225/0x5f0
> [<ffffffff811d0084>] generic_writepages+0x54/0x80
> [<ffffffff811d24e1>] do_writepages+0x21/0x30
> [<ffffffff811c4b40>] __filemap_fdatawrite_range+0x80/0xb0
> [<ffffffff811c4c7d>] filemap_write_and_wait_range+0x2d/0x70
> [<ffffffff8127ca8b>] blkdev_fsync+0x1b/0x50
> [<ffffffff81274a9b>] vfs_fsync_range+0x4b/0xb0
> [<ffffffff81274b5d>] do_fsync+0x3d/0x70
> [<ffffffff81274e10>] SyS_fsync+0x10/0x20
> [<ffffffff817e4a72>] entry_SYSCALL_64_fastpath+0x12/0x76
>
> -> (&(&mapping->tree_lock)->rlock){-.....} ops: 16821 {
> IN-HARDIRQ-W at:
> [<ffffffff810f9ad9>] __lock_acquire+0x999/0x1a50
> [<ffffffff810fb54e>] lock_acquire+0xce/0x1c0
> [<ffffffff817e4810>] _raw_spin_lock_irqsave+0x50/0x70
> [<ffffffff811d25ed>] test_clear_page_writeback+0x5d/0x210
> [<ffffffff811c2b2f>] end_page_writeback+0x1f/0xa0
> [<ffffffff81278df3>] end_buffer_async_write+0xd3/0x1a0
> [<ffffffff81277828>] end_bio_bh_io_sync+0x28/0x40
> [<ffffffff813a329f>] bio_endio+0x3f/0x60
> [<ffffffff8162d4d9>] dec_pending+0x149/0x310
> [<ffffffff8162e1d6>] clone_endio+0x76/0xe0
> [<ffffffff813a329f>] bio_endio+0x3f/0x60
> [<ffffffff813ab512>] blk_update_request+0xb2/0x3a0
> [<ffffffff813b63ca>] blk_mq_end_request+0x1a/0x70
> [<ffffffffa009e82f>] virtblk_request_done+0x3f/0x70 [virtio_blk]
> [<ffffffff813b4ea3>] __blk_mq_complete_request_remote+0x13/0x20
> [<ffffffff81137d9f>] flush_smp_call_function_queue+0x5f/0x160
> [<ffffffff811388d3>] generic_smp_call_function_single_interrupt+0x13/0x60
> [<ffffffff810530a7>] smp_call_function_single_interrupt+0x27/0x40
> [<ffffffff817e604c>] call_function_single_interrupt+0x8c/0xa0
> [<ffffffff81026703>] default_idle+0x23/0x150
> [<ffffffff8102702f>] arch_cpu_idle+0xf/0x20
> [<ffffffff810efd8a>] default_idle_call+0x2a/0x40
> [<ffffffff810f0184>] cpu_startup_entry+0x384/0x3f0
> [<ffffffff817d6aaa>] rest_init+0x13a/0x140
> [<ffffffff81d6b01f>] start_kernel+0x49e/0x4bf
> [<ffffffff81d6a339>] x86_64_start_reservations+0x2a/0x2c
> [<ffffffff81d6a480>] x86_64_start_kernel+0x145/0x168
> INITIAL USE at:
> [<ffffffff810f9538>] __lock_acquire+0x3f8/0x1a50
> [<ffffffff810fb54e>] lock_acquire+0xce/0x1c0
> [<ffffffff817e3fa4>] _raw_spin_lock_irq+0x44/0x60
> [<ffffffff811c303c>] __add_to_page_cache_locked+0xbc/0x340
> [<ffffffff811c3317>] add_to_page_cache_lru+0x37/0x90
> [<ffffffff811c3dc7>] pagecache_get_page+0xb7/0x200
> [<ffffffff811c4139>] grab_cache_page_write_begin+0x29/0x40
> [<ffffffff81269848>] simple_write_begin+0x28/0x1c0
> [<ffffffff811c2a01>] generic_perform_write+0xd1/0x1e0
> [<ffffffff811c55f2>] __generic_file_write_iter+0x1a2/0x1e0
> [<ffffffff811c5718>] generic_file_write_iter+0xe8/0x1e0
> [<ffffffff8123c409>] __vfs_write+0xc9/0x110
> [<ffffffff8123ca99>] vfs_write+0xa9/0x1a0
> [<ffffffff8123d778>] SyS_write+0x58/0xd0
> [<ffffffff81d6c3b1>] xwrite+0x29/0x5c
> [<ffffffff81d6c40e>] do_copy+0x2a/0xb8
> [<ffffffff81d6c151>] write_buffer+0x23/0x34
> [<ffffffff81d6c9e6>] unpack_to_rootfs+0xfd/0x294
> [<ffffffff81d6cbd9>] populate_rootfs+0x5c/0x108
> [<ffffffff81002123>] do_one_initcall+0xb3/0x200
> [<ffffffff81d6b22e>] kernel_init_freeable+0x1ee/0x28d
> [<ffffffff817d6abe>] kernel_init+0xe/0xe0
> [<ffffffff817e4ddf>] ret_from_fork+0x3f/0x70
> }
> ... key at: [<ffffffff82ca2760>] __key.39560+0x0/0x8
> ... acquired at:
> [<ffffffff810f7c1b>] check_usage_forwards+0x15b/0x160
> [<ffffffff810f88c3>] mark_lock+0x333/0x610
> [<ffffffff810f9ad9>] __lock_acquire+0x999/0x1a50
> [<ffffffff810fb54e>] lock_acquire+0xce/0x1c0
> [<ffffffff817e4810>] _raw_spin_lock_irqsave+0x50/0x70
> [<ffffffff811d25ed>] test_clear_page_writeback+0x5d/0x210
> [<ffffffff811c2b2f>] end_page_writeback+0x1f/0xa0
> [<ffffffff81278df3>] end_buffer_async_write+0xd3/0x1a0
> [<ffffffff81277828>] end_bio_bh_io_sync+0x28/0x40
> [<ffffffff813a329f>] bio_endio+0x3f/0x60
> [<ffffffff8162d4d9>] dec_pending+0x149/0x310
> [<ffffffff8162e1d6>] clone_endio+0x76/0xe0
> [<ffffffff813a329f>] bio_endio+0x3f/0x60
> [<ffffffff813ab512>] blk_update_request+0xb2/0x3a0
> [<ffffffff813b63ca>] blk_mq_end_request+0x1a/0x70
> [<ffffffffa009e82f>] virtblk_request_done+0x3f/0x70 [virtio_blk]
> [<ffffffff813b4ea3>] __blk_mq_complete_request_remote+0x13/0x20
> [<ffffffff81137d9f>] flush_smp_call_function_queue+0x5f/0x160
> [<ffffffff811388d3>] generic_smp_call_function_single_interrupt+0x13/0x60
> [<ffffffff810530a7>] smp_call_function_single_interrupt+0x27/0x40
> [<ffffffff817e604c>] call_function_single_interrupt+0x8c/0xa0
> [<ffffffff81026703>] default_idle+0x23/0x150
> [<ffffffff8102702f>] arch_cpu_idle+0xf/0x20
> [<ffffffff810efd8a>] default_idle_call+0x2a/0x40
> [<ffffffff810f0184>] cpu_startup_entry+0x384/0x3f0
> [<ffffffff817d6aaa>] rest_init+0x13a/0x140
> [<ffffffff81d6b01f>] start_kernel+0x49e/0x4bf
> [<ffffffff81d6a339>] x86_64_start_reservations+0x2a/0x2c
> [<ffffffff81d6a480>] x86_64_start_kernel+0x145/0x168
>
> #012stack backtrace:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.4.0-rc4+ #54
> Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> 0000000000000000 06776aa479e380f6 ffff88011fc03ad0 ffffffff813dd019
> ffffffff82a0f120 ffff88011fc03b10 ffffffff811c184d ffffffff81c10be8
> ffffffff81c10be8 ffffffff81c10500 ffffffff81a56510 0000000000000000
> Call Trace:
> <IRQ> [<ffffffff813dd019>] dump_stack+0x4b/0x72
> [<ffffffff811c184d>] print_irq_inversion_bug.part.38+0x1a4/0x1b0
> [<ffffffff810f7c1b>] check_usage_forwards+0x15b/0x160
> [<ffffffff810f88c3>] mark_lock+0x333/0x610
> [<ffffffff810f7ac0>] ? check_usage_backwards+0x160/0x160
> [<ffffffff810f9ad9>] __lock_acquire+0x999/0x1a50
> [<ffffffff810fb54e>] lock_acquire+0xce/0x1c0
> [<ffffffff811d25ed>] ? test_clear_page_writeback+0x5d/0x210
> [<ffffffff817e4810>] _raw_spin_lock_irqsave+0x50/0x70
> [<ffffffff811d25ed>] ? test_clear_page_writeback+0x5d/0x210
> [<ffffffff811d25ed>] test_clear_page_writeback+0x5d/0x210
> [<ffffffff811c2b2f>] end_page_writeback+0x1f/0xa0
> [<ffffffff81278df3>] end_buffer_async_write+0xd3/0x1a0
> [<ffffffff81277828>] end_bio_bh_io_sync+0x28/0x40
> [<ffffffff813a329f>] bio_endio+0x3f/0x60
> [<ffffffff8162d4d9>] dec_pending+0x149/0x310
> [<ffffffff811c6809>] ? mempool_free+0x29/0x80
> [<ffffffff8162e1d6>] clone_endio+0x76/0xe0
> [<ffffffff813a329f>] bio_endio+0x3f/0x60
> [<ffffffff813ab512>] blk_update_request+0xb2/0x3a0
> [<ffffffff813b4e90>] ? blkdev_issue_zeroout+0xf0/0xf0
> [<ffffffff813b63ca>] blk_mq_end_request+0x1a/0x70
> [<ffffffffa009e82f>] virtblk_request_done+0x3f/0x70 [virtio_blk]
> [<ffffffff813b4ea3>] __blk_mq_complete_request_remote+0x13/0x20
> [<ffffffff81137d9f>] flush_smp_call_function_queue+0x5f/0x160
> [<ffffffff811388d3>] generic_smp_call_function_single_interrupt+0x13/0x60
> [<ffffffff810530a7>] smp_call_function_single_interrupt+0x27/0x40
> [<ffffffff817e604c>] call_function_single_interrupt+0x8c/0xa0
> <EOI> [<ffffffff810653a6>] ? native_safe_halt+0x6/0x10
> [<ffffffff81026703>] default_idle+0x23/0x150
> [<ffffffff8102702f>] arch_cpu_idle+0xf/0x20
> [<ffffffff810efd8a>] default_idle_call+0x2a/0x40
> [<ffffffff810f0184>] cpu_startup_entry+0x384/0x3f0
> [<ffffffff817d6aaa>] rest_init+0x13a/0x140
> [<ffffffff81d6b01f>] start_kernel+0x49e/0x4bf
> [<ffffffff81d6a120>] ? early_idt_handler_array+0x120/0x120
> [<ffffffff81d6a339>] x86_64_start_reservations+0x2a/0x2c
> [<ffffffff81d6a480>] x86_64_start_kernel+0x145/0x168
>
> > fs/block_dev.c | 2 +
> > fs/fs-writeback.c | 148 +++++++++++++++++++++++++++++++++++---------
> > 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, 144 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/block_dev.c b/fs/block_dev.c
> > index f2a89be..2726ff1 100644
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -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;
> >
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index aa72536..5c005ad 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -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,9 @@ 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);
> > + struct inode *iput_inode = NULL;
> >
> > /*
> > * We need to be protected against the filesystem going from
> > @@ -1380,48 +1425,95 @@ 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);
> > + spin_unlock(&bdi->wb.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;
> > + if (iput_inode)
> > + iput(iput_inode);
> >
> > 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);
> > +
> > + /*
> > + * 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(&sb->s_inode_list_lock);
> > - iput(old_inode);
> > + spin_unlock(&bdi->wb.list_lock);
> > +
> > + if (iput_inode)
> > + iput(iput_inode);
> > +
> > mutex_unlock(&sb->s_sync_lock);
> > }
> >
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 770d684..8f00557 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -357,6 +357,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);
> > diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> > index d87d8ec..0d5bca2 100644
> > --- a/include/linux/backing-dev.h
> > +++ b/include/linux/backing-dev.h
> > @@ -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 */
> > };
> >
> > @@ -123,6 +124,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;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 6dd2ab2..d3baa08 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -636,6 +636,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;
> > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> > index e9ed047..ea39301 100644
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -369,6 +369,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);
> > }
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index eb59f7e..f3751d1 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -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,
> > --
> > 2.1.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
