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.
