| Message ID | 20190618054224.25985-3-chaitanya.kulkarni@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: improve print_req_error | expand |
On 6/17/19 10:42 PM, Chaitanya Kulkarni wrote: > +inline const char *blk_op_str(int op) > +{ > + const char *op_str = "UNKNOWN"; > + > + if (op < ARRAY_SIZE(blk_op_name) && blk_op_name[op]) > + op_str = blk_op_name[op]; > + > + return op_str; > +} This won't work correctly if op < 0. If you have another look at the block layer debugfs code you will see that 'op' is has an unsigned type in that code. Please either change the type of 'op' from 'int' to 'unsigned int' or change 'op < ARRAY_SIZE(blk_op_name)' into '(unsigned)op < ARRAY_SIZE(blk_op_name)'. Thanks, Bart.
On 6/18/19 8:26 AM, Bart Van Assche wrote: > On 6/17/19 10:42 PM, Chaitanya Kulkarni wrote: >> +inline const char *blk_op_str(int op) >> +{ >> + const char *op_str = "UNKNOWN"; >> + >> + if (op < ARRAY_SIZE(blk_op_name) && blk_op_name[op]) >> + op_str = blk_op_name[op]; >> + >> + return op_str; >> +} > > This won't work correctly if op < 0. If you have another look at the > block layer debugfs code you will see that 'op' is has an unsigned type > in that code. Please either change the type of 'op' from 'int' to > 'unsigned int' or change 'op < ARRAY_SIZE(blk_op_name)' into > '(unsigned)op < ARRAY_SIZE(blk_op_name)'. > Will change the op to unsigned in next version. > Thanks, > > Bart. >
On 06/18/2019 08:26 AM, Bart Van Assche wrote: > On 6/17/19 10:42 PM, Chaitanya Kulkarni wrote: >> +inline const char *blk_op_str(int op) >> +{ >> + const char *op_str = "UNKNOWN"; >> + >> + if (op < ARRAY_SIZE(blk_op_name) && blk_op_name[op]) >> + op_str = blk_op_name[op]; >> + >> + return op_str; >> +} > > This won't work correctly if op < 0. If you have another look at the > block layer debugfs code you will see that 'op' is has an unsigned type > in that code. Please either change the type of 'op' from 'int' to > 'unsigned int' or change 'op < ARRAY_SIZE(blk_op_name)' into > '(unsigned)op < ARRAY_SIZE(blk_op_name)'. > Sorry I'm little confused here, even though op is defined as a unsigned int we print it as a "%d". So I think we need to keep that as it is or I'll be happy to send a corrective patch to print it is using %u, your input will be very useful here. Regarding the correctness, I think if one of the operand is unsigned then signed values are converted to unsigned valued implicitly. However the exceptions is if the type of the operand with signed integer type can represent all of the values of the type of the operand with unsigned integer type, then the operand with unsigned integer type is converted to the type of the operand with signed integer type. I'm wondering how would above scenario occur when comparing int and size_t. (unless on a platform int can fit all the values into size_t). Since above comparison of the ARRAY_SIZE involves sizeof (size_t) type is a base unsigned integer value, even if op < 0 it will get converted into the unsigned and it will still work. Please correct me if I'm wrong. In order to validate my claim I ran a test with simple test module here are the result:- void test(void) { pr_info("%s\n", blk_op_str(REQ_OP_READ)); pr_info("%s\n", blk_op_str(REQ_OP_WRITE)); pr_info("%s\n", blk_op_str(REQ_OP_FLUSH)); pr_info("%s\n", blk_op_str(REQ_OP_DISCARD)); pr_info("%s\n", blk_op_str(REQ_OP_SECURE_ERASE)); pr_info("%s\n", blk_op_str(REQ_OP_ZONE_RESET)); pr_info("%s\n", blk_op_str(REQ_OP_WRITE_SAME)); pr_info("%s\n", blk_op_str(REQ_OP_WRITE_ZEROES)); pr_info("%s\n", blk_op_str(REQ_OP_SCSI_IN)); pr_info("%s\n", blk_op_str(REQ_OP_SCSI_OUT)); pr_info("%s\n", blk_op_str(REQ_OP_DRV_IN)); pr_info("%s\n", blk_op_str(REQ_OP_DRV_OUT)); pr_info("LAST %s\n", blk_op_str(REQ_OP_LAST)); pr_info("LAST + 1 %s\n", blk_op_str(REQ_OP_LAST + 1)); pr_info("-1 %s\n", blk_op_str(-1)); pr_info("-2 %s\n", blk_op_str(-2)); pr_info("-3 %s\n", blk_op_str(-3)); pr_info("-4 %s\n", blk_op_str(-4)); } [ 819.336023] READ [ 819.336030] WRITE [ 819.336034] FLUSH [ 819.336037] DISCARD [ 819.336041] SECURE_ERASE [ 819.336044] ZONE_RESET [ 819.336048] WRITE_SAME [ 819.336051] WRITE_ZEROES [ 819.336054] SCSI_IN [ 819.336058] SCSI_OUT [ 819.336061] DRV_IN [ 819.336064] DRV_OUT [ 819.336068] LAST UNKNOWN [ 819.336071] LAST + 1 UNKNOWN [ 819.336075] -1 UNKNOWN [ 819.336078] -2 UNKNOWN [ 819.336081] -3 UNKNOWN [ 819.336084] -4 UNKNOWN I'll make this change op int->unsigned as it is present in the debugfs code so that it will be consistent, thanks again for looking into this. > Thanks, > > Bart. >
On 6/18/19 3:00 PM, Chaitanya Kulkarni wrote: > Regarding the correctness, I think if one of the operand is unsigned > then signed values are converted to unsigned valued implicitly. > > However the exceptions is if the type of the operand with signed integer > type can represent all of the values of the type of the operand with > unsigned integer type, then the operand with unsigned integer type is > converted to the type of the operand with signed integer type. > > I'm wondering how would above scenario occur when comparing int and > size_t. (unless on a platform int can fit all the values into size_t). > Since above comparison of the ARRAY_SIZE involves sizeof (size_t) type > is a base unsigned integer value, even if op < 0 it will get converted > into the unsigned and it will still work. > > Please correct me if I'm wrong. Since a long comment was needed to explain this, that means that the mental effort for anyone who wants to verify the blk_op_str() code is large. Please make sure that kernel code is easy to read and to verify. Thanks, Bart.
diff --git a/block/blk-core.c b/block/blk-core.c index 6753231b529b..c92b5a16a27a 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -120,6 +120,42 @@ void blk_rq_init(struct request_queue *q, struct request *rq) } EXPORT_SYMBOL(blk_rq_init); +#define REQ_OP_NAME(name) [REQ_OP_##name] = #name +static const char *const blk_op_name[] = { + REQ_OP_NAME(READ), + REQ_OP_NAME(WRITE), + REQ_OP_NAME(FLUSH), + REQ_OP_NAME(DISCARD), + REQ_OP_NAME(SECURE_ERASE), + REQ_OP_NAME(ZONE_RESET), + REQ_OP_NAME(WRITE_SAME), + REQ_OP_NAME(WRITE_ZEROES), + REQ_OP_NAME(SCSI_IN), + REQ_OP_NAME(SCSI_OUT), + REQ_OP_NAME(DRV_IN), + REQ_OP_NAME(DRV_OUT), +}; +#undef REQ_OP_NAME + +/** + * blk_op_str - Return string XXX in the REQ_OP_XXX. + * @op: REQ_OP_XXX. + * + * Description: Centralize block layer function to convert REQ_OP_XXX into + * string format. Useful in the debugging and tracing bio or request. For + * invalid REQ_OP_XXX it returns string "UNKNOWN". + */ +inline const char *blk_op_str(int op) +{ + const char *op_str = "UNKNOWN"; + + if (op < ARRAY_SIZE(blk_op_name) && blk_op_name[op]) + op_str = blk_op_name[op]; + + return op_str; +} +EXPORT_SYMBOL_GPL(blk_op_str); + static const struct { int errno; const char *name; diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 592669bcc536..077a77a4a91c 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -867,6 +867,9 @@ extern void blk_execute_rq(struct request_queue *, struct gendisk *, extern void blk_execute_rq_nowait(struct request_queue *, struct gendisk *, struct request *, int, rq_end_io_fn *); +/* Helper to convert REQ_OP_XXX to its string format XXX */ +extern const char *blk_op_str(int op); + int blk_status_to_errno(blk_status_t status); blk_status_t errno_to_blk_status(int errno);
In order to centralize the REQ_OP_XXX to string conversion which can be used in the block layer and different places in the kernel like f2fs, this patch adds a new helper function along with an array similar to the one present in the blk-mq-debugfs.c. We keep this helper functionality centralize under blk-core.c instead of blk-mq-debugfs.c since blk-core.c is configured using CONFIG_BLOCK and it will not be dependent on blk-mq-debugfs.c which is configured using CONFIG_BLK_DEBUG_FS which can be disabled when block layer debugging is not needed by the user. Next patch adjusts the code in the blk-mq-debugfs.c with newly introduced helper. Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> --- block/blk-core.c | 36 ++++++++++++++++++++++++++++++++++++ include/linux/blkdev.h | 3 +++ 2 files changed, 39 insertions(+)