Re: [PATCH v2] scsi: sr: get rid of sr global mutex

From: "Merlijn B.W. Wajer" <merlijn@archive.org>
To: Christoph Hellwig <hch@infradead.org>,
	James Bottomley <jejb@linux.ibm.com>
Cc: merlijn@wizzup.org, linux-scsi@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Arnd Bergmann <arnd@arndb.de>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] scsi: sr: get rid of sr global mutex
Date: Tue, 18 Feb 2020 20:21:35 +0100	[thread overview]
Message-ID: <33da5f81-ad37-05fd-d765-8bd997995dd2@archive.org> (raw)
In-Reply-To: <20200218173158.GA18386@infradead.org>

Hi,

On 18/02/2020 18:31, Christoph Hellwig wrote:
> On Tue, Feb 18, 2020 at 09:28:34AM -0800, James Bottomley wrote:
>> On Tue, 2020-02-18 at 09:23 -0800, Christoph Hellwig wrote:
>>> On Tue, Feb 18, 2020 at 09:20:28AM -0800, James Bottomley wrote:
>>>>>> Replace the global mutex with per-sr-device mutex.
>>>>>
>>>>> Do we actually need the lock at all?  What is protected by it?
>>>>
>>>> We do at least for cdrom_open.  It modifies the cdi structure with
>>>> no other protection and concurrent modification would at least
>>>> screw up the use counter which is not atomic.  Same reasoning for
>>>> cdrom_release.
>>>
>>> Wouldn't the right fix to add locking to cdrom_open/release instead
>>> of having an undocumented requirement for the callers?
>>
>> Yes ... but that's somewhat of a bigger patch because you now have to
>> reason about the callbacks within cdrom.  There's also the question of
>> whether you can assume ops->generic_packet() has its own concurrency
>> protections ... it's certainly true for SCSI, but is it for anything
>> else?  Although I suppose you can just not care and run the internal
>> lock over it anyway.
> 
> We have 4 instances of struct cdrom_device_ops in the kernel, one of
> which has a no-op generic_packet.  So I don't think this should be a
> huge project.

The are two reasons I decided to make minor changes to fix the
performance regression.

First, being able to send the patch to the various stable branches once
merged. For people working with many CD drives attached to one station,
this is a pretty big deal, so I tried to keep the patch simple. It fixes
the regression introduced in another commit.

Secondly, I don't have the hardware to test sophisticated or old setups,
like some of the issues linked from my patch. I have SATA CD drives with
USB->SATA bridges, no IDE, no PATA, etc. So the testing I can do is
relatively limited.

Perhaps I or someone else can work on removing the usage of the locks,
but as it stands I think this addresses the performance issue present in
the current kernel, and removing locks and the associated testing
required with that is something I am not entirely comfortable doing.

Cheers,
Merlijn