Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions client_module/build/feature-detect.sh
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,11 @@ run_job check_struct_field \
inode::i_mtime \
KERNEL_HAS_INODE_MTIME \
linux/fs.h

run_job check_struct_field \
inode::i_write_hint \
KERNEL_HAS_INODE_I_WRITE_HINT \
linux/fs.h

run_job check_struct_field \
dentry::d_subdirs \
Expand Down
Copy link
Copy Markdown
Member

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 writeHint to WriteLocalFileMsg and WriteLocalFileRDMAMsg will 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?

Copy link
Copy Markdown
Author

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.

Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,7 @@ void WriteLocalFileMsg_serializePayload(NetMessage* this, SerializeCtx* ctx)

// targetID
Serialization_serializeUShort(ctx, thisCast->targetID);

// writeHint
Serialization_serializeUInt64(ctx, thisCast->writeHint);
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ typedef struct WriteLocalFileMsg WriteLocalFileMsg;
static inline void WriteLocalFileMsg_init(WriteLocalFileMsg* this);
static inline void WriteLocalFileMsg_initFromSession(WriteLocalFileMsg* this,
NumNodeID clientNumID, const char* fileHandleID, uint16_t targetID, PathInfo* pathInfo,
unsigned accessFlags, int64_t offset, int64_t count);
unsigned accessFlags, int64_t offset, int64_t count, uint64_t writeHint);

// virtual functions
extern void WriteLocalFileMsg_serializePayload(NetMessage* this, SerializeCtx* ctx);
Expand All @@ -51,6 +51,7 @@ struct WriteLocalFileMsg
PathInfo* pathInfo;
unsigned userID;
unsigned groupID;
uint64_t writeHint;
};

extern const struct NetMessageOps WriteLocalFileMsg_Ops;
Expand All @@ -67,7 +68,7 @@ void WriteLocalFileMsg_init(WriteLocalFileMsg* this)
void WriteLocalFileMsg_initFromSession(WriteLocalFileMsg* this,
NumNodeID clientNumID, const char* fileHandleID, uint16_t targetID, PathInfo* pathInfo,
unsigned accessFlags,
int64_t offset, int64_t count)
int64_t offset, int64_t count, uint64_t writeHint)
{
WriteLocalFileMsg_init(this);

Expand All @@ -83,6 +84,7 @@ void WriteLocalFileMsg_initFromSession(WriteLocalFileMsg* this,

this->offset = offset;
this->count = count;
this->writeHint = writeHint;
}

void WriteLocalFileMsg_setUserdataForQuota(WriteLocalFileMsg* this, unsigned userID,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,8 @@ void WriteLocalFileRDMAMsg_serializePayload(NetMessage* this, SerializeCtx* ctx)

// RDMA info
RdmaInfo_serialize(ctx, thisCast->rdmap);

// writeHint
Serialization_serializeUInt64(ctx, thisCast->writeHint);
}
#endif /* BEEGFS_NVFS */
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ typedef struct WriteLocalFileRDMAMsg WriteLocalFileRDMAMsg;
static inline void WriteLocalFileRDMAMsg_init(WriteLocalFileRDMAMsg* this);
static inline void WriteLocalFileRDMAMsg_initFromSession(WriteLocalFileRDMAMsg* this,
NumNodeID clientNumID, const char* fileHandleID, uint16_t targetID, PathInfo* pathInfo,
unsigned accessFlags, int64_t offset, int64_t count, RdmaInfo *rdmap);
unsigned accessFlags, int64_t offset, int64_t count, uint64_t writeHint, RdmaInfo *rdmap);

// virtual functions
extern void WriteLocalFileRDMAMsg_serializePayload(NetMessage* this, SerializeCtx* ctx);
Expand All @@ -53,6 +53,7 @@ struct WriteLocalFileRDMAMsg
PathInfo* pathInfo;
unsigned userID;
unsigned groupID;
uint64_t writeHint;
RdmaInfo *rdmap;
};

Expand All @@ -69,7 +70,7 @@ void WriteLocalFileRDMAMsg_init(WriteLocalFileRDMAMsg* this)
*/
void WriteLocalFileRDMAMsg_initFromSession(WriteLocalFileRDMAMsg* this,
NumNodeID clientNumID, const char* fileHandleID, uint16_t targetID, PathInfo* pathInfo,
unsigned accessFlags, int64_t offset, int64_t count, RdmaInfo *rdmap)
unsigned accessFlags, int64_t offset, int64_t count, uint64_t writeHint, RdmaInfo *rdmap)
{
WriteLocalFileRDMAMsg_init(this);

Expand All @@ -85,6 +86,7 @@ void WriteLocalFileRDMAMsg_initFromSession(WriteLocalFileRDMAMsg* this,

this->offset = offset;
this->count = count;
this->writeHint = writeHint;

this->rdmap = rdmap;
}
Expand Down
6 changes: 6 additions & 0 deletions client_module/source/filesystem/FhgfsInode.c
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,12 @@ void __FhgfsInode_initOpenIOInfo(FhgfsInode* this, FhgfsInodeFileHandle* fileHan
#ifdef BEEGFS_NVFS
outIOInfo->nvfs = false;
#endif

#if defined(KERNEL_HAS_INODE_I_WRITE_HINT)
outIOInfo->writeHint = this->vfs_inode.i_write_hint;
#else
outIOInfo->writeHint = RW_HINT_INVALID;
#endif
}

/**
Expand Down
5 changes: 3 additions & 2 deletions client_module/source/net/filesystem/FhgfsOpsCommKit.c
Original file line number Diff line number Diff line change
Expand Up @@ -1322,7 +1322,8 @@ static unsigned __commkit_writefile_prepareHeader(CommKitContext* context,
{
WriteLocalFileMsg_initFromSession(&writeMsg, localNodeNumID,
context->ioInfo->fileHandleID, info->targetID, context->ioInfo->pathInfo,
context->ioInfo->accessFlags, currentState->offset, currentState->totalSize);
context->ioInfo->accessFlags, currentState->offset, currentState->totalSize,
context->ioInfo->writeHint);

netMessage = &writeMsg.netMessage;
}
Expand All @@ -1332,7 +1333,7 @@ static unsigned __commkit_writefile_prepareHeader(CommKitContext* context,
WriteLocalFileRDMAMsg_initFromSession(&writeRDMAMsg, localNodeNumID,
context->ioInfo->fileHandleID, info->targetID, context->ioInfo->pathInfo,
context->ioInfo->accessFlags, currentState->offset, currentState->totalSize,
currentState->rdmap);
context->ioInfo->writeHint, currentState->rdmap);

netMessage = &writeRDMAMsg.netMessage;
}
Expand Down
2 changes: 1 addition & 1 deletion client_module/source/net/filesystem/FhgfsOpsCommKitVec.c
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ void __FhgfsOpsCommKitVec_writefileStagePREPARE(CommKitVecHelper* commHelper,
// prepare message
WriteLocalFileMsg_initFromSession(&writeMsg, localNodeNumID,
commHelper->ioInfo->fileHandleID, comm->targetID, commHelper->ioInfo->pathInfo,
commHelper->ioInfo->accessFlags, offset, remainingDataSize);
commHelper->ioInfo->accessFlags, offset, remainingDataSize, commHelper->ioInfo->writeHint);

NetMessage_setMsgHeaderTargetID( (NetMessage*)&writeMsg, nodeReferenceTargetID);

Expand Down
6 changes: 6 additions & 0 deletions client_module/source/net/filesystem/RemotingIOInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <toolkit/BitStore.h>
#include <app/App.h>

#define RW_HINT_INVALID 0xFF

struct RemotingIOInfo;
typedef struct RemotingIOInfo RemotingIOInfo;
Expand Down Expand Up @@ -44,6 +45,7 @@ struct RemotingIOInfo
#ifdef BEEGFS_NVFS
bool nvfs;
#endif
uint64_t writeHint;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

todo: we need to ensure the writeHint is also initialized in functions like RemotingIOInfo_initOpen() and RemotingIOInfo_initSpecialClose(). Otherwise it might be initialized to whatever random garbage is on the stack and try to assign random or invalid lifetimes on the storage side.

};


Expand Down Expand Up @@ -78,6 +80,8 @@ void RemotingIOInfo_initOpen(App* app, unsigned accessFlags, AtomicInt* maxUsedT
#ifdef BEEGFS_NVFS
outIOInfo->nvfs = false;
#endif

outIOInfo->writeHint = RW_HINT_INVALID;
}


Expand All @@ -104,6 +108,8 @@ void RemotingIOInfo_initSpecialClose(App* app, const char* fileHandleID,
#ifdef BEEGFS_NVFS
outIOInfo->nvfs = false;
#endif

outIOInfo->writeHint = RW_HINT_INVALID;
}

/**
Expand Down
18 changes: 13 additions & 5 deletions common/source/common/net/message/session/rw/WriteLocalFileMsg.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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 uint64_t writeHint)
{
this->clientNumID = clientNumID;

Expand All @@ -36,6 +37,7 @@ class WriteLocalFileMsgBase

this->offset = offset;
this->count = count;
this->writeHint = writeHint;
}

WriteLocalFileMsgBase() {}
Expand All @@ -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:
Expand All @@ -71,7 +74,7 @@ class WriteLocalFileMsgBase

uint32_t userID;
uint32_t groupID;

uint64_t writeHint;
// for serialization
const PathInfo* pathInfoPtr;

Expand Down Expand Up @@ -126,6 +129,11 @@ class WriteLocalFileMsgBase
return groupID;
}

uint64_t getWriteHint() const
{
return writeHint;
}

void setUserdataForQuota(unsigned userID, unsigned groupID)
{
this->userID = userID;
Expand Down Expand Up @@ -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) {}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ class WriteLocalFileRDMAMsg : public WriteLocalFileMsgBase, public NetMessageSer
*/
WriteLocalFileRDMAMsg(const NumNodeID clientNumID, const char* fileHandleID,
const uint16_t targetID, const PathInfo* pathInfo, const unsigned accessFlags,
const int64_t offset, const int64_t count) :
WriteLocalFileMsgBase(clientNumID, fileHandleID, targetID, pathInfo, accessFlags, offset, count),
const int64_t offset, const int64_t count, const uint64_t writeHint = RW_HINT_INVALID) :
WriteLocalFileMsgBase(clientNumID, fileHandleID, targetID, pathInfo, accessFlags, offset, count, writeHint),
BaseType(NETMSGTYPE_WriteLocalFileRDMA) {}

/**
Expand Down
7 changes: 4 additions & 3 deletions storage/source/net/message/session/rw/WriteLocalFileMsgEx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,8 @@ FhgfsOpsErr WriteLocalFileMsgExBase<Msg, WriteState>::openFile(const StorageTarg

SessionQuotaInfo quotaInfo(useQuota, enforceQuota, getUserID(), getGroupID() );

FhgfsOpsErr openChunkRes = sessionLocalFile->openFile(targetFD, getPathInfo(), true, &quotaInfo);
FhgfsOpsErr openChunkRes = sessionLocalFile->openFile(targetFD, getPathInfo(),
true, &quotaInfo, getWriteHint());

return openChunkRes;
}
Expand Down Expand Up @@ -638,7 +639,7 @@ FhgfsOpsErr WriteLocalFileMsgExBase<Msg, WriteState>::prepareMirroring(char* buf
mirrorToSock = mirrorToNode->getConnPool()->acquireStreamSocket();

WriteLocalFileMsg mirrorWriteMsg(getClientNumID(), getFileHandleID(), getTargetID(),
getPathInfo(), getAccessFlags(), getOffset(), getCount());
getPathInfo(), getAccessFlags(), getOffset(), getCount(), getWriteHint());

if(doSessionCheck() )
mirrorWriteMsg.addMsgHeaderFeatureFlag(WRITELOCALFILEMSG_FLAG_SESSION_CHECK);
Expand Down Expand Up @@ -733,7 +734,7 @@ FhgfsOpsErr WriteLocalFileMsgExBase<Msg, WriteState>::sendToMirror(const char* b
mirrorToSock = mirrorToNode->getConnPool()->acquireStreamSocket();

WriteLocalFileMsg mirrorWriteMsg(getClientNumID(), getFileHandleID(),
getTargetID(), getPathInfo(), getAccessFlags(), offset, toBeMirrored);
getTargetID(), getPathInfo(), getAccessFlags(), offset, toBeMirrored, getWriteHint());

if(doSessionCheck() )
mirrorWriteMsg.addMsgHeaderFeatureFlag(WRITELOCALFILEMSG_FLAG_SESSION_CHECK);
Expand Down
5 changes: 5 additions & 0 deletions storage/source/net/message/session/rw/WriteLocalFileMsgEx.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@ class WriteLocalFileMsgExBase : public Msg
{
return static_cast<Msg&>(*this).getPathInfo();
}

inline uint64_t getWriteHint() const
{
return static_cast<const Msg&>(*this).getWriteHint();
}
};

/**
Expand Down
6 changes: 3 additions & 3 deletions storage/source/session/SessionLocalFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ void SessionLocalFile::serializeNodeID(SessionLocalFile* obj, Deserializer& des)
* @param isWriteOpen if set to true, the file will be created if it didn't exist.
*/
FhgfsOpsErr SessionLocalFile::openFile(int targetFD, const PathInfo* pathInfo,
bool isWriteOpen, const SessionQuotaInfo* quotaInfo)
bool isWriteOpen, const SessionQuotaInfo* quotaInfo, uint64_t writeHint)
{
FhgfsOpsErr retVal = FhgfsOpsErr_SUCCESS;

Expand Down Expand Up @@ -94,7 +94,7 @@ FhgfsOpsErr SessionLocalFile::openFile(int targetFD, const PathInfo* pathInfo,

FhgfsOpsErr openChunkRes = chunkDirStore->openChunkFile(
targetFD, &chunkDirPath, chunkFilePathStr, hasOrigFeature, openFlags, &fd, quotaInfo,
exceededQuotaStore);
exceededQuotaStore, writeHint);

// fix chunk path permissions
if (unlikely(openChunkRes == FhgfsOpsErr_NOTOWNER && quotaInfo->useQuota) )
Expand All @@ -104,7 +104,7 @@ FhgfsOpsErr SessionLocalFile::openFile(int targetFD, const PathInfo* pathInfo,

openChunkRes = chunkDirStore->openChunkFile(
targetFD, &chunkDirPath, chunkFilePathStr, hasOrigFeature, openFlags, &fd,
quotaInfo, exceededQuotaStore);
quotaInfo, exceededQuotaStore, writeHint);
}

if (openChunkRes != FhgfsOpsErr_SUCCESS)
Expand Down
3 changes: 2 additions & 1 deletion storage/source/session/SessionLocalFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include <atomic>

#define RW_HINT_INVALID 0xFF
/**
* Represents the client session information for an open chunk file.
*/
Expand Down Expand Up @@ -97,7 +98,7 @@ class SessionLocalFile
}

FhgfsOpsErr openFile(int targetFD, const PathInfo* pathInfo, bool isWriteOpen,
const SessionQuotaInfo* quotaInfo);
const SessionQuotaInfo* quotaInfo, uint64_t writeHint = RW_HINT_INVALID);

NodeHandle setMirrorNodeExclusive(NodeHandle mirrorNode);

Expand Down
19 changes: 15 additions & 4 deletions storage/source/storage/ChunkStore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -583,6 +583,17 @@ std::pair<FhgfsOpsErr, int> ChunkStore::openAndChown(const int targetFD, const s
return {FhgfsOpsErrTk::fromSysErr(errno), -1};
}

if (writeHint != RW_HINT_INVALID)
{
int r = fcntl(fd, F_SET_RW_HINT, &writeHint);
if (r < 0)
{
LOG(GENERAL, ERR,
"Client requested a write lifetime hint, but server failed to set it.",
("writeHint", StringTk::uint64ToStr(writeHint)), sysErr);
}
}

if (!quota.useQuota)
return {FhgfsOpsErr_SUCCESS, fd};

Expand All @@ -607,7 +618,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;
Expand Down Expand Up @@ -637,7 +648,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;

Expand Down Expand Up @@ -665,7 +676,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
{
Expand Down
Loading