| Message ID | 20240617194451.435445-3-ushankar@purestorage.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ublk: support device recovery without I/O queueing | expand |
On Mon, Jun 17, 2024 at 01:44:49PM -0600, Uday Shankar wrote: > ublk currently supports the following behaviors on ublk server exit: > > A: outstanding I/Os get errors, subsequently issued I/Os get errors > B: outstanding I/Os get errors, subsequently issued I/Os queue > C: outstanding I/Os get reissued, subsequently issued I/Os queue > > and the following behaviors for recovery of preexisting block devices by > a future incarnation of the ublk server: > > 1: ublk devices stopped on ublk server exit (no recovery possible) > 2: ublk devices are recoverable using start/end_recovery commands > > The userspace interface allows selection of combinations of these > behaviors using flags specified at device creation time, namely: > > default behavior: A + 1 > UBLK_F_USER_RECOVERY: B + 2 > UBLK_F_USER_RECOVERY|UBLK_F_USER_RECOVERY_REISSUE: C + 2 ublk is supposed to support A, B & C for both 1 and both 2, but it may depend on how ublk server is implemented. In cover letter, it is mentioned that "A + 2 is a currently unsupported behavior", can you explain it a bit? Such as, how does ublk server handle the I/O error? And when/how to recover? why doesn't this way work? For example, one rough way is to kill the daemon in case of A, then recovery can be started and new daemon is created for handling IO. But we are open to improve this area to support more flexible ublk server implementation. Thanks, Ming
On Tue, Jun 18, 2024 at 10:11:51AM +0800, Ming Lei wrote: > On Mon, Jun 17, 2024 at 01:44:49PM -0600, Uday Shankar wrote: > > ublk currently supports the following behaviors on ublk server exit: > > > > A: outstanding I/Os get errors, subsequently issued I/Os get errors > > B: outstanding I/Os get errors, subsequently issued I/Os queue > > C: outstanding I/Os get reissued, subsequently issued I/Os queue > > > > and the following behaviors for recovery of preexisting block devices by > > a future incarnation of the ublk server: > > > > 1: ublk devices stopped on ublk server exit (no recovery possible) > > 2: ublk devices are recoverable using start/end_recovery commands > > > > The userspace interface allows selection of combinations of these > > behaviors using flags specified at device creation time, namely: > > > > default behavior: A + 1 > > UBLK_F_USER_RECOVERY: B + 2 > > UBLK_F_USER_RECOVERY|UBLK_F_USER_RECOVERY_REISSUE: C + 2 > > ublk is supposed to support A, B & C for both 1 and both 2, but it may > depend on how ublk server is implemented. > > In cover letter, it is mentioned that "A + 2 is a currently unsupported > behavior", can you explain it a bit? Such as, how does ublk server > handle the I/O error? And when/how to recover? why doesn't this way > work? Sorry if this was unclear - the behaviors I describe in A, B, C, 1, 2 are all referring to what is seen by the application using the ublk block device when the ublk server crashes. There is no sense in which the ublk server can "handle" the I/O error because during this time, there is no ublk server and all decisions on how to handle I/O are made by ublk_drv directly (based on configuration flags specified when the device was created). If the ublk server created the device with UBLK_F_USER_RECOVERY, then when the ublk server has crashed (and not restarted yet), I/Os issued by the application will queue/hang until the ublk server comes back and recovers the device, because the underlying request_queue is left in a quiesced state. So in this case, behavior A is not possible. If the ublk server created the device without UBLK_F_USER_RECOVERY, then when the ublk server has crashed (and not restarted yet), I/Os issued by the application will immediately error (since in this case, ublk will call del_gendisk). However, when the ublk server restarts, it cannot recover the existing ublk device - the disk has been deleted and the ublk device is in state UBLK_S_DEV_DEAD from which recovery is not permitted. So in this case, behavior 2 is not possible. Hence A + 2 is impossible with the current ublk_drv implementation. Please correct me if I missed something.
On Wed, Jun 26, 2024 at 11:22:43AM -0600, Uday Shankar wrote: > On Tue, Jun 18, 2024 at 10:11:51AM +0800, Ming Lei wrote: > > On Mon, Jun 17, 2024 at 01:44:49PM -0600, Uday Shankar wrote: > > > ublk currently supports the following behaviors on ublk server exit: > > > > > > A: outstanding I/Os get errors, subsequently issued I/Os get errors > > > B: outstanding I/Os get errors, subsequently issued I/Os queue > > > C: outstanding I/Os get reissued, subsequently issued I/Os queue > > > > > > and the following behaviors for recovery of preexisting block devices by > > > a future incarnation of the ublk server: > > > > > > 1: ublk devices stopped on ublk server exit (no recovery possible) > > > 2: ublk devices are recoverable using start/end_recovery commands > > > > > > The userspace interface allows selection of combinations of these > > > behaviors using flags specified at device creation time, namely: > > > > > > default behavior: A + 1 > > > UBLK_F_USER_RECOVERY: B + 2 > > > UBLK_F_USER_RECOVERY|UBLK_F_USER_RECOVERY_REISSUE: C + 2 > > > > ublk is supposed to support A, B & C for both 1 and both 2, but it may > > depend on how ublk server is implemented. > > > > In cover letter, it is mentioned that "A + 2 is a currently unsupported > > behavior", can you explain it a bit? Such as, how does ublk server > > handle the I/O error? And when/how to recover? why doesn't this way > > work? > > Sorry if this was unclear - the behaviors I describe in A, B, C, 1, 2 > are all referring to what is seen by the application using the ublk > block device when the ublk server crashes. There is no sense in which Yes, usually the app using ublk is supposed to be completely generic, and won't be taken into account. > the ublk server can "handle" the I/O error because during this time, > there is no ublk server and all decisions on how to handle I/O are made > by ublk_drv directly (based on configuration flags specified when the > device was created). > > If the ublk server created the device with UBLK_F_USER_RECOVERY, then > when the ublk server has crashed (and not restarted yet), I/Os issued by > the application will queue/hang until the ublk server comes back and > recovers the device, because the underlying request_queue is left in a > quiesced state. So in this case, behavior A is not possible. When ublk server is crashed, ublk_abort_requests() will be called to fail queued inflight requests. Meantime ubq->canceling is set to requeue new request instead of forwarding it to ublk server. So behavior A should be supported easily by failing request in ublk_queue_rq() if ubq->canceling is set. > > If the ublk server created the device without UBLK_F_USER_RECOVERY, then > when the ublk server has crashed (and not restarted yet), I/Os issued by > the application will immediately error (since in this case, ublk will > call del_gendisk). However, when the ublk server restarts, it cannot > recover the existing ublk device - the disk has been deleted and the > ublk device is in state UBLK_S_DEV_DEAD from which recovery is not > permitted. So in this case, behavior 2 is not possible. UBLK_F_USER_RECOVERY is supposed for supporting to recover device, and if this flag isn't enabled, we don't support the feature simply, so looks behavior 2 isn't one valid case, is it? > > Hence A + 2 is impossible with the current ublk_drv implementation. > Please correct me if I missed something. Please see if the above reply can address this case. Thanks, Ming
When I say "behavior A + 2," I mean behavior A and behavior 2 at the same time on the same ublk device. I still think this is not supported with current ublk_drv, see below. > > the ublk server can "handle" the I/O error because during this time, > > there is no ublk server and all decisions on how to handle I/O are made > > by ublk_drv directly (based on configuration flags specified when the > > device was created). > > > > If the ublk server created the device with UBLK_F_USER_RECOVERY, then > > when the ublk server has crashed (and not restarted yet), I/Os issued by > > the application will queue/hang until the ublk server comes back and > > recovers the device, because the underlying request_queue is left in a > > quiesced state. So in this case, behavior A is not possible. > > When ublk server is crashed, ublk_abort_requests() will be called to fail > queued inflight requests. Meantime ubq->canceling is set to requeue > new request instead of forwarding it to ublk server. > > So behavior A should be supported easily by failing request in > ublk_queue_rq() if ubq->canceling is set. This argument only works for devices created without UBLK_F_USER_RECOVERY. If UBLK_F_USER_RECOVERY is set, then the request_queue for the device is left in a quiesced state and so I/Os will not even get to ublk_queue_rq. See the following as proof (using a build of ublksrv master): # ./ublk add -t loop -f file -r 1 dev id 0: nr_hw_queues 1 queue_depth 128 block size 4096 dev_capacity 2097152 max rq size 524288 daemon pid 244608 flags 0x4a state LIVE ublkc: 240:0 ublkb: 259:0 owner: 0:0 queue 0: tid 244610 affinity(0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 ) target {"backing_file":"file","dev_size":1073741824,"direct_io":1,"name":"loop","type":1} # kill -9 244608 # dd if=/dev/urandom of=/dev/ublkb0 count=1 bs=4096 oflag=direct (hung) # ps aux | grep " D" root 244626 0.0 0.0 5620 1880 pts/0 D+ 16:57 0:00 dd if=/dev/urandom of=/dev/ublkb0 count=1 bs=4096 oflag=direct root 244656 0.0 0.0 6408 2188 pts/1 S+ 16:58 0:00 grep --color=auto D # cat /proc/244626/stack [<0>] submit_bio_wait+0x63/0x90 [<0>] __blkdev_direct_IO_simple+0xd9/0x1e0 [<0>] blkdev_write_iter+0x1b4/0x230 [<0>] vfs_write+0x2ae/0x3d0 [<0>] ksys_write+0x4f/0xc0 [<0>] do_syscall_64+0x5d/0x160 [<0>] entry_SYSCALL_64_after_hwframe+0x4b/0x53 # cat /sys/kernel/debug/block/ublkb0/state SAME_COMP|NONROT|IO_STAT|INIT_DONE|STATS|REGISTERED|QUIESCED|NOWAIT|SQ_SCHED Therefore, in order to obtain behavior A with current ublk_drv, one must not set UBLK_F_USER_RECOVERY. > > > > If the ublk server created the device without UBLK_F_USER_RECOVERY, then > > when the ublk server has crashed (and not restarted yet), I/Os issued by > > the application will immediately error (since in this case, ublk will > > call del_gendisk). However, when the ublk server restarts, it cannot > > recover the existing ublk device - the disk has been deleted and the > > ublk device is in state UBLK_S_DEV_DEAD from which recovery is not > > permitted. So in this case, behavior 2 is not possible. > > UBLK_F_USER_RECOVERY is supposed for supporting to recover device, and > if this flag isn't enabled, we don't support the feature simply, so > looks behavior 2 isn't one valid case, is it? Sure, so we're in agreement that recovery is impossible if UBLK_F_USER_RECOVERY is not set. So: - To get behavior A, UBLK_F_USER_RECOVERY must be unset - To get behavior 2, UBLK_F_USER_RECOVERY must be set Hence, having behavior A and behavior 2, at the same time, on the same device, would require UBLK_F_USER_RECOVERY to be both set and unset when that device is created. Obviously that's impossible.
On Thu, Jun 27, 2024 at 11:09:15AM -0600, Uday Shankar wrote: > When I say "behavior A + 2," I mean behavior A and behavior 2 at the > same time on the same ublk device. I still think this is not supported > with current ublk_drv, see below. > > > > the ublk server can "handle" the I/O error because during this time, > > > there is no ublk server and all decisions on how to handle I/O are made > > > by ublk_drv directly (based on configuration flags specified when the > > > device was created). > > > > > > If the ublk server created the device with UBLK_F_USER_RECOVERY, then > > > when the ublk server has crashed (and not restarted yet), I/Os issued by > > > the application will queue/hang until the ublk server comes back and > > > recovers the device, because the underlying request_queue is left in a > > > quiesced state. So in this case, behavior A is not possible. > > > > When ublk server is crashed, ublk_abort_requests() will be called to fail > > queued inflight requests. Meantime ubq->canceling is set to requeue > > new request instead of forwarding it to ublk server. > > > > So behavior A should be supported easily by failing request in > > ublk_queue_rq() if ubq->canceling is set. > > This argument only works for devices created without > UBLK_F_USER_RECOVERY. If UBLK_F_USER_RECOVERY is set, then the > request_queue for the device is left in a quiesced state and so I/Os > will not even get to ublk_queue_rq. See the following as proof (using a > build of ublksrv master): I meant that the following one-line patch may address your issue: diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 4e159948c912..a89240f4f7b0 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -1068,7 +1068,7 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq, struct request *rq) { /* We cannot process this rq so just requeue it. */ - if (ublk_queue_can_use_recovery(ubq)) + if (ublk_queue_can_use_recovery_reissue(ubq)) blk_mq_requeue_request(rq, false); else blk_mq_end_request(rq, BLK_STS_IOERR); Thanks, Ming
On Sun, Jun 30, 2024 at 09:56:36PM +0800, Ming Lei wrote: > I meant that the following one-line patch may address your issue: > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > index 4e159948c912..a89240f4f7b0 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -1068,7 +1068,7 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq, > struct request *rq) > { > /* We cannot process this rq so just requeue it. */ > - if (ublk_queue_can_use_recovery(ubq)) > + if (ublk_queue_can_use_recovery_reissue(ubq)) > blk_mq_requeue_request(rq, false); > else > blk_mq_end_request(rq, BLK_STS_IOERR); It does not work (ran the same test from my previous email, got the same results), and how could it? As I've already mentioned several times, the root of the issue is that when UBLK_F_USER_RECOVERY is set, the request queue remains quiesced when the server has exited. Quiescing the queue means that the block layer will not call the driver's queue_rq when I/Os are submitted. Instead the block layer will queue those I/Os internally, only submitting them to the driver when the queue is unquiesced, which, in the current ublk_drv, only happens when the device is recovered or deleted. Having ublk_drv return errors to I/Os issued while there is no ublk server requires the queue to be unquiesced. My patchset actually does this (see patch 4/4).
On Mon, Jul 01, 2024 at 03:02:49PM -0600, Uday Shankar wrote: > On Sun, Jun 30, 2024 at 09:56:36PM +0800, Ming Lei wrote: > > I meant that the following one-line patch may address your issue: > > > > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > > index 4e159948c912..a89240f4f7b0 100644 > > --- a/drivers/block/ublk_drv.c > > +++ b/drivers/block/ublk_drv.c > > @@ -1068,7 +1068,7 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq, > > struct request *rq) > > { > > /* We cannot process this rq so just requeue it. */ > > - if (ublk_queue_can_use_recovery(ubq)) > > + if (ublk_queue_can_use_recovery_reissue(ubq)) > > blk_mq_requeue_request(rq, false); > > else > > blk_mq_end_request(rq, BLK_STS_IOERR); > > It does not work (ran the same test from my previous email, got the same > results), and how could it? As I've already mentioned several times, the > root of the issue is that when UBLK_F_USER_RECOVERY is set, the request > queue remains quiesced when the server has exited. Quiescing the queue > means that the block layer will not call the driver's queue_rq when I/Os > are submitted. Instead the block layer will queue those I/Os internally, > only submitting them to the driver when the queue is unquiesced, which, > in the current ublk_drv, only happens when the device is recovered or > deleted. > > Having ublk_drv return errors to I/Os issued while there is no ublk > server requires the queue to be unquiesced. My patchset actually does > this (see patch 4/4). Sorry for ignoring the fact that queue is kept as quiesced after ublk server is crashed, and I will review patch 4 soon. Thanks, Ming
On Mon, Jun 17, 2024 at 01:44:49PM -0600, Uday Shankar wrote: > ublk currently supports the following behaviors on ublk server exit: > > A: outstanding I/Os get errors, subsequently issued I/Os get errors > B: outstanding I/Os get errors, subsequently issued I/Os queue > C: outstanding I/Os get reissued, subsequently issued I/Os queue > > and the following behaviors for recovery of preexisting block devices by > a future incarnation of the ublk server: > > 1: ublk devices stopped on ublk server exit (no recovery possible) > 2: ublk devices are recoverable using start/end_recovery commands > > The userspace interface allows selection of combinations of these > behaviors using flags specified at device creation time, namely: > > default behavior: A + 1 > UBLK_F_USER_RECOVERY: B + 2 > UBLK_F_USER_RECOVERY|UBLK_F_USER_RECOVERY_REISSUE: C + 2 > > We can't easily change the userspace interface to allow independent > selection of one of {A, B, C} and one of {1, 2}, but we can refactor the > internal helpers which test for the flags. Replace the existing helpers > with the following set: > > ublk_nosrv_should_reissue_outstanding: tests for behavior C > ublk_nosrv_should_queue_io: tests for behavior B > ublk_nosrv_should_stop_dev: tests for behavior 1 > > Signed-off-by: Uday Shankar <ushankar@purestorage.com> > --- > drivers/block/ublk_drv.c | 55 +++++++++++++++++++++++++--------------- > 1 file changed, 34 insertions(+), 21 deletions(-) > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > index 2752a9afe9d4..e8cca58a71bc 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -652,22 +652,35 @@ static inline int ublk_queue_cmd_buf_size(struct ublk_device *ub, int q_id) > PAGE_SIZE); > } > > -static inline bool ublk_queue_can_use_recovery_reissue( > - struct ublk_queue *ubq) > +/* > + * Should I/O outstanding to the ublk server when it exits be reissued? > + * If not, outstanding I/O will get errors. > + */ > +static inline bool ublk_nosrv_should_reissue_outstanding(struct ublk_device *ub) > { > - return (ubq->flags & UBLK_F_USER_RECOVERY) && > - (ubq->flags & UBLK_F_USER_RECOVERY_REISSUE); > + return (ub->dev_info.flags & UBLK_F_USER_RECOVERY) && > + (ub->dev_info.flags & UBLK_F_USER_RECOVERY_REISSUE); > } > > -static inline bool ublk_queue_can_use_recovery( > - struct ublk_queue *ubq) > +/* > + * Should I/O issued while there is no ublk server queue? If not, I/O > + * issued while there is no ublk server will get errors. > + */ > +static inline bool ublk_nosrv_should_queue_io(struct ublk_device *ub) > { > - return ubq->flags & UBLK_F_USER_RECOVERY; > + return ub->dev_info.flags & UBLK_F_USER_RECOVERY; > } > > -static inline bool ublk_can_use_recovery(struct ublk_device *ub) > +/* > + * Should ublk devices be stopped (i.e. no recovery possible) when the > + * ublk server exits? If not, devices can be used again by a future > + * incarnation of a ublk server via the start_recovery/end_recovery > + * commands. > + */ > +static inline bool ublk_nosrv_should_stop_dev(struct ublk_device *ub) > { > - return ub->dev_info.flags & UBLK_F_USER_RECOVERY; > + return (!(ub->dev_info.flags & UBLK_F_USER_RECOVERY)) && > + (!(ub->dev_info.flags & UBLK_F_USER_RECOVERY_REISSUE)); > } > > static void ublk_free_disk(struct gendisk *disk) > @@ -1043,7 +1056,7 @@ static void __ublk_fail_req(struct ublk_queue *ubq, struct ublk_io *io, > { > WARN_ON_ONCE(io->flags & UBLK_IO_FLAG_ACTIVE); > > - if (ublk_queue_can_use_recovery_reissue(ubq)) > + if (ublk_nosrv_should_reissue_outstanding(ubq->dev)) > blk_mq_requeue_request(req, false); > else > ublk_put_req_ref(ubq, req); > @@ -1071,7 +1084,7 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq, > struct request *rq) > { > /* We cannot process this rq so just requeue it. */ > - if (ublk_queue_can_use_recovery(ubq)) > + if (ublk_nosrv_should_queue_io(ubq->dev)) I feel the name of ublk_nosrv_should_queue_io() is a bit misleading. The difference between ublk_queue_can_use_recovery() and ublk_queue_can_use_recovery_reissue() is clear, and both two need to queue ios actually in case of nosrv most times except for this one. However, looks your patch just tries to replace ublk_queue_can_use_recovery() with ublk_nosrv_should_queue_io(). > blk_mq_requeue_request(rq, false); > else > blk_mq_end_request(rq, BLK_STS_IOERR); > @@ -1216,10 +1229,10 @@ static enum blk_eh_timer_return ublk_timeout(struct request *rq) > struct ublk_device *ub = ubq->dev; > > if (ublk_abort_requests(ub, ubq)) { > - if (ublk_can_use_recovery(ub)) > - schedule_work(&ub->quiesce_work); > - else > + if (ublk_nosrv_should_stop_dev(ub)) The helper looks easy to follow, include the following conversions. > schedule_work(&ub->stop_work); > + else > + schedule_work(&ub->quiesce_work); > } > return BLK_EH_DONE; > } > @@ -1248,7 +1261,7 @@ static blk_status_t ublk_queue_rq(struct blk_mq_hw_ctx *hctx, > * Note: force_abort is guaranteed to be seen because it is set > * before request queue is unqiuesced. > */ > - if (ublk_queue_can_use_recovery(ubq) && unlikely(ubq->force_abort)) > + if (ublk_nosrv_should_queue_io(ubq->dev) && unlikely(ubq->force_abort)) > return BLK_STS_IOERR; I'd rather to not fetch ublk_device in fast io path since ublk is MQ device, and only the queue structure should be touched in fast io path, but it is fine to check device in any slow path. Thanks, Ming
On Tue, Jul 02, 2024 at 07:09:24PM +0800, Ming Lei wrote: > I feel the name of ublk_nosrv_should_queue_io() is a bit misleading. > > The difference between ublk_queue_can_use_recovery() and > ublk_queue_can_use_recovery_reissue() is clear, and > both two need to queue ios actually in case of nosrv most times > except for this one. Well yeah, this refactoring is all to set up for the final patch in this series. Taken in isolation, it might appear pointless. I'm open to suggestions for better names if you have them. > I'd rather to not fetch ublk_device in fast io path since ublk is MQ > device, and only the queue structure should be touched in fast io path, > but it is fine to check device in any slow path. Added a helper which checks the queue-local copy of the device flags, and used it in ublk_queue_rq in v2. Sorry for the delay; please take a look.
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 2752a9afe9d4..e8cca58a71bc 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -652,22 +652,35 @@ static inline int ublk_queue_cmd_buf_size(struct ublk_device *ub, int q_id) PAGE_SIZE); } -static inline bool ublk_queue_can_use_recovery_reissue( - struct ublk_queue *ubq) +/* + * Should I/O outstanding to the ublk server when it exits be reissued? + * If not, outstanding I/O will get errors. + */ +static inline bool ublk_nosrv_should_reissue_outstanding(struct ublk_device *ub) { - return (ubq->flags & UBLK_F_USER_RECOVERY) && - (ubq->flags & UBLK_F_USER_RECOVERY_REISSUE); + return (ub->dev_info.flags & UBLK_F_USER_RECOVERY) && + (ub->dev_info.flags & UBLK_F_USER_RECOVERY_REISSUE); } -static inline bool ublk_queue_can_use_recovery( - struct ublk_queue *ubq) +/* + * Should I/O issued while there is no ublk server queue? If not, I/O + * issued while there is no ublk server will get errors. + */ +static inline bool ublk_nosrv_should_queue_io(struct ublk_device *ub) { - return ubq->flags & UBLK_F_USER_RECOVERY; + return ub->dev_info.flags & UBLK_F_USER_RECOVERY; } -static inline bool ublk_can_use_recovery(struct ublk_device *ub) +/* + * Should ublk devices be stopped (i.e. no recovery possible) when the + * ublk server exits? If not, devices can be used again by a future + * incarnation of a ublk server via the start_recovery/end_recovery + * commands. + */ +static inline bool ublk_nosrv_should_stop_dev(struct ublk_device *ub) { - return ub->dev_info.flags & UBLK_F_USER_RECOVERY; + return (!(ub->dev_info.flags & UBLK_F_USER_RECOVERY)) && + (!(ub->dev_info.flags & UBLK_F_USER_RECOVERY_REISSUE)); } static void ublk_free_disk(struct gendisk *disk) @@ -1043,7 +1056,7 @@ static void __ublk_fail_req(struct ublk_queue *ubq, struct ublk_io *io, { WARN_ON_ONCE(io->flags & UBLK_IO_FLAG_ACTIVE); - if (ublk_queue_can_use_recovery_reissue(ubq)) + if (ublk_nosrv_should_reissue_outstanding(ubq->dev)) blk_mq_requeue_request(req, false); else ublk_put_req_ref(ubq, req); @@ -1071,7 +1084,7 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq, struct request *rq) { /* We cannot process this rq so just requeue it. */ - if (ublk_queue_can_use_recovery(ubq)) + if (ublk_nosrv_should_queue_io(ubq->dev)) blk_mq_requeue_request(rq, false); else blk_mq_end_request(rq, BLK_STS_IOERR); @@ -1216,10 +1229,10 @@ static enum blk_eh_timer_return ublk_timeout(struct request *rq) struct ublk_device *ub = ubq->dev; if (ublk_abort_requests(ub, ubq)) { - if (ublk_can_use_recovery(ub)) - schedule_work(&ub->quiesce_work); - else + if (ublk_nosrv_should_stop_dev(ub)) schedule_work(&ub->stop_work); + else + schedule_work(&ub->quiesce_work); } return BLK_EH_DONE; } @@ -1248,7 +1261,7 @@ static blk_status_t ublk_queue_rq(struct blk_mq_hw_ctx *hctx, * Note: force_abort is guaranteed to be seen because it is set * before request queue is unqiuesced. */ - if (ublk_queue_can_use_recovery(ubq) && unlikely(ubq->force_abort)) + if (ublk_nosrv_should_queue_io(ubq->dev) && unlikely(ubq->force_abort)) return BLK_STS_IOERR; if (unlikely(ubq->canceling)) { @@ -1469,10 +1482,10 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, ublk_cancel_cmd(ubq, io, issue_flags); if (need_schedule) { - if (ublk_can_use_recovery(ub)) - schedule_work(&ub->quiesce_work); - else + if (ublk_nosrv_should_stop_dev(ub)) schedule_work(&ub->stop_work); + else + schedule_work(&ub->quiesce_work); } } @@ -1577,7 +1590,7 @@ static void ublk_stop_dev(struct ublk_device *ub) mutex_lock(&ub->mutex); if (ub->dev_info.state == UBLK_S_DEV_DEAD) goto unlock; - if (ublk_can_use_recovery(ub)) { + if (ublk_nosrv_should_queue_io(ub)) { if (ub->dev_info.state == UBLK_S_DEV_LIVE) __ublk_quiesce_dev(ub); ublk_unquiesce_dev(ub); @@ -2674,7 +2687,7 @@ static int ublk_ctrl_start_recovery(struct ublk_device *ub, int i; mutex_lock(&ub->mutex); - if (!ublk_can_use_recovery(ub)) + if (ublk_nosrv_should_stop_dev(ub)) goto out_unlock; /* * START_RECOVERY is only allowd after: @@ -2725,7 +2738,7 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub, __func__, ub->dev_info.nr_hw_queues, header->dev_id); mutex_lock(&ub->mutex); - if (!ublk_can_use_recovery(ub)) + if (ublk_nosrv_should_stop_dev(ub)) goto out_unlock; if (ub->dev_info.state != UBLK_S_DEV_QUIESCED) {
ublk currently supports the following behaviors on ublk server exit: A: outstanding I/Os get errors, subsequently issued I/Os get errors B: outstanding I/Os get errors, subsequently issued I/Os queue C: outstanding I/Os get reissued, subsequently issued I/Os queue and the following behaviors for recovery of preexisting block devices by a future incarnation of the ublk server: 1: ublk devices stopped on ublk server exit (no recovery possible) 2: ublk devices are recoverable using start/end_recovery commands The userspace interface allows selection of combinations of these behaviors using flags specified at device creation time, namely: default behavior: A + 1 UBLK_F_USER_RECOVERY: B + 2 UBLK_F_USER_RECOVERY|UBLK_F_USER_RECOVERY_REISSUE: C + 2 We can't easily change the userspace interface to allow independent selection of one of {A, B, C} and one of {1, 2}, but we can refactor the internal helpers which test for the flags. Replace the existing helpers with the following set: ublk_nosrv_should_reissue_outstanding: tests for behavior C ublk_nosrv_should_queue_io: tests for behavior B ublk_nosrv_should_stop_dev: tests for behavior 1 Signed-off-by: Uday Shankar <ushankar@purestorage.com> --- drivers/block/ublk_drv.c | 55 +++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 21 deletions(-)