[PATCH] SCSI tape: remove use of file_count(), was Re: [patchset] Lockfree fd lookup 0 of 5

[PATCH] SCSI tape: remove use of file_count(), was Re: [patchset] Lockfree fd lookup 0 of 5

[Kai Makisara]

On Tue, 30 Nov 2004, Ravikiran G Thirumalai wrote:

> Kai, can you mail the latest version of this patch to lkml?  Willem, can
> you make a patch similar to this for osst.c and mail it to lkml?
> 
OK. The patch at the end of this message modifies st so that file_count() 
is not used. It has been used to find the last call to the st_flush() 
method for possible writing of the final filemark and rewinding the tape. 
While these have been in st_flush(), it has been possible to return errors 
in the return value of close(). The patch moves these operations to st_release() 
and logs the possible errors since it is not possible to return these 
errors to the user any more. Flushing the tape drive buffer is added to  
st_flush() to minimise the possibility of errors not reported at close().

The patch is against 2.6.10-rc2-bk13, it compiles, and is lightly tested. 
It should not be applied unless file_count() really is removed.

Signed-off-by: Kai Makisara <kai.makisara@kolumbus.fi>

-- 
Kai

--- linux-2.6.10-rc2-bk13/drivers/scsi/st.c	2004-11-30 20:43:30.000000000 +0200
+++ linux-2.6.10-rc2-bk13-k1/drivers/scsi/st.c	2004-11-30 21:20:17.000000000 +0200
@@ -17,7 +17,7 @@
    Last modified: 18-JAN-1998 Richard Gooch <rgooch@atnf.csiro.au> Devfs support
  */
 
-static char *verstr = "20041025";
+static char *verstr = "20041130";
 
 #include <linux/module.h>
 
@@ -1078,20 +1078,21 @@
 static int st_flush(struct file *filp)
 {
 	int result = 0, result2;
-	unsigned char cmd[MAX_COMMAND_SIZE];
-	struct scsi_request *SRpnt;
 	struct scsi_tape *STp = filp->private_data;
-	struct st_modedef *STm = &(STp->modes[STp->current_mode]);
 	struct st_partstat *STps = &(STp->ps[STp->partition]);
-	char *name = tape_name(STp);
+	DEB(char *name = tape_name(STp));
 
-	if (file_count(filp) > 1)
-		return 0;
+	if (down_interruptible(&STp->lock))
+		return -ERESTARTSYS;
 
 	if (STps->rw == ST_WRITING && !STp->pos_unknown) {
 		result = flush_write_buffer(STp);
 		if (result != 0 && result != (-ENOSPC))
 			goto out;
+		/* Flush to tape */
+		result = st_int_ioctl(STp, MTWEOF, 0);
+		if (result)
+			goto out;
 	}
 
 	if (STp->can_partitions &&
@@ -1100,9 +1101,27 @@
                                "%s: switch_partition at close failed.\n", name));
 		if (result == 0)
 			result = result2;
-		goto out;
 	}
 
+ out:
+	up(&STp->lock);
+	return result;
+}
+
+
+/* Close the device and release it. BKL is not needed: this is the only thread
+   accessing this tape. STp->pos_unknown is set if to block further access if
+   tape re-positioning fails. */
+static int st_release(struct inode *inode, struct file *filp)
+{
+	int result = 0, result2;
+	unsigned char cmd[MAX_COMMAND_SIZE];
+	struct scsi_tape *STp = filp->private_data;
+	struct st_modedef *STm = &(STp->modes[STp->current_mode]);
+	struct st_partstat *STps = &(STp->ps[STp->partition]);
+	struct scsi_request *SRpnt;
+	char *name = tape_name(STp);
+
 	DEBC( if (STp->nbr_requests)
 		printk(KERN_WARNING "%s: Number of r/w requests %d, dio used in %d, pages %d (%d).\n",
 		       name, STp->nbr_requests, STp->nbr_dio, STp->nbr_pages, STp->nbr_combinable));
@@ -1175,24 +1194,22 @@
 			STps->drv_block = 0;
 			STps->eof = ST_FM;
 		}
+		if (result) {
+			printk(KERN_ERR "%s: Final tape positioning failed.\n", name);
+			STp->pos_unknown = 1;
+		}
 	}
 
-      out:
+out:
 	if (STp->rew_at_close) {
 		result2 = st_int_ioctl(STp, MTREW, 1);
+		if (result2) {
+			printk(KERN_ERR "%s: Auto-rewind failed.\n", name);
+			STp->pos_unknown = 1;
+		}
 		if (result == 0)
 			result = result2;
 	}
-	return result;
-}
-
-
-/* Close the device and release it. BKL is not needed: this is the only thread
-   accessing this tape. */
-static int st_release(struct inode *inode, struct file *filp)
-{
-	int result = 0;
-	struct scsi_tape *STp = filp->private_data;
 
 	if (STp->door_locked == ST_LOCKED_AUTO)
 		do_door_lock(STp, 0);

Re: [PATCH] SCSI tape: remove use of file_count(), was Re: [patchset] Lockfree fd lookup 0 of 5

[Willem Riede]

On 11/30/2004 03:30:26 PM, Kai Makisara wrote:
> On Tue, 30 Nov 2004, Ravikiran G Thirumalai wrote:
> 
> > Kai, can you mail the latest version of this patch to lkml?  Willem, can
> > you make a patch similar to this for osst.c and mail it to lkml?
> > 
> OK. The patch at the end of this message modifies st so that file_count() 
> is not used. It has been used to find the last call to the st_flush() 
> method for possible writing of the final filemark and rewinding the tape. 
> While these have been in st_flush(), it has been possible to return errors 
> in the return value of close(). The patch moves these operations to
> st_release() 
> and logs the possible errors since it is not possible to return these 
> errors to the user any more. Flushing the tape drive buffer is added to  
> st_flush() to minimise the possibility of errors not reported at close().
> 
> The patch is against 2.6.10-rc2-bk13, it compiles, and is lightly tested. 
> It should not be applied unless file_count() really is removed.

Here is the equivalent patch for osst.c -- the same considerations apply.
It has been compiled, and tested, and works for me.

Signed-off-by: Willem Riede <osst@riede.org>

--- /home/wriede/kernel/linux-2.6.10-rc2/drivers/scsi/osst.c	2004-12-01
19:31:49.000000000 -0500
+++ ./osst.c	2004-12-02 18:51:55.000000000 -0500
@@ -4590,22 +4590,38 @@
 {
 	int            result = 0, result2;
 	OS_Scsi_Tape * STp  = filp->private_data;
-	struct st_modedef      * STm  = &(STp->modes[STp->current_mode]);
-	struct st_partstat  * STps = &(STp->ps[STp->partition]);
 	Scsi_Request * SRpnt = NULL;
-	char         * name = tape_name(STp);
 
-	if (file_count(filp) > 1)
-		return 0;
+	if (down_interruptible(&STp->lock))
+		return (-ERESTARTSYS);
 
-	if ((STps->rw == ST_WRITING || STp->dirty) && !STp->pos_unknown) {
+	if ((STp->ps[STp->partition].rw == ST_WRITING || STp->dirty) &&
!STp->pos_unknown) {
 		STp->write_type = OS_WRITE_DATA;
 		result = osst_flush_write_buffer(STp, &SRpnt);
 		if (result != 0 && result != (-ENOSPC))
 			goto out;
+		/* wait for writing to complete */
+		result2 = osst_flush_drive_buffer(STp, &SRpnt);
+		if (result == 0)
+			result = result2;
 	}
-	if ( STps->rw >= ST_WRITING && !STp->pos_unknown) {
+out:
+	up(&STp->lock);
+	return result;
+}
 
+
+/* Close the device and release it */
+static int os_scsi_tape_close(struct inode * inode, struct file * filp)
+{
+	int                 result = 0, result2;
+	OS_Scsi_Tape       * STp   = filp->private_data;
+	struct st_modedef  * STm   = &(STp->modes[STp->current_mode]);
+	struct st_partstat * STps  = &(STp->ps[STp->partition]);
+	Scsi_Request       * SRpnt = NULL;
+	char               * name  = tape_name(STp);
+
+	if ( STps->rw >= ST_WRITING && !STp->pos_unknown) {
 #if DEBUG
 		if (debugging) {
 			printk(OSST_DEB_MSG "%s:D: File length %ld bytes.\n",
@@ -4620,6 +4636,10 @@
 			printk(OSST_DEB_MSG "%s:D: Buffer flushed, %d EOF(s)
written\n",
 					       name, 1+STp->two_fm);
 #endif
+                if (result) {
+                        printk(KERN_ERR "%s: Writing closing filemark
failed.\n", name);
+                        STp->pos_unknown = 1;
+                }
 	}
 	else if (!STp->rew_at_close) {
 		STps = &(STp->ps[STp->partition]);
@@ -4646,11 +4666,17 @@
 			STps->drv_block = 0;
 			STps->eof = ST_FM;
 		}
+		if (result) {
+			printk(KERN_ERR "%s: Final tape positioning
failed.\n", name);
+			STp->pos_unknown = 1;
+		}
 	}
-
-out:
 	if (STp->rew_at_close) {
 		result2 = osst_position_tape_and_confirm(STp, &SRpnt,
STp->first_data_ppos);
+		if (result2 < 0) {
+			printk(KERN_ERR "%s: Auto-rewind failed.\n", name);
+			STp->pos_unknown = 1;
+		}
 		STps->drv_file = STps->drv_block = STp->frame_seq_number =
STp->logical_blk_num = 0;
 		if (result == 0 && result2 < 0)
 			result = result2;
@@ -4669,19 +4695,6 @@
 	STp->write_count = 0;
 	STp->read_count  = 0;
 
-	return result;
-}
-
-
-/* Close the device and release it */
-static int os_scsi_tape_close(struct inode * inode, struct file * filp)
-{
-	int result = 0;
-	OS_Scsi_Tape * STp = filp->private_data;
-	Scsi_Request * SRpnt = NULL;
-
-	if (SRpnt) scsi_release_request(SRpnt);
-
 	if (STp->door_locked == ST_LOCKED_AUTO)
 		do_door_lock(STp, 0);