DM-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: JeffleXu <jefflexu@linux.alibaba.com>
To: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com, joseph.qi@linux.alibaba.com,
	ming.lei@redhat.com, linux-block@vger.kernel.org
Subject: Re: dm: fix imposition of queue_limits in dm_wq_work() thread
Date: Tue, 29 Sep 2020 12:08:07 +0800
Message-ID: <2f6f1882-0519-22fc-a11c-645b721ac731@linux.alibaba.com> (raw)
In-Reply-To: <20200928160322.GA23320@redhat.com>

I see. Thanks.

On 9/29/20 12:03 AM, Mike Snitzer wrote:
> On Sun, Sep 27 2020 at  8:04am -0400,
> Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
>
>> Hi Mike, would you mind further expalin why bio processed by dm_wq_work()
>> always gets a previous ->submit_bio. Considering the following call graph:
>>
>> ->submit_bio, that is, dm_submit_bio
>>    DMF_BLOCK_IO_FOR_SUSPEND set, thus queue_io()
>>
>> then worker thread dm_wq_work()
>>    dm_process_bio  // at this point. the input bio is the original bio
>>                       submitted to dm device
>>
>> Please let me know if I missed something.
>>
>> Thanks.
>> Jeffle
> In general you have a valid point, that blk_queue_split() won't have
> been done for the suspended device case, but blk_queue_split() cannot be
> used if not in ->submit_bio -- IIUC you cannot just do it from a worker
> thread and hope to have proper submission order (depth first) as
> provided by __submit_bio_noacct().  Because this IO will be submitted
> from worker you could have multiple threads allocating from the
> q->bio_split mempool at the same time.
>
> All said, I'm not quite sure how to address this report.  But I'll keep
> at it and see what I can come up with.
>
> Thanks,
> Mike
>
>> Original commit log:
>> dm_process_bio() can be called by dm_wq_work(), and if the currently
>> processing bio is the very beginning bio submitted to the dm device,
>> that is it has not been handled by previous ->submit_bio, then we also
>> need to impose the queue_limits when it's in thread (dm_wq_work()).
>>
>> Fixes: cf9c37865557 ("dm: fix comment in dm_process_bio()")
>> Fixes: 568c73a355e0 ("dm: update dm_process_bio() to split bio if in ->make_request_fn()")
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>> ---
>>   drivers/md/dm.c | 16 ++++++----------
>>   1 file changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>> index 6ed05ca65a0f..54471c75ddef 100644
>> --- a/drivers/md/dm.c
>> +++ b/drivers/md/dm.c
>> @@ -1744,17 +1744,13 @@ static blk_qc_t dm_process_bio(struct mapped_device *md,
>>   	}
>>   
>>   	/*
>> -	 * If in ->submit_bio we need to use blk_queue_split(), otherwise
>> -	 * queue_limits for abnormal requests (e.g. discard, writesame, etc)
>> -	 * won't be imposed.
>> -	 * If called from dm_wq_work() for deferred bio processing, bio
>> -	 * was already handled by following code with previous ->submit_bio.
>> +	 * Call blk_queue_split() so that queue_limits for abnormal requests
>> +	 * (e.g. discard, writesame, etc) are ensured to be imposed, while
>> +	 * it's not needed for regular IO, since regular IO will be split by
>> +	 * following __split_and_process_bio.
>>   	 */
>> -	if (current->bio_list) {
>> -		if (is_abnormal_io(bio))
>> -			blk_queue_split(&bio);
>> -		/* regular IO is split by __split_and_process_bio */
>> -	}
>> +	if (is_abnormal_io(bio))
>> +		blk_queue_split(&bio);
>>   
>>   	if (dm_get_md_type(md) == DM_TYPE_NVME_BIO_BASED)
>>   		return __process_bio(md, map, bio, ti);
>> -- 
>> 2.27.0
>>

  reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-27 12:04 [RFC] " Jeffle Xu
2020-09-28 16:03 ` Mike Snitzer
2020-09-29  4:08   ` JeffleXu [this message]
2020-09-29 20:39   ` [PATCH] dm: fix missing imposition of queue_limits from " Mike Snitzer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2f6f1882-0519-22fc-a11c-645b721ac731@linux.alibaba.com \
    --to=jefflexu@linux.alibaba.com \
    --cc=dm-devel@redhat.com \
    --cc=joseph.qi@linux.alibaba.com \
    --cc=linux-block@vger.kernel.org \
    --cc=ming.lei@redhat.com \
    --cc=snitzer@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DM-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dm-devel/0 dm-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dm-devel dm-devel/ https://lore.kernel.org/dm-devel \
		dm-devel@redhat.com
	public-inbox-index dm-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.redhat.dm-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git