From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753533AbbJBSTQ (ORCPT ); Fri, 2 Oct 2015 14:19:16 -0400 Received: from mail-yk0-f169.google.com ([209.85.160.169]:35231 "EHLO mail-yk0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751919AbbJBSTO (ORCPT ); Fri, 2 Oct 2015 14:19:14 -0400 Date: Fri, 2 Oct 2015 14:19:10 -0400 From: Tejun Heo To: Jan Kara Cc: axboe@kernel.dk, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, tytso@mit.edu, dedekind1@gmail.com, decui@microsoft.com, kernel-team@fb.com Subject: Re: [PATCH 3/5] writeback: bdi_writeback iteration must not skip dying ones Message-ID: <20151002181910.GA8983@mtj.duckdns.org> References: <1443545274-18787-1-git-send-email-tj@kernel.org> <1443545274-18787-4-git-send-email-tj@kernel.org> <20151002141212.GA17746@quack.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151002141212.GA17746@quack.suse.cz> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Oct 02, 2015 at 04:12:12PM +0200, Jan Kara wrote: > > + if (last_wb) { > > + wb_put(last_wb); > > + last_wb = NULL; > > + } > > But you seem to forget to drop last_wb reference in case this was the last > wb in the list, don't you? You're right. Will update the patch. > > @@ -686,6 +691,9 @@ static void cgwb_bdi_destroy(struct backing_dev_info *bdi) > > radix_tree_for_each_slot(slot, &bdi->cgwb_tree, &iter, 0) > > cgwb_kill(*slot); > > > > + /* wb may get released after @bdi is freed, sever list head */ > > + list_del(&bdi->wb_list); > > + > > But we wait for bdi->usage_cnt to drop to 0 which means there's no wb, > don't we? What am I missing? And I forgot that we were doing that. This isn't necessary. > > @@ -764,15 +772,22 @@ static void cgwb_bdi_destroy(struct backing_dev_info *bdi) { } > > > > int bdi_init(struct backing_dev_info *bdi) > > { > > + int ret; > > + > > bdi->dev = NULL; > > > > bdi->min_ratio = 0; > > bdi->max_ratio = 100; > > bdi->max_prop_frac = FPROP_FRAC_BASE; > > INIT_LIST_HEAD(&bdi->bdi_list); > > + INIT_LIST_HEAD(&bdi->wb_list); > > init_waitqueue_head(&bdi->wb_waitq); > > > > - return cgwb_bdi_init(bdi); > > + ret = cgwb_bdi_init(bdi); > > + > > + list_add_tail_rcu(&bdi->wb.bdi_node, &bdi->wb_list); > > Won't this be more logical in cgwb_bdi_init()? bdi->wb_list exists whether cgwb is enabled or not, so if we move this to cgwb_bdi_init(), we'll be duplicating it in both paths. Thanks. -- tejun