Message-ID: <20150202082405.6d7d0003@kant>
Date:	Mon, 2 Feb 2015 08:24:05 +0100
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	Joe Perches <joe@...ches.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: checkpatch: length of git commit IDs

On Feb 01 Joe Perches wrote:
> On Sun, 2015-02-01 at 21:30 +0100, Stefan Richter wrote:
> > On Feb 01 Joe Perches wrote:
> > > On Sun, 2015-02-01 at 20:59 +0100, Stefan Richter wrote:
> > > > > ERROR: Please use 12 or more chars for the git commit ID like: 'Commit
> > > > > 01234567890ab ("commit description")' #5: 
> > > > > Commit 2a48fc0ab242 "block: autoconvert trivial BKL users to private
[...]
> > Thanks for the tip; this works for me.  (checkpatch.pl from current -next
> > does actually not complain about the line without parentheses.)
> 
> Hmm, I think that should be a defect.
> 
> Can you send me your git format-patch output please?

First of all to be sure:  The checkpatch.pl which I last referred to is
158501 bytes in size, and the md5sum is bf953e10cbd7405bcf3e885b24c832bd.
$ scripts/checkpatch.pl patches/scsi-sr-fix-multi-drive-performance-remove-bkl-replacement.patch 
total: 0 errors, 0 warnings, 71 lines checked

patches/scsi-sr-fix-multi-drive-performance-remove-bkl-replacement.patch has no obvious style problems and is ready for submission.

Here is the patch file:

Date: Tue, 28 Feb 2012 15:32:44 +0100
From: Stefan Richter <stefanr@...6.in-berlin.de>
Subject: [SCSI] sr: fix multi-drive performance, remove BKL replacement

Commit 2a48fc0ab242 "block: autoconvert trivial BKL users to private
mutex" and other commits at the time mechanically swapped BKL for
per-driver global mutexes.  If the sr driver is any indication, these
replacements have still not been checked by anybody for their
necessessity, removed where possible, or the sections they serialize
reduced to a necessary minimum.

The sr_mutex in particular very noticably degraded performance of
CD-DA ripping with multiple drives in parallel.  When several
instances of "grip" are used with two or more drives, their GUIs
became laggier, as did the KDE file manager GUI, and drive utilization
was reduced.  (During ripping, drive lights flicker instead of staying
on most of the time.)  IOW time to rip a stack of CDs was increased.
I didn't measure this but it is highly noticeable.

On the other hand, I don't see what state sr_mutex would protect.
So I removed it entirely and that works fine for me.

Tested with up to six CD-ROM drives connected at the same time (1x
IDE, 5x FireWire), 12 grip instances running (2 per drive, one for
ripping and the other just polling the TOC as a test), and of course
udisks-daemon sitting in the background and polling the CD-ROM drives
as usual.  GUI lags are reduced and ripping throughput increased to
a level how I remember it from BKL era.

Also tested:  Erasing and writing a CD-RW with K3B/cdrecord while grip
and udisks-daemon poll the CD-RW drive and other grip instances rip
CD-DA from three other CD-ROM drives.

========
On Tue, 2012-02-28 at 16:42 +0000, Arnd Bergmann wrote:
> I took another look and I believe the cdi->use_count in
> cdrom_open/cdrom_release still requires some protection that is
> currently provided by sr_mutex. Some parts of cdrom_ioctl also
> access this variable and things like cdi->options or cdi->keeplocked.
> 
> I could imagine that you can get rid of the mutex if you turn those
> into atomics and bitops, but there may be other parts of cdrom_device_info
> that need locking. A safer option to solve the performance problems
> could be to replace sr_mutex with a per-device mutex inside of
> cdrom_device_info.
========
On Fri, 2013-01-04 at 00:11 +0100, Otto Meta wrote:
> Otto Meta wrote:  
> > The single mutex for the sr module, introduced as a BKL replacement,
> > globally serialises all sr ioctls, which hurts multi-drive performance.
> > 
> > This patch replaces sr_mutex with per-device mutexes in struct scsi_cd,
> > allowing concurrent ioctls on different sr devices.  
> 
> Unfortunately it wasn't as easy as that. The patch seems to introduce
> a race condition that corrupts a drive's state under certain circumstances.
> 
> When two drives (e.g. sr0 and sr1) are attached to the same IDE cable, one
> drive has its door locked, which will usually be the case after any operation
> on the drive with inserted media (and whenever it feels like it, even with
> dev.cdrom.lock=0), and the other drive is unlocked, then executing
> 
>   $ eject sr0 & eject sr1
> 
> will eject the unlocked drive and the locked drive will return
> 
>   eject: unable to eject, last error: Inappropriate ioctl for device
> 
> 
> Other drivers down the road probably don't expect concurrent ioctls, so this
> patch cannot be applied safely at this time. Sorry about the noise.
> 
> For the record: Tested with kernels 3.2.35 and 3.8.0-rc1, using IDE CD/DVD
> drives connected via the drivers ata_piix and pata_pdc202xx_old.  
========

Signed-off-by: Stefan Richter <stefanr@...6.in-berlin.de>
---
 drivers/scsi/sr.c |   24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -76,7 +76,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 int sr_init_command(struct scsi_cmnd *SCpnt);
@@ -516,27 +515,23 @@ static int sr_init_command(struct scsi_c
 
 static int sr_block_open(struct block_device *bdev, fmode_t mode)
 {
-	struct scsi_cd *cd;
+	struct scsi_cd *cd = scsi_cd_get(bdev->bd_disk);
 	int ret = -ENXIO;
 
-	mutex_lock(&sr_mutex);
-	cd = scsi_cd_get(bdev->bd_disk);
 	if (cd) {
 		ret = cdrom_open(&cd->cdi, bdev, mode);
 		if (ret)
 			scsi_cd_put(cd);
 	}
-	mutex_unlock(&sr_mutex);
 	return ret;
 }
 
 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,
@@ -547,12 +542,10 @@ static int sr_block_ioctl(struct block_d
 	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);
 	if (ret)
-		goto out;
+		return ret;
 
 	/*
 	 * Send SCSI addressing ioctls directly to mid level, send other
@@ -561,19 +554,14 @@ static int sr_block_ioctl(struct block_d
 	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;
 
-	ret = scsi_ioctl(sdev, cmd, argp);
-
-out:
-	mutex_unlock(&sr_mutex);
-	return ret;
+	return scsi_ioctl(sdev, cmd, argp);
 }
 
 static unsigned int sr_block_check_events(struct gendisk *disk,




-- 
Stefan Richter
-=====-===== --=- ---=-
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists