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=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=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=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.

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, >atomic_flags);
>>> +   return test_and_set_bit(0, >__deadline);
>>>  }
>>>  
>>>  static inline void blk_clear_rq_complete(struct request *rq)
>>>  {
>>> -   clear_bit(REQ_ATOM_COMPLETE, >atomic_flags);
>>> +   clear_bit(0, >__deadline);
>>> +}
>>> +
>>> +static inline bool blk_rq_is_complete(struct request *rq)
>>> +{
>>> +   return test_bit(0, >__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