| Message ID | 20200629234314.10509-8-chaitanya.kulkarni@wdc.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | block: blktrace framework cleanup | expand |
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 ? -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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);
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(-)