Re: [PATCH 4/5] nvme: split out metadata vs non metadata end_io uring_cmd completions

Jens Axboe
2022-09-23T20:53:07Z

On 9/23/22 9:21 AM, Christoph Hellwig wrote:
>> + union {
>> + struct {
>> + void *meta; /* kernel-resident buffer */
>> + void __user *meta_buffer;
>> + };
>> + struct {
>> + u32 nvme_flags;
>> + u32 nvme_status;
>> + u64 result;
>> + };
>> + };
>
> Without naming the arms of the union this is becoming a bit too much
> of a mess..
>
>> +static void nvme_uring_task_cb(struct io_uring_cmd *ioucmd)
>> +{
>> + struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
>> + int status;
>> +
>> + if (pdu->nvme_flags & NVME_REQ_CANCELLED)
>> + status = -EINTR;
>> + else
>> + status = pdu->nvme_status;
>
> If you add a signed int field you only need one field instead of
> two in the pdu for this (the nvme status is only 15 bits anyway).

For both of these, how about we just simplify like below? I think
at that point it's useless to name them anyway.

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 25f2f6df1602..6f955984ca14 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -350,16 +350,13 @@ struct nvme_uring_cmd_pdu {
struct request *req;
};
u32 meta_len;
+ u32 nvme_status;
union {
struct {
void *meta; /* kernel-resident buffer */
void __user *meta_buffer;
};
- struct {
- u32 nvme_flags;
- u32 nvme_status;
- u64 result;
- };
+ u64 result;
};
};

@@ -396,17 +393,11 @@ static void nvme_uring_task_meta_cb(struct io_uring_cmd *ioucmd)
static void nvme_uring_task_cb(struct io_uring_cmd *ioucmd)
{
struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
- int status;
-
- if (pdu->nvme_flags & NVME_REQ_CANCELLED)
- status = -EINTR;
- else
- status = pdu->nvme_status;

if (pdu->bio)
blk_rq_unmap_user(pdu->bio);

- io_uring_cmd_done(ioucmd, status, pdu->result);
+ io_uring_cmd_done(ioucmd, pdu->nvme_status, pdu->result);
}

static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
@@ -417,8 +408,10 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
void *cookie = READ_ONCE(ioucmd->cookie);

req->bio = pdu->bio;
- pdu->nvme_flags = nvme_req(req)->flags;
- pdu->nvme_status = nvme_req(req)->status;
+ if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
+ pdu->nvme_status = -EINTR;
+ else
+ pdu->nvme_status = nvme_req(req)->status;
pdu->result = le64_to_cpu(nvme_req(req)->result.u64);

/*

--
Jens Axboe