[PATCH] cdrom: Convert per-driver mutexes to per-device mutexes

From: Simon Arlott
Date: Fri Mar 06 2020 - 17:52:11 EST

All of the users of cdrom_open/cdrom_ioctl/cdrom_release hold a per-driver
mutex while calling these functions. This hurts multi-drive performance
because all ioctls get processed consecutively.

Except for ide-cd ioctls, none of the other drivers need a mutex on
accesses so they can be removed.

Replace all of the per-driver mutexes with a per-device mutex in the
cdrom functions, allowing concurrent ioctls on different cdrom devices.

Add a per-device mutex to idecd_ioctl (which calls generic_ide_ioctl
before cdrom_ioctl).

Signed-off-by: Simon Arlott <simon@xxxxxxxxxxx>
---
This has been discussed previously for sr_mutex:
https://linux-scsi.vger.kernel.narkive.com/JLvupYok/patch-scsi-sr-fix-multi-drive-performance-by-using-per-device-mutexes
https://linux-scsi.vger.kernel.narkive.com/Ai2VaLJ3/sr-sr-mutex-multi-drive-performance-is-bad

There is currently a conflicting patch in next-20200306 that makes the
sr_mutex per-device:
https://lore.kernel.org/linux-scsi/20200218143918.30267-1-merlijn@xxxxxxxxxxx/

I've tested this (sr only) with 4 PATA drives and 1 SATA drive on 2
separate IDE controllers (NVIDIA MCP55 and Promise 133 TX2). I've also
got another 3 SATA drives and 2 USB drives that I can test with.
---
drivers/block/paride/pcd.c | 19 +-------
drivers/cdrom/cdrom.c | 96 ++++++++++++++++++++++++++------------
drivers/cdrom/gdrom.c | 19 +-------
drivers/ide/ide-cd.c | 19 ++++----
drivers/ide/ide-cd.h | 2 +
drivers/scsi/sr.c | 10 ----
include/linux/cdrom.h | 2 +
7 files changed, 85 insertions(+), 82 deletions(-)

diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
index 117cfc8cd05a..42ef454e089d 100644
--- a/drivers/block/paride/pcd.c
+++ b/drivers/block/paride/pcd.c
@@ -138,10 +138,8 @@ enum {D_PRT, D_PRO, D_UNI, D_MOD, D_SLV, D_DLY};
#include <linux/cdrom.h>
#include <linux/spinlock.h>
#include <linux/blk-mq.h>
-#include <linux/mutex.h>
#include <linux/uaccess.h>

-static DEFINE_MUTEX(pcd_mutex);
static DEFINE_SPINLOCK(pcd_lock);

module_param(verbose, int, 0644);
@@ -231,36 +229,23 @@ static void *par_drv; /* reference of parport driver */
static int pcd_block_open(struct block_device *bdev, fmode_t mode)
{
struct pcd_unit *cd = bdev->bd_disk->private_data;
- int ret;

check_disk_change(bdev);

- mutex_lock(&pcd_mutex);
- ret = cdrom_open(&cd->info, bdev, mode);
- mutex_unlock(&pcd_mutex);
-
- return ret;
+ return cdrom_open(&cd->info, bdev, mode);
}

static void pcd_block_release(struct gendisk *disk, fmode_t mode)
{
struct pcd_unit *cd = disk->private_data;
- mutex_lock(&pcd_mutex);
cdrom_release(&cd->info, mode);
- mutex_unlock(&pcd_mutex);
}

static int pcd_block_ioctl(struct block_device *bdev, fmode_t mode,
unsigned cmd, unsigned long arg)
{
struct pcd_unit *cd = bdev->bd_disk->private_data;
- int ret;
-
- mutex_lock(&pcd_mutex);
- ret = cdrom_ioctl(&cd->info, bdev, mode, cmd, arg);
- mutex_unlock(&pcd_mutex);
-
- return ret;
+ return cdrom_ioctl(&cd->info, bdev, mode, cmd, arg);
}

static unsigned int pcd_block_check_events(struct gendisk *disk,
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index faca0f346fff..8c48541ba6ac 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -601,6 +601,8 @@ int register_cdrom(struct cdrom_device_info *cdi)
cdrom_sysctl_register();
}

+ mutex_init(&cdi->lock);
+
ENSURE(cdo, drive_status, CDC_DRIVE_STATUS);
if (cdo->check_events == NULL && cdo->media_changed == NULL)
WARN_ON_ONCE(cdo->capability & (CDC_MEDIA_CHANGED | CDC_SELECT_DISC));
@@ -653,6 +655,7 @@ void unregister_cdrom(struct cdrom_device_info *cdi)
cdi->exit(cdi);

cd_dbg(CD_REG_UNREG, "drive \"/dev/%s\" unregistered\n", cdi->name);
+ mutex_destroy(&cdi->lock);
}

int cdrom_get_media_event(struct cdrom_device_info *cdi,
@@ -1159,6 +1162,7 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,

cd_dbg(CD_OPEN, "entering cdrom_open\n");

+ mutex_lock(&cdi->lock);
/* if this was a O_NONBLOCK open and we should honor the flags,
* do a quick open without drive/disc integrity checks. */
cdi->use_count++;
@@ -1186,7 +1190,7 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,

cd_dbg(CD_OPEN, "Use count for \"/dev/%s\" now %d\n",
cdi->name, cdi->use_count);
- return 0;
+ goto out;
err_release:
if (CDROM_CAN(CDC_LOCK) && cdi->options & CDO_LOCK) {
cdi->ops->lock_door(cdi, 0);
@@ -1195,6 +1199,8 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,
cdi->ops->release(cdi);
err:
cdi->use_count--;
+out:
+ mutex_unlock(&cdi->lock);
return ret;
}

@@ -1262,6 +1268,7 @@ void cdrom_release(struct cdrom_device_info *cdi, fmode_t mode)

cd_dbg(CD_CLOSE, "entering cdrom_release\n");

+ mutex_lock(&cdi->lock);
if (cdi->use_count > 0)
cdi->use_count--;

@@ -1291,6 +1298,7 @@ void cdrom_release(struct cdrom_device_info *cdi, fmode_t mode)
cdi->options & CDO_AUTO_EJECT && CDROM_CAN(CDC_OPEN_TRAY))
cdo->tray_move(cdi, 1);
}
+ mutex_unlock(&cdi->lock);
}

static int cdrom_read_mech_status(struct cdrom_device_info *cdi,
@@ -3355,48 +3363,66 @@ int cdrom_ioctl(struct cdrom_device_info *cdi, struct block_device *bdev,
void __user *argp = (void __user *)arg;
int ret;

+ mutex_lock(&cdi->lock);
/*
* Try the generic SCSI command ioctl's first.
*/
ret = scsi_cmd_blk_ioctl(bdev, mode, cmd, argp);
if (ret != -ENOTTY)
- return ret;
+ goto out;

switch (cmd) {
case CDROMMULTISESSION:
- return cdrom_ioctl_multisession(cdi, argp);
+ ret = cdrom_ioctl_multisession(cdi, argp);
+ goto out;
case CDROMEJECT:
- return cdrom_ioctl_eject(cdi);
+ ret = cdrom_ioctl_eject(cdi);
+ goto out;
case CDROMCLOSETRAY:
- return cdrom_ioctl_closetray(cdi);
+ ret = cdrom_ioctl_closetray(cdi);
+ goto out;
case CDROMEJECT_SW:
- return cdrom_ioctl_eject_sw(cdi, arg);
+ ret = cdrom_ioctl_eject_sw(cdi, arg);
+ goto out;
case CDROM_MEDIA_CHANGED:
- return cdrom_ioctl_media_changed(cdi, arg);
+ ret = cdrom_ioctl_media_changed(cdi, arg);
+ goto out;
case CDROM_SET_OPTIONS:
- return cdrom_ioctl_set_options(cdi, arg);
+ ret = cdrom_ioctl_set_options(cdi, arg);
+ goto out;
case CDROM_CLEAR_OPTIONS:
- return cdrom_ioctl_clear_options(cdi, arg);
+ ret = cdrom_ioctl_clear_options(cdi, arg);
+ goto out;
case CDROM_SELECT_SPEED:
- return cdrom_ioctl_select_speed(cdi, arg);
+ ret = cdrom_ioctl_select_speed(cdi, arg);
+ goto out;
case CDROM_SELECT_DISC:
- return cdrom_ioctl_select_disc(cdi, arg);
+ ret = cdrom_ioctl_select_disc(cdi, arg);
+ goto out;
case CDROMRESET:
- return cdrom_ioctl_reset(cdi, bdev);
+ ret = cdrom_ioctl_reset(cdi, bdev);
+ goto out;
case CDROM_LOCKDOOR:
- return cdrom_ioctl_lock_door(cdi, arg);
+ ret = cdrom_ioctl_lock_door(cdi, arg);
+ goto out;
case CDROM_DEBUG:
- return cdrom_ioctl_debug(cdi, arg);
+ ret = cdrom_ioctl_debug(cdi, arg);
+ goto out;
case CDROM_GET_CAPABILITY:
- return cdrom_ioctl_get_capability(cdi);
+ ret = cdrom_ioctl_get_capability(cdi);
+ goto out;
case CDROM_GET_MCN:
- return cdrom_ioctl_get_mcn(cdi, argp);
+ ret = cdrom_ioctl_get_mcn(cdi, argp);
+ goto out;
case CDROM_DRIVE_STATUS:
- return cdrom_ioctl_drive_status(cdi, arg);
+ ret = cdrom_ioctl_drive_status(cdi, arg);
+ goto out;
case CDROM_DISC_STATUS:
- return cdrom_ioctl_disc_status(cdi);
+ ret = cdrom_ioctl_disc_status(cdi);
+ goto out;
case CDROM_CHANGER_NSLOTS:
- return cdrom_ioctl_changer_nslots(cdi);
+ ret = cdrom_ioctl_changer_nslots(cdi);
+ goto out;
}

/*
@@ -3408,7 +3434,7 @@ int cdrom_ioctl(struct cdrom_device_info *cdi, struct block_device *bdev,
if (CDROM_CAN(CDC_GENERIC_PACKET)) {
ret = mmc_ioctl(cdi, cmd, arg);
if (ret != -ENOTTY)
- return ret;
+ goto out;
}

/*
@@ -3418,27 +3444,39 @@ int cdrom_ioctl(struct cdrom_device_info *cdi, struct block_device *bdev,
*/
switch (cmd) {
case CDROMSUBCHNL:
- return cdrom_ioctl_get_subchnl(cdi, argp);
+ ret = cdrom_ioctl_get_subchnl(cdi, argp);
+ goto out;
case CDROMREADTOCHDR:
- return cdrom_ioctl_read_tochdr(cdi, argp);
+ ret = cdrom_ioctl_read_tochdr(cdi, argp);
+ goto out;
case CDROMREADTOCENTRY:
- return cdrom_ioctl_read_tocentry(cdi, argp);
+ ret = cdrom_ioctl_read_tocentry(cdi, argp);
+ goto out;
case CDROMPLAYMSF:
- return cdrom_ioctl_play_msf(cdi, argp);
+ ret = cdrom_ioctl_play_msf(cdi, argp);
+ goto out;
case CDROMPLAYTRKIND:
- return cdrom_ioctl_play_trkind(cdi, argp);
+ ret = cdrom_ioctl_play_trkind(cdi, argp);
+ goto out;
case CDROMVOLCTRL:
- return cdrom_ioctl_volctrl(cdi, argp);
+ ret = cdrom_ioctl_volctrl(cdi, argp);
+ goto out;
case CDROMVOLREAD:
- return cdrom_ioctl_volread(cdi, argp);
+ ret = cdrom_ioctl_volread(cdi, argp);
+ goto out;
case CDROMSTART:
case CDROMSTOP:
case CDROMPAUSE:
case CDROMRESUME:
- return cdrom_ioctl_audioctl(cdi, cmd);
+ ret = cdrom_ioctl_audioctl(cdi, cmd);
+ goto out;
}

- return -ENOSYS;
+ ret = -ENOSYS;
+
+out:
+ mutex_unlock(&cdi->lock);
+ return ret;
}

EXPORT_SYMBOL(cdrom_get_last_written);
diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index 886b2638c730..e333fe755deb 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -20,7 +20,6 @@
#include <linux/blk-mq.h>
#include <linux/interrupt.h>
#include <linux/device.h>
-#include <linux/mutex.h>
#include <linux/wait.h>
#include <linux/platform_device.h>
#include <scsi/scsi.h>
@@ -66,7 +65,6 @@

#define GDROM_DEFAULT_TIMEOUT (HZ * 7)

-static DEFINE_MUTEX(gdrom_mutex);
static const struct {
int sense_key;
const char * const text;
@@ -477,21 +475,14 @@ static const struct cdrom_device_ops gdrom_ops = {

static int gdrom_bdops_open(struct block_device *bdev, fmode_t mode)
{
- int ret;
-
check_disk_change(bdev);

- mutex_lock(&gdrom_mutex);
- ret = cdrom_open(gd.cd_info, bdev, mode);
- mutex_unlock(&gdrom_mutex);
- return ret;
+ return cdrom_open(gd.cd_info, bdev, mode);
}

static void gdrom_bdops_release(struct gendisk *disk, fmode_t mode)
{
- mutex_lock(&gdrom_mutex);
cdrom_release(gd.cd_info, mode);
- mutex_unlock(&gdrom_mutex);
}

static unsigned int gdrom_bdops_check_events(struct gendisk *disk,
@@ -503,13 +494,7 @@ static unsigned int gdrom_bdops_check_events(struct gendisk *disk,
static int gdrom_bdops_ioctl(struct block_device *bdev, fmode_t mode,
unsigned cmd, unsigned long arg)
{
- int ret;
-
- mutex_lock(&gdrom_mutex);
- ret = cdrom_ioctl(gd.cd_info, bdev, mode, cmd, arg);
- mutex_unlock(&gdrom_mutex);
-
- return ret;
+ return cdrom_ioctl(gd.cd_info, bdev, mode, cmd, arg);
}

static const struct block_device_operations gdrom_bdops = {
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index dcf8b51b47fd..20c46d67eb38 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -52,7 +52,6 @@

#include "ide-cd.h"

-static DEFINE_MUTEX(ide_cd_mutex);
static DEFINE_MUTEX(idecd_ref_mutex);

static void ide_cd_release(struct device *);
@@ -1586,6 +1585,7 @@ static void ide_cd_release(struct device *dev)
drive->prep_rq = NULL;
g->private_data = NULL;
put_disk(g);
+ mutex_destroy(&info->ioctl_lock);
kfree(info);
}

@@ -1614,7 +1614,6 @@ static int idecd_open(struct block_device *bdev, fmode_t mode)

check_disk_change(bdev);

- mutex_lock(&ide_cd_mutex);
info = ide_cd_get(bdev->bd_disk);
if (!info)
goto out;
@@ -1623,7 +1622,6 @@ static int idecd_open(struct block_device *bdev, fmode_t mode)
if (rc < 0)
ide_cd_put(info);
out:
- mutex_unlock(&ide_cd_mutex);
return rc;
}

@@ -1631,11 +1629,9 @@ static void idecd_release(struct gendisk *disk, fmode_t mode)
{
struct cdrom_info *info = ide_drv_g(disk, cdrom_info);

- mutex_lock(&ide_cd_mutex);
cdrom_release(&info->devinfo, mode);

ide_cd_put(info);
- mutex_unlock(&ide_cd_mutex);
}

static int idecd_set_spindown(struct cdrom_device_info *cdi, unsigned long arg)
@@ -1702,11 +1698,12 @@ static int idecd_locked_ioctl(struct block_device *bdev, fmode_t mode,
static int idecd_ioctl(struct block_device *bdev, fmode_t mode,
unsigned int cmd, unsigned long arg)
{
+ struct cdrom_info *info = ide_drv_g(bdev->bd_disk, cdrom_info);
int ret;

- mutex_lock(&ide_cd_mutex);
+ mutex_lock(&info->ioctl_lock);
ret = idecd_locked_ioctl(bdev, mode, cmd, arg);
- mutex_unlock(&ide_cd_mutex);
+ mutex_unlock(&info->ioctl_lock);

return ret;
}
@@ -1738,11 +1735,12 @@ static int idecd_locked_compat_ioctl(struct block_device *bdev, fmode_t mode,
static int idecd_compat_ioctl(struct block_device *bdev, fmode_t mode,
unsigned int cmd, unsigned long arg)
{
+ struct cdrom_info *info = ide_drv_g(bdev->bd_disk, cdrom_info);
int ret;

- mutex_lock(&ide_cd_mutex);
+ mutex_lock(&info->ioctl_lock);
ret = idecd_locked_compat_ioctl(bdev, mode, cmd, arg);
- mutex_unlock(&ide_cd_mutex);
+ mutex_unlock(&info->ioctl_lock);

return ret;
}
@@ -1804,6 +1802,8 @@ static int ide_cd_probe(ide_drive_t *drive)
goto failed;
}

+ mutex_init(&info->ioctl_lock);
+
g = alloc_disk(1 << PARTN_BITS);
if (!g)
goto out_free_cd;
@@ -1842,6 +1842,7 @@ static int ide_cd_probe(ide_drive_t *drive)
out_free_disk:
put_disk(g);
out_free_cd:
+ mutex_destroy(&info->ioctl_lock);
kfree(info);
failed:
return -ENODEV;
diff --git a/drivers/ide/ide-cd.h b/drivers/ide/ide-cd.h
index a69dc7f61c4d..93af8a8bc91f 100644
--- a/drivers/ide/ide-cd.h
+++ b/drivers/ide/ide-cd.h
@@ -7,6 +7,7 @@
#define _IDE_CD_H

#include <linux/cdrom.h>
+#include <linux/mutex.h>
#include <asm/byteorder.h>

#define IDECD_DEBUG_LOG 0
@@ -78,6 +79,7 @@ struct cdrom_info {
struct ide_driver *driver;
struct gendisk *disk;
struct device dev;
+ struct mutex ioctl_lock;

/* Buffer for table of contents. NULL if we haven't allocated
a TOC buffer for this device yet. */
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 0fbb8fe6e521..88b43d8d6863 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -46,7 +46,6 @@
#include <linux/init.h>
#include <linux/blkdev.h>
#include <linux/blk-pm.h>
-#include <linux/mutex.h>
#include <linux/slab.h>
#include <linux/pm_runtime.h>
#include <linux/uaccess.h>
@@ -79,7 +78,6 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_WORM);
CDC_CD_R|CDC_CD_RW|CDC_DVD|CDC_DVD_R|CDC_DVD_RAM|CDC_GENERIC_PACKET| \
CDC_MRW|CDC_MRW_W|CDC_RAM)

-static DEFINE_MUTEX(sr_mutex);
static int sr_probe(struct device *);
static int sr_remove(struct device *);
static blk_status_t sr_init_command(struct scsi_cmnd *SCpnt);
@@ -536,9 +534,7 @@ static int sr_block_open(struct block_device *bdev, fmode_t mode)
scsi_autopm_get_device(sdev);
check_disk_change(bdev);

- mutex_lock(&sr_mutex);
ret = cdrom_open(&cd->cdi, bdev, mode);
- mutex_unlock(&sr_mutex);

scsi_autopm_put_device(sdev);
if (ret)
@@ -551,10 +547,8 @@ static int sr_block_open(struct block_device *bdev, fmode_t mode)
static void sr_block_release(struct gendisk *disk, fmode_t mode)
{
struct scsi_cd *cd = scsi_cd(disk);
- mutex_lock(&sr_mutex);
cdrom_release(&cd->cdi, mode);
scsi_cd_put(cd);
- mutex_unlock(&sr_mutex);
}

static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
@@ -565,7 +559,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
void __user *argp = (void __user *)arg;
int ret;

- mutex_lock(&sr_mutex);

ret = scsi_ioctl_block_when_processing_errors(sdev, cmd,
(mode & FMODE_NDELAY) != 0);
@@ -595,7 +588,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
scsi_autopm_put_device(sdev);

out:
- mutex_unlock(&sr_mutex);
return ret;
}

@@ -608,7 +600,6 @@ static int sr_block_compat_ioctl(struct block_device *bdev, fmode_t mode, unsign
void __user *argp = compat_ptr(arg);
int ret;

- mutex_lock(&sr_mutex);

ret = scsi_ioctl_block_when_processing_errors(sdev, cmd,
(mode & FMODE_NDELAY) != 0);
@@ -638,7 +629,6 @@ static int sr_block_compat_ioctl(struct block_device *bdev, fmode_t mode, unsign
scsi_autopm_put_device(sdev);

out:
- mutex_unlock(&sr_mutex);
return ret;

}
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index 528271c60018..eb6c4de7006d 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -13,6 +13,7 @@

#include <linux/fs.h> /* not really needed, later.. */
#include <linux/list.h>
+#include <linux/mutex.h>
#include <scsi/scsi_common.h>
#include <uapi/linux/cdrom.h>

@@ -41,6 +42,7 @@ struct cdrom_device_info {
const struct cdrom_device_ops *ops; /* link to device_ops */
struct list_head list; /* linked list of all device_info */
struct gendisk *disk; /* matching block layer disk */
+ struct mutex lock;
void *handle; /* driver-dependent data */
/* specifications */
int mask; /* mask of capability: disables them */
--
2.17.1

--
Simon Arlott