> break;
> case REQ_OP_READ:
> - ret = nvme_setup_rw(ns, req, cmd, nvme_cmd_read);
> + if (unlikely(req->cmd_flags & REQ_COPY))
> + nvme_setup_copy_read(ns, req);
> + else
> + ret = nvme_setup_rw(ns, req, cmd, nvme_cmd_read);
> break;
> case REQ_OP_WRITE:
> - ret = nvme_setup_rw(ns, req, cmd, nvme_cmd_write);
> + if (unlikely(req->cmd_flags & REQ_COPY))
> + ret = nvme_setup_copy_write(ns, req, cmd);
> + else
> + ret = nvme_setup_rw(ns, req, cmd, nvme_cmd_write);
Yikes. Overloading REQ_OP_READ and REQ_OP_WRITE with something entirely
different brings us back the horrors of the block layer 15 years ago.
Don't do that. Please add separate REQ_COPY_IN/OUT (or maybe
SEND/RECEIVE or whatever) methods.
> + /* setting copy limits */
> + if (blk_queue_flag_test_and_set(QUEUE_FLAG_COPY, q))
I don't understand this comment.
> +struct nvme_copy_token {
> + char *subsys;
> + struct nvme_ns *ns;
> + sector_t src_sector;
> + sector_t sectors;
> +};
Why do we need a subsys token? Inter-namespace copy is pretty crazy,
and not really anything we should aim for. But this whole token design
is pretty odd anyway. The only thing we'd need is a sequence number /
idr / etc to find an input and output side match up, as long as we
stick to the proper namespace scope.
> + if (unlikely((req->cmd_flags & REQ_COPY) &&
> + (req_op(req) == REQ_OP_READ))) {
> + blk_mq_start_request(req);
> + return BLK_STS_OK;
> + }
This really needs to be hiden inside of nvme_setup_cmd. And given
that other drivers might need similar handling the best way is probably
to have a new magic BLK_STS_* value for request started but we're
not actually sending it to hardware.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
