Re: [PATCH 3/4] block: convert REQ_ATOM_COMPLETE to stealing rq->__deadline bit
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
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
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
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
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
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
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
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
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.