From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 87A0AC54EE9 for ; Tue, 20 Sep 2022 05:05:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229488AbiITFFM (ORCPT ); Tue, 20 Sep 2022 01:05:12 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48606 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229498AbiITFFI (ORCPT ); Tue, 20 Sep 2022 01:05:08 -0400 Received: from out30-43.freemail.mail.aliyun.com (out30-43.freemail.mail.aliyun.com [115.124.30.43]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3FE6240E23; Mon, 19 Sep 2022 22:05:07 -0700 (PDT) X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R121e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018046049;MF=ziyangzhang@linux.alibaba.com;NM=1;PH=DS;RN=6;SR=0;TI=SMTPD_---0VQHRu7D_1663650303; Received: from 30.97.56.91(mailfrom:ZiyangZhang@linux.alibaba.com fp:SMTPD_---0VQHRu7D_1663650303) by smtp.aliyun-inc.com; Tue, 20 Sep 2022 13:05:04 +0800 Message-ID: <3f4c227c-29ba-3d5b-47ab-9ca88c36044e@linux.alibaba.com> Date: Tue, 20 Sep 2022 13:05:01 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 Subject: Re: [PATCH V3 5/7] ublk_drv: consider recovery feature in aborting mechanism Content-Language: en-US From: Ziyang Zhang To: Ming Lei Cc: axboe@kernel.dk, xiaoguang.wang@linux.alibaba.com, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, joseph.qi@linux.alibaba.com References: <20220913041707.197334-1-ZiyangZhang@linux.alibaba.com> <20220913041707.197334-6-ZiyangZhang@linux.alibaba.com> <9a682fac-f022-1f4d-5c2c-e1f0a84746d8@linux.alibaba.com> <475726f7-bb65-5681-1967-a9ae9075004e@linux.alibaba.com> In-Reply-To: <475726f7-bb65-5681-1967-a9ae9075004e@linux.alibaba.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org On 2022/9/20 12:45, Ziyang Zhang wrote: > On 2022/9/20 11:04, Ming Lei wrote: >> On Tue, Sep 20, 2022 at 09:49:33AM +0800, Ziyang Zhang wrote: >>> 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. >> >> OK, looks I miss this point, but you should have quiesced queue at the >> beginning of ublk_quiesce_dev(), then the transition period can be kept >> as short as possible. Otherwise, if one queue pthread isn't dying, the >> device can be kept in this part-working state forever. >> > > Ming, this is what you said in PATCH V2: > " > The simplest handling might be to exit all ublk queues first, and re-create one > new process to recover all since the request queue is required to be > quiesced first, and all ublk queue is actually quiesced too. So from user > viewpoint, there is nothing visible comparing with just recovering > single ubq daemon/pthread. > " > > So I assume that quiesce_work starts only after all ubq_damons are dying. > Note that current ublk does not support mutpile process opening the same chardev. > > Really we should agree on this. My suggestion is that we only consider "all ubq_daemons > are dying". > > You mention that someone want to enable "switch ubq_daemon pthread to another one" and > I think it is another feature but not recovery feature. > > Regards, > Zhang. This should be considered very carefully, Ming.