-
Notifications
You must be signed in to change notification settings - Fork 46
feat: add support for multi-stream SSDs (such as FDP SSDs) #92
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -473,6 +473,7 @@ void __FhgfsInode_initOpenIOInfo(FhgfsInode* this, FhgfsInodeFileHandle* fileHan | |
|
|
||
| outIOInfo->userID = i_uid_read(&this->vfs_inode); | ||
| outIOInfo->groupID = i_gid_read(&this->vfs_inode); | ||
| outIOInfo->writeHint = this->vfs_inode.i_write_hint; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: The field Please add a feature-detect check in For example I presume the client should fall back to RWH_WRITE_LIFE_NOT_SET when the field is unavailable:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes made as suggested. Thanks for your feedback! |
||
| #ifdef BEEGFS_NVFS | ||
| outIOInfo->nvfs = false; | ||
| #endif | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,7 @@ struct RemotingIOInfo | |
| #ifdef BEEGFS_NVFS | ||
| bool nvfs; | ||
| #endif | ||
| uint64_t writeHint; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: we need to ensure the |
||
| }; | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| #define WRITELOCALFILEMSG_FLAG_BUDDYMIRROR_SECOND 16 /* secondary of group, otherwise primary */ | ||
| #define WRITELOCALFILEMSG_FLAG_BUDDYMIRROR_FORWARD 32 /* forward msg to secondary */ | ||
|
|
||
| #define RW_HINT_INVALID 0xFF | ||
| class WriteLocalFileMsgBase | ||
| { | ||
| public: | ||
|
|
@@ -21,7 +22,7 @@ class WriteLocalFileMsgBase | |
| */ | ||
| WriteLocalFileMsgBase(const NumNodeID clientNumID, const char* fileHandleID, | ||
| const uint16_t targetID, const PathInfo* pathInfo, const unsigned accessFlags, | ||
| const int64_t offset, const int64_t count) | ||
| const int64_t offset, const int64_t count, const unsigned writeHint) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: this uses |
||
| { | ||
| this->clientNumID = clientNumID; | ||
|
|
||
|
|
@@ -36,6 +37,7 @@ class WriteLocalFileMsgBase | |
|
|
||
| this->offset = offset; | ||
| this->count = count; | ||
| this->writeHint = writeHint; | ||
| } | ||
|
|
||
| WriteLocalFileMsgBase() {} | ||
|
|
@@ -57,7 +59,8 @@ class WriteLocalFileMsgBase | |
| % serdes::rawString(obj->fileHandleID, obj->fileHandleIDLen, 4) | ||
| % obj->clientNumID | ||
| % serdes::backedPtr(obj->pathInfoPtr, obj->pathInfo) | ||
| % obj->targetID; | ||
| % obj->targetID | ||
| % obj->writeHint; | ||
| } | ||
|
|
||
| protected: | ||
|
|
@@ -71,7 +74,7 @@ class WriteLocalFileMsgBase | |
|
|
||
| uint32_t userID; | ||
| uint32_t groupID; | ||
|
|
||
| uint64_t writeHint; | ||
| // for serialization | ||
| const PathInfo* pathInfoPtr; | ||
|
|
||
|
|
@@ -126,6 +129,11 @@ class WriteLocalFileMsgBase | |
| return groupID; | ||
| } | ||
|
|
||
| uint64_t getWriteHint() const | ||
| { | ||
| return writeHint; | ||
| } | ||
|
|
||
| void setUserdataForQuota(unsigned userID, unsigned groupID) | ||
| { | ||
| this->userID = userID; | ||
|
|
@@ -162,9 +170,9 @@ class WriteLocalFileMsg : public WriteLocalFileMsgBase, public NetMessageSerdes< | |
| */ | ||
| WriteLocalFileMsg(const NumNodeID clientNumID, const char* fileHandleID, | ||
| const uint16_t targetID, const PathInfo* pathInfo, const unsigned accessFlags, | ||
| const int64_t offset, const int64_t count) : | ||
| const int64_t offset, const int64_t count, const uint64_t writeHint = RW_HINT_INVALID) : | ||
| WriteLocalFileMsgBase(clientNumID, fileHandleID, targetID, pathInfo, accessFlags, | ||
| offset, count), | ||
| offset, count, writeHint), | ||
| BaseType(NETMSGTYPE_WriteLocalFile) {} | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -549,7 +549,7 @@ bool ChunkStore::mkdirChunkDirPath(int targetFD, const Path* chunkDirPath, bool | |
| } | ||
|
|
||
| std::pair<FhgfsOpsErr, int> ChunkStore::openAndChown(const int targetFD, const std::string& path, | ||
| const int openFlags, const SessionQuotaInfo& quota) | ||
| const int openFlags, const SessionQuotaInfo& quota, uint64_t writeHint) | ||
| { | ||
| // if we aren't using quota, we don't care about the file owner at all and may simply create the | ||
| // file if it does exist (and if openFlags requests it). | ||
|
|
@@ -583,6 +583,12 @@ std::pair<FhgfsOpsErr, int> ChunkStore::openAndChown(const int targetFD, const s | |
| return {FhgfsOpsErrTk::fromSysErr(errno), -1}; | ||
| } | ||
|
|
||
| int r = fcntl(fd, F_SET_RW_HINT, &writeHint); | ||
| if (r < 0) { | ||
| LOG(GENERAL, ERR, "Failed to set writeHint.", | ||
| ("writeHint", StringTk::uint64ToStr(writeHint))); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: If If a non-zero |
||
|
|
||
| if (!quota.useQuota) | ||
| return {FhgfsOpsErr_SUCCESS, fd}; | ||
|
|
||
|
|
@@ -607,7 +613,7 @@ std::pair<FhgfsOpsErr, int> ChunkStore::openAndChown(const int targetFD, const s | |
| */ | ||
| FhgfsOpsErr ChunkStore::openChunkFile(int targetFD, const Path* chunkDirPath, | ||
| const std::string& chunkFilePathStr, bool hasOrigFeature, int openFlags, int* outFD, | ||
| const SessionQuotaInfo* quotaInfo, const ExceededQuotaStorePtr exQuotaStore) | ||
| const SessionQuotaInfo* quotaInfo, const ExceededQuotaStorePtr exQuotaStore, uint64_t writeHint) | ||
| { | ||
| const char* logContext = "ChunkStore create chunkFile"; | ||
| FhgfsOpsErr retVal = FhgfsOpsErr_INTERNAL; | ||
|
|
@@ -637,7 +643,7 @@ FhgfsOpsErr ChunkStore::openChunkFile(int targetFD, const Path* chunkDirPath, | |
| } | ||
| } | ||
|
|
||
| std::tie(retVal, *outFD) = openAndChown(targetFD, chunkFilePathStr, openFlags, *quotaInfo); | ||
| std::tie(retVal, *outFD) = openAndChown(targetFD, chunkFilePathStr, openFlags, *quotaInfo, writeHint); | ||
| if (retVal == FhgfsOpsErr_SUCCESS) | ||
| return FhgfsOpsErr_SUCCESS; | ||
|
|
||
|
|
@@ -665,7 +671,7 @@ FhgfsOpsErr ChunkStore::openChunkFile(int targetFD, const Path* chunkDirPath, | |
| } | ||
|
|
||
| // dir created => try file open/create again... | ||
| std::tie(retVal, *outFD) = openAndChown(targetFD, chunkFilePathStr, openFlags, *quotaInfo); | ||
| std::tie(retVal, *outFD) = openAndChown(targetFD, chunkFilePathStr, openFlags, *quotaInfo, writeHint); | ||
|
|
||
| if (lastChunkDirElement) // old V2 files do not get this | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: Unfortunately adding fields to network messages like the
writeHinttoWriteLocalFileMsgandWriteLocalFileRDMAMsgwill create incompatibilities when there are mixed client/server versions. If a client sends a message with this field to a server that doesn't know about it yet, the server-side message deserialization will fail. This is also a problem if the server was updated and expects the new message format, but the client omits this field.The expectation is that any 8.x client can communicate to any 8.x server. While sometimes we can find a way to rollout a change like this in a minor BeeGFS release, it will always require extra handling. Because this tends to introduce technical debt we have to go back and cleanup at the next major release, we try to avoid these kinds of changes if possible.
So I'll need to still justify the change internally. It would be helpful if you could share any testing you've done that demonstrates the performance improvement and/or other benefits you've seen with this patch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I've added more details to the pull request. Please take a look.