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

Enable RAID support for FDP IO path #319

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arungeorge83
Copy link

This PR enables RAID in the FDP io path. RAID support was disabled in the original FDP support PR, since it involved a few more changes in the IO Path code. It also involves handling a few scenarios like one of the devices in the RAID does not support FDP etc. This PR contain the code changes for the same.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 7, 2024
// As of now, only one FDP enabled Device is supported
static constexpr uint16_t kDefaultFdpIdx = 0u;
// Map of file descriptors to FdpNvme device objects
const std::unordered_map<int, std::shared_ptr<FdpNvme>> fdpNvmeDevs_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a reference?
I thought all the AsyncIoContext would share the same map (vector before this change)? Or we actually want to make a copy here?

Choose a reason for hiding this comment

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

Should this be a reference? I thought all the AsyncIoContext would share the same map (vector before this change)? Or we actually want to make a copy here?

yes, you are right, we can use reference here.

return fdpNvmeVec_[kDefaultFdpIdx]->allocateFdpHandle();
if (fdpNvmeDevs_.size() > 0) {
auto fdpHandle = -1;
//Ensuring that same FDP placement handle is allocated for all FdpNvme
Copy link
Contributor

Choose a reason for hiding this comment

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

Asking to learn: I thought it would be fine if one RAID write gets written to different handles in different device. But because we don't want to keep track of a vector of different handles, we just make sure we have the same handle for all devices?

Choose a reason for hiding this comment

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

Asking to learn: I thought it would be fine if one RAID write gets written to different handles in different device. But because we don't want to keep track of a vector of different handles, we just make sure we have the same handle for all devices?

Yes, we wanted to avoid the code complexity and use a simple scheme of same handle for all the devices for a given placement handle. Having ability to write to different handles for different RAID devices could be nice but need to design carefully, and can be added later.

Summary:
This enables RAID0 in fdp io path by spliting io across all devices.

Signed-off-by: Vikash Kumar <[email protected]>
@arungeorge83 arungeorge83 force-pushed the fdp/fdp_raid_support branch from 1a07b4e to 40f1b33 Compare June 26, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants