On 19.10.2016 11:50, Ching Huang wrote:
> Hi Tomas,
>
> SCSI command checking in queuecommand function of arcmsr can be removed safely.
> Now driver can pass all scsi command to controller firmware.
thanks for your quick reply. Is this (passing the SYNCHRONIZE_CACHE command
to firmware) safe for all arcmsr cards, even the oldest?
Please start with your patch a new thread and add your 'Signed-off-by:' line.
>
> diff -uprN a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c 2016-10-19 16:18:45.000000000 +0800
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c 2016-10-19 16:31:59.665524699 +0800
> @@ -2623,13 +2623,6 @@ static int arcmsr_queue_command_lck(stru
> cmd->scsi_done = done;
> cmd->host_scribble = NULL;
> cmd->result = 0;
> - if ((scsicmd == SYNCHRONIZE_CACHE) ||(scsicmd == SEND_DIAGNOSTIC)){
> - if(acb->devstate[target][lun] == ARECA_RAID_GONE) {
> - cmd->result = (DID_NO_CONNECT << 16);
> - }
> - cmd->scsi_done(cmd);
> - return 0;
> - }
> if (target == 16) {
> /* virtual device for iop message transfer */
> arcmsr_handle_virtual_command(acb, cmd);
>
> Thanks
> Ching
> On Tue, 2016-10-18 at 15:56 +0200, Tomas Henzl wrote:
>> Hi,
>>
>> similar suspicious code path can be found in the queuecommand functions in other drivers too
>> these are -
>> pmcraid.c
>> arcmsr_hba.c
>> cc-ing maintainers -
>> (but both drivers seem to be unmaintained for a while -
>> I've added Ching for arcmsr and Raghava for pmcraid)
>>
>> please read this thread and verify that your driver and firmware is safe.
>>
>> It is expected that a raid card controls the disk write cache (switches it off)
>> but this might not be true for all modes of operation a raid adapter handles
>> - pass through/non-RAID etc. In such case when disk write cache is enabled
>> and your driver skips the SYNCHRONIZE_CACHE command a FS corruption
>> is very much possible during unexpected power loss or even a clean shutdown.
>>
>> tomash
>>
>> On 17.10.2016 19:51, James Bottomley wrote:
>>> On Mon, 2016-10-17 at 23:06 +0530, Kashyap Desai wrote:
>>>>> -----Original Message-----
>>>>> From: James Bottomley [mailto:jejb@xxxxxxxxxxxxxxxxxx]
>>>>> Sent: Monday, October 17, 2016 10:50 PM
>>>>> To: Ric Wheeler; Hannes Reinecke; Sumit Saxena;
>>>>> linux-scsi@xxxxxxxxxxxxxxx
>>>>> Cc: martin.petersen@xxxxxxxxxx; thenzl@xxxxxxxxxx;
>>>>> kashyap.desai@xxxxxxxxxxxx; Christoph Hellwig; Martin K. Petersen;
>>>>> Jeff
>>>>> Moyer; Gris Ge; Ewan Milne; Jens Axboe
>>>>> Subject: Re: [PATCH 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE
>>>>> command
>>>>> to firmware
>>>>>
>>>>> On Mon, 2016-10-17 at 12:27 -0400, Ric Wheeler wrote:
>>>>>> On 10/17/2016 12:20 PM, James Bottomley wrote:
>>>>>>> On Mon, 2016-10-17 at 09:01 -0400, Ric Wheeler wrote:
>>>>>>>> On 10/17/2016 07:34 AM, Hannes Reinecke wrote:
>>>>>>>>> On 10/17/2016 12:24 PM, Sumit Saxena wrote:
>>>>>>>>>> megaraid_sas driver returns SYNCHRONIZE_CACHE command
>>>>>>>>>> back to
>>>>>>>>>> SCSI layer without sending it to firmware as firmware
>>>>>>>>>> takes
>>>>>>>>>> care of flushing cache. This patch will change the
>>>>>>>>>> driver
>>>>>>>>>> behavior wrt SYNCHRONIZE_CACHE handling. If underlying
>>>>>>>>>> firmware has support to handle the SYNCHORNIZE_CACHE,
>>>>>>>>>> driver
>>>>>>>>>> will send it for firmware otherwise complete it back to
>>>>>>>>>> SCSI
>>>>>>>>>> layer with SUCCESS immediately. If Firmware handle
>>>>>>>>>> SYNCHORNIZE_CACHE for both VD and JBOD
>>>>>>>>>> "canHandleSyncCache"
>>>>>>>>>> bit in scratch pad register(offset
>>>>>>>>>> 0x00B4) will be set.
>>>>>>>>>>
>>>>>>>>>> This behavior can be controlled via module parameter and
>>>>>>>>>> user
>>>>>>>>>> can fallback to old behavior of returning
>>>>>>>>>> SYNCHRONIZE_CACHE by
>>>>>>>>>> driver only without sending it to firmware.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Sumit Saxena <sumit.saxena@xxxxxxxxxxxx>
>>>>>>>>>> Signed-off-by: Kashyap Desai <kashyap.desai@xxxxxxxxxxxx>
>>>>>>>>>> ---
>>>>>>>>>> drivers/scsi/megaraid/megaraid_sas.h | 3 +++
>>>>>>>>>> drivers/scsi/megaraid/megaraid_sas_base.c | 14
>>>>>>>>>> ++++++---
>>>>>>>>>> -----
>>>>>>>>>> drivers/scsi/megaraid/megaraid_sas_fusion.c | 3 +++
>>>>>>>>>> drivers/scsi/megaraid/megaraid_sas_fusion.h | 2 ++
>>>>>>>>>> 4 files changed, 14 insertions(+), 8 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/scsi/megaraid/megaraid_sas.h
>>>>>>>>>> b/drivers/scsi/megaraid/megaraid_sas.h
>>>>>>>>>> index ca86c88..43fd14f 100644
>>>>>>>>>> --- a/drivers/scsi/megaraid/megaraid_sas.h
>>>>>>>>>> +++ b/drivers/scsi/megaraid/megaraid_sas.h
>>>>>>>>>> @@ -1429,6 +1429,8 @@ enum FW_BOOT_CONTEXT {
>>>>>>>>>> #define MR_MAX_REPLY_QUEUES_EXT_OFFSET_SHIFT 14
>>>>>>>>>> #define MR_MAX_MSIX_REG_ARRAY 16
>>>>>>>>>> #define MR_RDPQ_MODE_OFFSET 0X0
>>>>>>>>>> 0800
>>>>>>>>>> 000
>>>>>>>>>> +#define MR_CAN_HANDLE_SYNC_CACHE_OFFSET 0
>>>>>>>>>> X010
>>>>>>>>>> 0000
>>>>>>>>>> 0
>>>>>>>>>> +
>>>>>>>>>> /*
>>>>>>>>>> * register set for both 1068 and 1078 controllers
>>>>>>>>>> * structure extended for 1078 registers @@ -2140,6
>>>>>>>>>> +2142,7
>>>>>>>>>> @@ struct megasas_instance {
>>>>>>>>>> u8 is_imr;
>>>>>>>>>> u8 is_rdpq;
>>>>>>>>>> bool dev_handle;
>>>>>>>>>> + bool fw_sync_cache_support;
>>>>>>>>>> };
>>>>>>>>>> struct MR_LD_VF_MAP {
>>>>>>>>>> u32 size;
>>>>>>>>>> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
>>>>>>>>>> b/drivers/scsi/megaraid/megaraid_sas_base.c
>>>>>>>>>> index 092387f..a4a8e2f 100644
>>>>>>>>>> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
>>>>>>>>>> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
>>>>>>>>>> @@ -104,6 +104,10 @@ unsigned int scmd_timeout =
>>>>>>>>>> MEGASAS_DEFAULT_CMD_TIMEOUT;
>>>>>>>>>> module_param(scmd_timeout, int, S_IRUGO);
>>>>>>>>>> MODULE_PARM_DESC(scmd_timeout, "scsi command timeout
>>>>>>>>>> (10
>>>>>>>>>> -90s), default 90s. See megasas_reset_timer.");
>>>>>>>>>>
>>>>>>>>>> +bool block_sync_cache;
>>>>>>>>>> +module_param(block_sync_cache, bool, S_IRUGO);
>>>>>>>>>> +MODULE_PARM_DESC(block_sync_cache, "Block SYNC CACHE by
>>>>>>>>>> driver
>>>>>>>>>> Default: 0(Send it to firmware)");
>>>>>>>>>> +
>>>>>>>>>> MODULE_LICENSE("GPL");
>>>>>>>>>> MODULE_VERSION(MEGASAS_VERSION);
>>>>>>>>>> MODULE_AUTHOR("megaraidlinux.pdl@xxxxxxxxxxxxx");
>>>>>>>>>> @@ -1700,16 +1704,10 @@ megasas_queue_command(struct
>>>>> Scsi_Host
>>>>>>>>>> *shost, struct scsi_cmnd *scmd)
>>>>>>>>>> goto out_done;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> - switch (scmd->cmnd[0]) {
>>>>>>>>>> - case SYNCHRONIZE_CACHE:
>>>>>>>>>> - /*
>>>>>>>>>> - * FW takes care of flush cache on its
>>>>>>>>>> own
>>>>>>>>>> - * No need to send it down
>>>>>>>>>> - */
>>>>>>>>>> + if ((scmd->cmnd[0] == SYNCHRONIZE_CACHE) &&
>>>>>>>>>> + (!instance->fw_sync_cache_support)) {
>>>>>>>>>> scmd->result = DID_OK << 16;
>>>>>>>>>> goto out_done;
>>>>>>>>>> - default:
>>>>>>>>>> - break;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> return instance->instancet
>>>>>>>>>> ->build_and_issue_cmd(instance, scmd);
>>>>>>>>>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>>>>>>>>> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>>>>>>>>> index 2159f6a..8237580 100644
>>>>>>>>>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>>>>>>>>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>>>>>>>>> @@ -747,6 +747,9 @@ megasas_ioc_init_fusion(struct
>>>>>>>>>> megasas_instance *instance)
>>>>>>>>>> ret = 1;
>>>>>>>>>> goto fail_fw_init;
>>>>>>>>>> }
>>>>>>>>>> + if (!block_sync_cache)
>>>>>>>>>> + instance->fw_sync_cache_support =
>>>>>>>>>> (scratch_pad_2
>>>>>>>>>> &
>>>>>>>>>> + MR_CAN_HANDLE_SYNC_CACHE_OFFSET)
>>>>>>>>>> ? 1
>>>>>>>>>> :
>>>>>>>>>> 0;
>>>>>>>>>>
>>>>>>>>>> IOCInitMessage =
>>>>>>>>>> dma_alloc_coherent(&instance->pdev->dev,
>>>>>>>>>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.h
>>>>>>>>>> b/drivers/scsi/megaraid/megaraid_sas_fusion.h
>>>>>>>>>> index e3bee04..2857154 100644
>>>>>>>>>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.h
>>>>>>>>>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.h
>>>>>>>>>> @@ -72,6 +72,8 @@
>>>>>>>>>> #define MPI2_SUP_REPLY_POST_HOST_INDEX_OFFSET
>>>>>>>>>> (0x0000030C)
>>>>>>>>>> #define MPI2_REPLY_POST_HOST_INDEX_OFFSET (0x00
>>>>>>>>>> 0000
>>>>>>>>>> 6C)
>>>>>>>>>>
>>>>>>>>>> +extern bool block_sync_cache;
>>>>>>>>>> +
>>>>>>>>>> /*
>>>>>>>>>> * Raid context flags
>>>>>>>>>> */
>>>>>>>>>>
>>>>>>>>> Be extra careful with that.
>>>>>>>>>
>>>>>>>>> SYNCHRONIZE_CACHE has (potentially) a global scope, as
>>>>>>>>> there
>>>>>>>>> might be an array-wide cache, and a cache flush would
>>>>>>>>> affect the
>>>>>>>>> entire cache. Linux is using SYNCHRONIZE_CACHE as a per
>>>>>>>>> device
>>>>>>>>> setting, ie it assumes that the effects of a cache flush is
>>>>>>>>> restricted to the device in question.
>>>>>>>>>
>>>>>>>>> If this is _not_ the case I'd rather not enable it.
>>>>>>>>> Have you checked that enabling this functionality doesn't
>>>>>>>>> have
>>>>>>>>> negative performance impact?
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>>
>>>>>>>>> Hannes
>>>>>>>> This must go in - without this fix, there is no data
>>>>>>>> integrity for
>>>>>>>> any file system.
>>>>>>> That's not what I get from the change log. What it says to me
>>>>>>> is
>>>>>>> that the caches are currently firmware managed. Barring
>>>>>>> firmware
>>>>>>> bugs, that means that we currently don't have any integrity
>>>>>>> issues.
>>>>>> Your understanding (or the change log) is incorrect.
>>>>> There's no current way in English of construing "as firmware takes
>>>>> care of
>>>>> flushing cache" to mean its inverse, so the changelog needs
>>>>> updating to
>>>>> explain
>>>>> that firmware does *not* take care of cache flushing, so
>>>>> effectively
>>>>> nothing
>>>>> does and we'll need a cc to stable if the problem is that nothing
>>>>> takes
>>>>> care of
>>>>> flushing the drive caches.
>>>>>
>>>>> James
>>>> Sorry for confusion. More accurate to say -
>>>>
>>>> Current megaraid_sas driver returns SYNCHRONIZE_CACHE command back to
>>>> SCSI layer without sending it down to firmware as firmware supposed
>>>> to takes care of flushing cache for all Virtual Disk at the time of
>>>> system reboot/shutdown. Because of above FW rule driver coded wrongly
>>>> and it added bug that SYNCHRONIZE_CACHE is not passed to FW for JBOD
>>>> as well. (MR Firmware wanted OS to manage SYNCHRONIZE_CACHE and pass
>>>> the same to the Firmware. ). We figure out that even for VD, why
>>>> driver should send back SYNC_CACHE command...let's have the same in
>>>> FW (giving all control in FW)
>>>>
>>>> New behavior described -
>>>>
>>>> IF application requests SYNCH_CACHE
>>>> IF any FW which supports new API bit called canHandleSyncCache
>>>> Driver sends SYNCH_CACHE command to the FW
>>>> IF 'VirtualDisk'
>>>> FW does not send SYNCH_CACHE to drives
>>>> FW returns SUCCESS
>>>> ELSE IF 'JBOD'
>>>> FW sends SYNCH_CACHE to drive
>>>> FW obtains status from drive and returns same status back to
>>>> driver
>>>> ENDIF
>>>>
>>>> Fixing this is robust if FW and driver changes are inline. See below
>>>> for more detail.
>>>>
>>>> Case - 1
>>>> This patch is attempt to fix one level problem where Virtual Disks
>>>> are not managed correctly in MR FW. There are some MR FW (E.a Gen2
>>>> and Gen2.5 FW), which set WCE bit for Virtual Disk but FW is not
>>>> reply correct for SYNCH_CACHE. This was handled in past and carry
>>>> forward (in driver + FW ) to all new generation products as well. We
>>>> tried to collect all possible details why it was handled such way to
>>>> provide better driver fix for this issue(mainly to avoid this FW
>>>> checks and module parameters etc..). But now it looks like to make
>>>> generic fix (Device ID based etc..)is even risky, so went with this
>>>> protective approach. It is very risky to find out which Device ID
>>>> and FW has capability to manage SYNC_CACHE, so we managed to figure
>>>> out which are the new FW using FW capability bit.
>>>>
>>>> E.g Liberator/Thunderbolt MR FW SYNC_CACHE is write Unsupported
>>>> command (this is a firmware bug) and immediately it will be failed to
>>>> SML. This will cause File system to go Read-only.
>>> That's a better description ... what you're saying is that the patch
>>> doesn't actually fix the bug Ric is worrying about.
>>>
>>>> Case -2
>>>> One more thing which we are trying and known driver can send
>>>> "SYNC_CACHE" unconditionally to all generation of FW. For this we
>>>> are doing more unit testing on various controllers/FW and planning to
>>>> provide another patch to fix JBOD path for any FW. (It will not be
>>>> based on FW capability/module parameter).
>>> OK, but we really need this ASAP. As Ric said, data integrity is at
>>> risk until this is fixed. Can you also reference the commit that
>>> introduced the problem so we know how far to do the sable backports?
>>>
>>>> If we remove module parameter, we can ask customer to disable WCE on
>>>> drive to get similar impact.
>>> Thanks,
>>>
>>> James
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
