[PATCH] SCSI, st: modify tape driver to allow writing immediate filemarks

[PATCH] SCSI, st: modify tape driver to allow writing immediate filemarks

[Lee Duncan]

Add an st module option st_nowait_eof which defaults to 0. Setting this
option to 1 tells the st driver not to wait when writing a filemark, which
can result in much faster times on streaming tape drives.

Legacy applications cannot take advantage of the newer MTWEOFI ioctl, so this
patch gives such applications the ability to write an immediate EOF using the
normal MTWEOF ioctl if they set st_nowait_eof=1.

Reference: https://bugzilla.novell.com/show_bug.cgi?id=688996

Signed-off-by: Lee Duncan <lduncan@suse.com>
--- 
 drivers/scsi/st.c |   18 +++++++++++++++---
 drivers/scsi/st.h |    1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 9b28f39..1dd617a 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -81,6 +81,7 @@ static int max_sg_segs;
 static int try_direct_io = TRY_DIRECT_IO;
 static int try_rdio = 1;
 static int try_wdio = 1;
+static int st_nowait_eof = 0;
 
 static int st_dev_max;
 static int st_nr_dev;
@@ -103,6 +104,8 @@ module_param_named(max_sg_segs, max_sg_segs, int, 0);
 MODULE_PARM_DESC(max_sg_segs, "Maximum number of scatter/gather segments to use (256)");
 module_param_named(try_direct_io, try_direct_io, int, 0);
 MODULE_PARM_DESC(try_direct_io, "Try direct I/O between user buffer and tape drive (1)");
+module_param_named(st_nowait_eof, st_nowait_eof, int, 0);
+MODULE_PARM_DESC(st_nowait_eof, "Do not wait when writing EOF (filemark) (0)");
 
 /* Extra parameters for testing */
 module_param_named(try_rdio, try_rdio, int, 0);
@@ -1106,6 +1109,11 @@ static int check_tape(struct scsi_tape *STp, struct file *filp)
                                     STp->drv_buffer));
 		}
 		STp->drv_write_prot = ((STp->buffer)->b_data[2] & 0x80) != 0;
+		if (!STp->drv_buffer && STp->immediate_filemark) {
+			printk(KERN_WARNING "%s: non-buffered tape: "
+			    "disabling writing immediate filemarks", name);
+			STp->immediate_filemark = 0;
+		}
 	}
 	st_release_request(SRpnt);
 	SRpnt = NULL;
@@ -1306,6 +1314,8 @@ static int st_flush(struct file *filp, fl_owner_t id)
 
 		memset(cmd, 0, MAX_COMMAND_SIZE);
 		cmd[0] = WRITE_FILEMARKS;
+		if (STp->immediate_filemark)
+			cmd[1] = 1;
 		cmd[4] = 1 + STp->two_fm;
 
 		SRpnt = st_do_scsi(NULL, STp, cmd, 0, DMA_NONE,
@@ -2172,8 +2182,8 @@ static void st_log_options(struct scsi_tape * STp, struct st_modedef * STm, char
 		       name, STm->defaults_for_writes, STp->omit_blklims, STp->can_partitions,
 		       STp->scsi2_logical);
 		printk(KERN_INFO
-		       "%s:    sysv: %d nowait: %d sili: %d\n", name, STm->sysv, STp->immediate,
-			STp->sili);
+		       "%s:    sysv: %d nowait: %d sili: %d nowait_filemark: %d\n",
+		       name, STm->sysv, STp->immediate, STp->sili, STp->immediate_filemark);
 		printk(KERN_INFO "%s:    debugging: %d\n",
 		       name, debugging);
 	}
@@ -2705,7 +2715,8 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 		cmd[0] = WRITE_FILEMARKS;
 		if (cmd_in == MTWSM)
 			cmd[1] = 2;
-		if (cmd_in == MTWEOFI)
+		if (cmd_in == MTWEOFI ||
+		    (cmd_in == MTWEOF && STp->immediate_filemark))
 			cmd[1] |= 1;
 		cmd[2] = (arg >> 16);
 		cmd[3] = (arg >> 8);
@@ -4084,6 +4095,7 @@ static int st_probe(struct device *dev)
 	tpnt->scsi2_logical = ST_SCSI2LOGICAL;
 	tpnt->sili = ST_SILI;
 	tpnt->immediate = ST_NOWAIT;
+	tpnt->immediate_filemark = st_nowait_eof;
 	tpnt->default_drvbuffer = 0xff;		/* No forced buffering */
 	tpnt->partition = 0;
 	tpnt->new_partition = 0;
diff --git a/drivers/scsi/st.h b/drivers/scsi/st.h
index f91a67c..24f97f1 100644
--- a/drivers/scsi/st.h
+++ b/drivers/scsi/st.h
@@ -120,6 +120,7 @@ struct scsi_tape {
 	unsigned char c_algo;			/* compression algorithm */
 	unsigned char pos_unknown;			/* after reset position unknown */
 	unsigned char sili;			/* use SILI when reading in variable b mode */
+	unsigned char immediate_filemark;	/* set immediate bit when writing filemark */
 	int tape_type;
 	int long_timeout;	/* timeout for commands known to take long time */
 

Re: [PATCH] SCSI, st: modify tape driver to allow writing immediate filemarks

[Joe Perches]

On Tue, 2012-02-07 at 17:07 -0800, Lee Duncan wrote:
> Add an st module option st_nowait_eof which defaults to 0. Setting this
> option to 1 tells the st driver not to wait when writing a filemark, which
> can result in much faster times on streaming tape drives.

trivial comments:

> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
[]
> @@ -81,6 +81,7 @@ static int max_sg_segs;
>  static int try_direct_io = TRY_DIRECT_IO;
>  static int try_rdio = 1;
>  static int try_wdio = 1;
> +static int st_nowait_eof = 0;

Could be bool.

> @@ -1106,6 +1109,11 @@ static int check_tape(struct scsi_tape *STp, struct file *filp)
>                                      STp->drv_buffer));
>  		}
>  		STp->drv_write_prot = ((STp->buffer)->b_data[2] & 0x80) != 0;
> +		if (!STp->drv_buffer && STp->immediate_filemark) {
> +			printk(KERN_WARNING "%s: non-buffered tape: "
> +			    "disabling writing immediate filemarks", name);

Needs terminating "\n" and it's better to coalesce the
format and ignore 80 char lines to make arbitrary greps
a bit easier.

> diff --git a/drivers/scsi/st.h b/drivers/scsi/st.h
[]
> @@ -120,6 +120,7 @@ struct scsi_tape {
>  	unsigned char c_algo;			/* compression algorithm */
>  	unsigned char pos_unknown;			/* after reset position unknown */
>  	unsigned char sili;			/* use SILI when reading in variable b mode */
> +	unsigned char immediate_filemark;	/* set immediate bit when writing filemark */

could be bool

Re: [PATCH] SCSI, st: modify tape driver to allow writing immediate filemarks

[Kai Makisara]

On Tue, 7 Feb 2012, Lee Duncan wrote:

> Add an st module option st_nowait_eof which defaults to 0. Setting this
> option to 1 tells the st driver not to wait when writing a filemark, which
> can result in much faster times on streaming tape drives.
> 
> Legacy applications cannot take advantage of the newer MTWEOFI ioctl, so this
> patch gives such applications the ability to write an immediate EOF using the
> normal MTWEOF ioctl if they set st_nowait_eof=1.
> 
> Reference: https://bugzilla.novell.com/show_bug.cgi?id=688996
> 
Is there a real application? I can't open your reference. (Yes, I know 
that this feature can speed up writing dramatically in some cases, but is 
there a case with legacy applications?)

Anyway, I don't think this should be implemented as a pure module option. 
The standard semantics specify that MTWEOF is a synchronization point and 
this module option breaks that for all users.

The driver supports several tape device files with different properties 
that can be set at run-time. Why not implement this as one of the mode 
options? This would allow the "normal" users to use a device file with 
synchronizing MTWEOF and the users needing unsynchronizing MTWEOF would 
use another device file.

The st driver exports the options in sysfs. This is important so that the 
users can check what the options for a device are. This new option should 
also be exported.

Kai

Re: [PATCH] SCSI, st: modify tape driver to allow writing immediate filemarks

[Lee Duncan]

Hi Joe:

Thanks for the quick review.

On 02/07/2012 05:15 PM, Joe Perches wrote:
> On Tue, 2012-02-07 at 17:07 -0800, Lee Duncan wrote:
>> Add an st module option st_nowait_eof which defaults to 0. Setting this
>> option to 1 tells the st driver not to wait when writing a filemark, which
>> can result in much faster times on streaming tape drives.
> 
> trivial comments:
> 
>> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> []
>> @@ -81,6 +81,7 @@ static int max_sg_segs;
>>  static int try_direct_io = TRY_DIRECT_IO;
>>  static int try_rdio = 1;
>>  static int try_wdio = 1;
>> +static int st_nowait_eof = 0;
> 
> Could be bool.

In both this case and the one you site below, the existing variables are
not bool. I'd rather see new variables match existing variables, in
naming, type, and usage. Next major update of the driver, I'd like to
see all of these bools-in-use fixed.

> 
>> @@ -1106,6 +1109,11 @@ static int check_tape(struct scsi_tape *STp, struct file *filp)
>>                                      STp->drv_buffer));
>>  		}
>>  		STp->drv_write_prot = ((STp->buffer)->b_data[2] & 0x80) != 0;
>> +		if (!STp->drv_buffer && STp->immediate_filemark) {
>> +			printk(KERN_WARNING "%s: non-buffered tape: "
>> +			    "disabling writing immediate filemarks", name);
> 
> Needs terminating "\n" and it's better to coalesce the
> format and ignore 80 char lines to make arbitrary greps
> a bit easier.

Good catch. Thanks.

-- 
Lee Duncan

Re: [PATCH] SCSI, st: modify tape driver to allow writing immediate filemarks

[Lee Duncan]

On 02/08/2012 09:19 AM, Kai Makisara wrote:
> On Tue, 7 Feb 2012, Lee Duncan wrote:
> 
>> Add an st module option st_nowait_eof which defaults to 0. Setting this
>> option to 1 tells the st driver not to wait when writing a filemark, which
>> can result in much faster times on streaming tape drives.
>>
>> Legacy applications cannot take advantage of the newer MTWEOFI ioctl, so this
>> patch gives such applications the ability to write an immediate EOF using the
>> normal MTWEOF ioctl if they set st_nowait_eof=1.
>>
>> Reference: https://bugzilla.novell.com/show_bug.cgi?id=688996
>>
> Is there a real application? I can't open your reference. (Yes, I know 
> that this feature can speed up writing dramatically in some cases, but is 
> there a case with legacy applications?)

Yes, there are real applications. My apologies for supplying a Reference
to a non-public bug listing. This bug appeared because SAN backup
software was writing to an LT04 tape drive, but performance sucked.

I was able to reproduce the problem with a simple program that wrote a
megabyte then an EOF to my LT04, then a megabyte then another EOF. Any
tape backup or copy program that uses this basic pattern will exhibit
this problem.

> 
> Anyway, I don't think this should be implemented as a pure module option. 
> The standard semantics specify that MTWEOF is a synchronization point and 
> this module option breaks that for all users.

The Standard specifies that writing a filemark (i.e. an EOF) is a
synchronization point if and only if the data is being written in
immediate mode. In other words, you are not supposed to set both
immediate writes and immediate EOFs. This is why I didn't overload the
already-existing "immediate" flag in the driver.

I will add a check to the driver to disallow immediate EOFs if immediate
writes are requested.

> 
> The driver supports several tape device files with different properties 
> that can be set at run-time. Why not implement this as one of the mode 
> options? This would allow the "normal" users to use a device file with 
> synchronizing MTWEOF and the users needing unsynchronizing MTWEOF would 
> use another device file.
> 
> The st driver exports the options in sysfs. This is important so that the 
> users can check what the options for a device are. This new option should 
> also be exported.
> 
> Kai

Thanks, Kai. I agree with this. I will update my patch and resubmit.
-- 
Lee Duncan

Re: [PATCH] SCSI, st: modify tape driver to allow writing immediate filemarks

[Kai Makisara]

On Thu, 9 Feb 2012, Lee Duncan wrote:

> On 02/08/2012 09:19 AM, Kai Makisara wrote:
> > On Tue, 7 Feb 2012, Lee Duncan wrote:
> > 
> >> Add an st module option st_nowait_eof which defaults to 0. Setting this
> >> option to 1 tells the st driver not to wait when writing a filemark, which
> >> can result in much faster times on streaming tape drives.
> >>
> >> Legacy applications cannot take advantage of the newer MTWEOFI ioctl, so this
> >> patch gives such applications the ability to write an immediate EOF using the
> >> normal MTWEOF ioctl if they set st_nowait_eof=1.
> >>
> >> Reference: https://bugzilla.novell.com/show_bug.cgi?id=688996
> >>
...
> > 
> > Anyway, I don't think this should be implemented as a pure module option. 
> > The standard semantics specify that MTWEOF is a synchronization point and 
> > this module option breaks that for all users.
> 
> The Standard specifies that writing a filemark (i.e. an EOF) is a
> synchronization point if and only if the data is being written in
> immediate mode. In other words, you are not supposed to set both
> immediate writes and immediate EOFs. This is why I didn't overload the
> already-existing "immediate" flag in the driver.
> 
Not quite what I mean. The following is from SSC-3 draft:
-
An IMMED bit of zero specifies the device server shall not return status 
until the write operation has completed. Any
buffered logical objects shall be written to the medium prior to 
completing the command.
-
So, what I mean is that all data in the drive buffer has been written to 
tape. This WRITE FILEMARKS command then returns any write errors that 
happen while the buffered data is written to tape. With immediate bit set, 
you may miss write errors.

> I will add a check to the driver to disallow immediate EOFs if immediate
> writes are requested.
> 
This may not be necessary. If someone wants to deliberately take risks, 
the driver should not prevent this ;-)

Kai