Commit 770e1a73 authored by Phoebe Buckheister's avatar Phoebe Buckheister 🦎

meta: correctly lock items in MovingFileInsertMsg

 * toDir always resides on the local node, so previously we would never
   have locked anything at all. check for locally generates requests
   instead.
 * don't lazily lock the new file and the unlinked file to avoid
   deadlocks
 * also lock the parent/name of the new dentry to comply with the
   resyncer protocol

(cherry picked from commit ec78409c4acf95814cafcd5097dda783f9308c70)
parent 2a0ab2fb
......@@ -9,6 +9,61 @@
#include "MovingFileInsertMsgEx.h"
std::tuple<FileIDLock, FileIDLock, DirIDLock, ParentNameLock> MovingFileInsertMsgEx::lock(
EntryLockStore& store)
{
// we must not lock the directory if it is owned by the current node. if it is, the
// current message was also sent by the local node, specifically by a rename msg, which
// also locks the directory for write
if (rctx->isLocallyGenerated())
return {};
DirIDLock dirLock(&store, getToDirInfo()->getEntryID(), true);
ParentNameLock nameLock(&store, getToDirInfo()->getEntryID(), getNewName());
FileIDLock newLock;
FileIDLock unlinkedLock;
auto dir = Program::getApp()->getMetaStore()->referenceDir(getToDirInfo()->getEntryID(),
getToDirInfo()->getIsBuddyMirrored(), true);
if (dir)
{
FileInode newInode;
Deserializer des(getSerialBuf(), getSerialBufLen());
newInode.deserializeMetaData(des);
if (des.good())
{
std::string unlinkedID = newInode.getEntryID();
EntryInfo unlinkedInfo;
dir->getFileEntryInfo(getNewName(), unlinkedInfo);
if (DirEntryType_ISFILE(unlinkedInfo.getEntryType()))
unlinkedID = unlinkedInfo.getEntryID();
if (newInode.getEntryID() < unlinkedID)
{
newLock = {&store, newInode.getEntryID()};
unlinkedLock = {&store, unlinkedID};
}
else if (newInode.getEntryID() == unlinkedID)
{
newLock = {&store, newInode.getEntryID()};
}
else
{
unlinkedLock = {&store, unlinkedID};
newLock = {&store, newInode.getEntryID()};
}
}
}
return std::make_tuple(
std::move(newLock),
std::move(unlinkedLock),
std::move(dirLock),
std::move(nameLock));
}
bool MovingFileInsertMsgEx::processIncoming(ResponseContext& ctx)
{
......@@ -39,9 +94,7 @@ std::unique_ptr<MirroredMessageResponseState> MovingFileInsertMsgEx::executeLoca
return boost::make_unique<MovingFileInsertResponseState>(FhgfsOpsErr_PATHNOTEXISTS);
FhgfsOpsErr moveRes = metaStore->moveRemoteFileInsert(
fromFileInfo, *toDir, newName,
getSerialBuf(), getSerialBufLen(), &unlinkInode, newFileInfo, std::get<0>(lockState),
std::get<1>(lockState));
fromFileInfo, *toDir, newName, getSerialBuf(), getSerialBufLen(), &unlinkInode, newFileInfo);
if (moveRes != FhgfsOpsErr_SUCCESS)
{
metaStore->releaseDir(toDir->getID());
......
......@@ -68,34 +68,21 @@ class MovingFileInsertResponseState : public MirroredMessageResponseState
};
class MovingFileInsertMsgEx : public MirroredMessage<MovingFileInsertMsg,
std::tuple<FileIDLock, FileIDLock, DirIDLock>>
std::tuple<FileIDLock, FileIDLock, DirIDLock, ParentNameLock>>
{
public:
typedef MovingFileInsertResponseState ResponseState;
virtual bool processIncoming(ResponseContext& ctx) override;
std::tuple<FileIDLock, FileIDLock, DirIDLock> lock(EntryLockStore& store) override
{
// the created file need not be locked, because only the requestor of the move operations
// knows the new parent of the file, and only knows it for certain once the origin server
// has acknowledged the operation.
// we must not lock the directory if it is owned by the current node. if it is, the
// current message was also sent by the local node, specifically by a RmDirMsgEx, which
// also locks the directory for write
uint16_t localID = Program::getApp()->getMetaBuddyGroupMapper()->getLocalGroupID();
if (getToDirInfo()->getOwnerNodeID().val() == localID)
return {};
return std::make_tuple(
FileIDLock(), // new file inode
FileIDLock(), // (maybe) overwritten file inode
DirIDLock(&store, getToDirInfo()->getEntryID(), true));
}
std::tuple<FileIDLock, FileIDLock, DirIDLock, ParentNameLock>
lock(EntryLockStore& store) override;
bool isMirrored() override { return getToDirInfo()->getIsBuddyMirrored(); }
private:
ResponseContext* rctx;
StringVector xattrNames;
EntryInfo newFileInfo;
MsgHelperXAttr::StreamXAttrState streamState;
......
......@@ -69,8 +69,7 @@ class MetaStore
FhgfsOpsErr moveRemoteFileInsert(EntryInfo* fromFileInfo, DirInode& toParent,
const std::string& newEntryName, const char* buf,
uint32_t bufLen, std::unique_ptr<FileInode>* outUnlinkedFile, EntryInfo& newFileInfo,
FileIDLock& newFileLock, FileIDLock& oldFileLock);
uint32_t bufLen, std::unique_ptr<FileInode>* outUnlinkedFile, EntryInfo& newFileInfo);
FhgfsOpsErr moveRemoteFileBegin(DirInode& dir, EntryInfo* entryInfo, char* buf, size_t bufLen,
size_t* outUsedBufLen);
......
......@@ -331,8 +331,7 @@ FhgfsOpsErr MetaStore::checkRenameOverwrite(EntryInfo* fromEntry, EntryInfo* ove
*/
FhgfsOpsErr MetaStore::moveRemoteFileInsert(EntryInfo* fromFileInfo, DirInode& toParent,
const std::string& newEntryName, const char* buf, uint32_t bufLen,
std::unique_ptr<FileInode>* outUnlinkedInode, EntryInfo& newFileInfo, FileIDLock& newFileLock,
FileIDLock& oldFileLock)
std::unique_ptr<FileInode>* outUnlinkedInode, EntryInfo& newFileInfo)
{
// note: we do not allow newEntry to be a file if the old entry was a directory (and vice versa)
const char* logContext = "rename(): Insert remote entry";
......@@ -363,12 +362,6 @@ FhgfsOpsErr MetaStore::moveRemoteFileInsert(EntryInfo* fromFileInfo, DirInode& t
goto outUnlock;
}
// if we overwrite an existing dentry, the inode behind the dentry will be either updated or
// unlinked, thus requiring a lock.
if (toParent.getIsBuddyMirrored() && overWriteInfo.getIsInlined())
oldFileLock = {Program::getApp()->getMirroredSessions()->getEntryLockStore(),
overWriteInfo.getEntryID()};
// only unlink the dir-entry-name here, so we can still restore it from dir-entry-id
FhgfsOpsErr unlinkRes = toParent.unlinkDirEntryUnlocked(newEntryName, overWrittenEntry,
DirEntry_UNLINK_FILENAME);
......@@ -418,19 +411,6 @@ FhgfsOpsErr MetaStore::moveRemoteFileInsert(EntryInfo* fromFileInfo, DirInode& t
retVal = FhgfsOpsErr_INTERNAL;
}
if (retVal == FhgfsOpsErr_SUCCESS)
{
// since this entry ID was not previously present on this node, and toParent is locked for
// write, no other thread can lock the entry ID.
// beware: the caller is responsible for locking an inlined file inode for the name if both
// source and target live on the same node!
if (toParent.getIsBuddyMirrored() && fromFileInfo->getIsInlined() &&
(!fromFileInfo->getIsBuddyMirrored() ||
fromFileInfo->getOwnerNodeID() != toParent.ownerNodeID))
newFileLock = {Program::getApp()->getMirroredSessions()->getEntryLockStore(),
newFileInfo.getEntryID()};
}
if (overWrittenEntry && retVal == FhgfsOpsErr_SUCCESS)
{ // unlink the overwritten entry, will unlock, release and return
bool unlinkedWasInlined = overWrittenEntry->getIsInodeInlined();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment