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

update the Expected Logical Block Application Tag and Reference Tag in nvme copy command #587

Closed
wants to merge 3 commits into from
Closed

update the Expected Logical Block Application Tag and Reference Tag in nvme copy command #587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b12c600
Improve the copy command based on the nvme documents
zwxdg Mar 1, 2023
63a5bce
Merge branch 'linux-nvme:master' into master
zwxdg Mar 3, 2023
2531c6d
update the Expected Logical Block Application Tag and Reference Tag i…
zwxdg Mar 1, 2023
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
4 changes: 2 additions & 2 deletions src/nvme/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4640,8 +4640,8 @@ struct nvme_copy_range {
__le16 nlb;
__u8 rsvd18[6];
__le32 eilbrt;
__le16 elbatm;
__le16 elbat;
__le16 elbatm;
};

/**
Expand All @@ -4661,8 +4661,8 @@ struct nvme_copy_range_f1 {
__le16 nlb;
__u8 rsvd18[8];
__u8 elbt[10];
__le16 elbatm;
__le16 elbat;
__le16 elbatm;
};

/**
Expand Down
7 changes: 5 additions & 2 deletions src/nvme/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,14 +403,17 @@ 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

@bjpaupor bjpaupor Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the issue here is that this stores the EILBRT values in big-endian order, like the linked PR which was closed.

Storing them in little-endian order requires the Storage Tag Size and Protection Information Format to determine where the EILBRT should start in the shared range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since both EILBRT and ELBST values are store in big-endian order, and the device software know the Storage Tag Size and what kind of Protection Information Format used, so i think the location of EILBRT in the shared range is handle by device software, as is the ELBST.
For nvme-cli，we only need to deliver EILBRT and ELBST in big-end mode to the device.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since both EILBRT and ELBST values are store in big-endian order

Most of the spec is using little endian and I can't find anything EILBRT and ELBST being big endian. The EILBRT and ELBST definition is, let's say a 'challenging' definition. Where do you find the info on big-endian?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
in section 5.2.1 Protection Information Formats. for example, the screenshot show the 16b guard protection information format, EILBRT and ELBST re store in big-endian order

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I didn't look at the Figure :)

I think you should use the be64 helpers as the argument type for eilbert is u64 which is in host endian.

But as I said pointed out, this change breaks the API. We can't just change data structures in this way. This was the reason why I didn't merge #455.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'd thought that the MSB/LSB in that figure might be "Most/Least Significant Bits" rather than defining each of these fields as big-endian byte ordering, particularly based on figure 122 in the next section on the shared space and the consistent mention of significant bits rather than bytes:

image

Definitely see how it could be describing the byte-ordering, but it feels odd without a more explicit call-out like NGUID or EUI64. Worth mentioning this would mean similar ordering changes are likely needed for the shared field and for application tag/guard fields across all IO commands.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, but also there's no requirement from the spec that the tags are divisible into bytes. Setting the reference tag in big-endian order should get the right bit ordering and properly handle cases like the spec example with a 30-bit reference tag.

Changes look good to me with fixing the ELBATM types location, the bit-ordering fix could be addressed more broadly but this works for copy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the feedback. I looked again with fresh eyes I figured that the github webui confused me. So the two commits should be obviously merged and the final result is just:

--- a/src/nvme/types.h
+++ b/src/nvme/types.h
@@ -4640,8 +4640,8 @@ struct nvme_copy_range {
        __le16                  nlb;
        __u8                    rsvd18[6];
        __le32                  eilbrt;
-       __le16                  elbatm;
        __le16                  elbat;
+       __le16                  elbatm;
 };
 
 /**
@@ -4661,8 +4661,8 @@ struct nvme_copy_range_f1 {
        __le16                  nlb;
        __u8                    rsvd18[8];
        __u8                    elbt[10];
-       __le16                  elbatm;
        __le16                  elbat;
+       __le16                  elbatm;
 };

which is okay I think to merge.

copy[i].elbt[1] = 0;
copy[i].elbt[0] = 0;
}
}

Expand Down