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

FR: define nvme-mi log truncate size in header #478

Open
drakedog2008 opened this issue Sep 23, 2022 · 6 comments
Open

FR: define nvme-mi log truncate size in header #478

drakedog2008 opened this issue Sep 23, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@drakedog2008
Copy link
Contributor

Hi @jk-ozlabs ,

Can we define the size in header file. So the client doesn't need to define their own and conflict in case of future change.

https://github.com/linux-nvme/libnvme/blob/master/src/nvme/mi.c#L502

@jk-ozlabs
Copy link
Collaborator

This should be transparent to the client; it's just the size that is transferred in each chunk. Is there a need to know this chunk size outside of the library?

@drakedog2008
Copy link
Contributor Author

Given all nvme calls are blocking for libnvme(mi), the client need to schedule the tasks itself.

One scenario is that the we need the thermal info for at most 1 second delay for pid loop. The log will be definitely large to block so truncate will be done by client. It is good (but not necessary) to know the payload size of the lib implementation so the client could better optimize the performance.

@jk-ozlabs
Copy link
Collaborator

A couple of comments/questions:

One scenario is that the we need the thermal info for at most 1 second delay for pid loop

OK, that's reasonable

The log will be definitely large to block so truncate will be done by client

Not sure I understand this by "client", do you mean the caller of the libnvme API?

In this case, wouldn't it be much better for the truncate to be done by libnvme instead? Otherwise we're transferring data from the drive, but then throwing part of that away. If there's a specific range of bytes in the Log Page that you need for this regular data, we can request just that range - that way, we can minimise the amount of data going over the (relatively slow) i2c bus.

This may need a new function to get a partial chunk of log page data - like we have for identify already. I can add that if you like.

But on to this feature request: the chunk size is really an implementation detail of libnvme - if we can avoid exposing it, I would prefer that.

@drakedog2008
Copy link
Contributor Author

Not sure I understand this by "client", do you mean the caller of the libnvme API?

Yes.

In this case, wouldn't it be much better for the truncate to be done by libnvme instead? Otherwise we're transferring data from the drive, but then throwing part of that away. If there's a specific range of bytes in the Log Page that you need for this regular data, we can request just that range - that way, we can minimise the amount of data going over the (relatively slow) i2c bus.

Given the libnvme is synchronous api, the internal truncate within the libnvme won't unblock the caller. So for the high priorities with concurrent task scenario , the client has to truncate the transfer by itself to do scheduling and context(task) switch. The internal chuck size may be a reasonable parameter for the client to determine the client side trunc size.

This may need a new function to get a partial chunk of log page data - like we have for identify already. I can add that if you like.

yes. offset is required for all large data transfer CMD, log page is especially the case.

@igaw igaw added the enhancement New feature or request label Oct 27, 2022
@igaw
Copy link
Collaborator

igaw commented Jul 3, 2024

Good to close? Or is there anything in the works?

@jk-ozlabs
Copy link
Collaborator

There's still an item to add a partial log page transfer. @drakedog2008 , I'm assuming using the lpo-based chunking isn't suitable for this case?

If so, I can rename this PR to something more suitable.

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

No branches or pull requests

3 participants