[V3,2/6] block: add centralize REQ_OP_XXX to string helper
diff mbox series

Message ID 20190618054224.25985-3-chaitanya.kulkarni@wdc.com
State New
Headers show
Series
  • block: improve print_req_error
Related show

Commit Message

Chaitanya Kulkarni June 18, 2019, 5:42 a.m. UTC
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(+)

Comments

Bart Van Assche June 18, 2019, 3:26 p.m. UTC | #1
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.
Chaitanya Kulkarni June 18, 2019, 4:04 p.m. UTC | #2
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.
>
Chaitanya Kulkarni June 18, 2019, 10 p.m. UTC | #3
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.
>
Bart Van Assche June 18, 2019, 10:19 p.m. UTC | #4
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.

Patch
diff mbox series

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);