On Mon, Oct 24, 2022 at 11:56:21AM +0100, John Garry wrote: > 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? blk_mq_bio_to_request() is one which setup these fields, then you add another one in blk_mq_rq_ctx_init(). > > 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, Did you know the exact issue on nvme-tcp, nvme-rdma or nvme-fc maybe with blk_mq_alloc_request_hctx()? > 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)? But all cpus on this hctx->cpumask could become offline. Thanks, Ming
