| Message ID | 20250416-ublk_task_per_io-v5-4-9261ad7bff20@purestorage.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ublk: decouple server threads from hctxs | expand |
On Wed, Apr 16, 2025 at 12:46 PM Uday Shankar <ushankar@purestorage.com> wrote: > > We now allow multiple tasks to operate on I/Os belonging to the same > queue concurrently. This means that any writes to ublk_queue in the I/O > path are potential sources of data races. Try to prevent these by > marking ublk_queue pointers as const in ublk_handle_need_get_data. Also > move a bit more of the NEED_GET_DATA-specific logic into > ublk_handle_need_get_data, to make the pattern in __ublk_ch_uring_cmd > more uniform. > > Suggested-by: Ming Lei <ming.lei@redhat.com> > Signed-off-by: Uday Shankar <ushankar@purestorage.com> Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
On Wed, Apr 16, 2025 at 01:46:08PM -0600, Uday Shankar wrote: > We now allow multiple tasks to operate on I/Os belonging to the same > queue concurrently. This means that any writes to ublk_queue in the I/O > path are potential sources of data races. Try to prevent these by > marking ublk_queue pointers as const in ublk_handle_need_get_data. Also > move a bit more of the NEED_GET_DATA-specific logic into > ublk_handle_need_get_data, to make the pattern in __ublk_ch_uring_cmd > more uniform. > > Suggested-by: Ming Lei <ming.lei@redhat.com> > Signed-off-by: Uday Shankar <ushankar@purestorage.com> > --- > drivers/block/ublk_drv.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > index 215ab45b00e10150e58d7f5ea5b5d13e40a1aa79..5f9679c03305576bee586388cab82a6ea5472f8b 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -1291,7 +1291,7 @@ static void ublk_cmd_tw_cb(struct io_uring_cmd *cmd, > ublk_dispatch_req(ubq, pdu->req, issue_flags); > } > > -static void ublk_queue_cmd(struct ublk_queue *ubq, struct request *rq) > +static void ublk_queue_cmd(const struct ublk_queue *ubq, struct request *rq) > { > struct io_uring_cmd *cmd = ubq->ios[rq->tag].cmd; > struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd); > @@ -1813,15 +1813,6 @@ static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq) > mutex_unlock(&ub->mutex); > } > > -static void ublk_handle_need_get_data(struct ublk_device *ub, int q_id, > - int tag) > -{ > - struct ublk_queue *ubq = ublk_get_queue(ub, q_id); > - struct request *req = blk_mq_tag_to_rq(ub->tag_set.tags[q_id], tag); > - > - ublk_queue_cmd(ubq, req); > -} > - > static inline int ublk_check_cmd_op(u32 cmd_op) > { > u32 ioc_type = _IOC_TYPE(cmd_op); > @@ -1933,6 +1924,20 @@ static int ublk_commit_and_fetch(const struct ublk_queue *ubq, > return 0; > } > > +static int ublk_get_data(const struct ublk_queue *ubq, struct ublk_io *io, > + struct io_uring_cmd *cmd, > + const struct ublksrv_io_cmd *ub_cmd, > + struct request *req) > +{ > + if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV)) > + return -EINVAL; > + > + ublk_fill_io_cmd(io, cmd, ub_cmd->addr); > + ublk_queue_cmd(ubq, req); > + > + return 0; > +} > + > static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd, > unsigned int issue_flags, > const struct ublksrv_io_cmd *ub_cmd) > @@ -2026,10 +2031,11 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd, > goto out; > break; > case UBLK_IO_NEED_GET_DATA: > - if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV)) > + ret = ublk_get_data( > + ubq, io, cmd, ub_cmd, > + blk_mq_tag_to_rq(ub->tag_set.tags[ub_cmd->q_id], tag)); Same with patch 2, moving request retrieval into ublk_get_data() could be cleaner and helpful for switching to store request pointer into `ublk_io`. Otherwise, looks nice cleanup. Thanks, Ming
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 215ab45b00e10150e58d7f5ea5b5d13e40a1aa79..5f9679c03305576bee586388cab82a6ea5472f8b 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -1291,7 +1291,7 @@ static void ublk_cmd_tw_cb(struct io_uring_cmd *cmd, ublk_dispatch_req(ubq, pdu->req, issue_flags); } -static void ublk_queue_cmd(struct ublk_queue *ubq, struct request *rq) +static void ublk_queue_cmd(const struct ublk_queue *ubq, struct request *rq) { struct io_uring_cmd *cmd = ubq->ios[rq->tag].cmd; struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd); @@ -1813,15 +1813,6 @@ static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq) mutex_unlock(&ub->mutex); } -static void ublk_handle_need_get_data(struct ublk_device *ub, int q_id, - int tag) -{ - struct ublk_queue *ubq = ublk_get_queue(ub, q_id); - struct request *req = blk_mq_tag_to_rq(ub->tag_set.tags[q_id], tag); - - ublk_queue_cmd(ubq, req); -} - static inline int ublk_check_cmd_op(u32 cmd_op) { u32 ioc_type = _IOC_TYPE(cmd_op); @@ -1933,6 +1924,20 @@ static int ublk_commit_and_fetch(const struct ublk_queue *ubq, return 0; } +static int ublk_get_data(const struct ublk_queue *ubq, struct ublk_io *io, + struct io_uring_cmd *cmd, + const struct ublksrv_io_cmd *ub_cmd, + struct request *req) +{ + if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV)) + return -EINVAL; + + ublk_fill_io_cmd(io, cmd, ub_cmd->addr); + ublk_queue_cmd(ubq, req); + + return 0; +} + static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags, const struct ublksrv_io_cmd *ub_cmd) @@ -2026,10 +2031,11 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd, goto out; break; case UBLK_IO_NEED_GET_DATA: - if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV)) + ret = ublk_get_data( + ubq, io, cmd, ub_cmd, + blk_mq_tag_to_rq(ub->tag_set.tags[ub_cmd->q_id], tag)); + if (ret) goto out; - ublk_fill_io_cmd(io, cmd, ub_cmd->addr); - ublk_handle_need_get_data(ub, ub_cmd->q_id, ub_cmd->tag); break; default: goto out;
We now allow multiple tasks to operate on I/Os belonging to the same queue concurrently. This means that any writes to ublk_queue in the I/O path are potential sources of data races. Try to prevent these by marking ublk_queue pointers as const in ublk_handle_need_get_data. Also move a bit more of the NEED_GET_DATA-specific logic into ublk_handle_need_get_data, to make the pattern in __ublk_ch_uring_cmd more uniform. Suggested-by: Ming Lei <ming.lei@redhat.com> Signed-off-by: Uday Shankar <ushankar@purestorage.com> --- drivers/block/ublk_drv.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)