linux-kernel - Re: [PATCH v6 4/5] writeback, cgroup: support switching multiple inodes at once

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210603101057.GH23647@quack2.suse.cz>
Date:   Thu, 3 Jun 2021 12:10:57 +0200
From:   Jan Kara <jack@...e.cz>
To:     Roman Gushchin <guro@...com>
Cc:     Jan Kara <jack@...e.cz>, Tejun Heo <tj@...nel.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Alexander Viro <viro@...iv.linux.org.uk>,
        Dennis Zhou <dennis@...nel.org>,
        Dave Chinner <dchinner@...hat.com>, cgroups@...r.kernel.org
Subject: Re: [PATCH v6 4/5] writeback, cgroup: support switching multiple
 inodes at once

On Wed 02-06-21 17:55:16, Roman Gushchin wrote:
> Currently only a single inode can be switched to another writeback
> structure at once. That means to switch an inode a separate
> inode_switch_wbs_context structure must be allocated, and a separate
> rcu callback and work must be scheduled.
> 
> It's fine for the existing ad-hoc switching, which is not happening
> that often, but sub-optimal for massive switching required in order to
> release a writeback structure. To prepare for it, let's add a support
> for switching multiple inodes at once.
> 
> Instead of containing a single inode pointer, inode_switch_wbs_context
> will contain a NULL-terminated array of inode
> pointers. inode_do_switch_wbs() will be called for each inode.
> 
> Signed-off-by: Roman Gushchin <guro@...com>

Two small comments below:

> @@ -473,10 +473,14 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
>  {
>  	struct inode_switch_wbs_context *isw =
>  		container_of(to_rcu_work(work), struct inode_switch_wbs_context, work);
> +	struct inode **inodep;
> +
> +	for (inodep = &isw->inodes[0]; *inodep; inodep++) {
                      ^^^^ why not just isw->inodes?

> +		inode_do_switch_wbs(*inodep, isw->new_wb);
> +		iput(*inodep);
> +	}

I was kind of hoping that we would save the repeated locking of
bdi->wb_switch_rwsem, old_wb->list_lock, and new_wb->list_lock for multiple
inodes. Maybe we can have 'old_wb' as part of isw as well and assert that
all inodes are still attached to the old_wb at this point to make this a
bit simpler. Or we can fetch old_wb from the first inode and then just
lock & assert using that one.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR