Very slow throughput when using cdparanoia on two SATA CDROM drives with /dev/sr but not /dev/sg

Very slow throughput when using cdparanoia on two SATA CDROM drives with /dev/sr but not /dev/sg

[Tim Small]

Hello,

I've got a big box of audio CDs to read, so I hooked up a load of SATA
CDROM drives to a machine (Intel motherboard AHCI, and SATA SiI3124
controllers - in this example one was attached to each host controller),
so that I could read them in parallel.

I'm using kernel 3.16.3, with cdparanoia 3.10.2 - both from Debian
Jessie.  https://www.xiph.org/paranoia/manual.html

When two (or more) drives are read simultaneously, the performance falls
to pieces (throughput from each drive drops by 95%) if more than one
/dev/sr* device is being read by cdparanoia.

If I tell cdparanoia to use the corresponding /dev/sg devices, then no
significant throughput drop is experienced when reading multiple drives
simultaneously.

As an example, using these two drives:

[1:0:0:0]    cd/dvd  TSSTcorp DVD+-RW TS-H653G DW10  /dev/sr0   /dev/sg4
[14:0:0:0]   cd/dvd  PLDS     DVD+-RW DH-16A6S YD11  /dev/sr9   /dev/sg16


... in the following results I used "time cdparanoia -v -d /dev/XXX 1
/tmp/1.wav" - where XXX was substituted for either sr9 or sg16


On an otherwise idle machine, I did these two sequentially:

sr9: 38 seconds

sg16: 38 seconds



Simultaneous with: cdparanoia -d /dev/sr0 11 /tmp/11.wav (and auto
restarted that command when it completed) I then ran these two sequentially:

sr9: 680 seconds

sg16: 38 seconds



Simultaneous with: cdparanoia -d /dev/sg4 11 /tmp/11.wav as above:

sr9: 40 seconds

sg16: 40 seconds


This is a diff of the two sets of cdparanoia -v output (using the sr
devices vs the sg devices):

--- /tmp/sr     2014-11-06 12:41:43.094867889 +0000
+++ /tmp/sg     2014-11-06 12:42:00.463123769 +0000
@@ -2,9 +2,9 @@
 
 Using cdda library version: 10.2
 Using paranoia library version: 10.2
-Checking /dev/sr9 for cdrom...
-        Testing /dev/sr9 for SCSI/MMC interface
-                SG_IO device: /dev/sr9
+Checking /dev/sg16 for cdrom...
+        Testing /dev/sg16 for SCSI/MMC interface
+                SG_IO device: /dev/sg16
 
 CDROM model sensed sensed: PLDS DVD+-RW DH-16A6S YD11
 
@@ -13,9 +13,9 @@
 
 Checking for MMC style command set...
         Drive is MMC style
-        DMA scatter/gather table entries: 1
+        DMA scatter/gather table entries: 167
         table entry size: 131072 bytes
-        maximum theoretical transfer: 55 sectors
+        maximum theoretical transfer: 9185 sectors
         Setting default read size to 27 sectors (63504 bytes).
 
 Verifying CDDA command set...
@@ -23,3 +23,5 @@


I'm happy to try other kernel versions to gather more data.  Which
kernel trees/branches should I try?

I'm also assuming this is more likely to be a SCSI layer bug than a SATA
one, so let me know if that's probably wrong.  Also, is reporting here
best or bugzilla?

Cheers,

Tim.

Re: Very slow throughput when using cdparanoia on two SATA CDROM drives with /dev/sr but not /dev/sg

[Wakko Warner]

Tim Small wrote:
> I've got a big box of audio CDs to read, so I hooked up a load of SATA
> CDROM drives to a machine (Intel motherboard AHCI, and SATA SiI3124
> controllers - in this example one was attached to each host controller),
> so that I could read them in parallel.
> 
> I'm using kernel 3.16.3, with cdparanoia 3.10.2 - both from Debian
> Jessie.  https://www.xiph.org/paranoia/manual.html
> 
> When two (or more) drives are read simultaneously, the performance falls
> to pieces (throughput from each drive drops by 95%) if more than one
> /dev/sr* device is being read by cdparanoia.
> 
> If I tell cdparanoia to use the corresponding /dev/sg devices, then no
> significant throughput drop is experienced when reading multiple drives
> simultaneously.

I had a similar problem burning multiple DVDs at the same time.  I asked
about this on the list more than 2 years ago and was pointed to a patch that
fixed it for me.  It involves sr_mod.  You can unload it, patch the source
and recomple.  When sr_mod.ko is built, insmod that and it worked for me.

See https://lkml.org/lkml/2012/2/28/230 for the patch.

The machine I use to do this is using 3.3.0 with the patch and quite stable.
I was using 3.0.0 at the time.  I haven't tested on any newer kernels.

Do a search for the thread "Burning multiple DVDs at one time".

> As an example, using these two drives:
> 
> [1:0:0:0]    cd/dvd  TSSTcorp DVD+-RW TS-H653G DW10  /dev/sr0   /dev/sg4
> [14:0:0:0]   cd/dvd  PLDS     DVD+-RW DH-16A6S YD11  /dev/sr9   /dev/sg16
> 
> 
> ... in the following results I used "time cdparanoia -v -d /dev/XXX 1
> /tmp/1.wav" - where XXX was substituted for either sr9 or sg16
> 
> 
> On an otherwise idle machine, I did these two sequentially:
> 
> sr9: 38 seconds
> 
> sg16: 38 seconds
> 
> Simultaneous with: cdparanoia -d /dev/sr0 11 /tmp/11.wav (and auto
> restarted that command when it completed) I then ran these two sequentially:
> 
> sr9: 680 seconds
> 
> sg16: 38 seconds
> 
> 
> 
> Simultaneous with: cdparanoia -d /dev/sg4 11 /tmp/11.wav as above:
> 
> sr9: 40 seconds
> 
> sg16: 40 seconds
> 
> 
> This is a diff of the two sets of cdparanoia -v output (using the sr
> devices vs the sg devices):
> 
> --- /tmp/sr     2014-11-06 12:41:43.094867889 +0000
> +++ /tmp/sg     2014-11-06 12:42:00.463123769 +0000
> @@ -2,9 +2,9 @@
>  
>  Using cdda library version: 10.2
>  Using paranoia library version: 10.2
> -Checking /dev/sr9 for cdrom...
> -        Testing /dev/sr9 for SCSI/MMC interface
> -                SG_IO device: /dev/sr9
> +Checking /dev/sg16 for cdrom...
> +        Testing /dev/sg16 for SCSI/MMC interface
> +                SG_IO device: /dev/sg16
>  
>  CDROM model sensed sensed: PLDS DVD+-RW DH-16A6S YD11
>  
> @@ -13,9 +13,9 @@
>  
>  Checking for MMC style command set...
>          Drive is MMC style
> -        DMA scatter/gather table entries: 1
> +        DMA scatter/gather table entries: 167
>          table entry size: 131072 bytes
> -        maximum theoretical transfer: 55 sectors
> +        maximum theoretical transfer: 9185 sectors
>          Setting default read size to 27 sectors (63504 bytes).
>  
>  Verifying CDDA command set...
> @@ -23,3 +23,5 @@
> 
> 
> I'm happy to try other kernel versions to gather more data.  Which
> kernel trees/branches should I try?
> 
> I'm also assuming this is more likely to be a SCSI layer bug than a SATA
> one, so let me know if that's probably wrong.  Also, is reporting here
> best or bugzilla?

-- 
 Microsoft has beaten Volkswagen's world record.  Volkswagen only created 22
 million bugs.

Re: Very slow throughput when using cdparanoia on two SATA CDROM drives with /dev/sr but not /dev/sg

[Christoph Hellwig]

Wakko, any chance you could resend a patch to remove the mutex from the
ioctl path?

Re: Very slow throughput when using cdparanoia on two SATA CDROM drives with /dev/sr but not /dev/sg

[Tim Small]

On 20/11/14 06:34, Christoph Hellwig wrote:
> Wakko, any chance you could resend a patch to remove the mutex from the
> ioctl path?

I'm trying out some local changes which removes the ex-BKL mutex from
sr_block_ioctl() in sr.c, and instead uses a per-drive mutex which I've
added to cdrom_device_info (as per Arnd Bergmann/ James Bottomley's
discussion here https://lkml.org/lkml/2012/2/28/305 ).

This seems to work in a quick test testing (simultaneous cdparanoia on
8-or-so drives took ~10 minutes, whereas last time I tried that it took
more than 18 hours), but I've been short on time to do much more.

At first inspection, it looks like it's just the cdrom-specific ioctls
in drivers/cdrom/cdrom.c which aren't safely preemptible (my conclusion
based on the fact that scsi_ioctl etc. is called elsewhere without
locking (e.g. from /dev/sg and /dev/sd devices - I haven't looked at ).

As a result, I was wondering about moving the mutex_lock / unlock, so
that they wrap just the cdrom-specific ioctl calls instead, although I
suspect there won't be a huge performance benefit from doing so, it
would have the advantage that all the code related to the new mutex to
be moved into cdrom.c

Hopefully I'll grab a bit of time to work on this later this week, and
post a patch.

Cheers,

Tim.

Re: Very slow throughput when using cdparanoia on two SATA CDROM drives with /dev/sr but not /dev/sg

[Christoph Hellwig]

On Thu, Nov 20, 2014 at 08:16:06AM +0000, Tim Small wrote:
> On 20/11/14 06:34, Christoph Hellwig wrote:
> > Wakko, any chance you could resend a patch to remove the mutex from the
> > ioctl path?
> 
> I'm trying out some local changes which removes the ex-BKL mutex from
> sr_block_ioctl() in sr.c, and instead uses a per-drive mutex which I've
> added to cdrom_device_info (as per Arnd Bergmann/ James Bottomley's
> discussion here https://lkml.org/lkml/2012/2/28/305 ).
> 
> This seems to work in a quick test testing (simultaneous cdparanoia on
> 8-or-so drives took ~10 minutes, whereas last time I tried that it took
> more than 18 hours), but I've been short on time to do much more.
> 
> At first inspection, it looks like it's just the cdrom-specific ioctls
> in drivers/cdrom/cdrom.c which aren't safely preemptible (my conclusion
> based on the fact that scsi_ioctl etc. is called elsewhere without
> locking (e.g. from /dev/sg and /dev/sd devices - I haven't looked at ).

Yes, they are safe.  Even inside cdrom_ioctl very little locking is
required, but it might not be worth a lot of effort to go for fine
grained locking there.

> As a result, I was wondering about moving the mutex_lock / unlock, so
> that they wrap just the cdrom-specific ioctl calls instead, although I
> suspect there won't be a huge performance benefit from doing so, it
> would have the advantage that all the code related to the new mutex to
> be moved into cdrom.c

That sounds fine.  It gets the performance critical SG_IO out of the
critical region, and replaces the silly global lock in all the CDROM
drivers with a per-device lock colocated with the data protected.


Totally offtopic:  Jens, is it time to drop the silly paride driver?

[PATCH 0/4] Fix performance burning or extracting audio etc. from multiple optical drives.

[Tim Small]

Fix performance burning or extracting audio etc. from multiple optical
drives.

Patches are against 3.18.0-rc6+

Currently each CDROM driver uses it's own per-driver mutex to protect
non-thread-safe code within their ioctl paths.

These per-driver mutexes were created during the removal of the BKL.

An in-progress ioctl on one optical drive will force an ioctl on
another optical drive in the same system to block (as long as both
drives use the same driver e.g. sr_mod).

Since user-space programs which carry out tasks such as recording
data, or reading raw audio must use ioctls to do-so, this makes it
impractical to carry out more than one such operation simultaneosly
on systems with multiple optical drives (e.g. record two DVDs at the
same time, and/or extract raw audio data from two CDs simultaneously).

The calls scsi_ioctl() and related now happen without any locking, as
these are thread-safe (e.g. as called by sd.c or sg.c).

With these patches applied, the time to simultaneously extract audio
from 10 CDs, using the 'cdparanoia' utility, reduced more than 100
fold from around 18 hours to 10 minutes.

I've only done little bits and bobs of driver development in the past,
so hopefully this first attempt isn't too far of the mark...

I've tested the IDE and SCSI subsystem patches, but I don't have any
paride, or GDROM hardware (however the changes which are specific to
these two are trivial).

Cheers,

Tim.


Tim Small (4):
  enable cdrom_ioctl() to be called without holding ex-BKL mutexes
  Remove ex-BKL lock from ioctl path; fix simultaneous record on >1
    drive
  Remove ex-BKL lock from ioctl path; fix simultaneous record on >1
    drive
  Remove ex-BKL lock from ioctl path; fix simultaneous record on >1
    drive

 drivers/block/paride/pcd.c |  8 +----
 drivers/cdrom/cdrom.c      | 86 +++++++++++++++++++++++++++++++---------------
 drivers/cdrom/gdrom.c      |  8 +----
 drivers/ide/ide-cd.c       | 42 ++++++++++------------
 drivers/scsi/sr.c          | 14 +++-----
 include/linux/cdrom.h      |  1 +
 6 files changed, 85 insertions(+), 74 deletions(-)

-- 
2.1.3

[PATCH 1/4] enable cdrom_ioctl() to be called without holding ex-BKL mutexes

[Tim Small]

Signed-off-by: Tim Small <tim@seoss.co.uk>
---
 drivers/cdrom/cdrom.c | 86 +++++++++++++++++++++++++++++++++++----------------
 include/linux/cdrom.h |  1 +
 2 files changed, 60 insertions(+), 27 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 5d28a45..c39bef1 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -633,6 +633,8 @@ int register_cdrom(struct cdrom_device_info *cdi)
 	if (!cdo->generic_packet)
 		cdo->generic_packet = cdrom_dummy_generic_packet;
 
+	mutex_init(&cdi->ioctl_mutex);
+
 	cd_dbg(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
 	mutex_lock(&cdrom_mutex);
 	list_add(&cdi->list, &cdrom_list);
@@ -3315,41 +3317,60 @@ int cdrom_ioctl(struct cdrom_device_info *cdi, struct block_device *bdev,
 	if (ret != -ENOTTY)
 		return ret;
 
+	mutex_lock(&cdi->ioctl_mutex);
+
 	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;
 	}
 
 	/*
@@ -3361,7 +3382,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;
 	}
 
 	/*
@@ -3371,27 +3392,38 @@ 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->ioctl_mutex);
+	return ret;
 }
 
 EXPORT_SYMBOL(cdrom_get_last_written);
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index 8609d57..954e659 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -40,6 +40,7 @@ struct cdrom_device_info {
 	struct list_head list;		/* linked list of all device_info */
 	struct gendisk *disk;		/* matching block layer disk */
 	void *handle;		        /* driver-dependent data */
+	struct mutex ioctl_mutex;	/* cdrom_ioctl_* not all thread safe */
 /* specifications */
 	int mask;                       /* mask of capability: disables them */
 	int speed;			/* maximum speed for reading data */
-- 
2.1.3

[PATCH 2/4] Remove ex-BKL lock from ioctl path; fix simultaneous record on >1 drive

[Tim Small]

Signed-off-by: Tim Small <tim@seoss.co.uk>
---
 drivers/scsi/sr.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 2de44cc..93111b1 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -547,8 +547,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);
-
 	/*
 	 * Send SCSI addressing ioctls directly to mid level, send other
 	 * ioctls to cdrom/block level.
@@ -556,13 +554,12 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	switch (cmd) {
 	case SCSI_IOCTL_GET_IDLUN:
 	case SCSI_IOCTL_GET_BUS_NUMBER:
-		ret = scsi_ioctl(sdev, cmd, argp);
-		goto out;
+		return scsi_ioctl(sdev, cmd, argp);
 	}
 
 	ret = cdrom_ioctl(&cd->cdi, bdev, mode, cmd, arg);
 	if (ret != -ENOSYS)
-		goto out;
+		return ret;
 
 	/*
 	 * ENODEV means that we didn't recognise the ioctl, or that we
@@ -573,12 +570,9 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	ret = scsi_nonblockable_ioctl(sdev, cmd, argp,
 					(mode & FMODE_NDELAY) != 0);
 	if (ret != -ENODEV)
-		goto out;
-	ret = scsi_ioctl(sdev, cmd, argp);
+		return ret;
 
-out:
-	mutex_unlock(&sr_mutex);
-	return ret;
+	return scsi_ioctl(sdev, cmd, argp);
 }
 
 static unsigned int sr_block_check_events(struct gendisk *disk,
-- 
2.1.3

[PATCH 3/4] Remove ex-BKL lock from ioctl path; fix simultaneous record on >1 drive

[Tim Small]

Signed-off-by: Tim Small <tim@seoss.co.uk>
---
 drivers/block/paride/pcd.c | 8 +-------
 drivers/cdrom/gdrom.c      | 8 +-------
 2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
index 3b7c9f1..61cabfd 100644
--- a/drivers/block/paride/pcd.c
+++ b/drivers/block/paride/pcd.c
@@ -248,13 +248,7 @@ 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/gdrom.c b/drivers/cdrom/gdrom.c
index 584bc31..f01d01f 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -519,13 +519,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 = {
-- 
2.1.3

[PATCH 4/4] Remove ex-BKL lock from ioctl path; fix simultaneous record on >1 drive

[Tim Small]

Signed-off-by: Tim Small <tim@seoss.co.uk>
---
 drivers/ide/ide-cd.c | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 0b510ba..946d458 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1656,37 +1656,33 @@ static int idecd_get_spindown(struct cdrom_device_info *cdi, unsigned long arg)
 	return 0;
 }
 
-static int idecd_locked_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 err;
-
-	switch (cmd) {
-	case CDROMSETSPINDOWN:
-		return idecd_set_spindown(&info->devinfo, arg);
-	case CDROMGETSPINDOWN:
-		return idecd_get_spindown(&info->devinfo, arg);
-	default:
-		break;
-	}
-
-	err = generic_ide_ioctl(info->drive, bdev, cmd, arg);
-	if (err == -EINVAL)
-		err = cdrom_ioctl(&info->devinfo, bdev, mode, cmd, arg);
-
-	return err;
-}
-
 static int idecd_ioctl(struct block_device *bdev, fmode_t mode,
 			     unsigned int cmd, unsigned long arg)
 {
+	struct cdrom_info *info;
 	int ret;
 
 	mutex_lock(&ide_cd_mutex);
-	ret = idecd_locked_ioctl(bdev, mode, cmd, arg);
+	info = ide_drv_g(bdev->bd_disk, cdrom_info);
+
+	switch (cmd) {
+	case CDROMSETSPINDOWN:
+		ret = idecd_set_spindown(&info->devinfo, arg);
+		goto outunlock;
+	case CDROMGETSPINDOWN:
+		ret = idecd_get_spindown(&info->devinfo, arg);
+		goto outunlock;
+	}
+
+	ret = generic_ide_ioctl(info->drive, bdev, cmd, arg);
+	if (ret != -EINVAL)
+		goto outunlock;
+
 	mutex_unlock(&ide_cd_mutex);
+	return cdrom_ioctl(&info->devinfo, bdev, mode, cmd, arg);
 
+outunlock:
+	mutex_unlock(&ide_cd_mutex);
 	return ret;
 }
 
-- 
2.1.3

Re: [PATCH 0/4] Fix performance burning or extracting audio etc. from multiple optical drives.

[Christoph Hellwig]

This looks reasonable to me.  Jens, do you want to takes this
through the block tree?

Acked-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 0/4] Fix performance burning or extracting audio etc. from multiple optical drives.

[Jens Axboe]

On 11/25/2014 09:26 AM, Christoph Hellwig wrote:
> This looks reasonable to me.  Jens, do you want to takes this
> through the block tree?
> 
> Acked-by: Christoph Hellwig <hch@lst.de>

Yeah I can do that, though do we really need to do paride here?

Tim, you also need to work on your changelogs. Please resend (dropping
paride), and add proper subject and commit message. Patches 2-4 have
identical subjects, and no commit message...

-- 
Jens Axboe

Re: [PATCH 0/4] Fix performance burning or extracting audio etc. from multiple optical drives.

[Christoph Hellwig]

On Tue, Nov 25, 2014 at 09:30:19AM -0700, Jens Axboe wrote:
> On 11/25/2014 09:26 AM, Christoph Hellwig wrote:
> > This looks reasonable to me.  Jens, do you want to takes this
> > through the block tree?
> > 
> > Acked-by: Christoph Hellwig <hch@lst.de>
> 
> Yeah I can do that, though do we really need to do paride here?

Well, no need to keep the mutex around in paride for no reason.  But my
preference would be to just kill off paride anyway :)

Re: [PATCH 0/4] Fix performance burning or extracting audio etc. from multiple optical drives.

[Jens Axboe]

On 11/25/2014 09:32 AM, Christoph Hellwig wrote:
> On Tue, Nov 25, 2014 at 09:30:19AM -0700, Jens Axboe wrote:
>> On 11/25/2014 09:26 AM, Christoph Hellwig wrote:
>>> This looks reasonable to me.  Jens, do you want to takes this
>>> through the block tree?
>>>
>>> Acked-by: Christoph Hellwig <hch@lst.de>
>>
>> Yeah I can do that, though do we really need to do paride here?
> 
> Well, no need to keep the mutex around in paride for no reason.  But my
> preference would be to just kill off paride anyway :)

That's sort of my point. Performance optimization and paride, well...
But it's not a huge deal, we can apply it.

-- 
Jens Axboe

Re: [PATCH 0/4] Fix performance burning or extracting audio etc. from multiple optical drives.

[Tim Small]

On 25/11/14 16:30, Jens Axboe wrote:

> do we really need to do paride here?

I did consider this, but I made the change there too on the basis that:

. paride has received a few commits this year (and is listed as being
    maintained)
. The change is trivial
. It fixes a performance regression which was introduced during the BKL
    removal (mutex being retained by sleeping processes).

I'm happy to drop it, if you prefer.

> Patches 2-4 have identical subjects, and no commit message...

Sorry about that, will fix it with next version.

Having just seen this thread from 2013:

http://permalink.gmane.org/gmane.linux.scsi/79483

I decided to exercise the eject code path a bit more by triggering
simultaneous eject commands on all 11 optical drives in my test box,
followed by simultaneous close-tray commands, repeatedly.

I haven't been able to reproduce the error reported in that email, but
from observing the behaviour of the drives it looks like access to pata
drives is being serialising elsewhere, so the issue in that link may
have been fixed?

Unfortunately running these tests did eventually make all further
attempts to open /dev/sr* block on my test box.

I've stared at the code for a while, but not making any headway
currently, except that a blocking blk_execute_rq (called by
test_unit_ready) is then causing all over cdrom open/close calls to
block (because sr_mutex is held by sr_block_open(), and in turn calls
check_disk_change... scsi_test_unit_ready).

How do I work out why blk_execute_rq is blocking?

# ps -l 3779 2383 3780
F S   UID   PID  PPID  C PRI  NI ADDR SZ WCHAN  TTY        TIME CMD
1 D     0  2383     2  0  80   0 -     0 blk_ex ?          0:00 [kworker/1:2]
0 D     0  3779  1034  0  80   0 -  1057 blk_ex pts/0      0:00 eject -t /dev/sr7
0 D     0  3780  1034  0  80   0 -     0 sr_blo pts/0      0:00 [eject]



/proc/3779/stack-[<ffffffff812e47cb>] blk_execute_rq+0x16b/0x210
/proc/3779/stack-[<ffffffffa0291cb1>] scsi_execute+0x141/0x1f0 [scsi_mod]
/proc/3779/stack-[<ffffffffa0293e1e>] scsi_execute_req_flags+0x8e/0x100 [scsi_mod]
/proc/3779/stack-[<ffffffffa02944f3>] scsi_test_unit_ready+0x83/0x130 [scsi_mod]
/proc/3779/stack:[<ffffffffa05a9a7c>] sr_check_events+0x13c/0x310 [sr_mod]
/proc/3779/stack-[<ffffffffa06cd05c>] cdrom_check_events+0x1c/0x40 [cdrom]
/proc/3779/stack:[<ffffffffa05a9ecd>] sr_block_check_events+0x2d/0x30 [sr_mod]
/proc/3779/stack-[<ffffffff812eed41>] disk_check_events+0x51/0x170
/proc/3779/stack-[<ffffffff812f068c>] disk_clear_events+0x6c/0x130
/proc/3779/stack-[<ffffffff81227150>] check_disk_change+0x30/0x80
/proc/3779/stack-[<ffffffffa06cfd9a>] cdrom_open+0x4a/0xba0 [cdrom]
/proc/3779/stack:[<ffffffffa05aa6c2>] sr_block_open+0x92/0x130 [sr_mod]
/proc/3779/stack-[<ffffffff81227b71>] __blkdev_get+0xd1/0x4b0
/proc/3779/stack-[<ffffffff81227f91>] blkdev_get+0x41/0x3e0
/proc/3779/stack-[<ffffffff812283fe>] blkdev_open+0x6e/0x90
/proc/3779/stack-[<ffffffff811e6cbd>] do_dentry_open+0x1ed/0x360
/proc/3779/stack-[<ffffffff811e6f99>] vfs_open+0x49/0x50
/proc/3779/stack-[<ffffffff811fa849>] do_last+0x239/0x1520
/proc/3779/stack-[<ffffffff811fbbe7>] path_openat+0xb7/0x6a0
/proc/3779/stack-[<ffffffff811fd3ea>] do_filp_open+0x3a/0xb0
/proc/3779/stack-[<ffffffff811e896c>] do_sys_open+0x12c/0x220
/proc/3779/stack-[<ffffffff811e8a7e>] SyS_open+0x1e/0x20


/proc/2383/stack-[<ffffffff812e47cb>] blk_execute_rq+0x16b/0x210
/proc/2383/stack-[<ffffffffa0291cb1>] scsi_execute+0x141/0x1f0 [scsi_mod]
/proc/2383/stack-[<ffffffffa0293e1e>] scsi_execute_req_flags+0x8e/0x100 [scsi_mod]
/proc/2383/stack-[<ffffffffa02944f3>] scsi_test_unit_ready+0x83/0x130 [scsi_mod]
/proc/2383/stack:[<ffffffffa05a9a7c>] sr_check_events+0x13c/0x310 [sr_mod]
/proc/2383/stack-[<ffffffffa06cd05c>] cdrom_check_events+0x1c/0x40 [cdrom]
/proc/2383/stack:[<ffffffffa05a9ecd>] sr_block_check_events+0x2d/0x30 [sr_mod]
/proc/2383/stack-[<ffffffff812eed41>] disk_check_events+0x51/0x170
/proc/2383/stack-[<ffffffff812eee7c>] disk_events_workfn+0x1c/0x20
/proc/2383/stack-[<ffffffff8108b91f>] process_one_work+0x1df/0x520
/proc/2383/stack-[<ffffffff8108bf3b>] worker_thread+0x6b/0x4a0
/proc/2383/stack-[<ffffffff8109130b>] kthread+0x10b/0x130
/proc/2383/stack-[<ffffffff815b8afc>] ret_from_fork+0x7c/0xb0
/proc/2383/stack-[<ffffffffffffffff>] 0xffffffffffffffff


/proc/3780/stack:[<ffffffffa05aa604>] sr_block_release+0x24/0x50 [sr_mod]
/proc/3780/stack-[<ffffffff81227a65>] __blkdev_put+0x185/0x1c0
/proc/3780/stack-[<ffffffff81228472>] blkdev_put+0x52/0x180
/proc/3780/stack-[<ffffffff81228658>] blkdev_close+0x28/0x30
/proc/3780/stack-[<ffffffff811eb4e5>] __fput+0xf5/0x210
/proc/3780/stack-[<ffffffff811eb64e>] ____fput+0xe/0x10
/proc/3780/stack-[<ffffffff8108f824>] task_work_run+0xc4/0xf0
/proc/3780/stack-[<ffffffff810734fa>] do_exit+0x3da/0xb70
/proc/3780/stack-[<ffffffff81073d34>] do_group_exit+0x54/0xe0
/proc/3780/stack-[<ffffffff81073dd4>] SyS_exit_group+0x14/0x20
/proc/3780/stack-[<ffffffff815b8bad>] system_call_fastpath+0x16/0x1b


# grep -l scsi_exec /proc/*/stack
/proc/2383/stack
/proc/3779/stack
# grep -l ioctl /proc/*/stack
# grep -l blk_execute /proc/*/stack
/proc/2383/stack
/proc/3779/stack
#


Cheers,

Tim.

Re: [PATCH 0/4] Fix performance burning or extracting audio etc. from multiple optical drives.

[Tim Small]

On 26/11/14 15:33, Tim Small wrote:
> I decided to exercise the eject code path a bit more by triggering
> simultaneous eject commands on all 11 optical drives in my test box,
> followed by simultaneous close-tray commands, repeatedly.
...
>
> Unfortunately running these tests did eventually make all further
> attempts to open /dev/sr* block on my test box.
>
> I've stared at the code for a while, but not making any headway
> currently, except that a blocking blk_execute_rq (called by
> test_unit_ready) is then causing all over cdrom open/close calls to
> block (because sr_mutex is held by sr_block_open(), and in turn calls
> check_disk_change... scsi_test_unit_ready).
>
> How do I work out why blk_execute_rq is blocking?
>
> # ps -l 3779 2383 3780
> F S   UID   PID  PPID  C PRI  NI ADDR SZ WCHAN  TTY        TIME CMD
> 1 D     0  2383     2  0  80   0 -     0 blk_ex ?          0:00 [kworker/1:2]
> 0 D     0  3779  1034  0  80   0 -  1057 blk_ex pts/0      0:00 eject -t /dev/sr7
> 0 D     0  3780  1034  0  80   0 -     0 sr_blo pts/0      0:00 [eject]
>
>
>
> /proc/3779/stack-[<ffffffff812e47cb>] blk_execute_rq+0x16b/0x210
> /proc/3779/stack-[<ffffffffa0291cb1>] scsi_execute+0x141/0x1f0 [scsi_mod]
> /proc/3779/stack-[<ffffffffa0293e1e>] scsi_execute_req_flags+0x8e/0x100 [scsi_mod]

> /proc/2383/stack-[<ffffffff812e47cb>] blk_execute_rq+0x16b/0x210
> /proc/2383/stack-[<ffffffffa0291cb1>] scsi_execute+0x141/0x1f0 [scsi_mod]
> /proc/2383/stack-[<ffffffffa0293e1e>] scsi_execute_req_flags+0x8e/0x100 [scsi_mod]
> /proc/2383/stack-[<ffffffffa02944f3>] scsi_test_unit_ready+0x83/0x130 [scsi_mod]

An extra data point, the drive is showing busy:

# cat /sys/block/sr7/device/device_busy
1

I was wondering if this might be an unrelated bug in the drive or host
adaptor driver?

This device is attached to an old PCI card using pata_pdc2027x, and has
an old firmware version, so I'm going to try and change both of those
things and try again.

Tim.

Re: [PATCH 0/4] Fix performance burning or extracting audio etc. from multiple optical drives.

[Julian Calaby]

Hi Tim,

On Thu, Nov 27, 2014 at 2:33 AM, Tim Small <tim@seoss.co.uk> wrote:
> On 25/11/14 16:30, Jens Axboe wrote:
>
>> do we really need to do paride here?
>
> I did consider this, but I made the change there too on the basis that:
>
> . paride has received a few commits this year (and is listed as being
>     maintained)
> . The change is trivial
> . It fixes a performance regression which was introduced during the BKL
>     removal (mutex being retained by sleeping processes).
>
> I'm happy to drop it, if you prefer.
>
>> Patches 2-4 have identical subjects, and no commit message...
>
> Sorry about that, will fix it with next version.
>
> Having just seen this thread from 2013:
>
> http://permalink.gmane.org/gmane.linux.scsi/79483
>
> I decided to exercise the eject code path a bit more by triggering
> simultaneous eject commands on all 11 optical drives in my test box,
> followed by simultaneous close-tray commands, repeatedly.
>
> I haven't been able to reproduce the error reported in that email, but
> from observing the behaviour of the drives it looks like access to pata
> drives is being serialising elsewhere, so the issue in that link may
> have been fixed?
>
> Unfortunately running these tests did eventually make all further
> attempts to open /dev/sr* block on my test box.
>
> I've stared at the code for a while, but not making any headway
> currently, except that a blocking blk_execute_rq (called by
> test_unit_ready) is then causing all over cdrom open/close calls to
> block (because sr_mutex is held by sr_block_open(), and in turn calls
> check_disk_change... scsi_test_unit_ready).
>
> How do I work out why blk_execute_rq is blocking?

As you're playing with locks, I assume you're running with LOCKDEP
enabled? If not, that might tell you what's going on.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 0/4] Fix performance burning or extracting audio etc. from multiple optical drives.

[Tim Small]

On 26/11/14 23:01, Julian Calaby wrote:
> As you're playing with locks, I assume you're running with LOCKDEP
> enabled? If not, that might tell you what's going on.

Hi,

Sorry - I should have said - LOCKDEP is on, and not reporting anything...

Cheers,

Tim.

Re: [PATCH 0/4] Fix performance burning or extracting audio etc. from multiple optical drives.

[Wakko Warner]

Tim Small wrote:
> Fix performance burning or extracting audio etc. from multiple optical
> drives.

I know this is a bit late and is still not in 4.3.  I applied 2 of the
patches.  I did not apply the ide-cd, the paride nor the gdrom since I don't
have any of those.

> Patches are against 3.18.0-rc6+

I had to make some modifications to the patch for sr.c since it has changed
since.  cdrom.[ch] worked verbatim.

I tested on a system with 3 drives.  ejecting all drives didn't happen at
the same time, but I think it's because they are different brands and one
didn't have a disc in.  I did notice the leds coming on about the same time
though.  eject -t on all drives happened at the same time.

The patch I used previously on 3.3.0 removed all mutex_lock and mutex_unlock
lines from sr.c where as this patchset didn't.  I plan on trying to burn 3
dvds to see if it works.

Thanks for your work on the patches.

-- 
 Microsoft has beaten Volkswagen's world record.  Volkswagen only created 22
 million bugs.

Re: [PATCH 0/4] Fix performance burning or extracting audio etc. from multiple optical drives.

[Tim Small]

On 05/11/15 01:38, Wakko Warner wrote:
> I tested on a system with 3 drives.  ejecting all drives didn't happen at
> the same time, but I think it's because they are different brands and one
> didn't have a disc in.  I did notice the leds coming on about the same time
> though.  eject -t on all drives happened at the same time.
> 
> The patch I used previously on 3.3.0 removed all mutex_lock and mutex_unlock
> lines from sr.c where as this patchset didn't.  I plan on trying to burn 3
> dvds to see if it works.
> 
> Thanks for your work on the patches.

No problem.  I haven't had any time to follow up (and probably won't for
the foreseeable - I've got far too much on at the moment unfortunately),
and the locking issues looked non-trivial unfortunately.

In my testing burning, and audio extracting etc. worked pretty
flawlessly IIRC, it was just the eject/load path which seemed to have
locking issues.

The test was just a shell scripts which ran:

while true ; do eject /dev/sr0 ; eject -T /dev/sr0 ; done

for every drive in the system simultaneously.

Hopefully it's a good start if someone wants to pick it up.  It's
possible that there's an easy way of leaving the old mutexes (or adding
more) around the relevant open/eject/load paths only, but I can't
remember the code now unfortunately.

If anyone wants to have a go, I think I can probably rig up about 8
drives to a testrig here, and will be happy to give it a test.

Tim.