Messages in this thread | | | Date | Mon, 24 Oct 2022 11:56:21 +0100 | From | John Garry <> | Subject | Re: [PATCH] blk-mq: Properly init bios from blk_mq_alloc_request_hctx() |
| |
On 23/10/2022 14:12, Ming Lei wrote: >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 8070b6c10e8d..260adeb2e455 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -402,6 +402,10 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data, >> } >> } >> >> + rq->__data_len = 0; >> + rq->__sector = (sector_t) -1; >> + rq->bio = rq->biotail = NULL; >> + >> return rq; >> } >> >> @@ -591,9 +595,6 @@ struct request *blk_mq_alloc_request(struct request_queue *q, blk_opf_t opf, >> if (!rq) >> goto out_queue_exit; >> } >> - rq->__data_len = 0; >> - rq->__sector = (sector_t) -1; >> - rq->bio = rq->biotail = NULL; > This patch looks not good, why do you switch to initialize the three fields > twice in fast path?
Can you please show me how these are initialized twice?
If there is a real concern with this then we go with my original idea, which was to copy the init method of blk_mq_alloc_request() (in blk_mq_alloc_request_hctx())
> > BTW, we know blk_mq_alloc_request_hctx() has big trouble, so please > avoid to extend it to other use cases.
Yeah, I know this, but sometimes we just need to allocate for a specific HW queue...
For my usecase of interest, it should not impact if the cpumask of the HW queue goes offline after selecting the cpu in blk_mq_alloc_request_hctx(), so any race is ok ... I think.
However it should be still possible to make blk_mq_alloc_request_hctx() more robust. How about using something like work_on_cpu_safe() to allocate and execute the request with blk_mq_alloc_request() on a cpu associated with the HW queue, such that we know the cpu is online and stays online until we execute it? Or also extent to work_on_cpumask_safe() variant, so that we don't need to try all cpus in the mask (to see if online)?
Thanks, John
| |