Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug()

[Linus Torvalds]

On Fri, Sep 11, 2015 at 1:02 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
>
> How about we instead:
>
>  (a) revert that commit d353d7587 as broken (because it clearly is)
>
>  (b) add a big honking comment about the fact that we hold 'list_lock'
> in writeback_sb_inodes()
>
>  (c) move the plugging up to wb_writeback() and writeback_inodes_wb()
> _outside_ the spinlock.

Ok, I've done (a) and (b) in my tree. And attached is the totally
untested patch for (c). It looks ObviouslyCorrect(tm), but since this
is a performance issue, I'm not going to commit it without some more
ACK's from people.

I obviously think this is a *much* better approach than dropping and
retaking the lock, but there might be something silly I'm missing.

For example, maybe we want to unplug and replug around the
"inode_sleep_on_writeback()" in wb_writeback()? So while the revert
was a no-brainer, this one I really want people to think about.

                       Linus

 fs/fs-writeback.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index d8ea7ed411b2..587ac08eabb6 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1546,12 +1546,15 @@ static long writeback_inodes_wb(struct bdi_writeback 
*wb, long nr_pages,
                .range_cyclic   = 1,
                .reason         = reason,
        };
+       struct blk_plug plug;
 
+       blk_start_plug(&plug);
        spin_lock(&wb->list_lock);
        if (list_empty(&wb->b_io))
                queue_io(wb, &work);
        __writeback_inodes_wb(wb, &work);
        spin_unlock(&wb->list_lock);
+       blk_finish_plug(&plug);
 
        return nr_pages - work.nr_pages;
 }
@@ -1579,10 +1582,12 @@ static long wb_writeback(struct bdi_writeback *wb,
        unsigned long oldest_jif;
        struct inode *inode;
        long progress;
+       struct blk_plug plug;
 
        oldest_jif = jiffies;
        work->older_than_this = &oldest_jif;
 
+       blk_start_plug(&plug);
        spin_lock(&wb->list_lock);
        for (;;) {
                /*
@@ -1662,6 +1667,7 @@ static long wb_writeback(struct bdi_writeback *wb,
                }
        }
        spin_unlock(&wb->list_lock);
+       blk_finish_plug(&plug);
 
        return nr_pages - work->nr_pages;
 }