Message ID | 20190520141359.30027-1-hch@lst.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | block: improve print_req_error | expand |
On 5/20/19 4:13 PM, Christoph Hellwig wrote: > Print the calling function instead of print_req_error as a prefix, and > print the operation and op_flags separately instrad of the whole field. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/blk-core.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index 419d600e6637..b1f7e244340e 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -167,18 +167,20 @@ int blk_status_to_errno(blk_status_t status) > } > EXPORT_SYMBOL_GPL(blk_status_to_errno); > > -static void print_req_error(struct request *req, blk_status_t status) > +static void print_req_error(struct request *req, blk_status_t status, > + const char *caller) > { > int idx = (__force int)status; > > if (WARN_ON_ONCE(idx >= ARRAY_SIZE(blk_errors))) > return; > > - printk_ratelimited(KERN_ERR "%s: %s error, dev %s, sector %llu flags %x\n", > - __func__, blk_errors[idx].name, > - req->rq_disk ? req->rq_disk->disk_name : "?", > - (unsigned long long)blk_rq_pos(req), > - req->cmd_flags); > + printk_ratelimited(KERN_ERR > + "%s: %s error, dev %s, sector %llu op 0x%x flags 0x%x\n", > + caller, blk_errors[idx].name, > + req->rq_disk ? req->rq_disk->disk_name : "?", > + blk_rq_pos(req), req_op(req), > + req->cmd_flags & ~REQ_OP_MASK); > } > > static void req_bio_endio(struct request *rq, struct bio *bio, > @@ -1418,7 +1420,7 @@ bool blk_update_request(struct request *req, blk_status_t error, > > if (unlikely(error && !blk_rq_is_passthrough(req) && > !(req->rq_flags & RQF_QUIET))) > - print_req_error(req, error); > + print_req_error(req, error, __func__); > > blk_account_io_completion(req, nr_bytes); > > Weell ... where's the point of the __func__ argument if there is only one place where it's called from? Can't we just drop it completely? Cheers, Hannes
Printing separate operations and flags is useful. How about we also add couple of more fields ? Compile tested patch on the top of this patch :- @@ -176,11 +176,14 @@ static void print_req_error(struct request *req, blk_status_t status, return; printk_ratelimited(KERN_ERR - "%s: %s error, dev %s, sector %llu op 0x%x flags 0x%x\n", + "%s: %s error, dev %s, sector %llu op 0x%x flags 0x%x " + "phys_seg %u prio class %u\n", caller, blk_errors[idx].name, req->rq_disk ? req->rq_disk->disk_name : "?", nit:- extra space after '?', if that is not intentional. blk_rq_pos(req), req_op(req), - req->cmd_flags & ~REQ_OP_MASK); + req->cmd_flags & ~REQ_OP_MASK, + req->nr_phys_segments, + IOPRIO_PRIO_CLASS(req->ioprio)); } On 05/20/2019 07:14 AM, Christoph Hellwig wrote: > Print the calling function instead of print_req_error as a prefix, and > print the operation and op_flags separately instrad of the whole field. Also, s/instrad/instead/. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/blk-core.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index 419d600e6637..b1f7e244340e 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -167,18 +167,20 @@ int blk_status_to_errno(blk_status_t status) > } > EXPORT_SYMBOL_GPL(blk_status_to_errno); > > -static void print_req_error(struct request *req, blk_status_t status) > +static void print_req_error(struct request *req, blk_status_t status, > + const char *caller) > { > int idx = (__force int)status; > > if (WARN_ON_ONCE(idx >= ARRAY_SIZE(blk_errors))) > return; > > - printk_ratelimited(KERN_ERR "%s: %s error, dev %s, sector %llu flags %x\n", > - __func__, blk_errors[idx].name, > - req->rq_disk ? req->rq_disk->disk_name : "?", > - (unsigned long long)blk_rq_pos(req), > - req->cmd_flags); > + printk_ratelimited(KERN_ERR > + "%s: %s error, dev %s, sector %llu op 0x%x flags 0x%x\n", > + caller, blk_errors[idx].name, > + req->rq_disk ? req->rq_disk->disk_name : "?", > + blk_rq_pos(req), req_op(req), > + req->cmd_flags & ~REQ_OP_MASK); > } > > static void req_bio_endio(struct request *rq, struct bio *bio, > @@ -1418,7 +1420,7 @@ bool blk_update_request(struct request *req, blk_status_t error, > > if (unlikely(error && !blk_rq_is_passthrough(req) && > !(req->rq_flags & RQF_QUIET))) > - print_req_error(req, error); > + print_req_error(req, error, __func__); > > blk_account_io_completion(req, nr_bytes); > >
If you are okay, we can print the req_op in string format, compile tested patch on the top of yours below, not that this is needed but it is just a nice way to present debug data:- +static inline const char *req_op_str(struct request *req) +{ + char *ret; + + switch (req_op(req)) { + case REQ_OP_READ: + ret = "read"; + break; + case REQ_OP_WRITE: + ret = "write"; + break; + case REQ_OP_FLUSH: + ret = "flush"; + break; + case REQ_OP_DISCARD: + ret = "discard"; + break; + case REQ_OP_SECURE_ERASE: + ret = "secure_erase"; + break; + case REQ_OP_ZONE_RESET: + ret = "zone_reset"; + break; + case REQ_OP_WRITE_SAME: + ret = "write_same"; + break; + case REQ_OP_WRITE_ZEROES: + ret = "write_zeroes"; + break; + case REQ_OP_SCSI_IN: + ret = "scsi_in"; + break; + case REQ_OP_SCSI_OUT: + ret = "scsi_out"; + break; + case REQ_OP_DRV_IN: + ret = "drv_in"; + break; + case REQ_OP_DRV_OUT: + ret = "drv_out"; + break; + default: + ret = "unknown"; + } + return ret; +} + static void print_req_error(struct request *req, blk_status_t status, const char *caller) { @@ -176,11 +223,14 @@ static void print_req_error(struct request *req, blk_status_t status, return; printk_ratelimited(KERN_ERR - "%s: %s error, dev %s, sector %llu op 0x%x flags 0x%x\n", + "%s: %s error, dev %s, sector %llu op 0x%x:(%s) flags 0x%x " + "phys_seg %u prio class %u\n", caller, blk_errors[idx].name, req->rq_disk ? req->rq_disk->disk_name : "?", - blk_rq_pos(req), req_op(req), - req->cmd_flags & ~REQ_OP_MASK); + blk_rq_pos(req), req_op(req), req_op_str(req), + req->cmd_flags & ~REQ_OP_MASK, + req->nr_phys_segments, + IOPRIO_PRIO_CLASS(req->ioprio)); } static void req_bio_endio(struct request *rq, struct bio *bio, On 05/20/2019 10:16 AM, Chaitanya Kulkarni wrote: > Printing separate operations and flags is useful. > > How about we also add couple of more fields ? > > Compile tested patch on the top of this patch :- > > @@ -176,11 +176,14 @@ static void print_req_error(struct request *req, > blk_status_t status, > return; > > printk_ratelimited(KERN_ERR > - "%s: %s error, dev %s, sector %llu op 0x%x flags 0x%x\n", > + "%s: %s error, dev %s, sector %llu op 0x%x flags 0x%x " > + "phys_seg %u prio class %u\n", > caller, blk_errors[idx].name, > req->rq_disk ? req->rq_disk->disk_name : "?", > nit:- extra space after '?', if that is not intentional. > blk_rq_pos(req), req_op(req), > - req->cmd_flags & ~REQ_OP_MASK); > + req->cmd_flags & ~REQ_OP_MASK, > + req->nr_phys_segments, > + IOPRIO_PRIO_CLASS(req->ioprio)); > } > > On 05/20/2019 07:14 AM, Christoph Hellwig wrote: >> Print the calling function instead of print_req_error as a prefix, and >> print the operation and op_flags separately instrad of the whole field. > Also, s/instrad/instead/. > >> >> Signed-off-by: Christoph Hellwig <hch@lst.de> >> --- >> block/blk-core.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/block/blk-core.c b/block/blk-core.c >> index 419d600e6637..b1f7e244340e 100644 >> --- a/block/blk-core.c >> +++ b/block/blk-core.c >> @@ -167,18 +167,20 @@ int blk_status_to_errno(blk_status_t status) >> } >> EXPORT_SYMBOL_GPL(blk_status_to_errno); >> >> -static void print_req_error(struct request *req, blk_status_t status) >> +static void print_req_error(struct request *req, blk_status_t status, >> + const char *caller) >> { >> int idx = (__force int)status; >> >> if (WARN_ON_ONCE(idx >= ARRAY_SIZE(blk_errors))) >> return; >> >> - printk_ratelimited(KERN_ERR "%s: %s error, dev %s, sector %llu flags %x\n", >> - __func__, blk_errors[idx].name, >> - req->rq_disk ? req->rq_disk->disk_name : "?", >> - (unsigned long long)blk_rq_pos(req), >> - req->cmd_flags); >> + printk_ratelimited(KERN_ERR >> + "%s: %s error, dev %s, sector %llu op 0x%x flags 0x%x\n", >> + caller, blk_errors[idx].name, >> + req->rq_disk ? req->rq_disk->disk_name : "?", >> + blk_rq_pos(req), req_op(req), >> + req->cmd_flags & ~REQ_OP_MASK); >> } >> >> static void req_bio_endio(struct request *rq, struct bio *bio, >> @@ -1418,7 +1420,7 @@ bool blk_update_request(struct request *req, blk_status_t error, >> >> if (unlikely(error && !blk_rq_is_passthrough(req) && >> !(req->rq_flags & RQF_QUIET))) >> - print_req_error(req, error); >> + print_req_error(req, error, __func__); >> >> blk_account_io_completion(req, nr_bytes); >> >> > >
diff --git a/block/blk-core.c b/block/blk-core.c index 419d600e6637..b1f7e244340e 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -167,18 +167,20 @@ int blk_status_to_errno(blk_status_t status) } EXPORT_SYMBOL_GPL(blk_status_to_errno); -static void print_req_error(struct request *req, blk_status_t status) +static void print_req_error(struct request *req, blk_status_t status, + const char *caller) { int idx = (__force int)status; if (WARN_ON_ONCE(idx >= ARRAY_SIZE(blk_errors))) return; - printk_ratelimited(KERN_ERR "%s: %s error, dev %s, sector %llu flags %x\n", - __func__, blk_errors[idx].name, - req->rq_disk ? req->rq_disk->disk_name : "?", - (unsigned long long)blk_rq_pos(req), - req->cmd_flags); + printk_ratelimited(KERN_ERR + "%s: %s error, dev %s, sector %llu op 0x%x flags 0x%x\n", + caller, blk_errors[idx].name, + req->rq_disk ? req->rq_disk->disk_name : "?", + blk_rq_pos(req), req_op(req), + req->cmd_flags & ~REQ_OP_MASK); } static void req_bio_endio(struct request *rq, struct bio *bio, @@ -1418,7 +1420,7 @@ bool blk_update_request(struct request *req, blk_status_t error, if (unlikely(error && !blk_rq_is_passthrough(req) && !(req->rq_flags & RQF_QUIET))) - print_req_error(req, error); + print_req_error(req, error, __func__); blk_account_io_completion(req, nr_bytes);
Print the calling function instead of print_req_error as a prefix, and print the operation and op_flags separately instrad of the whole field. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/blk-core.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)