Re: [PATCH RFC] block: trace: add block alignment information

From: John Garry
Date: Fri Sep 13 2024 - 05:58:50 EST

On 12/09/2024 21:48, Daniel Gomez via B4 Relay wrote:
From: Daniel Gomez <da.gomez@xxxxxxxxxxx>

Report block alignment in terms of LBA and size during block tracing for
block_rq. Calculate alignment only for read/writes where the length is
greater than 0. Otherwise, report 0 to indicate no alignment calculated.

Suggested-by: Dave Chinner <dchinner@xxxxxxxxxx>
Signed-off-by: Daniel Gomez <da.gomez@xxxxxxxxxxx>
---
This patch introduces LBA and size alignment information for
the block_rq tracepoints (block_rq{insert, issue, merge} and
block_{io_start, io_done}).

eh, shouldn't this belong in the description of the patch?

And I still don't know what we mean by alignment in this context.

From looking at the code, it seems to be the max detected block size granularity. For example, for a 64KB write at a 32KB offset, that would give a 32KB "alignment". But a 64KB write at a 64KB offset would be "64KB" alignment. While a 8KB write at 64KB offset would be 8KB "alignment". And a 24KB write at offset 0 is a 8KB "alignment", as 8KB is the lowest power-of-2 which is divisible into 24KB. Is this a correct understanding?


The idea of reporting alignment in a tracepoint was first suggested in
this thread [1] by Dave Chinner. Additionally, an eBPF-based equivalent
tracing tool [2] was developed and used during LBS development, as
mentioned in the patch series [3] and in [1].

With this addition, users can check block alignment directly through the
block layer tracepoints without needing any additional tools.

In case we have a use case, this can be extended to other tracepoints,
such as complete and error.

Another potential enhancement could be the integration of this
information into blktrace. Would that be a feasible option to consider?

[1] https://urldefense.com/v3/__https://lore.kernel.org/all/ZdvXAn1Q*2F*QX5sPQ@xxxxxxxxxxxxxxxxxxx/__;JSs!!ACWV5N9M2RV99hQ!P1ZM_E9uBSDLzz6M0dLc_vgEGWEY2HPBXJvEJLWp7w0l_G_r9Gvkm2kQiN586NSIH-JMx_YiCFy_6qdklHFY3pXtYsRb3aY$
[2] blkalgn tool written in eBPF/bcc:
https://urldefense.com/v3/__https://github.com/dkruces/bcc/tree/lbs__;!!ACWV5N9M2RV99hQ!P1ZM_E9uBSDLzz6M0dLc_vgEGWEY2HPBXJvEJLWp7w0l_G_r9Gvkm2kQiN586NSIH-JMx_YiCFy_6qdklHFY3pXthE7cfng$
[3] https://urldefense.com/v3/__https://lore.kernel.org/all/20240822135018.1931258-1-kernel@xxxxxxxxxxxxxxxx/__;!!ACWV5N9M2RV99hQ!P1ZM_E9uBSDLzz6M0dLc_vgEGWEY2HPBXJvEJLWp7w0l_G_r9Gvkm2kQiN586NSIH-JMx_YiCFy_6qdklHFY3pXtqQ5uwAE$
---
block/blk-mq.c | 29 +++++++++++++++++++++++++++++
include/linux/blk-mq.h | 11 +++++++++++
include/linux/blkdev.h | 6 ++++++
include/trace/events/block.h | 7 +++++--
4 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 831c5cf5d874..714452bc236b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4920,6 +4920,35 @@ int blk_rq_poll(struct request *rq, struct io_comp_batch *iob,
}
EXPORT_SYMBOL_GPL(blk_rq_poll);
+u32 __blk_rq_lba_algn(struct request *req)

why use "algn", and not "align"?

"algn" is not a natural abbreviation of "alignment".

And why can't userspace figure this out? All the info is available already, right?

+{
+ u32 lbs = queue_logical_block_size(req->q);
+ u32 lba_shift = ilog2(lbs);
+ u32 lba = req->__sector >> (lba_shift - SECTOR_SHIFT);

req->__sector is a u64 - is it safe to store the shifted value in a u32?

+ u32 len = req->__data_len;
+ u32 algn_len = len;
+ u32 algn_lba = len / lbs;
+ u32 alignment = lbs;
+
+ if (is_power_of_2(len) &&
+ blk_rq_lba_aligned(len, algn_len, lba, algn_lba))
+ return len;
+
+ algn_len = lbs << 1U;
+ algn_lba = algn_len / lbs;
+
+ while (algn_len < len) {
+ if (!blk_rq_lba_aligned(len, algn_len, lba, algn_lba))
+ break;
+
+ alignment = algn_len;
+ algn_len = algn_len << 1U;
+ algn_lba = algn_len / lbs;
+ }
+
+ return alignment;
+}
+
unsigned int blk_mq_rq_cpu(struct request *rq)
{
return rq->mq_ctx->cpu;
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 8d304b1d16b1..02959fbd5e28 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -740,6 +740,17 @@ void blk_mq_free_request(struct request *rq);
int blk_rq_poll(struct request *rq, struct io_comp_batch *iob,
unsigned int poll_flags);
+/* The alignment of the block in terms of LBA and size */
+u32 __blk_rq_lba_algn(struct request *req);
+static inline u32 blk_rq_lba_algn(struct request *req)
+{
+ if ((req_op(req) != REQ_OP_WRITE) && (req_op(req) != REQ_OP_READ) &&
+ !(req->__data_len))
+ return 0;
+
+ return __blk_rq_lba_algn(req);
+}
+
bool blk_mq_queue_inflight(struct request_queue *q);
enum {
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index bf1aa951fda2..28557987daa8 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1433,6 +1433,12 @@ static inline int blk_rq_aligned(struct request_queue *q, unsigned long addr,
return !(addr & alignment) && !(len & alignment);
}
+static inline bool blk_rq_lba_aligned(u32 len, u32 algn_len, u32 lba,
+ u32 algn_lba)
+{
+ return !(len % algn_len) && !(lba % algn_lba);
+}

why in blkdev.h? It is only used in block/blk-mq.c

+
/* assumes size > 256 */
static inline unsigned int blksize_bits(unsigned int size)
{
diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 1527d5d45e01..ba3764214dc7 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -202,6 +202,7 @@ DECLARE_EVENT_CLASS(block_rq,
__array( char, rwbs, RWBS_LEN )
__array( char, comm, TASK_COMM_LEN )
__dynamic_array( char, cmd, 1 )
+ __field( unsigned int, algn )
),
TP_fast_assign(
@@ -210,20 +211,22 @@ DECLARE_EVENT_CLASS(block_rq,
__entry->nr_sector = blk_rq_trace_nr_sectors(rq);
__entry->bytes = blk_rq_bytes(rq);
__entry->ioprio = rq->ioprio;
+ __entry->algn = blk_rq_lba_algn(rq);
blk_fill_rwbs(__entry->rwbs, rq->cmd_flags);
__get_str(cmd)[0] = '\0';
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
),
- TP_printk("%d,%d %s %u (%s) %llu + %u %s,%u,%u [%s]",
+ TP_printk("%d,%d %s %u (%s) %llu + %u %s,%u,%u |%u| [%s]",
MAJOR(__entry->dev), MINOR(__entry->dev),
__entry->rwbs, __entry->bytes, __get_str(cmd),
(unsigned long long)__entry->sector, __entry->nr_sector,
__print_symbolic(IOPRIO_PRIO_CLASS(__entry->ioprio),
IOPRIO_CLASS_STRINGS),
IOPRIO_PRIO_HINT(__entry->ioprio),
- IOPRIO_PRIO_LEVEL(__entry->ioprio), __entry->comm)
+ IOPRIO_PRIO_LEVEL(__entry->ioprio), __entry->algn,
+ __entry->comm)
);
/**

---
base-commit: 57f962b956f1d116cd64d5c406776c4975de549d
change-id: 20240912-add-blkalgn-block-trace-71e8ab6708f1

Best regards,