[PATCH 0/2 v3] blktrace: Fix sparse annotations and warn if enabling multiple traces

[PATCH 0/2 v3] blktrace: Fix sparse annotations and warn if enabling multiple traces

[Jan Kara]

Hello Jens,

this series contains a patch from Luis' blktrace series and then my patch
to fix sparse warnings in blktrace. Luis' patch stands on its own, was
reviewed, and changes what I need to change as well so I've decided it's just
simplest to pull it in with my patch.

								Honza

[PATCH 1/2] blktrace: break out of blktrace setup on concurrent calls

[Jan Kara]

From: Luis Chamberlain <mcgrof@kernel.org>

We use one blktrace per request_queue, that means one per the entire
disk.  So we cannot run one blktrace on say /dev/vda and then /dev/vda1,
or just two calls on /dev/vda.

We check for concurrent setup only at the very end of the blktrace setup though.

If we try to run two concurrent blktraces on the same block device the
second one will fail, and the first one seems to go on. However when
one tries to kill the first one one will see things like this:

The kernel will show these:

```
debugfs: File 'dropped' in directory 'nvme1n1' already present!
debugfs: File 'msg' in directory 'nvme1n1' already present!
debugfs: File 'trace0' in directory 'nvme1n1' already present!
``

And userspace just sees this error message for the second call:

```
blktrace /dev/nvme1n1
BLKTRACESETUP(2) /dev/nvme1n1 failed: 5/Input/output error
```

The first userspace process #1 will also claim that the files
were taken underneath their nose as well. The files are taken
away form the first process given that when the second blktrace
fails, it will follow up with a BLKTRACESTOP and BLKTRACETEARDOWN.
This means that even if go-happy process #1 is waiting for blktrace
data, we *have* been asked to take teardown the blktrace.

This can easily be reproduced with break-blktrace [0] run_0005.sh test.

Just break out early if we know we're already going to fail, this will
prevent trying to create the files all over again, which we know still
exist.

[0] https://github.com/mcgrof/break-blktrace

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 kernel/trace/blktrace.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index ea47f2084087..1a1943d39802 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -3,6 +3,9 @@
  * Copyright (C) 2006 Jens Axboe <axboe@kernel.dk>
  *
  */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
 #include <linux/blkdev.h>
 #include <linux/blktrace_api.h>
@@ -494,6 +497,16 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	 */
 	strreplace(buts->name, '/', '_');
 
+	/*
+	 * bdev can be NULL, as with scsi-generic, this is a helpful as
+	 * we can be.
+	 */
+	if (q->blk_trace) {
+		pr_warn("Concurrent blktraces are not allowed on %s\n",
+			buts->name);
+		return -EBUSY;
+	}
+
 	bt = kzalloc(sizeof(*bt), GFP_KERNEL);
 	if (!bt)
 		return -ENOMEM;
-- 
2.16.4

Re: [PATCH 1/2] blktrace: break out of blktrace setup on concurrent calls

[Chaitanya Kulkarni]

On 6/5/20 7:58 AM, Jan Kara wrote:
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>   #include <linux/kernel.h>
>   #include <linux/blkdev.h>
>   #include <linux/blktrace_api.h>
> @@ -494,6 +497,16 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
>   	 */
>   	strreplace(buts->name, '/', '_');
>   
> +	/*
> +	 * bdev can be NULL, as with scsi-generic, this is a helpful as
> +	 * we can be.
> +	 */
> +	if (q->blk_trace) {
> +		pr_warn("Concurrent blktraces are not allowed on %s\n",
> +			buts->name);
> +		return -EBUSY;
> +	}
> +
>   	bt = kzalloc(sizeof(*bt), GFP_KERNEL);
>   	if (!bt)
>   		return -ENOMEM;
> -- 2.16.4

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

[PATCH 2/2] blktrace: Avoid sparse warnings when assigning q->blk_trace

[Jan Kara]

Mostly for historical reasons, q->blk_trace is assigned through xchg()
and cmpxchg() atomic operations. Although this is correct, sparse
complains about this because it violates rcu annotations since commit
c780e86dd48e ("blktrace: Protect q->blk_trace with RCU") which started
to use rcu for accessing q->blk_trace. Furthermore there's no real need
for atomic operations anymore since all changes to q->blk_trace happen
under q->blk_trace_mutex and since it also makes more sense to check if
q->blk_trace is set with the mutex held earlier.

So let's just replace xchg() with rcu_replace_pointer() and cmpxchg()
with explicit check and rcu_assign_pointer(). This makes the code more
efficient and sparse happy.

Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 kernel/trace/blktrace.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 1a1943d39802..1b8e2eaf23f7 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -347,7 +347,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;
 
@@ -501,7 +502,8 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	 * bdev can be NULL, as with scsi-generic, this is a helpful as
 	 * we can be.
 	 */
-	if (q->blk_trace) {
+	if (rcu_dereference_protected(q->blk_trace,
+				      lockdep_is_held(&q->blk_trace_mutex))) {
 		pr_warn("Concurrent blktraces are not allowed on %s\n",
 			buts->name);
 		return -EBUSY;
@@ -556,10 +558,7 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	bt->pid = buts->pid;
 	bt->trace_state = Blktrace_setup;
 
-	ret = -EBUSY;
-	if (cmpxchg(&q->blk_trace, NULL, bt))
-		goto err;
-
+	rcu_assign_pointer(q->blk_trace, bt);
 	get_probe_ref();
 
 	ret = 0;
@@ -1650,7 +1649,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;
 
@@ -1682,10 +1682,7 @@ 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))
-		goto free_bt;
-
+	rcu_assign_pointer(q->blk_trace, bt);
 	get_probe_ref();
 	return 0;
 
-- 
2.16.4

Re: [PATCH 2/2] blktrace: Avoid sparse warnings when assigning q->blk_trace

[Chaitanya Kulkarni]

On 6/5/20 7:58 AM, Jan Kara wrote:
> Mostly for historical reasons, q->blk_trace is assigned through xchg()
> and cmpxchg() atomic operations. Although this is correct, sparse
> complains about this because it violates rcu annotations since commit
> c780e86dd48e ("blktrace: Protect q->blk_trace with RCU") which started
> to use rcu for accessing q->blk_trace. Furthermore there's no real need
> for atomic operations anymore since all changes to q->blk_trace happen
> under q->blk_trace_mutex and since it also makes more sense to check if
> q->blk_trace is set with the mutex held earlier.
> 
> So let's just replace xchg() with rcu_replace_pointer() and cmpxchg()
> with explicit check and rcu_assign_pointer(). This makes the code more
> efficient and sparse happy.
> 
> Reported-by: kbuild test robot<lkp@intel.com>
> Signed-off-by: Jan Kara<jack@suse.cz>

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Re: [PATCH 0/2 v3] blktrace: Fix sparse annotations and warn if enabling multiple traces

[Luis Chamberlain]

On Fri, Jun 05, 2020 at 04:58:35PM +0200, Jan Kara wrote:
> Hello Jens,
> 
> this series contains a patch from Luis' blktrace series and then my patch
> to fix sparse warnings in blktrace. Luis' patch stands on its own, was
> reviewed, and changes what I need to change as well so I've decided it's just
> simplest to pull it in with my patch.

Hey Jan,

Since these patches have contexts which can affect other patches in the
series of fixes I have, if possible I'd prefer if these get addressed in
order. I was just waiting for 0-day to finish grinding on the series.
I already have a successful build results, but just waiting on the
series of other tests, part of which include blktests.

If this is agreeable, I hope to post that series later today.

  Luis

Re: [PATCH 0/2 v3] blktrace: Fix sparse annotations and warn if enabling multiple traces

[Jan Kara]

Hi Luis!

On Fri 05-06-20 15:03:50, Luis Chamberlain wrote:
> On Fri, Jun 05, 2020 at 04:58:35PM +0200, Jan Kara wrote:
> > Hello Jens,
> > 
> > this series contains a patch from Luis' blktrace series and then my patch
> > to fix sparse warnings in blktrace. Luis' patch stands on its own, was
> > reviewed, and changes what I need to change as well so I've decided it's just
> > simplest to pull it in with my patch.
> 
> Hey Jan,
> 
> Since these patches have contexts which can affect other patches in the
> series of fixes I have, if possible I'd prefer if these get addressed in
> order. I was just waiting for 0-day to finish grinding on the series.
> I already have a successful build results, but just waiting on the
> series of other tests, part of which include blktests.
> 
> If this is agreeable, I hope to post that series later today.

Whatever... I wasn't sure how long it's going to take to finish your series
and since Jens was asking about the sparse fixes, I've just sent them
along. BTW the two patches applied just fine separately from the rest of
the series so I don't think there's any dependency to the rest of your
series even in context.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/2 v3] blktrace: Fix sparse annotations and warn if enabling multiple traces

[Luis Chamberlain]

On Mon, Jun 08, 2020 at 10:25:28AM +0200, Jan Kara wrote:
> BTW the two patches applied just fine separately from the rest of
> the series so I don't think there's any dependency to the rest of your
> series even in context.

The depndency was just minor contextual edits, which can be fixed with
a rebase, I've done that now, so this is good to go in first. I'll skip
these two patches fromy series then.

  Luis

Re: [PATCH 0/2 v3] blktrace: Fix sparse annotations and warn if enabling multiple traces

[Jan Kara]

Hello Jens!

On Fri 05-06-20 16:58:35, Jan Kara wrote:
> this series contains a patch from Luis' blktrace series and then my patch
> to fix sparse warnings in blktrace. Luis' patch stands on its own, was
> reviewed, and changes what I need to change as well so I've decided it's just
> simplest to pull it in with my patch.

Can you please merge this series?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/2 v3] blktrace: Fix sparse annotations and warn if enabling multiple traces

[Jens Axboe]

On 6/17/20 6:51 AM, Jan Kara wrote:
> Hello Jens!
> 
> On Fri 05-06-20 16:58:35, Jan Kara wrote:
>> this series contains a patch from Luis' blktrace series and then my patch
>> to fix sparse warnings in blktrace. Luis' patch stands on its own, was
>> reviewed, and changes what I need to change as well so I've decided it's just
>> simplest to pull it in with my patch.
> 
> Can you please merge this series?

Yep done, thanks.

-- 
Jens Axboe