On 4/12/23 02:41, Damien Le Moal wrote:
> On 4/12/23 17:47, Chaitanya Kulkarni wrote:
>> QUEUE_FLAG_NOWAIT is set by default to mq drivers such null_blk when
>> it is used with NULL_Q_MQ mode as a part of QUEUE_FLAG_MQ_DEFAULT that
>> gets assigned in following code path see blk_mq_init_allocated_queue():-
> Can you fix the grammar/punctuation in this sentence ? Looks like some words are
> missing, making it hard to read.
done.
>> null_add_dev()
>> if (dev->queue_mode == NULL_Q_MQ) {
>> blk_mq_alloc_disk()
>> __blk_mq_alloc_disk()
>> blk_mq_init_queue_data()
>> blk_mq_init_allocated_queue()
>> q->queue_flags |= QUEUE_FLAG_MQ_DEFAULT;
>> }
>>
>> But it is not set when null_blk is loaded with NULL_Q_BIO mode in following
>> code path like other bio drivers do e.g. nvme-multipath :-
>>
>> if (dev->queue_mode == NULL_Q_BIO) {
>> nullb->disk = blk_alloc_disk(nullb->dev->home_node);
>> blk_alloc_disk()
>> blk_alloc_queue()
>> __alloc_disk_nodw()
>> }
>>
>> Add a new module parameter nowait and respective configfs attr that will
>> set or clear the QUEUE_FLAG_NOWAIT based on a value set by user in
>> null_add_dev() irrespective of the queue mode, by default keep it
>> enabled to retain the original behaviour for the NULL_Q_MQ mode.
> Nope. You are changing the behavior. See below.
>
true, changed it so that this flag is only set for the NULL_Q_BIO ...
>> Depending on nowait value use GFP_NOWAIT or GFP_NOIO for the alloction
>> in the null_alloc_page() for alloc_pages() and in null_insert_page()
>> for radix_tree_preload().
>>
>> Observed performance difference with this patch for fio iouring with
>> configfs and non configfs null_blk with queue modes NULL_Q_BIO and
>> NULL_Q_MQ:-
> [...]
>
>> @@ -983,11 +990,11 @@ static struct nullb_page *null_insert_page(struct nullb *nullb,
>>
>> spin_unlock_irq(&nullb->lock);
>>
>> - t_page = null_alloc_page();
>> + t_page = null_alloc_page(nullb->dev->nowait ? GFP_NOWAIT : GFP_NOIO);
> This can potentially result in failed allocations, so IO errors, which otherwise
> would not happen without this change. Not nice. Memory backing also sets
> BLK_MQ_F_BLOCKING, which I am not sure is compatible with QUEUE_FLAG_NOWAIT...
> Would need to check that again.
>
>
yes we should avoid that, here is explanation :-
BLK_MQ_F_BLOCKING is set in null_init_tag_set().
null_init_tag_set() has two callers:-
1. null_init() :- checks if queue_mode=NULL_Q_MQ and shared_tags=1
before setting BLK_MQ_F_BLOCKING for shared tagset.
2. null_add_dev():- checks if queue_mode=NULL_Q_MQ before
setting BLK_MQ_F_BLOCKING for non-shared tagset.
With nowait only allowed for qmode=NULL_Q_BIO (see [1]) it will error
out with qmode=NULL_Q_MQ and will never set BLK_MQ_F_BLOCKING,
I'll add check in the null_validate_conf() just to make sure in V3.
Thanks for review comments.
-ck
[1] V2:- https://marc.info/?l=linux-block&r=1&b=202304&w=2
