Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libnvme: Fix the Reference Tag in copy command #455

Closed
wants to merge 1 commit into from
Closed

libnvme: Fix the Reference Tag in copy command #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/nvme/api-types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,8 +666,6 @@ struct nvme_dsm_args {
* @prinfow: Protection information field for write
* @dtype: Directive type
* @format: Descriptor format
* @ilbrt_u64: Initial logical block reference tag - 8 byte
* version required for enhanced protection info
*/
struct nvme_copy_args {
__u64 sdlba;
Expand All @@ -677,7 +675,7 @@ struct nvme_copy_args {
int fd;
__u32 timeout;
__u32 nsid;
__u32 ilbrt;
__u64 ilbrt;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the API

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one more patch for nvme-cli to fix this issue. In that patch, this renamed ilbrt variable is handled. I hope the API will not break once it is merged.

Copy link
Collaborator

@igaw igaw Aug 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far I can tell, libnvme is not just used by nvme-cli. This is why I keep repeating we can't break the API for the 1.x series of the library. For 2.x sure, I don't have any problems to modify the API.

BTW, I would suggest to open an in issue with the exact work description and queue it up for the 2.x library. I fear if we don't write it down, this will be lost.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @igaw
I missed to mention a point.
In struct nvme_copy_args, ilbrt and ilbrt_u64 are used to store the reference tag values of 32-bit and 64-bit respectively. The ilbrt_u64 is only assigned with cfg.ilbrt value in nvme-cli. Whereas ilbrt is not assigned with any value. But, both the variables are handled in nvme_copy() libnvme. It seems fishy for me. That's why I tried to use single ilbrt instead of two variable with different data type.

int lr;
int fua;
__u16 nr;
Expand All @@ -688,7 +686,6 @@ struct nvme_copy_args {
__u8 prinfow;
__u8 dtype;
__u8 format;
__u64 ilbrt_u64;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this one too.

};

/**
Expand Down
20 changes: 9 additions & 11 deletions src/nvme/ioctl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1718,11 +1718,9 @@ int nvme_dsm(struct nvme_dsm_args *args)

int nvme_copy(struct nvme_copy_args *args)
{
const size_t size_v1 = sizeof_args(struct nvme_copy_args, format, __u64);
const size_t size_v2 = sizeof_args(struct nvme_copy_args, ilbrt_u64, __u64);
__u32 cdw3, cdw12, cdw14, data_len;

if (args->args_size < size_v1 || args->args_size > size_v2) {
if (args->args_size < sizeof(*args)) {
errno = EINVAL;
return -1;
}
Expand All @@ -1732,19 +1730,19 @@ int nvme_copy(struct nvme_copy_args *args)
((args->prinfow & 0xf) << 26) | ((args->fua & 0x1) << 30) |
((args->lr & 0x1) << 31);

if (args->args_size == size_v1) {
if (args->format == 0) {
cdw3 = 0;
cdw14 = args->ilbrt;
data_len = args->nr * sizeof(struct nvme_copy_range);
} else if (args->format == 1) {
cdw3 = (args->ilbrt >> 32) & 0xffffffff;
cdw14 = args->ilbrt & 0xffffffff;
data_len = args->nr * sizeof(struct nvme_copy_range_f1);
} else {
cdw3 = (args->ilbrt_u64 >> 32) & 0xffffffff;
cdw14 = args->ilbrt_u64 & 0xffffffff;
errno = EINVAL;
return -1;
}

if (args->format == 1)
data_len = args->nr * sizeof(struct nvme_copy_range_f1);
else
data_len = args->nr * sizeof(struct nvme_copy_range);

struct nvme_passthru_cmd cmd = {
.opcode = nvme_cmd_copy,
.nsid = args->nsid,
Expand Down
6 changes: 4 additions & 2 deletions src/nvme/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,14 +401,16 @@ void nvme_init_copy_range_f1(struct nvme_copy_range_f1 *copy, __u16 *nlbs,
__u64 *slbas, __u64 *eilbrts, __u32 *elbatms,
__u32 *elbats, __u16 nr)
{
int i;
int i, j;

for (i = 0; i < nr; i++) {
copy[i].nlb = cpu_to_le16(nlbs[i]);
copy[i].slba = cpu_to_le64(slbas[i]);
copy[i].elbt[2] = cpu_to_le64(eilbrts[i]);
copy[i].elbatm = cpu_to_le16(elbatms[i]);
copy[i].elbat = cpu_to_le16(elbats[i]);

for (j = 0; j < 8; j++)
copy[i].elbt[9 - j] = (eilbrts[i] >> (8 * j)) & 0xff;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this just a more complicated way of storing the eilbrt little-endian order in the last 8 bytes of elbt (as is removed above)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have verified the copy command with QEMU emulated NVMe device. On using previous logic, the controller received the eilbrt value as zero via format1. Hence, I stored the value byte by byte. Here observed the controller is receiving the appropriate value.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which version of the nvme-cli and libnvme did you use? This might be caused by my brown bag release where I
crippled the nvme_init_copy_range_f1 function. The result was, calling nvme_init_copy_range_f1 was a no-op.

linux-nvme/nvme-cli@fd582b0

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always I use the latest version of nvme-cli and libnvme. The issue was observed even before the above mentioned changes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would have been too easy...

}
}

Expand Down