Re: [PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit

[Omar Sandoval]

On Wed, Jan 10, 2018 at 06:42:17PM +, Bart Van Assche wrote:
> On Wed, 2018-01-10 at 11:35 -0700, Jens Axboe wrote:
> > On 1/10/18 11:33 AM, Bart Van Assche wrote:
> > > On Wed, 2018-01-10 at 11:32 -0700, Jens Axboe wrote:
> > > > On 1/10/18 11:29 AM, Bart Van Assche wrote:
> > > > > On Tue, 2018-01-09 at 17:29 -0700, Jens Axboe wrote:
> > > > > > @@ -313,8 +307,6 @@ int __blk_mq_debugfs_rq_show(struct seq_file 
> > > > > > *m, struct request *rq)
> > > > > > seq_puts(m, ", .rq_flags=");
> > > > > > blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
> > > > > >ARRAY_SIZE(rqf_name));
> > > > > > -   seq_puts(m, ", .atomic_flags=");
> > > > > > -   blk_flags_show(m, rq->atomic_flags, rqaf_name, 
> > > > > > ARRAY_SIZE(rqaf_name));
> > > > > > seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
> > > > > >rq->internal_tag);
> > > > > > if (mq_ops->show_rq)
> > > > > 
> > > > > Whether or not a request has been marked complete is very useful to 
> > > > > know. Have you
> > > > > considered to show the return value of blk_rq_is_complete() in the 
> > > > > debugfs output?
> > > > 
> > > > Yeah, that's a good point. Let me add that in lieu of the atomic flags 
> > > > that
> > > > are being killed. Are you fine with the patch then?
> > > 
> > > The rest of the patch looks fine to me. This is the only comment I had 
> > > about this patch.
> > 
> > http://git.kernel.dk/cgit/linux-block/commit/?h=blk-kill-atomic-flags&id=3d34009082f5e72137d6bb38cbc2ff6047f03fd1
> > 
> > Added a complete= entry for it.
> 
> For that patch: Reviewed-by: Bart Van Assche 

Also Reviewed-by: Omar Sandoval 

Re: [PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit

[Bart Van Assche]

On Wed, 2018-01-10 at 11:35 -0700, Jens Axboe wrote:
> On 1/10/18 11:33 AM, Bart Van Assche wrote:
> > On Wed, 2018-01-10 at 11:32 -0700, Jens Axboe wrote:
> > > On 1/10/18 11:29 AM, Bart Van Assche wrote:
> > > > On Tue, 2018-01-09 at 17:29 -0700, Jens Axboe wrote:
> > > > > @@ -313,8 +307,6 @@ int __blk_mq_debugfs_rq_show(struct seq_file *m, 
> > > > > struct request *rq)
> > > > >   seq_puts(m, ", .rq_flags=");
> > > > >   blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
> > > > >  ARRAY_SIZE(rqf_name));
> > > > > - seq_puts(m, ", .atomic_flags=");
> > > > > - blk_flags_show(m, rq->atomic_flags, rqaf_name, 
> > > > > ARRAY_SIZE(rqaf_name));
> > > > >   seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
> > > > >  rq->internal_tag);
> > > > >   if (mq_ops->show_rq)
> > > > 
> > > > Whether or not a request has been marked complete is very useful to 
> > > > know. Have you
> > > > considered to show the return value of blk_rq_is_complete() in the 
> > > > debugfs output?
> > > 
> > > Yeah, that's a good point. Let me add that in lieu of the atomic flags 
> > > that
> > > are being killed. Are you fine with the patch then?
> > 
> > The rest of the patch looks fine to me. This is the only comment I had 
> > about this patch.
> 
> http://git.kernel.dk/cgit/linux-block/commit/?h=blk-kill-atomic-flags&id=3d34009082f5e72137d6bb38cbc2ff6047f03fd1
> 
> Added a complete= entry for it.

For that patch: Reviewed-by: Bart Van Assche 

Thanks,

Bart.

Re: [PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit

[Jens Axboe]

On 1/10/18 11:33 AM, Bart Van Assche wrote:
> On Wed, 2018-01-10 at 11:32 -0700, Jens Axboe wrote:
>> On 1/10/18 11:29 AM, Bart Van Assche wrote:
>>> On Tue, 2018-01-09 at 17:29 -0700, Jens Axboe wrote:
 @@ -313,8 +307,6 @@ int __blk_mq_debugfs_rq_show(struct seq_file *m, 
 struct request *rq)
seq_puts(m, ", .rq_flags=");
blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
   ARRAY_SIZE(rqf_name));
 -  seq_puts(m, ", .atomic_flags=");
 -  blk_flags_show(m, rq->atomic_flags, rqaf_name, ARRAY_SIZE(rqaf_name));
seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
   rq->internal_tag);
if (mq_ops->show_rq)
>>>
>>> Whether or not a request has been marked complete is very useful to know. 
>>> Have you
>>> considered to show the return value of blk_rq_is_complete() in the debugfs 
>>> output?
>>
>> Yeah, that's a good point. Let me add that in lieu of the atomic flags that
>> are being killed. Are you fine with the patch then?
> 
> The rest of the patch looks fine to me. This is the only comment I had about 
> this patch.

http://git.kernel.dk/cgit/linux-block/commit/?h=blk-kill-atomic-flags&id=3d34009082f5e72137d6bb38cbc2ff6047f03fd1

Added a complete= entry for it.

-- 
Jens Axboe

Re: [PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit

[Bart Van Assche]

On Wed, 2018-01-10 at 11:32 -0700, Jens Axboe wrote:
> On 1/10/18 11:29 AM, Bart Van Assche wrote:
> > On Tue, 2018-01-09 at 17:29 -0700, Jens Axboe wrote:
> > > @@ -313,8 +307,6 @@ int __blk_mq_debugfs_rq_show(struct seq_file *m, 
> > > struct request *rq)
> > >   seq_puts(m, ", .rq_flags=");
> > >   blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
> > >  ARRAY_SIZE(rqf_name));
> > > - seq_puts(m, ", .atomic_flags=");
> > > - blk_flags_show(m, rq->atomic_flags, rqaf_name, ARRAY_SIZE(rqaf_name));
> > >   seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
> > >  rq->internal_tag);
> > >   if (mq_ops->show_rq)
> > 
> > Whether or not a request has been marked complete is very useful to know. 
> > Have you
> > considered to show the return value of blk_rq_is_complete() in the debugfs 
> > output?
> 
> Yeah, that's a good point. Let me add that in lieu of the atomic flags that
> are being killed. Are you fine with the patch then?

The rest of the patch looks fine to me. This is the only comment I had about 
this patch.

Bart.

Re: [PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit

[Jens Axboe]

On 1/10/18 11:29 AM, Bart Van Assche wrote:
> On Tue, 2018-01-09 at 17:29 -0700, Jens Axboe wrote:
>> @@ -313,8 +307,6 @@ int __blk_mq_debugfs_rq_show(struct seq_file *m, struct 
>> request *rq)
>>  seq_puts(m, ", .rq_flags=");
>>  blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
>> ARRAY_SIZE(rqf_name));
>> -seq_puts(m, ", .atomic_flags=");
>> -blk_flags_show(m, rq->atomic_flags, rqaf_name, ARRAY_SIZE(rqaf_name));
>>  seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
>> rq->internal_tag);
>>  if (mq_ops->show_rq)
> 
> Whether or not a request has been marked complete is very useful to know. 
> Have you
> considered to show the return value of blk_rq_is_complete() in the debugfs 
> output?

Yeah, that's a good point. Let me add that in lieu of the atomic flags that
are being killed. Are you fine with the patch then?

-- 
Jens Axboe

Re: [PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit

[Bart Van Assche]

On Tue, 2018-01-09 at 17:29 -0700, Jens Axboe wrote:
> @@ -313,8 +307,6 @@ int __blk_mq_debugfs_rq_show(struct seq_file *m, struct 
> request *rq)
>   seq_puts(m, ", .rq_flags=");
>   blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
>  ARRAY_SIZE(rqf_name));
> - seq_puts(m, ", .atomic_flags=");
> - blk_flags_show(m, rq->atomic_flags, rqaf_name, ARRAY_SIZE(rqaf_name));
>   seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
>  rq->internal_tag);
>   if (mq_ops->show_rq)

Whether or not a request has been marked complete is very useful to know. Have 
you
considered to show the return value of blk_rq_is_complete() in the debugfs 
output?

Thanks,

Bart.

[PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit

[Jens Axboe]

We only have one atomic flag left. Instead of using an entire
unsigned long for that, steal the bottom bit of the deadline
field that we already reserved.

Remove ->atomic_flags, since it's now unused.

Signed-off-by: Jens Axboe 
---
 block/blk-core.c   |  2 +-
 block/blk-mq-debugfs.c |  8 
 block/blk-mq.c |  2 +-
 block/blk.h| 19 +--
 include/linux/blkdev.h |  2 --
 5 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index f843ae4f858d..7ba607527487 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2853,7 +2853,7 @@ void blk_start_request(struct request *req)
wbt_issue(req->q->rq_wb, &req->issue_stat);
}
 
-   BUG_ON(test_bit(REQ_ATOM_COMPLETE, &req->atomic_flags));
+   BUG_ON(blk_rq_is_complete(req));
blk_add_timer(req);
 }
 EXPORT_SYMBOL(blk_start_request);
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 2a9c9f8b6162..ac99b78415ec 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -291,12 +291,6 @@ static const char *const rqf_name[] = {
 };
 #undef RQF_NAME
 
-#define RQAF_NAME(name) [REQ_ATOM_##name] = #name
-static const char *const rqaf_name[] = {
-   RQAF_NAME(COMPLETE),
-};
-#undef RQAF_NAME
-
 int __blk_mq_debugfs_rq_show(struct seq_file *m, struct request *rq)
 {
const struct blk_mq_ops *const mq_ops = rq->q->mq_ops;
@@ -313,8 +307,6 @@ int __blk_mq_debugfs_rq_show(struct seq_file *m, struct 
request *rq)
seq_puts(m, ", .rq_flags=");
blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
   ARRAY_SIZE(rqf_name));
-   seq_puts(m, ", .atomic_flags=");
-   blk_flags_show(m, rq->atomic_flags, rqaf_name, ARRAY_SIZE(rqaf_name));
seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
   rq->internal_tag);
if (mq_ops->show_rq)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index d875c51bcff8..7248ee043651 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -294,7 +294,6 @@ static struct request *blk_mq_rq_ctx_init(struct 
blk_mq_alloc_data *data,
rq->rq_flags |= RQF_PREEMPT;
if (blk_queue_io_stat(data->q))
rq->rq_flags |= RQF_IO_STAT;
-   /* do not touch atomic flags, it needs atomic ops against the timer */
rq->cpu = -1;
INIT_HLIST_NODE(&rq->hash);
RB_CLEAR_NODE(&rq->rb_node);
@@ -313,6 +312,7 @@ static struct request *blk_mq_rq_ctx_init(struct 
blk_mq_alloc_data *data,
rq->special = NULL;
/* tag was already set */
rq->extra_len = 0;
+   rq->__deadline = 0;
 
INIT_LIST_HEAD(&rq->timeout_list);
rq->timeout = 0;
diff --git a/block/blk.h b/block/blk.h
index bcd9cf7db0d4..c84ae0e21ebd 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -120,24 +120,23 @@ void blk_account_io_completion(struct request *req, 
unsigned int bytes);
 void blk_account_io_done(struct request *req);
 
 /*
- * Internal atomic flags for request handling
- */
-enum rq_atomic_flags {
-   REQ_ATOM_COMPLETE = 0,
-};
-
-/*
  * EH timer and IO completion will both attempt to 'grab' the request, make
- * sure that only one of them succeeds
+ * sure that only one of them succeeds. Steal the bottom bit of the
+ * __deadline field for this.
  */
 static inline int blk_mark_rq_complete(struct request *rq)
 {
-   return test_and_set_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
+   return test_and_set_bit(0, &rq->__deadline);
 }
 
 static inline void blk_clear_rq_complete(struct request *rq)
 {
-   clear_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
+   clear_bit(0, &rq->__deadline);
+}
+
+static inline bool blk_rq_is_complete(struct request *rq)
+{
+   return test_bit(0, &rq->__deadline);
 }
 
 /*
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index aa6698cf483c..d4b2f7bb18d6 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -156,8 +156,6 @@ struct request {
 
int internal_tag;
 
-   unsigned long atomic_flags;
-
/* the following two fields are internal, NEVER access directly */
unsigned int __data_len;/* total data len */
int tag;
-- 
2.7.4

Re: [PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit

[Jens Axboe]

On 1/9/18 11:44 AM, Jens Axboe wrote:
> On 1/9/18 11:43 AM, Bart Van Assche wrote:
>> On Tue, 2018-01-09 at 11:27 -0700, Jens Axboe wrote:
>>>  static inline int blk_mark_rq_complete(struct request *rq)
>>>  {
>>> -   return test_and_set_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
>>> +   return test_and_set_bit(0, &rq->__deadline);
>>>  }
>>>  
>>>  static inline void blk_clear_rq_complete(struct request *rq)
>>>  {
>>> -   clear_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
>>> +   clear_bit(0, &rq->__deadline);
>>> +}
>>> +
>>> +static inline bool blk_rq_is_complete(struct request *rq)
>>> +{
>>> +   return test_bit(0, &rq->__deadline);
>>>  }
>>
>> Hello Jens,
>>
>> With this change setting or changing the deadline clears the COMPLETE flag.
>> Is that the intended behavior? If so, should perhaps a comment be added above
>> blk_rq_set_deadline()?
> 
> Yeah, it's intentional. I can add a comment to that effect. It's only done
> before queueing - except for the case where we force a timeout, but for that
> it's only on the blk-mq side, which doesn't care.

Since we clear it when we init the request, we could also just leave the
bit intact when setting the deadline. That's probably the safer choice:

static inline void blk_rq_set_deadline(struct request *rq, unsigned long time)  
{   
rq->__deadline = (time & ~0x1UL) | (rq->__deadline & 0x1UL);
}

I'll test that, previous testing didn't find anything wrong with clearing
the bit, but this does seem safer.

-- 
Jens Axboe

Re: [PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit

[Jens Axboe]

On 1/9/18 11:43 AM, Bart Van Assche wrote:
> On Tue, 2018-01-09 at 11:27 -0700, Jens Axboe wrote:
>>  static inline int blk_mark_rq_complete(struct request *rq)
>>  {
>> -return test_and_set_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
>> +return test_and_set_bit(0, &rq->__deadline);
>>  }
>>  
>>  static inline void blk_clear_rq_complete(struct request *rq)
>>  {
>> -clear_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
>> +clear_bit(0, &rq->__deadline);
>> +}
>> +
>> +static inline bool blk_rq_is_complete(struct request *rq)
>> +{
>> +return test_bit(0, &rq->__deadline);
>>  }
> 
> Hello Jens,
> 
> With this change setting or changing the deadline clears the COMPLETE flag.
> Is that the intended behavior? If so, should perhaps a comment be added above
> blk_rq_set_deadline()?

Yeah, it's intentional. I can add a comment to that effect. It's only done
before queueing - except for the case where we force a timeout, but for that
it's only on the blk-mq side, which doesn't care.

-- 
Jens Axboe

Re: [PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit

[Bart Van Assche]

On Tue, 2018-01-09 at 11:27 -0700, Jens Axboe wrote:
>  static inline int blk_mark_rq_complete(struct request *rq)
>  {
> - return test_and_set_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
> + return test_and_set_bit(0, &rq->__deadline);
>  }
>  
>  static inline void blk_clear_rq_complete(struct request *rq)
>  {
> - clear_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
> + clear_bit(0, &rq->__deadline);
> +}
> +
> +static inline bool blk_rq_is_complete(struct request *rq)
> +{
> + return test_bit(0, &rq->__deadline);
>  }

Hello Jens,

With this change setting or changing the deadline clears the COMPLETE flag.
Is that the intended behavior? If so, should perhaps a comment be added above
blk_rq_set_deadline()?

Thanks,

Bart.

[PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit

[Jens Axboe]

We only have one atomic flag left. Instead of using an entire
unsigned long for that, steal the bottom bit of the deadline
field that we already reserved.

Remove ->atomic_flags, since it's now unused.

Signed-off-by: Jens Axboe 
---
 block/blk-core.c   |  2 +-
 block/blk-mq-debugfs.c |  8 
 block/blk.h| 19 +--
 include/linux/blkdev.h |  2 --
 4 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index f843ae4f858d..7ba607527487 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2853,7 +2853,7 @@ void blk_start_request(struct request *req)
wbt_issue(req->q->rq_wb, &req->issue_stat);
}
 
-   BUG_ON(test_bit(REQ_ATOM_COMPLETE, &req->atomic_flags));
+   BUG_ON(blk_rq_is_complete(req));
blk_add_timer(req);
 }
 EXPORT_SYMBOL(blk_start_request);
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 2a9c9f8b6162..ac99b78415ec 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -291,12 +291,6 @@ static const char *const rqf_name[] = {
 };
 #undef RQF_NAME
 
-#define RQAF_NAME(name) [REQ_ATOM_##name] = #name
-static const char *const rqaf_name[] = {
-   RQAF_NAME(COMPLETE),
-};
-#undef RQAF_NAME
-
 int __blk_mq_debugfs_rq_show(struct seq_file *m, struct request *rq)
 {
const struct blk_mq_ops *const mq_ops = rq->q->mq_ops;
@@ -313,8 +307,6 @@ int __blk_mq_debugfs_rq_show(struct seq_file *m, struct 
request *rq)
seq_puts(m, ", .rq_flags=");
blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
   ARRAY_SIZE(rqf_name));
-   seq_puts(m, ", .atomic_flags=");
-   blk_flags_show(m, rq->atomic_flags, rqaf_name, ARRAY_SIZE(rqaf_name));
seq_printf(m, ", .tag=%d, .internal_tag=%d", rq->tag,
   rq->internal_tag);
if (mq_ops->show_rq)
diff --git a/block/blk.h b/block/blk.h
index 8b26a8872e05..d5ae07cc4abb 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -120,24 +120,23 @@ void blk_account_io_completion(struct request *req, 
unsigned int bytes);
 void blk_account_io_done(struct request *req);
 
 /*
- * Internal atomic flags for request handling
- */
-enum rq_atomic_flags {
-   REQ_ATOM_COMPLETE = 0,
-};
-
-/*
  * EH timer and IO completion will both attempt to 'grab' the request, make
- * sure that only one of them succeeds
+ * sure that only one of them succeeds. Steal the bottom bit of the
+ * __deadline field for this.
  */
 static inline int blk_mark_rq_complete(struct request *rq)
 {
-   return test_and_set_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
+   return test_and_set_bit(0, &rq->__deadline);
 }
 
 static inline void blk_clear_rq_complete(struct request *rq)
 {
-   clear_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
+   clear_bit(0, &rq->__deadline);
+}
+
+static inline bool blk_rq_is_complete(struct request *rq)
+{
+   return test_bit(0, &rq->__deadline);
 }
 
 /*
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index aa6698cf483c..d4b2f7bb18d6 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -156,8 +156,6 @@ struct request {
 
int internal_tag;
 
-   unsigned long atomic_flags;
-
/* the following two fields are internal, NEVER access directly */
unsigned int __data_len;/* total data len */
int tag;
-- 
2.7.4