[mm-unstable,RFC,1/5] writeback: move wb_over_bg_thresh() call outside lock section
Message ID | 20230403220337.443510-2-yosryahmed@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp2611209vqo; Mon, 3 Apr 2023 15:05:17 -0700 (PDT) X-Google-Smtp-Source: AKy350Z58i51j5qwQ/O70t7ASen0f9zYWQNxl9gp0sLYkVYBHj6iuXsS9Gi9u6BlZhRPavKi6v37 X-Received: by 2002:a05:6a20:65a2:b0:da:7d8a:2cc9 with SMTP id p34-20020a056a2065a200b000da7d8a2cc9mr210448pzh.24.1680559517738; Mon, 03 Apr 2023 15:05:17 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1680559517; cv=none; d=google.com; s=arc-20160816; b=HEca+EVaY6nrkyl2s4Ta3a2JyoXBJpHUoAXkmBSUc+Ll3BAJfDZGwf0sO7qGK2ucni FOiCQA6FTidqMwo44E2UV3AQ+lXlHxNTwiFxtY/8dVHHWxvITzoZPaCuBXphbAfdrOjY E906kIJTiVum63SZM1aDzPn8Bog2N4r1I4Nr1tvLfiCIjQ6z4SHKSc4ezUk+ioF3Qwjz VTLgBV19CReeBCZ7psroredHTlQU7c9jrciObnjCA7fnQTpPdhCwNhCGcg5nVmN43bqI WI6mKXKXfWONmrN6ACLuZZvbXYIcPv77oBHVx1R0dLAqW4cua1qr9McTFEvfBy5LG8xA LMxA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:dkim-signature; bh=kId3QKjERA5KglVeO1VKvGvv5azlGeISt3c2gJrYgyo=; b=fWqW8a5WxauP3f2fRta6xjpIzYNLzH4mc9k13sj7YabrgAkkkloZN9qY6/SGW9g+zm enhrIR/w9PblQTquHz6ptcLm1y8Mq6Ksj+KgE0xawMlnaiAcqI5fgiKamspADHRxt6gB gL0s3a9eieTvJ/T9cOAtzPGgP9788ldseYrQkZP/PuW2pA2cja5R5Tksrg/lml10E/eO aPySNUMqH0X2Z1o5pA4qdoOVi1/jetDEXb/Hvjt7hHOinsGR5Lfu8UpNnz2kAve0re70 Nu1YjnaNYN+4k0fc7XAKTa54BeLyhmaFrIpbhFkzZqsbHrMgpbLV1j/dV+Ma0QcnT1pO zGKw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b="W/ze/sAl"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id g130-20020a636b88000000b0050faad0977dsi8153826pgc.594.2023.04.03.15.05.04; Mon, 03 Apr 2023 15:05:17 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b="W/ze/sAl"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233650AbjDCWDs (ORCPT <rfc822;zwp10758@gmail.com> + 99 others); Mon, 3 Apr 2023 18:03:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56982 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232029AbjDCWDm (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 3 Apr 2023 18:03:42 -0400 Received: from mail-pl1-x649.google.com (mail-pl1-x649.google.com [IPv6:2607:f8b0:4864:20::649]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 99D03270F for <linux-kernel@vger.kernel.org>; Mon, 3 Apr 2023 15:03:41 -0700 (PDT) Received: by mail-pl1-x649.google.com with SMTP id p9-20020a170902e74900b001a1c7b2e7afso18011386plf.0 for <linux-kernel@vger.kernel.org>; Mon, 03 Apr 2023 15:03:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680559421; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=kId3QKjERA5KglVeO1VKvGvv5azlGeISt3c2gJrYgyo=; b=W/ze/sAlU7O+7GhoVMA+gO+b3zK1GLSBb2wxG7cqOl+9vjmm6QdV40GnYhvFBAr6we 6QpU1r1iDXHFCTCDI3KvZCA5Fdx66xMvZLtqAf+RggvSzScbqU6Qb+D3U4BXwhb/OMA1 mj7EUoKa/2hKudyISX9IDUT5QtGjglqN6doUhxmfcL1+w5LR9gcm9RCdAs4iQ3yY+fKw 8EP2M2Ss5aeWYRlPCW8BW8qsios85nr5jm7RooCKJYkRU/xUY3Tcdg08l4PT4At3QjPp 8RJrh+/qwzB7v9VlZ60Az/txlakHCjAKDKadt2/B61f8k1qeYBs4X+ujruMC4SFwxMjN IvmA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680559421; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=kId3QKjERA5KglVeO1VKvGvv5azlGeISt3c2gJrYgyo=; b=JZm+cP9rP79FBj0yIznPWl6LmmSiTI/fNFMRxHEQdnI1ZHejhfGinWyyzNghPakpDi 8DZ7zOQ69wN/fsHe/Hou5D1S6LINbjKg9ocF0ylPBoN2o5SADcz2CwsGk7RKvUdoasxI 7BkHHBbSMVtOuNNanpQQ6/L0M4afFWow8HDodUPSRgL0XW7nM+3y38jW++nnauoXiUoE QiCmGMkf4lJ3NQCBJlOi2tbBk5GV3TZjr7iuinFip2zVvjjeejiYXnjY2RbK67Q/PCV/ uiTEf2i9t7h32Vo/iT2MaEv5CtfaAllyHZ3Ve60wNAwPPQLR3xJIGBrU962M96jA2qhB Y/wQ== X-Gm-Message-State: AAQBX9dZrVdlKUxTzPKNVOGHOKp+zImpTEe/XqjQT1itObWQEGDGK7wb U10Iuy/Er8dyvnC/XWYAIgBpOZNOiDIPMU06 X-Received: from yosry.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:2327]) (user=yosryahmed job=sendgmr) by 2002:a63:f307:0:b0:50c:bd0:eb8c with SMTP id l7-20020a63f307000000b0050c0bd0eb8cmr39435pgh.6.1680559421112; Mon, 03 Apr 2023 15:03:41 -0700 (PDT) Date: Mon, 3 Apr 2023 22:03:33 +0000 In-Reply-To: <20230403220337.443510-1-yosryahmed@google.com> Mime-Version: 1.0 References: <20230403220337.443510-1-yosryahmed@google.com> X-Mailer: git-send-email 2.40.0.348.gf938b09366-goog Message-ID: <20230403220337.443510-2-yosryahmed@google.com> Subject: [PATCH mm-unstable RFC 1/5] writeback: move wb_over_bg_thresh() call outside lock section From: Yosry Ahmed <yosryahmed@google.com> To: Alexander Viro <viro@zeniv.linux.org.uk>, Christian Brauner <brauner@kernel.org>, Johannes Weiner <hannes@cmpxchg.org>, Michal Hocko <mhocko@kernel.org>, Roman Gushchin <roman.gushchin@linux.dev>, Shakeel Butt <shakeelb@google.com>, Muchun Song <songmuchun@bytedance.com>, Andrew Morton <akpm@linux-foundation.org> Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-mm@kvack.org, Yosry Ahmed <yosryahmed@google.com> Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-7.7 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1762194376982689166?= X-GMAIL-MSGID: =?utf-8?q?1762194376982689166?= |
Series |
cgroup: eliminate atomic rstat
|
|
Commit Message
Yosry Ahmed
April 3, 2023, 10:03 p.m. UTC
wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat
flush, which can be expensive on large systems. Currently,
wb_writeback() calls wb_over_bg_thresh() within a lock section, so we
have to make the rstat flush atomically. On systems with a lot of
cpus/cgroups, this can cause us to disable irqs for a long time,
potentially causing problems.
Move the call to wb_over_bg_thresh() outside the lock section in
preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic.
The list_empty(&wb->work_list) should be okay outside the lock section
of wb->list_lock as it is protected by a separate lock (wb->work_lock),
and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_*
lists the wb->list_lock is protecting. Also, the loop seems to be
already releasing and reacquring the lock, so this refactoring looks
safe.
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
---
fs/fs-writeback.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
Comments
On Mon, Apr 03, 2023 at 10:03:33PM +0000, Yosry Ahmed <yosryahmed@google.com> wrote: > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > flush, which can be expensive on large systems. Currently, > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > have to make the rstat flush atomically. On systems with a lot of > cpus/cgroups, this can cause us to disable irqs for a long time, > potentially causing problems. > > Move the call to wb_over_bg_thresh() outside the lock section in > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > The list_empty(&wb->work_list) should be okay outside the lock section > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > lists the wb->list_lock is protecting. Also, the loop seems to be > already releasing and reacquring the lock, so this refactoring looks > safe. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > fs/fs-writeback.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 195dc23e0d831..012357bc8daa3 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > struct blk_plug plug; > > blk_start_plug(&plug); > - spin_lock(&wb->list_lock); > for (;;) { > /* > * Stop writeback when nr_pages has been consumed > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > if (work->for_background && !wb_over_bg_thresh(wb)) > break; > > + > + spin_lock(&wb->list_lock); > + > /* > * Kupdate and background works are special and we want to > * include all inodes that need writing. Livelock avoidance is > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > * mean the overall work is done. So we keep looping as long > * as made some progress on cleaning pages or inodes. > */ > - if (progress) > + if (progress) { > + spin_unlock(&wb->list_lock); > continue; > + } > + This would release wb->list_lock temporarily with progress but that's already not held continuously due to writeback_sb_inodes(). Holding the lock could even be shortened by taking it later after trace_writeback_start(). Altogether, the change looks OK, Reviewed-by: Michal Koutný <mkoutny@suse.com>
+Jens & Jan The patch looks good but it would be nice to pass this patch through the eyes of experts of this area. On Mon, Apr 3, 2023 at 3:03 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > flush, which can be expensive on large systems. Currently, > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > have to make the rstat flush atomically. On systems with a lot of > cpus/cgroups, this can cause us to disable irqs for a long time, > potentially causing problems. > > Move the call to wb_over_bg_thresh() outside the lock section in > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > The list_empty(&wb->work_list) should be okay outside the lock section > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > lists the wb->list_lock is protecting. Also, the loop seems to be > already releasing and reacquring the lock, so this refactoring looks > safe. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > fs/fs-writeback.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 195dc23e0d831..012357bc8daa3 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > struct blk_plug plug; > > blk_start_plug(&plug); > - spin_lock(&wb->list_lock); > for (;;) { > /* > * Stop writeback when nr_pages has been consumed > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > if (work->for_background && !wb_over_bg_thresh(wb)) > break; > > + > + spin_lock(&wb->list_lock); > + > /* > * Kupdate and background works are special and we want to > * include all inodes that need writing. Livelock avoidance is > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > * mean the overall work is done. So we keep looping as long > * as made some progress on cleaning pages or inodes. > */ > - if (progress) > + if (progress) { > + spin_unlock(&wb->list_lock); > continue; > + } > + > /* > * No more inodes for IO, bail > */ > - if (list_empty(&wb->b_more_io)) > + if (list_empty(&wb->b_more_io)) { > + spin_unlock(&wb->list_lock); > break; > + } > + > /* > * Nothing written. Wait for some inode to > * become available for writeback. Otherwise > @@ -2093,9 +2101,7 @@ static long wb_writeback(struct bdi_writeback *wb, > spin_unlock(&wb->list_lock); > /* This function drops i_lock... */ > inode_sleep_on_writeback(inode); > - spin_lock(&wb->list_lock); > } > - spin_unlock(&wb->list_lock); > blk_finish_plug(&plug); > > return nr_pages - work->nr_pages; > -- > 2.40.0.348.gf938b09366-goog >
On Thu, Apr 20, 2023 at 11:53 AM Shakeel Butt <shakeelb@google.com> wrote: > > +Jens & Jan > > The patch looks good but it would be nice to pass this patch through > the eyes of experts of this area. Thanks for taking a look and CC'ing folks. I will make sure to include them in the next rounds as well. FWIW, Jens & Jan did not show up when I ran scripts/get_maintainers.ph if I remember correctly. > > On Mon, Apr 3, 2023 at 3:03 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > > flush, which can be expensive on large systems. Currently, > > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > > have to make the rstat flush atomically. On systems with a lot of > > cpus/cgroups, this can cause us to disable irqs for a long time, > > potentially causing problems. > > > > Move the call to wb_over_bg_thresh() outside the lock section in > > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > > The list_empty(&wb->work_list) should be okay outside the lock section > > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > > lists the wb->list_lock is protecting. Also, the loop seems to be > > already releasing and reacquring the lock, so this refactoring looks > > safe. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > fs/fs-writeback.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > > index 195dc23e0d831..012357bc8daa3 100644 > > --- a/fs/fs-writeback.c > > +++ b/fs/fs-writeback.c > > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > > struct blk_plug plug; > > > > blk_start_plug(&plug); > > - spin_lock(&wb->list_lock); > > for (;;) { > > /* > > * Stop writeback when nr_pages has been consumed > > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > > if (work->for_background && !wb_over_bg_thresh(wb)) > > break; > > > > + > > + spin_lock(&wb->list_lock); > > + > > /* > > * Kupdate and background works are special and we want to > > * include all inodes that need writing. Livelock avoidance is > > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > > * mean the overall work is done. So we keep looping as long > > * as made some progress on cleaning pages or inodes. > > */ > > - if (progress) > > + if (progress) { > > + spin_unlock(&wb->list_lock); > > continue; > > + } > > + > > /* > > * No more inodes for IO, bail > > */ > > - if (list_empty(&wb->b_more_io)) > > + if (list_empty(&wb->b_more_io)) { > > + spin_unlock(&wb->list_lock); > > break; > > + } > > + > > /* > > * Nothing written. Wait for some inode to > > * become available for writeback. Otherwise > > @@ -2093,9 +2101,7 @@ static long wb_writeback(struct bdi_writeback *wb, > > spin_unlock(&wb->list_lock); > > /* This function drops i_lock... */ > > inode_sleep_on_writeback(inode); > > - spin_lock(&wb->list_lock); > > } > > - spin_unlock(&wb->list_lock); > > blk_finish_plug(&plug); > > > > return nr_pages - work->nr_pages; > > -- > > 2.40.0.348.gf938b09366-goog > >
On Wed, Apr 19, 2023 at 4:38 AM Michal Koutný <mkoutny@suse.com> wrote: > > On Mon, Apr 03, 2023 at 10:03:33PM +0000, Yosry Ahmed <yosryahmed@google.com> wrote: > > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > > flush, which can be expensive on large systems. Currently, > > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > > have to make the rstat flush atomically. On systems with a lot of > > cpus/cgroups, this can cause us to disable irqs for a long time, > > potentially causing problems. > > > > Move the call to wb_over_bg_thresh() outside the lock section in > > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > > The list_empty(&wb->work_list) should be okay outside the lock section > > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > > lists the wb->list_lock is protecting. Also, the loop seems to be > > already releasing and reacquring the lock, so this refactoring looks > > safe. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > fs/fs-writeback.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > > index 195dc23e0d831..012357bc8daa3 100644 > > --- a/fs/fs-writeback.c > > +++ b/fs/fs-writeback.c > > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > > struct blk_plug plug; > > > > blk_start_plug(&plug); > > - spin_lock(&wb->list_lock); > > for (;;) { > > /* > > * Stop writeback when nr_pages has been consumed > > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > > if (work->for_background && !wb_over_bg_thresh(wb)) > > break; > > > > + > > + spin_lock(&wb->list_lock); > > + > > /* > > * Kupdate and background works are special and we want to > > * include all inodes that need writing. Livelock avoidance is > > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > > * mean the overall work is done. So we keep looping as long > > * as made some progress on cleaning pages or inodes. > > */ > > - if (progress) > > + if (progress) { > > + spin_unlock(&wb->list_lock); > > continue; > > + } > > + > > This would release wb->list_lock temporarily with progress but that's > already not held continuously due to writeback_sb_inodes(). > Holding the lock could even be shortened by taking it later after > trace_writeback_start(). > > Altogether, the change looks OK, > Reviewed-by: Michal Koutný <mkoutny@suse.com> Thanks for taking a look! >
On Mon 03-04-23 22:03:33, Yosry Ahmed wrote: > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > flush, which can be expensive on large systems. Currently, > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > have to make the rstat flush atomically. On systems with a lot of > cpus/cgroups, this can cause us to disable irqs for a long time, > potentially causing problems. > > Move the call to wb_over_bg_thresh() outside the lock section in > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > The list_empty(&wb->work_list) should be okay outside the lock section > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > lists the wb->list_lock is protecting. Also, the loop seems to be > already releasing and reacquring the lock, so this refactoring looks > safe. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> The patch looks good to me. Nice find. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/fs-writeback.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 195dc23e0d831..012357bc8daa3 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > struct blk_plug plug; > > blk_start_plug(&plug); > - spin_lock(&wb->list_lock); > for (;;) { > /* > * Stop writeback when nr_pages has been consumed > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > if (work->for_background && !wb_over_bg_thresh(wb)) > break; > > + > + spin_lock(&wb->list_lock); > + > /* > * Kupdate and background works are special and we want to > * include all inodes that need writing. Livelock avoidance is > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > * mean the overall work is done. So we keep looping as long > * as made some progress on cleaning pages or inodes. > */ > - if (progress) > + if (progress) { > + spin_unlock(&wb->list_lock); > continue; > + } > + > /* > * No more inodes for IO, bail > */ > - if (list_empty(&wb->b_more_io)) > + if (list_empty(&wb->b_more_io)) { > + spin_unlock(&wb->list_lock); > break; > + } > + > /* > * Nothing written. Wait for some inode to > * become available for writeback. Otherwise > @@ -2093,9 +2101,7 @@ static long wb_writeback(struct bdi_writeback *wb, > spin_unlock(&wb->list_lock); > /* This function drops i_lock... */ > inode_sleep_on_writeback(inode); > - spin_lock(&wb->list_lock); > } > - spin_unlock(&wb->list_lock); > blk_finish_plug(&plug); > > return nr_pages - work->nr_pages; > -- > 2.40.0.348.gf938b09366-goog >
On Fri, Apr 21, 2023 at 1:53 AM Jan Kara <jack@suse.cz> wrote: > > On Mon 03-04-23 22:03:33, Yosry Ahmed wrote: > > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > > flush, which can be expensive on large systems. Currently, > > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > > have to make the rstat flush atomically. On systems with a lot of > > cpus/cgroups, this can cause us to disable irqs for a long time, > > potentially causing problems. > > > > Move the call to wb_over_bg_thresh() outside the lock section in > > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > > The list_empty(&wb->work_list) should be okay outside the lock section > > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > > lists the wb->list_lock is protecting. Also, the loop seems to be > > already releasing and reacquring the lock, so this refactoring looks > > safe. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > The patch looks good to me. Nice find. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> Thanks for taking a look! > > Honza > > > --- > > fs/fs-writeback.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > > index 195dc23e0d831..012357bc8daa3 100644 > > --- a/fs/fs-writeback.c > > +++ b/fs/fs-writeback.c > > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > > struct blk_plug plug; > > > > blk_start_plug(&plug); > > - spin_lock(&wb->list_lock); > > for (;;) { > > /* > > * Stop writeback when nr_pages has been consumed > > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > > if (work->for_background && !wb_over_bg_thresh(wb)) > > break; > > > > + > > + spin_lock(&wb->list_lock); > > + > > /* > > * Kupdate and background works are special and we want to > > * include all inodes that need writing. Livelock avoidance is > > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > > * mean the overall work is done. So we keep looping as long > > * as made some progress on cleaning pages or inodes. > > */ > > - if (progress) > > + if (progress) { > > + spin_unlock(&wb->list_lock); > > continue; > > + } > > + > > /* > > * No more inodes for IO, bail > > */ > > - if (list_empty(&wb->b_more_io)) > > + if (list_empty(&wb->b_more_io)) { > > + spin_unlock(&wb->list_lock); > > break; > > + } > > + > > /* > > * Nothing written. Wait for some inode to > > * become available for writeback. Otherwise > > @@ -2093,9 +2101,7 @@ static long wb_writeback(struct bdi_writeback *wb, > > spin_unlock(&wb->list_lock); > > /* This function drops i_lock... */ > > inode_sleep_on_writeback(inode); > > - spin_lock(&wb->list_lock); > > } > > - spin_unlock(&wb->list_lock); > > blk_finish_plug(&plug); > > > > return nr_pages - work->nr_pages; > > -- > > 2.40.0.348.gf938b09366-goog > > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 195dc23e0d831..012357bc8daa3 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, struct blk_plug plug; blk_start_plug(&plug); - spin_lock(&wb->list_lock); for (;;) { /* * Stop writeback when nr_pages has been consumed @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, if (work->for_background && !wb_over_bg_thresh(wb)) break; + + spin_lock(&wb->list_lock); + /* * Kupdate and background works are special and we want to * include all inodes that need writing. Livelock avoidance is @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, * mean the overall work is done. So we keep looping as long * as made some progress on cleaning pages or inodes. */ - if (progress) + if (progress) { + spin_unlock(&wb->list_lock); continue; + } + /* * No more inodes for IO, bail */ - if (list_empty(&wb->b_more_io)) + if (list_empty(&wb->b_more_io)) { + spin_unlock(&wb->list_lock); break; + } + /* * Nothing written. Wait for some inode to * become available for writeback. Otherwise @@ -2093,9 +2101,7 @@ static long wb_writeback(struct bdi_writeback *wb, spin_unlock(&wb->list_lock); /* This function drops i_lock... */ inode_sleep_on_writeback(inode); - spin_lock(&wb->list_lock); } - spin_unlock(&wb->list_lock); blk_finish_plug(&plug); return nr_pages - work->nr_pages;