linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/10] block: blktrace framework cleanup
@ 2020-06-29 23:43 Chaitanya Kulkarni
  2020-06-29 23:43 ` [PATCH 01/11] " Chaitanya Kulkarni
                   ` (11 more replies)
  0 siblings, 12 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

Hi,

There are many places where trace API accepts the struct request_queue*
parameter which can be derived from other function parameters.

This patch-series removes the struct request queue parameter from the
blktrace framework and adjusts the tracepoints definition and usage
along with the tracing API itself.

Also with the queue parameter removed now we can merge some of the trace
events with creating generic event class for bio based tarce events.

Finally few of more cleanups to remove the trace_block_rq_insert()
wrapper blk_mq_sched_request_inserted(), get rid of the extra parameter
for trace_block_getrq, remove the blk_trace_request_get_cgid(),
fix tracepoint comment header,  and blk_fill_rwbs() related triggred
cleanup.

I've added patches after first cleanup as I scan the code for more
cleanup. I think patch distribution can be better but it will be great
to have some feedback as the amount of clenaup has grown bigger.

Any comments are welcome.

Regards,
Chaitanya

Chaitanya Kulkarni (11):
  block: blktrace framework cleanup
  block: rename block_bio_merge class to block_bio
  block: use block_bio event class for bio_queue
  block: use block_bio event class for bio_bounce
  block: get rid of the trace rq insert wrapper
  block: remove extra param for trace_block_getrq()
  block: get rid of blk_trace_request_get_cgid()
  block: fix the comments in the trace event block.h
  block: remove unsed param in blk_fill_rwbs()
  block: use block_bio class for getrq and sleeprq
  block: remove block_get_rq event class

 block/bfq-iosched.c           |   4 +-
 block/blk-core.c              |   6 +-
 block/blk-merge.c             |   4 +-
 block/blk-mq-sched.c          |   6 -
 block/blk-mq-sched.h          |   1 -
 block/blk-mq.c                |  10 +-
 block/bounce.c                |   2 +-
 block/kyber-iosched.c         |   4 +-
 block/mq-deadline.c           |   4 +-
 drivers/md/dm.c               |   4 +-
 include/linux/blktrace_api.h  |   2 +-
 include/trace/events/bcache.h |  10 +-
 include/trace/events/block.h  | 226 +++++++++++-----------------------
 kernel/trace/blktrace.c       | 129 +++++++++----------
 14 files changed, 161 insertions(+), 251 deletions(-)

-- 
2.26.0


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 01/11] block: blktrace framework cleanup
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
@ 2020-06-29 23:43 ` Chaitanya Kulkarni
  2020-06-30  5:10   ` Christoph Hellwig
  2020-06-29 23:43 ` [PATCH 02/11] block: rename block_bio_merge class to block_bio Chaitanya Kulkarni
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

This patch removes the extra variables from the trace events and
overall kernel blktrace framework. The removed members can easily be
extracted from the remaining argument which reduces the code
significantly and allows us to optimize the several tracepoints like
the next patch in the series.      

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 block/blk-core.c             |  6 +--
 block/blk-merge.c            |  4 +-
 block/blk-mq-sched.c         |  2 +-
 block/blk-mq.c               | 10 ++--
 block/bounce.c               |  2 +-
 drivers/md/dm.c              |  4 +-
 include/trace/events/block.h | 87 +++++++++++++-------------------
 kernel/trace/blktrace.c      | 98 ++++++++++++++++++------------------
 8 files changed, 98 insertions(+), 115 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 72b102a389a5..6d4c57ef4533 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -674,7 +674,7 @@ bool bio_attempt_back_merge(struct request *req, struct bio *bio,
 	if (!ll_back_merge_fn(req, bio, nr_segs))
 		return false;
 
-	trace_block_bio_backmerge(req->q, req, bio);
+	trace_block_bio_backmerge(bio);
 	rq_qos_merge(req->q, req, bio);
 
 	if ((req->cmd_flags & REQ_FAILFAST_MASK) != ff)
@@ -698,7 +698,7 @@ bool bio_attempt_front_merge(struct request *req, struct bio *bio,
 	if (!ll_front_merge_fn(req, bio, nr_segs))
 		return false;
 
-	trace_block_bio_frontmerge(req->q, req, bio);
+	trace_block_bio_frontmerge(bio);
 	rq_qos_merge(req->q, req, bio);
 
 	if ((req->cmd_flags & REQ_FAILFAST_MASK) != ff)
@@ -1082,7 +1082,7 @@ generic_make_request_checks(struct bio *bio)
 		return false;
 
 	if (!bio_flagged(bio, BIO_TRACE_COMPLETION)) {
-		trace_block_bio_queue(q, bio);
+		trace_block_bio_queue(bio);
 		/* Now that enqueuing has been traced, we need to trace
 		 * completion as well.
 		 */
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 9c9fb21584b6..8333ccd60ee1 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -337,7 +337,7 @@ void __blk_queue_split(struct request_queue *q, struct bio **bio,
 		split->bi_opf |= REQ_NOMERGE;
 
 		bio_chain(split, *bio);
-		trace_block_split(q, split, (*bio)->bi_iter.bi_sector);
+		trace_block_split(split, (*bio)->bi_iter.bi_sector);
 		generic_make_request(*bio);
 		*bio = split;
 	}
@@ -793,7 +793,7 @@ static struct request *attempt_merge(struct request_queue *q,
 	 */
 	blk_account_io_merge_request(next);
 
-	trace_block_rq_merge(q, next);
+	trace_block_rq_merge(next);
 
 	/*
 	 * ownership of bio passed from next to req, return 'next' for
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index fdcc2c1dd178..a3cade16ef80 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -409,7 +409,7 @@ EXPORT_SYMBOL_GPL(blk_mq_sched_try_insert_merge);
 
 void blk_mq_sched_request_inserted(struct request *rq)
 {
-	trace_block_rq_insert(rq->q, rq);
+	trace_block_rq_insert(rq);
 }
 EXPORT_SYMBOL_GPL(blk_mq_sched_request_inserted);
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index b8738b3c6d06..dbb98b2bc868 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -742,7 +742,7 @@ void blk_mq_start_request(struct request *rq)
 {
 	struct request_queue *q = rq->q;
 
-	trace_block_rq_issue(q, rq);
+	trace_block_rq_issue(rq);
 
 	if (test_bit(QUEUE_FLAG_STATS, &q->queue_flags)) {
 		rq->io_start_time_ns = ktime_get_ns();
@@ -769,7 +769,7 @@ static void __blk_mq_requeue_request(struct request *rq)
 
 	blk_mq_put_driver_tag(rq);
 
-	trace_block_rq_requeue(q, rq);
+	trace_block_rq_requeue(rq);
 	rq_qos_requeue(q, rq);
 
 	if (blk_mq_request_started(rq)) {
@@ -1758,7 +1758,7 @@ static inline void __blk_mq_insert_req_list(struct blk_mq_hw_ctx *hctx,
 
 	lockdep_assert_held(&ctx->lock);
 
-	trace_block_rq_insert(hctx->queue, rq);
+	trace_block_rq_insert(rq);
 
 	if (at_head)
 		list_add(&rq->queuelist, &ctx->rq_lists[type]);
@@ -1814,7 +1814,7 @@ void blk_mq_insert_requests(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx,
 	 */
 	list_for_each_entry(rq, list, queuelist) {
 		BUG_ON(rq->mq_ctx != ctx);
-		trace_block_rq_insert(hctx->queue, rq);
+		trace_block_rq_insert(rq);
 	}
 
 	spin_lock(&ctx->lock);
@@ -2111,7 +2111,7 @@ blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
 		goto queue_exit;
 	}
 
-	trace_block_getrq(q, bio, bio->bi_opf);
+	trace_block_getrq(bio, bio->bi_opf);
 
 	rq_qos_track(q, rq, bio);
 
diff --git a/block/bounce.c b/block/bounce.c
index c3aaed070124..9550640b1f86 100644
--- a/block/bounce.c
+++ b/block/bounce.c
@@ -341,7 +341,7 @@ static void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig,
 		}
 	}
 
-	trace_block_bio_bounce(q, *bio_orig);
+	trace_block_bio_bounce(*bio_orig);
 
 	bio->bi_flags |= (1 << BIO_BOUNCED);
 
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 109e81f33edb..4b9ff226904d 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1678,7 +1678,7 @@ static blk_qc_t __split_and_process_bio(struct mapped_device *md,
 				part_stat_unlock();
 
 				bio_chain(b, bio);
-				trace_block_split(md->queue, b, bio->bi_iter.bi_sector);
+				trace_block_split(b, bio->bi_iter.bi_sector);
 				ret = generic_make_request(bio);
 				break;
 			}
@@ -1745,7 +1745,7 @@ static void dm_queue_split(struct mapped_device *md, struct dm_target *ti, struc
 		struct bio *split = bio_split(*bio, len, GFP_NOIO, &md->queue->bio_split);
 
 		bio_chain(split, *bio);
-		trace_block_split(md->queue, split, (*bio)->bi_iter.bi_sector);
+		trace_block_split(split, (*bio)->bi_iter.bi_sector);
 		generic_make_request(*bio);
 		*bio = split;
 	}
diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 34d64ca306b1..237d40a48429 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -64,7 +64,6 @@ DEFINE_EVENT(block_buffer, block_dirty_buffer,
 
 /**
  * block_rq_requeue - place block IO request back on a queue
- * @q: queue holding operation
  * @rq: block IO operation request
  *
  * The block operation request @rq is being placed back into queue
@@ -73,9 +72,9 @@ DEFINE_EVENT(block_buffer, block_dirty_buffer,
  */
 TRACE_EVENT(block_rq_requeue,
 
-	TP_PROTO(struct request_queue *q, struct request *rq),
+	TP_PROTO(struct request *rq),
 
-	TP_ARGS(q, rq),
+	TP_ARGS(rq),
 
 	TP_STRUCT__entry(
 		__field(  dev_t,	dev			)
@@ -103,7 +102,6 @@ TRACE_EVENT(block_rq_requeue,
 
 /**
  * block_rq_complete - block IO operation completed by device driver
- * @rq: block operations request
  * @error: status code
  * @nr_bytes: number of completed bytes
  *
@@ -147,9 +145,9 @@ TRACE_EVENT(block_rq_complete,
 
 DECLARE_EVENT_CLASS(block_rq,
 
-	TP_PROTO(struct request_queue *q, struct request *rq),
+	TP_PROTO(struct request *rq),
 
-	TP_ARGS(q, rq),
+	TP_ARGS(rq),
 
 	TP_STRUCT__entry(
 		__field(  dev_t,	dev			)
@@ -181,24 +179,22 @@ DECLARE_EVENT_CLASS(block_rq,
 
 /**
  * block_rq_insert - insert block operation request into queue
- * @q: target queue
  * @rq: block IO operation request
  *
  * Called immediately before block operation request @rq is inserted
- * into queue @q.  The fields in the operation request @rq struct can
+ * into queue.  The fields in the operation request @rq struct can
  * be examined to determine which device and sectors the pending
  * operation would access.
  */
 DEFINE_EVENT(block_rq, block_rq_insert,
 
-	TP_PROTO(struct request_queue *q, struct request *rq),
+	TP_PROTO(struct request *rq),
 
-	TP_ARGS(q, rq)
+	TP_ARGS(rq)
 );
 
 /**
  * block_rq_issue - issue pending block IO request operation to device driver
- * @q: queue holding operation
  * @rq: block IO operation operation request
  *
  * Called when block operation request @rq from queue @q is sent to a
@@ -206,32 +202,30 @@ DEFINE_EVENT(block_rq, block_rq_insert,
  */
 DEFINE_EVENT(block_rq, block_rq_issue,
 
-	TP_PROTO(struct request_queue *q, struct request *rq),
+	TP_PROTO(struct request *rq),
 
-	TP_ARGS(q, rq)
+	TP_ARGS(rq)
 );
 
 /**
  * block_rq_merge - merge request with another one in the elevator
- * @q: queue holding operation
  * @rq: block IO operation operation request
  *
- * Called when block operation request @rq from queue @q is merged to another
+ * Called when block operation request @rq from queue is merged to another
  * request queued in the elevator.
  */
 DEFINE_EVENT(block_rq, block_rq_merge,
 
-	TP_PROTO(struct request_queue *q, struct request *rq),
+	TP_PROTO(struct request *rq),
 
-	TP_ARGS(q, rq)
+	TP_ARGS(rq)
 );
 
 /**
  * block_bio_bounce - used bounce buffer when processing block operation
- * @q: queue holding the block operation
  * @bio: block operation
  *
- * A bounce buffer was used to handle the block operation @bio in @q.
+ * A bounce buffer was used to handle the block operation @bio in queue.
  * This occurs when hardware limitations prevent a direct transfer of
  * data between the @bio data memory area and the IO device.  Use of a
  * bounce buffer requires extra copying of data and decreases
@@ -239,9 +233,9 @@ DEFINE_EVENT(block_rq, block_rq_merge,
  */
 TRACE_EVENT(block_bio_bounce,
 
-	TP_PROTO(struct request_queue *q, struct bio *bio),
+	TP_PROTO(struct bio *bio),
 
-	TP_ARGS(q, bio),
+	TP_ARGS(bio),
 
 	TP_STRUCT__entry(
 		__field( dev_t,		dev			)
@@ -303,9 +297,9 @@ TRACE_EVENT(block_bio_complete,
 
 DECLARE_EVENT_CLASS(block_bio_merge,
 
-	TP_PROTO(struct request_queue *q, struct request *rq, struct bio *bio),
+	TP_PROTO(struct bio *bio),
 
-	TP_ARGS(q, rq, bio),
+	TP_ARGS(bio),
 
 	TP_STRUCT__entry(
 		__field( dev_t,		dev			)
@@ -331,48 +325,43 @@ DECLARE_EVENT_CLASS(block_bio_merge,
 
 /**
  * block_bio_backmerge - merging block operation to the end of an existing operation
- * @q: queue holding operation
- * @rq: request bio is being merged into
  * @bio: new block operation to merge
  *
  * Merging block request @bio to the end of an existing block request
- * in queue @q.
+ * in queue.
  */
 DEFINE_EVENT(block_bio_merge, block_bio_backmerge,
 
-	TP_PROTO(struct request_queue *q, struct request *rq, struct bio *bio),
+	TP_PROTO(struct bio *bio),
 
-	TP_ARGS(q, rq, bio)
+	TP_ARGS(bio)
 );
 
 /**
  * block_bio_frontmerge - merging block operation to the beginning of an existing operation
- * @q: queue holding operation
- * @rq: request bio is being merged into
  * @bio: new block operation to merge
  *
  * Merging block IO operation @bio to the beginning of an existing block
- * operation in queue @q.
+ * operation in queue.
  */
 DEFINE_EVENT(block_bio_merge, block_bio_frontmerge,
 
-	TP_PROTO(struct request_queue *q, struct request *rq, struct bio *bio),
+	TP_PROTO(struct bio *bio),
 
-	TP_ARGS(q, rq, bio)
+	TP_ARGS(bio)
 );
 
 /**
  * block_bio_queue - putting new block IO operation in queue
- * @q: queue holding operation
  * @bio: new block operation
  *
- * About to place the block IO operation @bio into queue @q.
+ * About to place the block IO operation @bio into queue.
  */
 TRACE_EVENT(block_bio_queue,
 
-	TP_PROTO(struct request_queue *q, struct bio *bio),
+	TP_PROTO(struct bio *bio),
 
-	TP_ARGS(q, bio),
+	TP_ARGS(bio),
 
 	TP_STRUCT__entry(
 		__field( dev_t,		dev			)
@@ -398,9 +387,9 @@ TRACE_EVENT(block_bio_queue,
 
 DECLARE_EVENT_CLASS(block_get_rq,
 
-	TP_PROTO(struct request_queue *q, struct bio *bio, int rw),
+	TP_PROTO(struct bio *bio, int rw),
 
-	TP_ARGS(q, bio, rw),
+	TP_ARGS(bio, rw),
 
 	TP_STRUCT__entry(
 		__field( dev_t,		dev			)
@@ -427,36 +416,34 @@ DECLARE_EVENT_CLASS(block_get_rq,
 
 /**
  * block_getrq - get a free request entry in queue for block IO operations
- * @q: queue for operations
  * @bio: pending block IO operation (can be %NULL)
  * @rw: low bit indicates a read (%0) or a write (%1)
  *
- * A request struct for queue @q has been allocated to handle the
+ * A request struct for queue has been allocated to handle the
  * block IO operation @bio.
  */
 DEFINE_EVENT(block_get_rq, block_getrq,
 
-	TP_PROTO(struct request_queue *q, struct bio *bio, int rw),
+	TP_PROTO(struct bio *bio, int rw),
 
-	TP_ARGS(q, bio, rw)
+	TP_ARGS(bio, rw)
 );
 
 /**
  * block_sleeprq - waiting to get a free request entry in queue for block IO operation
- * @q: queue for operation
  * @bio: pending block IO operation (can be %NULL)
  * @rw: low bit indicates a read (%0) or a write (%1)
  *
- * In the case where a request struct cannot be provided for queue @q
+ * In the case where a request struct cannot be provided for queue
  * the process needs to wait for an request struct to become
  * available.  This tracepoint event is generated each time the
  * process goes to sleep waiting for request struct become available.
  */
 DEFINE_EVENT(block_get_rq, block_sleeprq,
 
-	TP_PROTO(struct request_queue *q, struct bio *bio, int rw),
+	TP_PROTO(struct bio *bio, int rw),
 
-	TP_ARGS(q, bio, rw)
+	TP_ARGS(bio, rw)
 );
 
 /**
@@ -521,7 +508,6 @@ DEFINE_EVENT(block_unplug, block_unplug,
 
 /**
  * block_split - split a single bio struct into two bio structs
- * @q: queue containing the bio
  * @bio: block operation being split
  * @new_sector: The starting sector for the new bio
  *
@@ -532,10 +518,9 @@ DEFINE_EVENT(block_unplug, block_unplug,
  */
 TRACE_EVENT(block_split,
 
-	TP_PROTO(struct request_queue *q, struct bio *bio,
-		 unsigned int new_sector),
+	TP_PROTO(struct bio *bio, unsigned int new_sector),
 
-	TP_ARGS(q, bio, new_sector),
+	TP_ARGS(bio, new_sector),
 
 	TP_STRUCT__entry(
 		__field( dev_t,		dev				)
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 7ba62d68885a..7b72781a591d 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -28,6 +28,11 @@
 
 #ifdef CONFIG_BLK_DEV_IO_TRACE
 
+static inline struct request_queue *bio_q(struct bio *bio)
+{
+	return bio->bi_disk->queue;
+}
+
 static unsigned int blktrace_seq __read_mostly = 1;
 
 static struct trace_array *blk_tr;
@@ -846,33 +851,28 @@ static void blk_add_trace_rq(struct request *rq, int error,
 	rcu_read_unlock();
 }
 
-static void blk_add_trace_rq_insert(void *ignore,
-				    struct request_queue *q, struct request *rq)
+static void blk_add_trace_rq_insert(void *ignore, struct request *rq)
 {
 	blk_add_trace_rq(rq, 0, blk_rq_bytes(rq), BLK_TA_INSERT,
-			 blk_trace_request_get_cgid(q, rq));
+			 blk_trace_request_get_cgid(rq->q, rq));
 }
 
-static void blk_add_trace_rq_issue(void *ignore,
-				   struct request_queue *q, struct request *rq)
+static void blk_add_trace_rq_issue(void *ignore, struct request *rq)
 {
 	blk_add_trace_rq(rq, 0, blk_rq_bytes(rq), BLK_TA_ISSUE,
-			 blk_trace_request_get_cgid(q, rq));
+			 blk_trace_request_get_cgid(rq->q, rq));
 }
 
-static void blk_add_trace_rq_merge(void *ignore,
-				   struct request_queue *q, struct request *rq)
+static void blk_add_trace_rq_merge(void *ignore, struct request *rq)
 {
 	blk_add_trace_rq(rq, 0, blk_rq_bytes(rq), BLK_TA_BACKMERGE,
-			 blk_trace_request_get_cgid(q, rq));
+			 blk_trace_request_get_cgid(rq->q, rq));
 }
 
-static void blk_add_trace_rq_requeue(void *ignore,
-				     struct request_queue *q,
-				     struct request *rq)
+static void blk_add_trace_rq_requeue(void *ignore, struct request *rq)
 {
 	blk_add_trace_rq(rq, 0, blk_rq_bytes(rq), BLK_TA_REQUEUE,
-			 blk_trace_request_get_cgid(q, rq));
+			 blk_trace_request_get_cgid(rq->q, rq));
 }
 
 static void blk_add_trace_rq_complete(void *ignore, struct request *rq,
@@ -893,13 +893,12 @@ static void blk_add_trace_rq_complete(void *ignore, struct request *rq,
  *     Records an action against a bio. Will log the bio offset + size.
  *
  **/
-static void blk_add_trace_bio(struct request_queue *q, struct bio *bio,
-			      u32 what, int error)
+static void blk_add_trace_bio(struct bio *bio, u32 what, int error)
 {
 	struct blk_trace *bt;
 
 	rcu_read_lock();
-	bt = rcu_dereference(q->blk_trace);
+	bt = rcu_dereference(bio_q(bio)->blk_trace);
 	if (likely(!bt)) {
 		rcu_read_unlock();
 		return;
@@ -907,56 +906,59 @@ static void blk_add_trace_bio(struct request_queue *q, struct bio *bio,
 
 	__blk_add_trace(bt, bio->bi_iter.bi_sector, bio->bi_iter.bi_size,
 			bio_op(bio), bio->bi_opf, what, error, 0, NULL,
-			blk_trace_bio_get_cgid(q, bio));
+			blk_trace_bio_get_cgid(bio_q(bio), bio));
 	rcu_read_unlock();
 }
 
-static void blk_add_trace_bio_bounce(void *ignore,
-				     struct request_queue *q, struct bio *bio)
+static void blk_add_trace_bio_bounce(void *ignore, struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_BOUNCE, 0);
+	blk_add_trace_bio(bio, BLK_TA_BOUNCE, 0);
 }
 
-static void blk_add_trace_bio_complete(void *ignore,
-				       struct request_queue *q, struct bio *bio)
+static void blk_add_trace_bio_complete(void *ignore, struct request_queue *q,
+				       struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_COMPLETE,
-			  blk_status_to_errno(bio->bi_status));
+	struct blk_trace *bt;
+
+	rcu_read_lock();
+	bt = rcu_dereference(q->blk_trace);
+	if (likely(!bt)) {
+		rcu_read_unlock();
+		return;
+	}
+
+	__blk_add_trace(bt, bio->bi_iter.bi_sector, bio->bi_iter.bi_size,
+			bio_op(bio), bio->bi_opf, BLK_TA_COMPLETE,
+			blk_status_to_errno(bio->bi_status), 0, NULL,
+			blk_trace_bio_get_cgid(q, bio));
+	rcu_read_unlock();
 }
 
-static void blk_add_trace_bio_backmerge(void *ignore,
-					struct request_queue *q,
-					struct request *rq,
-					struct bio *bio)
+static void blk_add_trace_bio_backmerge(void *ignore, struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_BACKMERGE, 0);
+	blk_add_trace_bio(bio, BLK_TA_BACKMERGE, 0);
 }
 
-static void blk_add_trace_bio_frontmerge(void *ignore,
-					 struct request_queue *q,
-					 struct request *rq,
-					 struct bio *bio)
+static void blk_add_trace_bio_frontmerge(void *ignore, struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_FRONTMERGE, 0);
+	blk_add_trace_bio(bio, BLK_TA_FRONTMERGE, 0);
 }
 
-static void blk_add_trace_bio_queue(void *ignore,
-				    struct request_queue *q, struct bio *bio)
+static void blk_add_trace_bio_queue(void *ignore, struct bio *bio)
 {
-	blk_add_trace_bio(q, bio, BLK_TA_QUEUE, 0);
+	blk_add_trace_bio(bio, BLK_TA_QUEUE, 0);
 }
 
 static void blk_add_trace_getrq(void *ignore,
-				struct request_queue *q,
 				struct bio *bio, int rw)
 {
 	if (bio)
-		blk_add_trace_bio(q, bio, BLK_TA_GETRQ, 0);
+		blk_add_trace_bio(bio, BLK_TA_GETRQ, 0);
 	else {
 		struct blk_trace *bt;
 
 		rcu_read_lock();
-		bt = rcu_dereference(q->blk_trace);
+		bt = rcu_dereference(bio_q(bio)->blk_trace);
 		if (bt)
 			__blk_add_trace(bt, 0, 0, rw, 0, BLK_TA_GETRQ, 0, 0,
 					NULL, 0);
@@ -965,17 +967,15 @@ static void blk_add_trace_getrq(void *ignore,
 }
 
 
-static void blk_add_trace_sleeprq(void *ignore,
-				  struct request_queue *q,
-				  struct bio *bio, int rw)
+static void blk_add_trace_sleeprq(void *ignore, struct bio *bio, int rw)
 {
 	if (bio)
-		blk_add_trace_bio(q, bio, BLK_TA_SLEEPRQ, 0);
+		blk_add_trace_bio(bio, BLK_TA_SLEEPRQ, 0);
 	else {
 		struct blk_trace *bt;
 
 		rcu_read_lock();
-		bt = rcu_dereference(q->blk_trace);
+		bt = rcu_dereference(bio_q(bio)->blk_trace);
 		if (bt)
 			__blk_add_trace(bt, 0, 0, rw, 0, BLK_TA_SLEEPRQ,
 					0, 0, NULL, 0);
@@ -1015,14 +1015,12 @@ static void blk_add_trace_unplug(void *ignore, struct request_queue *q,
 	rcu_read_unlock();
 }
 
-static void blk_add_trace_split(void *ignore,
-				struct request_queue *q, struct bio *bio,
-				unsigned int pdu)
+static void blk_add_trace_split(void *ignore, struct bio *bio, unsigned int pdu)
 {
 	struct blk_trace *bt;
 
 	rcu_read_lock();
-	bt = rcu_dereference(q->blk_trace);
+	bt = rcu_dereference(bio_q(bio)->blk_trace);
 	if (bt) {
 		__be64 rpdu = cpu_to_be64(pdu);
 
@@ -1031,7 +1029,7 @@ static void blk_add_trace_split(void *ignore,
 				BLK_TA_SPLIT,
 				blk_status_to_errno(bio->bi_status),
 				sizeof(rpdu), &rpdu,
-				blk_trace_bio_get_cgid(q, bio));
+				blk_trace_bio_get_cgid(bio_q(bio), bio));
 	}
 	rcu_read_unlock();
 }
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 02/11] block: rename block_bio_merge class to block_bio
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
  2020-06-29 23:43 ` [PATCH 01/11] " Chaitanya Kulkarni
@ 2020-06-29 23:43 ` Chaitanya Kulkarni
  2020-06-30  5:10   ` Christoph Hellwig
  2020-06-29 23:43 ` [PATCH 03/11] block: use block_bio event class for bio_queue Chaitanya Kulkarni
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

There are identical TRACE_EVENTS presents which can now take an
advantage of the block_bio_merge trace event class.

This is a prep patch which renames block_bio_merge to block_bio so
that the next patches in this series will be able to resue it.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 include/trace/events/block.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 237d40a48429..b5be387c4115 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -295,7 +295,7 @@ TRACE_EVENT(block_bio_complete,
 		  __entry->nr_sector, __entry->error)
 );
 
-DECLARE_EVENT_CLASS(block_bio_merge,
+DECLARE_EVENT_CLASS(block_bio,
 
 	TP_PROTO(struct bio *bio),
 
@@ -330,7 +330,7 @@ DECLARE_EVENT_CLASS(block_bio_merge,
  * Merging block request @bio to the end of an existing block request
  * in queue.
  */
-DEFINE_EVENT(block_bio_merge, block_bio_backmerge,
+DEFINE_EVENT(block_bio, block_bio_backmerge,
 
 	TP_PROTO(struct bio *bio),
 
@@ -344,7 +344,7 @@ DEFINE_EVENT(block_bio_merge, block_bio_backmerge,
  * Merging block IO operation @bio to the beginning of an existing block
  * operation in queue.
  */
-DEFINE_EVENT(block_bio_merge, block_bio_frontmerge,
+DEFINE_EVENT(block_bio, block_bio_frontmerge,
 
 	TP_PROTO(struct bio *bio),
 
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 03/11] block: use block_bio event class for bio_queue
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
  2020-06-29 23:43 ` [PATCH 01/11] " Chaitanya Kulkarni
  2020-06-29 23:43 ` [PATCH 02/11] block: rename block_bio_merge class to block_bio Chaitanya Kulkarni
@ 2020-06-29 23:43 ` Chaitanya Kulkarni
  2020-06-29 23:43 ` [PATCH 04/11] block: use block_bio event class for bio_bounce Chaitanya Kulkarni
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

Remove block_bio_queue trace event which shares the code with block_bio.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 include/trace/events/block.h | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index b5be387c4115..5e9f46469f50 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -357,32 +357,12 @@ DEFINE_EVENT(block_bio, block_bio_frontmerge,
  *
  * About to place the block IO operation @bio into queue.
  */
-TRACE_EVENT(block_bio_queue,
 
-	TP_PROTO(struct bio *bio),
-
-	TP_ARGS(bio),
-
-	TP_STRUCT__entry(
-		__field( dev_t,		dev			)
-		__field( sector_t,	sector			)
-		__field( unsigned int,	nr_sector		)
-		__array( char,		rwbs,	RWBS_LEN	)
-		__array( char,		comm,	TASK_COMM_LEN	)
-	),
+DEFINE_EVENT(block_bio, block_bio_queue,
 
-	TP_fast_assign(
-		__entry->dev		= bio_dev(bio);
-		__entry->sector		= bio->bi_iter.bi_sector;
-		__entry->nr_sector	= bio_sectors(bio);
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf, bio->bi_iter.bi_size);
-		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
-	),
+	TP_PROTO(struct bio *bio),
 
-	TP_printk("%d,%d %s %llu + %u [%s]",
-		  MAJOR(__entry->dev), MINOR(__entry->dev), __entry->rwbs,
-		  (unsigned long long)__entry->sector,
-		  __entry->nr_sector, __entry->comm)
+	TP_ARGS(bio)
 );
 
 DECLARE_EVENT_CLASS(block_get_rq,
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 04/11] block: use block_bio event class for bio_bounce
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
                   ` (2 preceding siblings ...)
  2020-06-29 23:43 ` [PATCH 03/11] block: use block_bio event class for bio_queue Chaitanya Kulkarni
@ 2020-06-29 23:43 ` Chaitanya Kulkarni
  2020-06-29 23:43 ` [PATCH 05/11] block: get rid of the trace rq insert wrapper Chaitanya Kulkarni
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

Remove block_bio_bounce trace event which shares the code with block_bio.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 include/trace/events/block.h | 56 ++++++++++++------------------------
 1 file changed, 18 insertions(+), 38 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 5e9f46469f50..d7289576f1fd 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -221,44 +221,6 @@ DEFINE_EVENT(block_rq, block_rq_merge,
 	TP_ARGS(rq)
 );
 
-/**
- * block_bio_bounce - used bounce buffer when processing block operation
- * @bio: block operation
- *
- * A bounce buffer was used to handle the block operation @bio in queue.
- * This occurs when hardware limitations prevent a direct transfer of
- * data between the @bio data memory area and the IO device.  Use of a
- * bounce buffer requires extra copying of data and decreases
- * performance.
- */
-TRACE_EVENT(block_bio_bounce,
-
-	TP_PROTO(struct bio *bio),
-
-	TP_ARGS(bio),
-
-	TP_STRUCT__entry(
-		__field( dev_t,		dev			)
-		__field( sector_t,	sector			)
-		__field( unsigned int,	nr_sector		)
-		__array( char,		rwbs,	RWBS_LEN	)
-		__array( char,		comm,	TASK_COMM_LEN	)
-	),
-
-	TP_fast_assign(
-		__entry->dev		= bio_dev(bio);
-		__entry->sector		= bio->bi_iter.bi_sector;
-		__entry->nr_sector	= bio_sectors(bio);
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf, bio->bi_iter.bi_size);
-		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
-	),
-
-	TP_printk("%d,%d %s %llu + %u [%s]",
-		  MAJOR(__entry->dev), MINOR(__entry->dev), __entry->rwbs,
-		  (unsigned long long)__entry->sector,
-		  __entry->nr_sector, __entry->comm)
-);
-
 /**
  * block_bio_complete - completed all work on the block operation
  * @q: queue holding the block operation
@@ -351,6 +313,24 @@ DEFINE_EVENT(block_bio, block_bio_frontmerge,
 	TP_ARGS(bio)
 );
 
+/**
+ * block_bio_bounce - used bounce buffer when processing block operation
+ * @bio: block operation
+ *
+ * A bounce buffer was used to handle the block operation @bio in queue.
+ * This occurs when hardware limitations prevent a direct transfer of
+ * data between the @bio data memory area and the IO device.  Use of a
+ * bounce buffer requires extra copying of data and decreases
+ * performance.
+ */
+
+DEFINE_EVENT(block_bio, block_bio_bounce,
+
+	TP_PROTO(struct bio *bio),
+
+	TP_ARGS(bio)
+);
+
 /**
  * block_bio_queue - putting new block IO operation in queue
  * @bio: new block operation
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 05/11] block: get rid of the trace rq insert wrapper
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
                   ` (3 preceding siblings ...)
  2020-06-29 23:43 ` [PATCH 04/11] block: use block_bio event class for bio_bounce Chaitanya Kulkarni
@ 2020-06-29 23:43 ` Chaitanya Kulkarni
  2020-06-30  5:11   ` Christoph Hellwig
  2020-07-03 23:29   ` Chaitanya Kulkarni
  2020-06-29 23:43 ` [PATCH 06/11] block: remove extra param for trace_block_getrq() Chaitanya Kulkarni
                   ` (6 subsequent siblings)
  11 siblings, 2 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

Get rid of the wrapper for trace_block_rq_insert() and call the function
directly.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 block/bfq-iosched.c   | 4 +++-
 block/blk-mq-sched.c  | 6 ------
 block/blk-mq-sched.h  | 1 -
 block/kyber-iosched.c | 4 +++-
 block/mq-deadline.c   | 4 +++-
 5 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 50c8f034c01c..e2b9b700ed34 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -125,6 +125,8 @@
 #include <linux/delay.h>
 #include <linux/backing-dev.h>
 
+#include <trace/events/block.h>
+
 #include "blk.h"
 #include "blk-mq.h"
 #include "blk-mq-tag.h"
@@ -5507,7 +5509,7 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
 
 	spin_unlock_irq(&bfqd->lock);
 
-	blk_mq_sched_request_inserted(rq);
+	trace_block_rq_insert(rq);
 
 	spin_lock_irq(&bfqd->lock);
 	bfqq = bfq_init_rq(rq);
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index a3cade16ef80..20b6a59fbd5a 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -407,12 +407,6 @@ bool blk_mq_sched_try_insert_merge(struct request_queue *q, struct request *rq)
 }
 EXPORT_SYMBOL_GPL(blk_mq_sched_try_insert_merge);
 
-void blk_mq_sched_request_inserted(struct request *rq)
-{
-	trace_block_rq_insert(rq);
-}
-EXPORT_SYMBOL_GPL(blk_mq_sched_request_inserted);
-
 static bool blk_mq_sched_bypass_insert(struct blk_mq_hw_ctx *hctx,
 				       bool has_sched,
 				       struct request *rq)
diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
index 126021fc3a11..04c40c695bf0 100644
--- a/block/blk-mq-sched.h
+++ b/block/blk-mq-sched.h
@@ -10,7 +10,6 @@ void blk_mq_sched_free_hctx_data(struct request_queue *q,
 
 void blk_mq_sched_assign_ioc(struct request *rq);
 
-void blk_mq_sched_request_inserted(struct request *rq);
 bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio,
 		unsigned int nr_segs, struct request **merged_request);
 bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
diff --git a/block/kyber-iosched.c b/block/kyber-iosched.c
index a38c5ab103d1..e42d78deee90 100644
--- a/block/kyber-iosched.c
+++ b/block/kyber-iosched.c
@@ -13,6 +13,8 @@
 #include <linux/module.h>
 #include <linux/sbitmap.h>
 
+#include <trace/events/block.h>
+
 #include "blk.h"
 #include "blk-mq.h"
 #include "blk-mq-debugfs.h"
@@ -602,7 +604,7 @@ static void kyber_insert_requests(struct blk_mq_hw_ctx *hctx,
 			list_move_tail(&rq->queuelist, head);
 		sbitmap_set_bit(&khd->kcq_map[sched_domain],
 				rq->mq_ctx->index_hw[hctx->type]);
-		blk_mq_sched_request_inserted(rq);
+		trace_block_rq_insert(rq);
 		spin_unlock(&kcq->lock);
 	}
 }
diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index b57470e154c8..f3631a287466 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -18,6 +18,8 @@
 #include <linux/rbtree.h>
 #include <linux/sbitmap.h>
 
+#include <trace/events/block.h>
+
 #include "blk.h"
 #include "blk-mq.h"
 #include "blk-mq-debugfs.h"
@@ -496,7 +498,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
 	if (blk_mq_sched_try_insert_merge(q, rq))
 		return;
 
-	blk_mq_sched_request_inserted(rq);
+	trace_block_rq_insert(rq);
 
 	if (at_head || blk_rq_is_passthrough(rq)) {
 		if (at_head)
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 06/11] block: remove extra param for trace_block_getrq()
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
                   ` (4 preceding siblings ...)
  2020-06-29 23:43 ` [PATCH 05/11] block: get rid of the trace rq insert wrapper Chaitanya Kulkarni
@ 2020-06-29 23:43 ` Chaitanya Kulkarni
  2020-06-29 23:43 ` [PATCH 07/11] block: get rid of blk_trace_request_get_cgid() Chaitanya Kulkarni
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

Remove the extra parameter for trace_block_getrq() since we can derive
I/O direction from bio->bi_opf.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 block/blk-mq.c               |  2 +-
 include/trace/events/block.h | 14 ++++++--------
 kernel/trace/blktrace.c      | 13 ++++++-------
 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index dbb98b2bc868..d66bb299d4ae 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2111,7 +2111,7 @@ blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
 		goto queue_exit;
 	}
 
-	trace_block_getrq(bio, bio->bi_opf);
+	trace_block_getrq(bio);
 
 	rq_qos_track(q, rq, bio);
 
diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index d7289576f1fd..3d8923062fc4 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -347,9 +347,9 @@ DEFINE_EVENT(block_bio, block_bio_queue,
 
 DECLARE_EVENT_CLASS(block_get_rq,
 
-	TP_PROTO(struct bio *bio, int rw),
+	TP_PROTO(struct bio *bio),
 
-	TP_ARGS(bio, rw),
+	TP_ARGS(bio),
 
 	TP_STRUCT__entry(
 		__field( dev_t,		dev			)
@@ -377,22 +377,20 @@ DECLARE_EVENT_CLASS(block_get_rq,
 /**
  * block_getrq - get a free request entry in queue for block IO operations
  * @bio: pending block IO operation (can be %NULL)
- * @rw: low bit indicates a read (%0) or a write (%1)
  *
  * A request struct for queue has been allocated to handle the
  * block IO operation @bio.
  */
 DEFINE_EVENT(block_get_rq, block_getrq,
 
-	TP_PROTO(struct bio *bio, int rw),
+	TP_PROTO(struct bio *bio),
 
-	TP_ARGS(bio, rw)
+	TP_ARGS(bio)
 );
 
 /**
  * block_sleeprq - waiting to get a free request entry in queue for block IO operation
  * @bio: pending block IO operation (can be %NULL)
- * @rw: low bit indicates a read (%0) or a write (%1)
  *
  * In the case where a request struct cannot be provided for queue
  * the process needs to wait for an request struct to become
@@ -401,9 +399,9 @@ DEFINE_EVENT(block_get_rq, block_getrq,
  */
 DEFINE_EVENT(block_get_rq, block_sleeprq,
 
-	TP_PROTO(struct bio *bio, int rw),
+	TP_PROTO(struct bio *bio),
 
-	TP_ARGS(bio, rw)
+	TP_ARGS(bio)
 );
 
 /**
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 7b72781a591d..1d36e6153ab8 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -949,8 +949,7 @@ static void blk_add_trace_bio_queue(void *ignore, struct bio *bio)
 	blk_add_trace_bio(bio, BLK_TA_QUEUE, 0);
 }
 
-static void blk_add_trace_getrq(void *ignore,
-				struct bio *bio, int rw)
+static void blk_add_trace_getrq(void *ignore, struct bio *bio)
 {
 	if (bio)
 		blk_add_trace_bio(bio, BLK_TA_GETRQ, 0);
@@ -960,14 +959,14 @@ static void blk_add_trace_getrq(void *ignore,
 		rcu_read_lock();
 		bt = rcu_dereference(bio_q(bio)->blk_trace);
 		if (bt)
-			__blk_add_trace(bt, 0, 0, rw, 0, BLK_TA_GETRQ, 0, 0,
-					NULL, 0);
+			__blk_add_trace(bt, 0, 0, bio->bi_opf, 0,
+					BLK_TA_GETRQ, 0, 0, NULL, 0);
 		rcu_read_unlock();
 	}
 }
 
 
-static void blk_add_trace_sleeprq(void *ignore, struct bio *bio, int rw)
+static void blk_add_trace_sleeprq(void *ignore, struct bio *bio)
 {
 	if (bio)
 		blk_add_trace_bio(bio, BLK_TA_SLEEPRQ, 0);
@@ -977,8 +976,8 @@ static void blk_add_trace_sleeprq(void *ignore, struct bio *bio, int rw)
 		rcu_read_lock();
 		bt = rcu_dereference(bio_q(bio)->blk_trace);
 		if (bt)
-			__blk_add_trace(bt, 0, 0, rw, 0, BLK_TA_SLEEPRQ,
-					0, 0, NULL, 0);
+			__blk_add_trace(bt, 0, 0, bio->bi_opf, 0,
+					BLK_TA_SLEEPRQ, 0, 0, NULL, 0);
 		rcu_read_unlock();
 	}
 }
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 07/11] block: get rid of blk_trace_request_get_cgid()
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
                   ` (5 preceding siblings ...)
  2020-06-29 23:43 ` [PATCH 06/11] block: remove extra param for trace_block_getrq() Chaitanya Kulkarni
@ 2020-06-29 23:43 ` Chaitanya Kulkarni
  2020-06-30  5:12   ` Christoph Hellwig
  2020-06-29 23:43 ` [PATCH 08/11] block: fix the comments in the trace event block.h Chaitanya Kulkarni
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

Now that we have done cleanup we can safely get rid of the
blk_trace_request_get_cgid() and replace it with
blk_trace_bio_get_cgid().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 kernel/trace/blktrace.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 1d36e6153ab8..bb864a50307f 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -804,15 +804,6 @@ u64 blk_trace_bio_get_cgid(struct request_queue *q, struct bio *bio)
 }
 #endif
 
-static u64
-blk_trace_request_get_cgid(struct request_queue *q, struct request *rq)
-{
-	if (!rq->bio)
-		return 0;
-	/* Use the first bio */
-	return blk_trace_bio_get_cgid(q, rq->bio);
-}
-
 /*
  * blktrace probes
  */
@@ -854,32 +845,32 @@ static void blk_add_trace_rq(struct request *rq, int error,
 static void blk_add_trace_rq_insert(void *ignore, struct request *rq)
 {
 	blk_add_trace_rq(rq, 0, blk_rq_bytes(rq), BLK_TA_INSERT,
-			 blk_trace_request_get_cgid(rq->q, rq));
+			 rq->bio ? blk_trace_bio_get_cgid(rq->q, rq->bio) : 0);
 }
 
 static void blk_add_trace_rq_issue(void *ignore, struct request *rq)
 {
 	blk_add_trace_rq(rq, 0, blk_rq_bytes(rq), BLK_TA_ISSUE,
-			 blk_trace_request_get_cgid(rq->q, rq));
+			 rq->bio ? blk_trace_bio_get_cgid(rq->q, rq->bio) : 0);
 }
 
 static void blk_add_trace_rq_merge(void *ignore, struct request *rq)
 {
 	blk_add_trace_rq(rq, 0, blk_rq_bytes(rq), BLK_TA_BACKMERGE,
-			 blk_trace_request_get_cgid(rq->q, rq));
+			 rq->bio ? blk_trace_bio_get_cgid(rq->q, rq->bio) : 0);
 }
 
 static void blk_add_trace_rq_requeue(void *ignore, struct request *rq)
 {
 	blk_add_trace_rq(rq, 0, blk_rq_bytes(rq), BLK_TA_REQUEUE,
-			 blk_trace_request_get_cgid(rq->q, rq));
+			 rq->bio ? blk_trace_bio_get_cgid(rq->q, rq->bio) : 0);
 }
 
 static void blk_add_trace_rq_complete(void *ignore, struct request *rq,
 			int error, unsigned int nr_bytes)
 {
 	blk_add_trace_rq(rq, error, nr_bytes, BLK_TA_COMPLETE,
-			 blk_trace_request_get_cgid(rq->q, rq));
+			 rq->bio ? blk_trace_bio_get_cgid(rq->q, rq->bio) : 0);
 }
 
 /**
@@ -1105,7 +1096,8 @@ static void blk_add_trace_rq_remap(void *ignore,
 
 	__blk_add_trace(bt, blk_rq_pos(rq), blk_rq_bytes(rq),
 			rq_data_dir(rq), 0, BLK_TA_REMAP, 0,
-			sizeof(r), &r, blk_trace_request_get_cgid(q, rq));
+			sizeof(r), &r,
+			rq->bio ? blk_trace_bio_get_cgid(q, rq->bio) : 0);
 	rcu_read_unlock();
 }
 
@@ -1134,8 +1126,8 @@ void blk_add_driver_data(struct request_queue *q,
 	}
 
 	__blk_add_trace(bt, blk_rq_trace_sector(rq), blk_rq_bytes(rq), 0, 0,
-				BLK_TA_DRV_DATA, 0, len, data,
-				blk_trace_request_get_cgid(q, rq));
+			BLK_TA_DRV_DATA, 0, len, data,
+			rq->bio ? blk_trace_bio_get_cgid(q, rq->bio) : 0);
 	rcu_read_unlock();
 }
 EXPORT_SYMBOL_GPL(blk_add_driver_data);
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 08/11] block: fix the comments in the trace event block.h
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
                   ` (6 preceding siblings ...)
  2020-06-29 23:43 ` [PATCH 07/11] block: get rid of blk_trace_request_get_cgid() Chaitanya Kulkarni
@ 2020-06-29 23:43 ` Chaitanya Kulkarni
  2020-06-30  5:12   ` Christoph Hellwig
  2020-06-29 23:43 ` [PATCH 09/11] block: remove unsed param in blk_fill_rwbs() Chaitanya Kulkarni
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

This is purely cleanup patch which fixes the comment in trace event
header for block_rq_issue() and block_rq_merge() events.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 include/trace/events/block.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 3d8923062fc4..6a74fb9f4dba 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -195,7 +195,7 @@ DEFINE_EVENT(block_rq, block_rq_insert,
 
 /**
  * block_rq_issue - issue pending block IO request operation to device driver
- * @rq: block IO operation operation request
+ * @rq: block IO operation request
  *
  * Called when block operation request @rq from queue @q is sent to a
  * device driver for processing.
@@ -209,7 +209,7 @@ DEFINE_EVENT(block_rq, block_rq_issue,
 
 /**
  * block_rq_merge - merge request with another one in the elevator
- * @rq: block IO operation operation request
+ * @rq: block IO operation request
  *
  * Called when block operation request @rq from queuei is merged to another
  * request queued in the elevator.
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 09/11] block: remove unsed param in blk_fill_rwbs()
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
                   ` (7 preceding siblings ...)
  2020-06-29 23:43 ` [PATCH 08/11] block: fix the comments in the trace event block.h Chaitanya Kulkarni
@ 2020-06-29 23:43 ` Chaitanya Kulkarni
  2020-06-30  5:12   ` Christoph Hellwig
  2020-06-29 23:43 ` [PATCH 10/11] block: use block_bio class for getrq and sleeprq Chaitanya Kulkarni
                   ` (2 subsequent siblings)
  11 siblings, 1 reply; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

The last parameter for the function blk_fill_rwbs() was added in
5782138e47 ("tracing/events: convert block trace points to
TRACE_EVENT()") in order to signal read request and use of that parameter
was replaced with using switch case REQ_OP_READ with
1b9a9ab78b0 ("blktrace: use op accessors"), but the parameter was never
removed.

This patch removes unused parameter which also allows us to merge existing
trace points in the following patch.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 include/linux/blktrace_api.h  |  2 +-
 include/trace/events/bcache.h | 10 +++++-----
 include/trace/events/block.h  | 19 +++++++++----------
 kernel/trace/blktrace.c       |  2 +-
 4 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index 3b6ff5902edc..80123eebf005 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -120,7 +120,7 @@ struct compat_blk_user_trace_setup {
 
 #endif
 
-extern void blk_fill_rwbs(char *rwbs, unsigned int op, int bytes);
+extern void blk_fill_rwbs(char *rwbs, unsigned int op);
 
 static inline sector_t blk_rq_trace_sector(struct request *rq)
 {
diff --git a/include/trace/events/bcache.h b/include/trace/events/bcache.h
index 0bddea663b3b..41415637e92c 100644
--- a/include/trace/events/bcache.h
+++ b/include/trace/events/bcache.h
@@ -28,7 +28,7 @@ DECLARE_EVENT_CLASS(bcache_request,
 		__entry->sector		= bio->bi_iter.bi_sector;
 		__entry->orig_sector	= bio->bi_iter.bi_sector - 16;
 		__entry->nr_sector	= bio->bi_iter.bi_size >> 9;
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf, bio->bi_iter.bi_size);
+		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
 	),
 
 	TP_printk("%d,%d %s %llu + %u (from %d,%d @ %llu)",
@@ -102,7 +102,7 @@ DECLARE_EVENT_CLASS(bcache_bio,
 		__entry->dev		= bio_dev(bio);
 		__entry->sector		= bio->bi_iter.bi_sector;
 		__entry->nr_sector	= bio->bi_iter.bi_size >> 9;
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf, bio->bi_iter.bi_size);
+		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
 	),
 
 	TP_printk("%d,%d  %s %llu + %u",
@@ -137,7 +137,7 @@ TRACE_EVENT(bcache_read,
 		__entry->dev		= bio_dev(bio);
 		__entry->sector		= bio->bi_iter.bi_sector;
 		__entry->nr_sector	= bio->bi_iter.bi_size >> 9;
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf, bio->bi_iter.bi_size);
+		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
 		__entry->cache_hit = hit;
 		__entry->bypass = bypass;
 	),
@@ -168,7 +168,7 @@ TRACE_EVENT(bcache_write,
 		__entry->inode		= inode;
 		__entry->sector		= bio->bi_iter.bi_sector;
 		__entry->nr_sector	= bio->bi_iter.bi_size >> 9;
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf, bio->bi_iter.bi_size);
+		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
 		__entry->writeback = writeback;
 		__entry->bypass = bypass;
 	),
@@ -238,7 +238,7 @@ TRACE_EVENT(bcache_journal_write,
 		__entry->sector		= bio->bi_iter.bi_sector;
 		__entry->nr_sector	= bio->bi_iter.bi_size >> 9;
 		__entry->nr_keys	= keys;
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf, bio->bi_iter.bi_size);
+		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
 	),
 
 	TP_printk("%d,%d  %s %llu + %u keys %u",
diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 6a74fb9f4dba..d191d2cd1070 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -89,7 +89,7 @@ TRACE_EVENT(block_rq_requeue,
 		__entry->sector    = blk_rq_trace_sector(rq);
 		__entry->nr_sector = blk_rq_trace_nr_sectors(rq);
 
-		blk_fill_rwbs(__entry->rwbs, rq->cmd_flags, blk_rq_bytes(rq));
+		blk_fill_rwbs(__entry->rwbs, rq->cmd_flags);
 		__get_str(cmd)[0] = '\0';
 	),
 
@@ -132,7 +132,7 @@ TRACE_EVENT(block_rq_complete,
 		__entry->nr_sector = nr_bytes >> 9;
 		__entry->error     = error;
 
-		blk_fill_rwbs(__entry->rwbs, rq->cmd_flags, nr_bytes);
+		blk_fill_rwbs(__entry->rwbs, rq->cmd_flags);
 		__get_str(cmd)[0] = '\0';
 	),
 
@@ -165,7 +165,7 @@ DECLARE_EVENT_CLASS(block_rq,
 		__entry->nr_sector = blk_rq_trace_nr_sectors(rq);
 		__entry->bytes     = blk_rq_bytes(rq);
 
-		blk_fill_rwbs(__entry->rwbs, rq->cmd_flags, blk_rq_bytes(rq));
+		blk_fill_rwbs(__entry->rwbs, rq->cmd_flags);
 		__get_str(cmd)[0] = '\0';
 		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
 	),
@@ -248,7 +248,7 @@ TRACE_EVENT(block_bio_complete,
 		__entry->sector		= bio->bi_iter.bi_sector;
 		__entry->nr_sector	= bio_sectors(bio);
 		__entry->error		= blk_status_to_errno(bio->bi_status);
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf, bio->bi_iter.bi_size);
+		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
 	),
 
 	TP_printk("%d,%d %s %llu + %u [%d]",
@@ -275,7 +275,7 @@ DECLARE_EVENT_CLASS(block_bio,
 		__entry->dev		= bio_dev(bio);
 		__entry->sector		= bio->bi_iter.bi_sector;
 		__entry->nr_sector	= bio_sectors(bio);
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf, bio->bi_iter.bi_size);
+		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
 		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
 	),
 
@@ -363,8 +363,7 @@ DECLARE_EVENT_CLASS(block_get_rq,
 		__entry->dev		= bio ? bio_dev(bio) : 0;
 		__entry->sector		= bio ? bio->bi_iter.bi_sector : 0;
 		__entry->nr_sector	= bio ? bio_sectors(bio) : 0;
-		blk_fill_rwbs(__entry->rwbs,
-			      bio ? bio->bi_opf : 0, __entry->nr_sector);
+		blk_fill_rwbs(__entry->rwbs, bio ? bio->bi_opf : 0);
 		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
         ),
 
@@ -492,7 +491,7 @@ TRACE_EVENT(block_split,
 		__entry->dev		= bio_dev(bio);
 		__entry->sector		= bio->bi_iter.bi_sector;
 		__entry->new_sector	= new_sector;
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf, bio->bi_iter.bi_size);
+		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
 		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
 	),
 
@@ -535,7 +534,7 @@ TRACE_EVENT(block_bio_remap,
 		__entry->nr_sector	= bio_sectors(bio);
 		__entry->old_dev	= dev;
 		__entry->old_sector	= from;
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf, bio->bi_iter.bi_size);
+		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
 	),
 
 	TP_printk("%d,%d %s %llu + %u <- (%d,%d) %llu",
@@ -581,7 +580,7 @@ TRACE_EVENT(block_rq_remap,
 		__entry->old_dev	= dev;
 		__entry->old_sector	= from;
 		__entry->nr_bios	= blk_rq_count_bios(rq);
-		blk_fill_rwbs(__entry->rwbs, rq->cmd_flags, blk_rq_bytes(rq));
+		blk_fill_rwbs(__entry->rwbs, rq->cmd_flags);
 	),
 
 	TP_printk("%d,%d %s %llu + %u <- (%d,%d) %llu %u",
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index bb864a50307f..62cb6197ce1f 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -1950,7 +1950,7 @@ void blk_trace_remove_sysfs(struct device *dev)
 
 #ifdef CONFIG_EVENT_TRACING
 
-void blk_fill_rwbs(char *rwbs, unsigned int op, int bytes)
+void blk_fill_rwbs(char *rwbs, unsigned int op)
 {
 	int i = 0;
 
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 10/11] block: use block_bio class for getrq and sleeprq
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
                   ` (8 preceding siblings ...)
  2020-06-29 23:43 ` [PATCH 09/11] block: remove unsed param in blk_fill_rwbs() Chaitanya Kulkarni
@ 2020-06-29 23:43 ` Chaitanya Kulkarni
  2020-06-30  5:13   ` Christoph Hellwig
  2020-06-29 23:43 ` [PATCH 11/11] block: remove block_get_rq event class Chaitanya Kulkarni
  2020-06-30  0:58 ` [PATCH 00/10] block: blktrace framework cleanup Steven Rostedt
  11 siblings, 1 reply; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

The only difference in block_get_rq and block_bio was the last param
passed  __entry->nr_sector & bio->bi_iter.bi_size respectively. Since
that is not the case anymore replace block_get_rq class with block_bio
for block_getrq and block_sleeprq events, also adjust the code to handle
null bio case in block_bio.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 include/trace/events/block.h | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index d191d2cd1070..21f1daaf012b 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -272,11 +272,19 @@ DECLARE_EVENT_CLASS(block_bio,
 	),
 
 	TP_fast_assign(
-		__entry->dev		= bio_dev(bio);
-		__entry->sector		= bio->bi_iter.bi_sector;
-		__entry->nr_sector	= bio_sectors(bio);
-		blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
-		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+		if (bio) {
+			__entry->dev		= bio_dev(bio);
+			__entry->sector		= bio->bi_iter.bi_sector;
+			__entry->nr_sector	= bio_sectors(bio);
+			blk_fill_rwbs(__entry->rwbs, bio->bi_opf);
+			memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+		} else {
+			__entry->dev		= 0;
+			__entry->sector		= 0;
+			__entry->nr_sector	= 0;
+			blk_fill_rwbs(__entry->rwbs, 0);
+			memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
+		}
 	),
 
 	TP_printk("%d,%d %s %llu + %u [%s]",
@@ -380,7 +388,7 @@ DECLARE_EVENT_CLASS(block_get_rq,
  * A request struct for queue has been allocated to handle the
  * block IO operation @bio.
  */
-DEFINE_EVENT(block_get_rq, block_getrq,
+DEFINE_EVENT(block_bio, block_getrq,
 
 	TP_PROTO(struct bio *bio),
 
@@ -396,7 +404,7 @@ DEFINE_EVENT(block_get_rq, block_getrq,
  * available.  This tracepoint event is generated each time the
  * process goes to sleep waiting for request struct become available.
  */
-DEFINE_EVENT(block_get_rq, block_sleeprq,
+DEFINE_EVENT(block_bio, block_sleeprq,
 
 	TP_PROTO(struct bio *bio),
 
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 11/11] block: remove block_get_rq event class
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
                   ` (9 preceding siblings ...)
  2020-06-29 23:43 ` [PATCH 10/11] block: use block_bio class for getrq and sleeprq Chaitanya Kulkarni
@ 2020-06-29 23:43 ` Chaitanya Kulkarni
  2020-06-30  0:58 ` [PATCH 00/10] block: blktrace framework cleanup Steven Rostedt
  11 siblings, 0 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-06-29 23:43 UTC (permalink / raw)
  To: linux-block, dm-devel
  Cc: jack, rdunlap, sagi, mingo, rostedt, snitzer, agk, axboe,
	paolo.valente, ming.lei, bvanassche, fangguoju, colyli, hch,
	Chaitanya Kulkarni

Now that there are no users for the block_get_rq event class remove the
event class.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 include/trace/events/block.h | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 21f1daaf012b..dc1834250baa 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -353,34 +353,6 @@ DEFINE_EVENT(block_bio, block_bio_queue,
 	TP_ARGS(bio)
 );
 
-DECLARE_EVENT_CLASS(block_get_rq,
-
-	TP_PROTO(struct bio *bio),
-
-	TP_ARGS(bio),
-
-	TP_STRUCT__entry(
-		__field( dev_t,		dev			)
-		__field( sector_t,	sector			)
-		__field( unsigned int,	nr_sector		)
-		__array( char,		rwbs,	RWBS_LEN	)
-		__array( char,		comm,	TASK_COMM_LEN	)
-        ),
-
-	TP_fast_assign(
-		__entry->dev		= bio ? bio_dev(bio) : 0;
-		__entry->sector		= bio ? bio->bi_iter.bi_sector : 0;
-		__entry->nr_sector	= bio ? bio_sectors(bio) : 0;
-		blk_fill_rwbs(__entry->rwbs, bio ? bio->bi_opf : 0);
-		memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
-        ),
-
-	TP_printk("%d,%d %s %llu + %u [%s]",
-		  MAJOR(__entry->dev), MINOR(__entry->dev), __entry->rwbs,
-		  (unsigned long long)__entry->sector,
-		  __entry->nr_sector, __entry->comm)
-);
-
 /**
  * block_getrq - get a free request entry in queue for block IO operations
  * @bio: pending block IO operation (can be %NULL)
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 00/10] block: blktrace framework cleanup
  2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
                   ` (10 preceding siblings ...)
  2020-06-29 23:43 ` [PATCH 11/11] block: remove block_get_rq event class Chaitanya Kulkarni
@ 2020-06-30  0:58 ` Steven Rostedt
  11 siblings, 0 replies; 32+ messages in thread
From: Steven Rostedt @ 2020-06-30  0:58 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, snitzer, agk,
	axboe, paolo.valente, ming.lei, bvanassche, fangguoju, colyli,
	hch

On Mon, 29 Jun 2020 16:43:03 -0700
Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> wrote:

> Hi,
> 
> There are many places where trace API accepts the struct request_queue*
> parameter which can be derived from other function parameters.
> 
> This patch-series removes the struct request queue parameter from the
> blktrace framework and adjusts the tracepoints definition and usage
> along with the tracing API itself.
> 
> Also with the queue parameter removed now we can merge some of the trace
> events with creating generic event class for bio based tarce events.
> 
> Finally few of more cleanups to remove the trace_block_rq_insert()
> wrapper blk_mq_sched_request_inserted(), get rid of the extra parameter
> for trace_block_getrq, remove the blk_trace_request_get_cgid(),
> fix tracepoint comment header,  and blk_fill_rwbs() related triggred
> cleanup.
> 
> I've added patches after first cleanup as I scan the code for more
> cleanup. I think patch distribution can be better but it will be great
> to have some feedback as the amount of clenaup has grown bigger.
> 
> Any comments are welcome.
> 
> Regards,
> Chaitanya
> 
> Chaitanya Kulkarni (11):
>   block: blktrace framework cleanup
>   block: rename block_bio_merge class to block_bio
>   block: use block_bio event class for bio_queue
>   block: use block_bio event class for bio_bounce
>   block: get rid of the trace rq insert wrapper
>   block: remove extra param for trace_block_getrq()
>   block: get rid of blk_trace_request_get_cgid()
>   block: fix the comments in the trace event block.h
>   block: remove unsed param in blk_fill_rwbs()
>   block: use block_bio class for getrq and sleeprq
>   block: remove block_get_rq event class

From a tracing perspective:

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve


> 
>  block/bfq-iosched.c           |   4 +-
>  block/blk-core.c              |   6 +-
>  block/blk-merge.c             |   4 +-
>  block/blk-mq-sched.c          |   6 -
>  block/blk-mq-sched.h          |   1 -
>  block/blk-mq.c                |  10 +-
>  block/bounce.c                |   2 +-
>  block/kyber-iosched.c         |   4 +-
>  block/mq-deadline.c           |   4 +-
>  drivers/md/dm.c               |   4 +-
>  include/linux/blktrace_api.h  |   2 +-
>  include/trace/events/bcache.h |  10 +-
>  include/trace/events/block.h  | 226 +++++++++++-----------------------
>  kernel/trace/blktrace.c       | 129 +++++++++----------
>  14 files changed, 161 insertions(+), 251 deletions(-)
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 01/11] block: blktrace framework cleanup
  2020-06-29 23:43 ` [PATCH 01/11] " Chaitanya Kulkarni
@ 2020-06-30  5:10   ` Christoph Hellwig
  2020-07-01  4:32     ` Chaitanya Kulkarni
  0 siblings, 1 reply; 32+ messages in thread
From: Christoph Hellwig @ 2020-06-30  5:10 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli, hch

On Mon, Jun 29, 2020 at 04:43:04PM -0700, Chaitanya Kulkarni wrote:
> This patch removes the extra variables from the trace events and
> overall kernel blktrace framework. The removed members can easily be
> extracted from the remaining argument which reduces the code
> significantly and allows us to optimize the several tracepoints like
> the next patch in the series.      

The subject sounds a litle strange, I'd rather say:

"block: remove superflous arguments from tracepoints"

The actual changes look good to me.

> +		trace_block_rq_insert(rq);
>  	}
>  
>  	spin_lock(&ctx->lock);
> @@ -2111,7 +2111,7 @@ blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>  		goto queue_exit;
>  	}
>  
> -	trace_block_getrq(q, bio, bio->bi_opf);
> +	trace_block_getrq(bio, bio->bi_opf);

But now that you remove more than the q argument in some spots you
might remove the op one here as well.  Or limit the first patch to
just the queue argument..


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 02/11] block: rename block_bio_merge class to block_bio
  2020-06-29 23:43 ` [PATCH 02/11] block: rename block_bio_merge class to block_bio Chaitanya Kulkarni
@ 2020-06-30  5:10   ` Christoph Hellwig
  2020-07-01  4:33     ` Chaitanya Kulkarni
  0 siblings, 1 reply; 32+ messages in thread
From: Christoph Hellwig @ 2020-06-30  5:10 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli, hch

On Mon, Jun 29, 2020 at 04:43:05PM -0700, Chaitanya Kulkarni wrote:
> There are identical TRACE_EVENTS presents which can now take an
> advantage of the block_bio_merge trace event class.
> 
> This is a prep patch which renames block_bio_merge to block_bio so
> that the next patches in this series will be able to resue it.

The changes look good, but I'd merged it with the patches adding
actual new users (which also look good to me).

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 05/11] block: get rid of the trace rq insert wrapper
  2020-06-29 23:43 ` [PATCH 05/11] block: get rid of the trace rq insert wrapper Chaitanya Kulkarni
@ 2020-06-30  5:11   ` Christoph Hellwig
  2020-07-01  4:34     ` Chaitanya Kulkarni
  2020-07-03 23:29   ` Chaitanya Kulkarni
  1 sibling, 1 reply; 32+ messages in thread
From: Christoph Hellwig @ 2020-06-30  5:11 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli, hch

On Mon, Jun 29, 2020 at 04:43:08PM -0700, Chaitanya Kulkarni wrote:
> Get rid of the wrapper for trace_block_rq_insert() and call the function
> directly.

I'd mention blk_mq_sched_request_inserted instead of the tracepoint
in the subject and commit message.  Otherwise this looks fine.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 07/11] block: get rid of blk_trace_request_get_cgid()
  2020-06-29 23:43 ` [PATCH 07/11] block: get rid of blk_trace_request_get_cgid() Chaitanya Kulkarni
@ 2020-06-30  5:12   ` Christoph Hellwig
  2020-07-01  4:38     ` Chaitanya Kulkarni
  0 siblings, 1 reply; 32+ messages in thread
From: Christoph Hellwig @ 2020-06-30  5:12 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli, hch

On Mon, Jun 29, 2020 at 04:43:10PM -0700, Chaitanya Kulkarni wrote:
> Now that we have done cleanup we can safely get rid of the
> blk_trace_request_get_cgid() and replace it with
> blk_trace_bio_get_cgid().

To me the helper actually looks useful compared to open coding the
check in a bunch of callers.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 08/11] block: fix the comments in the trace event block.h
  2020-06-29 23:43 ` [PATCH 08/11] block: fix the comments in the trace event block.h Chaitanya Kulkarni
@ 2020-06-30  5:12   ` Christoph Hellwig
  0 siblings, 0 replies; 32+ messages in thread
From: Christoph Hellwig @ 2020-06-30  5:12 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli, hch

On Mon, Jun 29, 2020 at 04:43:11PM -0700, Chaitanya Kulkarni wrote:
> This is purely cleanup patch which fixes the comment in trace event
> header for block_rq_issue() and block_rq_merge() events.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 09/11] block: remove unsed param in blk_fill_rwbs()
  2020-06-29 23:43 ` [PATCH 09/11] block: remove unsed param in blk_fill_rwbs() Chaitanya Kulkarni
@ 2020-06-30  5:12   ` Christoph Hellwig
  2020-07-01  4:38     ` Chaitanya Kulkarni
  0 siblings, 1 reply; 32+ messages in thread
From: Christoph Hellwig @ 2020-06-30  5:12 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli, hch

On Mon, Jun 29, 2020 at 04:43:12PM -0700, Chaitanya Kulkarni wrote:
> The last parameter for the function blk_fill_rwbs() was added in
> 5782138e47 ("tracing/events: convert block trace points to
> TRACE_EVENT()") in order to signal read request and use of that parameter
> was replaced with using switch case REQ_OP_READ with
> 1b9a9ab78b0 ("blktrace: use op accessors"), but the parameter was never
> removed.
> 
> This patch removes unused parameter which also allows us to merge existing
> trace points in the following patch.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  include/linux/blktrace_api.h  |  2 +-
>  include/trace/events/bcache.h | 10 +++++-----
>  include/trace/events/block.h  | 19 +++++++++----------
>  kernel/trace/blktrace.c       |  2 +-
>  4 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
> index 3b6ff5902edc..80123eebf005 100644
> --- a/include/linux/blktrace_api.h
> +++ b/include/linux/blktrace_api.h
> @@ -120,7 +120,7 @@ struct compat_blk_user_trace_setup {
>  
>  #endif
>  
> -extern void blk_fill_rwbs(char *rwbs, unsigned int op, int bytes);
> +extern void blk_fill_rwbs(char *rwbs, unsigned int op);

You might want to drop the extern while you're at it.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 10/11] block: use block_bio class for getrq and sleeprq
  2020-06-29 23:43 ` [PATCH 10/11] block: use block_bio class for getrq and sleeprq Chaitanya Kulkarni
@ 2020-06-30  5:13   ` Christoph Hellwig
  2020-07-01  4:45     ` Chaitanya Kulkarni
  0 siblings, 1 reply; 32+ messages in thread
From: Christoph Hellwig @ 2020-06-30  5:13 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli, hch

On Mon, Jun 29, 2020 at 04:43:13PM -0700, Chaitanya Kulkarni wrote:
> The only difference in block_get_rq and block_bio was the last param
> passed  __entry->nr_sector & bio->bi_iter.bi_size respectively. Since
> that is not the case anymore replace block_get_rq class with block_bio
> for block_getrq and block_sleeprq events, also adjust the code to handle
> null bio case in block_bio.

To me it seems like keeping the NULL bio case separate actually is a
little simpler..


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 01/11] block: blktrace framework cleanup
  2020-06-30  5:10   ` Christoph Hellwig
@ 2020-07-01  4:32     ` Chaitanya Kulkarni
  0 siblings, 0 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-07-01  4:32 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli

On 6/29/20 10:10 PM, Christoph Hellwig wrote:
> On Mon, Jun 29, 2020 at 04:43:04PM -0700, Chaitanya Kulkarni wrote:
>> This patch removes the extra variables from the trace events and
>> overall kernel blktrace framework. The removed members can easily be
>> extracted from the remaining argument which reduces the code
>> significantly and allows us to optimize the several tracepoints like
>> the next patch in the series.
> The subject sounds a litle strange, I'd rather say:
> 
> "block: remove superflous arguments from tracepoints"
> 
> The actual changes look good to me.
Okay, will change that.
> 
>> +		trace_block_rq_insert(rq);
>>   	}
>>   
>>   	spin_lock(&ctx->lock);
>> @@ -2111,7 +2111,7 @@ blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>>   		goto queue_exit;
>>   	}
>>   
>> -	trace_block_getrq(q, bio, bio->bi_opf);
>> +	trace_block_getrq(bio, bio->bi_opf);
> But now that you remove more than the q argument in some spots you
> might remove the op one here as well.  Or limit the first patch to
> just the queue argument..
> 
> 

Yeah thats is where I had difficulty, and when I tried to do it in one
patch it got complicated to review. I'll keep the first patch for the
q only and patch(s) for rest arguments as needed it easy to review that
way.


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 02/11] block: rename block_bio_merge class to block_bio
  2020-06-30  5:10   ` Christoph Hellwig
@ 2020-07-01  4:33     ` Chaitanya Kulkarni
  0 siblings, 0 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-07-01  4:33 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli

On 6/29/20 10:10 PM, Christoph Hellwig wrote:
> On Mon, Jun 29, 2020 at 04:43:05PM -0700, Chaitanya Kulkarni wrote:
>> There are identical TRACE_EVENTS presents which can now take an
>> advantage of the block_bio_merge trace event class.
>>
>> This is a prep patch which renames block_bio_merge to block_bio so
>> that the next patches in this series will be able to resue it.
> The changes look good, but I'd merged it with the patches adding
> actual new users (which also look good to me).
> 
Okay, will merge it.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 05/11] block: get rid of the trace rq insert wrapper
  2020-06-30  5:11   ` Christoph Hellwig
@ 2020-07-01  4:34     ` Chaitanya Kulkarni
  0 siblings, 0 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-07-01  4:34 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli

On 6/29/20 10:11 PM, Christoph Hellwig wrote:
> On Mon, Jun 29, 2020 at 04:43:08PM -0700, Chaitanya Kulkarni wrote:
>> Get rid of the wrapper for trace_block_rq_insert() and call the function
>> directly.
> I'd mention blk_mq_sched_request_inserted instead of the tracepoint
> in the subject and commit message.  Otherwise this looks fine.
> 
Okay, will change the message.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 07/11] block: get rid of blk_trace_request_get_cgid()
  2020-06-30  5:12   ` Christoph Hellwig
@ 2020-07-01  4:38     ` Chaitanya Kulkarni
  2020-07-01  6:16       ` Christoph Hellwig
  0 siblings, 1 reply; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-07-01  4:38 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli

On 6/29/20 10:12 PM, Christoph Hellwig wrote:
> On Mon, Jun 29, 2020 at 04:43:10PM -0700, Chaitanya Kulkarni wrote:
>> Now that we have done cleanup we can safely get rid of the
>> blk_trace_request_get_cgid() and replace it with
>> blk_trace_bio_get_cgid().
> To me the helper actually looks useful compared to open coding the
> check in a bunch of callers.
> 

The helper adds an additional function call which can be avoided easily
since blk_trace_bio_get_cgid() is written nicely, that also maintains
the readability of the code.

Unless the cost of the function call doesn't matter and readability is
not lost can we please not use helper ?

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 09/11] block: remove unsed param in blk_fill_rwbs()
  2020-06-30  5:12   ` Christoph Hellwig
@ 2020-07-01  4:38     ` Chaitanya Kulkarni
  0 siblings, 0 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-07-01  4:38 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli

On 6/29/20 10:12 PM, Christoph Hellwig wrote:
>> +extern void blk_fill_rwbs(char *rwbs, unsigned int op);
> You might want to drop the extern while you're at it.
> 

Good point.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 10/11] block: use block_bio class for getrq and sleeprq
  2020-06-30  5:13   ` Christoph Hellwig
@ 2020-07-01  4:45     ` Chaitanya Kulkarni
  2020-07-01  6:18       ` Christoph Hellwig
  0 siblings, 1 reply; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-07-01  4:45 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli

On 6/29/20 10:13 PM, Christoph Hellwig wrote:
> On Mon, Jun 29, 2020 at 04:43:13PM -0700, Chaitanya Kulkarni wrote:
>> The only difference in block_get_rq and block_bio was the last param
>> passed  __entry->nr_sector & bio->bi_iter.bi_size respectively. Since
>> that is not the case anymore replace block_get_rq class with block_bio
>> for block_getrq and block_sleeprq events, also adjust the code to handle
>> null bio case in block_bio.
> To me it seems like keeping the NULL bio case separate actually is a
> little simpler..
> 
> 

Keeping it separate will have an extra event class and related
event(s) for only handling null bio case.

Also the block_get_rq class uses 4 comparisons with ?:.
This patch reduces it to only one comparison in fast path.

With above explanation does it make sense to get rid of the
blk_get_rq ?




^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 07/11] block: get rid of blk_trace_request_get_cgid()
  2020-07-01  4:38     ` Chaitanya Kulkarni
@ 2020-07-01  6:16       ` Christoph Hellwig
  2020-07-01 21:06         ` Chaitanya Kulkarni
  0 siblings, 1 reply; 32+ messages in thread
From: Christoph Hellwig @ 2020-07-01  6:16 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: Christoph Hellwig, linux-block, dm-devel, jack, rdunlap, sagi,
	mingo, rostedt, snitzer, agk, axboe, paolo.valente, ming.lei,
	bvanassche, fangguoju, colyli

On Wed, Jul 01, 2020 at 04:38:06AM +0000, Chaitanya Kulkarni wrote:
> On 6/29/20 10:12 PM, Christoph Hellwig wrote:
> > On Mon, Jun 29, 2020 at 04:43:10PM -0700, Chaitanya Kulkarni wrote:
> >> Now that we have done cleanup we can safely get rid of the
> >> blk_trace_request_get_cgid() and replace it with
> >> blk_trace_bio_get_cgid().
> > To me the helper actually looks useful compared to open coding the
> > check in a bunch of callers.
> > 
> 
> The helper adds an additional function call which can be avoided easily
> since blk_trace_bio_get_cgid() is written nicely, that also maintains
> the readability of the code.
> 
> Unless the cost of the function call doesn't matter and readability is
> not lost can we please not use helper ?

I think readability is lost.  I'd much rather drop the q argument
from blk_trace_request_get_cgid and keep the helper, as it pretty
clearly documents what is done.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 10/11] block: use block_bio class for getrq and sleeprq
  2020-07-01  4:45     ` Chaitanya Kulkarni
@ 2020-07-01  6:18       ` Christoph Hellwig
  2020-07-01 21:06         ` Chaitanya Kulkarni
  0 siblings, 1 reply; 32+ messages in thread
From: Christoph Hellwig @ 2020-07-01  6:18 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: Christoph Hellwig, linux-block, dm-devel, jack, rdunlap, sagi,
	mingo, rostedt, snitzer, agk, axboe, paolo.valente, ming.lei,
	bvanassche, fangguoju, colyli

On Wed, Jul 01, 2020 at 04:45:03AM +0000, Chaitanya Kulkarni wrote:
> On 6/29/20 10:13 PM, Christoph Hellwig wrote:
> > On Mon, Jun 29, 2020 at 04:43:13PM -0700, Chaitanya Kulkarni wrote:
> >> The only difference in block_get_rq and block_bio was the last param
> >> passed  __entry->nr_sector & bio->bi_iter.bi_size respectively. Since
> >> that is not the case anymore replace block_get_rq class with block_bio
> >> for block_getrq and block_sleeprq events, also adjust the code to handle
> >> null bio case in block_bio.
> > To me it seems like keeping the NULL bio case separate actually is a
> > little simpler..
> > 
> > 
> 
> Keeping it separate will have an extra event class and related
> event(s) for only handling null bio case.
> 
> Also the block_get_rq class uses 4 comparisons with ?:.
> This patch reduces it to only one comparison in fast path.
> 
> With above explanation does it make sense to get rid of the
> blk_get_rq ?

Without this we don't need the request_queue argument to the bio
class, as we can derive it from the bio, and don't have any
conditionals at all.  I'd rather keep the special case with a 
queue and an optional bio separate.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 07/11] block: get rid of blk_trace_request_get_cgid()
  2020-07-01  6:16       ` Christoph Hellwig
@ 2020-07-01 21:06         ` Chaitanya Kulkarni
  0 siblings, 0 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-07-01 21:06 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli

On 6/30/20 11:16 PM, Christoph Hellwig wrote:
> On Wed, Jul 01, 2020 at 04:38:06AM +0000, Chaitanya Kulkarni wrote:
>> On 6/29/20 10:12 PM, Christoph Hellwig wrote:
>>> On Mon, Jun 29, 2020 at 04:43:10PM -0700, Chaitanya Kulkarni wrote:
>>>> Now that we have done cleanup we can safely get rid of the
>>>> blk_trace_request_get_cgid() and replace it with
>>>> blk_trace_bio_get_cgid().
>>> To me the helper actually looks useful compared to open coding the
>>> check in a bunch of callers.
>>>
>> The helper adds an additional function call which can be avoided easily
>> since blk_trace_bio_get_cgid() is written nicely, that also maintains
>> the readability of the code.
>>
>> Unless the cost of the function call doesn't matter and readability is
>> not lost can we please not use helper ?
> I think readability is lost.  I'd much rather drop the q argument
> from blk_trace_request_get_cgid and keep the helper, as it pretty
> clearly documents what is done.
> 
Okay I'll add to V2.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 10/11] block: use block_bio class for getrq and sleeprq
  2020-07-01  6:18       ` Christoph Hellwig
@ 2020-07-01 21:06         ` Chaitanya Kulkarni
  0 siblings, 0 replies; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-07-01 21:06 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli

On 6/30/20 11:19 PM, Christoph Hellwig wrote:
> On Wed, Jul 01, 2020 at 04:45:03AM +0000, Chaitanya Kulkarni wrote:
>> On 6/29/20 10:13 PM, Christoph Hellwig wrote:
>>> On Mon, Jun 29, 2020 at 04:43:13PM -0700, Chaitanya Kulkarni wrote:
>>>> The only difference in block_get_rq and block_bio was the last param
>>>> passed  __entry->nr_sector & bio->bi_iter.bi_size respectively. Since
>>>> that is not the case anymore replace block_get_rq class with block_bio
>>>> for block_getrq and block_sleeprq events, also adjust the code to handle
>>>> null bio case in block_bio.
>>> To me it seems like keeping the NULL bio case separate actually is a
>>> little simpler..
>>>
>>>
>> Keeping it separate will have an extra event class and related
>> event(s) for only handling null bio case.
>>
>> Also the block_get_rq class uses 4 comparisons with ?:.
>> This patch reduces it to only one comparison in fast path.
>>
>> With above explanation does it make sense to get rid of the
>> blk_get_rq ?
> Without this we don't need the request_queue argument to the bio
> class, as we can derive it from the bio, and don't have any
> conditionals at all.  I'd rather keep the special case with a
> queue and an optional bio separate.
> 

Okay.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 05/11] block: get rid of the trace rq insert wrapper
  2020-06-29 23:43 ` [PATCH 05/11] block: get rid of the trace rq insert wrapper Chaitanya Kulkarni
  2020-06-30  5:11   ` Christoph Hellwig
@ 2020-07-03 23:29   ` Chaitanya Kulkarni
  2020-07-07  6:57     ` hch
  1 sibling, 1 reply; 32+ messages in thread
From: Chaitanya Kulkarni @ 2020-07-03 23:29 UTC (permalink / raw)
  To: hch
  Cc: linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli

Christoph,

On 6/29/20 4:44 PM, Chaitanya Kulkarni wrote:
> Get rid of the wrapper for trace_block_rq_insert() and call the function
> directly.
> 
> Signed-off-by: Chaitanya Kulkarni<chaitanya.kulkarni@wdc.com>
> ---

Can we re-consider adding this patch ? here are couple of reasons:-

1. Increase in the size of the text region of the object file:-

    By adding the trace header #include <trace/events/block.h>
    in io-scheduler where it is calling trace_block_rq_insert()
    increases the size of the text region of the object file
    kyber(+215) & bfq (+317) [1].

2. Mandatory io-sched built-in kernel compilation:-

    When testing with a different io-sched KConfig options ("*" vs "M"),
    when kyber and bfq compilation option set to "M" having this patch
    reports error[2].

If I've not missed something here then can we drop this patch ?

In case we really want to do this change it will need to have KConfig
separate patch such that if tracing is selected it will force * 
selection (built-in KConfig) for schedulers in question and etc.

Do we want to do this ?

Regards,
Chaitanya


[1] Scheduler IO object size comparison :-

    Without this patch :-
    ---------------------
    # size block/bfq-iosched.o
     text	   data	    bss	    dec	    hex	filename
    62204	   1011	     32	  63247	   f70f	block/bfq-iosched.o
    # size block/kyber-iosched.o
     text	   data	    bss	    dec	    hex	filename
    14808	   2699	     48	  17555	   4493	block/kyber-iosched.o
    With this patch :-
    ------------------
    # size block/bfq-iosched.o
     text	   data	    bss	    dec	    hex	filename
    62521	   1028	     32	  63581	   f85d	block/bfq-iosched.o
    # size block/kyber-iosched.o
     text	   data	    bss	    dec	    hex	filename
    15023	   2716	     48	  17787	   457b	block/kyber-iosched.o

[2] Error with selecting M for io-sched kyber & bfq :-

ERROR: modpost: "__tracepoint_block_rq_insert" [block/bfq.ko] undefined!
ERROR: modpost: "__tracepoint_block_rq_insert" [block/kyber-iosched.ko] 
undefined!
make[2]: *** [Module.symvers] Error 1
make[2]: *** Deleting file `Module.symvers'
make[1]: *** [modules] Error 2
make[1]: *** Waiting for unfinished jobs....
arch/x86/tools/insn_decoder_test: success: Decoded and checked 4932572 
instructions
   TEST    posttest
   arch/x86/tools/insn_sanity: Success: decoded and checked 1000000 
random instructions with 0 errors (seed:0x4c6e1a40)
	CC      arch/x86/boot/version.o
	VOFFSET arch/x86/boot/compressed/../voffset.h
	OBJCOPY arch/x86/boot/compressed/vmlinux.bin
	RELOCS  arch/x86/boot/compressed/vmlinux.relocs
	CC      arch/x86/boot/compressed/kaslr.o
	CC      arch/x86/boot/compressed/misc.o
	GZIP    arch/x86/boot/compressed/vmlinux.bin.gz
	MKPIGGY arch/x86/boot/compressed/piggy.S
	AS      arch/x86/boot/compressed/piggy.o
	LD      arch/x86/boot/compressed/vmlinux
	ZOFFSET arch/x86/boot/zoffset.h
	OBJCOPY arch/x86/boot/vmlinux.bin
	AS      arch/x86/boot/header.o
	LD      arch/x86/boot/setup.elf
	OBJCOPY arch/x86/boot/setup.bin
	BUILD   arch/x86/boot/bzImage
	Setup is 15132 bytes (padded to 15360 bytes).
	System is 8951 kB
	CRC ff6eac72
Kernel: arch/x86/boot/bzImage is ready  (#59)
	make: *** [__sub-make] Error 2

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 05/11] block: get rid of the trace rq insert wrapper
  2020-07-03 23:29   ` Chaitanya Kulkarni
@ 2020-07-07  6:57     ` hch
  0 siblings, 0 replies; 32+ messages in thread
From: hch @ 2020-07-07  6:57 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: hch, linux-block, dm-devel, jack, rdunlap, sagi, mingo, rostedt,
	snitzer, agk, axboe, paolo.valente, ming.lei, bvanassche,
	fangguoju, colyli

On Fri, Jul 03, 2020 at 11:29:25PM +0000, Chaitanya Kulkarni wrote:
> Christoph,
> 
> On 6/29/20 4:44 PM, Chaitanya Kulkarni wrote:
> > Get rid of the wrapper for trace_block_rq_insert() and call the function
> > directly.
> > 
> > Signed-off-by: Chaitanya Kulkarni<chaitanya.kulkarni@wdc.com>
> > ---
> 
> Can we re-consider adding this patch ? here are couple of reasons:-
> 
> 1. Increase in the size of the text region of the object file:-
> 
>     By adding the trace header #include <trace/events/block.h>
>     in io-scheduler where it is calling trace_block_rq_insert()
>     increases the size of the text region of the object file
>     kyber(+215) & bfq (+317) [1].

You really shouldn't have both loaded, never mind used at the same
time.  It also avoid a function call for the scheduler fast path.

> 2. Mandatory io-sched built-in kernel compilation:-
> 
>     When testing with a different io-sched KConfig options ("*" vs "M"),
>     when kyber and bfq compilation option set to "M" having this patch
>     reports error[2].

See EXPORT_TRACEPOINT_SYMBOL_GPL.

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2020-07-07  6:57 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29 23:43 [PATCH 00/10] block: blktrace framework cleanup Chaitanya Kulkarni
2020-06-29 23:43 ` [PATCH 01/11] " Chaitanya Kulkarni
2020-06-30  5:10   ` Christoph Hellwig
2020-07-01  4:32     ` Chaitanya Kulkarni
2020-06-29 23:43 ` [PATCH 02/11] block: rename block_bio_merge class to block_bio Chaitanya Kulkarni
2020-06-30  5:10   ` Christoph Hellwig
2020-07-01  4:33     ` Chaitanya Kulkarni
2020-06-29 23:43 ` [PATCH 03/11] block: use block_bio event class for bio_queue Chaitanya Kulkarni
2020-06-29 23:43 ` [PATCH 04/11] block: use block_bio event class for bio_bounce Chaitanya Kulkarni
2020-06-29 23:43 ` [PATCH 05/11] block: get rid of the trace rq insert wrapper Chaitanya Kulkarni
2020-06-30  5:11   ` Christoph Hellwig
2020-07-01  4:34     ` Chaitanya Kulkarni
2020-07-03 23:29   ` Chaitanya Kulkarni
2020-07-07  6:57     ` hch
2020-06-29 23:43 ` [PATCH 06/11] block: remove extra param for trace_block_getrq() Chaitanya Kulkarni
2020-06-29 23:43 ` [PATCH 07/11] block: get rid of blk_trace_request_get_cgid() Chaitanya Kulkarni
2020-06-30  5:12   ` Christoph Hellwig
2020-07-01  4:38     ` Chaitanya Kulkarni
2020-07-01  6:16       ` Christoph Hellwig
2020-07-01 21:06         ` Chaitanya Kulkarni
2020-06-29 23:43 ` [PATCH 08/11] block: fix the comments in the trace event block.h Chaitanya Kulkarni
2020-06-30  5:12   ` Christoph Hellwig
2020-06-29 23:43 ` [PATCH 09/11] block: remove unsed param in blk_fill_rwbs() Chaitanya Kulkarni
2020-06-30  5:12   ` Christoph Hellwig
2020-07-01  4:38     ` Chaitanya Kulkarni
2020-06-29 23:43 ` [PATCH 10/11] block: use block_bio class for getrq and sleeprq Chaitanya Kulkarni
2020-06-30  5:13   ` Christoph Hellwig
2020-07-01  4:45     ` Chaitanya Kulkarni
2020-07-01  6:18       ` Christoph Hellwig
2020-07-01 21:06         ` Chaitanya Kulkarni
2020-06-29 23:43 ` [PATCH 11/11] block: remove block_get_rq event class Chaitanya Kulkarni
2020-06-30  0:58 ` [PATCH 00/10] block: blktrace framework cleanup Steven Rostedt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).