Re: [PATCH] st: convert to using driver attr groups for sysfs
> On 23.6.2015, at 11.11, Seymour, Shane M wrote: > > This patch changes the st driver to use attribute groups so > driver sysfs files are created automatically. See the > following for reference: > > http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/ > Acked-by: Kai Mäkisara Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO
> On 24.6.2015, at 9.54, Seymour, Shane M wrote: > > > Convert DRIVER_ATTR macros to DRIVER_ATTR_RO requested by > Greg KH. Also switched to using scnprintf instead of snprintf > per Documentation/filesystems/sysfs.txt. > > Suggested-by: Greg Kroah-Hartman > Signed-off-by: Shane Seymour > --- > This patch was implemented on top of the previous patch to > convert to using driver attr groups. > > Changes from v1: > - switched to scnprintf from sprintf after feedback from Sergey > Senozhatsky. Acked-by: Kai Mäkisara Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: null pointer dereference panic caused by use after kref_put by st_open
> On 2.7.2015, at 15.01, Seymour, Shane M wrote: > > > Two SLES11 SP3 servers encountered similar crashes simultaneously > following some kind of SAN/tape target issue: > ... > The crash is fixed by reordering the code so we no longer access > the struct scsi_tape after the kref_put() is done on it in st_open(). > > Signed-off-by: Shane Seymour > Signed-off-by: Darren Lavender Acked-by: Kai Mäkisara Thanks for finding this. No code should touch an object after put(). Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: Destroy st_index_idr on module exit
> On 8.7.2015, at 18.24, Johannes Thumshirn wrote: > > Destroy st_index_idr on module exit, reclaiming the allocated memory. > > This was detected by the following semantic patch (written by Luis Rodriguez > ) > ... > > Signed-off-by: Johannes Thumshirn Acked-by: Kai Mäkisara Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: allow debug output to be enabled or disabled via sysfs
> On 12.10.2015, at 7.31, Seymour, Shane M wrote: > > > Change st driver to allow enabling or disabling debug output > via sysfs file /sys/bus/scsi/drivers/st/debug_flag. > > Previously the only way to enable debug output was: > > 1. loading the driver with the module parameter debug_flag=1 > 2. an ioctl call (this method was also the only way to dynamically > disable debug output). > > To use the ioctl you need a second tape drive (if you are > actively testing the first tape drive) since a second process > cannot open the first tape drive if it is in use. > Good justification for the feature. Acked-by: Kai Mäkisara Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning
> On 28.1.2016, at 9.36, Seymour, Shane M wrote: > > Hi Kai, > > With the changes the I get a failure partitioning a HP DAT72 drive (DDS-5): > > # ./mt -f /dev/st1 stsetoption debug > # ./mt -f /dev/st1 stsetoption can-partitions > # ./mt -f /dev/st1 mkpartition 1000 > /dev/st1: Input/output error > ... > [ 3976.389605] st 6:0:3:0: [st1] Partition page length is 10 bytes. > [ 3976.389610] st 6:0:3:0: [st1] PP: max 1, add 0, xdp 0, psum 02, pofmetc 0, > rec 03, units 00, sizes: 0 65535 > [ 3976.389614] st 6:0:3:0: [st1] MP: 11 08 01 00 10 03 00 00 00 00 ff ff > [ 3976.389618] st 6:0:3:0: [st1] psd_cnt 2, max.parts 1, nbr_parts 0 ^ The problem is here ... > Using a slightly older kernel to partition the DAT72 drive works (same 3 > commands as above): ... > [ 351.584906] st 6:0:3:0: [st1] Partition page length is 10 bytes. > [ 351.584908] st 6:0:3:0: [st1] psd_cnt 1, max.parts 1, nbr_parts 0 The old driver computes the psd_cnt from the returned page length. The same applies to the patched driver if the SCSI level of the device < SCSI_3. This works correctly with my drive that reports SCSI_2. So, the question is: what SCSI level does your device report? Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning
> On 27.1.2016, at 1.35, Seymour, Shane M wrote: > > Hi Emmanuel, > >> Hmm in fact if we keep using MB we'll be stuck when tapes reach ~2 PB >> which leaves some time to think about it, until LTO-15 circa 2036 :) > > There will be other issues to solve before then (by LTO-9 2 with compression > or LTO-10 without compression and we're at LTO-7 now). Take tar format > archives with a standard block size of 10k can take this much data to get > to 2^32 blocks and cause the current 32bit block number to wrap: > > 43,980,465,111,040 (2^32 * 10240) > This is a somewhat theoretic example. In the 1990s no reasonable person used block size below 32 kB. Now the limit is probably higher. But, eventually we have to enlarge the block position and count variables. > After that much data has been written the SCSI-2 command READ POSITION > will not be able to show the current position correctly (which is what the st > driver uses to determine the position for an MTIOCPOS). It may be less > than that since some drives include file marks in the logical block number if > the program that produced the tape writes them out. > > That means switching to the extended block id form of READ POSITION > so we have 64bit counts for those values, see page 150: > > https://docs.oracle.com/cd/E21419_04/en/LTO5_Vol3_E5b/LTO5_Vol3_E5b.pdf > > That's going to require new ioctls like MTIOCPOS64 and other changes within > the driver to support larger types for holding some values. That will also > raise > all sorts of fun compatibility questions as well (should MTIOCPOS work at all > for such a tape drive or should it work until something overflows and return > what data it can and give an errno of -EOVERFLOW etc). > I think we should support MTIOCPOS as far as we can. There is no point to make existing software unusable for people who happen to buy a modern drive. Note also that the block number given to MTIOCPOS is long, i.e., 64 bits in the important architectures ;-) (The argument to MTSEEK is int, though.) > That's probably the correct time to also look at adding support for more > partitions. Not sure when the extended block id form of READ POSITION > got added but it may mean only supporting the wider values only with tape > drives that support REPORT SUPPORTED OPCODES (if that can indicate that > it supports READ POSITION with extended block ids and anything else > required to support block numbers larger than 2^32). > The current driver supports using up to ST_NBR_PARTITIONS (in the source set to 4). Support for using partitions must be in the driver. I am still not sure if partitioning the tape should be in the driver. > The 0x91 version of SPACE needs to be used as well (the 32bit version 0x11 > Is currently used) to allow tape movement with counts >2^32. That requires > a new ioctl call. I haven't looked at what else may need to change but there's > likely to be more. The alternate version of SPACE is from page 220 of the > above HP LTO5 tape reference. > The first step is to make the internal counters 64 bits. Then the code should be changed so that it can handle the large addresses. Note that this is necessary for handling partition changes even if no positioning commands are exposed to the user. The last step is to introduce the new positioning methods. The new positioning methods should perhaps be defined so that both the partition number and the block number are specified together. A proper tape address needs both. But I think we should first fix the existing partitioning command so that it works for current drives. Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning
> On 28.1.2016, at 21.21, Laurence Oberman wrote: > > Hi Kai > > What kernel was the last patch you attached against. > It was against the latest git version from Jan 24 evening (Finnish time). It is 4.4.0 plus from 4.5 merge window. The patch applies to 3.18.25 with offsets and should apply to anything between these. Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning
> On 1.2.2016, at 8.31, Seymour, Shane M wrote:
>
> Hi Kai,
>
> Thanks for the changes the HPE DAT72 DDS5 drive now works as expected:
>
Good. Thanks for testing.
...
>
> I'm asking around again one final time to see if I can lay my hands on a LTO5
> or greater drive so I can test LTO partitioning as well.
>
> The only other thing I can think of (I'm not sure if this is an improvement
> or not) is if bp[pgo + PP_OFF_MAX_ADD_PARTS] + bp[pgo + PP_OFF_NBR_ADD_PARTS]
> (max.parts and nbr_parts in the debug message) is zero just return -EINVAL
> unless you know of any take drives that report them both as 0 but can be
> partitioned? That is after this:
>
>DEBC_printk(STp, "psd_cnt %d, max.parts %d, nbr_parts %d\n",
>psd_cnt, bp[pgo + PP_OFF_MAX_ADD_PARTS],
>bp[pgo + PP_OFF_NBR_ADD_PARTS]);
>
> add (and also turn off the can-partitions option):
>
> if ((bp[pgo + PP_OFF_MAX_ADD_PARTS] + bp[pgo + PP_OFF_NBR_ADD_PARTS])
> == 0) {
> DEBC_printk(STp, "Drive not partitionable - max.parts+nbr_parts
> is 0\n");
> STp->can_partitions = 0;
> return -EINVAL;
> }
>
> I'm not especially fussed if you don't want to add that though.
>
I thought about a test like this (only test maximum number) but decided not to
add it. The reason was that
I did not want to change anything that has worked before. I quite trust that
the current drives return sense
data instead of crashing and the end result for the user would be the same.
However, one can argue that
returning EINVAL is better than EIO but does the user notice? If the common
opinion is that a test like this
should be added, I am not against it. It can be added to the code for SCSI >=3
where it does not risk
anything for the old drives.
IMHO, can_partitions should not be cleared based on the test. For example,
trying to partition a LTO-4 tape
in a LTO-5 drive should not disable partitioning. (The mode page should return
zero as maximum number of
partitions when a LTO-4 tape is inserted.)
Thanks,
Kai
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning
> On 4.2.2016, at 3.43, Seymour, Shane M wrote:
>
> Hi Kai,
>
> Tested with patched kernel 4.5.0-rc2-next-20160202+. It's looking good
> everything partition related passed with DDS5 and LTO6. You can definitely
> add me as a tested-by. I did find one issue below but it's not related to the
> partitioning changes.
>
Thanks for testing. It would be interesting to get confirmation from a LTO-5
user that partitioning
works. Even without that I will make the final patch within a few days (remove
some debugging
and update the documentation).
...
> I did find one issue in testing unrelated to the changes, the tell option
> didn't work with my LTO-6 drive:
>
> # ./mt -f /dev/st0 tell
> /dev/st0: Input/output error
>
> [ 2045.974642] st 3:0:0:0: [st0] Block limits 1 - 16777215 bytes.
> [ 2045.975221] st 3:0:0:0: [st0] Mode sense. Length 11, medium 0, WBS 10, BLL
> 8
> [ 2045.975224] st 3:0:0:0: [st0] Density 5a, tape length: 0, drv buffer: 1
> [ 2045.975226] st 3:0:0:0: [st0] Block size: 0, buffer size: 4096 (1 blocks).
> [ 2045.975718] st 3:0:0:0: [st0] Error: 802, cmd: 34 1 0 0 0 0
> [ 2045.975723] st 3:0:0:0: [st0] Sense Key : Illegal Request [current]
> [ 2045.975726] st 3:0:0:0: [st0] Add. Sense: Invalid field in cdb
> [ 2045.975729] st 3:0:0:0: [st0] Can't read tape position.
> [ 2045.975857] st 3:0:0:0: [st0] Rewinding tape.
>
> I believe that in get_location() we're doing this:
>
> static int get_location(struct scsi_tape *STp, unsigned int *block, int
> *partition,
>int logical)
> {
>int result;
>unsigned char scmd[MAX_COMMAND_SIZE];
>struct st_request *SRpnt;
>
>if (STp->ready != ST_READY)
>return (-EIO);
>
>memset(scmd, 0, MAX_COMMAND_SIZE);
>if ((STp->device)->scsi_level < SCSI_2) {
>scmd[0] = QFA_REQUEST_BLOCK;
>scmd[4] = 3;
>} else {
>scmd[0] = READ_POSITION;
>if (!logical && !STp->scsi2_logical)
>scmd[1] = 1; <<
>}
>
> When called from the ioctl that the tell option uses the variable logical is
> passed in as 0 (from what I could see everything else sets it to 1). For a
> READ_POSITION the drive I'm using only supports 0, 6, or 8 in the service
> action field of the second byte:
>
I think you have not set the scsi2_logical option bit with mt or stinit or some
other tool.
The default of device-specific addresses is a historical mistake but we have to
live with
it. I don’t see this as a big problem because any user of current drives should
enable
some driver options anyway.
Thanks,
Kai
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: trivial: remove form feed characters
> On 4.11.2015, at 11.52, Maurizio Lombardi wrote: > > Signed-off-by: Maurizio Lombardi > --- > drivers/scsi/st.c | 24 > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c > index b37b9b0..7c4e518 100644 > --- a/drivers/scsi/st.c > +++ b/drivers/scsi/st.c > @@ -226,7 +226,6 @@ static DEFINE_SPINLOCK(st_use_lock); > static DEFINE_IDR(st_index_idr); > > > - What’s the point? Is there an “official” rule that form feeds are not allowed (to put different things to different pages in printout)? Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [SCSI] st: Remove obsolete scsi_tape.max_pfn
> On 15.11.2015, at 13.48, Geert Uytterhoeven wrote:
>
> Its last user was removed 10 years ago, in commit
> 8b05b773b6030de5 ("[SCSI] convert st to use scsi_execute_async").
>
> Signed-off-by: Geert Uytterhoeven
Acked-by: Kai Mäkisara
Thanks,
Kai
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] st: fix potential null pointer dereference.
> On 18.11.2015, at 16.32, Maurizio Lombardi wrote: > > If cdev_add() returns an error, the code calls > cdev_del() passing the STm->cdevs[rew] pointer as parameter; > the problem is that the pointer has not been initialized yet. > > This patch fixes the problem by moving the STm->cdevs[rew] pointer > initialization before the call to cdev_add(). > It also sets STm->devs[rew] and STm->cdevs[rew] to NULL in > case of failure. > > Signed-off-by: Maurizio Lombardi Acked-by: Kai Mäkisara Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: st driver doesn't seem to grok LTO partitioning
> On 21.12.2015, at 14.46, Emmanuel Florac wrote: > > Le Fri, 18 Dec 2015 17:06:44 +0100 > Emmanuel Florac écrivait: > >> >> I'm trying to use mt to work with LTO-5 and bigger tapes. Switching >> partitions works: >> >> # tapeinfo -f /dev/sg1 >> Product Type: Tape Drive >> Vendor ID: 'HP ' >> Product ID: 'Ultrium 5-SCSI ' >> Revision: 'Z61U' >> Attached Changer API: No >> SerialNumber: 'HU1249TP88' >> MinBlock: 1 >> MaxBlock: 16777215 >> SCSI ID: 1 >> SCSI LUN: 0 >> Ready: yes >> BufferedMode: yes >> Medium Type: Not Loaded >> Density Code: 0x58 >> BlockSize: 0 >> DataCompEnabled: yes >> DataCompCapable: yes >> DataDeCompEnabled: yes >> CompType: 0x1 >> DeCompType: 0x1 >> BOP: yes >> Block Position: 0 >> Partition 0 Remaining Kbytes: 1459056 >> Partition 0 Size in Kbytes: 1459056 >> ActivePartition: 0 >> EarlyWarningSize: 0 >> NumPartitions: 1 >> MaxPartitions: 1 >> Partition0: 38 >> Partition1: 1453 >> >> # mt -f /dev/nst0 setpartition 1 >> >> >> However "mt mkpartition" fails miserably: >> >> # mt -f /dev/nst0 mkpartition 1453 >> /dev/nst0: Input/output error >> >> # dmesg | tail >> st 6:0:1:0: st0: Failed to read 65536 byte block with 256 byte >> transfer. st0: Sense Key : Illegal Request [current] >> st0: Add. Sense: Invalid field in parameter list >> >> Is this a limitation of mt or the st driver? >> > > I'm replying to myself: this is very obviously a limitation of the st > driver. Checking st.c partition_tape() function, it looks like it only > knows of hardware from past century… > True, it does support only the methods supported by the drives available to the developers at that time :-) However, I am not any more convinced that partitioning a tape should be done by the kernel driver. A more flexible method would be a user space program using sg (or bsg) driver. > OTOH it seems that Cygwin does that properly... by using > CreateTapePartition, a windows kernel32.dll function. Argh. We'll have > to do the heavy lifting of SCSI commands by hand, then. > > Where should we post an eventual patch, given that the linux-tape ML > looks like a ghost town? It would also be great to be able to support > more than 2 partitions (LTO-6 and 7 support 4), but that would require > patching the mt utility too, but I don't where it currently lives :) > Any hints welcome. > This (linux-scsi) is the correct mailing list. The correct source code of your mt depends on the version you are using (check the distribution). Not also that the ioctl method for creating partitions available in the kernel supports only up to two partitions. If you want more, you should design a new interface. Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: st driver doesn't seem to grok LTO partitioning
> On 21.12.2015, at 19.57, Emmanuel Florac wrote: > > Le Mon, 21 Dec 2015 19:25:27 +0200 > "Kai Mäkisara (Kolumbus)" écrivait: > >>> >>> I'm replying to myself: this is very obviously a limitation of the >>> st driver. Checking st.c partition_tape() function, it looks like >>> it only knows of hardware from past century… >>> >> True, it does support only the methods supported by the drives >> available to the developers at that time :-) However, I am not any >> more convinced that partitioning a tape should be done by the kernel >> driver. A more flexible method would be a user space program using sg >> (or bsg) driver. > > Yes, that's the way the LTFS utility works. That makes the code of the > application quite hard to read, though, as partitioning on modern > drives is pretty complex and requires understanding ugly, unholy SCSI > commands magic. Then Windows/Cygwin, IBM AIX and maybe others seem to > implement partitioning at the kernel level. > > Do you suggest the current mkpartition/setpartition scheme should be > removed instead of enhanced ? :) > I don’t suggest that it should be removed. I just pointed out another possibility to implement the more complex formatting cases. I think that SCSI is well defined, but it offers too many ways to do one thing :-) In user space one can implement several alternatives but the kernel driver has to choose only one (or move the complexity to the interface). On the other hand, the kernel driver can allow anyone (who can access the st device) to partition the tape. A user space program needs more rights. If the current interface (MTMKPART) is acceptable, we could enhance the driver so that it handles properly more drives. I looked at the HP Ultrium SCSI reference and it may not be too complicated (for instance, it needs a separate FORMAT MEDIUM command) . I have also found an IBM reference and others can probably be found. I can look at the manuals during Christmas holidays and try to think about a suggestion. All other suggestions are, of course, welcome. My view may be somewhat theoretical because I don’t have access to current hardware. >>> OTOH it seems that Cygwin does that properly... by using >>> CreateTapePartition, a windows kernel32.dll function. Argh. We'll >>> have to do the heavy lifting of SCSI commands by hand, then. >>> >>> Where should we post an eventual patch, given that the linux-tape ML >>> looks like a ghost town? It would also be great to be able to >>> support more than 2 partitions (LTO-6 and 7 support 4), but that >>> would require patching the mt utility too, but I don't where it >>> currently lives :) Any hints welcome. >>> >> This (linux-scsi) is the correct mailing list. >> > > OK. As I mentioned, the bizarre IBM lin_tape driver (what use is it?) > implements the complete scheme properly apparently, in a nice GPL2 code. > Why they didn't contribute it directly to the st driver is mysterious. > I will look at the driver. HP also seems to have an implementation (for LTFS) but I did not find the code because their web links don’t seem to be uptodate. >> The correct source code of your mt depends on the version you are >> using (check the distribution). Not also that the ioctl method for >> creating partitions available in the kernel supports only up to two >> partitions. If you want more, you should design a new interface. > > I'm using mt-st on debian, the one that seems to go hand in hand with > the st driver. Both carry your copyright :) > OK. Then the mt part will not be a problem. I have not updated it “recently” because the kernel interface has not changed. Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: st driver doesn't seem to grok LTO partitioning
> On 29.12.2015, at 18.59, Emmanuel Florac wrote: > > Le Fri, 25 Dec 2015 17:53:46 +0200 (EET) > Kai Makisara écrivait: > >> the patch implements the following: if the >> size is 1, the driver tells the drive to use default partitioning for >> two partitions. For the HP Ultrium this should result in partition 0 >> of 1425 GB and 1 of 37.5 GB. I don't know if this is a useful >> addition. > > Oh BTW I didn't check what's happening in code, but actual values > should be 37.5 GB for partition 0 and 1425 GB for partition 1, not the > other way around. > What I quoted is from the HP manual. The driver tells the drive to format the tape according to the default of the drive (i.e., sets the FDP bit in the mode page). The order of partitions is an interesting question. The usual use case is to have a small partition for the index and a large partition for the data. The small partition should positioned so that it can be accessed fast. For the old really linear drives this means the beginning. It was decided that the first partition on the tape was given number one and the second partition had the number zero. The HP LTO seems to use this numbering for the default. The SCSI standard does not specify how the partitions should be numbered. Many drives use physically sequential numbering nowadays. LTO uses wrap-wise partitioning and I don’t think locations of the partitions have any effect on access speed. Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: st driver doesn't seem to grok LTO partitioning
> On 29.12.2015, at 18.58, Emmanuel Florac wrote: > > Le Fri, 25 Dec 2015 17:53:46 +0200 (EET) > Kai Makisara écrivait: > >> The patch uses the scsi level of the device to separate processing. >> The FORMAT MEDIUM command is defined in SCSI-3 and I suppose that no >> current drive is still SCSI-2. In addition to the "sane" changes >> using the method to specify partitions, the patch implements the >> following: if the size is 1, the driver tells the drive to use >> default partitioning for two partitions. For the HP Ultrium this >> should result in partition 0 of 1425 GB and 1 of 37.5 GB. I don't >> know if this is a useful addition. > > Still testing, with another, LTO-5 tape: > > ~# mt -f /dev/nst0 mkpartition 0 > /dev/nst0: Invalid argument > It seems that you have not told the st driver that your drive knows partitions. One way to set the options is to use the stint program and proper definitions. You can also use mtst -f /dev/nst0 stsetoption can-partitions > Dec 29 17:57:38 shakuhachi kernel: st 7:0:0:0: st0: Block limits 1 - 16777215 > bytes. > Dec 29 17:57:38 shakuhachi kernel: st 7:0:0:0: st0: Mode sense. Length 11, > medium 0, WBS 10, BLL 8 > Dec 29 17:57:38 shakuhachi kernel: st 7:0:0:0: st0: Density 58, tape length: > 0, drv buffer: 1 > Dec 29 17:57:38 shakuhachi kernel: st 7:0:0:0: st0: Block size: 0, buffer > size: 4096 (1 blocks). > The st driver prints some debugging data to the kernel log if debugging is enabled. If the driver is compiled with debugging (it is nowadays default but in 3.18 you should change DEBUG to 1 in st.c). The debugging output can be enabled with the command: mtst -f /dev/nst0 stsetoption debug Here is the debugging output when I make a 10 MB partition with my drive: [ 2199.572546] st 4:0:5:0: [st0] Block limits 1 - 16777215 bytes. [ 2199.573367] st 4:0:5:0: [st0] Mode sense. Length 11, medium 0, WBS 10, BLL 8 [ 2199.573375] st 4:0:5:0: [st0] Density 26, tape length: 0, drv buffer: 1 [ 2199.573380] st 4:0:5:0: [st0] Block size: 0, buffer size: 4096 (1 blocks). [ 2199.573403] st 4:0:5:0: [st0] Rewinding tape. [ 2199.575319] st 4:0:5:0: [st0] Partition page length is 10 bytes. [ 2199.575328] st 4:0:5:0: [st0] PP: max 1, add 1, xdp 0, psum 02, pofmetc 0,rec 03 units 00, sizes: 100 65535 [ 2199.575333] st 4:0:5:0: [st0] psd_cnt 1, max.parts 1, nbr_parts 1 [ 2199.575338] st 4:0:5:0: [st0] Formatting tape with two partitions (1 = 10 MB). [ 2199.575342] st 4:0:5:0: [st0] Sent partition page length is 10 bytes. needs_format: 0 [ 2199.575348] st 4:0:5:0: [st0] PP: max 1, add 1, xdp 1, psum 02, pofmetc 0,rec 03 units 00, sizes: 10 65535 Regardless whether the partitioning works with your drive, your should see at least few lines after “Rewinding tape”. The driver first reads the partition mode page and (for testing) prints some data from the page. Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: st driver doesn't seem to grok LTO partitioning
> On 30.12.2015, at 20.33, Emmanuel Florac wrote: > > Le Wed, 30 Dec 2015 19:54:01 +0200 (EET) > Kai Makisara écrivait: > >> I think I found out why it did not work with the default format. At >> the end of this message you find a new patch that should correct >> that. There are also other changes: >> - some changes when specifying the size in the mode page; needed for >> the IBM 3592 drives but should not change results with LTO >> - formatting with one partition is done directly with FORMAT MEDIUM if >> the drive requires that command >> - some other bugs fixed > > Hum, I can't seem to be able to go back to 1 partition, or to reformat > the tape (didn't change it since yesterday), am I missing something? > > Dec 30 19:21:29 shakuhachi kernel: st: Version 20151230, fixed bufsize 32768, > s/g segs 256 > Dec 30 19:21:29 shakuhachi kernel: st 7:0:0:0: Attached scsi tape st0 > Dec 30 19:21:29 shakuhachi kernel: st 7:0:0:0: st0: try direct i/o: yes > (alignment 512 B) > > ~# mt -f /dev/nst0 mkpartition 0 > > Dec 30 19:24:41 shakuhachi kernel: st 7:0:0:0: st0: Block limits 1 - 16777215 > bytes. > Dec 30 19:24:41 shakuhachi kernel: st 7:0:0:0: st0: Mode sense. Length 11, > medium 0, WBS 10, BLL 8 > Dec 30 19:24:41 shakuhachi kernel: st 7:0:0:0: st0: Density 5a, tape length: > 0, drv buffer: 1 > Dec 30 19:24:41 shakuhachi kernel: st 7:0:0:0: st0: Block size: 0, buffer > size: 4096 (1 blocks). > Dec 30 19:24:41 shakuhachi kernel: st 7:0:0:0: st0: Rewinding tape. > Dec 30 19:24:41 shakuhachi kernel: st 7:0:0:0: st0: Partition page length is > 16 bytes. > Dec 30 19:24:41 shakuhachi kernel: st 7:0:0:0: st0: PP: max 3, add 1, xdp 1, > psum 03, pofmetc 4, rec 03, units 09, sizes: 2543 38 > Dec 30 19:24:41 shakuhachi kernel: st 7:0:0:0: st0: Sending FORMAT MEDIUM > Dec 30 19:24:41 shakuhachi kernel: st 7:0:0:0: st0: Error: 802, cmd: 4 0 > 0 0 0 0 > Dec 30 19:24:41 shakuhachi kernel: st0: Sense Key : Illegal Request [current] > Dec 30 19:24:41 shakuhachi kernel: st0: Add. Sense: Position past beginning > of medium This happens if the position is not at the beginning of partition 0. Could you try to switch to partition 0: mt -f /dev/nst0 setpartition 0 mt -f /dev/nst0 status and the retry partitioning. > ~# mt -f /dev/nst0 mkpartition 1 > > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: Block limits 1 - 16777215 > bytes. > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: Mode sense. Length 11, > medium 0, WBS 10, BLL 8 > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: Density 5a, tape length: > 0, drv buffer: 1 > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: Block size: 0, buffer > size: 4096 (1 blocks). > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: Rewinding tape. > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: Partition page length is > 16 bytes. > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: PP: max 3, add 1, xdp 1, > psum 03, pofmetc 4, rec 03, units 09, sizes: 2543 38 > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: psd_cnt 2, max.parts 3, > nbr_parts 1 > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: Formatting tape with two > partitions (FDP). > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: Sent partition page > length is 12 bytes. needs_format: 1 > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: PP: max 3, add 1, xdp 4, > psum 00, pofmetc 4, rec 03, units 00, sizes: 65535 0 > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: Error: 802, cmd: 15 > 10 0 0 18 0 > Dec 30 19:26:12 shakuhachi kernel: st0: Sense Key : Illegal Request [current] > Dec 30 19:26:12 shakuhachi kernel: st0: Add. Sense: Invalid field in > parameter list > Dec 30 19:26:12 shakuhachi kernel: st 7:0:0:0: st0: Partitioning of tape > failed. > I have to recheck what it set into the mode page. It is still not correct in this case. Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: st driver doesn't seem to grok LTO partitioning
> On 30.12.2015, at 23.24, Emmanuel Florac wrote: > > Le Wed, 30 Dec 2015 21:21:47 +0200 > "Kai Mäkisara (Kolumbus)" écrivait: > >> This happens if the position is not at the beginning of partition 0. >> Could you try to switch to partition 0: >> mt -f /dev/nst0 setpartition 0 >> mt -f /dev/nst0 status >> >> and the retry partitioning. > > Alas, no dice: > OK. In the HP LTFS sources I found an interesting detail: the code does LOAD before unformatting. A comment says that it is in some cases better method to put the position to beginning of partition 0 than other methods. You could try ‘mt load’ before trying to partition. Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning
> On 15.1.2016, at 2.21, Seymour, Shane M wrote: > > Unfortunately I'm unable to lay my hands on an LTO 5 tape drive so I'm not > able to test that it works either. If it helps at all I can test in the > negative and make sure that for an LTO 3 drive it fails gracefully but that's > about it at the moment. Thanks for all testers and those who attempted to test. The latest patch applies the standard quite strictly and I think it should work with most drives. The implementation can be fixed later if problems are found. However, before making the final patch, we should decide which partition the specified size should apply to. For the SCSI level <=2 it applies to partition 1. For other drives we may have some freedom to “tune” the definition. The size should apply to the partition the users expect it to apply. The current documentation says "the argument gives in megabytes the size of partition 1 that is physically the first partition of the tape”. The documentation I have found for current drives (HP and IBM LTO, IBM 3592, Storagetek T1000) all number the partitions sequentially from the start of the tape. The access time for any partition is probably about the same when wrapwise partitioning is used. It does matter with linear partitioning. Unfortunately, the standards leave the numbering to the implementor. Partitioning with two partitions is used for storing index in a small partition and use the rest of the tape for data. In this case, it is probably natural to specify the size of the index. The LTFS definition supports index in any partition. The open source code I have seen seem to default to index in partition 0. The HP and IBM LTO default partitioning (FDP=1) specifies two wraps (minimum) to partition 1 and the rest to 0. There seem to be lot of arguments supporting both possible choices. Should we use the existing definition (1) or change it for the drives supporting SCSI level >= 3 (or supporting FORMAT MEDIUM)? The definition can’t be changed later. This is why we should make a good decision. Opinions? Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: add debug flag parameter for SCSI tape driver - 2nd request
Hello,
I am responding to this, but noticed your next, fixed version.
> On 17.10.2014, at 23.20, Laurence Oberman wrote:
>
> Hello Kai
>
> You have seen this patch before. The first time around, given that we don't
> enable DEBUG by default, I let it go.
> However we have been looking into defining DEBUG 1 by default here at Redhat
> and then setting the default to disabled.
>
> Are you open to considering changing the driver based on this patch.
> i.e. default DEFINE 1 and adding this code with default set to off.
>
Yes. I certainly think defining DEBUG 1 and changing the default to zero should
be done if it is useful for supporting users. The runtime overhead is
negligible and the extra code does not matter nowadays (it did matter, at least
theoretically, for years ago).
I am not so sure about the module option. When the debugging code is compiled
in, debugging can be enabled and disabled for each device by the MTIOCTOP ioctl
(e.g., mtst -f tape_device stsetoptions debug). The module option enables
debugging for all tape devices. However, if you think this additional module
option is useful, I am not against it. It does not remove the possibility for
controlling debugging for each device for those who want to do it that way.
Anyway, you should modify the documentation (Documentation/scsi/st.txt)
according to the changes.
> Note that with DEBUG 0, as you know you need to change that and recompile.
> That is exactly what I am trying to avoid with Enterprise customers.
>
I have also noticed this when someone has asked me about some tape problems.
> This patch adds a debug_flag parameter that can be set on module load, and
> allows the DEBUG facility without a module recompile.
> Note that now DEBUG 1 is the default with this patch.
>
> Usage: modprobe st debug_flag=1
>
> Signed-off-by: Laurence Oberman
>
> diff -Nur a/st.c b/st.c
> --- a/st.c2014-10-17 16:15:54.103810627 -0400
> +++ b/st.c2014-10-17 16:22:12.303810392 -0400
> @@ -56,7 +56,7 @@
>
> /* The driver prints some debugging information on the console if DEBUG
>is defined and non-zero. */
> -#define DEBUG 0
> +#define DEBUG 1
>
> #define ST_DEB_MSG KERN_NOTICE
> #if DEBUG
> @@ -80,6 +80,7 @@
> static int try_direct_io = TRY_DIRECT_IO;
> static int try_rdio = 1;
> static int try_wdio = 1;
> +static int debug_flag = 0;
>
> static struct class st_sysfs_class;
> static const struct attribute_group *st_dev_groups[];
> @@ -100,6 +101,9 @@
> 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(debug_flag, debug_flag, int, 0);
> +MODULE_PARM_DESC(debug_flag, "Enable DEBUG, same as setting debugging=1");
> +
>
> /* Extra parameters for testing */
> module_param_named(try_rdio, try_rdio, int, 0);
> @@ -124,6 +128,9 @@
> },
> {
> "try_direct_io", &try_direct_io
> +},
> +{
> +"debug_flag", &debug_flag
> }
> };
> #endif
> @@ -4306,6 +4313,11 @@
>
> validate_options();
>
> +debugging = (debug_flag > 0) ? debug_flag : DEBUG;
> + if (debugging)
> +printk(KERN_INFO "st: Debugging enabled debug_flag =
> %d\n",debugging);
> +
> +
I think you have an extra newline here?
I also think that the kernel log would look nicer if the print below would be
before setting the option. The driver would introduce itself first and print
the debug flag status after that.
> printk(KERN_INFO "st: Version %s, fixed bufsize %d, s/g segs %d\n",
> verstr, st_fixed_buffer_size, st_max_sg_segs);
Thanks,
Kai
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: add debug flag parameter for SCSI tape driver - 2nd request
> On 19.10.2014, at 16.44, Laurence Oberman wrote:
>
> Hello Kai
>
> Thanks.
>
> Here is v3
>
> This patch adds a debug_flag parameter that can be set on module load, and
> allows the DEBUG facility without a module recompile.
> Note that now DEBUG 1 is the default with this patch.
>
> Usage: modprobe st debug_flag=1
>
> Signed-off-by: Laurence Oberman
Acked-by: Kai Mäkisara
Thanks,
Kai
>
> diff -Nur a/Documentation/scsi/st.txt b/Documentation/scsi/st.txt
> --- a/Documentation/scsi/st.txt 2014-10-19 09:36:52.243863716 -0400
> +++ b/Documentation/scsi/st.txt 2014-10-19 09:43:19.222863447 -0400
> @@ -506,9 +506,11 @@
>
> DEBUGGING HINTS
>
> -To enable debugging messages, edit st.c and #define DEBUG 1. As seen
> -above, debugging can be switched off with an ioctl if debugging is
> -compiled into the driver. The debugging output is not voluminous.
> +Debugging code is now compiled in by default but debugging is turned off
> with
> +the kernel module parameter debug_flag defaulting to 0.
> +Debugging can still be switched on and off with an ioctl.
> +To enable debug at module load time add debug_flag=1 to the module load
> +options, the debugging output is not voluminous.
>
> If the tape seems to hang, I would be very interested to hear where
> the driver is waiting. With the command 'ps -l' you can see the state
>
> diff -Nur a/drivers/scsi/st.c b/drivers/scsi/st.c
> --- a/drivers/scsi/st.c 2014-10-19 09:35:45.673863756 -0400
> +++ b/drivers/scsi/st.c 2014-10-19 09:35:30.621863483 -0400
> @@ -56,7 +56,8 @@
>
> /* The driver prints some debugging information on the console if DEBUG
>is defined and non-zero. */
> -#define DEBUG 0
> +#define DEBUG 1
> +#define NO_DEBUG 0
>
> #define ST_DEB_MSG KERN_NOTICE
> #if DEBUG
> @@ -80,6 +81,7 @@
> static int try_direct_io = TRY_DIRECT_IO;
> static int try_rdio = 1;
> static int try_wdio = 1;
> +static int debug_flag = 0;
>
> static struct class st_sysfs_class;
> static const struct attribute_group *st_dev_groups[];
> @@ -100,6 +102,9 @@
> 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(debug_flag, debug_flag, int, 0);
> +MODULE_PARM_DESC(debug_flag, "Enable DEBUG, same as setting debugging=1");
> +
>
> /* Extra parameters for testing */
> module_param_named(try_rdio, try_rdio, int, 0);
> @@ -124,6 +129,9 @@
> },
> {
> "try_direct_io", &try_direct_io
> +},
> +{
> +"debug_flag", &debug_flag
> }
> };
> #endif
> @@ -4309,6 +4317,10 @@
> printk(KERN_INFO "st: Version %s, fixed bufsize %d, s/g segs %d\n",
> verstr, st_fixed_buffer_size, st_max_sg_segs);
>
> +debugging = (debug_flag > 0) ? debug_flag : NO_DEBUG;
> + if (debugging)
> +printk(KERN_INFO "st: Debugging enabled debug_flag =
> %d\n",debugging);
> +
> err = class_register(&st_sysfs_class);
> if (err) {
> pr_err("Unable register sysfs class for SCSI tapes\n");
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PATCH: st.c: Fix blk_get_queue usage
On 14.11.2013, at 16.48, Bodo Stroesser wrote: > Hi, > > in st_probe(), st.c I stumbled across what I'd call a minor problem. > > So I'd like to suggest the following patch. > > Best Regards, > Bodo > > P.S.: Please CC me, I'm not on the list. > > - > > > From: Bodo Stroesser > Date: Thu, 14 Nov 2013 14:35:00 +0100 > Subject: [PATCH] sg: fix blk_get_queue usage > > If blk_queue_get() in st_probe fails, disk->queue must not > be set to SDp->request_queue, as that would result in > put_disk() dropping a not taken reference. > > Thus, disk->queue should be set only after a successful > blk_queue_get(). > > Signed-off-by: Bodo Stroesser > > --- > > --- a/drivers/scsi/st.c 2013-11-14 14:10:40.0 +0100 > +++ b/drivers/scsi/st.c 2013-11-14 14:10:57.0 +0100 > @@ -4107,11 +4107,11 @@ > kref_init(&tpnt->kref); > tpnt->disk = disk; > disk->private_data = &tpnt->driver; > - disk->queue = SDp->request_queue; > /* SCSI tape doesn't register this gendisk via add_disk(). Manually >* take queue reference that release_disk() expects. */ With this patch, blk_get_queue() is not called with the correct argument. Maybe change the call to blk_get_queue(SDp->request_queue) ? > if (blk_get_queue(disk->queue)) > goto out_put_disk; > + disk->queue = SDp->request_queue; > tpnt->driver = &st_template; > > tpnt->device = SDp-- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ; Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] st.ko: fix enlarge_buffer
On 2.12.2013, at 21.00, Bodo Stroesser wrote:
> From: Bodo Stroesser
> Date: Mon, 2 Dec 2013 18:52:10 +0100
> Subject: [PATCH 1/3] st.ko: fix enlarge_buffer
>
> This patch removes a bug in enlarge_buffer() that can make a
> read or write fail under special conditions.
>
> After changing TRY_DIRECT_IO to 0 and ST_MAX_SG to 32 in
> st_options.h, a program that writes a first block of 128k and
> than a second bigger block (e.g. 256k) fails. The second write
> returns errno EOVERFLOW, as enlarge_buffer() checks the sg list
> and detects that it already is full.
> As enlarge_buffer uses different page allocation orders
> depending on the size of the buffer needed, the check does not
> make sense.
>
Yes, it is not useful any more. It may have been necessary at some time but I
am now not sure about that either ;-)
> Cc: Kai Makisara
Acked-by: Kai Mäkisara
Thanks,
Kai
> Signed-off-by: Bodo Stroesser
>
> ---
>
> --- a/drivers/scsi/st.c 2013-12-02 18:52:10.0 +0100
> +++ b/drivers/scsi/st.c 2013-12-02 18:52:10.0 +0100
> @@ -3719,7 +3719,7 @@ static struct st_buffer *new_tape_buffer
>
> static int enlarge_buffer(struct st_buffer * STbuffer, int new_size, int
> need_dma)
> {
> - int segs, nbr, max_segs, b_size, order, got;
> + int segs, max_segs, b_size, order, got;
> gfp_t priority;
>
> if (new_size <= STbuffer->buffer_size)
> @@ -3729,9 +3729,6 @@ static int enlarge_buffer(struct st_buff
> normalize_buffer(STbuffer); /* Avoid extra segment */
>
> max_segs = STbuffer->use_sg;
> - nbr = max_segs - STbuffer->frp_segs;
> - if (nbr <= 0)
> - return 0;
>
> priority = GFP_KERNEL | __GFP_NOWARN;
> if (need_dma)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] st.ko: remove unnecessary normalize_buffer
On 2.12.2013, at 21.00, Bodo Stroesser wrote: > From: Bodo Stroesser > Date: Mon, 2 Dec 2013 18:52:10 +0100 > Subject: [PATCH 2/3] st.ko: remove unnecessary normalize_buffer > > This patch removes an unnecessary call to normalize_buffer() > in enlarge_buffer() > > In st_open() always a buffer of one page is allocated. > When the buffer needs to be enlarged later, it does not make > sense to free this page unconditionally. > The original reason for this was to make the function to allocate the minimum number of segments for maximum efficiency. In some cases it was essential not to waste one segment because that would have made the allocation fail. Now there is another reason to have this “optimization”. Reading the code you probably have noticed that nowadays all segments must have the same size. If the first segment is only one page, the first allocation may it may not be possible to allocate the buffer using single page segments. This leads to freeing the pages and trying again. This is not efficient. So, I think this fragment of code should not be removed. Thanks, Kai > Cc: Kai Makisara > Signed-off-by: Bodo Stroesser > > --- > > --- a/drivers/scsi/st.c 2013-12-02 18:52:10.0 +0100 > +++ b/drivers/scsi/st.c 2013-12-02 18:52:10.0 +0100 > @@ -3725,9 +3725,6 @@ static int enlarge_buffer(struct st_buff > if (new_size <= STbuffer->buffer_size) > return 1; > > - if (STbuffer->buffer_size <= PAGE_SIZE) > - normalize_buffer(STbuffer); /* Avoid extra segment */ > - > max_segs = STbuffer->use_sg; > > priority = GFP_KERNEL | __GFP_NOWARN; -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] st.ko: change enlarge_buffer result
On 2.12.2013, at 21.00, Bodo Stroesser wrote: > From: Bodo Stroesser > Date: Mon, 2 Dec 2013 18:52:10 +0100 > Subject: [PATCH 3/3] st.ko: change enlarge_buffer result > > enlarge_buffer() just returns 1 or 0 if it could or could > not allocate the requested buffer. > > In case of result 0, the callers always set the error to > EOVERFLOW. I think, this is not a good errno for those > cases, where enlarge_buffer() could not allocate the pages > it needed. > > So I changed enlarge_buffer() to return a meaningful > result (-ENOMEM or -EOVERFLOW in case of error, 0 in case of > success) and the callers to use this result. > ENOMEM is used for telling the user that, in variable block mode, the byte count in read() is smaller than the next block. This may not sound like proper use of this code but this is how the tape drivers have done. When ENOMEM is not used, the patch would be only cosmetic. Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: int overflow in st_ioctl()
On 17.12.2013, at 16.43, Yongjian Xu xuyongjia...@gmail.com wrote: > From: Yongjian Xu > > mtc.mt_count comes from user-space. > int overflow may occur: > mtc.mt_count++; > mtc.mt_count—; I agree that this is a problem. However, it seems that there is also another problem: the SPACE command uses a 24-bit count field and this overflows far below INT_MAX. Should the count in MTFSF etc. be limited to (2<<24-2)? This would make the checks you suggest unnecessary. (-2 so that the behavior does not depend on whether we are at file mark or not.) I am a bit hesitant with this suggestion because someone may use count INT_MAX to space to the end of the tape. This probably succeeds now but after the fix it would return an error. Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: Do not rewind for SG_IO
On 1.2.2014, at 16.06, Hannes Reinecke wrote: > On 01/31/2014 05:43 PM, Jeremy Linton wrote: >> -BEGIN PGP SIGNED MESSAGE- >> Hash: SHA1 >> >> On 1/31/2014 2:46 AM, Hannes Reinecke wrote: >> >>> This patch make the tape always non-rewinding when SG_IO is used, thus >>> allowing udev to get a proper device id for tapes. >> >> Maybe instead of silently changing the behavior, if you just _HAVE_ to >> open >> the st device, add an ioctl or st/mt_op that disables the rewind on close. >> That way applications have to explicitly disable the rewind on close. >> > Okay, that sounds like a better alternative. > Point is for udev we simply _have_ to use the given device node. > And when this happens to be set to rewind on close we're doomed. > I don’t quite understand why it has to use the auto-rewind device instead of the non-rewind device. From the minor number it can see what the non-rewind device is. If the problem is that it is not guaranteed that the non-rewind device exits, you should rather change the order the devices are created. If you absolutely have to do this, then do it. But new ioctls are deprecated and also it is a bad habit to change the kernel to make things easier for a single program. > I'll be drafting up a patch. > If you do, don’t forget to update the documentation. Thanks, Kai P.S. I don’t like the auto-rewind devices at all. But the Unix systems have those and so we also must to have those ;-) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: Do not rewind for SG_IO
On 2.2.2014, at 13.42, Hannes Reinecke wrote: [part of explanation snipped from reply] > But we're now trying to deprecate the original (and unmaintained) > scsi_id program and replace it with the standard 'sg_inq' program. > Which is a standard program which just issues the respective SCSI command; > most of the post-processing will be done by udev rules. > And implementing the same workaround here is really a bit hackish. > Hence this proposal to allow 'sg_inq' (or any program from sg3_utils) > to be called without interrupting normal operations on a tape device. > OK. After your explanation I think adding the new MTIOCTOP operation is the least ugly solution :-) >> If you absolutely have to do this, then do it. But new ioctls are deprecated >> and >> also it is a bad habit to change the kernel to make things easier for a >> single >> program. >> > Well, the actual problem here is that the 'st' driver is not designed for > multi-initiator environments. The original design for the driver assumed that > a single program had control over the 'st' driver, and there is only one > instance talking to the hardware. > Which simply doesn't fit well with the modern, asynchronous, setup. > The basic problem is that a sequential access device does not fit well the asynchronous setup. I admit that some simplifications have been made because of this. > And it's not just udev which suffers here; try to setup multipath on a tape > device ... > >>> I'll be drafting up a patch. >>> >> If you do, don’t forget to update the documentation. >> > Okay. New patch attached. > I think you have not compiled the patched st.c: @@ -2263,6 +2265,8 @@ static int st_set_options(struct scsi_tape *STp, long options) STm->sysv = value; if ((options & MT_ST_SILI) != 0) STp->sili = value; + if (value && (options & MT_ST_NOREWIND) != 0) + Stp->rew_at_close = 0; ^^^ Should be STp. However, looking at the code already suggests that this operation does not belong to MT_ST_*. These bits set/clear options that persist. The operations you need is transient so that the effect disappears when the device is closed. I think it would be better to define it as and ordinary MTIOCTOP operation code, e.g., in mtio.h: #define MTNOAUTOREWIND 36 /* suppress possible pending automatic rewind */ or something like that? Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: fix corruption of the st_modedef structures in st_set_options()
On 11.2.2014, at 23.22, Maurizio Lombardi wrote:
> When copying the st_modedef structures the devs pointers must be preserved
> in the same way as with the cdevs pointers.
>
> This fixes bug 70271: https://bugzilla.kernel.org/show_bug.cgi?id=70271
>
> [ 135.037052] BUG: unable to handle kernel NULL pointer dereference at
> 0098
> [ 135.045048] IP: [] kernfs_find_ns+0x21/0x150
> [ 135.050999] PGD 220623067 PUD 222171067 PMD 0
...
> [ 135.357859] Code: ff eb e3 0f 1f 80 00 00 00 00 55 48 89 e5 48 83 ec 30 48
> 89 5d d8 4c 89 65 e0 4c 89 6d e8 4c 89 75 f0 4c 89 7d f8 66 66 66 66 90 <44>
> 0f b7 bf 98 00 00 00 8b 05 71 6d 87 00 48 89 fb 49 89 f4 49
> [ 135.378282] RIP [] kernfs_find_ns+0x21/0x150
> [ 135.384355] RSP
> [ 135.387881] CR2: 0098
> [ 135.391298] ---[ end trace 1968409221ddb3c8 ]---
>
> Signed-off-by: Maurizio Lombardi
Acked-by: Kai Mäkisara
> ---
> drivers/scsi/st.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index a1d6986..afc834e 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -2198,12 +2198,19 @@ static int st_set_options(struct scsi_tape *STp, long
> options)
> struct st_modedef *STm;
> char *name = tape_name(STp);
> struct cdev *cd0, *cd1;
> + struct device *d0, *d1;
>
> STm = &(STp->modes[STp->current_mode]);
> if (!STm->defined) {
> - cd0 = STm->cdevs[0]; cd1 = STm->cdevs[1];
> + cd0 = STm->cdevs[0];
> + cd1 = STm->cdevs[1];
> + d0 = STm->devs[0];
> + d1 = STm->devs[1];
> memcpy(STm, &(STp->modes[0]), sizeof(struct st_modedef));
> - STm->cdevs[0] = cd0; STm->cdevs[1] = cd1;
> + STm->cdevs[0] = cd0;
> + STm->cdevs[1] = cd1;
> + STm->devs[0] = d0;
> + STm->devs[1] = d1;
> modes_defined = 1;
> DEBC(printk(ST_DEB_MSG
> "%s: Initialized mode %d definition from mode
> 0\n",
> --
> Maurizio Lombardi
Thanks,
Kai
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: add debug flag parameter for SCSI tape driver
On 11.6.2014, at 2.48, Laurence Oberman wrote: > Hello > > Take 2 of this patch, changed module description and subject line. > > This patch adds a debug_flag parameter that can be set on module load, and > allows the DEBUG facility without a module recompile. > Usage: mpdprobe st debug_flag=1 > > Signed-off-by: Laurence Oberman > What is wrong with the existing methods to control debugging? You can enable and disable debugging for each device with ioctl() (as described in the driver documentation). You can use mt-st to do this from command line. Your patch just allows one to change the default for all devices. The real problem may be that the distributions don’t compile the debugging code into the drivets but your patch does not change this. Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] st: call scsi_set_medium_removal directly
> On 7.11.2014, at 1.54, Elliott, Robert (Server Storage) > wrote: > ... > 3. Reviewing the callers, st_release has an initialized > result variable but does nothing else with it but return it: > int result = 0; > ... > return result; > > It ignores the do_door_lock -> scsi_set_medium_removal > result (like many others). > The return code for automatic door locking/unlocking has been ignored on purpose. It is returned to caller for explicit door locking/unlocking. One can, of course, revise this decision. The return value of release() is ignored by VFS. The proper fix would probably be to define release() void. Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: implement sysfs based tape statistics v2
> On 2.2.2015, at 17.16, Laurence Oberman wrote: > > I pulled this this morning and will be testing. The prior version was > stable for me on the upstream and RHEL 6.5 kernel without exhaustive > testing. > We also just received more requests to get this into RHEL from HP / > Red Hat customers. > > Kai, what are your thoughts. I realize this is a large amount of > additional code. I am not keen to create a driver just for stats as we > would have to keep the rest of the st driver changes always in sync. > I still think that the tape statistics should be exported like the statistics of “real” block devices, i.e., one sysfs file exporting on a single line the statistics that temporally belong together. James rejected this approach. I am leaving the decision about this code to him. I will neither ack nor nak this code. Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: implement sysfs based tape statistics v2
> On 5.2.2015, at 19.40, Laurence Oberman wrote: > > - Original Message - > From: "Kai Mäkisara (Kolumbus)" > To: "Laurence Oberman" > Cc: "Shane M Seymour" , lober...@redhat.com, > linux-scsi@vger.kernel.org, "James E.J. Bottomley (jbottom...@parallels.com)" > , je...@suse.com > Sent: Thursday, February 5, 2015 12:03:29 PM > Subject: Re: [PATCH] st: implement sysfs based tape statistics v2 > > >> On 2.2.2015, at 17.16, Laurence Oberman wrote: >> >> I pulled this this morning and will be testing. The prior version was >> stable for me on the upstream and RHEL 6.5 kernel without exhaustive >> testing. >> We also just received more requests to get this into RHEL from HP / >> Red Hat customers. >> >> Kai, what are your thoughts. I realize this is a large amount of >> additional code. I am not keen to create a driver just for stats as we >> would have to keep the rest of the st driver changes always in sync. >> > > I still think that the tape statistics should be exported like the statistics > of “real” block devices, i.e., one sysfs file exporting on a single line the > statistics that temporally belong together. James rejected this approach. I > am leaving the decision about this code to him. I will neither ack nor nak > this code. > > Thanks, > Kai > > Hello Kai, > > I missed the earlier conversations with James, I will go search for them. > Do you mean add them so they are similar to the /proc/diskstats > > cat /proc/diskstats > .. > 8 0 sda 2258346 152801 291907067 5263795 388817 1518048 15013833 > 4542062 0 4794931 9803495 > 8 1 sda1 717 102 26154 1179 8 2 80 76 0 1172 1254 > 8 2 sda2 328 31 2872 1554 0 0 0 0 0 1554 1554 > 8 3 sda3 2195205 151617 290898283 5203627 355053 1518046 15009528 > 4370598 0 4594137 9571937 > 8 4 sda4 61921 1050 978350 57218 18 0 4225 34 0 56384 57185 > 11 0 sr0 0 0 0 0 0 0 0 0 0 0 0 > .. > Not exactly. I mean the data exported in sysfs, for example: > cat /sys/block/sda/sda1/stat 159740 9006 594150664461 12472455907 12772208 3598677 0 299875 3663235 Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] implementing tape statistics single file vs multi-file in sysfs
> On 8.2.2015, at 4.45, Greg KH wrote: > > On Sat, Feb 07, 2015 at 09:27:05PM -0500, Laurence Oberman wrote: >> Hello >> Its not going to be tens of thousands of devices. That count was an >> aggregate based on 1000's of servers. >> In reality its unlikely to ever be more than 100 tapes drives per >> individual Linux kernel instance. >> Therefore sysfs will be the valid way to do this and make the data >> available to user space. > > Even if it is only 2 tape drives, again, what's wrong with using the > existing i/o statistic interfaces that all block devices have? Don't go > making special one-off interfaces for one type of device if at all > possible. > The tape driver does not have access to the block device i/o statistics because the tapes are not block devices but character devices. They use the Linux block subsystem enough to do i/o but this does not include access to the statistics counters. The purpose of the suggested text vector format patch is to create statistics that have the same format as the existing i/o statistics that the block devices so that the existing tools can be used with minimal modifications. One alternative is, of course, to tie the st driver more into the Linux block device system. I have looked into this several times but have never seen how to do this in a simple enough way. Thanks, Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: implement sysfs based tape statistics v2
> On 9.2.2015, at 8.00, Seymour, Shane M wrote: > > Kai - see last 3 paragraphs for question about if something is a bug or not. > > BTW I had a look - I couldn't quickly find out if there was a way to tell if > the medium has changed in a tape drive (there could be something device > specific). At the device level there's a record of I/O errors: > > [root@tapedrive device]# pwd > /sys/class/scsi_tape/st0/device > [root@tapedrive device]# cat ioerr_cnt > 0x5 > > That doesn't distinguish between read or write or any other kind of error > though - it doesn't even really have to mean an error since reading with a > block size too small also increments the error count: The counts in the device directory are not specific to tapes. From linux/scsi/scsi_lib.c one can see that ierr_cnt is incremented every time the scsi call returns any kind of error, including when the tape drive wants to return some information with sense data. > [root@tapedrive tape-stats]# ./tape_exercise write /dev/st0 /dev/urandom > 100 > About to write from /dev/urandom to /dev/st0, max size = 100, blksize = > 4096 > Write complete on /dev/st0 after 1003520 bytes > ./tape_[root@tapedrive tape-stats]# ./tape_exercise read /dev/st0 > About to read from /dev/st0 blksize = 256 > Failed to read from /dev/st0 using current blksize, will try using a bigger > blksize > About to read from /dev/st0 blksize = 512 > Failed to read from /dev/st0 using current blksize, will try using a bigger > blksize > About to read from /dev/st0 blksize = 1024 > Failed to read from /dev/st0 using current blksize, will try using a bigger > blksize > About to read from /dev/st0 blksize = 2048 > Failed to read from /dev/st0 using current blksize, will try using a bigger > blksize > About to read from /dev/st0 blksize = 4096 > Reading complete for /dev/st0, 987136 bytes read > > [root@tapedrive tape-stats]# cd /sys/class/scsi_tape/st0/device > [root@tapedrive device]# cat ioerr_cnt > 0xa > > It would seem that ioerr_cnt is probably a count of SCSI check conditions > encountered? > > Unfortunately for the MTIOCGET ioctl the value of mt_resid is the partition > number of the tape not what I would have expected it to be - the residual > left after the last read or write that returned an error (and 0 if the last > read/write didn't return an error). > > Kai - is that a bug? Shouldn't mt_resid be the residual from the last failed > read or write indicating how much data didn't make it to the device and 0 on > a successful read or write? I've used mt_resid from MTIOCGET on HP-UX > previously when issuing reads and repositioning and retrying tape reads when > starting with too low a block size to try and automatically determine the > block size in use (it's never a good idea to under or overread tape blocks > because it always results in check conditions and in the st driver under > reading the block size always creates messages in dmesg). > This is not a bug. man st documents that mt_resid does return the partition number. In the different unix systems the field did not consistently return the residual count. The Linux SCSI drivers did not at that time return the residual. These are reasons why the field is used for partition number. For writes in any real situation (drive in buffered mode) you don’t know when the write() returns whether the data can be written to tape. All writes are on tape when write file marks returns successfully of the device is successfully closed. I am not sure that the amount of data successfully written is really useful. If I see a write error, I want to rewrite at least the latest file. For reads, there are other ways to determine the block size. (Use a large enough byte count and see what you get.) The st driver does not write to the log if the block is shorter than requested. Short read is logged (together with the real block size). If you don’t want the overhead of returning the sense data for short reads, you can set the SILI flag. > If you don't have time to look at it I may look at if it's possible to > provide the correct mt_resid for MTIOCGET - assuming that a long time if > misuse prevents us from correcting it. If there's no way to export a > partition number for the devices that support it I can add a new sysfs file > (call it partition) to export it that way and see if I can get the correct > value into mt_resid. > Changing the definition of a field should not be done. There is software that is correctly using the field as it is documented. Kai -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: fix blk_get_queue usage
> On 1 Aug 2017, at 15.42, Hannes Reinecke wrote: > > From: Bodo Stroesser > > If blk_queue_get() in st_probe fails, disk->queue must not > be set to SDp->request_queue, as that would result in > put_disk() dropping a not taken reference. > > Thus, disk->queue should be set only after a successful > blk_queue_get(). > > Signed-off-by: Bodo Stroesser > Acked-by: Shirish Pargaonkar > Signed-off-by: Hannes Reinecke Acked-by: Kai Mäkisara Thanks, Kai
Re: [PATCH 37/41] scsi: st: mark expected switch fall-throughs
> On 10 Jan 2019, at 21.56, Gustavo A. R. Silva wrote:
>
> Hi,
>
> Friendly ping (second one):
>
> Who can ack/review/take this patch, please?
>
Acked-by: Kai Mäkisara
Thanks,
Kai
> Thanks
> --
> Gustavo
>
> On 12/19/18 6:08 PM, Gustavo A. R. Silva wrote:
>> Hi,
>> Friendly ping:
>> Who can ack or review this patch, please?
>> Thanks
>> --
>> Gustavo
>> On 11/27/18 10:33 PM, Gustavo A. R. Silva wrote:
>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>> where we are expecting to fall through.
>>>
>>> Addresses-Coverity-ID: 114994 ("Missing break in switch")
>>> Addresses-Coverity-ID: 114995 ("Missing break in switch")
>>> Signed-off-by: Gustavo A. R. Silva
>>> ---
>>> drivers/scsi/st.c | 4
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
>>> index 7ff22d3f03e3..7c7a742a5ef7 100644
>>> --- a/drivers/scsi/st.c
>>> +++ b/drivers/scsi/st.c
>>> @@ -337,12 +337,14 @@ static void st_analyze_sense(struct st_request
>>> *SRpnt, struct st_cmdstatus *s)
>>> switch (sense[0] & 0x7f) {
>>> case 0x71:
>>> s->deferred = 1;
>>> +/* fall through */
>>> case 0x70:
>>> s->fixed_format = 1;
>>> s->flags = sense[2] & 0xe0;
>>> break;
>>> case 0x73:
>>> s->deferred = 1;
>>> +/* fall through */
>>> case 0x72:
>>> s->fixed_format = 0;
>>> ucp = scsi_sense_desc_find(sense, SCSI_SENSE_BUFFERSIZE, 4);
>>> @@ -2721,6 +2723,7 @@ static int st_int_ioctl(struct scsi_tape *STp,
>>> unsigned int cmd_in, unsigned lon
>>> switch (cmd_in) {
>>> case MTFSFM:
>>> chg_eof = 0;/* Changed from the FSF after this */
>>> +/* fall through */
>>> case MTFSF:
>>> cmd[0] = SPACE;
>>> cmd[1] = 0x01;/* Space FileMarks */
>>> @@ -2735,6 +2738,7 @@ static int st_int_ioctl(struct scsi_tape *STp,
>>> unsigned int cmd_in, unsigned lon
>>> break;
>>> case MTBSFM:
>>> chg_eof = 0;/* Changed from the FSF after this */
>>> +/* fall through */
>>> case MTBSF:
>>> cmd[0] = SPACE;
>>> cmd[1] = 0x01;/* Space FileMarks */
>>>
Re: [RFC] Re: broken userland ABI in configfs binary attributes
> On 26 Aug 2019, at 19.29, Al Viro wrote: > > On Mon, Aug 26, 2019 at 03:48:38AM +0100, Al Viro wrote: > >> We might be able to paper over that mess by doing what /dev/st does - >> checking that file_count(file) == 1 in ->flush() instance and doing commit >> there in such case. It's not entirely reliable, though, and it's definitely >> not something I'd like to see spreading. > > This "not entirely reliable" turns out to be an understatement. > If you have /proc/*/fdinfo/* being read from at the time of final close(2), > you'll get file_count(file) > 1 the last time ->flush() is called. In other > words, we'd get the data not committed at all. > ... > PS: just dropping the check in st_flush() is probably a bad idea - > as it is, it can't overlap with st_write() and after such change it > will… Yes, don’t just drop it. The tape semantics require that a file mark is written when the last opener closes this sequential device. This is why the check is there. Of course, it is good if someone finds a better solution for this. Thanks, Kai
