Re: [PATCH 6/6] ublk: lift need_map check out of ublk_{,un}map_io()

[Caleb Sander Mateos]

On Tue, Jul 28, 2026 at 6:30 PM Caleb Sander Mateos
<csander@xxxxxxxxxxxxxxx> wrote:
>
> ublk_map_io() and ublk_unmap_io() are no-ops for ublk devices that
> enable user copy or zero copy. However, the implementation is a bit
> convoluted, returning the full request data length and relying on the
> caller to check the return value against the request length.
> UBLK_F_SHMEM_ZC recently added branches to skip the ublk_{,un}map_io()
> call for I/Os using a shared-memory buffer. This is a more logical place
> for the need_map check, so move it there from ublk_{,un}map_io().
>
> Checking need_map early also avoids the expensive pointer-chasing for
> the ublk_iod_is_shmem_zc() check in __ublk_complete_rq() in the common
> case of a ublk device using user copy or zero copy.
>
> Signed-off-by: Caleb Sander Mateos <csander@xxxxxxxxxxxxxxx>
> ---
>  drivers/block/ublk_drv.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 1da45e382253..d4443a43229e 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1464,19 +1464,15 @@ static inline bool ublk_need_unmap_req(const struct request *req)
>  {
>         return blk_rq_has_data(req) &&
>                (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN);
>  }
>
> -static unsigned int ublk_map_io(const struct ublk_queue *ubq,
> -                               const struct request *req,
> +static unsigned int ublk_map_io(const struct request *req,
>                                 const struct ublk_io *io)
>  {
>         const unsigned int rq_bytes = blk_rq_bytes(req);
>
> -       if (!ublk_need_map_io(ubq))
> -               return rq_bytes;
> -
>         /*
>          * no zero copy, we delay copy WRITE request data into ublksrv
>          * context and the big benefit is that pinning pages in current
>          * context is pretty fast, see ublk_pin_user_pages
>          */
> @@ -1488,19 +1484,15 @@ static unsigned int ublk_map_io(const struct ublk_queue *ubq,
>                 return ublk_copy_user_pages(req, 0, &iter, dir);
>         }
>         return rq_bytes;
>  }
>
> -static unsigned int ublk_unmap_io(bool need_map,
> -               const struct request *req,
> +static unsigned int ublk_unmap_io(const struct request *req,
>                 const struct ublk_io *io)
>  {
>         const unsigned int rq_bytes = blk_rq_bytes(req);
>
> -       if (!need_map)
> -               return rq_bytes;
> -
>         if (ublk_need_unmap_req(req)) {
>                 struct iov_iter iter;
>                 const int dir = ITER_SOURCE;
>
>                 import_ubuf(dir, u64_to_user_ptr(io->buf.addr), io->res, &iter);
> @@ -1591,15 +1583,16 @@ static inline void __ublk_complete_rq(struct request *req, struct ublk_io *io,
>         if (req_op(req) != REQ_OP_READ && req_op(req) != REQ_OP_WRITE &&
>             req_op(req) != REQ_OP_DRV_IN)
>                 goto exit;
>
>         /* shmem zero copy: no data to unmap, pages already shared */
> -       if (ublk_iod_is_shmem_zc(req->mq_hctx->driver_data, req->tag))
> +       if (!need_map ||
> +           ublk_iod_is_shmem_zc(req->mq_hctx->driver_data, req->tag))

Realized the ublk_need_unmap_req() check in ublk_unmap_io() can also
be combined with the req_op(req) check above. Will update in v2.

Best,
Caleb

>                 goto exit;
>
>         /* for READ request, writing data in iod->addr to rq buffers */
> -       unmapped_bytes = ublk_unmap_io(need_map, req, io);
> +       unmapped_bytes = ublk_unmap_io(req, io);
>
>         /*
>          * Extremely impossible since we got data filled in just before
>          *
>          * Re-read simply for this unlikely case.
> @@ -1755,14 +1748,14 @@ static bool ublk_start_io(const struct ublk_queue *ubq, struct request *req,
>                           struct ublk_io *io)
>  {
>         unsigned mapped_bytes;
>
>         /* shmem zero copy: skip data copy, pages already shared */
> -       if (ublk_iod_is_shmem_zc(ubq, req->tag))
> +       if (!ublk_need_map_io(ubq) || ublk_iod_is_shmem_zc(ubq, req->tag))
>                 return true;
>
> -       mapped_bytes = ublk_map_io(ubq, req, io);
> +       mapped_bytes = ublk_map_io(req, io);
>
>         /* partially mapped, update io descriptor */
>         if (unlikely(mapped_bytes != blk_rq_bytes(req))) {
>                 /*
>                  * Nothing mapped, retry until we succeed.
> --
> 2.54.0
>