From patchwork Fri Sep 11 20:37:19 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Linus Torvalds X-Patchwork-Id: 7163841 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 2E8FCBEEC1 for ; Fri, 11 Sep 2015 20:37:36 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8BEF02081E for ; Fri, 11 Sep 2015 20:37:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D7CBE20805 for ; Fri, 11 Sep 2015 20:37:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752890AbbIKUhV (ORCPT ); Fri, 11 Sep 2015 16:37:21 -0400 Received: from mail-ig0-f169.google.com ([209.85.213.169]:34895 "EHLO mail-ig0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751841AbbIKUhU (ORCPT ); Fri, 11 Sep 2015 16:37:20 -0400 Received: by igbkq10 with SMTP id kq10so50495030igb.0; Fri, 11 Sep 2015 13:37:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:content-type; bh=ocBgsN0Vf1v2Ghf6O5AGlzMSyNgJkvsiqZGeVAyi0yI=; b=iiBLg8IcK5yyZXc7y/vskzp3qQXdu3P7+j6gnfKrkXB3Ap0mtO+/YObLDyxhcYr16q aH49LHRY73zRtA0M6MYS7BczIz4bOl2N1ZjA9I+tZO186ZRGviGErYI8OoZ25YzKqOhY qYtCEdUq2V4XKDQz9UZAYR02Z1CRAi62MVE8fr1zF9umfZxHkwhTzCKWubIgBbSaU1YG S4QKf+QnKp9M6YjVetU9Cg9+O8z9FPvq+9cj8Aq3JjBKh0tIGPsc/zEy9uL91vMIACGA AOm1SRUHGi4SdLALyhROwiI4EqiOKsLT/xZQMbF6CiWX6UiEzfRMAZbYvP7QLsVTi6jL +h4g== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:content-type; bh=ocBgsN0Vf1v2Ghf6O5AGlzMSyNgJkvsiqZGeVAyi0yI=; b=iBEJiruXLutgzqNCPVkUNXzPlS4019crh9KV87FT/6PbBPfgjtTwNQBboi3yzRCl8G YHLdQApIjKIqvGE2ScbFTBurPeon5JNHAwT/+3UHdFIC8+8LlKDy6LzlgZcKigfI+4uw wJCINUUyYzPJAdtGn3MoEPMRE6bRiHOvKIaAc= MIME-Version: 1.0 X-Received: by 10.50.79.197 with SMTP id l5mr153652igx.93.1442003839177; Fri, 11 Sep 2015 13:37:19 -0700 (PDT) Received: by 10.36.124.195 with HTTP; Fri, 11 Sep 2015 13:37:19 -0700 (PDT) In-Reply-To: References: <20150911193747.GA4150@ret.masoncoding.com> Date: Fri, 11 Sep 2015 13:37:19 -0700 X-Google-Sender-Auth: Wi6Z7IogsCb_dQ1QlboGqnsqeeA Message-ID: Subject: Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() From: Linus Torvalds To: Chris Mason , LKML , linux-fsdevel , Josef Bacik , Dave Chinner , Neil Brown , Jan Kara , Christoph Hellwig Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID,T_RP_MATCHES_RCVD,T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Fri, Sep 11, 2015 at 1:02 PM, Linus Torvalds 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; }