linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 3/5] st: get rid of scsi_tapes array
@ 2012-05-21 23:14 Lee Duncan
  2012-07-01  8:57 ` Kai Makisara
  0 siblings, 1 reply; 5+ messages in thread
From: Lee Duncan @ 2012-05-21 23:14 UTC (permalink / raw)
  To: linux-scsi; +Cc: linux-kernel, kai.makisara, jeffm

From: Jeff Mahoney <jeffm@suse.com>

st currently allocates an array to store pointers to all of the
scsi_tape objects. It's used to discover available indexes to use as the
base for the minor number selection and later to look up scsi_tape
devices for character devices.

We switch to using an IDR for minor selection and a pointer from
st_modedef back to scsi_tape for the lookups.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/st.c |  172 ++++++++++++++++++++---------------------------------
 drivers/scsi/st.h |    2 +
 2 files changed, 65 insertions(+), 109 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index f437b7f..a3718a5 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -37,6 +37,7 @@ static const char *verstr = "20101219";
 #include <linux/blkdev.h>
 #include <linux/moduleparam.h>
 #include <linux/cdev.h>
+#include <linux/idr.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
 
@@ -81,9 +82,6 @@ static int try_direct_io = TRY_DIRECT_IO;
 static int try_rdio = 1;
 static int try_wdio = 1;
 
-static int st_dev_max;
-static int st_nr_dev;
-
 static struct class st_sysfs_class;
 static struct device_attribute st_dev_attrs[];
 
@@ -174,13 +172,9 @@ static int debugging = DEBUG;
    24 bits) */
 #define SET_DENS_AND_BLK 0x10001
 
-static DEFINE_RWLOCK(st_dev_arr_lock);
-
 static int st_fixed_buffer_size = ST_FIXED_BUFFER_SIZE;
 static int st_max_sg_segs = ST_MAX_SG;
 
-static struct scsi_tape **scsi_tapes = NULL;
-
 static int modes_defined;
 
 static int enlarge_buffer(struct st_buffer *, int, int);
@@ -222,6 +216,10 @@ static void scsi_tape_release(struct kref *);
 #define to_scsi_tape(obj) container_of(obj, struct scsi_tape, kref)
 
 static DEFINE_MUTEX(st_ref_mutex);
+static DEFINE_SPINLOCK(st_index_lock);
+static DEFINE_SPINLOCK(st_use_lock);
+static DEFINE_IDR(st_index_idr);
+
 
 \f

 #include "osst_detect.h"
@@ -239,10 +237,9 @@ static struct scsi_tape *scsi_tape_get(int dev)
 	struct scsi_tape *STp = NULL;
 
 	mutex_lock(&st_ref_mutex);
-	write_lock(&st_dev_arr_lock);
+	spin_lock(&st_index_lock);
 
-	if (dev < st_dev_max && scsi_tapes != NULL)
-		STp = scsi_tapes[dev];
+	STp = idr_find(&st_index_idr, dev);
 	if (!STp) goto out;
 
 	kref_get(&STp->kref);
@@ -259,7 +256,7 @@ out_put:
 	kref_put(&STp->kref, scsi_tape_release);
 	STp = NULL;
 out:
-	write_unlock(&st_dev_arr_lock);
+	spin_unlock(&st_index_lock);
 	mutex_unlock(&st_ref_mutex);
 	return STp;
 }
@@ -1202,12 +1199,12 @@ static int st_open(struct inode *inode, struct file *filp)
 		return -ENXIO;
 	}
 
-	write_lock(&st_dev_arr_lock);
 	filp->private_data = STp;
 	name = tape_name(STp);
 
+	spin_lock(&st_use_lock);
 	if (STp->in_use) {
-		write_unlock(&st_dev_arr_lock);
+		spin_unlock(&st_use_lock);
 		scsi_tape_put(STp);
 		mutex_unlock(&st_mutex);
 		DEB( printk(ST_DEB_MSG "%s: Device already in use.\n", name); )
@@ -1215,7 +1212,7 @@ static int st_open(struct inode *inode, struct file *filp)
 	}
 
 	STp->in_use = 1;
-	write_unlock(&st_dev_arr_lock);
+	spin_unlock(&st_use_lock);
 	STp->rew_at_close = STp->autorew_dev = (iminor(inode) & 0x80) == 0;
 
 	if (scsi_autopm_get_device(STp->device) < 0) {
@@ -1404,9 +1401,9 @@ static int st_release(struct inode *inode, struct file *filp)
 		do_door_lock(STp, 0);
 
 	normalize_buffer(STp->buffer);
-	write_lock(&st_dev_arr_lock);
+	spin_lock(&st_use_lock);
 	STp->in_use = 0;
-	write_unlock(&st_dev_arr_lock);
+	spin_unlock(&st_use_lock);
 	scsi_autopm_put_device(STp->device);
 	scsi_tape_put(STp);
 
@@ -4029,58 +4026,16 @@ static int st_probe(struct device *dev)
 		goto out_buffer_free;
 	}
 
-	write_lock(&st_dev_arr_lock);
-	if (st_nr_dev >= st_dev_max) {
-		struct scsi_tape **tmp_da;
-		int tmp_dev_max;
-
-		tmp_dev_max = max(st_nr_dev * 2, 8);
-		if (tmp_dev_max > ST_MAX_TAPES)
-			tmp_dev_max = ST_MAX_TAPES;
-		if (tmp_dev_max <= st_nr_dev) {
-			write_unlock(&st_dev_arr_lock);
-			printk(KERN_ERR "st: Too many tape devices (max. %d).\n",
-			       ST_MAX_TAPES);
-			goto out_put_disk;
-		}
-
-		tmp_da = kzalloc(tmp_dev_max * sizeof(struct scsi_tape *), GFP_ATOMIC);
-		if (tmp_da == NULL) {
-			write_unlock(&st_dev_arr_lock);
-			printk(KERN_ERR "st: Can't extend device array.\n");
-			goto out_put_disk;
-		}
-
-		if (scsi_tapes != NULL) {
-			memcpy(tmp_da, scsi_tapes,
-			       st_dev_max * sizeof(struct scsi_tape *));
-			kfree(scsi_tapes);
-		}
-		scsi_tapes = tmp_da;
-
-		st_dev_max = tmp_dev_max;
-	}
-
-	for (i = 0; i < st_dev_max; i++)
-		if (scsi_tapes[i] == NULL)
-			break;
-	if (i >= st_dev_max)
-		panic("scsi_devices corrupt (st)");
-
 	tpnt = kzalloc(sizeof(struct scsi_tape), GFP_ATOMIC);
 	if (tpnt == NULL) {
-		write_unlock(&st_dev_arr_lock);
 		printk(KERN_ERR "st: Can't allocate device descriptor.\n");
 		goto out_put_disk;
 	}
 	kref_init(&tpnt->kref);
 	tpnt->disk = disk;
-	sprintf(disk->disk_name, "st%d", i);
 	disk->private_data = &tpnt->driver;
 	disk->queue = SDp->request_queue;
 	tpnt->driver = &st_template;
-	scsi_tapes[i] = tpnt;
-	dev_num = i;
 
 	tpnt->device = SDp;
 	if (SDp->scsi_level <= 2)
@@ -4126,6 +4081,7 @@ static int st_probe(struct device *dev)
 		STm->default_compression = ST_DONT_TOUCH;
 		STm->default_blksize = (-1);	/* No forced size */
 		STm->default_density = (-1);	/* No forced density */
+		STm->tape = tpnt;
 	}
 
 	for (i = 0; i < ST_NBR_PARTITIONS; i++) {
@@ -4145,8 +4101,30 @@ static int st_probe(struct device *dev)
 	    tpnt->blksize_changed = 0;
 	mutex_init(&tpnt->lock);
 
-	st_nr_dev++;
-	write_unlock(&st_dev_arr_lock);
+	if (!idr_pre_get(&st_index_idr, GFP_KERNEL)) {
+		printk(KERN_WARNING "st: idr expansion failed\n");
+		error = -ENOMEM;
+		goto out_put_disk;
+	}
+
+	spin_lock(&st_index_lock);
+	error = idr_get_new(&st_index_idr, tpnt, &dev_num);
+	spin_unlock(&st_index_lock);
+	if (error) {
+		printk(KERN_WARNING "st: idr allocation failed: %d\n", error);
+		goto out_put_disk;
+	}
+
+	if (dev_num > ST_MAX_TAPES) {
+		printk(KERN_ERR "st: Too many tape devices (max. %d).\n",
+		       ST_MAX_TAPES);
+		goto out_put_index;
+	}
+
+	tpnt->index = dev_num;
+	sprintf(disk->disk_name, "st%d", dev_num);
+
+	dev_set_drvdata(dev, tpnt);
 
 	for (mode = 0; mode < ST_NBR_MODES; ++mode) {
 		STm = &(tpnt->modes[mode]);
@@ -4189,10 +4167,10 @@ static int st_probe(struct device *dev)
 	return 0;
 
 out_free_tape:
+	sysfs_remove_link(&tpnt->device->sdev_gendev.kobj,
+			  "tape");
 	for (mode=0; mode < ST_NBR_MODES; mode++) {
 		STm = &(tpnt->modes[mode]);
-		sysfs_remove_link(&tpnt->device->sdev_gendev.kobj,
-				  "tape");
 		for (j=0; j < 2; j++) {
 			if (STm->cdevs[j]) {
 				device_destroy(&st_sysfs_class,
@@ -4202,10 +4180,8 @@ out_free_tape:
 			}
 		}
 	}
-	write_lock(&st_dev_arr_lock);
-	scsi_tapes[dev_num] = NULL;
-	st_nr_dev--;
-	write_unlock(&st_dev_arr_lock);
+out_put_index:
+	idr_remove(&st_index_idr, dev_num);
 out_put_disk:
 	put_disk(disk);
 	kfree(tpnt);
@@ -4218,38 +4194,29 @@ out:
 
 static int st_remove(struct device *dev)
 {
-	struct scsi_device *SDp = to_scsi_device(dev);
-	struct scsi_tape *tpnt;
-	int i, j, mode;
-
-	scsi_autopm_get_device(SDp);
-	write_lock(&st_dev_arr_lock);
-	for (i = 0; i < st_dev_max; i++) {
-		tpnt = scsi_tapes[i];
-		if (tpnt != NULL && tpnt->device == SDp) {
-			scsi_tapes[i] = NULL;
-			st_nr_dev--;
-			write_unlock(&st_dev_arr_lock);
-			sysfs_remove_link(&tpnt->device->sdev_gendev.kobj,
-					  "tape");
-			for (mode = 0; mode < ST_NBR_MODES; ++mode) {
-				for (j=0; j < 2; j++) {
-					device_destroy(&st_sysfs_class,
-						       MKDEV(SCSI_TAPE_MAJOR,
-							     TAPE_MINOR(i, mode, j)));
-					cdev_del(tpnt->modes[mode].cdevs[j]);
-					tpnt->modes[mode].cdevs[j] = NULL;
-				}
-			}
+	struct scsi_tape *tpnt = dev_get_drvdata(dev);
+	int rew, mode;
+	dev_t cdev_devno;
+	struct cdev *cdev;
 
-			mutex_lock(&st_ref_mutex);
-			kref_put(&tpnt->kref, scsi_tape_release);
-			mutex_unlock(&st_ref_mutex);
-			return 0;
+	scsi_autopm_get_device(tpnt->device);
+
+	sysfs_remove_link(&tpnt->device->sdev_gendev.kobj, "tape");
+	for (mode = 0; mode < ST_NBR_MODES; ++mode) {
+		for (rew=0; rew < 2; rew++) {
+			cdev = tpnt->modes[mode].cdevs[rew];
+			if (!cdev)
+				continue;
+			cdev_devno = cdev->dev;
+			device_destroy(&st_sysfs_class, cdev_devno);
+			cdev_del(tpnt->modes[mode].cdevs[rew]);
+			tpnt->modes[mode].cdevs[rew] = NULL;
 		}
 	}
 
-	write_unlock(&st_dev_arr_lock);
+	mutex_lock(&st_ref_mutex);
+	kref_put(&tpnt->kref, scsi_tape_release);
+	mutex_unlock(&st_ref_mutex);
 	return 0;
 }
 
@@ -4336,7 +4303,6 @@ static void __exit exit_st(void)
 	unregister_chrdev_region(MKDEV(SCSI_TAPE_MAJOR, 0),
 				 ST_MAX_TAPE_ENTRIES);
 	class_unregister(&st_sysfs_class);
-	kfree(scsi_tapes);
 	printk(KERN_INFO "st: Unloaded.\n");
 }
 
@@ -4459,22 +4425,10 @@ static ssize_t
 options_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct st_modedef *STm = dev_get_drvdata(dev);
-	struct scsi_tape *STp;
-	int i, j, options;
+	struct scsi_tape *STp = STm->tape;
+	int options;
 	ssize_t l = 0;
 
-	for (i=0; i < st_dev_max; i++) {
-		for (j=0; j < ST_NBR_MODES; j++)
-			if (&scsi_tapes[i]->modes[j] == STm)
-				break;
-		if (j < ST_NBR_MODES)
-			break;
-	}
-	if (i == st_dev_max)
-		return 0;  /* should never happen */
-
-	STp = scsi_tapes[i];
-
 	options = STm->do_buffer_writes ? MT_ST_BUFFER_WRITES : 0;
 	options |= STm->do_async_writes ? MT_ST_ASYNC_WRITES : 0;
 	options |= STm->do_read_ahead ? MT_ST_READ_AHEAD : 0;
diff --git a/drivers/scsi/st.h b/drivers/scsi/st.h
index ea35632..4456735 100644
--- a/drivers/scsi/st.h
+++ b/drivers/scsi/st.h
@@ -66,6 +66,7 @@ struct st_modedef {
 	unsigned char default_compression;	/* 0 = don't touch, etc */
 	short default_density;	/* Forced density, -1 = no value */
 	int default_blksize;	/* Forced blocksize, -1 = no value */
+	struct scsi_tape *tape;
 	struct cdev *cdevs[2];  /* Auto-rewind and non-rewind devices */
 };
 
@@ -99,6 +100,7 @@ struct scsi_tape {
 	struct mutex lock;	/* For serialization */
 	struct completion wait;	/* For SCSI commands */
 	struct st_buffer *buffer;
+	int index;
 
 	/* Drive characteristics */
 	unsigned char omit_blklims;
-- 
1.7.9.2

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 3/5] st: get rid of scsi_tapes array
  2012-05-21 23:14 [PATCH v3 3/5] st: get rid of scsi_tapes array Lee Duncan
@ 2012-07-01  8:57 ` Kai Makisara
  2012-07-03  0:16   ` Lee Duncan
  0 siblings, 1 reply; 5+ messages in thread
From: Kai Makisara @ 2012-07-01  8:57 UTC (permalink / raw)
  To: Lee Duncan; +Cc: linux-scsi, linux-kernel, jeffm

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3410 bytes --]

On Mon, 21 May 2012, Lee Duncan wrote:

> From: Jeff Mahoney <jeffm@suse.com>
> 
> st currently allocates an array to store pointers to all of the
> scsi_tape objects. It's used to discover available indexes to use as the
> base for the minor number selection and later to look up scsi_tape
> devices for character devices.
> 
> We switch to using an IDR for minor selection and a pointer from
> st_modedef back to scsi_tape for the lookups.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> Signed-off-by: Lee Duncan <lduncan@suse.com>
> ---
>  drivers/scsi/st.c |  172 ++++++++++++++++++++---------------------------------
>  drivers/scsi/st.h |    2 +
>  2 files changed, 65 insertions(+), 109 deletions(-)
> 
... patch removed

I have finally had time to review and test this patch set. I am sorry this 
has taken so long.

I have found one change of behaviour and a theoretical problem:
The new code does not re-use the tape numbers when freed and re-scanned. 
The current code does re-use the freed numbers. Are there any reasons for 
this changed behaviour? (The theoretical problem is that the new code 
frees the tape structure but leaves the pointer in the idr tree.)

The patch at the end of this message (applies after the whole series) is 
an attempt to implement re-use of tape numbers. I am not completely sure 
that the change is correctly placed but it seems to work.

Another minor thing is that the documentation should be updated :-)

The patch at the end also updates the version code. I am not sure if the 
version code is useful, but it should be either updated or removed.

Otherwise no problems found. I am ready to ack the patch set after the 
re-use thing has been resolved (one way or another).

Thanks,
Kai

-----------------------------8<--------------------------------------------
--- linux-3.5-rc4/drivers/scsi/st.c.orig	2012-07-01 09:32:21.826666580 +0300
+++ linux-3.5-rc4/drivers/scsi/st.c	2012-07-01 11:43:49.154332920 +0300
@@ -17,7 +17,7 @@
    Last modified: 18-JAN-1998 Richard Gooch <rgooch@atnf.csiro.au> Devfs support
  */
 
-static const char *verstr = "20101219";
+static const char *verstr = "20120701";
 
 #include <linux/module.h>
 
@@ -4273,6 +4273,9 @@ static void scsi_tape_release(struct kre
 
 	disk->private_data = NULL;
 	put_disk(disk);
+	spin_lock(&st_index_lock);
+	idr_remove(&st_index_idr, tpnt->index);
+	spin_unlock(&st_index_lock);
 	kfree(tpnt);
 	return;
 }
--- linux-3.5-rc4/Documentation/scsi/st.txt.orig	2012-07-01 11:42:34.706607154 +0300
+++ linux-3.5-rc4/Documentation/scsi/st.txt	2012-07-01 11:41:27.542857135 +0300
@@ -2,7 +2,7 @@ This file contains brief information abo
 The driver is currently maintained by Kai Mäkisara (email
 Kai.Makisara@kolumbus.fi)
 
-Last modified: Sun Aug 29 18:25:47 2010 by kai.makisara
+Last modified: Sun Jul  1 11:41:27 2012 by kai.makisara
 
 
 BASICS
@@ -112,10 +112,8 @@ attempted).
 
 MINOR NUMBERS
 
-The tape driver currently supports 128 drives by default. This number
-can be increased by editing st.h and recompiling the driver if
-necessary. The upper limit is 2^17 drives if 4 modes for each drive
-are used.
+The upper limit is 2^17 drives if 4 modes for each drive are used. The
+tape driver currently supports drives up to this limit.
 
 The minor numbers consist of the following bit fields:
 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 3/5] st: get rid of scsi_tapes array
  2012-07-01  8:57 ` Kai Makisara
@ 2012-07-03  0:16   ` Lee Duncan
  2012-07-11 18:43     ` Lee Duncan
  0 siblings, 1 reply; 5+ messages in thread
From: Lee Duncan @ 2012-07-03  0:16 UTC (permalink / raw)
  To: Kai Makisara; +Cc: linux-scsi, linux-kernel, jeffm


On 07/01/2012 01:57 AM, Kai Makisara wrote:
> On Mon, 21 May 2012, Lee Duncan wrote:
> 
>> From: Jeff Mahoney <jeffm@suse.com>
>>
>> st currently allocates an array to store pointers to all of the
>> scsi_tape objects. It's used to discover available indexes to use as the
>> base for the minor number selection and later to look up scsi_tape
>> devices for character devices.
>>
>> We switch to using an IDR for minor selection and a pointer from
>> st_modedef back to scsi_tape for the lookups.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> Signed-off-by: Lee Duncan <lduncan@suse.com>
>> ---
>>  drivers/scsi/st.c |  172 ++++++++++++++++++++---------------------------------
>>  drivers/scsi/st.h |    2 +
>>  2 files changed, 65 insertions(+), 109 deletions(-)
>>
> ... patch removed
> 
> I have finally had time to review and test this patch set. I am sorry this 
> has taken so long.
> 
> I have found one change of behaviour and a theoretical problem:
> The new code does not re-use the tape numbers when freed and re-scanned. 
> The current code does re-use the freed numbers. Are there any reasons for 
> this changed behaviour? (The theoretical problem is that the new code 
> frees the tape structure but leaves the pointer in the idr tree.)
> 
> The patch at the end of this message (applies after the whole series) is 
> an attempt to implement re-use of tape numbers. I am not completely sure 
> that the change is correctly placed but it seems to work.


Thanks for the review, and good catch. I'll look over your added patch
and give feedback as soon as I can.

> 
> Another minor thing is that the documentation should be updated :-)

Of course.

> 
> The patch at the end also updates the version code. I am not sure if the 
> version code is useful, but it should be either updated or removed.
> 
> Otherwise no problems found. I am ready to ack the patch set after the 
> re-use thing has been resolved (one way or another).
> 
> Thanks,
> Kai
> 
-- 
The Lee-Man

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 3/5] st: get rid of scsi_tapes array
  2012-07-03  0:16   ` Lee Duncan
@ 2012-07-11 18:43     ` Lee Duncan
  2012-07-12 19:09       ` Jeff Mahoney
  0 siblings, 1 reply; 5+ messages in thread
From: Lee Duncan @ 2012-07-11 18:43 UTC (permalink / raw)
  To: Kai Makisara; +Cc: linux-scsi, linux-kernel, jeffm

Kai:

Your added patch looks great, and I see you fixed the documentation as
well. Thanks for your help.


On 07/02/2012 05:16 PM, Lee Duncan wrote:
> 
> On 07/01/2012 01:57 AM, Kai Makisara wrote:
>> On Mon, 21 May 2012, Lee Duncan wrote:
>>
>>> From: Jeff Mahoney <jeffm@suse.com>
>>>
>>> st currently allocates an array to store pointers to all of the
>>> scsi_tape objects. It's used to discover available indexes to use as the
>>> base for the minor number selection and later to look up scsi_tape
>>> devices for character devices.
>>>
>>> We switch to using an IDR for minor selection and a pointer from
>>> st_modedef back to scsi_tape for the lookups.
>>>
>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>>> Signed-off-by: Lee Duncan <lduncan@suse.com>
>>> ---
>>>  drivers/scsi/st.c |  172 ++++++++++++++++++++---------------------------------
>>>  drivers/scsi/st.h |    2 +
>>>  2 files changed, 65 insertions(+), 109 deletions(-)
>>>
>> ... patch removed
>>
>> I have finally had time to review and test this patch set. I am sorry this 
>> has taken so long.
>>
>> I have found one change of behaviour and a theoretical problem:
>> The new code does not re-use the tape numbers when freed and re-scanned. 
>> The current code does re-use the freed numbers. Are there any reasons for 
>> this changed behaviour? (The theoretical problem is that the new code 
>> frees the tape structure but leaves the pointer in the idr tree.)
>>
>> The patch at the end of this message (applies after the whole series) is 
>> an attempt to implement re-use of tape numbers. I am not completely sure 
>> that the change is correctly placed but it seems to work.
> 
> 
> Thanks for the review, and good catch. I'll look over your added patch
> and give feedback as soon as I can.
> 
>>
>> Another minor thing is that the documentation should be updated :-)
> 
> Of course.
> 
>>
>> The patch at the end also updates the version code. I am not sure if the 
>> version code is useful, but it should be either updated or removed.
>>
>> Otherwise no problems found. I am ready to ack the patch set after the 
>> re-use thing has been resolved (one way or another).
>>
>> Thanks,
>> Kai
>>

- Lee


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 3/5] st: get rid of scsi_tapes array
  2012-07-11 18:43     ` Lee Duncan
@ 2012-07-12 19:09       ` Jeff Mahoney
  0 siblings, 0 replies; 5+ messages in thread
From: Jeff Mahoney @ 2012-07-12 19:09 UTC (permalink / raw)
  To: Lee Duncan; +Cc: Kai Makisara, linux-scsi, linux-kernel

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 7/11/12 2:43 PM, Lee Duncan wrote:
> Kai:
> 
> Your added patch looks great, and I see you fixed the documentation
> as well. Thanks for your help.
> 

Actually, we've run into a hiccup here. Using cdev as the target of
the link doesn't work, the tape symlink isn't created, and we end up
with lsscsi not listing tape device nodes.

I'll work up a fix.

- -Jeff

> On 07/02/2012 05:16 PM, Lee Duncan wrote:
>> 
>> On 07/01/2012 01:57 AM, Kai Makisara wrote:
>>> On Mon, 21 May 2012, Lee Duncan wrote:
>>> 
>>>> From: Jeff Mahoney <jeffm@suse.com>
>>>> 
>>>> st currently allocates an array to store pointers to all of
>>>> the scsi_tape objects. It's used to discover available
>>>> indexes to use as the base for the minor number selection and
>>>> later to look up scsi_tape devices for character devices.
>>>> 
>>>> We switch to using an IDR for minor selection and a pointer
>>>> from st_modedef back to scsi_tape for the lookups.
>>>> 
>>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com> Signed-off-by:
>>>> Lee Duncan <lduncan@suse.com> --- drivers/scsi/st.c |  172
>>>> ++++++++++++++++++++--------------------------------- 
>>>> drivers/scsi/st.h |    2 + 2 files changed, 65 insertions(+),
>>>> 109 deletions(-)
>>>> 
>>> ... patch removed
>>> 
>>> I have finally had time to review and test this patch set. I am
>>> sorry this has taken so long.
>>> 
>>> I have found one change of behaviour and a theoretical
>>> problem: The new code does not re-use the tape numbers when
>>> freed and re-scanned. The current code does re-use the freed
>>> numbers. Are there any reasons for this changed behaviour? (The
>>> theoretical problem is that the new code frees the tape
>>> structure but leaves the pointer in the idr tree.)
>>> 
>>> The patch at the end of this message (applies after the whole
>>> series) is an attempt to implement re-use of tape numbers. I am
>>> not completely sure that the change is correctly placed but it
>>> seems to work.
>> 
>> 
>> Thanks for the review, and good catch. I'll look over your added
>> patch and give feedback as soon as I can.
>> 
>>> 
>>> Another minor thing is that the documentation should be updated
>>> :-)
>> 
>> Of course.
>> 
>>> 
>>> The patch at the end also updates the version code. I am not
>>> sure if the version code is useful, but it should be either
>>> updated or removed.
>>> 
>>> Otherwise no problems found. I am ready to ack the patch set
>>> after the re-use thing has been resolved (one way or another).
>>> 
>>> Thanks, Kai
>>> 
> 
> - Lee
> 


- -- 
Jeff Mahoney
SUSE Labs


-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.18 (Darwin)
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJP/yDqAAoJEB57S2MheeWyVQIQAK51+gbTWNkak42ZrJp9oyv4
6kBMkvD2/iJkcMFeg4dLxiIhuJKPVzqExBvRrk8Yvu4eWhvNBUt27KVtrMC3uzNn
utZhzZebp6qpUTwHp6cD+XJXtcHqyn04OpOxWq/oiRmJaffI/sgluLJKV7xQZpns
tV4vvhN00sGZ1CIJqJMj7g2dOk4k+kb6CXIQsKCIb/r25rLEH8f9YVAzo3jAJFrv
YPbyzC5OHDvghpiJZlm/H9Tp2VTovSjrITrGAp+g8fzXA4LKnx2Q+N5yfMF5lWaI
OXVyt97aGiYJRtxvdbTvtcf3CD7vsUVYqFdRVE8/5St1TsKQ9oqOJoKcb7jgTtmx
02rbZC8fsh0OTgyDlV+Ktja6knyWTHgVPqobUumx10wTpQYo1q00u/rqBXi3VEsS
swpbOcU20S+wetnBAkX50g+kCgfRskjj0nodDfCfGl/NgfDNe7sbAoc6NVDaaAGw
+h4pkyWsl8z0Uhj9Eju7Sbu6aAwjthxWTAKvz3TSuFffRmFf8Z55sc+UxouosXkX
hR7H/grUlk80J+rOyh+31Lx1707XcMSmnQSG0ZWePNOIukMkps0d3IXnRf6cHZ1/
0nnOxGfIQI/bo+WcDqhd2s5hlg9ZNCCe/JYoTj6jn040P63xxRK+soCNL0I+Ql4h
wnpeyXbDdDc6cBpp+jpf
=8KWj
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2012-07-12 19:09 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-21 23:14 [PATCH v3 3/5] st: get rid of scsi_tapes array Lee Duncan
2012-07-01  8:57 ` Kai Makisara
2012-07-03  0:16   ` Lee Duncan
2012-07-11 18:43     ` Lee Duncan
2012-07-12 19:09       ` Jeff Mahoney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).