Re: [PATCH v4] block: genhd: don't call probe function with major_names_lock held

From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Christoph Hellwig <hch@lst.de>
Cc: Jens Axboe <axboe@kernel.dk>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Hannes Reinecke <hare@suse.de>,
	Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>,
	Hillf Danton <hdanton@sina.com>,
	Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>,
	linux-block <linux-block@vger.kernel.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	Tyler Hicks <tyhicks@linux.microsoft.com>,
	Pavel Tatashin <pasha.tatashin@soleen.com>
Subject: Re: [PATCH v4] block: genhd: don't call probe function with major_names_lock held
Date: Wed, 18 Aug 2021 23:34:15 +0900	[thread overview]
Message-ID: <1f4218ca-9bfa-7d80-1c69-f5902715d8d9@i-love.sakura.ne.jp> (raw)
In-Reply-To: <20210818134752.GA7453@lst.de>

On 2021/08/18 22:47, Christoph Hellwig wrote:
> Hi Tetsuo,
> 
> I might sound like a broken record, but we need to reduce the locking
> complexity, not make it worse and fall down the locking cliff.  I did
> send a suggested series this morning, in which you found a bunch of
> bugs.  I'd appreciate it if you could use your superior skills to
> actually help explain the issue and arrive at a common solution that
> actually simplifies things instead of steaming down the locking cliff
> full speed.  Thank you very much.

I posted "[PATCH] loop: break loop_ctl_mutex into loop_idr_spinlock and loop_removal_mutex"
which reduces the locking complexity while fixing bugs, but you ignored it. Instead,
you decided to remove cryptoloop module (where userspace doing "modprobe cryptoloop"
will break). That is, you decided not to provide patches which can be backported.

Please do review my patches carefully instead of posting broken random patch series
at full speed. "[PATCH v3] block: genhd: don't call probe function with major_names_lock
held" is a revertable patch which we can revert in future after all locking dependencies
are simplified enough. Applying this patch does not break userspace.

Then, if you review "[PATCH] loop: break loop_ctl_mutex into loop_idr_spinlock and
loop_removal_mutex" carefully, you can easily figure out why I used HIDDEN_LOOP_DEVICE
magic. You are not aware of why loop_ctl_mutex is held during entire add/remove/lookup
operations. Since reducing the duration of loop_ctl_mutex might break existing userspace,
this patch is not suitable for backporting. We can apply this patch based on agreement
for future kernels and userspace.

After "[PATCH v3] block: genhd: don't call probe function with major_names_lock held"
and "[PATCH] loop: break loop_ctl_mutex into loop_idr_spinlock and loop_removal_mutex"
are applied, you can propose changes which remove cryptoloop module (of course, with
cares added for not to break userspace).