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.

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

Incorrect slba Values in DSM Commands for TRIM/DISCARD #165

Open
Choe-Jongin opened this issue Nov 19, 2024 · 1 comment
Open

Incorrect slba Values in DSM Commands for TRIM/DISCARD #165

Choe-Jongin opened this issue Nov 19, 2024 · 1 comment

Comments

@Choe-Jongin
Copy link
Contributor

Dear FEMU,

Currently, I am working on implementing deallocation via DSM commands for trim/discard operations. However, I have encountered an issue with the slba values in DSM commands.

When printing the slba for each DSM command, I noticed that it is always 0. For instance:

Deleting a 2GB file results in multiple DSM requests, each intended to deallocate a total of 4,194,312 blocks. These requests successfully propagate to the nvme_dsm function in nvme-io.c, but every DSM command shows slba = 0.
Similarly, when deleting two distinct 2GB files, the deallocated slba ranges for both files are reported as the same, starting at slba = 0.
It seems that the slba values in DSM commands are incorrectly recorded. I am uncertain whether this issue lies with FEMU itself or stems from Linux or the file system layer. I have tested this with both ext4 and xfs file systems, and the behavior is consistent across both.

Resolving the slba issue would make supporting trim and discard operations straightforward. Could you please advise whether this might be an issue with FEMU or if I should investigate further into the Linux kernel or file system behavior?

Thank you for your time and support.

@Choe-Jongin
Copy link
Contributor Author

There is an issue in the following code:

NvmeDsmRange *range = g_malloc0(sizeof(NvmeDsmRange) * nr);

if (dma_write_prp(n, (uint8_t *)range, sizeof(range), prp1, prp2)) {
//                                            ^ THIS!!
    nvme_set_error_page(n, req->sq->sqid, cmd->cid, NVME_INVALID_FIELD,
                        offsetof(NvmeCmd, dptr.prp1), 0, ns->id);
    g_free(range);
    return;
}

The problem is that sizeof(range) is used when reading the range. Since range is a pointer, sizeof(range) returns the size of a pointer (e.g., 8 bytes on a 64-bit system), not the actual size of the NvmeDsmRange structure.

As a result, only 8 bytes are read, but the slba field of the NvmeDsmRange structure occupies the 8–15 byte range. This causes incomplete or incorrect data to be read.

image

To fix this, sizeof(range) should be replaced with sizeof(NvmeDsmRange) * nr, ensuring that the correct amount of data for nr ranges is read. The corrected code is as follows:

NvmeDsmRange *range = g_malloc0(sizeof(NvmeDsmRange) * nr);

if (dma_write_prp(n, (uint8_t *)range, sizeof(NvmeDsmRange) * nr, prp1, prp2)) {
    nvme_set_error_page(n, req->sq->sqid, cmd->cid, NVME_INVALID_FIELD,
                        offsetof(NvmeCmd, dptr.prp1), 0, ns->id);
    g_free(range);
    return;
}

This ensures that dma_write_prp correctly reads the nr number of NvmeDsmRange structures.

I will full reques the fixed version soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant