[PATCH v4] scsi: sd_zbc: trace zone append emulation

[PATCH v4] scsi: sd_zbc: trace zone append emulation

[Johannes Thumshirn]

Add tracepoints to the SCSI zone append emulation, in order to trace the
zone start to write-pointer aligned LBA translation and the corresponding
completion.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

---
Changes to v3:
- Move tracepoints into own sd trace events header

Changes to v2:
- Fix linking when SCSI is a module

Changes to v1:
- Fix style issues pointed out by Damien
---
 drivers/scsi/scsi_trace.c |  1 +
 drivers/scsi/sd_zbc.c     |  6 +++
 include/trace/events/sd.h | 81 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+)
 create mode 100644 include/trace/events/sd.h

diff --git a/drivers/scsi/scsi_trace.c b/drivers/scsi/scsi_trace.c
index 41a950075913..224b38c0fb0f 100644
--- a/drivers/scsi/scsi_trace.c
+++ b/drivers/scsi/scsi_trace.c
@@ -389,3 +389,4 @@ scsi_trace_parse_cdb(struct trace_seq *p, unsigned char *cdb, int len)
 		return scsi_trace_misc(p, cdb, len);
 	}
 }
+EXPORT_SYMBOL(scsi_trace_parse_cdb);
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index bd15624c6322..8985681cffbd 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -18,6 +18,9 @@
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/sd.h>
+
 #include "sd.h"
 
 /**
@@ -450,6 +453,7 @@ blk_status_t sd_zbc_prepare_zone_append(struct scsi_cmnd *cmd, sector_t *lba,
 			break;
 		}
 
+		trace_scsi_prepare_zone_append(cmd, *lba, wp_offset);
 		*lba += wp_offset;
 	}
 	spin_unlock_irqrestore(&sdkp->zones_wp_offset_lock, flags);
@@ -558,6 +562,8 @@ static unsigned int sd_zbc_zone_wp_update(struct scsi_cmnd *cmd,
 
 	switch (op) {
 	case REQ_OP_ZONE_APPEND:
+		trace_scsi_zone_wp_update(cmd, rq->__sector,
+				  sdkp->zones_wp_offset[zno], good_bytes);
 		rq->__sector += sdkp->zones_wp_offset[zno];
 		fallthrough;
 	case REQ_OP_WRITE_ZEROES:
diff --git a/include/trace/events/sd.h b/include/trace/events/sd.h
new file mode 100644
index 000000000000..0b11fed8327b
--- /dev/null
+++ b/include/trace/events/sd.h
@@ -0,0 +1,81 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 Western Digital Corporation or its affiliates.
+ */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM sd
+
+#if !defined(_SD_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_host.h>
+#include <linux/tracepoint.h>
+#include <linux/trace_seq.h>
+
+TRACE_EVENT(scsi_prepare_zone_append,
+
+	    TP_PROTO(struct scsi_cmnd *cmnd, sector_t lba,
+		     unsigned int wp_offset),
+
+	    TP_ARGS(cmnd, lba, wp_offset),
+
+	    TP_STRUCT__entry(
+		     __field( unsigned int, host_no )
+		     __field( unsigned int, channel )
+		     __field( unsigned int, id )
+		     __field( unsigned int, lun )
+		     __field( sector_t,     lba )
+		     __field( unsigned int, wp_offset )
+	    ),
+
+	    TP_fast_assign(
+		__entry->host_no	= cmnd->device->host->host_no;
+		__entry->channel	= cmnd->device->channel;
+		__entry->id		= cmnd->device->id;
+		__entry->lun		= cmnd->device->lun;
+		__entry->lba		= lba;
+		__entry->wp_offset	= wp_offset;
+	    ),
+
+	    TP_printk("host_no=%u, channel=%u id=%u lun=%u lba=%llu wp_offset=%u",
+		      __entry->host_no, __entry->channel, __entry->id,
+		      __entry->lun, __entry->lba, __entry->wp_offset)
+);
+
+TRACE_EVENT(scsi_zone_wp_update,
+
+	    TP_PROTO(struct scsi_cmnd *cmnd, sector_t rq_sector,
+		     unsigned int wp_offset, unsigned int good_bytes),
+
+	    TP_ARGS(cmnd, rq_sector, wp_offset, good_bytes),
+
+	    TP_STRUCT__entry(
+		     __field( unsigned int, host_no )
+		     __field( unsigned int, channel )
+		     __field( unsigned int, id )
+		     __field( unsigned int, lun )
+		     __field( sector_t,     rq_sector )
+		     __field( unsigned int, wp_offset )
+		     __field( unsigned int, good_bytes )
+	    ),
+
+	    TP_fast_assign(
+		__entry->host_no	= cmnd->device->host->host_no;
+		__entry->channel	= cmnd->device->channel;
+		__entry->id		= cmnd->device->id;
+		__entry->lun		= cmnd->device->lun;
+		__entry->rq_sector	= rq_sector;
+		__entry->wp_offset	= wp_offset;
+		__entry->good_bytes	= good_bytes;
+	    ),
+
+	    TP_printk("host_no=%u, channel=%u id=%u lun=%u rq_sector=%llu" \
+		      " wp_offset=%u good_bytes=%u",
+		      __entry->host_no, __entry->channel, __entry->id,
+		      __entry->lun, __entry->rq_sector, __entry->wp_offset,
+		      __entry->good_bytes)
+);
+#endif /* _SD_TRACE_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
+
-- 
2.38.1

Re: [PATCH v4] scsi: sd_zbc: trace zone append emulation

[Damien Le Moal]

On 11/30/22 17:39, Johannes Thumshirn wrote:
> Add tracepoints to the SCSI zone append emulation, in order to trace the
> zone start to write-pointer aligned LBA translation and the corresponding
> completion.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[...]

> diff --git a/include/trace/events/sd.h b/include/trace/events/sd.h
> new file mode 100644
> index 000000000000..0b11fed8327b
> --- /dev/null
> +++ b/include/trace/events/sd.h

Why not drivers/scsi/sd_trace.h ? That would work too, no ? Do we need
that header as a common thing under include/trace/events/ ?
-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4] scsi: sd_zbc: trace zone append emulation

[Christoph Hellwig]

On Wed, Nov 30, 2022 at 06:59:43PM +0900, Damien Le Moal wrote:
> Why not drivers/scsi/sd_trace.h ? That would work too, no ?

Yes, that's also what I suggested previously.

Re: [PATCH v4] scsi: sd_zbc: trace zone append emulation

[Johannes Thumshirn]

On 30.11.22 11:01, Christoph Hellwig wrote:
> On Wed, Nov 30, 2022 at 06:59:43PM +0900, Damien Le Moal wrote:
>> Why not drivers/scsi/sd_trace.h ? That would work too, no ?
> 
> Yes, that's also what I suggested previously.
> 

But what's the reasoning behind it? All other trace events
are in include/trace/events/.

Re: [PATCH v4] scsi: sd_zbc: trace zone append emulation

[hch]

On Wed, Nov 30, 2022 at 10:02:20AM +0000, Johannes Thumshirn wrote:
> But what's the reasoning behind it? All other trace events
> are in include/trace/events/.

No, they aren't.  Do a quick grep for TRACE_SYSTEM to show they mostly
are with the actual drivers and subsystems.

Re: [PATCH v4] scsi: sd_zbc: trace zone append emulation

[Johannes Thumshirn]

On 30.11.22 11:03, hch@infradead.org wrote:
> On Wed, Nov 30, 2022 at 10:02:20AM +0000, Johannes Thumshirn wrote:
>> But what's the reasoning behind it? All other trace events
>> are in include/trace/events/.
> 
> No, they aren't.  Do a quick grep for TRACE_SYSTEM to show they mostly
> are with the actual drivers and subsystems.
> 

For scsi it is in include/trace/events/scsi.h, but if you want
I'll move it to drivers/scsi.

Re: [PATCH v4] scsi: sd_zbc: trace zone append emulation

[Johannes Thumshirn]

On 30.11.22 10:59, Damien Le Moal wrote:
> On 11/30/22 17:39, Johannes Thumshirn wrote:
>> Add tracepoints to the SCSI zone append emulation, in order to trace the
>> zone start to write-pointer aligned LBA translation and the corresponding
>> completion.
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> [...]
> 
>> diff --git a/include/trace/events/sd.h b/include/trace/events/sd.h
>> new file mode 100644
>> index 000000000000..0b11fed8327b
>> --- /dev/null
>> +++ b/include/trace/events/sd.h
> 
> Why not drivers/scsi/sd_trace.h ? That would work too, no ? Do we need
> that header as a common thing under include/trace/events/ ?

Well it's the common place for tracepoint headers.

Re: [PATCH v4] scsi: sd_zbc: trace zone append emulation

[Christoph Hellwig]

On Wed, Nov 30, 2022 at 12:39:21AM -0800, Johannes Thumshirn wrote:
> +EXPORT_SYMBOL(scsi_trace_parse_cdb);

Unless I'm missing something you don't actually use this anywhere.

Re: [PATCH v4] scsi: sd_zbc: trace zone append emulation

[Johannes Thumshirn]

On 30.11.22 11:02, Christoph Hellwig wrote:
> On Wed, Nov 30, 2022 at 12:39:21AM -0800, Johannes Thumshirn wrote:
>> +EXPORT_SYMBOL(scsi_trace_parse_cdb);
> 
> Unless I'm missing something you don't actually use this anywhere.
> 

Gah correct, that's a leftover that isn't used.