Re: [PATCH v5 2/3] scsi: core: Support disabling fair tag sharing

Yu Kuai
2023-11-28T02:03:40Z

Hi,

在 2023/11/28 7:05, Bart Van Assche 写道:
> On 11/22/23 22:29, Yu Kuai wrote:
>> 在 2023/11/22 3:32, Bart Van Assche 写道:
>>> +static ssize_t queue_fair_sharing_store(struct request_queue *q,
>>> +                    const char *page, size_t count)
>>> +{
>>> +    const unsigned int DFTS_BIT =
>>> ilog2(BLK_MQ_F_DISABLE_FAIR_TAG_SHARING);
>>> +    struct blk_mq_tag_set *set = q->tag_set;
>>> +    struct blk_mq_hw_ctx *hctx;
>>> +    unsigned long i;
>>> +    int res;
>>> +    bool val;
>>> +
>>> +    res = kstrtobool(page, &val);
>>> +    if (res < 0)
>>> +        return res;
>>> +
>>> +    mutex_lock(&set->tag_list_lock);
>>> +    clear_bit(DFTS_BIT, &set->flags);
>>> +    list_for_each_entry(q, &set->tag_list, tag_set_list) {
>>> +        /* Serialize against blk_mq_realloc_hw_ctxs() */
>>
>> If set/clear bit concurrent with test bit from io path, will there be
>> problem? Why don't freeze these queues?
>
> If that happens the changes applied through this sysfs attribute may
> only take
> effect after a short delay (depending on how fast changes are propagated
> from
> one CPU to another). I don't think that this is an issue?

Because wake_batch is not updated, hence actually wait/wakeup is still
the same before tag sharing is disabled.

I was worried that there might be missing wakeups, why not using
blk_mq_update_tag_set_shared() directly to disable tag sharing? And for
new disks, change blk_mq_add_queue_tag_set() to not set
BLK_MQ_F_TAG_QUEUE_SHARED as well. This way we only need a new flag for
tag_set, that's why I want to add the new sysfs entry for scsi_host,
since there are no entry represent tag_set for now...

>  >> +#define QUEUE_RW_ENTRY_NO_SYSFS_MUTEX(_prefix, _name)       \
>>> +    static struct queue_sysfs_entry _prefix##_entry = { \
>>> +        .attr = { .name = _name, .mode = 0644 },    \
>>> +        .show = _prefix##_show,                     \
>>> +        .store = _prefix##_store,                   \
>>> +        .no_sysfs_mutex = true,                     \
>>> +    };
>>> +
>>
>> This actually change all the queues from the same tagset, can we add
>> this new entry to /sys/class/scsi_host/hostx/xxx ?
>
> That would make it impossible to disable fair tag sharing for block drivers
> that are not based on the SCSI core. Are you sure that's what you want?

Yes, if there are other drivers that are sharing driver tags, this is
not good, can you give some examples?

Thanks,
Kuai
>
> Thanks,
>
> Bart.
> .
>