Re: [PATCH v6 2/5] blk-mq: Introduce atomic variants of blk_mq_(all_tag|tagset_busy)_iter

[John Garry <john.garry@huawei.com>]

On 07/04/2021 19:42, Bart Van Assche wrote:
>>
>>> - scsi_host_busy(), scsi_host_complete_all_commands() and
>>>    scsi_host_busy_iter() are used by multiple SCSI LLDs so analyzing 
>>> whether
>>>    or not these functions may sleep is hard. Instead of performing that
>>>    analysis, make it safe to call these functions from atomic context.
>>
>> Please help me understand this solution. The background is that we are 
>> unsure if the SCSI iters callback functions may sleep. So we use the 
>> blk_mq_all_tag_iter_atomic() iter, which tells us that we must not 
>> sleep. And internally, it uses rcu read lock protection mechanism, 
>> which relies on not sleeping. So it seems that we're making the SCSI 
>> iter functions being safe in atomic context, and, as such, rely on the 
>> iter callbacks not to sleep.
>>
>> But if we call the SCSI iter function from non-atomic context and the 
>> iter callback may sleep, then that is a problem, right? We're still 
>> using rcu.
> 

Hi Bart,

> 
> Please take a look at the output of the following grep command:
> 
> git grep -nHEw 'blk_mq_tagset_busy_iter|scsi_host_busy_iter'\ drivers/scsi
> 
> Do you agree with me that it is safe to call all the callback functions 
> passed to blk_mq_tagset_busy_iter() and scsi_host_busy_iter() from an 
> atomic context?
> 

That list output I got is at the bottom (with this patchset, not 
mainline - not sure which you meant).

The following does not look atomic safe with the mutex usage:
drivers/block/nbd.c:819:        blk_mq_tagset_busy_iter(&nbd->tag_set, 
nbd_clear_req, NULL);

static bool nbd_clear_req(struct request *req, void *data, bool reserved)
{
	struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);

	mutex_lock(&cmd->lock);
	cmd->status = BLK_STS_IOERR;
	mutex_unlock(&cmd->lock);

	blk_mq_complete_request(req);
	return true;
}

But blk_mq_tagset_busy_iter() uses BT_TAG_ITER_MAY sleep flag in your 
series.

As for the fc, I am not sure. I assume that you would know more about 
this. My concern is

__nvme_fc_abort_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_fcp_op *op)
{
...

	ctrl->lport->ops->fcp_abort(&ctrl->lport->localport, ..);
}

Looking at many instances of fcp_abort callback, they look atomic safe 
from general high usage of spinlock, but I am not certain. They are 
quite complex.

Thanks,
John

block/blk-core.c:287: * blk_mq_tagset_busy_iter() and also for their 
atomic variants to finish
block/blk-mq-debugfs.c:418: 
blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy_rq,
block/blk-mq-tag.c:405: * blk_mq_tagset_busy_iter - iterate over all 
started requests in a tag set
block/blk-mq-tag.c:416:void blk_mq_tagset_busy_iter(struct 
blk_mq_tag_set *tagset,
block/blk-mq-tag.c:436:EXPORT_SYMBOL(blk_mq_tagset_busy_iter);
drivers/block/mtip32xx/mtip32xx.c:2685: 
blk_mq_tagset_busy_iter(&dd->tags, mtip_queue_cmd, dd);
drivers/block/mtip32xx/mtip32xx.c:2690: 
blk_mq_tagset_busy_iter(&dd->tags,
drivers/block/mtip32xx/mtip32xx.c:3800: 
blk_mq_tagset_busy_iter(&dd->tags, mtip_no_dev_cleanup, dd);
drivers/block/nbd.c:819:        blk_mq_tagset_busy_iter(&nbd->tag_set, 
nbd_clear_req, NULL);
drivers/nvme/host/core.c:392: 
blk_mq_tagset_busy_iter(ctrl->tagset,
drivers/nvme/host/core.c:402: 
blk_mq_tagset_busy_iter(ctrl->admin_tagset,
drivers/nvme/host/fc.c:2477: 
blk_mq_tagset_busy_iter(&ctrl->tag_set,
drivers/nvme/host/fc.c:2500: 
blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
drivers/nvme/host/pci.c:2504:   blk_mq_tagset_busy_iter(&dev->tagset, 
nvme_cancel_request, &dev->ctrl);
drivers/nvme/host/pci.c:2505: 
blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_request, 
&dev->ctrl);
drivers/nvme/target/loop.c:420: 
blk_mq_tagset_busy_iter(&ctrl->tag_set,
drivers/nvme/target/loop.c:430: 
blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
include/linux/blk-mq.h:527:void blk_mq_tagset_busy_iter(struct 
blk_mq_tag_set *tagset,
lines 1-18/18 (END)