* [PATCH 2/2] tracing/blktrace: Fix to allow setting same value [not found] <153441301698.6024.1680500212551396320.stgit@devbox> @ 2018-08-16 9:51 ` Masami Hiramatsu 2018-08-16 14:38 ` Steven Rostedt 0 siblings, 1 reply; 5+ messages in thread From: Masami Hiramatsu @ 2018-08-16 9:51 UTC (permalink / raw) To: Steven Rostedt Cc: Ingo Molnar, Masami Hiramatsu, linux-kernel, Jens Axboe, linux-block Current trace-enable attribute in sysfs returns an error if user writes the same setting value as current one, e.g. # cat /sys/block/sda/trace/enable 0 # echo 0 > /sys/block/sda/trace/enable bash: echo: write error: Invalid argument # echo 1 > /sys/block/sda/trace/enable # echo 1 > /sys/block/sda/trace/enable bash: echo: write error: Device or resource busy But this is not a preferred behavior, it should ignore if new setting is same as current one. This fixes the problem as below. # cat /sys/block/sda/trace/enable 0 # echo 0 > /sys/block/sda/trace/enable # echo 1 > /sys/block/sda/trace/enable # echo 1 > /sys/block/sda/trace/enable Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> Cc: Jens Axboe <axboe@kernel.dk> Cc: linux-block@vger.kernel.org --- kernel/trace/blktrace.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c index 987d9a9ae283..e67db1e8a000 100644 --- a/kernel/trace/blktrace.c +++ b/kernel/trace/blktrace.c @@ -1602,11 +1602,11 @@ static int blk_trace_remove_queue(struct request_queue *q) struct blk_trace *bt; bt = xchg(&q->blk_trace, NULL); - if (bt == NULL) - return -EINVAL; + if (bt != NULL) { + put_probe_ref(); + blk_trace_free(bt); + } - put_probe_ref(); - blk_trace_free(bt); return 0; } @@ -1619,6 +1619,9 @@ static int blk_trace_setup_queue(struct request_queue *q, struct blk_trace *bt = NULL; int ret = -ENOMEM; + if (q->blk_trace) + return 0; + bt = kzalloc(sizeof(*bt), GFP_KERNEL); if (!bt) return -ENOMEM; ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] tracing/blktrace: Fix to allow setting same value 2018-08-16 9:51 ` [PATCH 2/2] tracing/blktrace: Fix to allow setting same value Masami Hiramatsu @ 2018-08-16 14:38 ` Steven Rostedt 2018-08-16 16:38 ` Masami Hiramatsu 0 siblings, 1 reply; 5+ messages in thread From: Steven Rostedt @ 2018-08-16 14:38 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: Ingo Molnar, linux-kernel, Jens Axboe, linux-block On Thu, 16 Aug 2018 18:51:15 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > Current trace-enable attribute in sysfs returns an error > if user writes the same setting value as current one, > e.g. > > # cat /sys/block/sda/trace/enable > 0 > # echo 0 > /sys/block/sda/trace/enable > bash: echo: write error: Invalid argument > # echo 1 > /sys/block/sda/trace/enable > # echo 1 > /sys/block/sda/trace/enable > bash: echo: write error: Device or resource busy > > But this is not a preferred behavior, it should ignore > if new setting is same as current one. This fixes the > problem as below. > > # cat /sys/block/sda/trace/enable > 0 > # echo 0 > /sys/block/sda/trace/enable > # echo 1 > /sys/block/sda/trace/enable > # echo 1 > /sys/block/sda/trace/enable > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Jens Axboe <axboe@kernel.dk> > Cc: linux-block@vger.kernel.org > --- > kernel/trace/blktrace.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c > index 987d9a9ae283..e67db1e8a000 100644 > --- a/kernel/trace/blktrace.c > +++ b/kernel/trace/blktrace.c > @@ -1602,11 +1602,11 @@ static int blk_trace_remove_queue(struct request_queue *q) > struct blk_trace *bt; > > bt = xchg(&q->blk_trace, NULL); > - if (bt == NULL) > - return -EINVAL; > + if (bt != NULL) { > + put_probe_ref(); > + blk_trace_free(bt); > + } > > - put_probe_ref(); > - blk_trace_free(bt); > return 0; > } > > @@ -1619,6 +1619,9 @@ static int blk_trace_setup_queue(struct request_queue *q, > struct blk_trace *bt = NULL; > int ret = -ENOMEM; > > + if (q->blk_trace) > + return 0; > + > bt = kzalloc(sizeof(*bt), GFP_KERNEL); > if (!bt) > return -ENOMEM; Hmm, why not just do this? Does this fix it too? diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c index 987d9a9ae283..10b2b9c5fd25 100644 --- a/kernel/trace/blktrace.c +++ b/kernel/trace/blktrace.c @@ -1841,6 +1841,8 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev, mutex_lock(&q->blk_trace_mutex); if (attr == &dev_attr_enable) { + if (!!value == !!q->blk_trace) + goto out_unlock_bdev; if (value) ret = blk_trace_setup_queue(q, bdev); else -- Steve ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] tracing/blktrace: Fix to allow setting same value 2018-08-16 14:38 ` Steven Rostedt @ 2018-08-16 16:38 ` Masami Hiramatsu 2018-08-16 16:49 ` Steven Rostedt 0 siblings, 1 reply; 5+ messages in thread From: Masami Hiramatsu @ 2018-08-16 16:38 UTC (permalink / raw) To: Steven Rostedt; +Cc: Ingo Molnar, linux-kernel, Jens Axboe, linux-block On Thu, 16 Aug 2018 10:38:02 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Thu, 16 Aug 2018 18:51:15 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > Current trace-enable attribute in sysfs returns an error > > if user writes the same setting value as current one, > > e.g. > > > > # cat /sys/block/sda/trace/enable > > 0 > > # echo 0 > /sys/block/sda/trace/enable > > bash: echo: write error: Invalid argument > > # echo 1 > /sys/block/sda/trace/enable > > # echo 1 > /sys/block/sda/trace/enable > > bash: echo: write error: Device or resource busy > > > > But this is not a preferred behavior, it should ignore > > if new setting is same as current one. This fixes the > > problem as below. > > > > # cat /sys/block/sda/trace/enable > > 0 > > # echo 0 > /sys/block/sda/trace/enable > > # echo 1 > /sys/block/sda/trace/enable > > # echo 1 > /sys/block/sda/trace/enable > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > Cc: Jens Axboe <axboe@kernel.dk> > > Cc: linux-block@vger.kernel.org > > --- > > kernel/trace/blktrace.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c > > index 987d9a9ae283..e67db1e8a000 100644 > > --- a/kernel/trace/blktrace.c > > +++ b/kernel/trace/blktrace.c > > @@ -1602,11 +1602,11 @@ static int blk_trace_remove_queue(struct request_queue *q) > > struct blk_trace *bt; > > > > bt = xchg(&q->blk_trace, NULL); > > - if (bt == NULL) > > - return -EINVAL; > > + if (bt != NULL) { > > + put_probe_ref(); > > + blk_trace_free(bt); > > + } > > > > - put_probe_ref(); > > - blk_trace_free(bt); > > return 0; > > } > > > > @@ -1619,6 +1619,9 @@ static int blk_trace_setup_queue(struct request_queue *q, > > struct blk_trace *bt = NULL; > > int ret = -ENOMEM; > > > > + if (q->blk_trace) > > + return 0; > > + > > bt = kzalloc(sizeof(*bt), GFP_KERNEL); > > if (!bt) > > return -ENOMEM; > > Hmm, why not just do this? Does this fix it too? Ah, yes, but we need to clear "ret" in that case. Thanks! > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c > index 987d9a9ae283..10b2b9c5fd25 100644 > --- a/kernel/trace/blktrace.c > +++ b/kernel/trace/blktrace.c > @@ -1841,6 +1841,8 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev, > mutex_lock(&q->blk_trace_mutex); > > if (attr == &dev_attr_enable) { > + if (!!value == !!q->blk_trace) > + goto out_unlock_bdev; > if (value) > ret = blk_trace_setup_queue(q, bdev); > else > > -- Steve -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] tracing/blktrace: Fix to allow setting same value 2018-08-16 16:38 ` Masami Hiramatsu @ 2018-08-16 16:49 ` Steven Rostedt 2018-08-16 17:07 ` Masami Hiramatsu 0 siblings, 1 reply; 5+ messages in thread From: Steven Rostedt @ 2018-08-16 16:49 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: Ingo Molnar, linux-kernel, Jens Axboe, linux-block On Fri, 17 Aug 2018 01:38:47 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > On Thu, 16 Aug 2018 10:38:02 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > On Thu, 16 Aug 2018 18:51:15 +0900 > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > Current trace-enable attribute in sysfs returns an error > > > if user writes the same setting value as current one, > > > e.g. > > > > > > # cat /sys/block/sda/trace/enable > > > 0 > > > # echo 0 > /sys/block/sda/trace/enable > > > bash: echo: write error: Invalid argument > > > # echo 1 > /sys/block/sda/trace/enable > > > # echo 1 > /sys/block/sda/trace/enable > > > bash: echo: write error: Device or resource busy > > > > > > But this is not a preferred behavior, it should ignore > > > if new setting is same as current one. This fixes the > > > problem as below. > > > > > > # cat /sys/block/sda/trace/enable > > > 0 > > > # echo 0 > /sys/block/sda/trace/enable > > > # echo 1 > /sys/block/sda/trace/enable > > > # echo 1 > /sys/block/sda/trace/enable > > > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > > Cc: Jens Axboe <axboe@kernel.dk> > > > Cc: linux-block@vger.kernel.org > > > --- > > > kernel/trace/blktrace.c | 11 +++++++---- > > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c > > > index 987d9a9ae283..e67db1e8a000 100644 > > > --- a/kernel/trace/blktrace.c > > > +++ b/kernel/trace/blktrace.c > > > @@ -1602,11 +1602,11 @@ static int blk_trace_remove_queue(struct request_queue *q) > > > struct blk_trace *bt; > > > > > > bt = xchg(&q->blk_trace, NULL); > > > - if (bt == NULL) > > > - return -EINVAL; > > > + if (bt != NULL) { > > > + put_probe_ref(); > > > + blk_trace_free(bt); > > > + } > > > > > > - put_probe_ref(); > > > - blk_trace_free(bt); > > > return 0; > > > } > > > > > > @@ -1619,6 +1619,9 @@ static int blk_trace_setup_queue(struct request_queue *q, > > > struct blk_trace *bt = NULL; > > > int ret = -ENOMEM; > > > > > > + if (q->blk_trace) > > > + return 0; > > > + > > > bt = kzalloc(sizeof(*bt), GFP_KERNEL); > > > if (!bt) > > > return -ENOMEM; > > > > Hmm, why not just do this? Does this fix it too? > > Ah, yes, but we need to clear "ret" in that case. Sure. > > Thanks! > > > > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c > > index 987d9a9ae283..10b2b9c5fd25 100644 > > --- a/kernel/trace/blktrace.c > > +++ b/kernel/trace/blktrace.c > > @@ -1841,6 +1841,8 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev, > > mutex_lock(&q->blk_trace_mutex); > > > > if (attr == &dev_attr_enable) { > > + if (!!value == !!q->blk_trace) { + ret = 0; > > + goto out_unlock_bdev; + } -- Steve > > if (value) > > ret = blk_trace_setup_queue(q, bdev); > > else > > > > -- Steve > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] tracing/blktrace: Fix to allow setting same value 2018-08-16 16:49 ` Steven Rostedt @ 2018-08-16 17:07 ` Masami Hiramatsu 0 siblings, 0 replies; 5+ messages in thread From: Masami Hiramatsu @ 2018-08-16 17:07 UTC (permalink / raw) To: Steven Rostedt; +Cc: Ingo Molnar, linux-kernel, Jens Axboe, linux-block On Thu, 16 Aug 2018 12:49:17 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > Hmm, why not just do this? Does this fix it too? > > > > Ah, yes, but we need to clear "ret" in that case. > > Sure. > > > > > Thanks! > > > > > > > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c > > > index 987d9a9ae283..10b2b9c5fd25 100644 > > > --- a/kernel/trace/blktrace.c > > > +++ b/kernel/trace/blktrace.c > > > @@ -1841,6 +1841,8 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev, > > > mutex_lock(&q->blk_trace_mutex); > > > > > > > if (attr == &dev_attr_enable) { > > > + if (!!value == !!q->blk_trace) { > + ret = 0; > > > + goto out_unlock_bdev; > + } > OK, I tested and ensured that this works. Tested-by: Masami Hiramatsu <mhiramat@kernel.org> Thanks! -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-08-16 17:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <153441301698.6024.1680500212551396320.stgit@devbox>
2018-08-16 9:51 ` [PATCH 2/2] tracing/blktrace: Fix to allow setting same value Masami Hiramatsu
2018-08-16 14:38 ` Steven Rostedt
2018-08-16 16:38 ` Masami Hiramatsu
2018-08-16 16:49 ` Steven Rostedt
2018-08-16 17:07 ` Masami Hiramatsu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox