[PATCH 0/4] blktrace: fix sparse warnings

[PATCH 0/4] blktrace: fix sparse warnings

[Chaitanya Kulkarni]

Hi,

This is a small patch-series which fixes various sparse warnings.

Regards,
Chaitanya

Chaitanya Kulkarni (4):
  blktrace: use rcu calls to access q->blk_trace
  blktrace: use errno instead of bi_status
  blktrace: fix endianness in get_pdu_int()
  blktrace: fix endianness for blk_log_remap()

 kernel/trace/blktrace.c | 48 +++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

-- 
2.22.1

[PATCH 1/4] blktrace: use rcu calls to access q->blk_trace

[Chaitanya Kulkarni]

Since request queue's blk_trace member is been markds as __rcu,
replace xchg() and cmdxchg() calls with appropriate rcu API.
This removes the sparse warnings.

The xchg() call is replaced with rcu_replace_pointer() since all the
calls for xchg are protected are q->blk_trace_mutex. On replacing
the pointer rcu_replace_pointer returns the old value if that value is
NULL then existing code returns an error.

In setup functions cmpxchg() call is replaced with calls to
rcu_dereference_pointer() and rcu_replace_pointer(). The first
dereference pointer call returns the existing value. If the value is
not NULL existing code returns an error, otherwise the second call to
replace pointer sets the new value.        

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

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index ea47f2084087..27c19db01ba4 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -344,7 +344,8 @@ static int __blk_trace_remove(struct request_queue *q)
 {
 	struct blk_trace *bt;
 
-	bt = xchg(&q->blk_trace, NULL);
+	bt = rcu_replace_pointer(q->blk_trace, NULL,
+				 lockdep_is_held(&q->blk_trace_mutex));
 	if (!bt)
 		return -EINVAL;
 
@@ -544,9 +545,13 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	bt->trace_state = Blktrace_setup;
 
 	ret = -EBUSY;
-	if (cmpxchg(&q->blk_trace, NULL, bt))
+	if (rcu_dereference_protected(q->blk_trace,
+				      lockdep_is_held(&q->blk_trace_mutex)))
 		goto err;
 
+	rcu_replace_pointer(q->blk_trace, bt,
+			    lockdep_is_held(&q->blk_trace_mutex));
+
 	get_probe_ref();
 
 	ret = 0;
@@ -1637,7 +1642,8 @@ static int blk_trace_remove_queue(struct request_queue *q)
 {
 	struct blk_trace *bt;
 
-	bt = xchg(&q->blk_trace, NULL);
+	bt = rcu_replace_pointer(q->blk_trace, NULL,
+				 lockdep_is_held(&q->blk_trace_mutex));
 	if (bt == NULL)
 		return -EINVAL;
 
@@ -1670,9 +1676,13 @@ static int blk_trace_setup_queue(struct request_queue *q,
 	blk_trace_setup_lba(bt, bdev);
 
 	ret = -EBUSY;
-	if (cmpxchg(&q->blk_trace, NULL, bt))
+	if (rcu_dereference_protected(q->blk_trace,
+				      lockdep_is_held(&q->blk_trace_mutex)))
 		goto free_bt;
 
+	rcu_replace_pointer(q->blk_trace, bt,
+			    lockdep_is_held(&q->blk_trace_mutex));
+
 	get_probe_ref();
 	return 0;
 
-- 
2.22.1

[PATCH 2/4] blktrace: use errno instead of bi_status

[Chaitanya Kulkarni]

In blk_add_trace_spliti() blk_add_trace_bio_remap() use
blk_status_to_errno() to pass the error instead of pasing the bi_status.
This fixes the sparse warning.

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

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 27c19db01ba4..44a2678b8f44 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -1000,8 +1000,10 @@ static void blk_add_trace_split(void *ignore,
 
 		__blk_add_trace(bt, bio->bi_iter.bi_sector,
 				bio->bi_iter.bi_size, bio_op(bio), bio->bi_opf,
-				BLK_TA_SPLIT, bio->bi_status, sizeof(rpdu),
-				&rpdu, blk_trace_bio_get_cgid(q, bio));
+				BLK_TA_SPLIT,
+				blk_status_to_errno(bio->bi_status),
+				sizeof(rpdu), &rpdu,
+				blk_trace_bio_get_cgid(q, bio));
 	}
 	rcu_read_unlock();
 }
@@ -1038,7 +1040,8 @@ static void blk_add_trace_bio_remap(void *ignore,
 	r.sector_from = cpu_to_be64(from);
 
 	__blk_add_trace(bt, bio->bi_iter.bi_sector, bio->bi_iter.bi_size,
-			bio_op(bio), bio->bi_opf, BLK_TA_REMAP, bio->bi_status,
+			bio_op(bio), bio->bi_opf, BLK_TA_REMAP,
+			blk_status_to_errno(bio->bi_status),
 			sizeof(r), &r, blk_trace_bio_get_cgid(q, bio));
 	rcu_read_unlock();
 }
-- 
2.22.1

[PATCH 3/4] blktrace: fix endianness in get_pdu_int()

[Chaitanya Kulkarni]

In function get_pdu_len() replace variable type from __u64 to
__be64. This fixes sparse warning.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 kernel/trace/blktrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 44a2678b8f44..01e192a52c9f 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -1261,7 +1261,7 @@ static inline __u16 t_error(const struct trace_entry *ent)
 
 static __u64 get_pdu_int(const struct trace_entry *ent, bool has_cg)
 {
-	const __u64 *val = pdu_start(ent, has_cg);
+	const __be64 *val = pdu_start(ent, has_cg);
 	return be64_to_cpu(*val);
 }
 
-- 
2.22.1

[PATCH 4/4] blktrace: fix endianness for blk_log_remap()

[Chaitanya Kulkarni]

The function blk_log_remap() can be simplified by removing the
call to get_pdu_remap() that copies the values into extra variable to
print the data, which also fixes the endiannness warning reported by
sparse.

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

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 01e192a52c9f..7cfc293f9697 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -1265,17 +1265,6 @@ static __u64 get_pdu_int(const struct trace_entry *ent, bool has_cg)
 	return be64_to_cpu(*val);
 }
 
-static void get_pdu_remap(const struct trace_entry *ent,
-			  struct blk_io_trace_remap *r, bool has_cg)
-{
-	const struct blk_io_trace_remap *__r = pdu_start(ent, has_cg);
-	__u64 sector_from = __r->sector_from;
-
-	r->device_from = be32_to_cpu(__r->device_from);
-	r->device_to   = be32_to_cpu(__r->device_to);
-	r->sector_from = be64_to_cpu(sector_from);
-}
-
 typedef void (blk_log_action_t) (struct trace_iterator *iter, const char *act,
 	bool has_cg);
 
@@ -1415,13 +1404,13 @@ static void blk_log_with_error(struct trace_seq *s,
 
 static void blk_log_remap(struct trace_seq *s, const struct trace_entry *ent, bool has_cg)
 {
-	struct blk_io_trace_remap r = { .device_from = 0, };
+	const struct blk_io_trace_remap *__r = pdu_start(ent, has_cg);
 
-	get_pdu_remap(ent, &r, has_cg);
 	trace_seq_printf(s, "%llu + %u <- (%d,%d) %llu\n",
 			 t_sector(ent), t_sec(ent),
-			 MAJOR(r.device_from), MINOR(r.device_from),
-			 (unsigned long long)r.sector_from);
+			 MAJOR(be32_to_cpu(__r->device_from)),
+			 MINOR(be32_to_cpu(__r->device_from)),
+			 be64_to_cpu(__r->sector_from));
 }
 
 static void blk_log_plug(struct trace_seq *s, const struct trace_entry *ent, bool has_cg)
-- 
2.22.1

Re: [PATCH 1/4] blktrace: use rcu calls to access q->blk_trace

[Bart Van Assche]

On 2020-06-01 18:32, Chaitanya Kulkarni wrote:
> Since request queue's blk_trace member is been markds as __rcu,
> replace xchg() and cmdxchg() calls with appropriate rcu API.
> This removes the sparse warnings.

This patch looks like a subset of a patch that was posted a few days ago
by Jan Kara? See also
https://lore.kernel.org/linux-block/20200528092910.11118-1-jack@suse.cz/.

Bart.

Re: [PATCH 1/4] blktrace: use rcu calls to access q->blk_trace

[Chaitanya Kulkarni]

On 6/1/20 7:27 PM, Bart Van Assche wrote:
> On 2020-06-01 18:32, Chaitanya Kulkarni wrote:
>> Since request queue's blk_trace member is been markds as __rcu,
>> replace xchg() and cmdxchg() calls with appropriate rcu API.
>> This removes the sparse warnings.
> This patch looks like a subset of a patch that was posted a few days ago
> by Jan Kara? See also
> https://lore.kernel.org/linux-block/20200528092910.11118-1-jack@suse.cz/.
> 
> Bart.
> 

Thanks for pointing out. I think my patch maintains the goto labels and
keeps the original code flow with two calls as mentioned in the patch
description. Maybe we can merge both patches keeping Jan as an Original 
author since his patch was sent first ?

At the end of the I just want to fix these warnings before I re-spin
blktrace extension series.

Re: [PATCH 1/4] blktrace: use rcu calls to access q->blk_trace

[Jan Kara]

On Tue 02-06-20 03:10:42, Chaitanya Kulkarni wrote:
> On 6/1/20 7:27 PM, Bart Van Assche wrote:
> > On 2020-06-01 18:32, Chaitanya Kulkarni wrote:
> >> Since request queue's blk_trace member is been markds as __rcu,
> >> replace xchg() and cmdxchg() calls with appropriate rcu API.
> >> This removes the sparse warnings.
> > This patch looks like a subset of a patch that was posted a few days ago
> > by Jan Kara? See also
> > https://lore.kernel.org/linux-block/20200528092910.11118-1-jack@suse.cz/.
> > 
> > Bart.
> > 
> 
> Thanks for pointing out. I think my patch maintains the goto labels and
> keeps the original code flow with two calls as mentioned in the patch
> description. Maybe we can merge both patches keeping Jan as an Original 
> author since his patch was sent first ?

Yes, my patch also removes the unnecessary atomic operations besides fixing
the sparse warnings. I'll rebase it on top of Luis' patches [1] which conflict
with it and resend it.

								Honza

[1] https://lore.kernel.org/linux-block/20200516031956.2605-1-mcgrof@kernel.org/
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR