Re: [PATCH v3 3/5] st: get rid of scsi_tapes array

From: Kai Makisara
Date: Sun Jul 01 2012 - 05:06:43 EST


On Mon, 21 May 2012, Lee Duncan wrote:

> From: Jeff Mahoney <jeffm@xxxxxxxx>
>
> 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@xxxxxxxx>
> Signed-off-by: Lee Duncan <lduncan@xxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxx)

-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: