From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.5 required=3.0 tests=INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A616C2BB55 for ; Wed, 15 Apr 2020 13:19:21 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 9CA022074F for ; Wed, 15 Apr 2020 13:19:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9CA022074F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 095A58E001C; Wed, 15 Apr 2020 09:19:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 045A08E0001; Wed, 15 Apr 2020 09:19:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E4F278E001C; Wed, 15 Apr 2020 09:19:19 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0216.hostedemail.com [216.40.44.216]) by kanga.kvack.org (Postfix) with ESMTP id C990E8E0001 for ; Wed, 15 Apr 2020 09:19:19 -0400 (EDT) Received: from smtpin21.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 874A98245571 for ; Wed, 15 Apr 2020 13:19:19 +0000 (UTC) X-FDA: 76710145638.21.meat42_6e7ab44920632 X-HE-Tag: meat42_6e7ab44920632 X-Filterd-Recvd-Size: 6243 Received: from mail-pl1-f196.google.com (mail-pl1-f196.google.com [209.85.214.196]) by imf33.hostedemail.com (Postfix) with ESMTP for ; Wed, 15 Apr 2020 13:19:19 +0000 (UTC) Received: by mail-pl1-f196.google.com with SMTP id y12so1245423pll.2 for ; Wed, 15 Apr 2020 06:19:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=7IvG2bEA5VCaC2qgBtVR/FZ0IDKm4MJccJ5LrCzVHtM=; b=oYpXr6+K1cIygxpKS4/yd1eoM5WuPfRY8dTi817yJHorAafoRD4RabfgUKsUzLvPJc dM6QoQzhMn38mtAXi6OJbzJS+GySkN7zVhdQaEn62G70+yXlT7EkfWUUIMNkGniL8M0P et7oUnpgAsi3CDgnEWxsB25j75N/MdHbrdlyO4nIUX6ZdwfN1b9gy+iCjzHOvPdz8H2X GVGcrBKM3Rp35+zCx90bjcPiSQqssLlAmsZDBLjQGkFztIYsx6SMEszAv+MflzXX+70V +s2o6FR9R2OBd8JecZzhU9mOA/e2nenfJSQ6bDewWXoJlYMxKDWLdNWHl8GZg8SIwyhm vG8Q== X-Gm-Message-State: AGi0PuabuCT3MNGGSzXvlEslskQShaGX8CA3tQWXcHdpQ6p3MIJt7sT6 9juPF26EYPvJuqRLVFyTsBY= X-Google-Smtp-Source: APiQypKt0dyTdKtn4rGriYEeFWzmB80X5z3lTmEXBrfOHZD1nC8rYnZFAuTFIJUuQI3bysuLAnuDnA== X-Received: by 2002:a17:902:8485:: with SMTP id c5mr5057637plo.242.1586956757999; Wed, 15 Apr 2020 06:19:17 -0700 (PDT) Received: from 42.do-not-panic.com (42.do-not-panic.com. [157.230.128.187]) by smtp.gmail.com with ESMTPSA id n9sm3466685pjt.29.2020.04.15.06.19.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Apr 2020 06:19:16 -0700 (PDT) Received: by 42.do-not-panic.com (Postfix, from userid 1000) id CFE5D40277; Wed, 15 Apr 2020 13:19:15 +0000 (UTC) Date: Wed, 15 Apr 2020 13:19:15 +0000 From: Luis Chamberlain To: Christoph Hellwig Cc: Alan Jenkins , axboe@kernel.dk, viro@zeniv.linux.org.uk, bvanassche@acm.org, gregkh@linuxfoundation.org, rostedt@goodmis.org, mingo@redhat.com, jack@suse.cz, ming.lei@redhat.com, nstange@suse.de, akpm@linux-foundation.org, mhocko@suse.com, yukuai3@huawei.com, linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Omar Sandoval , Hannes Reinecke , Michal Hocko Subject: Re: [PATCH 4/5] mm/swapfile: refcount block and queue before using blkcg_schedule_throttle() Message-ID: <20200415131915.GV11244@42.do-not-panic.com> References: <20200414041902.16769-1-mcgrof@kernel.org> <20200414041902.16769-5-mcgrof@kernel.org> <20200414154447.GC25765@infradead.org> <20200415054234.GQ11244@42.do-not-panic.com> <20200415072712.GB21099@infradead.org> <20200415073443.GA21036@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200415073443.GA21036@infradead.org> User-Agent: Mutt/1.10.1 (2018-07-13) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Apr 15, 2020 at 12:34:43AM -0700, Christoph Hellwig wrote: > On Wed, Apr 15, 2020 at 12:27:12AM -0700, Christoph Hellwig wrote: > > On Wed, Apr 15, 2020 at 05:42:34AM +0000, Luis Chamberlain wrote: > > > > I don't understand the atomic part of the comment. How does > > > > bdgrab/bdput help us there? > > > > > > The commit log above did a better job at explaining this in terms of our > > > goal to use the request_queue and how this use would prevent the risk of > > > releasing the request_queue, which could sleep. > > > > So bdput eventually does and iput, but what leads to an out of context > > offload? > > > > But anyway, isn't the original problem better solved by simply not > > releasing the queue from atomic context to start with? There isn't > > really any good reason we keep holding the spinlock once we have a > > reference on the queue, so something like this (not even compile tested) > > should do the work: > > Actually - mem_cgroup_throttle_swaprate already checks for a non-NULL > current->throttle_queue above, so we should never even call > blk_put_queue here. Was this found by code inspection, or was there > a real report? No but report, this code path came up as a possible use of blk_put_queue() which already exists in atomic context. So yes, it already uses blk_put_queue(), but how are we *sure* its not going to be the last one? Because if it is that mean the release will happen in atomic context, defeating the goal of the last patch in this series. Using bdgrab() however ensures that during the lifecycle of this path, blk_put_queue() won't be the last if used after, but instead we are sure it will be upon disk release. In fact, with bdgrab() isn't the blk_get_queue() on blkcg_schedule_throttle() no longer needed? > In the latter case we need to figure out what protects >throttle_queue, > as the way blkcg_schedule_throttle first put the reference and only then > assign a value to it already looks like it introduces a tiny race > window. > > Otherwise just open coding the applicable part of blkcg_schedule_throttle > in mem_cgroup_throttle_swaprate seems easiest: > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 5871a2aa86a5..e16051ef074c 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -3761,15 +3761,20 @@ void mem_cgroup_throttle_swaprate(struct mem_cgroup *memcg, int node, > */ > if (current->throttle_queue) > return; > + if (unlikely(current->flags & PF_KTHREAD)) > + return; > > spin_lock(&swap_avail_lock); > plist_for_each_entry_safe(si, next, &swap_avail_heads[node], > avail_lists[node]) { > - if (si->bdev) { > - blkcg_schedule_throttle(bdev_get_queue(si->bdev), > - true); > - break; > + if (!si->bdev) > + continue; > + if (blk_get_queue(dev_get_queue(si->bdev))) { > + current->throttle_queue = dev_get_queue(si->bdev); > + current->use_memdelay = true; > + set_notify_resume(current); > } > + break; > } > spin_unlock(&swap_avail_lock); > } Sorry, its not clear to me who calls the respective blk_put_queue() here? Luis