| Message ID | 1544152185-32667-1-git-send-email-jianchao.w.wang@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | blk-mq: refactor code of issue directly | expand |
On 12/6/18 8:09 PM, Jianchao Wang wrote: > Hi Jens > > Please consider this patchset for 4.21. > > It refactors the code of issue request directly to unify the interface > and make the code clearer and more readable. > > This patch set is rebased on the recent for-4.21/block and add the 1st > patch which inserts the non-read-write request to hctx dispatch > list to avoid to involve merge and io scheduler when bypass_insert > is true, otherwise, inserting is ignored, BLK_STS_RESOURCE is returned > and the caller will fail forever. > > The 2nd patch refactors the code of issue request directly to unify the > helper interface which could handle all the cases. > > The 3rd patch make blk_mq_sched_insert_requests issue requests directly > with 'bypass' false, then it needn't to handle the non-issued requests > any more. > > The 4th patch replace and kill the blk_mq_request_issue_directly. Sorry to keep iterating on this, but let's default to inserting to the dispatch list if we ever see busy from a direct dispatch. I'm fine with doing that for 4.21, as suggested by Ming, I just didn't want to fiddle with it for 4.20. This will prevent any merging on the request going forward, which I think is a much safer default. You do this already for some cases. Let's do it unconditionally for a request that was ever subjected to ->queue_rq() and we didn't either error or finish after the fact.
On 12/7/18 11:16 AM, Jens Axboe wrote: > On 12/6/18 8:09 PM, Jianchao Wang wrote: >> Hi Jens >> >> Please consider this patchset for 4.21. >> >> It refactors the code of issue request directly to unify the interface >> and make the code clearer and more readable. >> >> This patch set is rebased on the recent for-4.21/block and add the 1st >> patch which inserts the non-read-write request to hctx dispatch >> list to avoid to involve merge and io scheduler when bypass_insert >> is true, otherwise, inserting is ignored, BLK_STS_RESOURCE is returned >> and the caller will fail forever. >> >> The 2nd patch refactors the code of issue request directly to unify the >> helper interface which could handle all the cases. >> >> The 3rd patch make blk_mq_sched_insert_requests issue requests directly >> with 'bypass' false, then it needn't to handle the non-issued requests >> any more. >> >> The 4th patch replace and kill the blk_mq_request_issue_directly. > > Sorry to keep iterating on this, but let's default to inserting to > the dispatch list if we ever see busy from a direct dispatch. I'm fine > with doing that for 4.21, as suggested by Ming, I just didn't want to > fiddle with it for 4.20. This will prevent any merging on the request > going forward, which I think is a much safer default. > > You do this already for some cases. Let's do it unconditionally for > a request that was ever subjected to ->queue_rq() and we didn't either > error or finish after the fact. > I have done it in this version if I get your point correctly. Please refer to the following fragment in the 2nd patch. + /* + * If the request is issued unsuccessfully with + * BLK_STS_DEV_RESOURCE or BLK_STS_RESOURCE, insert + * the request to hctx dispatch list due to attached + * lldd resource. + */ + force = true; + ret = __blk_mq_issue_directly(hctx, rq, cookie, last); +out_unlock: + hctx_unlock(hctx, srcu_idx); +out: + switch (ret) { + case BLK_STS_OK: + break; + case BLK_STS_DEV_RESOURCE: + case BLK_STS_RESOURCE: + if (force) { + blk_mq_request_bypass_insert(rq, run_queue); + ret = bypass ? BLK_STS_OK : ret; + } else if (!bypass) { + blk_mq_sched_insert_request(rq, false, + run_queue, false); + } + break; + default: Thanks Jianchao
On 12/6/18 8:26 PM, jianchao.wang wrote: > > > On 12/7/18 11:16 AM, Jens Axboe wrote: >> On 12/6/18 8:09 PM, Jianchao Wang wrote: >>> Hi Jens >>> >>> Please consider this patchset for 4.21. >>> >>> It refactors the code of issue request directly to unify the interface >>> and make the code clearer and more readable. >>> >>> This patch set is rebased on the recent for-4.21/block and add the 1st >>> patch which inserts the non-read-write request to hctx dispatch >>> list to avoid to involve merge and io scheduler when bypass_insert >>> is true, otherwise, inserting is ignored, BLK_STS_RESOURCE is returned >>> and the caller will fail forever. >>> >>> The 2nd patch refactors the code of issue request directly to unify the >>> helper interface which could handle all the cases. >>> >>> The 3rd patch make blk_mq_sched_insert_requests issue requests directly >>> with 'bypass' false, then it needn't to handle the non-issued requests >>> any more. >>> >>> The 4th patch replace and kill the blk_mq_request_issue_directly. >> >> Sorry to keep iterating on this, but let's default to inserting to >> the dispatch list if we ever see busy from a direct dispatch. I'm fine >> with doing that for 4.21, as suggested by Ming, I just didn't want to >> fiddle with it for 4.20. This will prevent any merging on the request >> going forward, which I think is a much safer default. >> >> You do this already for some cases. Let's do it unconditionally for >> a request that was ever subjected to ->queue_rq() and we didn't either >> error or finish after the fact. >> > I have done it in this version if I get your point correctly. > Please refer to the following fragment in the 2nd patch. > > + /* > + * If the request is issued unsuccessfully with > + * BLK_STS_DEV_RESOURCE or BLK_STS_RESOURCE, insert > + * the request to hctx dispatch list due to attached > + * lldd resource. > + */ > + force = true; > + ret = __blk_mq_issue_directly(hctx, rq, cookie, last); > +out_unlock: > + hctx_unlock(hctx, srcu_idx); > +out: > + switch (ret) { > + case BLK_STS_OK: > + break; > + case BLK_STS_DEV_RESOURCE: > + case BLK_STS_RESOURCE: > + if (force) { > + blk_mq_request_bypass_insert(rq, run_queue); > + ret = bypass ? BLK_STS_OK : ret; > + } else if (!bypass) { > + blk_mq_sched_insert_request(rq, false, > + run_queue, false); > + } > + break; > + default: You are right, I missed that you set force = true before doing the issue. So this looks good to me!
On 12/6/18 8:32 PM, Jens Axboe wrote: > On 12/6/18 8:26 PM, jianchao.wang wrote: >> >> >> On 12/7/18 11:16 AM, Jens Axboe wrote: >>> On 12/6/18 8:09 PM, Jianchao Wang wrote: >>>> Hi Jens >>>> >>>> Please consider this patchset for 4.21. >>>> >>>> It refactors the code of issue request directly to unify the interface >>>> and make the code clearer and more readable. >>>> >>>> This patch set is rebased on the recent for-4.21/block and add the 1st >>>> patch which inserts the non-read-write request to hctx dispatch >>>> list to avoid to involve merge and io scheduler when bypass_insert >>>> is true, otherwise, inserting is ignored, BLK_STS_RESOURCE is returned >>>> and the caller will fail forever. >>>> >>>> The 2nd patch refactors the code of issue request directly to unify the >>>> helper interface which could handle all the cases. >>>> >>>> The 3rd patch make blk_mq_sched_insert_requests issue requests directly >>>> with 'bypass' false, then it needn't to handle the non-issued requests >>>> any more. >>>> >>>> The 4th patch replace and kill the blk_mq_request_issue_directly. >>> >>> Sorry to keep iterating on this, but let's default to inserting to >>> the dispatch list if we ever see busy from a direct dispatch. I'm fine >>> with doing that for 4.21, as suggested by Ming, I just didn't want to >>> fiddle with it for 4.20. This will prevent any merging on the request >>> going forward, which I think is a much safer default. >>> >>> You do this already for some cases. Let's do it unconditionally for >>> a request that was ever subjected to ->queue_rq() and we didn't either >>> error or finish after the fact. >>> >> I have done it in this version if I get your point correctly. >> Please refer to the following fragment in the 2nd patch. >> >> + /* >> + * If the request is issued unsuccessfully with >> + * BLK_STS_DEV_RESOURCE or BLK_STS_RESOURCE, insert >> + * the request to hctx dispatch list due to attached >> + * lldd resource. >> + */ >> + force = true; >> + ret = __blk_mq_issue_directly(hctx, rq, cookie, last); >> +out_unlock: >> + hctx_unlock(hctx, srcu_idx); >> +out: >> + switch (ret) { >> + case BLK_STS_OK: >> + break; >> + case BLK_STS_DEV_RESOURCE: >> + case BLK_STS_RESOURCE: >> + if (force) { >> + blk_mq_request_bypass_insert(rq, run_queue); >> + ret = bypass ? BLK_STS_OK : ret; >> + } else if (!bypass) { >> + blk_mq_sched_insert_request(rq, false, >> + run_queue, false); >> + } >> + break; >> + default: > > You are right, I missed that you set force = true before doing the > issue. So this looks good to me! I applied your series. With this, we should be good to remove the REQ_NOMERGE logic that was added for the corruption case, and the blk_rq_can_direct_dispatch() as well?
On 12/7/18 11:34 AM, Jens Axboe wrote: > On 12/6/18 8:32 PM, Jens Axboe wrote: >> On 12/6/18 8:26 PM, jianchao.wang wrote: >>> >>> >>> On 12/7/18 11:16 AM, Jens Axboe wrote: >>>> On 12/6/18 8:09 PM, Jianchao Wang wrote: >>>>> Hi Jens >>>>> >>>>> Please consider this patchset for 4.21. >>>>> >>>>> It refactors the code of issue request directly to unify the interface >>>>> and make the code clearer and more readable. >>>>> >>>>> This patch set is rebased on the recent for-4.21/block and add the 1st >>>>> patch which inserts the non-read-write request to hctx dispatch >>>>> list to avoid to involve merge and io scheduler when bypass_insert >>>>> is true, otherwise, inserting is ignored, BLK_STS_RESOURCE is returned >>>>> and the caller will fail forever. >>>>> >>>>> The 2nd patch refactors the code of issue request directly to unify the >>>>> helper interface which could handle all the cases. >>>>> >>>>> The 3rd patch make blk_mq_sched_insert_requests issue requests directly >>>>> with 'bypass' false, then it needn't to handle the non-issued requests >>>>> any more. >>>>> >>>>> The 4th patch replace and kill the blk_mq_request_issue_directly. >>>> >>>> Sorry to keep iterating on this, but let's default to inserting to >>>> the dispatch list if we ever see busy from a direct dispatch. I'm fine >>>> with doing that for 4.21, as suggested by Ming, I just didn't want to >>>> fiddle with it for 4.20. This will prevent any merging on the request >>>> going forward, which I think is a much safer default. >>>> >>>> You do this already for some cases. Let's do it unconditionally for >>>> a request that was ever subjected to ->queue_rq() and we didn't either >>>> error or finish after the fact. >>>> >>> I have done it in this version if I get your point correctly. >>> Please refer to the following fragment in the 2nd patch. >>> >>> + /* >>> + * If the request is issued unsuccessfully with >>> + * BLK_STS_DEV_RESOURCE or BLK_STS_RESOURCE, insert >>> + * the request to hctx dispatch list due to attached >>> + * lldd resource. >>> + */ >>> + force = true; >>> + ret = __blk_mq_issue_directly(hctx, rq, cookie, last); >>> +out_unlock: >>> + hctx_unlock(hctx, srcu_idx); >>> +out: >>> + switch (ret) { >>> + case BLK_STS_OK: >>> + break; >>> + case BLK_STS_DEV_RESOURCE: >>> + case BLK_STS_RESOURCE: >>> + if (force) { >>> + blk_mq_request_bypass_insert(rq, run_queue); >>> + ret = bypass ? BLK_STS_OK : ret; >>> + } else if (!bypass) { >>> + blk_mq_sched_insert_request(rq, false, >>> + run_queue, false); >>> + } >>> + break; >>> + default: >> >> You are right, I missed that you set force = true before doing the >> issue. So this looks good to me! > > I applied your series. With this, we should be good to remove the > REQ_NOMERGE logic that was added for the corruption case, and the > blk_rq_can_direct_dispatch() as well? > Yes, it should be that. Every thing rejected by .queue_rq is ended or inserted into hctx dispatch list. And also direct-issue path is unified with normal path. Thanks Jianchao
On 12/6/18 8:41 PM, jianchao.wang wrote: > > > On 12/7/18 11:34 AM, Jens Axboe wrote: >> On 12/6/18 8:32 PM, Jens Axboe wrote: >>> On 12/6/18 8:26 PM, jianchao.wang wrote: >>>> >>>> >>>> On 12/7/18 11:16 AM, Jens Axboe wrote: >>>>> On 12/6/18 8:09 PM, Jianchao Wang wrote: >>>>>> Hi Jens >>>>>> >>>>>> Please consider this patchset for 4.21. >>>>>> >>>>>> It refactors the code of issue request directly to unify the interface >>>>>> and make the code clearer and more readable. >>>>>> >>>>>> This patch set is rebased on the recent for-4.21/block and add the 1st >>>>>> patch which inserts the non-read-write request to hctx dispatch >>>>>> list to avoid to involve merge and io scheduler when bypass_insert >>>>>> is true, otherwise, inserting is ignored, BLK_STS_RESOURCE is returned >>>>>> and the caller will fail forever. >>>>>> >>>>>> The 2nd patch refactors the code of issue request directly to unify the >>>>>> helper interface which could handle all the cases. >>>>>> >>>>>> The 3rd patch make blk_mq_sched_insert_requests issue requests directly >>>>>> with 'bypass' false, then it needn't to handle the non-issued requests >>>>>> any more. >>>>>> >>>>>> The 4th patch replace and kill the blk_mq_request_issue_directly. >>>>> >>>>> Sorry to keep iterating on this, but let's default to inserting to >>>>> the dispatch list if we ever see busy from a direct dispatch. I'm fine >>>>> with doing that for 4.21, as suggested by Ming, I just didn't want to >>>>> fiddle with it for 4.20. This will prevent any merging on the request >>>>> going forward, which I think is a much safer default. >>>>> >>>>> You do this already for some cases. Let's do it unconditionally for >>>>> a request that was ever subjected to ->queue_rq() and we didn't either >>>>> error or finish after the fact. >>>>> >>>> I have done it in this version if I get your point correctly. >>>> Please refer to the following fragment in the 2nd patch. >>>> >>>> + /* >>>> + * If the request is issued unsuccessfully with >>>> + * BLK_STS_DEV_RESOURCE or BLK_STS_RESOURCE, insert >>>> + * the request to hctx dispatch list due to attached >>>> + * lldd resource. >>>> + */ >>>> + force = true; >>>> + ret = __blk_mq_issue_directly(hctx, rq, cookie, last); >>>> +out_unlock: >>>> + hctx_unlock(hctx, srcu_idx); >>>> +out: >>>> + switch (ret) { >>>> + case BLK_STS_OK: >>>> + break; >>>> + case BLK_STS_DEV_RESOURCE: >>>> + case BLK_STS_RESOURCE: >>>> + if (force) { >>>> + blk_mq_request_bypass_insert(rq, run_queue); >>>> + ret = bypass ? BLK_STS_OK : ret; >>>> + } else if (!bypass) { >>>> + blk_mq_sched_insert_request(rq, false, >>>> + run_queue, false); >>>> + } >>>> + break; >>>> + default: >>> >>> You are right, I missed that you set force = true before doing the >>> issue. So this looks good to me! >> >> I applied your series. With this, we should be good to remove the >> REQ_NOMERGE logic that was added for the corruption case, and the >> blk_rq_can_direct_dispatch() as well? >> > > Yes, it should be that. > Every thing rejected by .queue_rq is ended or inserted into hctx dispatch > list. And also direct-issue path is unified with normal path. Why are we doing that return value dance, depending on whether this is a bypass insert or not? That seems confusing.
On 12/7/18 11:42 AM, Jens Axboe wrote: > On 12/6/18 8:41 PM, jianchao.wang wrote: >> >> >> On 12/7/18 11:34 AM, Jens Axboe wrote: >>> On 12/6/18 8:32 PM, Jens Axboe wrote: >>>> On 12/6/18 8:26 PM, jianchao.wang wrote: >>>>> >>>>> >>>>> On 12/7/18 11:16 AM, Jens Axboe wrote: >>>>>> On 12/6/18 8:09 PM, Jianchao Wang wrote: >>>>>>> Hi Jens >>>>>>> >>>>>>> Please consider this patchset for 4.21. >>>>>>> >>>>>>> It refactors the code of issue request directly to unify the interface >>>>>>> and make the code clearer and more readable. >>>>>>> >>>>>>> This patch set is rebased on the recent for-4.21/block and add the 1st >>>>>>> patch which inserts the non-read-write request to hctx dispatch >>>>>>> list to avoid to involve merge and io scheduler when bypass_insert >>>>>>> is true, otherwise, inserting is ignored, BLK_STS_RESOURCE is returned >>>>>>> and the caller will fail forever. >>>>>>> >>>>>>> The 2nd patch refactors the code of issue request directly to unify the >>>>>>> helper interface which could handle all the cases. >>>>>>> >>>>>>> The 3rd patch make blk_mq_sched_insert_requests issue requests directly >>>>>>> with 'bypass' false, then it needn't to handle the non-issued requests >>>>>>> any more. >>>>>>> >>>>>>> The 4th patch replace and kill the blk_mq_request_issue_directly. >>>>>> >>>>>> Sorry to keep iterating on this, but let's default to inserting to >>>>>> the dispatch list if we ever see busy from a direct dispatch. I'm fine >>>>>> with doing that for 4.21, as suggested by Ming, I just didn't want to >>>>>> fiddle with it for 4.20. This will prevent any merging on the request >>>>>> going forward, which I think is a much safer default. >>>>>> >>>>>> You do this already for some cases. Let's do it unconditionally for >>>>>> a request that was ever subjected to ->queue_rq() and we didn't either >>>>>> error or finish after the fact. >>>>>> >>>>> I have done it in this version if I get your point correctly. >>>>> Please refer to the following fragment in the 2nd patch. >>>>> >>>>> + /* >>>>> + * If the request is issued unsuccessfully with >>>>> + * BLK_STS_DEV_RESOURCE or BLK_STS_RESOURCE, insert >>>>> + * the request to hctx dispatch list due to attached >>>>> + * lldd resource. >>>>> + */ >>>>> + force = true; >>>>> + ret = __blk_mq_issue_directly(hctx, rq, cookie, last); >>>>> +out_unlock: >>>>> + hctx_unlock(hctx, srcu_idx); >>>>> +out: >>>>> + switch (ret) { >>>>> + case BLK_STS_OK: >>>>> + break; >>>>> + case BLK_STS_DEV_RESOURCE: >>>>> + case BLK_STS_RESOURCE: >>>>> + if (force) { >>>>> + blk_mq_request_bypass_insert(rq, run_queue); >>>>> + ret = bypass ? BLK_STS_OK : ret; >>>>> + } else if (!bypass) { >>>>> + blk_mq_sched_insert_request(rq, false, >>>>> + run_queue, false); >>>>> + } >>>>> + break; >>>>> + default: >>>> >>>> You are right, I missed that you set force = true before doing the >>>> issue. So this looks good to me! >>> >>> I applied your series. With this, we should be good to remove the >>> REQ_NOMERGE logic that was added for the corruption case, and the >>> blk_rq_can_direct_dispatch() as well? >>> >> >> Yes, it should be that. >> Every thing rejected by .queue_rq is ended or inserted into hctx dispatch >> list. And also direct-issue path is unified with normal path. > > Why are we doing that return value dance, depending on whether this > is a bypass insert or not? That seems confusing. > For the 'bypass == false' case, it need to know whether the request is issued successfully. This is for the 3rd patch. I used to use the returned cookie to identify the result, but you don't like it. So I have to use this return value. Thanks Jianchao
On 12/6/18 8:46 PM, jianchao.wang wrote: > > > On 12/7/18 11:42 AM, Jens Axboe wrote: >> On 12/6/18 8:41 PM, jianchao.wang wrote: >>> >>> >>> On 12/7/18 11:34 AM, Jens Axboe wrote: >>>> On 12/6/18 8:32 PM, Jens Axboe wrote: >>>>> On 12/6/18 8:26 PM, jianchao.wang wrote: >>>>>> >>>>>> >>>>>> On 12/7/18 11:16 AM, Jens Axboe wrote: >>>>>>> On 12/6/18 8:09 PM, Jianchao Wang wrote: >>>>>>>> Hi Jens >>>>>>>> >>>>>>>> Please consider this patchset for 4.21. >>>>>>>> >>>>>>>> It refactors the code of issue request directly to unify the interface >>>>>>>> and make the code clearer and more readable. >>>>>>>> >>>>>>>> This patch set is rebased on the recent for-4.21/block and add the 1st >>>>>>>> patch which inserts the non-read-write request to hctx dispatch >>>>>>>> list to avoid to involve merge and io scheduler when bypass_insert >>>>>>>> is true, otherwise, inserting is ignored, BLK_STS_RESOURCE is returned >>>>>>>> and the caller will fail forever. >>>>>>>> >>>>>>>> The 2nd patch refactors the code of issue request directly to unify the >>>>>>>> helper interface which could handle all the cases. >>>>>>>> >>>>>>>> The 3rd patch make blk_mq_sched_insert_requests issue requests directly >>>>>>>> with 'bypass' false, then it needn't to handle the non-issued requests >>>>>>>> any more. >>>>>>>> >>>>>>>> The 4th patch replace and kill the blk_mq_request_issue_directly. >>>>>>> >>>>>>> Sorry to keep iterating on this, but let's default to inserting to >>>>>>> the dispatch list if we ever see busy from a direct dispatch. I'm fine >>>>>>> with doing that for 4.21, as suggested by Ming, I just didn't want to >>>>>>> fiddle with it for 4.20. This will prevent any merging on the request >>>>>>> going forward, which I think is a much safer default. >>>>>>> >>>>>>> You do this already for some cases. Let's do it unconditionally for >>>>>>> a request that was ever subjected to ->queue_rq() and we didn't either >>>>>>> error or finish after the fact. >>>>>>> >>>>>> I have done it in this version if I get your point correctly. >>>>>> Please refer to the following fragment in the 2nd patch. >>>>>> >>>>>> + /* >>>>>> + * If the request is issued unsuccessfully with >>>>>> + * BLK_STS_DEV_RESOURCE or BLK_STS_RESOURCE, insert >>>>>> + * the request to hctx dispatch list due to attached >>>>>> + * lldd resource. >>>>>> + */ >>>>>> + force = true; >>>>>> + ret = __blk_mq_issue_directly(hctx, rq, cookie, last); >>>>>> +out_unlock: >>>>>> + hctx_unlock(hctx, srcu_idx); >>>>>> +out: >>>>>> + switch (ret) { >>>>>> + case BLK_STS_OK: >>>>>> + break; >>>>>> + case BLK_STS_DEV_RESOURCE: >>>>>> + case BLK_STS_RESOURCE: >>>>>> + if (force) { >>>>>> + blk_mq_request_bypass_insert(rq, run_queue); >>>>>> + ret = bypass ? BLK_STS_OK : ret; >>>>>> + } else if (!bypass) { >>>>>> + blk_mq_sched_insert_request(rq, false, >>>>>> + run_queue, false); >>>>>> + } >>>>>> + break; >>>>>> + default: >>>>> >>>>> You are right, I missed that you set force = true before doing the >>>>> issue. So this looks good to me! >>>> >>>> I applied your series. With this, we should be good to remove the >>>> REQ_NOMERGE logic that was added for the corruption case, and the >>>> blk_rq_can_direct_dispatch() as well? >>>> >>> >>> Yes, it should be that. >>> Every thing rejected by .queue_rq is ended or inserted into hctx dispatch >>> list. And also direct-issue path is unified with normal path. >> >> Why are we doing that return value dance, depending on whether this >> is a bypass insert or not? That seems confusing. >> > > For the 'bypass == false' case, it need to know whether the request is issued > successfully. This is for the 3rd patch. > I used to use the returned cookie to identify the result, but you don't like it. > So I have to use this return value. Makes sense, but could probably do with a comment. I'm going to let the series float for a day or two to ensure others get a chance to review it, then we can move forward.
On 12/7/18 11:47 AM, Jens Axboe wrote: > On 12/6/18 8:46 PM, jianchao.wang wrote: >> >> >> On 12/7/18 11:42 AM, Jens Axboe wrote: >>> On 12/6/18 8:41 PM, jianchao.wang wrote: >>>> >>>> >>>> On 12/7/18 11:34 AM, Jens Axboe wrote: >>>>> On 12/6/18 8:32 PM, Jens Axboe wrote: >>>>>> On 12/6/18 8:26 PM, jianchao.wang wrote: >>>>>>> >>>>>>> >>>>>>> On 12/7/18 11:16 AM, Jens Axboe wrote: >>>>>>>> On 12/6/18 8:09 PM, Jianchao Wang wrote: >>>>>>>>> Hi Jens >>>>>>>>> >>>>>>>>> Please consider this patchset for 4.21. >>>>>>>>> >>>>>>>>> It refactors the code of issue request directly to unify the interface >>>>>>>>> and make the code clearer and more readable. >>>>>>>>> >>>>>>>>> This patch set is rebased on the recent for-4.21/block and add the 1st >>>>>>>>> patch which inserts the non-read-write request to hctx dispatch >>>>>>>>> list to avoid to involve merge and io scheduler when bypass_insert >>>>>>>>> is true, otherwise, inserting is ignored, BLK_STS_RESOURCE is returned >>>>>>>>> and the caller will fail forever. >>>>>>>>> >>>>>>>>> The 2nd patch refactors the code of issue request directly to unify the >>>>>>>>> helper interface which could handle all the cases. >>>>>>>>> >>>>>>>>> The 3rd patch make blk_mq_sched_insert_requests issue requests directly >>>>>>>>> with 'bypass' false, then it needn't to handle the non-issued requests >>>>>>>>> any more. >>>>>>>>> >>>>>>>>> The 4th patch replace and kill the blk_mq_request_issue_directly. >>>>>>>> >>>>>>>> Sorry to keep iterating on this, but let's default to inserting to >>>>>>>> the dispatch list if we ever see busy from a direct dispatch. I'm fine >>>>>>>> with doing that for 4.21, as suggested by Ming, I just didn't want to >>>>>>>> fiddle with it for 4.20. This will prevent any merging on the request >>>>>>>> going forward, which I think is a much safer default. >>>>>>>> >>>>>>>> You do this already for some cases. Let's do it unconditionally for >>>>>>>> a request that was ever subjected to ->queue_rq() and we didn't either >>>>>>>> error or finish after the fact. >>>>>>>> >>>>>>> I have done it in this version if I get your point correctly. >>>>>>> Please refer to the following fragment in the 2nd patch. >>>>>>> >>>>>>> + /* >>>>>>> + * If the request is issued unsuccessfully with >>>>>>> + * BLK_STS_DEV_RESOURCE or BLK_STS_RESOURCE, insert >>>>>>> + * the request to hctx dispatch list due to attached >>>>>>> + * lldd resource. >>>>>>> + */ >>>>>>> + force = true; >>>>>>> + ret = __blk_mq_issue_directly(hctx, rq, cookie, last); >>>>>>> +out_unlock: >>>>>>> + hctx_unlock(hctx, srcu_idx); >>>>>>> +out: >>>>>>> + switch (ret) { >>>>>>> + case BLK_STS_OK: >>>>>>> + break; >>>>>>> + case BLK_STS_DEV_RESOURCE: >>>>>>> + case BLK_STS_RESOURCE: >>>>>>> + if (force) { >>>>>>> + blk_mq_request_bypass_insert(rq, run_queue); >>>>>>> + ret = bypass ? BLK_STS_OK : ret; >>>>>>> + } else if (!bypass) { >>>>>>> + blk_mq_sched_insert_request(rq, false, >>>>>>> + run_queue, false); >>>>>>> + } >>>>>>> + break; >>>>>>> + default: >>>>>> >>>>>> You are right, I missed that you set force = true before doing the >>>>>> issue. So this looks good to me! >>>>> >>>>> I applied your series. With this, we should be good to remove the >>>>> REQ_NOMERGE logic that was added for the corruption case, and the >>>>> blk_rq_can_direct_dispatch() as well? >>>>> >>>> >>>> Yes, it should be that. >>>> Every thing rejected by .queue_rq is ended or inserted into hctx dispatch >>>> list. And also direct-issue path is unified with normal path. >>> >>> Why are we doing that return value dance, depending on whether this >>> is a bypass insert or not? That seems confusing. >>> >> >> For the 'bypass == false' case, it need to know whether the request is issued >> successfully. This is for the 3rd patch. >> I used to use the returned cookie to identify the result, but you don't like it. >> So I have to use this return value. > > Makes sense, but could probably do with a comment. I'm going to let the > series float for a day or two to ensure others get a chance to review it, > then we can move forward. > Do I need a respin about the comment ? Thanks Jianchao
On 12/9/18 6:18 PM, jianchao.wang wrote: > > > On 12/7/18 11:47 AM, Jens Axboe wrote: >> On 12/6/18 8:46 PM, jianchao.wang wrote: >>> >>> >>> On 12/7/18 11:42 AM, Jens Axboe wrote: >>>> On 12/6/18 8:41 PM, jianchao.wang wrote: >>>>> >>>>> >>>>> On 12/7/18 11:34 AM, Jens Axboe wrote: >>>>>> On 12/6/18 8:32 PM, Jens Axboe wrote: >>>>>>> On 12/6/18 8:26 PM, jianchao.wang wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 12/7/18 11:16 AM, Jens Axboe wrote: >>>>>>>>> On 12/6/18 8:09 PM, Jianchao Wang wrote: >>>>>>>>>> Hi Jens >>>>>>>>>> >>>>>>>>>> Please consider this patchset for 4.21. >>>>>>>>>> >>>>>>>>>> It refactors the code of issue request directly to unify the interface >>>>>>>>>> and make the code clearer and more readable. >>>>>>>>>> >>>>>>>>>> This patch set is rebased on the recent for-4.21/block and add the 1st >>>>>>>>>> patch which inserts the non-read-write request to hctx dispatch >>>>>>>>>> list to avoid to involve merge and io scheduler when bypass_insert >>>>>>>>>> is true, otherwise, inserting is ignored, BLK_STS_RESOURCE is returned >>>>>>>>>> and the caller will fail forever. >>>>>>>>>> >>>>>>>>>> The 2nd patch refactors the code of issue request directly to unify the >>>>>>>>>> helper interface which could handle all the cases. >>>>>>>>>> >>>>>>>>>> The 3rd patch make blk_mq_sched_insert_requests issue requests directly >>>>>>>>>> with 'bypass' false, then it needn't to handle the non-issued requests >>>>>>>>>> any more. >>>>>>>>>> >>>>>>>>>> The 4th patch replace and kill the blk_mq_request_issue_directly. >>>>>>>>> >>>>>>>>> Sorry to keep iterating on this, but let's default to inserting to >>>>>>>>> the dispatch list if we ever see busy from a direct dispatch. I'm fine >>>>>>>>> with doing that for 4.21, as suggested by Ming, I just didn't want to >>>>>>>>> fiddle with it for 4.20. This will prevent any merging on the request >>>>>>>>> going forward, which I think is a much safer default. >>>>>>>>> >>>>>>>>> You do this already for some cases. Let's do it unconditionally for >>>>>>>>> a request that was ever subjected to ->queue_rq() and we didn't either >>>>>>>>> error or finish after the fact. >>>>>>>>> >>>>>>>> I have done it in this version if I get your point correctly. >>>>>>>> Please refer to the following fragment in the 2nd patch. >>>>>>>> >>>>>>>> + /* >>>>>>>> + * If the request is issued unsuccessfully with >>>>>>>> + * BLK_STS_DEV_RESOURCE or BLK_STS_RESOURCE, insert >>>>>>>> + * the request to hctx dispatch list due to attached >>>>>>>> + * lldd resource. >>>>>>>> + */ >>>>>>>> + force = true; >>>>>>>> + ret = __blk_mq_issue_directly(hctx, rq, cookie, last); >>>>>>>> +out_unlock: >>>>>>>> + hctx_unlock(hctx, srcu_idx); >>>>>>>> +out: >>>>>>>> + switch (ret) { >>>>>>>> + case BLK_STS_OK: >>>>>>>> + break; >>>>>>>> + case BLK_STS_DEV_RESOURCE: >>>>>>>> + case BLK_STS_RESOURCE: >>>>>>>> + if (force) { >>>>>>>> + blk_mq_request_bypass_insert(rq, run_queue); >>>>>>>> + ret = bypass ? BLK_STS_OK : ret; >>>>>>>> + } else if (!bypass) { >>>>>>>> + blk_mq_sched_insert_request(rq, false, >>>>>>>> + run_queue, false); >>>>>>>> + } >>>>>>>> + break; >>>>>>>> + default: >>>>>>> >>>>>>> You are right, I missed that you set force = true before doing the >>>>>>> issue. So this looks good to me! >>>>>> >>>>>> I applied your series. With this, we should be good to remove the >>>>>> REQ_NOMERGE logic that was added for the corruption case, and the >>>>>> blk_rq_can_direct_dispatch() as well? >>>>>> >>>>> >>>>> Yes, it should be that. >>>>> Every thing rejected by .queue_rq is ended or inserted into hctx dispatch >>>>> list. And also direct-issue path is unified with normal path. >>>> >>>> Why are we doing that return value dance, depending on whether this >>>> is a bypass insert or not? That seems confusing. >>>> >>> >>> For the 'bypass == false' case, it need to know whether the request is issued >>> successfully. This is for the 3rd patch. >>> I used to use the returned cookie to identify the result, but you don't like it. >>> So I have to use this return value. >> >> Makes sense, but could probably do with a comment. I'm going to let the >> series float for a day or two to ensure others get a chance to review it, >> then we can move forward. >> > > Do I need a respin about the comment ? I pulled in the two fixes from this week, so it would probably need a respin on top of that.