Re: [PATCH 10/23] zfcp: drop duplicate fsf_command from zfcp_fsf_req which is also in QTCB header

2018-11-16 Thread Hannes Reinecke

On 11/8/18 3:44 PM, Steffen Maier wrote:

Status read buffers (SRBs, unsolicited notifications) never use a QTCB
[zfcp_fsf_req_create()]. zfcp_fsf_req_send() already uses this to
distinguish SRBs from other FSF request types. We can re-use this method
in zfcp_fsf_req_complete(). Introduce a helper function to make the check
for req->qtcb less magic.

SRBs always are FSF_QTCB_UNSOLICITED_STATUS, so we can hard-code this for
the two trace functions dealing with SRBs.

All other FSF request types have a QTCB and we can get the fsf_command
from there.

zfcp_dbf_hba_fsf_response() and thus zfcp_dbf_hba_fsf_res() are only called
for non-SRB requests so it's safe to dereference the QTCB
[zfcp_fsf_req_complete() returns early on SRB,  else calls
  zfcp_fsf_protstatus_eval() which calls zfcp_dbf_hba_fsf_response()].
In zfcp_scsi_forget_cmnd() we guard the QTCB dereference with a preceding
NULL check and rely on boolean shortcut evaluation.

As a side effect, this causes an alignment hole which we can close in
a later patch after having cleaned up all fields of struct zfcp_fsf_req.
Before:
$ pahole -C zfcp_fsf_req drivers/s390/scsi/zfcp.ko
...
u32status;   /*   136 4 */
u32fsf_command;  /*   140 4 */
struct fsf_qtcb *  qtcb; /*   144 8 */
...
After:
$ pahole -C zfcp_fsf_req drivers/s390/scsi/zfcp.ko
...
u32status;   /*   136 4 */
/* XXX 4 bytes hole, try to pack */
struct fsf_qtcb *  qtcb; /*   144 8 */
...

Signed-off-by: Steffen Maier 
Reviewed-by: Benjamin Block 
---
  drivers/s390/scsi/zfcp_dbf.c  |  8 
  drivers/s390/scsi/zfcp_dbf.h  |  4 ++--
  drivers/s390/scsi/zfcp_def.h  |  7 +--
  drivers/s390/scsi/zfcp_fsf.c  | 14 ++
  drivers/s390/scsi/zfcp_scsi.c |  4 +++-
  5 files changed, 20 insertions(+), 17 deletions(-)


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)


[PATCH 10/23] zfcp: drop duplicate fsf_command from zfcp_fsf_req which is also in QTCB header

2018-11-08 Thread Steffen Maier
Status read buffers (SRBs, unsolicited notifications) never use a QTCB
[zfcp_fsf_req_create()]. zfcp_fsf_req_send() already uses this to
distinguish SRBs from other FSF request types. We can re-use this method
in zfcp_fsf_req_complete(). Introduce a helper function to make the check
for req->qtcb less magic.

SRBs always are FSF_QTCB_UNSOLICITED_STATUS, so we can hard-code this for
the two trace functions dealing with SRBs.

All other FSF request types have a QTCB and we can get the fsf_command
from there.

zfcp_dbf_hba_fsf_response() and thus zfcp_dbf_hba_fsf_res() are only called
for non-SRB requests so it's safe to dereference the QTCB
[zfcp_fsf_req_complete() returns early on SRB,  else calls
 zfcp_fsf_protstatus_eval() which calls zfcp_dbf_hba_fsf_response()].
In zfcp_scsi_forget_cmnd() we guard the QTCB dereference with a preceding
NULL check and rely on boolean shortcut evaluation.

As a side effect, this causes an alignment hole which we can close in
a later patch after having cleaned up all fields of struct zfcp_fsf_req.
Before:
$ pahole -C zfcp_fsf_req drivers/s390/scsi/zfcp.ko
...
u32status;   /*   136 4 */
u32fsf_command;  /*   140 4 */
struct fsf_qtcb *  qtcb; /*   144 8 */
...
After:
$ pahole -C zfcp_fsf_req drivers/s390/scsi/zfcp.ko
...
u32status;   /*   136 4 */
/* XXX 4 bytes hole, try to pack */
struct fsf_qtcb *  qtcb; /*   144 8 */
...

Signed-off-by: Steffen Maier 
Reviewed-by: Benjamin Block 
---
 drivers/s390/scsi/zfcp_dbf.c  |  8 
 drivers/s390/scsi/zfcp_dbf.h  |  4 ++--
 drivers/s390/scsi/zfcp_def.h  |  7 +--
 drivers/s390/scsi/zfcp_fsf.c  | 14 ++
 drivers/s390/scsi/zfcp_scsi.c |  4 +++-
 5 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 3b368fcf13f4..d20977bb27a4 100644
--- a/drivers/s390/scsi/zfcp_dbf.c
+++ b/drivers/s390/scsi/zfcp_dbf.c
@@ -81,7 +81,7 @@ void zfcp_dbf_hba_fsf_res(char *tag, int level, struct 
zfcp_fsf_req *req)
rec->id = ZFCP_DBF_HBA_RES;
rec->fsf_req_id = req->req_id;
rec->fsf_req_status = req->status;
-   rec->fsf_cmd = req->fsf_command;
+   rec->fsf_cmd = q_head->fsf_command;
rec->fsf_seq_no = req->seq_no;
rec->u.res.req_issued = req->issued;
rec->u.res.prot_status = q_pref->prot_status;
@@ -94,7 +94,7 @@ void zfcp_dbf_hba_fsf_res(char *tag, int level, struct 
zfcp_fsf_req *req)
memcpy(rec->u.res.fsf_status_qual, _head->fsf_status_qual,
   FSF_STATUS_QUALIFIER_SIZE);
 
-   if (req->fsf_command != FSF_QTCB_FCP_CMND) {
+   if (q_head->fsf_command != FSF_QTCB_FCP_CMND) {
rec->pl_len = q_head->log_length;
zfcp_dbf_pl_write(dbf, (char *)q_pref + q_head->log_start,
  rec->pl_len, "fsf_res", req->req_id);
@@ -127,7 +127,7 @@ void zfcp_dbf_hba_fsf_uss(char *tag, struct zfcp_fsf_req 
*req)
rec->id = ZFCP_DBF_HBA_USS;
rec->fsf_req_id = req->req_id;
rec->fsf_req_status = req->status;
-   rec->fsf_cmd = req->fsf_command;
+   rec->fsf_cmd = FSF_QTCB_UNSOLICITED_STATUS;
 
if (!srb)
goto log;
@@ -174,7 +174,7 @@ void zfcp_dbf_hba_bit_err(char *tag, struct zfcp_fsf_req 
*req)
rec->id = ZFCP_DBF_HBA_BIT;
rec->fsf_req_id = req->req_id;
rec->fsf_req_status = req->status;
-   rec->fsf_cmd = req->fsf_command;
+   rec->fsf_cmd = FSF_QTCB_UNSOLICITED_STATUS;
memcpy(>u.be, _buf->payload.bit_error,
   sizeof(struct fsf_bit_error_payload));
 
diff --git a/drivers/s390/scsi/zfcp_dbf.h b/drivers/s390/scsi/zfcp_dbf.h
index d116c07ed77a..b4438713d1cc 100644
--- a/drivers/s390/scsi/zfcp_dbf.h
+++ b/drivers/s390/scsi/zfcp_dbf.h
@@ -339,8 +339,8 @@ void zfcp_dbf_hba_fsf_response(struct zfcp_fsf_req *req)
  zfcp_dbf_hba_fsf_resp_suppress(req)
  ? 5 : 1, req);
 
-   } else if ((req->fsf_command == FSF_QTCB_OPEN_PORT_WITH_DID) ||
-  (req->fsf_command == FSF_QTCB_OPEN_LUN)) {
+   } else if ((qtcb->header.fsf_command == FSF_QTCB_OPEN_PORT_WITH_DID) ||
+  (qtcb->header.fsf_command == FSF_QTCB_OPEN_LUN)) {
zfcp_dbf_hba_fsf_resp("fs_open", 4, req);
 
} else if (qtcb->header.log_length) {
diff --git a/drivers/s390/scsi/zfcp_def.h b/drivers/s390/scsi/zfcp_def.h
index 572debf2f528..d65adb0ae9f1 100644
--- a/drivers/s390/scsi/zfcp_def.h
+++ b/drivers/s390/scsi/zfcp_def.h
@@ -277,7 +277,6 @@ static inline u64 zfcp_scsi_dev_lun(struct scsi_device 
*sdev)
  * @qdio_req: qdio queue related values
  * @completion: used to signal the completion of the request
  * @status: status of the request
- *