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=-2.5 required=3.0 tests=MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 02E8EC2BA19 for ; Thu, 16 Apr 2020 01:17:07 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id C0D5E2076D for ; Thu, 16 Apr 2020 01:17:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C0D5E2076D 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 643EB8E0063; Wed, 15 Apr 2020 21:17:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5F5108E0001; Wed, 15 Apr 2020 21:17:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 50A828E0063; Wed, 15 Apr 2020 21:17:06 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0053.hostedemail.com [216.40.44.53]) by kanga.kvack.org (Postfix) with ESMTP id 364558E0001 for ; Wed, 15 Apr 2020 21:17:06 -0400 (EDT) Received: from smtpin22.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id F1037612E for ; Thu, 16 Apr 2020 01:17:05 +0000 (UTC) X-FDA: 76711954410.22.pet32_7825f0512565c X-HE-Tag: pet32_7825f0512565c X-Filterd-Recvd-Size: 6091 Received: from mail-pl1-f195.google.com (mail-pl1-f195.google.com [209.85.214.195]) by imf48.hostedemail.com (Postfix) with ESMTP for ; Thu, 16 Apr 2020 01:17:05 +0000 (UTC) Received: by mail-pl1-f195.google.com with SMTP id h11so720542plr.11 for ; Wed, 15 Apr 2020 18:17:05 -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=WR6dS8sppVhsHDqaOLN6r5u6vSBUIRk19qqS7mXyQHc=; b=KDoapf/VZ+e+pvsA9/1SEtDN9fU/ZalhDgfhpySrboiKsfVALsu7GI86O15DffOU/i Kl/yigu0ZyDoNP9RI19NjF2nqb/jr0Fa19XS7vSXSAjE4YxHv9aJg2SRYdFAEmBZx7Z0 Jlae22UDd4PWYGN3nppeE7JQ1muSwgsxNspDF6LbjasyOLzz1D4dsb0NPst2p/ujJ2sj 9UCNiBdMta1JeYIfp7/teBGARTGJ7RsaI+4BMYfMlDJZfc2gZITKOF5xI7JcHzgZ9UYx xTam7ZsoLCnyPBOCBxyq1uPq9+CxtBHWBxfJT4IoajKa2283lGlDu0Fqb4pSQ412r6wo xPXg== X-Gm-Message-State: AGi0PubZBD7K3eFSVq2HzpODjNOuRDf7sKmUHz8Fx/9vcszerladcRCa ueagONPYNhVxt4wf886Q4rE= X-Google-Smtp-Source: APiQypLfsZVuN4sYX8KfJSMniabfDLJgj3Rsrj4/IDbZiZZSHXaIrSBXDqfPp8TpiuntbH07OtI37g== X-Received: by 2002:a17:90b:4c8f:: with SMTP id my15mr2146114pjb.63.1586999824688; Wed, 15 Apr 2020 18:17:04 -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 o11sm8556031pgd.58.2020.04.15.18.17.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Apr 2020 18:17:03 -0700 (PDT) Received: by 42.do-not-panic.com (Postfix, from userid 1000) id AEA6B40277; Thu, 16 Apr 2020 01:17:02 +0000 (UTC) Date: Thu, 16 Apr 2020 01:17:02 +0000 From: Luis Chamberlain To: Bart Van Assche Cc: Christoph Hellwig , axboe@kernel.dk, viro@zeniv.linux.org.uk, 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 3/5] blktrace: refcount the request_queue during ioctl Message-ID: <20200416011702.GC11244@42.do-not-panic.com> References: <20200414041902.16769-1-mcgrof@kernel.org> <20200414041902.16769-4-mcgrof@kernel.org> <20200414154044.GB25765@infradead.org> <20200415061649.GS11244@42.do-not-panic.com> <49bfcbe0-2630-5c82-f305-fcee489ac9ea@acm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <49bfcbe0-2630-5c82-f305-fcee489ac9ea@acm.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 07:45:18AM -0700, Bart Van Assche wrote: > On 2020-04-14 23:16, Luis Chamberlain wrote: > > On Tue, Apr 14, 2020 at 08:40:44AM -0700, Christoph Hellwig wrote: > >> Hmm, where exactly does the race come in so that it can only happen > >> after where you take the reference, but not before it? I'm probably > >> missing something, but that just means it needs to be explained a little > >> better :) > > > >>From the trace on patch 2/5: > > > > BLKTRACE_SETUP(loop0) #2 > > [ 13.933961] == blk_trace_ioctl(2, BLKTRACESETUP) start > > [ 13.936758] === do_blk_trace_setup(2) start > > [ 13.938944] === do_blk_trace_setup(2) creating directory > > [ 13.941029] === do_blk_trace_setup(2) using what debugfs_lookup() gave > > > > ---> From LOOP_CTL_DEL(loop0) #2 > > [ 13.971046] === blk_trace_cleanup(7) end > > [ 13.973175] == __blk_trace_remove(7) end > > [ 13.975352] == blk_trace_shutdown(7) end > > [ 13.977415] = __blk_release_queue(7) calling blk_mq_debugfs_unregister() > > [ 13.980645] ==== blk_mq_debugfs_unregister(7) begin > > [ 13.980696] ==== blk_mq_debugfs_unregister(7) debugfs_remove_recursive(q->debugfs_dir) > > [ 13.983118] ==== blk_mq_debugfs_unregister(7) end q->debugfs_dir is NULL > > [ 13.986945] = __blk_release_queue(7) blk_mq_debugfs_unregister() end > > [ 13.993155] = __blk_release_queue(7) end > > > > ---> From BLKTRACE_SETUP(loop0) #2 > > [ 13.995928] === do_blk_trace_setup(2) end with ret: 0 > > [ 13.997623] == blk_trace_ioctl(2, BLKTRACESETUP) end > > > > The BLKTRACESETUP above works on request_queue which later > > LOOP_CTL_DEL races on and sweeps the debugfs dir underneath us. > > If you use this commit alone though, this doesn't fix the race issue > > however, and that's because of both still the debugfs_lookup() use > > and that we're still using asynchronous removal at this point. > > > > refcounting will just ensure we don't take the request_queue underneath > > our noses. > > I think the above trace reveals a bug in the loop driver. The loop > driver shouldn't allow the associated request queue to disappear while > the loop device is open. The bug was *not* in the driver, the bug was in that deferal of removal was allowed to be asynchronous, therefore the removal from a userspace perspective *finishes*, but its not actually really done. Back when the removal was synchronous, the loop driver waited on cleanup, and didn't return to userspace until it was really removed. This is why I annotated that the move to asynch removal turns out to actually be a userspace API regression. > One may want to have a look at sd_open() in the > sd driver. The scsi_disk_get() call in that function not only increases > the reference count of the SCSI disk but also of the underlying SCSI device. Are you saying to use this as a template for what a driver should do or do you suspect there is a bug there? Not sure what you mean here. Luis