Thread (85 messages) 85 messages, 6 authors, 2021-03-30

Re: [PATCH V3 04/25] smartpqi: add support for raid5 and raid6 writes

From: Martin Wilck <hidden>
Date: 2021-01-07 16:45:52

On Thu, 2020-12-10 at 14:34 -0600, Don Brace wrote:
quoted hunk ↗ jump to hunk
* Add in new IU definition.
* Add in support raid5 and raid6 writes.

Reviewed-by: Scott Benesh <redacted>
Reviewed-by: Scott Teel <redacted>
Reviewed-by: Kevin Barnett <redacted>
Signed-off-by: Don Brace <don.brace@microchip.com>
---
 drivers/scsi/smartpqi/smartpqi.h      |   39 +++++
 drivers/scsi/smartpqi/smartpqi_init.c |  247
++++++++++++++++++++++++++++++++-
 2 files changed, 278 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/smartpqi/smartpqi.h
b/drivers/scsi/smartpqi/smartpqi.h
index d486a2ec3045..e9844210c4a0 100644
--- a/drivers/scsi/smartpqi/smartpqi.h
+++ b/drivers/scsi/smartpqi/smartpqi.h
@@ -257,6 +257,7 @@ struct pqi_device_capability {
 };
 
 #define PQI_MAX_EMBEDDED_SG_DESCRIPTORS                4
+#define PQI_MAX_EMBEDDED_R56_SG_DESCRIPTORS    3
 
 struct pqi_raid_path_request {
        struct pqi_iu_header header;
@@ -312,6 +313,39 @@ struct pqi_aio_path_request {
                sg_descriptors[PQI_MAX_EMBEDDED_SG_DESCRIPTORS];
 };
 
+#define PQI_RAID56_XFER_LIMIT_4K       0x1000 /* 4Kib */
+#define PQI_RAID56_XFER_LIMIT_8K       0x2000 /* 8Kib */
You don't seem to use these, and you'll remove them again in patch
06/25.
quoted hunk ↗ jump to hunk
+struct pqi_aio_r56_path_request {
+       struct pqi_iu_header header;
+       __le16  request_id;
+       __le16  volume_id;              /* ID of the RAID volume */
+       __le32  data_it_nexus;          /* IT nexus for the data
drive */
+       __le32  p_parity_it_nexus;      /* IT nexus for the P parity
drive */
+       __le32  q_parity_it_nexus;      /* IT nexus for the Q parity
drive */
+       __le32  data_length;            /* total bytes to read/write
*/
+       u8      data_direction : 2;
+       u8      partial : 1;
+       u8      mem_type : 1;           /* 0b: PCIe, 1b: DDR */
+       u8      fence : 1;
+       u8      encryption_enable : 1;
+       u8      reserved : 2;
+       u8      task_attribute : 3;
+       u8      command_priority : 4;
+       u8      reserved1 : 1;
+       __le16  data_encryption_key_index;
+       u8      cdb[16];
+       __le16  error_index;
+       u8      num_sg_descriptors;
+       u8      cdb_length;
+       u8      xor_multiplier;
+       u8      reserved2[3];
+       __le32  encrypt_tweak_lower;
+       __le32  encrypt_tweak_upper;
+       u8      row;                    /* row = logical lba/blocks
per row */
+       u8      reserved3[8];
+       struct pqi_sg_descriptor
sg_descriptors[PQI_MAX_EMBEDDED_R56_SG_DESCRIPTORS];
+};
+
 struct pqi_io_response {
        struct pqi_iu_header header;
        __le16  request_id;
@@ -484,6 +518,8 @@ struct pqi_raid_error_info {
 #define PQI_REQUEST_IU_TASK_MANAGEMENT                 0x13
 #define PQI_REQUEST_IU_RAID_PATH_IO                    0x14
 #define PQI_REQUEST_IU_AIO_PATH_IO                     0x15
+#define PQI_REQUEST_IU_AIO_PATH_RAID5_IO               0x18
+#define PQI_REQUEST_IU_AIO_PATH_RAID6_IO               0x19
 #define PQI_REQUEST_IU_GENERAL_ADMIN                   0x60
 #define PQI_REQUEST_IU_REPORT_VENDOR_EVENT_CONFIG      0x72
 #define PQI_REQUEST_IU_SET_VENDOR_EVENT_CONFIG         0x73
@@ -1179,6 +1215,7 @@ struct pqi_ctrl_info {
        u16             max_inbound_iu_length_per_firmware;
        u16             max_inbound_iu_length;
        unsigned int    max_sg_per_iu;
+       unsigned int    max_sg_per_r56_iu;
        void            *admin_queue_memory_base;
        u32             admin_queue_memory_length;
        dma_addr_t      admin_queue_memory_base_dma_handle;
@@ -1210,6 +1247,8 @@ struct pqi_ctrl_info {
        u8              soft_reset_handshake_supported : 1;
        u8              raid_iu_timeout_supported: 1;
        u8              tmf_iu_timeout_supported: 1;
+       u8              enable_r5_writes : 1;
+       u8              enable_r6_writes : 1;
 
        struct list_head scsi_device_list;
        spinlock_t      scsi_device_list_lock;
diff --git a/drivers/scsi/smartpqi/smartpqi_init.c
b/drivers/scsi/smartpqi/smartpqi_init.c
index 6bcb037ae9d7..c813cec10003 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -67,6 +67,10 @@ static int pqi_aio_submit_io(struct pqi_ctrl_info
*ctrl_info,
        struct scsi_cmnd *scmd, u32 aio_handle, u8 *cdb,
        unsigned int cdb_length, struct pqi_queue_group *queue_group,
        struct pqi_encryption_info *encryption_info, bool
raid_bypass);
+static int pqi_aio_submit_r56_write_io(struct pqi_ctrl_info
*ctrl_info,
+       struct scsi_cmnd *scmd, struct pqi_queue_group *queue_group,
+       struct pqi_encryption_info *encryption_info, struct
pqi_scsi_dev *device,
+       struct pqi_scsi_dev_raid_map_data *rmd);
 static void pqi_ofa_ctrl_quiesce(struct pqi_ctrl_info *ctrl_info);
 static void pqi_ofa_ctrl_unquiesce(struct pqi_ctrl_info *ctrl_info);
 static int pqi_ofa_ctrl_restart(struct pqi_ctrl_info *ctrl_info);
@@ -2237,7 +2241,8 @@ static inline void pqi_set_encryption_info(
  * Attempt to perform RAID bypass mapping for a logical volume I/O.
  */
 
-static bool pqi_aio_raid_level_supported(struct
pqi_scsi_dev_raid_map_data *rmd)
+static bool pqi_aio_raid_level_supported(struct pqi_ctrl_info
*ctrl_info,
+       struct pqi_scsi_dev_raid_map_data *rmd)
 {
        bool is_supported = true;
 
@@ -2245,13 +2250,14 @@ static bool
pqi_aio_raid_level_supported(struct pqi_scsi_dev_raid_map_data *rmd)
        case SA_RAID_0:
                break;
        case SA_RAID_1:
-               if (rmd->is_write)
-                       is_supported = false;
+               is_supported = false;
You disable RAID1 READs with this patch. I can see you fix it again in
05/25, still it looks wrong.
quoted hunk ↗ jump to hunk
                break;
        case SA_RAID_5:
-               fallthrough;
+               if (rmd->is_write && !ctrl_info->enable_r5_writes)
+                       is_supported = false;
+               break;
        case SA_RAID_6:
-               if (rmd->is_write)
+               if (rmd->is_write && !ctrl_info->enable_r6_writes)
                        is_supported = false;
                break;
        case SA_RAID_ADM:
@@ -2526,6 +2532,26 @@ static int pqi_calc_aio_r5_or_r6(struct
pqi_scsi_dev_raid_map_data *rmd,
                rmd->total_disks_per_row)) +
                (rmd->map_row * rmd->total_disks_per_row) + rmd-
quoted
first_column;
 
+       if (rmd->is_write) {
+               rmd->p_index = (rmd->map_row * rmd-
quoted
total_disks_per_row) + rmd->data_disks_per_row;
+               rmd->p_parity_it_nexus = raid_map->disk_data[rmd-
quoted
p_index].aio_handle;
I suppose you have made sure rmd->p_index can't be larger than the
size of raid_map->disk_data. A comment explaining that would be helpful
for the reader though.

+               if (rmd->raid_level == SA_RAID_6) {
+                       rmd->q_index = (rmd->map_row * rmd-
quoted
total_disks_per_row) +
+                               (rmd->data_disks_per_row + 1);
+                       rmd->q_parity_it_nexus = raid_map-
quoted
disk_data[rmd->q_index].aio_handle;
+                       rmd->xor_mult = raid_map->disk_data[rmd-
quoted
map_index].xor_mult[1];
See above.
+               }
+               if (rmd->blocks_per_row == 0)
+                       return PQI_RAID_BYPASS_INELIGIBLE;
+#if BITS_PER_LONG == 32
+               tmpdiv = rmd->first_block;
+               do_div(tmpdiv, rmd->blocks_per_row);
+               rmd->row = tmpdiv;
+#else
+               rmd->row = rmd->first_block / rmd->blocks_per_row;
+#endif
Why not always use do_div()?
quoted hunk ↗ jump to hunk
+       }
+
        return 0;
 }
 
@@ -2567,7 +2593,7 @@ static int
pqi_raid_bypass_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info,
 
        rmd.raid_level = device->raid_level;
 
-       if (!pqi_aio_raid_level_supported(&rmd))
+       if (!pqi_aio_raid_level_supported(ctrl_info, &rmd))
                return PQI_RAID_BYPASS_INELIGIBLE;
 
        if (unlikely(rmd.block_cnt == 0))
@@ -2587,7 +2613,8 @@ static int
pqi_raid_bypass_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info,
        } else if (device->raid_level == SA_RAID_ADM) {
                rc = pqi_calc_aio_raid_adm(&rmd, device);
        } else if ((device->raid_level == SA_RAID_5 ||
-               device->raid_level == SA_RAID_6) &&
rmd.layout_map_count > 1) {
+               device->raid_level == SA_RAID_6) &&
+               (rmd.layout_map_count > 1 || rmd.is_write)) {
                rc = pqi_calc_aio_r5_or_r6(&rmd, raid_map);
                if (rc)
                        return PQI_RAID_BYPASS_INELIGIBLE;
@@ -2622,9 +2649,27 @@ static int
pqi_raid_bypass_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info,
                encryption_info_ptr = NULL;
        }
 
-       return pqi_aio_submit_io(ctrl_info, scmd, rmd.aio_handle,
+       if (rmd.is_write) {
+               switch (device->raid_level) {
+               case SA_RAID_0:
+                       return pqi_aio_submit_io(ctrl_info, scmd,
rmd.aio_handle,
                                rmd.cdb, rmd.cdb_length, queue_group,
                                encryption_info_ptr, true);
+               case SA_RAID_5:
+               case SA_RAID_6:
+                       return pqi_aio_submit_r56_write_io(ctrl_info,
scmd, queue_group,
+                                       encryption_info_ptr, device,
&rmd);
+               default:
+                       return pqi_aio_submit_io(ctrl_info, scmd,
rmd.aio_handle,
+                               rmd.cdb, rmd.cdb_length, queue_group,
+                               encryption_info_ptr, true);
+               }
+       } else {
+               return pqi_aio_submit_io(ctrl_info, scmd,
rmd.aio_handle,
+                       rmd.cdb, rmd.cdb_length, queue_group,
+                       encryption_info_ptr, true);
+       }
+
 }
 
 #define PQI_STATUS_IDLE                0x0
@@ -4844,6 +4889,12 @@ static void
pqi_calculate_queue_resources(struct pqi_ctrl_info *ctrl_info)
                PQI_OPERATIONAL_IQ_ELEMENT_LENGTH) /
                sizeof(struct pqi_sg_descriptor)) +
                PQI_MAX_EMBEDDED_SG_DESCRIPTORS;
+
+       ctrl_info->max_sg_per_r56_iu =
+               ((ctrl_info->max_inbound_iu_length -
+               PQI_OPERATIONAL_IQ_ELEMENT_LENGTH) /
+               sizeof(struct pqi_sg_descriptor)) +
+               PQI_MAX_EMBEDDED_R56_SG_DESCRIPTORS;
 }
 
 static inline void pqi_set_sg_descriptor(
@@ -4931,6 +4982,44 @@ static int pqi_build_raid_sg_list(struct
pqi_ctrl_info *ctrl_info,
        return 0;
 }
 
+static int pqi_build_aio_r56_sg_list(struct pqi_ctrl_info
*ctrl_info,
+       struct pqi_aio_r56_path_request *request, struct scsi_cmnd
*scmd,
+       struct pqi_io_request *io_request)
+{
+       u16 iu_length;
+       int sg_count;
+       bool chained;
+       unsigned int num_sg_in_iu;
+       struct scatterlist *sg;
+       struct pqi_sg_descriptor *sg_descriptor;
+
+       sg_count = scsi_dma_map(scmd);
+       if (sg_count < 0)
+               return sg_count;
+
+       iu_length = offsetof(struct pqi_aio_r56_path_request,
sg_descriptors) -
+               PQI_REQUEST_HEADER_LENGTH;
+       num_sg_in_iu = 0;
+
+       if (sg_count == 0)
+               goto out;
An if {} block would be better readable here.
quoted hunk ↗ jump to hunk
+
+       sg = scsi_sglist(scmd);
+       sg_descriptor = request->sg_descriptors;
+
+       num_sg_in_iu = pqi_build_sg_list(sg_descriptor, sg, sg_count,
io_request,
+               ctrl_info->max_sg_per_r56_iu, &chained);
+
+       request->partial = chained;
+       iu_length += num_sg_in_iu * sizeof(*sg_descriptor);
+
+out:
+       put_unaligned_le16(iu_length, &request->header.iu_length);
+       request->num_sg_descriptors = num_sg_in_iu;
+
+       return 0;
+}
+
 static int pqi_build_aio_sg_list(struct pqi_ctrl_info *ctrl_info,
        struct pqi_aio_path_request *request, struct scsi_cmnd *scmd,
        struct pqi_io_request *io_request)
@@ -5335,6 +5424,88 @@ static int pqi_aio_submit_io(struct
pqi_ctrl_info *ctrl_info,
        return 0;
 }
 
+static int pqi_aio_submit_r56_write_io(struct pqi_ctrl_info
*ctrl_info,
+       struct scsi_cmnd *scmd, struct pqi_queue_group *queue_group,
+       struct pqi_encryption_info *encryption_info, struct
pqi_scsi_dev *device,
+       struct pqi_scsi_dev_raid_map_data *rmd)
+{
+       int rc;
+       struct pqi_io_request *io_request;
+       struct pqi_aio_r56_path_request *r56_request;
+
+       io_request = pqi_alloc_io_request(ctrl_info);
+       io_request->io_complete_callback = pqi_aio_io_complete;
+       io_request->scmd = scmd;
+       io_request->raid_bypass = true;
+
+       r56_request = io_request->iu;
+       memset(r56_request, 0, offsetof(struct
pqi_aio_r56_path_request, sg_descriptors));
+
+       if (device->raid_level == SA_RAID_5 || device->raid_level ==
SA_RAID_51)
+               r56_request->header.iu_type =
PQI_REQUEST_IU_AIO_PATH_RAID5_IO;
+       else
+               r56_request->header.iu_type =
PQI_REQUEST_IU_AIO_PATH_RAID6_IO;
+
+       put_unaligned_le16(*(u16 *)device->scsi3addr & 0x3fff,
&r56_request->volume_id);
+       put_unaligned_le32(rmd->aio_handle, &r56_request-
quoted
data_it_nexus);
+       put_unaligned_le32(rmd->p_parity_it_nexus, &r56_request-
quoted
p_parity_it_nexus);
+       if (rmd->raid_level == SA_RAID_6) {
+               put_unaligned_le32(rmd->q_parity_it_nexus,
&r56_request->q_parity_it_nexus);
+               r56_request->xor_multiplier = rmd->xor_mult;
+       }
+       put_unaligned_le32(scsi_bufflen(scmd), &r56_request-
quoted
data_length);
+       r56_request->task_attribute = SOP_TASK_ATTRIBUTE_SIMPLE;
+       put_unaligned_le64(rmd->row, &r56_request->row);
+
+       put_unaligned_le16(io_request->index, &r56_request-
quoted
request_id);
+       r56_request->error_index = r56_request->request_id;
+
+       if (rmd->cdb_length > sizeof(r56_request->cdb))
+               rmd->cdb_length = sizeof(r56_request->cdb);
+       r56_request->cdb_length = rmd->cdb_length;
+       memcpy(r56_request->cdb, rmd->cdb, rmd->cdb_length);
+
+       switch (scmd->sc_data_direction) {
+       case DMA_TO_DEVICE:
+               r56_request->data_direction = SOP_READ_FLAG;
+               break;
I wonder how it would be possible that sc_data_direction is anything
else but DMA_TO_DEVICE here. AFAICS we will only reach this code for
WRITE commands. Add a comment, please.

quoted hunk ↗ jump to hunk
+       case DMA_FROM_DEVICE:
+               r56_request->data_direction = SOP_WRITE_FLAG;
+               break;
+       case DMA_NONE:
+               r56_request->data_direction = SOP_NO_DIRECTION_FLAG;
+               break;
+       case DMA_BIDIRECTIONAL:
+               r56_request->data_direction = SOP_BIDIRECTIONAL;
+               break;
+       default:
+               dev_err(&ctrl_info->pci_dev->dev,
+                       "unknown data direction: %d\n",
+                       scmd->sc_data_direction);
+               break;
+       }
+
+       if (encryption_info) {
+               r56_request->encryption_enable = true;
+               put_unaligned_le16(encryption_info-
quoted
data_encryption_key_index,
+                               &r56_request-
quoted
data_encryption_key_index);
+               put_unaligned_le32(encryption_info-
quoted
encrypt_tweak_lower,
+                               &r56_request->encrypt_tweak_lower);
+               put_unaligned_le32(encryption_info-
quoted
encrypt_tweak_upper,
+                               &r56_request->encrypt_tweak_upper);
+       }
+
+       rc = pqi_build_aio_r56_sg_list(ctrl_info, r56_request, scmd,
io_request);
+       if (rc) {
+               pqi_free_io_request(io_request);
+               return SCSI_MLQUEUE_HOST_BUSY;
+       }
+
+       pqi_start_io(ctrl_info, queue_group, AIO_PATH, io_request);
+
+       return 0;
+}
+
 static inline u16 pqi_get_hw_queue(struct pqi_ctrl_info *ctrl_info,
        struct scsi_cmnd *scmd)
 {
@@ -6298,6 +6469,60 @@ static ssize_t pqi_lockup_action_store(struct
device *dev,
        return -EINVAL;
 }
 
+static ssize_t pqi_host_enable_r5_writes_show(struct device *dev,
+       struct device_attribute *attr, char *buffer)
+{
+       struct Scsi_Host *shost = class_to_shost(dev);
+       struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost);
+
+       return scnprintf(buffer, 10, "%hhx\n", ctrl_info-
quoted
enable_r5_writes);
"%hhx" is deprecated, see
https://lore.kernel.org/lkml/20190914015858.7c76e036@lwn.net/T/ (local)

+}
+
+static ssize_t pqi_host_enable_r5_writes_store(struct device *dev,
+       struct device_attribute *attr, const char *buffer, size_t
count)
+{
+       struct Scsi_Host *shost = class_to_shost(dev);
+       struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost);
+       u8 set_r5_writes = 0;
+
+       if (kstrtou8(buffer, 0, &set_r5_writes))
+               return -EINVAL;
+
+       if (set_r5_writes > 0)
+               set_r5_writes = 1;
+
+       ctrl_info->enable_r5_writes = set_r5_writes;
+
+       return count;
+}
+
+static ssize_t pqi_host_enable_r6_writes_show(struct device *dev,
+       struct device_attribute *attr, char *buffer)
+{
+       struct Scsi_Host *shost = class_to_shost(dev);
+       struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost);
+
+       return scnprintf(buffer, 10, "%hhx\n", ctrl_info-
quoted
enable_r6_writes);
See above
quoted hunk ↗ jump to hunk
+}
+
+static ssize_t pqi_host_enable_r6_writes_store(struct device *dev,
+       struct device_attribute *attr, const char *buffer, size_t
count)
+{
+       struct Scsi_Host *shost = class_to_shost(dev);
+       struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost);
+       u8 set_r6_writes = 0;
+
+       if (kstrtou8(buffer, 0, &set_r6_writes))
+               return -EINVAL;
+
+       if (set_r6_writes > 0)
+               set_r6_writes = 1;
+
+       ctrl_info->enable_r6_writes = set_r6_writes;
+
+       return count;
+}
+
 static DEVICE_ATTR(driver_version, 0444, pqi_driver_version_show,
NULL);
 static DEVICE_ATTR(firmware_version, 0444,
pqi_firmware_version_show, NULL);
 static DEVICE_ATTR(model, 0444, pqi_model_show, NULL);
@@ -6306,6 +6531,10 @@ static DEVICE_ATTR(vendor, 0444,
pqi_vendor_show, NULL);
 static DEVICE_ATTR(rescan, 0200, NULL, pqi_host_rescan_store);
 static DEVICE_ATTR(lockup_action, 0644, pqi_lockup_action_show,
        pqi_lockup_action_store);
+static DEVICE_ATTR(enable_r5_writes, 0644,
+       pqi_host_enable_r5_writes_show,
pqi_host_enable_r5_writes_store);
+static DEVICE_ATTR(enable_r6_writes, 0644,
+       pqi_host_enable_r6_writes_show,
pqi_host_enable_r6_writes_store);
 
 static struct device_attribute *pqi_shost_attrs[] = {
        &dev_attr_driver_version,
@@ -6315,6 +6544,8 @@ static struct device_attribute
*pqi_shost_attrs[] = {
        &dev_attr_vendor,
        &dev_attr_rescan,
        &dev_attr_lockup_action,
+       &dev_attr_enable_r5_writes,
+       &dev_attr_enable_r6_writes,
        NULL
 };
 



Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help