Re: [PATCH] lpfc: Fix race on command completion

[Hannes Reinecke]

On 01/20/2016 05:26 PM, Tomas Henzl wrote:
> On 15.1.2016 10:48, Hannes Reinecke wrote:
>> Upon command completion the lpfc driver would call ->done()
>> on the scsi command before taking the host lock and
>> releasing the command internally.
>> This opens up a race window there this command might be re-used
>> after ->done(), leading to a double completion on the same command.
> 
> I agree that a driver should clean up the command before calling
> ->done, but this driver uses a list based system where a command 
> can't be reused only until it was returned to the list,
> so I don't understand how a 'done' before internal free could
> cause an issue other than a failed lpfc_get_scsi_buf in .queuecommand.
> Is your issue related to the abort_handler 
> (maybe cmd->host_scribble = NULL; changes the abort handler flow)?
> 
Yes, this was (originally) an issue with the abort handler. But it
seems to be gone with the upstream driver, so this patch should be
retracted.

Will be reposting if and when the issue resurfaces.

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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/2] scsi: Do not attach VPD to devices that don't support it

[Alexander Duyck]

On Wed, Jan 20, 2016 at 11:37 PM, Hannes Reinecke  wrote:
> On 01/21/2016 07:35 AM, Alexander Duyck wrote:
>> The patch "scsi: rescan VPD attributes" introduced a regression in which
>> devices that don't support VPD were being scanned for VPD attributes
>> anyway.  This could cause issues for this parts and should be avoided so
>> the check for scsi_level has been moved out of scsi_add_lun and into
>> scsi_attach_vpd so that all callers will not scan VPD for devices that
>> don't support it.
>>
>> Fixes: 09e2b0b14690 ("scsi: rescan VPD attributes")
>> Signed-off-by: Alexander Duyck 
>> ---
>>  drivers/scsi/scsi.c  |3 +++
>>  drivers/scsi/scsi_scan.c |3 +--
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index b1bf42b93fcc..ed085e78c893 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -784,6 +784,9 @@ void scsi_attach_vpd(struct scsi_device *sdev)
>>   int pg83_supported = 0;
>>   unsigned char __rcu *vpd_buf, *orig_vpd_buf = NULL;
>>
>> + if (sdev->scsi_level < SCSI_3)
>> + return;
>> +
>>   if (sdev->skip_vpd_pages)
>>   return;
>>  retry_pg0:
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 6a820668d442..1b16c89e0cf9 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -986,8 +986,7 @@ static int scsi_add_lun(struct scsi_device *sdev, 
>> unsigned char *inq_result,
>>   }
>>   }
>>
>> - if (sdev->scsi_level >= SCSI_3)
>> - scsi_attach_vpd(sdev);
>> + scsi_attach_vpd(sdev);
>>
>>   sdev->max_queue_depth = sdev->queue_depth;
>>
>>
> Isn't this slightly pointless, given that we're testing the inverse
> condition in scsi_attach_vpd()?

I'm not sure what you are getting at.  What I basically did is move
the check here into the function.  No point in checking it in 2 spots
when checking it inside the function is good enough.

> And in anycase, I guess we should be using the same logic sd.c is
> using. Please see the attached patch.

The attached patch looks good as it also takes care of the opt-in case
which I had overlooked.  The only bit missing is the fact that we are
still checking scsi_level twice when we don't need to.

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

[Kai Mäkisara (Kolumbus)]

> 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: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning

[Laurence Oberman]

Given what we see at customers I am leaning towards the SCSI level <=2 to 
ensure the older LTO5's are supported.
The newer ones should be backwards compatible.
I may have an older LTO5 showing up that wont need a F/W update to work, and 
will be able to add a "tested by" once I get it.

But lets see what the others have to say

Laurence Oberman
Principal Software Maintenance Engineer
Red Hat Global Support Services

- Original Message -
From: "Kai Mäkisara (Kolumbus)" 
To: "Shane M Seymour" 
Cc: "Laurence Oberman" , "Emmanuel Florac" 
, "Laurence Oberman" , 
linux-scsi@vger.kernel.org
Sent: Thursday, January 21, 2016 3:58:46 PM
Subject: 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: What partition should the MTMKPART argument specify? Was: Re: st driver doesn't seem to grok LTO partitioning

[Seymour, Shane M]

Hi Kai,

> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of "Kai Mäkisara (Kolumbus)"
> Sent: Friday, January 22, 2016 7:59 AM
> To: Seymour, Shane M
> Cc: Laurence Oberman; Emmanuel Florac; Laurence Oberman; linux-
> s...@vger.kernel.org
> Subject: 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.

I'd argue for consistency here in the current interface and that it should 
apply in the same way more on that below.

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

It may be worth noting (if you're going to update any documentation)
that isn't 100% accurate. You actually get one wrap in partition 1 and the
rest minus one wrap into partition 0. There is one wrap used as a guard
between the two partitions. The size given to a partition is rounded up
to the size of a wrap as well.

See https://docs.oracle.com/cd/E21419_04/en/LTO5_Vol3_E5b/LTO5_Vol3_E5b.pdf

Page 100 where it gives examples of how partition sizes are calculated on HP 
LTO5 drives.

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

How about using the fact the size is signed to indicate one slightly different
thing? I'm not sure if you'd call this using or abusing the fact that it's 
signed.

Make the default behavior for a positive size the same as the current
behavior for SCSI-2 (size applies to partition 1). If the size is negative then
the absolute value of the size applies to partition 0. That provides some
flexibility in choosing which partition the size applies to if it worked that
way for all devices.

With that you also get consistent behavior between tape drives without
having to know if the size will apply to partition 0 or partition 1 based on
the tape technology and you get the benefit of being able to set an
explicit size for partition 0 or partition 1.

You could overload the value of 0 as well to use FDP to choose the sizes
for the partitions (assuming 0 doesn't already have a side effect in
the code). Then you get whatever the tape drive wants to do.

Thanks
Shane

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

[Seymour, Shane M]

My applogies:

> It may be worth noting (if you're going to update any documentation) that
> isn't 100% accurate. You actually get one wrap in partition 1 and the rest
> minus one wrap into partition 0. There is one wrap used as a guard between
> the two partitions. The size given to a partition is rounded up to the size 
> of a
> wrap as well.
> 
> See
> https://docs.oracle.com/cd/E21419_04/en/LTO5_Vol3_E5b/LTO5_Vol3_E5b.
> pdf
> 
> Page 100 where it gives examples of how partition sizes are calculated on HP
> LTO5 drives.

I should have actually said:

You do get two wraps as a minimum in partition 1 and the rest minus two wraps
into partition 0. The extra two wraps are used as a guard between the two 
partitions
and all sizes will be rounded to a multiple of two wraps.

That was just to make it clear that partition sizes will always end up being a 
multiple
of 2x the wrap size and that there was some fixed overhead in partitioning an
LTO5+ drive (2 wraps).
N�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

[ANNOUNCE]: SCST 3.1 release

[Vladislav Bolkhovitin]

Hi All,

I'm glad to announce that SCST version 3.1 has just been released and available 
for
download from http://scst.sourceforge.net/downloads.html.

Highlights for this release:

 - Cluster support for SCSI reservations. This feature is essential for 
initiator-side
clustering approaches based on persistent reservations, e.g. the quorum disk
implementation in Windows Clustering.

 - Full support for VAAI or vStorage API for Array Integration: Extended Copy 
command
support has been added as well as performance of WRITE SAME and of Atomic Test 
& Set,
also known as COMPARE AND WRITE, has been improved.

 - T10-PI support has been added.

 - ALUA support has been improved: explicit ALUA (SET TARGET PORT GROUPS 
command) has
been added and DRBD compatibility has been improved.

 - SCST events user space infrastructure has been added, so now SCST can notify 
a user
space agent about important internal and fabric events.

 - QLogic target driver has been significantly improved.

SCST is alternative SCSI target stack for Linux. SCST allows creation of 
sophisticated
storage devices, which can provide advanced functionality, like replication, 
thin
provisioning, deduplication, high availability, automatic backup, etc. Majority 
of
recently developed SAN appliances, especially higher end ones, are SCST based. 
It might
well be that your favorite storage appliance running SCST in the firmware.

More info about SCST and its modules you can find on: 
http://scst.sourceforge.net

Thanks to all who made it happen, especially to SanDisk for the great support! 
All
above highlights development was supported by SanDisk.

Vlad

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