[PATCH 0/2] target: Simplify lu_gp handling code
This set changes the behavior added here: https://lkml.org/lkml/2009/2/12/22 We can simplify code paths dealing with lu_gp if we just have to handle (default_lu_gp vs explicit lu_gp), instead of (default_lu_gp vs explicit lu_gp vs NULL). The only consequence would be that showing logical unit group identifier (target_core_spc.c:358) in vpd83 info couldn't be disabled, and there didn't seem to be a reason I could come up with for why anyone would ever want to disable it - if you don't care, just don't look at it. Andy Grover (2): target: Ensure lu_gp_mem->lu_gp is never NULL target: Simplify target_core_store_alua_lu_gp drivers/target/target_core_alua.c | 6 ++--- drivers/target/target_core_configfs.c | 47 +++ 2 files changed, 16 insertions(+), 37 deletions(-) -- 1.8.5.3 -- 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
[PATCH 2/2] target: Simplify target_core_store_alua_lu_gp
If we know that lu_gp_mem->lu_gp will never be NULL, and "NULL" means
"set back to default_lu_gp" instead of really set to NULL (not allowed),
then we can simplify this function by not special-casing the
!lu_gp_new case, and setting lu_gp_new to default_lu_gp at the top.
Signed-off-by: Andy Grover
---
drivers/target/target_core_configfs.c | 46 ++-
1 file changed, 13 insertions(+), 33 deletions(-)
diff --git a/drivers/target/target_core_configfs.c
b/drivers/target/target_core_configfs.c
index 2a7a922..689cbf1 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1673,7 +1673,6 @@ static ssize_t target_core_store_alua_lu_gp(
struct t10_alua_lu_gp *lu_gp = NULL, *lu_gp_new = NULL;
struct t10_alua_lu_gp_member *lu_gp_mem;
unsigned char buf[LU_GROUP_NAME_BUF];
- int move = 0;
lu_gp_mem = dev->dev_alua_lu_gp_mem;
if (!lu_gp_mem)
@@ -1689,7 +1688,13 @@ static ssize_t target_core_store_alua_lu_gp(
* Any ALUA logical unit alias besides "NULL" means we will be
* making a new group association.
*/
- if (strcmp(strstrip(buf), "NULL")) {
+ if (!strcmp(strstrip(buf), "NULL")) {
+ /*
+* Clearing an existing lu_gp association, and going back to
+* the default group.
+*/
+ lu_gp_new = default_lu_gp;
+ } else {
/*
* core_alua_get_lu_gp_by_name() will increment reference to
* struct t10_alua_lu_gp. This reference is released with
@@ -1701,48 +1706,23 @@ static ssize_t target_core_store_alua_lu_gp(
}
spin_lock(&lu_gp_mem->lu_gp_mem_lock);
+
lu_gp = lu_gp_mem->lu_gp;
- if (lu_gp) {
- /*
-* Clearing an existing lu_gp association, and replacing
-* with NULL
-*/
- if (!lu_gp_new) {
- pr_debug("Target_Core_ConfigFS: Releasing %s/%s"
- " from ALUA LU Group: core/alua/lu_gps/%s, ID:"
- " %hu\n",
- config_item_name(&hba->hba_group.cg_item),
- config_item_name(&dev->dev_group.cg_item),
- config_item_name(&lu_gp->lu_gp_group.cg_item),
- lu_gp->lu_gp_id);
-
- __core_alua_drop_lu_gp_mem(lu_gp_mem, lu_gp);
- __core_alua_attach_lu_gp_mem(lu_gp_mem, default_lu_gp);
- spin_unlock(&lu_gp_mem->lu_gp_mem_lock);
-
- return count;
- }
- /*
-* Removing existing association of lu_gp_mem with lu_gp
-*/
- __core_alua_drop_lu_gp_mem(lu_gp_mem, lu_gp);
- move = 1;
- }
- /*
-* Associate lu_gp_mem with lu_gp_new.
-*/
+ __core_alua_drop_lu_gp_mem(lu_gp_mem, lu_gp);
__core_alua_attach_lu_gp_mem(lu_gp_mem, lu_gp_new);
+
spin_unlock(&lu_gp_mem->lu_gp_mem_lock);
pr_debug("Target_Core_ConfigFS: %s %s/%s to ALUA LU Group:"
" core/alua/lu_gps/%s, ID: %hu\n",
- (move) ? "Moving" : "Adding",
+ (lu_gp != default_lu_gp) ? "Moving" : "Adding",
config_item_name(&hba->hba_group.cg_item),
config_item_name(&dev->dev_group.cg_item),
config_item_name(&lu_gp_new->lu_gp_group.cg_item),
lu_gp_new->lu_gp_id);
- core_alua_put_lu_gp_from_name(lu_gp_new);
+ if (lu_gp_new != default_lu_gp)
+ core_alua_put_lu_gp_from_name(lu_gp_new);
return count;
}
--
1.8.5.3
--
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
[PATCH 1/2] target: Ensure lu_gp_mem->lu_gp is never NULL
->lu_gp should never be NULL, because if lu_gp_mem is not in an explicit group, it should be in default_lu_gp. Change target_core_store_alua_lu_gp to make this so. Also, fix a typo in a comment in that function. Finally, in core_alua_free_lu_gp, explicitly leave the old group before joining default_lu_gp, allowing simpler logic. Signed-off-by: Andy Grover --- drivers/target/target_core_alua.c | 6 ++ drivers/target/target_core_configfs.c | 3 ++- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c index 12da9b3..60a6a2c 100644 --- a/drivers/target/target_core_alua.c +++ b/drivers/target/target_core_alua.c @@ -1599,11 +1599,9 @@ void core_alua_free_lu_gp(struct t10_alua_lu_gp *lu_gp) * we want to re-associate a given lu_gp_mem with default_lu_gp. */ spin_lock(&lu_gp_mem->lu_gp_mem_lock); + __core_alua_drop_lu_gp_mem(lu_gp_mem, lu_gp); if (lu_gp != default_lu_gp) - __core_alua_attach_lu_gp_mem(lu_gp_mem, - default_lu_gp); - else - lu_gp_mem->lu_gp = NULL; + __core_alua_attach_lu_gp_mem(lu_gp_mem, default_lu_gp); spin_unlock(&lu_gp_mem->lu_gp_mem_lock); spin_lock(&lu_gp->lu_gp_lock); diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c index f0e85b1..2a7a922 100644 --- a/drivers/target/target_core_configfs.c +++ b/drivers/target/target_core_configfs.c @@ -1693,7 +1693,7 @@ static ssize_t target_core_store_alua_lu_gp( /* * core_alua_get_lu_gp_by_name() will increment reference to * struct t10_alua_lu_gp. This reference is released with -* core_alua_get_lu_gp_by_name below(). +* core_alua_put_lu_gp_from_name() below. */ lu_gp_new = core_alua_get_lu_gp_by_name(strstrip(buf)); if (!lu_gp_new) @@ -1717,6 +1717,7 @@ static ssize_t target_core_store_alua_lu_gp( lu_gp->lu_gp_id); __core_alua_drop_lu_gp_mem(lu_gp_mem, lu_gp); + __core_alua_attach_lu_gp_mem(lu_gp_mem, default_lu_gp); spin_unlock(&lu_gp_mem->lu_gp_mem_lock); return count; -- 1.8.5.3 -- 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 v10 0/4] ata: Add APM X-Gene SoC SATA host controller support
On Fri, Feb 14, 2014 at 01:44:08PM -0800, Loc Ho wrote: > Ok... I will look into Hans' patch again. To be sure that I am looking > at the correct patch, are you referring to this one: > > http://www.spinics.net/lists/linux-ide/msg47628.html Yeah, v5 is the latest. Thanks! -- tejun -- 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 v10 0/4] ata: Add APM X-Gene SoC SATA host controller support
Hi Tejun, > > On Thu, Feb 13, 2014 at 03:28:01PM -0800, Loc Ho wrote: >> 1. There are a number of errata that require workaround. Some can be >> fixed by adding broken flags while others are better to just wrap >> around the existent libahci library routines and not overly polluting >> the libahci routines. >> 2. There are additional controller programming sequences to configure. >> 2a. By default, RAM are powered down and require brought out of shutdown. >> 2b. The controller has an additional corresponding PHY part that needs >> to be programmed after PHY configuration. > > Have you looked at the latest patchset Hans posted? He added multiple > PHY support and split up init to three steps so that each platform > driver can mix and match as they see fit. Looking at xgene driver, > sure there are things specific to the driver but there also are > non-insignificant amount of boilerplate code and that's what I'm > primarily concerned about. It may be okay when you have two or three > drivers duplicating some code but it looks like we could have many > more and I *really* want to avoid the situation where the same piece > of code is copied over N times. In addition, frankly, not many people > except yourself would care about these drivers once they're merged and > many of these are gonna be painful to test making later refactoring a > lot harder. > Ok... I will look into Hans' patch again. To be sure that I am looking at the correct patch, are you referring to this one: http://www.spinics.net/lists/linux-ide/msg47628.html -Loc -- 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 16/16] scsi_dh_alua: Use workqueue for RTPG
On 01/31/14 10:30, Hannes Reinecke wrote:
> +static void alua_check(struct scsi_device *sdev)
> +{
> + struct alua_dh_data *h = get_alua_data(sdev);
> + struct alua_port_group *pg;
> +
> + if (!h)
> + return;
> +
> + rcu_read_lock();
> + pg = rcu_dereference(h->pg);
> + if (pg) {
> + kref_get(&pg->kref);
> + rcu_read_unlock();
> + alua_rtpg_queue(pg, sdev, NULL);
> + kref_put(&pg->kref, release_port_group);
> + } else
> + rcu_read_unlock();
> +}
>From the implementation it seems to me like the target port group data
is kept just as long as it is in use ? Has it been considered to keep
that data as long as the sdev exists instead ? I think that would result
in far fewer reference count manipulations and hence in code that is
easier to read and to verify.
Thanks,
Bart.
--
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 04/16] scsi_dh_alua: Make stpg synchronous
On 2/14/14 5:37 AM, Bart Van Assche wrote:
On 02/13/14 10:31, Hannes Reinecke wrote:
>On 02/12/2014 06:31 PM, Mike Christie wrote:
>>On 2/12/14 10:26 AM, Mike Christie wrote:
>>>On 2/12/14 10:11 AM, Mike Christie wrote:
On 2/12/14 9:29 AM, Hannes Reinecke wrote:
>On 02/07/2014 02:54 AM, Mike Christie wrote:
>>On 02/06/2014 07:24 PM, Mike Christie wrote:
>>>On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
We should be issuing STPG synchronously as we need to
evaluate the return code on failure.
Signed-off-by: Hannes Reinecke
>>>
>>>I think we need to also make dm-mpath.c use a non-ordered
>>>workqueue
>>>for
>>>kmpath_handlerd. With this patch and the current ordered
>>>workqueue in
>>>dm-mpath I think we will only be able to do one STPG at a time. I
>>>think
>>>if we use a normal old non-ordered workqueue then we would be
>>>limited to
>>>have max_active STPGs executing.
>>
>>I goofed and commented in the order I saw the patches:) I take
>>this
>>comment back for this patch, because I see in 16/16 you added a
>>new
>>workqueue to scsi_dh_alua to do rtpgs and stpgs.
>>
>>For 16/16 though, do we want to make kmpath_aluad a non single
>>threaded
>>workqueue? It looks like max_active for single threaded is only
>>one
>>work
>>at a time too.
>>
>Well, that was by intention.
>
>The workqueue will be triggered very infrequently (basically for
>every path switch).
>For implicit ALUA we just need to issue a RTPG to get the new path
>status; there we might be suffering from single threaded behaviour.
>But we need to issue it only once and it should be processed
>reasonably fast.
>For explicit ALUA we'll have to send an STPG, which has potentially
>system-wide implications. So sending several to (supposedly)
>different targets might actually be contraproductive, as the first
>might have already set the status for the second call.
>Here we most definitely_want_ serialisation to avoid
>superfluous STPGs.
>
What target is this?
For our target it adds a regression. It only affects devices on
the same
port group. We then have multiple groups. Before the patch, we could
failover/failback multiple devices in parallel. To do 64 devices
it took
about 3 seconds. With your patch it takes around 3 minutes.
>>>
>>>Also, with your pg change patch, I think we can serialize based on
>>>group
>>>and it will do what you want and also allow us to do STPGs to
>>>different
>>>groups in parallel.
>>
>>I guess that would work for me, but I think if you had the same
>>target port in multiple port groups then you could hit the issue you
>>described, right?
>>
>Yes, we might. But it's worth a shot anyway.
>
>So to alleviate all this, this patch:
>
>diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
>b/drivers/scsi/device_ha
>ndler/scsi_dh_alua.c
>index 569af9d..46cc1ef 100644
>--- a/drivers/scsi/device_handler/scsi_dh_alua.c
>+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>@@ -1353,7 +1353,7 @@ static int __init alua_init(void)
> {
> int r;
>
>- kmpath_aluad = create_singlethread_workqueue("kmpath_aluad");
>+ kmpath_aluad = create_workqueue("kmpath_aluad");
> if (!kmpath_aluad) {
> printk(KERN_ERR "kmpath_aluad creation failed.\n");
> return -EINVAL;
>
>should be sufficient, right?
I think this will only be sufficient if there are more CPU threads
available than the number of LUNs for which an STPG has to be issued.
Ah shoot, you reminded me of some other issue. With the above command we
only can do 1 unit of work at a time. With the alloc_workqueue example I
sent in the other mail, I think it would create a new problem.
alloc_workqueue with max_active=0 looks like it has the wq code
dynamically create worker threads as needed, so with that we could end
up with a lot of threads and can failover lots of devices (512 with the
current max) in parallel. The threads will even get reaped when they are
not needed. However, trying to create lots of threads at failover time
might have issues, and in general creating 32 or 64 threads or more is
probably not something we want to do.
--
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 07/16] scsi_dh_alua: Use separate alua_port_group structure
On 02/14/14 17:12, Hannes Reinecke wrote: > The reason I did this was that I don't have to allocate memory > unnecesarily. > If I move the allocation out of the spinlock I'll have to recheck > the list upon insertion to ensure no duplicates are present. > Upon hitting a duplicate I would have to release the memory again. > > I do agree that GFP_KERNEL is probably not the correct thing here; > so either I move it to GFP_ATOMIC or we may run into a chance of > having to release the memory again afterwards. > > Personally I'm inclined to use GFP_ATOMIC, but I'm not sure what'd > be best. > > What would you suggest here? I have not yet had a chance to audit all code where list_lock is used. Is sleeping allowed in each function where list_lock is used ? If so, how about using a mutex instead of a spinlock ? Bart. -- 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 04/16] scsi_dh_alua: Make stpg synchronous
On 2/13/14 3:31 AM, Hannes Reinecke wrote:
On 02/12/2014 06:31 PM, Mike Christie wrote:
On 2/12/14 10:26 AM, Mike Christie wrote:
On 2/12/14 10:11 AM, Mike Christie wrote:
On 2/12/14 9:29 AM, Hannes Reinecke wrote:
On 02/07/2014 02:54 AM, Mike Christie wrote:
On 02/06/2014 07:24 PM, Mike Christie wrote:
On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
We should be issuing STPG synchronously as we need to
evaluate the return code on failure.
Signed-off-by: Hannes Reinecke
I think we need to also make dm-mpath.c use a non-ordered
workqueue
for
kmpath_handlerd. With this patch and the current ordered
workqueue in
dm-mpath I think we will only be able to do one STPG at a time. I
think
if we use a normal old non-ordered workqueue then we would be
limited to
have max_active STPGs executing.
I goofed and commented in the order I saw the patches :) I take
this
comment back for this patch, because I see in 16/16 you added a
new
workqueue to scsi_dh_alua to do rtpgs and stpgs.
For 16/16 though, do we want to make kmpath_aluad a non single
threaded
workqueue? It looks like max_active for single threaded is only
one
work
at a time too.
Well, that was by intention.
The workqueue will be triggered very infrequently (basically for
every path switch).
For implicit ALUA we just need to issue a RTPG to get the new path
status; there we might be suffering from single threaded behaviour.
But we need to issue it only once and it should be processed
reasonably fast.
For explicit ALUA we'll have to send an STPG, which has potentially
system-wide implications. So sending several to (supposedly)
different targets might actually be contraproductive, as the first
might have already set the status for the second call.
Here we most definitely _want_ serialisation to avoid
superfluous STPGs.
What target is this?
For our target it adds a regression. It only affects devices on
the same
port group. We then have multiple groups. Before the patch, we could
failover/failback multiple devices in parallel. To do 64 devices
it took
about 3 seconds. With your patch it takes around 3 minutes.
Also, with your pg change patch, I think we can serialize based on
group
and it will do what you want and also allow us to do STPGs to
different
groups in parallel.
I guess that would work for me, but I think if you had the same
target port in multiple port groups then you could hit the issue you
described, right?
Yes, we might. But it's worth a shot anyway.
So to alleviate all this, this patch:
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
b/drivers/scsi/device_ha
ndler/scsi_dh_alua.c
index 569af9d..46cc1ef 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -1353,7 +1353,7 @@ static int __init alua_init(void)
{
int r;
- kmpath_aluad = create_singlethread_workqueue("kmpath_aluad");
+ kmpath_aluad = create_workqueue("kmpath_aluad");
if (!kmpath_aluad) {
printk(KERN_ERR "kmpath_aluad creation failed.\n");
return -EINVAL;
should be sufficient, right?
I think you need to do
alloc_workqueue("kmpath_aluad", WQ_MEM_RECLAIM, 0);
If you use create_workqueue then it sets max_active to 1 like is done
for create_singlethread_workqueue.
Could you test and see if it makes a difference?
Testing both.
--
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: 3x check of "Caching mode page"
> "Toralf" == Toralf Förster writes: Toralf> I'm just wondering why it happens 3 times in a row : http://marc.info/?l=linux-scsi&m=138879119205889 James: Ping on this patch. -- Martin K. Petersen Oracle Linux Engineering -- 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 07/16] scsi_dh_alua: Use separate alua_port_group structure
On 02/14/2014 12:56 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> +static void release_port_group(struct kref *kref)
>> +{
>> +struct alua_port_group *pg;
>> +
>> +pg = container_of(kref, struct alua_port_group, kref);
>> +printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);
>> +spin_lock(&port_group_lock);
>> +list_del(&pg->node);
>> +spin_unlock(&port_group_lock);
>> +if (pg->buff && pg->inq != pg->buff)
>> +kfree(pg->buff);
>> +kfree(pg);
>> +}
>
> Does that message really need level "WARNING" ?
No, probably not.
>
>> +sdev_printk(KERN_INFO, sdev,
>> +"%s: port group %02x rel port %02x\n",
>> +ALUA_DH_NAME, group_id, h->rel_port);
>> +spin_lock(&port_group_lock);
>> +pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
>> +if (!pg) {
>> +sdev_printk(KERN_WARNING, sdev,
>> +"%s: kzalloc port group failed\n",
>> +ALUA_DH_NAME);
>> +/* Temporary failure, bypass */
>> +spin_unlock(&port_group_lock);
>> +err = SCSI_DH_DEV_TEMP_BUSY;
>> +goto out;
>> +}
>> +pg->group_id = group_id;
>> +pg->buff = pg->inq;
>> +pg->bufflen = ALUA_INQUIRY_SIZE;
>> +pg->tpgs = h->tpgs;
>> +pg->state = TPGS_STATE_OPTIMIZED;
>> +pg->flags = h->flags;
>> +kref_init(&pg->kref);
>> +list_add(&pg->node, &port_group_list);
>> +h->pg = pg;
>> +spin_unlock(&port_group_lock);
>> +err = SCSI_DH_OK;
>
> A GFP_KERNEL allocation with a spin lock held ?? Please move that
> allocation outside the critical section and also the code for
> initializing *pg. What's not clear to me is why no uniqueness check is
> performed before invoking list_add() ? Does that mean that information
> for the same port group ID can end up multiple times in the
> port_group_list ? Such a uniqueness check can only be performed if a
> storage array identification is available so patches 07 and 08 may have
> to be swapped.
>
The reason I did this was that I don't have to allocate memory
unnecesarily.
If I move the allocation out of the spinlock I'll have to recheck
the list upon insertion to ensure no duplicates are present.
Upon hitting a duplicate I would have to release the memory again.
I do agree that GFP_KERNEL is probably not the correct thing here;
so either I move it to GFP_ATOMIC or we may run into a chance of
having to release the memory again afterwards.
Personally I'm inclined to use GFP_ATOMIC, but I'm not sure what'd
be best.
What would you suggest here?
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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
3x check of "Caching mode page"
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 I'm just wondering why it happens 3 times in a row : Feb 14 16:12:53 n22 kernel: usb 1-1.2: new high-speed USB device number 7 using ehci-pci Feb 14 16:12:53 n22 kernel: usb 1-1.2: New USB device found, idVendor=04b4, idProduct=6830 Feb 14 16:12:53 n22 kernel: usb 1-1.2: New USB device strings: Mfr=56, Product=72, SerialNumber=91 Feb 14 16:12:53 n22 kernel: usb 1-1.2: Product: Lacie Mobile Drive Feb 14 16:12:53 n22 kernel: usb 1-1.2: Manufacturer: Lacie Group. SA Feb 14 16:12:53 n22 kernel: usb 1-1.2: SerialNumber: DEF10D5E95F5 Feb 14 16:12:53 n22 kernel: usb-storage 1-1.2:1.0: USB Mass Storage device detected Feb 14 16:12:53 n22 kernel: scsi9 : usb-storage 1-1.2:1.0 Feb 14 16:12:54 n22 kernel: scsi 9:0:0:0: Direct-Access SAMSUNG HM160JC PQ: 0 ANSI: 0 Feb 14 16:12:54 n22 kernel: sd 9:0:0:0: Attached scsi generic sg3 type 0 Feb 14 16:12:54 n22 kernel: sd 9:0:0:0: [sdc] 312581808 512-byte logical blocks: (160 GB/149 GiB) Feb 14 16:12:54 n22 kernel: sd 9:0:0:0: [sdc] Write Protect is off Feb 14 16:12:54 n22 kernel: sd 9:0:0:0: [sdc] Mode Sense: 27 00 00 00 Feb 14 16:12:54 n22 kernel: sd 9:0:0:0: [sdc] No Caching mode page found Feb 14 16:12:54 n22 kernel: sd 9:0:0:0: [sdc] Assuming drive cache: write through Feb 14 16:12:54 n22 kernel: sd 9:0:0:0: [sdc] No Caching mode page found Feb 14 16:12:54 n22 kernel: sd 9:0:0:0: [sdc] Assuming drive cache: write through Feb 14 16:12:54 n22 kernel: sdc: sdc1 Feb 14 16:12:54 n22 kernel: sd 9:0:0:0: [sdc] No Caching mode page found Feb 14 16:12:54 n22 kernel: sd 9:0:0:0: [sdc] Assuming drive cache: write through Feb 14 16:12:54 n22 kernel: sd 9:0:0:0: [sdc] Attached SCSI disk - -- MfG/Sincerely Toralf Förster pgp finger print:1A37 6F99 4A9D 026F 13E2 4DCF C4EA CDDE 0076 E94E -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iF4EAREIAAYFAlL+PcQACgkQxOrN3gB26U43VQD+Lhk1W1u2Mn7ppqI38i5jzFLT i8NUkgjNx9Kz5EC/oPwA/iNdNIwnh8Gnpixd5E7W1ffXlJ8oWq9Wo1h8DZuNo6vF =6C32 -END PGP SIGNATURE- -- 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 v10 0/4] ata: Add APM X-Gene SoC SATA host controller support
Hello, Loc. On Thu, Feb 13, 2014 at 03:28:01PM -0800, Loc Ho wrote: > 1. There are a number of errata that require workaround. Some can be > fixed by adding broken flags while others are better to just wrap > around the existent libahci library routines and not overly polluting > the libahci routines. > 2. There are additional controller programming sequences to configure. > 2a. By default, RAM are powered down and require brought out of shutdown. > 2b. The controller has an additional corresponding PHY part that needs > to be programmed after PHY configuration. Have you looked at the latest patchset Hans posted? He added multiple PHY support and split up init to three steps so that each platform driver can mix and match as they see fit. Looking at xgene driver, sure there are things specific to the driver but there also are non-insignificant amount of boilerplate code and that's what I'm primarily concerned about. It may be okay when you have two or three drivers duplicating some code but it looks like we could have many more and I *really* want to avoid the situation where the same piece of code is copied over N times. In addition, frankly, not many people except yourself would care about these drivers once they're merged and many of these are gonna be painful to test making later refactoring a lot harder. > 2c. The controller requires extra programming sequence for the > hardreset due to errata. > 2d. For the IO flush, it requires additional memory resources. Sure, you'll need to override good parts of the driver. What I'm saying is please try to reuse whatever you can. If that takes refactoring and librarize ahci_platform, please do so and I do see healthy chunk of duplicated code in the init path. Please take a look at Hans' patches and if necessary work with him so that your driver can be part of the refactoring. Thanks. -- tejun -- 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: Instructions to release your payment.
Attention Sir, Following an application brought, seeking the release of your due payment through British bank, I am directed to inform you that the application has been approved and Natwest bank of London has been mandated to make transfer of your payment to the bank account you will nominate. Please kindly reply for immediate release of your US$6.2 Million to you nominates account. Sir, for the avoidance of doubts, reconfirm the following information to me to enable us forward same to Natwest bank to contact you for your payment. Name: Address: Tel/Fax No.: Nationality: Occupation: Date of birth: As soon as I received the above information, I will forward them to Natwest bank to contact you for your approved payment. Please see in the attachment, letter I wrote to Natwest bank informing them of the transaction Yours faithfully Dr.William Davies Chairman,British Banking Regulatory Board. -- 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 08/16] scsi_dh_alua: parse target device id
On 02/14/2014 01:09 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> +if (!target_id_size) {
>> +/* Check for EMC Clariion extended inquiry */
>> +if (!strncmp(sdev->vendor, "DGC ", 8) &&
>> +sdev->inquiry_len > 160) {
>> +target_id_size = sdev->inquiry[160];
>> +target_id = sdev->inquiry + 161;
>> +strcpy(target_id_str, "emc.");
>> +memcpy(target_id_str + 4, target_id, target_id_size);
>> +}
>> +/* Check for HP EVA extended inquiry */
>> +if (!strncmp(sdev->vendor, "HP ", 8) &&
>> +!strncmp(sdev->model, "HSV", 3) &&
>> +sdev->inquiry_len > 170) {
>> +target_id_size = 16;
>> +target_id = sdev->inquiry + 154;
>> +strcpy(target_id_str, "naa.");
>> +memcpy(target_id_str + 4, target_id, target_id_size);
>> +}
>> +}
>
> Being able to identify a storage array unambiguously is essential for
> the new ALUA device handler algorithm introduced by this patch series.
> What if a new storage array is introduced that is not covered by one of
> the heuristics in this patch ? Has it been considered to let storage
> array identification occur in user space instead of in the kernel ?
>
As noted in the other mail, if we cannot decipher the array
identifier the whole thing degrades into one pg per sdev.
IE we cannot bunch RTPG / STPG for those.
The idea here is that those arrays do not have any limitations,
so that no special treatment is required.
Adding identifiers from userspace are a bit tricky; there is no
actual sysfs structure for the device handler.
Which we would need if we were to use these tricks.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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 07/16] scsi_dh_alua: Use separate alua_port_group structure
On 02/14/2014 12:56 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> +static void release_port_group(struct kref *kref)
>> +{
>> +struct alua_port_group *pg;
>> +
>> +pg = container_of(kref, struct alua_port_group, kref);
>> +printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);
>> +spin_lock(&port_group_lock);
>> +list_del(&pg->node);
>> +spin_unlock(&port_group_lock);
>> +if (pg->buff && pg->inq != pg->buff)
>> +kfree(pg->buff);
>> +kfree(pg);
>> +}
>
> Does that message really need level "WARNING" ?
>
>> +sdev_printk(KERN_INFO, sdev,
>> +"%s: port group %02x rel port %02x\n",
>> +ALUA_DH_NAME, group_id, h->rel_port);
>> +spin_lock(&port_group_lock);
>> +pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
>> +if (!pg) {
>> +sdev_printk(KERN_WARNING, sdev,
>> +"%s: kzalloc port group failed\n",
>> +ALUA_DH_NAME);
>> +/* Temporary failure, bypass */
>> +spin_unlock(&port_group_lock);
>> +err = SCSI_DH_DEV_TEMP_BUSY;
>> +goto out;
>> +}
>> +pg->group_id = group_id;
>> +pg->buff = pg->inq;
>> +pg->bufflen = ALUA_INQUIRY_SIZE;
>> +pg->tpgs = h->tpgs;
>> +pg->state = TPGS_STATE_OPTIMIZED;
>> +pg->flags = h->flags;
>> +kref_init(&pg->kref);
>> +list_add(&pg->node, &port_group_list);
>> +h->pg = pg;
>> +spin_unlock(&port_group_lock);
>> +err = SCSI_DH_OK;
>
> A GFP_KERNEL allocation with a spin lock held ?? Please move that
> allocation outside the critical section and also the code for
> initializing *pg. What's not clear to me is why no uniqueness check is
> performed before invoking list_add() ? Does that mean that information
> for the same port group ID can end up multiple times in the
> port_group_list ? Such a uniqueness check can only be performed if a
> storage array identification is available so patches 07 and 08 may have
> to be swapped.
>
With this patch I'm allocating one pg per sdev, so I won't need any
uniqueness check here (sdevs are already unique).
I only need to check for uniqueness once I have array identifiers.
But yes, the allocation can be moved to outside the critical section.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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 08/16] scsi_dh_alua: parse target device id
On 01/31/14 10:29, Hannes Reinecke wrote:
> + if (!target_id_size) {
> + /* Check for EMC Clariion extended inquiry */
> + if (!strncmp(sdev->vendor, "DGC ", 8) &&
> + sdev->inquiry_len > 160) {
> + target_id_size = sdev->inquiry[160];
> + target_id = sdev->inquiry + 161;
> + strcpy(target_id_str, "emc.");
> + memcpy(target_id_str + 4, target_id, target_id_size);
> + }
> + /* Check for HP EVA extended inquiry */
> + if (!strncmp(sdev->vendor, "HP ", 8) &&
> + !strncmp(sdev->model, "HSV", 3) &&
> + sdev->inquiry_len > 170) {
> + target_id_size = 16;
> + target_id = sdev->inquiry + 154;
> + strcpy(target_id_str, "naa.");
> + memcpy(target_id_str + 4, target_id, target_id_size);
> + }
> + }
Being able to identify a storage array unambiguously is essential for
the new ALUA device handler algorithm introduced by this patch series.
What if a new storage array is introduced that is not covered by one of
the heuristics in this patch ? Has it been considered to let storage
array identification occur in user space instead of in the kernel ?
Thanks,
Bart.
--
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 07/16] scsi_dh_alua: Use separate alua_port_group structure
On 01/31/14 10:29, Hannes Reinecke wrote:
> +static void release_port_group(struct kref *kref)
> +{
> + struct alua_port_group *pg;
> +
> + pg = container_of(kref, struct alua_port_group, kref);
> + printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);
> + spin_lock(&port_group_lock);
> + list_del(&pg->node);
> + spin_unlock(&port_group_lock);
> + if (pg->buff && pg->inq != pg->buff)
> + kfree(pg->buff);
> + kfree(pg);
> +}
Does that message really need level "WARNING" ?
> + sdev_printk(KERN_INFO, sdev,
> + "%s: port group %02x rel port %02x\n",
> + ALUA_DH_NAME, group_id, h->rel_port);
> + spin_lock(&port_group_lock);
> + pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
> + if (!pg) {
> + sdev_printk(KERN_WARNING, sdev,
> + "%s: kzalloc port group failed\n",
> + ALUA_DH_NAME);
> + /* Temporary failure, bypass */
> + spin_unlock(&port_group_lock);
> + err = SCSI_DH_DEV_TEMP_BUSY;
> + goto out;
> + }
> + pg->group_id = group_id;
> + pg->buff = pg->inq;
> + pg->bufflen = ALUA_INQUIRY_SIZE;
> + pg->tpgs = h->tpgs;
> + pg->state = TPGS_STATE_OPTIMIZED;
> + pg->flags = h->flags;
> + kref_init(&pg->kref);
> + list_add(&pg->node, &port_group_list);
> + h->pg = pg;
> + spin_unlock(&port_group_lock);
> + err = SCSI_DH_OK;
A GFP_KERNEL allocation with a spin lock held ?? Please move that
allocation outside the critical section and also the code for
initializing *pg. What's not clear to me is why no uniqueness check is
performed before invoking list_add() ? Does that mean that information
for the same port group ID can end up multiple times in the
port_group_list ? Such a uniqueness check can only be performed if a
storage array identification is available so patches 07 and 08 may have
to be swapped.
Bart.
--
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 04/16] scsi_dh_alua: Make stpg synchronous
On 02/14/2014 12:37 PM, Bart Van Assche wrote:
> On 02/13/14 10:31, Hannes Reinecke wrote:
>> On 02/12/2014 06:31 PM, Mike Christie wrote:
>>> On 2/12/14 10:26 AM, Mike Christie wrote:
On 2/12/14 10:11 AM, Mike Christie wrote:
> On 2/12/14 9:29 AM, Hannes Reinecke wrote:
>> On 02/07/2014 02:54 AM, Mike Christie wrote:
>>> On 02/06/2014 07:24 PM, Mike Christie wrote:
On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
> We should be issuing STPG synchronously as we need to
> evaluate the return code on failure.
>
> Signed-off-by: Hannes Reinecke
I think we need to also make dm-mpath.c use a non-ordered
workqueue
for
kmpath_handlerd. With this patch and the current ordered
workqueue in
dm-mpath I think we will only be able to do one STPG at a time. I
think
if we use a normal old non-ordered workqueue then we would be
limited to
have max_active STPGs executing.
>>>
>>> I goofed and commented in the order I saw the patches :) I take
>>> this
>>> comment back for this patch, because I see in 16/16 you added a
>>> new
>>> workqueue to scsi_dh_alua to do rtpgs and stpgs.
>>>
>>> For 16/16 though, do we want to make kmpath_aluad a non single
>>> threaded
>>> workqueue? It looks like max_active for single threaded is only
>>> one
>>> work
>>> at a time too.
>>>
>> Well, that was by intention.
>>
>> The workqueue will be triggered very infrequently (basically for
>> every path switch).
>> For implicit ALUA we just need to issue a RTPG to get the new path
>> status; there we might be suffering from single threaded behaviour.
>> But we need to issue it only once and it should be processed
>> reasonably fast.
>> For explicit ALUA we'll have to send an STPG, which has potentially
>> system-wide implications. So sending several to (supposedly)
>> different targets might actually be contraproductive, as the first
>> might have already set the status for the second call.
>> Here we most definitely _want_ serialisation to avoid
>> superfluous STPGs.
>>
>
> What target is this?
>
> For our target it adds a regression. It only affects devices on
> the same
> port group. We then have multiple groups. Before the patch, we could
> failover/failback multiple devices in parallel. To do 64 devices
> it took
> about 3 seconds. With your patch it takes around 3 minutes.
>
Also, with your pg change patch, I think we can serialize based on
group
and it will do what you want and also allow us to do STPGs to
different
groups in parallel.
>>>
>>> I guess that would work for me, but I think if you had the same
>>> target port in multiple port groups then you could hit the issue you
>>> described, right?
>>>
>> Yes, we might. But it's worth a shot anyway.
>>
>> So to alleviate all this, this patch:
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
>> b/drivers/scsi/device_ha
>> ndler/scsi_dh_alua.c
>> index 569af9d..46cc1ef 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>> @@ -1353,7 +1353,7 @@ static int __init alua_init(void)
>> {
>> int r;
>>
>> - kmpath_aluad = create_singlethread_workqueue("kmpath_aluad");
>> + kmpath_aluad = create_workqueue("kmpath_aluad");
>> if (!kmpath_aluad) {
>> printk(KERN_ERR "kmpath_aluad creation failed.\n");
>> return -EINVAL;
>>
>> should be sufficient, right?
>
> I think this will only be sufficient if there are more CPU threads
> available than the number of LUNs for which an STPG has to be issued.
> My preference is also to keep the asynchronous invocation of the STPG
> commands to avoid introducing a regression if the number of LUNs that
> has to be failed over is large. Has it been considered to add the "if
> (err == SCSI_DH_RETRY) err = alua_rtpg(sdev, h)" code in the
> stpg_endio() handler, together with a counter mechanism that prevents
> infinite retries ? And if a storage array reports that the target portal
> group state is transitioning, shouldn't the retry be delayed instead of
> submitting it immediately ?
>
Errm. I thought I was doing that ...
if (pg->flags & ALUA_PG_RUN_RTPG) {
spin_unlock_irqrestore(&pg->rtpg_lock, flags);
err = alua_rtpg(sdev, pg);
if (err == SCSI_DH_RETRY) {
queue_delayed_work(kmpath_aluad, &pg->rtpg_work,
pg->interval * HZ);
return;
}
spin_lock_irqsave(&pg->rtpg_lock, flags);
pg->flags &= ~ALUA_PG_RUN_RTPG;
}
As for making stpg asynchronous again; I haven't thought about it as
of now, but it seems li
Re: [PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry
On 01/31/14 10:29, Hannes Reinecke wrote:
> - len = (h->buff[2] << 8) + h->buff[3] + 4;
> - if (len > h->bufflen) {
> + len = (buff[2] << 8) + buff[3] + 4;
> + if (len > bufflen) {
I think this code will become easier to read when using
get_unaligned_be16() for extracting the length from the VPD response.
Bart.
--
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 04/16] scsi_dh_alua: Make stpg synchronous
On 02/14/2014 12:27 PM, Bart Van Assche wrote: > On 01/31/14 10:29, Hannes Reinecke wrote: >> -memset(h->buff, 0, stpg_len); >> -h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f; >> -h->buff[6] = (h->group_id >> 8) & 0xff; >> -h->buff[7] = h->group_id & 0xff; >> +memset(stpg_data, 0, stpg_len); >> +stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f; >> +stpg_data[6] = (group_id >> 8) & 0xff; >> +stpg_data[7] = group_id & 0xff; > > Any reason why put_unaligned_be16() is not used here ? > No specific one. Will be fixing it up in the next round. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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 04/16] scsi_dh_alua: Make stpg synchronous
On 02/13/14 10:31, Hannes Reinecke wrote:
> On 02/12/2014 06:31 PM, Mike Christie wrote:
>> On 2/12/14 10:26 AM, Mike Christie wrote:
>>> On 2/12/14 10:11 AM, Mike Christie wrote:
On 2/12/14 9:29 AM, Hannes Reinecke wrote:
> On 02/07/2014 02:54 AM, Mike Christie wrote:
>> On 02/06/2014 07:24 PM, Mike Christie wrote:
>>> On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
We should be issuing STPG synchronously as we need to
evaluate the return code on failure.
Signed-off-by: Hannes Reinecke
>>>
>>> I think we need to also make dm-mpath.c use a non-ordered
>>> workqueue
>>> for
>>> kmpath_handlerd. With this patch and the current ordered
>>> workqueue in
>>> dm-mpath I think we will only be able to do one STPG at a time. I
>>> think
>>> if we use a normal old non-ordered workqueue then we would be
>>> limited to
>>> have max_active STPGs executing.
>>
>> I goofed and commented in the order I saw the patches :) I take
>> this
>> comment back for this patch, because I see in 16/16 you added a
>> new
>> workqueue to scsi_dh_alua to do rtpgs and stpgs.
>>
>> For 16/16 though, do we want to make kmpath_aluad a non single
>> threaded
>> workqueue? It looks like max_active for single threaded is only
>> one
>> work
>> at a time too.
>>
> Well, that was by intention.
>
> The workqueue will be triggered very infrequently (basically for
> every path switch).
> For implicit ALUA we just need to issue a RTPG to get the new path
> status; there we might be suffering from single threaded behaviour.
> But we need to issue it only once and it should be processed
> reasonably fast.
> For explicit ALUA we'll have to send an STPG, which has potentially
> system-wide implications. So sending several to (supposedly)
> different targets might actually be contraproductive, as the first
> might have already set the status for the second call.
> Here we most definitely _want_ serialisation to avoid
> superfluous STPGs.
>
What target is this?
For our target it adds a regression. It only affects devices on
the same
port group. We then have multiple groups. Before the patch, we could
failover/failback multiple devices in parallel. To do 64 devices
it took
about 3 seconds. With your patch it takes around 3 minutes.
>>>
>>> Also, with your pg change patch, I think we can serialize based on
>>> group
>>> and it will do what you want and also allow us to do STPGs to
>>> different
>>> groups in parallel.
>>
>> I guess that would work for me, but I think if you had the same
>> target port in multiple port groups then you could hit the issue you
>> described, right?
>>
> Yes, we might. But it's worth a shot anyway.
>
> So to alleviate all this, this patch:
>
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
> b/drivers/scsi/device_ha
> ndler/scsi_dh_alua.c
> index 569af9d..46cc1ef 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -1353,7 +1353,7 @@ static int __init alua_init(void)
> {
> int r;
>
> - kmpath_aluad = create_singlethread_workqueue("kmpath_aluad");
> + kmpath_aluad = create_workqueue("kmpath_aluad");
> if (!kmpath_aluad) {
> printk(KERN_ERR "kmpath_aluad creation failed.\n");
> return -EINVAL;
>
> should be sufficient, right?
I think this will only be sufficient if there are more CPU threads
available than the number of LUNs for which an STPG has to be issued.
My preference is also to keep the asynchronous invocation of the STPG
commands to avoid introducing a regression if the number of LUNs that
has to be failed over is large. Has it been considered to add the "if
(err == SCSI_DH_RETRY) err = alua_rtpg(sdev, h)" code in the
stpg_endio() handler, together with a counter mechanism that prevents
infinite retries ? And if a storage array reports that the target portal
group state is transitioning, shouldn't the retry be delayed instead of
submitting it immediately ?
Bart.
--
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 03/16] scsi_dh_alua: Pass buffer as function argument
On 02/14/2014 12:22 PM, Bart Van Assche wrote: > On 01/31/14 10:29, Hannes Reinecke wrote: >> -rq->cmd[6] = (h->bufflen >> 24) & 0xff; >> -rq->cmd[7] = (h->bufflen >> 16) & 0xff; >> -rq->cmd[8] = (h->bufflen >> 8) & 0xff; >> -rq->cmd[9] = h->bufflen & 0xff; >> +rq->cmd[6] = (bufflen >> 24) & 0xff; >> +rq->cmd[7] = (bufflen >> 16) & 0xff; >> +rq->cmd[8] = (bufflen >> 8) & 0xff; >> +rq->cmd[9] = bufflen & 0xff; > > Since you are changing this code, please use put_unaligned_be32() for > storing bufflen in the CDB. > Ok. Will be fixing it up in the next round. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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 01/16] scsi_dh_alua: Improve error handling
On 02/14/2014 12:16 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> - * alua_stpg - Evaluate SET TARGET GROUP STATES
>> + * stpg_endio - Evaluate SET TARGET GROUP STATES
>
> Great that you are fixing the function header of stpg_endio(). But
> please consider to use here the official terminology from SPC ("SET
> TARGET PORT GROUPS").
>
Ok. Will be fixing it for the next round.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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 04/16] scsi_dh_alua: Make stpg synchronous
On 01/31/14 10:29, Hannes Reinecke wrote: > - memset(h->buff, 0, stpg_len); > - h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f; > - h->buff[6] = (h->group_id >> 8) & 0xff; > - h->buff[7] = h->group_id & 0xff; > + memset(stpg_data, 0, stpg_len); > + stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f; > + stpg_data[6] = (group_id >> 8) & 0xff; > + stpg_data[7] = group_id & 0xff; Any reason why put_unaligned_be16() is not used here ? Bart. -- 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 03/16] scsi_dh_alua: Pass buffer as function argument
On 01/31/14 10:29, Hannes Reinecke wrote: > - rq->cmd[6] = (h->bufflen >> 24) & 0xff; > - rq->cmd[7] = (h->bufflen >> 16) & 0xff; > - rq->cmd[8] = (h->bufflen >> 8) & 0xff; > - rq->cmd[9] = h->bufflen & 0xff; > + rq->cmd[6] = (bufflen >> 24) & 0xff; > + rq->cmd[7] = (bufflen >> 16) & 0xff; > + rq->cmd[8] = (bufflen >> 8) & 0xff; > + rq->cmd[9] = bufflen & 0xff; Since you are changing this code, please use put_unaligned_be32() for storing bufflen in the CDB. Bart. -- 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 01/16] scsi_dh_alua: Improve error handling
On 01/31/14 10:29, Hannes Reinecke wrote:
> - * alua_stpg - Evaluate SET TARGET GROUP STATES
> + * stpg_endio - Evaluate SET TARGET GROUP STATES
Great that you are fixing the function header of stpg_endio(). But
please consider to use here the official terminology from SPC ("SET
TARGET PORT GROUPS").
Bart.
--
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] : pm80xx : fix problem of pm8001_work_fn reseting uncorrect phy device
On Friday, February 14, 2014 4:01 PM, XinHong Zhu wrote:
> If a phy device is removed ,the device can get error of I/O and HBA maybe
> receieve IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS of event which causes
pm8001_work_fn
> to reset the phy device ,but in pm8001_task_exec don't assign a value for
> field device of ccb and in other case a ccb used have device field set ,
when
> ccb is freeed the field device of the ccb don't be set NULL.So there is
> possibility of getting another devcie reset in fun of mpi_ssp_completion
.Also
> there are another way to solve problem by adding following code in
> mpi_SSP_completion:
> pm8001_dev = t->dev->lldd_dev;
> Signed-off-by: zhuxh
> ---
> drivers/scsi/pm8001/pm8001_sas.c |1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c
> b/drivers/scsi/pm8001/pm8001_sas.c
> index f50ac44..f0ea5db 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -434,6 +434,7 @@ static int pm8001_task_exec(struct sas_task *task,
const
> int num,
> ccb->n_elem = n_elem;
> ccb->ccb_tag = tag;
> ccb->task = t;
> + ccb->device = pm8001_dev;
> switch (t->task_proto) {
> case SAS_PROTOCOL_SMP:
> rc = pm8001_task_prep_smp(pm8001_ha, ccb);
Acked-by: Lindar Liu
This is the best way to fix such issue. Thanks.
> --
> 1.7.9
--
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
[PATCH v2 33/52] scsi, bnx2i: Fix CPU hotplug callback registration
Subsystems that want to register CPU hotplug callbacks, as well as perform initialization for the CPUs that are already online, often do it as shown below: get_online_cpus(); for_each_online_cpu(cpu) init_cpu(cpu); register_cpu_notifier(&foobar_cpu_notifier); put_online_cpus(); This is wrong, since it is prone to ABBA deadlocks involving the cpu_add_remove_lock and the cpu_hotplug.lock (when running concurrently with CPU hotplug operations). Instead, the correct and race-free way of performing the callback registration is: cpu_notifier_register_begin(); for_each_online_cpu(cpu) init_cpu(cpu); /* Note the use of the double underscored version of the API */ __register_cpu_notifier(&foobar_cpu_notifier); cpu_notifier_register_done(); Fix the bnx2i code in scsi by using this latter form of callback registration. Cc: Eddie Wai Cc: "James E.J. Bottomley" Cc: Ingo Molnar Cc: linux-scsi@vger.kernel.org Signed-off-by: Srivatsa S. Bhat --- drivers/scsi/bnx2i/bnx2i_init.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/bnx2i/bnx2i_init.c b/drivers/scsi/bnx2i/bnx2i_init.c index 34c294b..80c03b4 100644 --- a/drivers/scsi/bnx2i/bnx2i_init.c +++ b/drivers/scsi/bnx2i/bnx2i_init.c @@ -537,11 +537,15 @@ static int __init bnx2i_mod_init(void) p->iothread = NULL; } + cpu_notifier_register_begin(); + for_each_online_cpu(cpu) bnx2i_percpu_thread_create(cpu); /* Initialize per CPU interrupt thread */ - register_hotcpu_notifier(&bnx2i_cpu_notifier); + __register_hotcpu_notifier(&bnx2i_cpu_notifier); + + cpu_notifier_register_done(); return 0; @@ -581,11 +585,15 @@ static void __exit bnx2i_mod_exit(void) } mutex_unlock(&bnx2i_dev_lock); - unregister_hotcpu_notifier(&bnx2i_cpu_notifier); + cpu_notifier_register_begin(); for_each_online_cpu(cpu) bnx2i_percpu_thread_destroy(cpu); + __unregister_hotcpu_notifier(&bnx2i_cpu_notifier); + + cpu_notifier_register_done(); + iscsi_unregister_transport(&bnx2i_iscsi_transport); cnic_unregister_driver(CNIC_ULP_ISCSI); } -- 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
[PATCH v2 34/52] scsi, bnx2fc: Fix CPU hotplug callback registration
Subsystems that want to register CPU hotplug callbacks, as well as perform
initialization for the CPUs that are already online, often do it as shown
below:
get_online_cpus();
for_each_online_cpu(cpu)
init_cpu(cpu);
register_cpu_notifier(&foobar_cpu_notifier);
put_online_cpus();
This is wrong, since it is prone to ABBA deadlocks involving the
cpu_add_remove_lock and the cpu_hotplug.lock (when running concurrently
with CPU hotplug operations).
Instead, the correct and race-free way of performing the callback
registration is:
cpu_notifier_register_begin();
for_each_online_cpu(cpu)
init_cpu(cpu);
/* Note the use of the double underscored version of the API */
__register_cpu_notifier(&foobar_cpu_notifier);
cpu_notifier_register_done();
Fix the bnx2fc code in scsi by using this latter form of callback
registration.
Cc: Eddie Wai
Cc: "James E.J. Bottomley"
Cc: Ingo Molnar
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat
---
drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 9b94850..c4ec235 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -2586,12 +2586,16 @@ static int __init bnx2fc_mod_init(void)
spin_lock_init(&p->fp_work_lock);
}
+ cpu_notifier_register_begin();
+
for_each_online_cpu(cpu) {
bnx2fc_percpu_thread_create(cpu);
}
/* Initialize per CPU interrupt thread */
- register_hotcpu_notifier(&bnx2fc_cpu_notifier);
+ __register_hotcpu_notifier(&bnx2fc_cpu_notifier);
+
+ cpu_notifier_register_done();
cnic_register_driver(CNIC_ULP_FCOE, &bnx2fc_cnic_cb);
@@ -2656,13 +2660,17 @@ static void __exit bnx2fc_mod_exit(void)
if (l2_thread)
kthread_stop(l2_thread);
- unregister_hotcpu_notifier(&bnx2fc_cpu_notifier);
+ cpu_notifier_register_begin();
/* Destroy per cpu threads */
for_each_online_cpu(cpu) {
bnx2fc_percpu_thread_destroy(cpu);
}
+ __unregister_hotcpu_notifier(&bnx2fc_cpu_notifier);
+
+ cpu_notifier_register_done();
+
destroy_workqueue(bnx2fc_wq);
/*
* detach from scsi transport
--
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
[PATCH v2 35/52] scsi, fcoe: Fix CPU hotplug callback registration
Subsystems that want to register CPU hotplug callbacks, as well as perform
initialization for the CPUs that are already online, often do it as shown
below:
get_online_cpus();
for_each_online_cpu(cpu)
init_cpu(cpu);
register_cpu_notifier(&foobar_cpu_notifier);
put_online_cpus();
This is wrong, since it is prone to ABBA deadlocks involving the
cpu_add_remove_lock and the cpu_hotplug.lock (when running concurrently
with CPU hotplug operations).
Instead, the correct and race-free way of performing the callback
registration is:
cpu_notifier_register_begin();
for_each_online_cpu(cpu)
init_cpu(cpu);
/* Note the use of the double underscored version of the API */
__register_cpu_notifier(&foobar_cpu_notifier);
cpu_notifier_register_done();
Fix the fcoe code in scsi by using this latter form of callback registration.
Cc: Robert Love
Cc: "James E.J. Bottomley"
Cc: Ingo Molnar
Cc: fcoe-de...@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat
---
drivers/scsi/fcoe/fcoe.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index f317000..d5e105b 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -2633,14 +2633,18 @@ static int __init fcoe_init(void)
skb_queue_head_init(&p->fcoe_rx_list);
}
+ cpu_notifier_register_begin();
+
for_each_online_cpu(cpu)
fcoe_percpu_thread_create(cpu);
/* Initialize per CPU interrupt thread */
- rc = register_hotcpu_notifier(&fcoe_cpu_notifier);
+ rc = __register_hotcpu_notifier(&fcoe_cpu_notifier);
if (rc)
goto out_free;
+ cpu_notifier_register_done();
+
/* Setup link change notification */
fcoe_dev_setup();
@@ -2655,6 +2659,9 @@ out_free:
for_each_online_cpu(cpu) {
fcoe_percpu_thread_destroy(cpu);
}
+
+ cpu_notifier_register_done();
+
mutex_unlock(&fcoe_config_mutex);
destroy_workqueue(fcoe_wq);
return rc;
@@ -2687,11 +2694,15 @@ static void __exit fcoe_exit(void)
}
rtnl_unlock();
- unregister_hotcpu_notifier(&fcoe_cpu_notifier);
+ cpu_notifier_register_begin();
for_each_online_cpu(cpu)
fcoe_percpu_thread_destroy(cpu);
+ __unregister_hotcpu_notifier(&fcoe_cpu_notifier);
+
+ cpu_notifier_register_done();
+
mutex_unlock(&fcoe_config_mutex);
/*
--
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
[PATCH] [scsi] : pm80xx : fix problem of pm8001_work_fn reseting uncorrect phy device
If a phy device is removed ,the device can get error of I/O and HBA maybe
receieve
IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS of event which causes pm8001_work_fn to reset
the phy
device ,but in pm8001_task_exec don't assign a value for field device of ccb
and in
other case a ccb used have device field set , when ccb is freeed the field
device of
the ccb don't be set NULL.So there is possibility of getting another devcie
reset in fun
of mpi_ssp_completion .Also there are another way to solve problem by adding
following
code in mpi_SSP_completion:
pm8001_dev = t->dev->lldd_dev;
Signed-off-by: zhuxh
---
drivers/scsi/pm8001/pm8001_sas.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index f50ac44..f0ea5db 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -434,6 +434,7 @@ static int pm8001_task_exec(struct sas_task *task, const
int num,
ccb->n_elem = n_elem;
ccb->ccb_tag = tag;
ccb->task = t;
+ ccb->device = pm8001_dev;
switch (t->task_proto) {
case SAS_PROTOCOL_SMP:
rc = pm8001_task_prep_smp(pm8001_ha, ccb);
--
1.7.9
--
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
