On Thu, Dec 10, 2015 at 11:08:20AM +0100, Jan Kara wrote:
> 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.
>
Hmm, yeah that does sound like a problem...
> 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.
>
Agree, that sounds like a more clean approach on first thought. We kill
the more wasteful per-bdi_writeback list since it would only be used by
the root structure embedded in backing_dev_info and isolates the code to
dealing with sync. I'll have a closer look at this.
> > 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.
>
Ok...
> 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.
>
That thought crossed my mind briefly as well. It looked like it could be
racy once we drop mapping->tree_lock, but actually I wonder if the lazy
list removal technique helps us get away with that (e.g., it's expected
that the writeback list will be populated with non-writeback inodes for
some time as it is).
Anyways, I can take a closer look at this as well. It could be a couple
weeks or so before I get back to this, but I'll try to incorporate the
above once I do. Thanks for the feedback! :)
Brian
> 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
