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

[PATCH] 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. Furthermore
there's no real need for atomic operations anymore since all changes to
q->blk_trace happen under q->blk_trace_mutex. 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 | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index ca39dc3230cb..e4a9ba85b76f 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;
 
@@ -485,6 +486,10 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	if (!blk_debugfs_root)
 		return -ENOENT;
 
+	if (rcu_dereference_protected(q->blk_trace,
+				      lockdep_is_held(&q->blk_trace_mutex)))
+		return -EBUSY;
+
 	strncpy(buts->name, name, BLKTRACE_BDEV_SIZE);
 	buts->name[BLKTRACE_BDEV_SIZE - 1] = '\0';
 
@@ -543,10 +548,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;
@@ -1637,7 +1639,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;
 
@@ -1669,10 +1672,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] blktrace: Avoid sparse warnings when assigning q->blk_trace

[Bart Van Assche]

(+Luis)

On 2020-05-28 02:29, 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. Furthermore
> there's no real need for atomic operations anymore since all changes to
> q->blk_trace happen under q->blk_trace_mutex. 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>

How about adding a reference to commit c780e86dd48e ("blktrace: Protect
q->blk_trace with RCU") in the description of this patch?

> @@ -1669,10 +1672,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;

This changes a conditional assignment of q->blk_trace into an
unconditional assignment. Shouldn't q->blk_trace only be assigned if
q->blk_trace == NULL?

Thanks,

Bart.

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

[Luis Chamberlain]

On Thu, May 28, 2020 at 07:44:38AM -0700, Bart Van Assche wrote:
> > @@ -1669,10 +1672,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;
> 
> Shouldn't q->blk_trace only be assigned if
> q->blk_trace == NULL?

Yes but the old call checked for that and left it as NULL if
it was not NULL.

I have a patch in my series which checks for q->blkt_trace *early*
prior to continuing to avoid concurrent calls proactively, otherwise
this will fail only at the very end.

  Luis

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

[Jan Kara]

On Thu 28-05-20 07:44:38, Bart Van Assche wrote:
> (+Luis)
> 
> On 2020-05-28 02:29, 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. Furthermore
> > there's no real need for atomic operations anymore since all changes to
> > q->blk_trace happen under q->blk_trace_mutex. 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>
> 
> How about adding a reference to commit c780e86dd48e ("blktrace: Protect
> q->blk_trace with RCU") in the description of this patch?

Yes, that's probably a good idea.

> > @@ -1669,10 +1672,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;
> 
> This changes a conditional assignment of q->blk_trace into an
> unconditional assignment. Shouldn't q->blk_trace only be assigned if
> q->blk_trace == NULL?

Yes but both callers of blk_trace_setup_queue() actually check that
q->blk_trace is NULL before calling blk_trace_setup_queue() and since we
hold blk_trace_mutex all the time, the value of q->blk_trace cannot change.
So the conditional assignment was just bogus.

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

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

[Luis Chamberlain]

On Thu, May 28, 2020 at 08:31:52PM +0200, Jan Kara wrote:
> On Thu 28-05-20 07:44:38, Bart Van Assche wrote:
> > (+Luis)
> > 
> > On 2020-05-28 02:29, 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. Furthermore
> > > there's no real need for atomic operations anymore since all changes to
> > > q->blk_trace happen under q->blk_trace_mutex. 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>
> > 
> > How about adding a reference to commit c780e86dd48e ("blktrace: Protect
> > q->blk_trace with RCU") in the description of this patch?
> 
> Yes, that's probably a good idea.
> 
> > > @@ -1669,10 +1672,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;
> > 
> > This changes a conditional assignment of q->blk_trace into an
> > unconditional assignment. Shouldn't q->blk_trace only be assigned if
> > q->blk_trace == NULL?
> 
> Yes but both callers of blk_trace_setup_queue() actually check that
> q->blk_trace is NULL before calling blk_trace_setup_queue() and since we
> hold blk_trace_mutex all the time, the value of q->blk_trace cannot change.
> So the conditional assignment was just bogus.

If you run a blktrace against a different partition the check does have
an effect today. This is because the request_queue is shared between
partitions implicitly, even though they end up using a different struct
dentry. So the check is actually still needed, however my change adds
this check early as well so we don't do a memory allocation just to
throw it away.

Even then, the last final check is neede then.

Try these:

https://github.com/mcgrof/break-blktrace

  Luis

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

[Jan Kara]

On Thu 28-05-20 18:43:33, Luis Chamberlain wrote:
> On Thu, May 28, 2020 at 08:31:52PM +0200, Jan Kara wrote:
> > On Thu 28-05-20 07:44:38, Bart Van Assche wrote:
> > > (+Luis)
> > > 
> > > On 2020-05-28 02:29, 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. Furthermore
> > > > there's no real need for atomic operations anymore since all changes to
> > > > q->blk_trace happen under q->blk_trace_mutex. 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>
> > > 
> > > How about adding a reference to commit c780e86dd48e ("blktrace: Protect
> > > q->blk_trace with RCU") in the description of this patch?
> > 
> > Yes, that's probably a good idea.
> > 
> > > > @@ -1669,10 +1672,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;
> > > 
> > > This changes a conditional assignment of q->blk_trace into an
> > > unconditional assignment. Shouldn't q->blk_trace only be assigned if
> > > q->blk_trace == NULL?
> > 
> > Yes but both callers of blk_trace_setup_queue() actually check that
> > q->blk_trace is NULL before calling blk_trace_setup_queue() and since we
> > hold blk_trace_mutex all the time, the value of q->blk_trace cannot change.
> > So the conditional assignment was just bogus.
> 
> If you run a blktrace against a different partition the check does have
> an effect today. This is because the request_queue is shared between
> partitions implicitly, even though they end up using a different struct
> dentry. So the check is actually still needed, however my change adds
> this check early as well so we don't do a memory allocation just to
> throw it away.

I'm not sure we are speaking about the same check but I might be missing
something. blk_trace_setup_queue() is only called from
sysfs_blk_trace_attr_store(). That does:

        mutex_lock(&q->blk_trace_mutex);

        bt = rcu_dereference_protected(q->blk_trace,
                                       lockdep_is_held(&q->blk_trace_mutex));
        if (attr == &dev_attr_enable) {
                if (!!value == !!bt) {
                        ret = 0;
                        goto out_unlock_bdev;
                }

		^^^ So if 'bt' is non-NULL, and we are enabling, we bail
instead of calling blk_trace_setup_queue().

Similarly later:

        if (bt == NULL) {
                ret = blk_trace_setup_queue(q, bdev);
	...
so we again call blk_trace_setup_queue() only if bt is NULL. So IMO the
cmpxchg() in blk_trace_setup_queue() could never fail to set the value.
Am I missing something?

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

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

[Luis Chamberlain]

On Thu, May 28, 2020 at 08:55:39PM +0200, Jan Kara wrote:
> On Thu 28-05-20 18:43:33, Luis Chamberlain wrote:
> > On Thu, May 28, 2020 at 08:31:52PM +0200, Jan Kara wrote:
> > > On Thu 28-05-20 07:44:38, Bart Van Assche wrote:
> > > > (+Luis)
> > > > 
> > > > On 2020-05-28 02:29, 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. Furthermore
> > > > > there's no real need for atomic operations anymore since all changes to
> > > > > q->blk_trace happen under q->blk_trace_mutex. 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>
> > > > 
> > > > How about adding a reference to commit c780e86dd48e ("blktrace: Protect
> > > > q->blk_trace with RCU") in the description of this patch?
> > > 
> > > Yes, that's probably a good idea.
> > > 
> > > > > @@ -1669,10 +1672,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;
> > > > 
> > > > This changes a conditional assignment of q->blk_trace into an
> > > > unconditional assignment. Shouldn't q->blk_trace only be assigned if
> > > > q->blk_trace == NULL?
> > > 
> > > Yes but both callers of blk_trace_setup_queue() actually check that
> > > q->blk_trace is NULL before calling blk_trace_setup_queue() and since we
> > > hold blk_trace_mutex all the time, the value of q->blk_trace cannot change.
> > > So the conditional assignment was just bogus.
> > 
> > If you run a blktrace against a different partition the check does have
> > an effect today. This is because the request_queue is shared between
> > partitions implicitly, even though they end up using a different struct
> > dentry. So the check is actually still needed, however my change adds
> > this check early as well so we don't do a memory allocation just to
> > throw it away.
> 
> I'm not sure we are speaking about the same check but I might be missing
> something. blk_trace_setup_queue() is only called from
> sysfs_blk_trace_attr_store(). That does:
> 
>         mutex_lock(&q->blk_trace_mutex);
> 
>         bt = rcu_dereference_protected(q->blk_trace,
>                                        lockdep_is_held(&q->blk_trace_mutex));
>         if (attr == &dev_attr_enable) {
>                 if (!!value == !!bt) {
>                         ret = 0;
>                         goto out_unlock_bdev;
>                 }
> 
> 		^^^ So if 'bt' is non-NULL, and we are enabling, we bail
> instead of calling blk_trace_setup_queue().
> 
> Similarly later:
> 
>         if (bt == NULL) {
>                 ret = blk_trace_setup_queue(q, bdev);
> 	...
> so we again call blk_trace_setup_queue() only if bt is NULL. So IMO the
> cmpxchg() in blk_trace_setup_queue() could never fail to set the value.
> Am I missing something?

I believe we are talking about the same check indeed. Consider the
situation not as a race, but instead consider the state machine of
the ioctl. The BLKTRACESETUP goes first, and when that is over we
have not ran BLKTRACESTART. So, prior to BLKTRACESTART we can have
another BLKTRACESETUP run but against another partition. At that
point we have two cases trying to use the same request_queue and
the same q->blk_trace, even though this was well protected with
the mutex.

And so the final check is needed to ensure we only give one of
the users the right to blktrace.

Did I misunderstand the check though?

  Luis

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

[Jan Kara]

On Fri 29-05-20 08:00:56, Luis Chamberlain wrote:
> On Thu, May 28, 2020 at 08:55:39PM +0200, Jan Kara wrote:
> > On Thu 28-05-20 18:43:33, Luis Chamberlain wrote:
> > > On Thu, May 28, 2020 at 08:31:52PM +0200, Jan Kara wrote:
> > > > On Thu 28-05-20 07:44:38, Bart Van Assche wrote:
> > > > > (+Luis)
> > > > > 
> > > > > On 2020-05-28 02:29, 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. Furthermore
> > > > > > there's no real need for atomic operations anymore since all changes to
> > > > > > q->blk_trace happen under q->blk_trace_mutex. 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>
> > > > > 
> > > > > How about adding a reference to commit c780e86dd48e ("blktrace: Protect
> > > > > q->blk_trace with RCU") in the description of this patch?
> > > > 
> > > > Yes, that's probably a good idea.
> > > > 
> > > > > > @@ -1669,10 +1672,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;
> > > > > 
> > > > > This changes a conditional assignment of q->blk_trace into an
> > > > > unconditional assignment. Shouldn't q->blk_trace only be assigned if
> > > > > q->blk_trace == NULL?
> > > > 
> > > > Yes but both callers of blk_trace_setup_queue() actually check that
> > > > q->blk_trace is NULL before calling blk_trace_setup_queue() and since we
> > > > hold blk_trace_mutex all the time, the value of q->blk_trace cannot change.
> > > > So the conditional assignment was just bogus.
> > > 
> > > If you run a blktrace against a different partition the check does have
> > > an effect today. This is because the request_queue is shared between
> > > partitions implicitly, even though they end up using a different struct
> > > dentry. So the check is actually still needed, however my change adds
> > > this check early as well so we don't do a memory allocation just to
> > > throw it away.
> > 
> > I'm not sure we are speaking about the same check but I might be missing
> > something. blk_trace_setup_queue() is only called from
> > sysfs_blk_trace_attr_store(). That does:
> > 
> >         mutex_lock(&q->blk_trace_mutex);
> > 
> >         bt = rcu_dereference_protected(q->blk_trace,
> >                                        lockdep_is_held(&q->blk_trace_mutex));
> >         if (attr == &dev_attr_enable) {
> >                 if (!!value == !!bt) {
> >                         ret = 0;
> >                         goto out_unlock_bdev;
> >                 }
> > 
> > 		^^^ So if 'bt' is non-NULL, and we are enabling, we bail
> > instead of calling blk_trace_setup_queue().
> > 
> > Similarly later:
> > 
> >         if (bt == NULL) {
> >                 ret = blk_trace_setup_queue(q, bdev);
> > 	...
> > so we again call blk_trace_setup_queue() only if bt is NULL. So IMO the
> > cmpxchg() in blk_trace_setup_queue() could never fail to set the value.
> > Am I missing something?
> 
> I believe we are talking about the same check indeed. Consider the
> situation not as a race, but instead consider the state machine of
> the ioctl. The BLKTRACESETUP goes first, and when that is over we
> have not ran BLKTRACESTART. So, prior to BLKTRACESTART we can have
> another BLKTRACESETUP run but against another partition.

So first note that BLKTRACESETUP goes through do_blk_trace_setup() while
'echo 1 >/sys/block/../trace/enable' goes through blk_trace_setup_queue().
Although these operations achieve a very similar things, they are completely
separate code paths. I was speaking about the second case while you are now
speaking about the first one.

WRT to your BLKTRACESETUP example, the first BLKTRACESETUP will end up
setting q->blk_trace to 'bt' so the second BLKTRACESETUP will see
q->blk_trace is not NULL (my patch adds this check to do_blk_trace_setup()
so we bail out earlier than during cmpxchg()) and fails. Again I don't see
any problem here...

> At that
> point we have two cases trying to use the same request_queue and
> the same q->blk_trace, even though this was well protected with
> the mutex.
> 
> And so the final check is needed to ensure we only give one of
> the users the right to blktrace.

Well, the check to make sure there's only one trace setup is the one
checking q->blk_trace is NULL while holding q->blk_trace_mutex...

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

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

[Luis Chamberlain]

On Fri, May 29, 2020 at 11:04:48AM +0200, Jan Kara wrote:
> On Fri 29-05-20 08:00:56, Luis Chamberlain wrote:
> > On Thu, May 28, 2020 at 08:55:39PM +0200, Jan Kara wrote:
> > > On Thu 28-05-20 18:43:33, Luis Chamberlain wrote:
> > > > On Thu, May 28, 2020 at 08:31:52PM +0200, Jan Kara wrote:
> > > > > On Thu 28-05-20 07:44:38, Bart Van Assche wrote:
> > > > > > (+Luis)
> > > > > > 
> > > > > > On 2020-05-28 02:29, 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. Furthermore
> > > > > > > there's no real need for atomic operations anymore since all changes to
> > > > > > > q->blk_trace happen under q->blk_trace_mutex. 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>
> > > > > > 
> > > > > > How about adding a reference to commit c780e86dd48e ("blktrace: Protect
> > > > > > q->blk_trace with RCU") in the description of this patch?
> > > > > 
> > > > > Yes, that's probably a good idea.
> > > > > 
> > > > > > > @@ -1669,10 +1672,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;
> > > > > > 
> > > > > > This changes a conditional assignment of q->blk_trace into an
> > > > > > unconditional assignment. Shouldn't q->blk_trace only be assigned if
> > > > > > q->blk_trace == NULL?
> > > > > 
> > > > > Yes but both callers of blk_trace_setup_queue() actually check that
> > > > > q->blk_trace is NULL before calling blk_trace_setup_queue() and since we
> > > > > hold blk_trace_mutex all the time, the value of q->blk_trace cannot change.
> > > > > So the conditional assignment was just bogus.
> > > > 
> > > > If you run a blktrace against a different partition the check does have
> > > > an effect today. This is because the request_queue is shared between
> > > > partitions implicitly, even though they end up using a different struct
> > > > dentry. So the check is actually still needed, however my change adds
> > > > this check early as well so we don't do a memory allocation just to
> > > > throw it away.
> > > 
> > > I'm not sure we are speaking about the same check but I might be missing
> > > something. blk_trace_setup_queue() is only called from
> > > sysfs_blk_trace_attr_store(). That does:
> > > 
> > >         mutex_lock(&q->blk_trace_mutex);
> > > 
> > >         bt = rcu_dereference_protected(q->blk_trace,
> > >                                        lockdep_is_held(&q->blk_trace_mutex));
> > >         if (attr == &dev_attr_enable) {
> > >                 if (!!value == !!bt) {
> > >                         ret = 0;
> > >                         goto out_unlock_bdev;
> > >                 }
> > > 
> > > 		^^^ So if 'bt' is non-NULL, and we are enabling, we bail
> > > instead of calling blk_trace_setup_queue().
> > > 
> > > Similarly later:
> > > 
> > >         if (bt == NULL) {
> > >                 ret = blk_trace_setup_queue(q, bdev);
> > > 	...
> > > so we again call blk_trace_setup_queue() only if bt is NULL. So IMO the
> > > cmpxchg() in blk_trace_setup_queue() could never fail to set the value.
> > > Am I missing something?
> > 
> > I believe we are talking about the same check indeed. Consider the
> > situation not as a race, but instead consider the state machine of
> > the ioctl. The BLKTRACESETUP goes first, and when that is over we
> > have not ran BLKTRACESTART. So, prior to BLKTRACESTART we can have
> > another BLKTRACESETUP run but against another partition.
> 
> So first note that BLKTRACESETUP goes through do_blk_trace_setup() while
> 'echo 1 >/sys/block/../trace/enable' goes through blk_trace_setup_queue().
> Although these operations achieve a very similar things, they are completely
> separate code paths. I was speaking about the second case while you are now
> speaking about the first one.
> 
> WRT to your BLKTRACESETUP example, the first BLKTRACESETUP will end up
> setting q->blk_trace to 'bt' so the second BLKTRACESETUP will see
> q->blk_trace is not NULL (my patch adds this check to do_blk_trace_setup()
> so we bail out earlier than during cmpxchg()) and fails. Again I don't see
> any problem here...

Ah, the patch I was CC'd on didn't contain this hunk! It only had the
change from cmpxchg() to the rcu_assign_pointer(), so I misunderstood
your intention, sorry!

In that case, I already proposed a patch to do that, and it also adds
a tiny bit of verbiage given we currently don't inform the user about
why this fails [0].

Let me know how you folks would like to proceed.

[0] https://lkml.kernel.org/r/20200516031956.2605-7-mcgrof@kernel.org

  Luis

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

[Jan Kara]

On Fri 29-05-20 11:43:00, Luis Chamberlain wrote:
> On Fri, May 29, 2020 at 11:04:48AM +0200, Jan Kara wrote:
> > On Fri 29-05-20 08:00:56, Luis Chamberlain wrote:
> > > On Thu, May 28, 2020 at 08:55:39PM +0200, Jan Kara wrote:
> > > > On Thu 28-05-20 18:43:33, Luis Chamberlain wrote:
> > > > > On Thu, May 28, 2020 at 08:31:52PM +0200, Jan Kara wrote:
> > > > > > On Thu 28-05-20 07:44:38, Bart Van Assche wrote:
> > > > > > > (+Luis)
> > > > > > > 
> > > > > > > On 2020-05-28 02:29, 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. Furthermore
> > > > > > > > there's no real need for atomic operations anymore since all changes to
> > > > > > > > q->blk_trace happen under q->blk_trace_mutex. 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>
> > > > > > > 
> > > > > > > How about adding a reference to commit c780e86dd48e ("blktrace: Protect
> > > > > > > q->blk_trace with RCU") in the description of this patch?
> > > > > > 
> > > > > > Yes, that's probably a good idea.
> > > > > > 
> > > > > > > > @@ -1669,10 +1672,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;
> > > > > > > 
> > > > > > > This changes a conditional assignment of q->blk_trace into an
> > > > > > > unconditional assignment. Shouldn't q->blk_trace only be assigned if
> > > > > > > q->blk_trace == NULL?
> > > > > > 
> > > > > > Yes but both callers of blk_trace_setup_queue() actually check that
> > > > > > q->blk_trace is NULL before calling blk_trace_setup_queue() and since we
> > > > > > hold blk_trace_mutex all the time, the value of q->blk_trace cannot change.
> > > > > > So the conditional assignment was just bogus.
> > > > > 
> > > > > If you run a blktrace against a different partition the check does have
> > > > > an effect today. This is because the request_queue is shared between
> > > > > partitions implicitly, even though they end up using a different struct
> > > > > dentry. So the check is actually still needed, however my change adds
> > > > > this check early as well so we don't do a memory allocation just to
> > > > > throw it away.
> > > > 
> > > > I'm not sure we are speaking about the same check but I might be missing
> > > > something. blk_trace_setup_queue() is only called from
> > > > sysfs_blk_trace_attr_store(). That does:
> > > > 
> > > >         mutex_lock(&q->blk_trace_mutex);
> > > > 
> > > >         bt = rcu_dereference_protected(q->blk_trace,
> > > >                                        lockdep_is_held(&q->blk_trace_mutex));
> > > >         if (attr == &dev_attr_enable) {
> > > >                 if (!!value == !!bt) {
> > > >                         ret = 0;
> > > >                         goto out_unlock_bdev;
> > > >                 }
> > > > 
> > > > 		^^^ So if 'bt' is non-NULL, and we are enabling, we bail
> > > > instead of calling blk_trace_setup_queue().
> > > > 
> > > > Similarly later:
> > > > 
> > > >         if (bt == NULL) {
> > > >                 ret = blk_trace_setup_queue(q, bdev);
> > > > 	...
> > > > so we again call blk_trace_setup_queue() only if bt is NULL. So IMO the
> > > > cmpxchg() in blk_trace_setup_queue() could never fail to set the value.
> > > > Am I missing something?
> > > 
> > > I believe we are talking about the same check indeed. Consider the
> > > situation not as a race, but instead consider the state machine of
> > > the ioctl. The BLKTRACESETUP goes first, and when that is over we
> > > have not ran BLKTRACESTART. So, prior to BLKTRACESTART we can have
> > > another BLKTRACESETUP run but against another partition.
> > 
> > So first note that BLKTRACESETUP goes through do_blk_trace_setup() while
> > 'echo 1 >/sys/block/../trace/enable' goes through blk_trace_setup_queue().
> > Although these operations achieve a very similar things, they are completely
> > separate code paths. I was speaking about the second case while you are now
> > speaking about the first one.
> > 
> > WRT to your BLKTRACESETUP example, the first BLKTRACESETUP will end up
> > setting q->blk_trace to 'bt' so the second BLKTRACESETUP will see
> > q->blk_trace is not NULL (my patch adds this check to do_blk_trace_setup()
> > so we bail out earlier than during cmpxchg()) and fails. Again I don't see
> > any problem here...
> 
> Ah, the patch I was CC'd on didn't contain this hunk! It only had the
> change from cmpxchg() to the rcu_assign_pointer(), so I misunderstood
> your intention, sorry!

Good that we are on the same page now :)

> In that case, I already proposed a patch to do that, and it also adds
> a tiny bit of verbiage given we currently don't inform the user about
> why this fails [0].

Honestly, I'm not sure pr_warn() you've added is that useful. We usually
don't spam logs due to someone trying to use already used resource. But
anyway, I can see other people are fine with that so I don't insist.

> Let me know how you folks would like to proceed.

I guess I can rebase my patch on top of your series since that seems pretty
much done. I was aware of it but didn't realize there's a conflict...

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

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

[Luis Chamberlain]

On Fri, May 29, 2020 at 02:11:14PM +0200, Jan Kara wrote:
> On Fri 29-05-20 11:43:00, Luis Chamberlain wrote:
> > On Fri, May 29, 2020 at 11:04:48AM +0200, Jan Kara wrote:
> > > On Fri 29-05-20 08:00:56, Luis Chamberlain wrote:
> > > > On Thu, May 28, 2020 at 08:55:39PM +0200, Jan Kara wrote:
> > > > > On Thu 28-05-20 18:43:33, Luis Chamberlain wrote:
> > > > > > On Thu, May 28, 2020 at 08:31:52PM +0200, Jan Kara wrote:
> > > > > > > On Thu 28-05-20 07:44:38, Bart Van Assche wrote:
> > > > > > > > (+Luis)
> > > > > > > > 
> > > > > > > > On 2020-05-28 02:29, 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. Furthermore
> > > > > > > > > there's no real need for atomic operations anymore since all changes to
> > > > > > > > > q->blk_trace happen under q->blk_trace_mutex. 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>
> > > > > > > > 
> > > > > > > > How about adding a reference to commit c780e86dd48e ("blktrace: Protect
> > > > > > > > q->blk_trace with RCU") in the description of this patch?
> > > > > > > 
> > > > > > > Yes, that's probably a good idea.
> > > > > > > 
> > > > > > > > > @@ -1669,10 +1672,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;
> > > > > > > > 
> > > > > > > > This changes a conditional assignment of q->blk_trace into an
> > > > > > > > unconditional assignment. Shouldn't q->blk_trace only be assigned if
> > > > > > > > q->blk_trace == NULL?
> > > > > > > 
> > > > > > > Yes but both callers of blk_trace_setup_queue() actually check that
> > > > > > > q->blk_trace is NULL before calling blk_trace_setup_queue() and since we
> > > > > > > hold blk_trace_mutex all the time, the value of q->blk_trace cannot change.
> > > > > > > So the conditional assignment was just bogus.
> > > > > > 
> > > > > > If you run a blktrace against a different partition the check does have
> > > > > > an effect today. This is because the request_queue is shared between
> > > > > > partitions implicitly, even though they end up using a different struct
> > > > > > dentry. So the check is actually still needed, however my change adds
> > > > > > this check early as well so we don't do a memory allocation just to
> > > > > > throw it away.
> > > > > 
> > > > > I'm not sure we are speaking about the same check but I might be missing
> > > > > something. blk_trace_setup_queue() is only called from
> > > > > sysfs_blk_trace_attr_store(). That does:
> > > > > 
> > > > >         mutex_lock(&q->blk_trace_mutex);
> > > > > 
> > > > >         bt = rcu_dereference_protected(q->blk_trace,
> > > > >                                        lockdep_is_held(&q->blk_trace_mutex));
> > > > >         if (attr == &dev_attr_enable) {
> > > > >                 if (!!value == !!bt) {
> > > > >                         ret = 0;
> > > > >                         goto out_unlock_bdev;
> > > > >                 }
> > > > > 
> > > > > 		^^^ So if 'bt' is non-NULL, and we are enabling, we bail
> > > > > instead of calling blk_trace_setup_queue().
> > > > > 
> > > > > Similarly later:
> > > > > 
> > > > >         if (bt == NULL) {
> > > > >                 ret = blk_trace_setup_queue(q, bdev);
> > > > > 	...
> > > > > so we again call blk_trace_setup_queue() only if bt is NULL. So IMO the
> > > > > cmpxchg() in blk_trace_setup_queue() could never fail to set the value.
> > > > > Am I missing something?
> > > > 
> > > > I believe we are talking about the same check indeed. Consider the
> > > > situation not as a race, but instead consider the state machine of
> > > > the ioctl. The BLKTRACESETUP goes first, and when that is over we
> > > > have not ran BLKTRACESTART. So, prior to BLKTRACESTART we can have
> > > > another BLKTRACESETUP run but against another partition.
> > > 
> > > So first note that BLKTRACESETUP goes through do_blk_trace_setup() while
> > > 'echo 1 >/sys/block/../trace/enable' goes through blk_trace_setup_queue().
> > > Although these operations achieve a very similar things, they are completely
> > > separate code paths. I was speaking about the second case while you are now
> > > speaking about the first one.
> > > 
> > > WRT to your BLKTRACESETUP example, the first BLKTRACESETUP will end up
> > > setting q->blk_trace to 'bt' so the second BLKTRACESETUP will see
> > > q->blk_trace is not NULL (my patch adds this check to do_blk_trace_setup()
> > > so we bail out earlier than during cmpxchg()) and fails. Again I don't see
> > > any problem here...
> > 
> > Ah, the patch I was CC'd on didn't contain this hunk! It only had the
> > change from cmpxchg() to the rcu_assign_pointer(), so I misunderstood
> > your intention, sorry!
> 
> Good that we are on the same page now :)

Yay!

> > In that case, I already proposed a patch to do that, and it also adds
> > a tiny bit of verbiage given we currently don't inform the user about
> > why this fails [0].
> 
> Honestly, I'm not sure pr_warn() you've added is that useful. We usually
> don't spam logs due to someone trying to use already used resource. But
> anyway, I can see other people are fine with that so I don't insist.

Well I would typically agree... however... 

It is in no way shape or form, not even in the blktrace documentation
that the request_queue / and therefore blktrace is shared between
partitions. Likewise for scsi-generic and say its respective block
device for TYPE_BLOCK.

If it is not obvious to some developer, it won't be obvious to users.
So *why* this fails really today is a mystery to users.

These limitations to the design of blktrace is not well documented
at all.

> > Let me know how you folks would like to proceed.
> 
> I guess I can rebase my patch on top of your series since that seems pretty
> much done.

I think so as well.

> I was aware of it but didn't realize there's a conflict...

Thanks for Bart for pointing it out!

  Luis

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

[Jan Kara]

On Tue 02-06-20 15:10:33, Luis Chamberlain wrote:
> On Tue, Jun 02, 2020 at 02:17:34PM +0000, Luis Chamberlain wrote:
> > On Tue, Jun 02, 2020 at 09:12:05AM +0200, Jan Kara wrote:
> > > Here is version of my patch rebased on top of Luis' blktrace fixes. Luis, if
> > > the patch looks fine, can you perhaps include it in your series since it seems
> > > you'll do another revision of your series due to discussion over patch 5/7?
> > > Thanks!
> > 
> > Sure thing, will throw in the pile.
> 
> I've updated the commit log as follows as well, as I think its important
> to annotate that the check for processing of the blktrace only makes
> sense if it was not set. Let me know if this is fine. The commit log
> is below.

Thanks! The changelog looks good to me.

								Honza

> 
> From: Jan Kara <jack@suse.cz>
> Date: Tue, 2 Jun 2020 09:12:05 +0200
> Subject: [PATCH 1/8] blktrace: Avoid sparse warnings when assigning
>  q->blk_trace
> 
> 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* and this is now
> done through the patch titled "blktrace: break out on concurrent calls"
> and was already before on blk_trace_setup_queue().
> 
> 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>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Luis Chamberlain]

On Tue, Jun 02, 2020 at 02:17:34PM +0000, Luis Chamberlain wrote:
> On Tue, Jun 02, 2020 at 09:12:05AM +0200, Jan Kara wrote:
> > Here is version of my patch rebased on top of Luis' blktrace fixes. Luis, if
> > the patch looks fine, can you perhaps include it in your series since it seems
> > you'll do another revision of your series due to discussion over patch 5/7?
> > Thanks!
> 
> Sure thing, will throw in the pile.

I've updated the commit log as follows as well, as I think its important
to annotate that the check for processing of the blktrace only makes
sense if it was not set. Let me know if this is fine. The commit log
is below.

From: Jan Kara <jack@suse.cz>
Date: Tue, 2 Jun 2020 09:12:05 +0200
Subject: [PATCH 1/8] blktrace: Avoid sparse warnings when assigning
 q->blk_trace

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* and this is now
done through the patch titled "blktrace: break out on concurrent calls"
and was already before on blk_trace_setup_queue().

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>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>

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

[Luis Chamberlain]

On Tue, Jun 02, 2020 at 09:12:05AM +0200, Jan Kara wrote:
> Here is version of my patch rebased on top of Luis' blktrace fixes. Luis, if
> the patch looks fine, can you perhaps include it in your series since it seems
> you'll do another revision of your series due to discussion over patch 5/7?
> Thanks!

Sure thing, will throw in the pile.

  Luis

[PATCH] 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. 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(-)

Here is version of my patch rebased on top of Luis' blktrace fixes. Luis, if
the patch looks fine, can you perhaps include it in your series since it seems
you'll do another revision of your series due to discussion over patch 5/7?
Thanks!

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index ac6650828d49..13bc09e4594c 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -346,7 +346,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;
 
@@ -500,7 +501,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;
@@ -570,10 +572,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;
@@ -1662,7 +1661,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;
 
@@ -1694,10 +1694,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