From: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>
To: Ming Lei <ming.lei@redhat.com>
Cc: axboe@kernel.dk, xiaoguang.wang@linux.alibaba.com,
linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
joseph.qi@linux.alibaba.com
Subject: Re: [PATCH V3 5/7] ublk_drv: consider recovery feature in aborting mechanism
Date: Tue, 20 Sep 2022 09:49:33 +0800 [thread overview]
Message-ID: <dbc78e92-ede7-fc63-1bee-83794bf1e33b@linux.alibaba.com> (raw)
In-Reply-To: <YyhhnbrHTJpW4Xcm@T590>
On 2022/9/19 20:33, Ming Lei wrote:
>>>> +
>>>> +static void ublk_quiesce_queue(struct ublk_device *ub,
>>>> + struct ublk_queue *ubq)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < ubq->q_depth; i++) {
>>>> + struct ublk_io *io = &ubq->ios[i];
>>>> +
>>>> + if (!(io->flags & UBLK_IO_FLAG_ACTIVE)) {
>>>> + struct request *rq = blk_mq_tag_to_rq(
>>>> + ub->tag_set.tags[ubq->q_id], i);
>>>> +
>>>> + WARN_ON_ONCE(!rq);
>>>> + pr_devel("%s: %s rq: qid %d tag %d io_flags %x\n", __func__,
>>>> + ublk_queue_can_use_recovery_reissue(ubq) ?
>>>> + "requeue" : "abort",
>>>> + ubq->q_id, i, io->flags);
>>>> + if (ublk_queue_can_use_recovery_reissue(ubq))
>>>> + blk_mq_requeue_request(rq, false);
>>>
>>> This way is too violent.
>>>
>>> There may be just one queue dying, but you requeue all requests
>>> from any queue. I'd suggest to take the approach in ublk_daemon_monitor_work(),
>>> such as, just requeuing requests in dying queue.
>>
>> If we want to start a new process after a crash for USER_RECOVERY, all old ubq_daemons
>> must exit and rqs of all queues have to be requeued/aborted. We cannot let live
>> ubq_daemons run any more because they do not belong to the new process.
>
> IMO, the old process really can exist, and recently even I got such
> requirement for switching queue from one thread to another.
For now, only one process can open /dev/ublkcX, so a new process is necessary now.
If you think "per ubq_daemon" recovery is reasonable, I can do that in the future
if multiple processes is supported. But I really suggest that we can keep current
design as the first step which assumes all ubq_daemons are exited and a new process
is started, and that really meets our requirement.
BTW, START_USER_RECOVERY has to be reconsidered because we may need to pass a ubq_id
with it.
>
> What we should do is to get all inflight requests done, and cancel all io
> commands, no matter if the ubq pthread is dead or live.
>
>>
>> BTW, I really wonder why there could be just one queue dying? All queues must be dying
>> shortly after any ubq_daemon is dying since they are all pthreads in the same process.
>
> You can't assume it is always so. Maybe one pthread is dead first, and
> others are dying later, maybe just one is dead.
Yes, I know there may be only one pthread is dead while others keep running, but now
ublk_drv only support one process opening the same /dev/ublkcX, so other pthreads
must dead(no matter they are aborted by signal or themselves) later.
>
> If one queue's pthread is live, you may get trouble by simply requeuing
> the request, that is why I suggest to re-use the logic of
> ublk_daemon_monitor_work/ublk_abort_queue().
Actually, if any ubq_daemon is live, no rqs are requeued, please see the check in
ublk_quiesce_dev(). It always makes sure that ALL ubq_daemons are dying, then it
starts quiesce jobs.
>
> For stopping device, request queue is frozen in del_gendisk() and all
> in-flight requests are drained, and monitor work provides such
> guarantee.
>
> For user recovery, monitor work should help you too by aborting one
> queue if it is dying until all requests are drained.
Monitor work can schedule quiesce_work if it finds a dying ubq_daemon.
Then quiesce_work calls ublk_quiesce_dev(). I do this because ublk_quiesce_dev()
has to wait all inflight rqs with ACTIVE set being requeued.
>
>>
>>>
>>> That said you still can re-use the logic in ublk_abort_queue()/ublk_daemon_monitor_work()
>>> for making progress, just changing aborting request with requeue in
>>> ublk_abort_queue().
>>
>> I get your point, but it may be hard to reuse the logic in ublk_daemon_monitor_work()
>> because:
>> (1) we have to quiesce request queue in ublk_quiesce_dev(). This has to be done with
>> ub_mutex.
>> (2) ublk_quiesce_dev() cannot be run after rqs are requeued/aborted.
>
> I don't get your point, the request queue needs to be quiesced once, then
> either inflight requests are requeued if the queue is dying, or completed by
> the queue's pthread if it is live. As you mentioned, in reality, most times,
> all pthreads will be killed, but timing can be different, and I think
> you can not requeue one request if the ubq pthread isn't dying.
I do not requeue rqs with a live ubq_daemon. ublk_quiesce_dev() always starts
after all ubq_daemons are dying.
Regards,
Zhang.
next prev parent reply other threads:[~2022-09-20 1:49 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-13 4:17 [PATCH V3 0/7] ublk_drv: add USER_RECOVERY support ZiyangZhang
2022-09-13 4:17 ` [PATCH V3 1/7] ublk_drv: check 'current' instead of 'ubq_daemon' ZiyangZhang
2022-09-13 4:17 ` [PATCH V3 2/7] ublk_drv: refactor ublk_cancel_queue() ZiyangZhang
2022-09-13 4:17 ` [PATCH V3 3/7] ublk_drv: define macros for recovery feature and check them ZiyangZhang
2022-09-20 5:04 ` Ming Lei
2022-09-13 4:17 ` [PATCH V3 4/7] ublk_drv: requeue rqs with recovery feature enabled ZiyangZhang
2022-09-19 3:55 ` Ming Lei
2022-09-19 9:12 ` Ziyang Zhang
2022-09-19 12:39 ` Ming Lei
2022-09-20 1:31 ` Ziyang Zhang
2022-09-20 2:39 ` Ming Lei
2022-09-20 3:04 ` Ziyang Zhang
2022-09-20 3:18 ` Ming Lei
2022-09-20 3:34 ` Ziyang Zhang
2022-09-20 4:41 ` Ming Lei
2022-09-13 4:17 ` [PATCH V3 5/7] ublk_drv: consider recovery feature in aborting mechanism ZiyangZhang
2022-09-19 9:32 ` Ming Lei
2022-09-19 9:55 ` Ziyang Zhang
2022-09-19 12:33 ` Ming Lei
2022-09-20 1:49 ` Ziyang Zhang [this message]
2022-09-20 3:04 ` Ming Lei
2022-09-20 3:24 ` Ziyang Zhang
2022-09-20 4:01 ` Ming Lei
2022-09-20 4:39 ` Ziyang Zhang
2022-09-20 4:49 ` Ming Lei
2022-09-20 5:03 ` Ziyang Zhang
2022-09-20 4:45 ` Ziyang Zhang
2022-09-20 5:05 ` Ziyang Zhang
2022-09-13 4:17 ` [PATCH V3 6/7] ublk_drv: add START_USER_RECOVERY and END_USER_RECOVERY support ZiyangZhang
2022-09-19 13:03 ` Ming Lei
2022-09-20 2:41 ` Ziyang Zhang
2022-09-13 4:17 ` [PATCH V3 7/7] ublk_drv: do not run monitor_work while ub's state is QUIESCED ZiyangZhang
2022-09-19 2:17 ` [PATCH V3 0/7] ublk_drv: add USER_RECOVERY support Ziyang Zhang
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=dbc78e92-ede7-fc63-1bee-83794bf1e33b@linux.alibaba.com \
--to=ziyangzhang@linux.alibaba.com \
--cc=axboe@kernel.dk \
--cc=joseph.qi@linux.alibaba.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ming.lei@redhat.com \
--cc=xiaoguang.wang@linux.alibaba.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).