Skip to content

Commit

Permalink
Fix conflict over unlocked refcount DB
Browse files Browse the repository at this point in the history 
If BackupStoreCheck is are read-only, then it should not open a "temporary"
database in the old sense, because that does actually change the store: the
temporary file would conflict with any other process which wants to do the same
thing at the same time (e.g. housekeeping), and if neither process locks the
store, they will break each other.

Instead, we now distinguish between "potential" refcount DBs (of which there
can only be one at a time, and which require a lock to create) and really
"temporary" ones which are created in a temporary directory, can never become
permanent via Commit(), do not require a lock, and are unlimited in number.
Commit() will rename them, but still in the temporary directory, so it has no
effect on the store.

This requires some changes to the BackupStoreRefCountDatabase::Create interface
to add new flags.
  • Loading branch information
@qris
qris committed Oct 7, 2017
1 parent 819f467 commit 116bb26
Show file tree
Hide file tree
Showing 4 changed files with 149 additions and 66 deletions.
36 changes: 35 additions & 1 deletion lib/backupstore/BackupStoreCheck.cpp
View file
Expand Up @@ -125,7 +125,41 @@ void BackupStoreCheck::Check()
}

BackupStoreAccountDatabase::Entry account(mAccountID, mDiscSetNumber);
mapNewRefs = BackupStoreRefCountDatabase::Create(account);
// If we are read-only, then we should not call GetPotentialRefCountDatabase because
// that does actually change the store: the temporary file would conflict with any other
// process which wants to do the same thing at the same time (e.g. housekeeping), and if
// neither process locks the store, they will break each other. We can still create a
// refcount DB in a temporary directory, and Commit() will not really commit it in that
// case (it will rename it, but still in the temporary directory).
if(mFixErrors)
{
mapNewRefs = BackupStoreRefCountDatabase::Create(account);
}
else
{
std::string temp_file = GetTempDirPath() + "boxbackup_refcount_db_XXXXXX";
char temp_file_buf[PATH_MAX];
strncpy(temp_file_buf, temp_file.c_str(), sizeof(temp_file_buf));
#ifdef WIN32
if(_mktemp_s(temp_file_buf, sizeof(temp_file_buf)) != 0)
{
THROW_EXCEPTION_MESSAGE(BackupStoreException, FailedToCreateTemporaryFile,
"Failed to get a temporary file name based on " << temp_file);
}
#else
int fd = mkstemp(temp_file_buf);
if(fd == -1)
{
THROW_SYS_FILE_ERROR("Failed to get a temporary file name based on",
temp_file, BackupStoreException, FailedToCreateTemporaryFile);
}
close(fd);
#endif

BOX_TRACE("Creating temporary refcount DB in a temporary file: " << temp_file_buf);
mapNewRefs = BackupStoreRefCountDatabase::Create(temp_file_buf,
mAccountID, true); // reuse_existing_file
}

// Phase 1, check objects
if(!mQuiet)
Expand Down
1 change: 1 addition & 0 deletions lib/backupstore/BackupStoreException.txt
View file
Expand Up @@ -77,3 +77,4 @@ InvalidEtagHeader 73 The S3 HTTP response contain a malformed or unexpected ETa
FileSystemNotInitialised 74 No BackupFileSystem has been configured for this account
ObjectDoesNotExist 75 The specified object ID does not exist in the store, or is not of this type
AccountAlreadyExists 76 Tried to create an account that already exists
FailedToCreateTemporaryFile 77 Failed to create a temporary file
154 changes: 100 additions & 54 deletions lib/backupstore/BackupStoreRefCountDatabase.cpp
View file
Expand Up @@ -27,6 +27,9 @@
#define REFCOUNT_MAGIC_VALUE 0x52656643 // RefC
#define REFCOUNT_FILENAME "refcount"

typedef BackupStoreRefCountDatabase::refcount_t refcount_t;


// --------------------------------------------------------------------------
//
// Function
Expand All @@ -35,22 +38,27 @@
// Created: 2003/08/28
//
// --------------------------------------------------------------------------
BackupStoreRefCountDatabase::BackupStoreRefCountDatabase(const
BackupStoreAccountDatabase::Entry& rAccount, bool ReadOnly,
bool Temporary, std::auto_ptr<FileStream> apDatabaseFile)
: mAccount(rAccount),
mFilename(GetFilename(rAccount, Temporary)),
BackupStoreRefCountDatabase::BackupStoreRefCountDatabase(
const std::string& Filename, int64_t AccountID, bool ReadOnly, bool PotentialDB,
bool TemporaryDB, std::auto_ptr<FileStream> apDatabaseFile)
: mAccountID(AccountID),
mFilename(Filename + (PotentialDB ? "X" : "")),
mFinalFilename(TemporaryDB ? "" : Filename),
mReadOnly(ReadOnly),
mIsModified(false),
mIsTemporaryFile(Temporary),
mIsPotentialDB(PotentialDB),
mIsTemporaryDB(TemporaryDB),
mapDatabaseFile(apDatabaseFile)
{
ASSERT(!(ReadOnly && Temporary)); // being both doesn't make sense
ASSERT(!(PotentialDB && TemporaryDB)); // being both doesn't make sense
ASSERT(!(ReadOnly && PotentialDB)); // being both doesn't make sense
}

void BackupStoreRefCountDatabase::Commit()
{
if (!mIsTemporaryFile)
ASSERT(!mIsTemporaryDB);

if (!mIsPotentialDB)
{
THROW_EXCEPTION_MESSAGE(CommonException, Internal,
"Cannot commit a permanent reference count database");
Expand All @@ -65,32 +73,30 @@ void BackupStoreRefCountDatabase::Commit()
mapDatabaseFile->Close();
mapDatabaseFile.reset();

std::string Final_Filename = GetFilename(mAccount, false);

#ifdef WIN32
if(FileExists(Final_Filename) && EMU_UNLINK(Final_Filename.c_str()) != 0)
if(FileExists(mFinalFilename) && EMU_UNLINK(mFinalFilename.c_str()) != 0)
{
THROW_EMU_FILE_ERROR("Failed to delete old permanent refcount "
"database file", mFilename, CommonException,
"database file", mFinalFilename, CommonException,
OSFileError);
}
#endif

if(rename(mFilename.c_str(), Final_Filename.c_str()) != 0)
if(rename(mFilename.c_str(), mFinalFilename.c_str()) != 0)
{
THROW_EMU_ERROR("Failed to rename temporary refcount database "
"file from " << mFilename << " to " <<
Final_Filename, CommonException, OSFileError);
mFinalFilename, CommonException, OSFileError);
}

mFilename = Final_Filename;
mFilename = mFinalFilename;
mIsModified = false;
mIsTemporaryFile = false;
mIsPotentialDB = false;
}

void BackupStoreRefCountDatabase::Discard()
{
if (!mIsTemporaryFile)
if (!mIsTemporaryDB && !mIsPotentialDB)
{
THROW_EXCEPTION_MESSAGE(CommonException, Internal,
"Cannot discard a permanent reference count database");
Expand All @@ -108,13 +114,14 @@ void BackupStoreRefCountDatabase::Discard()

if(EMU_UNLINK(mFilename.c_str()) != 0)
{
THROW_EMU_FILE_ERROR("Failed to delete temporary refcount "
THROW_EMU_FILE_ERROR("Failed to delete temporary/potential refcount "
"database file", mFilename, CommonException,
OSFileError);
}

mIsModified = false;
mIsTemporaryFile = false;
mIsTemporaryDB = false;
mIsPotentialDB = false;
}

// --------------------------------------------------------------------------
Expand All @@ -127,11 +134,15 @@ void BackupStoreRefCountDatabase::Discard()
// --------------------------------------------------------------------------
BackupStoreRefCountDatabase::~BackupStoreRefCountDatabase()
{
if (mIsTemporaryFile)
if (mIsTemporaryDB || mIsPotentialDB)
{
// Don't throw exceptions in a destructor.
BOX_ERROR("BackupStoreRefCountDatabase destroyed without "
"explicit commit or discard");
if(mIsPotentialDB)
{
BOX_ERROR("Potential new BackupStoreRefCountDatabase destroyed "
"without explicit commit or discard");
}

try
{
Discard();
Expand All @@ -145,17 +156,13 @@ BackupStoreRefCountDatabase::~BackupStoreRefCountDatabase()
}

std::string BackupStoreRefCountDatabase::GetFilename(const
BackupStoreAccountDatabase::Entry& rAccount, bool Temporary)
BackupStoreAccountDatabase::Entry& rAccount)
{
std::string RootDir = BackupStoreAccounts::GetAccountRoot(rAccount);
ASSERT(RootDir[RootDir.size() - 1] == '/' ||
RootDir[RootDir.size() - 1] == DIRECTORY_SEPARATOR_ASCHAR);

std::string fn(RootDir + REFCOUNT_FILENAME ".rdb");
if(Temporary)
{
fn += "X";
}
RaidFileController &rcontroller(RaidFileController::GetController());
RaidFileDiscSet rdiscSet(rcontroller.GetDiscSet(rAccount.GetDiscSet()));
return RaidFileUtil::MakeWriteFileName(rdiscSet, fn);
Expand All @@ -164,48 +171,85 @@ std::string BackupStoreRefCountDatabase::GetFilename(const
// --------------------------------------------------------------------------
//
// Function
// Name: BackupStoreRefCountDatabase::Create(int32_t,
// const std::string &, int, bool)
// Name: BackupStoreRefCountDatabase::Create(
// const BackupStoreAccountDatabase::Entry& rAccount)
// Purpose: Create a blank database, using a temporary file that
// you must Discard() or Commit() to make permanent.
// Created: 2003/08/28
//
// --------------------------------------------------------------------------
std::auto_ptr<BackupStoreRefCountDatabase>
BackupStoreRefCountDatabase::Create
(const BackupStoreAccountDatabase::Entry& rAccount)
BackupStoreRefCountDatabase::Create(const BackupStoreAccountDatabase::Entry& rAccount)
{
// Initial header
refcount_StreamFormat hdr;
hdr.mMagicValue = htonl(REFCOUNT_MAGIC_VALUE);
hdr.mAccountID = htonl(rAccount.GetID());

std::string Filename = GetFilename(rAccount, true); // temporary
std::string Filename = GetFilename(rAccount);
return Create(Filename, rAccount.GetID());
}

// --------------------------------------------------------------------------
//
// Function
// Name: BackupStoreRefCountDatabase::Create(
// const std::string& Filename, int64_t AccountID,
// bool reuse_existing_file)
// Purpose: Create a blank database, using a temporary file that
// you must Discard() or Commit() to make permanent.
// Be careful with reuse_existing_file, because it
// makes it easy to bypass the restriction of only one
// (committable) temporary database at a time, and to
// accidentally overwrite the main DB.
// Created: 2003/08/28
//
// --------------------------------------------------------------------------
std::auto_ptr<BackupStoreRefCountDatabase>
BackupStoreRefCountDatabase::Create(const std::string& Filename, int64_t AccountID,
bool reuse_existing_file)
{
// Open the file for writing
if (FileExists(Filename))
std::string temp_filename = Filename + (reuse_existing_file ? "" : "X");
int flags = O_CREAT | O_BINARY | O_RDWR | O_EXCL;

if(FileExists(temp_filename))
{
BOX_WARNING(BOX_FILE_MESSAGE(Filename, "Overwriting existing "
"temporary reference count database"));
if (EMU_UNLINK(Filename.c_str()) != 0)
if(reuse_existing_file)
{
// Don't warn, and don't fail because the file already exists. This allows
// creating a temporary file securely with mkstemp() and then opening it as
// a refcount database, avoiding a race condition.
flags &= ~O_EXCL;
}
else
{
THROW_SYS_FILE_ERROR("Failed to delete old temporary "
"reference count database file", Filename,
CommonException, OSFileError);
BOX_WARNING(BOX_FILE_MESSAGE(temp_filename, "Overwriting existing "
"temporary reference count database"));
if(EMU_UNLINK(temp_filename.c_str()) != 0)
{
THROW_SYS_FILE_ERROR("Failed to delete old temporary "
"reference count database file", temp_filename,
CommonException, OSFileError);
}
}
}

int flags = O_CREAT | O_BINARY | O_RDWR | O_EXCL;
std::auto_ptr<FileStream> DatabaseFile(new FileStream(Filename, flags));
#ifdef BOX_OPEN_LOCK
flags |= BOX_OPEN_LOCK;
#endif
std::auto_ptr<FileStream> database_file(new FileStream(temp_filename, flags));

// Write header
DatabaseFile->Write(&hdr, sizeof(hdr));
refcount_StreamFormat hdr;
hdr.mMagicValue = htonl(REFCOUNT_MAGIC_VALUE);
hdr.mAccountID = htonl(AccountID);
database_file->Write(&hdr, sizeof(hdr));

// Make new object
std::auto_ptr<BackupStoreRefCountDatabase> refcount(
new BackupStoreRefCountDatabase(rAccount, false, true,
DatabaseFile));

BackupStoreRefCountDatabase* p_impl = new BackupStoreRefCountDatabase(
Filename, AccountID,
false, // ReadOnly
!reuse_existing_file, // PotentialDB
reuse_existing_file, // TemporaryDB
database_file);
std::auto_ptr<BackupStoreRefCountDatabase> refcount(p_impl);

// The root directory must always have one reference for a database
// to be valid, so set that now on the new database. This will leave
// mIsModified set to true.
Expand All @@ -231,7 +275,7 @@ std::auto_ptr<BackupStoreRefCountDatabase> BackupStoreRefCountDatabase::Load(
{
// Generate the filename. Cannot open a temporary database, so it must
// be a permanent one.
std::string Filename = GetFilename(rAccount, false);
std::string Filename = GetFilename(rAccount);
int flags = ReadOnly ? O_RDONLY : O_RDWR;

// Open the file for read/write
Expand All @@ -258,9 +302,11 @@ std::auto_ptr<BackupStoreRefCountDatabase> BackupStoreRefCountDatabase::Load(

// Make new object
std::auto_ptr<BackupStoreRefCountDatabase> refcount(
new BackupStoreRefCountDatabase(rAccount, ReadOnly, false,
new BackupStoreRefCountDatabase(Filename, rAccount.GetID(), ReadOnly,
false, // PotentialDB
false, // TemporaryDB
dbfile));

// return it to caller
return refcount;
}
Expand Down
24 changes: 13 additions & 11 deletions lib/backupstore/BackupStoreRefCountDatabase.h
View file
Expand Up @@ -59,8 +59,8 @@ class BackupStoreRefCountDatabase
~BackupStoreRefCountDatabase();
private:
// Creation through static functions only
BackupStoreRefCountDatabase(const BackupStoreAccountDatabase::Entry&
rAccount, bool ReadOnly, bool Temporary,
BackupStoreRefCountDatabase(const std::string& Filename, int64_t AccountID,
bool ReadOnly, bool PotentialDB, bool TemporaryDB,
std::auto_ptr<FileStream> apDatabaseFile);
// No copying allowed
BackupStoreRefCountDatabase(const BackupStoreRefCountDatabase &);
Expand All @@ -70,6 +70,8 @@ class BackupStoreRefCountDatabase
// Discard() or Commit() to make permanent.
static std::auto_ptr<BackupStoreRefCountDatabase> Create
(const BackupStoreAccountDatabase::Entry& rAccount);
static std::auto_ptr<BackupStoreRefCountDatabase> Create
(const std::string& Filename, int64_t AccountID, bool reuse_existing_file = false);
void Commit();
void Discard();

Expand All @@ -90,8 +92,7 @@ class BackupStoreRefCountDatabase
int ReportChangesTo(BackupStoreRefCountDatabase& rOldRefs);

private:
static std::string GetFilename(const BackupStoreAccountDatabase::Entry&
rAccount, bool Temporary);
static std::string GetFilename(const BackupStoreAccountDatabase::Entry& rAccount);

IOStream::pos_type GetSize() const
{
Expand All @@ -110,16 +111,17 @@ class BackupStoreRefCountDatabase
void SetRefCount(int64_t ObjectID, refcount_t NewRefCount);

// Location information
BackupStoreAccountDatabase::Entry mAccount;
std::string mFilename;
bool mReadOnly;
bool mIsModified;
bool mIsTemporaryFile;
std::auto_ptr<FileStream> mapDatabaseFile;
int64_t mAccountID;
std::string mFilename, mFinalFilename;
bool mReadOnly;
bool mIsModified;
bool mIsPotentialDB;
bool mIsTemporaryDB;
std::auto_ptr<FileStream> mapDatabaseFile;

bool NeedsCommitOrDiscard()
{
return mapDatabaseFile.get() && mIsModified && mIsTemporaryFile;
return mapDatabaseFile.get() && mIsModified && mIsPotentialDB;
}
};

Expand Down

0 comments on commit 116bb26

Please sign in to comment.