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;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Tue, 9 May 2023 13:47:20 -1000
From:   Tejun Heo <tj@kernel.org>
To:     David Sterba <dsterba@suse.cz>
Cc:     jiangshanlai@gmail.com, linux-kernel@vger.kernel.org,
        kernel-team@meta.com, Wang Yugui <wangyugui@e16-tech.com>,
        Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 08/13] btrfs: Use alloc_ordered_workqueue() to create
 ordered workqueues
Message-ID: <ZFrbiAyCiZ2aIZ4_@slm.duckdns.org>
References: <20230509015032.3768622-1-tj@kernel.org>
 <20230509015032.3768622-9-tj@kernel.org>
 <20230509145332.GA32559@twin.jikos.cz>
 <ZFptXG44WVoWE0s4@slm.duckdns.org>
 <20230509233620.GN32559@twin.jikos.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230509233620.GN32559@twin.jikos.cz>
Precedence: bulk

Hello, David.

On Wed, May 10, 2023 at 01:36:20AM +0200, David Sterba wrote:
...
> Yeah I think so but I'm not entierly sure. The ordering for all queues
> that don't start with max_active > 1 should not be required, here the
> parallelization and out of order processing is expected and serialized
> or decided once the work is done.
> 
> > > In btrfs_resize_thread_pool the workqueue_set_max_active is called
> > > directly or indirectly so this can set the max_active to a user-defined
> > > mount option. Could this be a problem or trigger a warning? This would
> > > lead to max_active==1 + WQ_UNBOUND.
> > 
> > That's not a problem. The only thing we need to make sure is that the
> > workqueues which actually *must* be ordered use alloc_ordered_workqueue() as
> > they won't be implicitly treated as ordered in the future.
> > 
> > * The current patch converts two - fs_info->discard_ctl.discard_workers and
> >   scrub_workers when @is_dev_replace is set. Do they actually need to be
> >   ordered?
> > 
> > * As you pointed out, fs_info->fixup_workers and
> >   fs_info->qgroup_rescan_workers are also currently implicitly ordered. Do
> >   they actually need to be ordered?
> 
> I think all of them somehow implictly depend on the ordering. The
> replace process sequentially goes over a block group and copies blocks.
> 
> The fixup process is quite obscure and we should preserve the semantics
> as much as possible. It has something to do with pages that get out of
> sync with extent state without btrfs knowing and that there are more such
> requests hapenning at the same time is low but once it happens it can
> lead to corruptions.
> 
> Quota rescan is in its nature also a sequential process but I think it
> does not need to be ordered, it's started from higher level context like
> enabling quotas or rescan but there are also calls at remount time so
> this makes it less clear.
> 
> In summary, if the ordered queue could be used then I'd recommend to do
> it as the safe option.

I see. It seems rather error-prone to make workqueues implicitly ordered
from btrfs_alloc_workqueue(). I'll see if I can make it explicit and keep
all workqueues which are currently guaranteed to be ordered ordered.

Thanks.

-- 
tejun