On 19.10.2016 20:07, Raghava Aditya Renukunta wrote:
> Hi Tomas,
>
>> -----Original Message-----
>> From: Tomas Henzl [mailto:the...@redhat.com]
>> Sent: Wednesday, October 19, 2016 5:56 AM
>> To: Ching Huang
>> Cc: James Bottomley; Kashyap Desai; Ric Wheeler; Hannes Reinecke; Sumit
>> Saxena; linux-scsi@vger.kernel.org; martin.peter...@oracle.com; Christoph
>> Hellwig; Martin K. Petersen; Jeff Moyer; Gris Ge; Ewan Milne; Jens Axboe;
>> Raghava Aditya Renukunta
>> Subject: Re: [PATCH 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE
>> command to firmware
>>
>> EXTERNAL EMAIL
>>
>>
>> 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)
> We do not support this card  anymore nor do we have the hardware to test
> it out, but let me  try and procure the hardware for testing although 
> chances are very slim.

Thanks for looking into this even though the driver is no more supported.


>
> Regards,
> Raghava Aditya
>
>>>> 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:j...@linux.vnet.ibm.com]
>>>>>>> Sent: Monday, October 17, 2016 10:50 PM
>>>>>>> To: Ric Wheeler; Hannes Reinecke; Sumit Saxena;
>>>>>>> linux-scsi@vger.kernel.org
>>>>>>> Cc: martin.peter...@oracle.com; the...@redhat.com;
>>>>>>> kashyap.de...@broadcom.com; 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.sax...@broadcom.com>
>>>>>>>>>>>> Signed-off-by: Kashyap Desai
>> <kashyap.de...@broadcom.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>    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....@avagotech.com");
>>>>>>>>>>>> @@ -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 majord...@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{���"�{ay�ʇڙ�,j��f���h���z��w���
> ���j:+v���w�j�m��������zZ+�����ݢj"��!tml=


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

Reply via email to