Skip to content

Commit

Permalink
client: fix segment fault in SourceReader
Browse files Browse the repository at this point in the history 
After opencurve#172 and opencurve#209, source volume data of the clone volume
can be read within the client through SourceReader, and each
source volume was also opened and represented by a FileInstance,
but `FileInstance::mdsclient_` points to `FileClient::mdsClient_`,
so after FileClient is destroyed, `FileInstance::mdsclient_`
becomes dangling pointer.

To fix this problem, we let `FileClient::mdsClient_` be a shared_ptr,
and each FileInstance holds ownership of it.

Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>
  • Loading branch information
@wu-hanqing
wu-hanqing committed May 19, 2021
1 parent 4b772c2 commit 75c0f8a
Show file tree
Hide file tree
Showing 16 changed files with 185 additions and 259 deletions.
31 changes: 17 additions & 14 deletions src/client/file_instance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#include <butil/endpoint.h>
#include <glog/logging.h>
#include <utility>

#include "proto/nameserver2.pb.h"
#include "proto/topology.pb.h"
Expand All @@ -50,7 +51,7 @@ FileInstance::FileInstance()
readonly_(false) {}

bool FileInstance::Initialize(const std::string& filename,
MDSClient* mdsclient,
std::shared_ptr<MDSClient> mdsclient,
const UserInfo_t& userinfo,
const FileServiceOption& fileservicopt,
bool readonly) {
Expand All @@ -69,11 +70,12 @@ bool FileInstance::Initialize(const std::string& filename,
}

finfo_.userinfo = userinfo;
mdsclient_ = mdsclient;
mdsclient_ = std::move(mdsclient);

finfo_.fullPathName = filename;

if (!iomanager4file_.Initialize(filename, fileopt_.ioOpt, mdsclient_)) {
if (!iomanager4file_.Initialize(filename, fileopt_.ioOpt,
mdsclient_.get())) {
LOG(ERROR) << "Init io context manager failed, filename = "
<< filename;
break;
Expand All @@ -82,7 +84,8 @@ bool FileInstance::Initialize(const std::string& filename,
iomanager4file_.UpdateFileInfo(finfo_);

leaseExecutor_.reset(new (std::nothrow) LeaseExecutor(
fileopt_.leaseOpt, finfo_.userinfo, mdsclient_, &iomanager4file_));
fileopt_.leaseOpt, finfo_.userinfo, mdsclient_.get(),
&iomanager4file_));
if (CURVE_UNLIKELY(leaseExecutor_ == nullptr)) {
LOG(ERROR) << "Allocate LeaseExecutor failed, filename = "
<< filename;
Expand All @@ -100,32 +103,32 @@ void FileInstance::UnInitialize() {
}

int FileInstance::Read(char* buf, off_t offset, size_t length) {
return iomanager4file_.Read(buf, offset, length, mdsclient_);
return iomanager4file_.Read(buf, offset, length, mdsclient_.get());
}

int FileInstance::Write(const char* buf, off_t offset, size_t len) {
if (readonly_) {
DVLOG(9) << "open with read only, do not support write!";
return -1;
}
return iomanager4file_.Write(buf, offset, len, mdsclient_);
return iomanager4file_.Write(buf, offset, len, mdsclient_.get());
}

int FileInstance::AioRead(CurveAioContext* aioctx, UserDataType dataType) {
return iomanager4file_.AioRead(aioctx, mdsclient_, dataType);
return iomanager4file_.AioRead(aioctx, mdsclient_.get(), dataType);
}

int FileInstance::AioWrite(CurveAioContext* aioctx, UserDataType dataType) {
if (readonly_) {
DVLOG(9) << "open with read only, do not support write!";
return -1;
}
return iomanager4file_.AioWrite(aioctx, mdsclient_, dataType);
return iomanager4file_.AioWrite(aioctx, mdsclient_.get(), dataType);
}

int FileInstance::Discard(off_t offset, size_t length) {
if (!readonly_) {
return iomanager4file_.Discard(offset, length, mdsclient_);
return iomanager4file_.Discard(offset, length, mdsclient_.get());
}

LOG(ERROR) << "Open with read only, not support Discard";
Expand All @@ -134,7 +137,7 @@ int FileInstance::Discard(off_t offset, size_t length) {

int FileInstance::AioDiscard(CurveAioContext* aioctx) {
if (!readonly_) {
return iomanager4file_.AioDiscard(aioctx, mdsclient_);
return iomanager4file_.AioDiscard(aioctx, mdsclient_.get());
}

LOG(ERROR) << "Open with read only, not support AioDiscard";
Expand Down Expand Up @@ -197,7 +200,7 @@ int FileInstance::Close() {

FileInstance* FileInstance::NewInitedFileInstance(
const FileServiceOption& fileServiceOption,
MDSClient* mdsClient,
std::shared_ptr<MDSClient> mdsClient,
const std::string& filename,
const UserInfo& userInfo,
bool readonly) {
Expand All @@ -207,7 +210,7 @@ FileInstance* FileInstance::NewInitedFileInstance(
return nullptr;
}

bool ret = instance->Initialize(filename, mdsClient, userInfo,
bool ret = instance->Initialize(filename, std::move(mdsClient), userInfo,
fileServiceOption, readonly);
if (!ret) {
LOG(ERROR) << "FileInstance initialize failed"
Expand All @@ -222,11 +225,11 @@ FileInstance* FileInstance::NewInitedFileInstance(
}

FileInstance* FileInstance::Open4Readonly(const FileServiceOption& opt,
MDSClient* mdsclient,
std::shared_ptr<MDSClient> mdsclient,
const std::string& filename,
const UserInfo& userInfo) {
FileInstance* instance = FileInstance::NewInitedFileInstance(
opt, mdsclient, filename, userInfo, true);
opt, std::move(mdsclient), filename, userInfo, true);
if (instance == nullptr) {
LOG(ERROR) << "NewInitedFileInstance failed, filename = " << filename;
return nullptr;
Expand Down
8 changes: 4 additions & 4 deletions src/client/file_instance.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class CURVE_CACHELINE_ALIGNMENT FileInstance {
* @return: 成功返回true、否则返回false
*/
bool Initialize(const std::string& filename,
MDSClient* mdsclient,
std::shared_ptr<MDSClient> mdsclient,
const UserInfo_t& userinfo,
const FileServiceOption& fileservicopt,
bool readonly = false);
Expand Down Expand Up @@ -152,13 +152,13 @@ class CURVE_CACHELINE_ALIGNMENT FileInstance {

static FileInstance* NewInitedFileInstance(
const FileServiceOption& fileServiceOption,
MDSClient* mdsClient,
std::shared_ptr<MDSClient> mdsClient,
const std::string& filename,
const UserInfo& userInfo,
bool readonly);

static FileInstance* Open4Readonly(const FileServiceOption& opt,
MDSClient* mdsclient,
std::shared_ptr<MDSClient> mdsclient,
const std::string& filename,
const UserInfo& userInfo);

Expand All @@ -170,7 +170,7 @@ class CURVE_CACHELINE_ALIGNMENT FileInstance {
FileServiceOption fileopt_;

// MDSClient是FileInstance与mds通信的唯一出口
MDSClient* mdsclient_;
std::shared_ptr<MDSClient> mdsclient_;

// 每个文件都持有与MDS通信的lease，LeaseExecutor是续约执行者
std::unique_ptr<LeaseExecutor> leaseExecutor_;
Expand Down
36 changes: 19 additions & 17 deletions src/client/libcurve_file.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <memory>
#include <mutex> // NOLINT
#include <thread> // NOLINT
#include <utility>

#include "include/client/libcurve.h"
#include "include/curve_compiler_specific.h"
Expand Down Expand Up @@ -96,10 +97,17 @@ void InitLogging(const std::string& confPath) {
static LoggerGuard guard(confPath);
}

FileClient::FileClient(): fdcount_(0), openedFileNum_("opened_file_num") {
inited_ = false;
mdsClient_ = nullptr;
fileserviceMap_.clear();
FileClient::FileClient()
: rwlock_(),
fdcount_(0),
fileserviceMap_(),
clientconfig_(),
mdsClient_(),
inited_(false),
openedFileNum_() {
std::ostringstream oss;
oss << "0x" << std::hex << reinterpret_cast<uint64_t>(this);
openedFileNum_.expose(oss.str());
}

int FileClient::Init(const std::string& configpath) {
Expand All @@ -116,17 +124,13 @@ int FileClient::Init(const std::string& configpath) {
return -LIBCURVE_ERROR::FAILED;
}

mdsClient_ = new (std::nothrow) MDSClient();
if (mdsClient_ == nullptr) {
return -LIBCURVE_ERROR::FAILED;
}

auto ret = mdsClient_->Initialize(clientconfig_.
GetFileServiceOption().metaServerOpt);
auto tmpMdsClient = std::make_shared<MDSClient>();

auto ret = tmpMdsClient->Initialize(
clientconfig_.GetFileServiceOption().metaServerOpt);
if (LIBCURVE_ERROR::OK != ret) {
LOG(ERROR) << "Init global mds client failed!";
delete mdsClient_;
mdsClient_ = nullptr;
return -LIBCURVE_ERROR::FAILED;
}

Expand All @@ -139,12 +143,11 @@ int FileClient::Init(const std::string& configpath) {

bool rc = StartDummyServer();
if (rc == false) {
mdsClient_->UnInitialize();
delete mdsClient_;
mdsClient_ = nullptr;
tmpMdsClient->UnInitialize();
return -LIBCURVE_ERROR::FAILED;
}

mdsClient_ = std::move(tmpMdsClient);
inited_ = true;
return LIBCURVE_ERROR::OK;
}
Expand All @@ -163,8 +166,7 @@ void FileClient::UnInit() {

if (mdsClient_ != nullptr) {
mdsClient_->UnInitialize();
delete mdsClient_;
mdsClient_ = nullptr;
mdsClient_.reset();
}
inited_ = false;
}
Expand Down
28 changes: 2 additions & 26 deletions src/client/libcurve_file.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <string>
#include <unordered_map>
#include <vector>
#include <memory>

#include "include/client/libcurve.h"
#include "src/client/client_common.h"
Expand Down Expand Up @@ -314,31 +315,6 @@ class FileClient {
return openedFileNum_.get_value();
}

/**
* test use, set the mdsclient_
*/
void SetMdsClient(MDSClient* client) {
mdsClient_ = client;
}

/**
* test use, set the clientconfig_
*/
void SetClientConfig(ClientConfig cfg) {
clientconfig_ = cfg;
}

const ClientConfig& GetClientConfig() {
return clientconfig_;
}

/**
* test use, get the fileserviceMap_
*/
std::unordered_map<int, FileInstance*>& GetFileServiceMap() {
return fileserviceMap_;
}

private:
bool StartDummyServer();

Expand All @@ -360,7 +336,7 @@ class FileClient {
ClientConfig clientconfig_;

// fileclient对应的全局mdsclient
MDSClient* mdsClient_;
std::shared_ptr<MDSClient> mdsClient_;

// 是否初始化成功
bool inited_;
Expand Down
Loading

0 comments on commit 75c0f8a

Please sign in to comment.