| Message ID | 20220824054744.77812-2-ZiyangZhang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ublk_drv: add USER_RECOVERY support | expand |
On Wed, Aug 24, 2022 at 01:47:36PM +0800, ZiyangZhang wrote: > This check is not atomic. So with recovery feature, ubq_daemon may be > updated simultaneously by recovery task. Instead, check 'current' is > safe here because 'current' never changes. > > Also add comment explaining this check, which is really important for > understanding recovery feature. > > Signed-off-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com> > --- > drivers/block/ublk_drv.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > index 6a4a94b4cdf4..c39b67d7133d 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -645,14 +645,22 @@ static inline void __ublk_rq_task_work(struct request *req) > struct ublk_device *ub = ubq->dev; > int tag = req->tag; > struct ublk_io *io = &ubq->ios[tag]; > - bool task_exiting = current != ubq->ubq_daemon || ubq_daemon_is_dying(ubq); > unsigned int mapped_bytes; > > pr_devel("%s: complete: op %d, qid %d tag %d io_flags %x addr %llx\n", > __func__, io->cmd->cmd_op, ubq->q_id, req->tag, io->flags, > ublk_get_iod(ubq, req->tag)->addr); > > - if (unlikely(task_exiting)) { > + /* > + * Task is exiting if either: > + * > + * (1) current != ubq_daemon. > + * io_uring_cmd_complete_in_task() tries to run task_work > + * in a workqueue if ubq_daemon(cmd's task) is PF_EXITING. > + * > + * (2) current->flags & PF_EXITING. > + */ > + if (unlikely(current != ubq->ubq_daemon || current->flags & PF_EXITING)) { Reviewed-by: Ming Lei <ming.lei@redhat.com> Thanks, Ming
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 6a4a94b4cdf4..c39b67d7133d 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -645,14 +645,22 @@ static inline void __ublk_rq_task_work(struct request *req) struct ublk_device *ub = ubq->dev; int tag = req->tag; struct ublk_io *io = &ubq->ios[tag]; - bool task_exiting = current != ubq->ubq_daemon || ubq_daemon_is_dying(ubq); unsigned int mapped_bytes; pr_devel("%s: complete: op %d, qid %d tag %d io_flags %x addr %llx\n", __func__, io->cmd->cmd_op, ubq->q_id, req->tag, io->flags, ublk_get_iod(ubq, req->tag)->addr); - if (unlikely(task_exiting)) { + /* + * Task is exiting if either: + * + * (1) current != ubq_daemon. + * io_uring_cmd_complete_in_task() tries to run task_work + * in a workqueue if ubq_daemon(cmd's task) is PF_EXITING. + * + * (2) current->flags & PF_EXITING. + */ + if (unlikely(current != ubq->ubq_daemon || current->flags & PF_EXITING)) { blk_mq_end_request(req, BLK_STS_IOERR); mod_delayed_work(system_wq, &ub->monitor_work, 0); return;
This check is not atomic. So with recovery feature, ubq_daemon may be updated simultaneously by recovery task. Instead, check 'current' is safe here because 'current' never changes. Also add comment explaining this check, which is really important for understanding recovery feature. Signed-off-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com> --- drivers/block/ublk_drv.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)