From ebd7af34c19d98b7a7d3879e44a01f0341391469 Mon Sep 17 00:00:00 2001 From: Soumya Koduri Date: Mon, 3 Jan 2022 17:41:38 +0530 Subject: [PATCH] rgw/dbstore: Use mutex to protect DB objectmap and prepared stmt Signed-off-by: Soumya Koduri --- src/rgw/store/dbstore/common/dbstore.cc | 67 ++++-------------------- src/rgw/store/dbstore/common/dbstore.h | 65 +++++++++++------------ src/rgw/store/dbstore/sqlite/sqliteDB.cc | 1 + src/rgw/store/dbstore/sqlite/sqliteDB.h | 2 +- 4 files changed, 44 insertions(+), 91 deletions(-) diff --git a/src/rgw/store/dbstore/common/dbstore.cc b/src/rgw/store/dbstore/common/dbstore.cc index 6a56bc338a852..719ec62c074d2 100644 --- a/src/rgw/store/dbstore/common/dbstore.cc +++ b/src/rgw/store/dbstore/common/dbstore.cc @@ -39,20 +39,10 @@ int DB::Initialize(string logfile, int loglevel) return ret; } - ret = LockInit(dpp); - - if (ret) { - ldpp_dout(dpp, 0) <<"Error: mutex is NULL " << dendl; - closeDB(dpp); - db = NULL; - return ret; - } - ret = InitializeDBOps(dpp); if (ret) { ldpp_dout(dpp, 0) <<"InitializeDBOps failed " << dendl; - LockDestroy(dpp); closeDB(dpp); db = NULL; return ret; @@ -71,7 +61,6 @@ int DB::Destroy(const DoutPrefixProvider *dpp) closeDB(dpp); - LockDestroy(dpp); FreeDBOps(dpp); @@ -81,49 +70,6 @@ int DB::Destroy(const DoutPrefixProvider *dpp) return 0; } -int DB::LockInit(const DoutPrefixProvider *dpp) { - int ret; - - ret = pthread_mutex_init(&mutex, NULL); - - if (ret) - ldpp_dout(dpp, 0)<<"pthread_mutex_init failed " << dendl; - - return ret; -} - -int DB::LockDestroy(const DoutPrefixProvider *dpp) { - int ret; - - ret = pthread_mutex_destroy(&mutex); - - if (ret) - ldpp_dout(dpp, 0)<<"pthread_mutex_destroy failed " << dendl; - - return ret; -} - -int DB::Lock(const DoutPrefixProvider *dpp) { - int ret; - - ret = pthread_mutex_lock(&mutex); - - if (ret) - ldpp_dout(dpp, 0)<<"pthread_mutex_lock failed " << dendl; - - return ret; -} - -int DB::Unlock(const DoutPrefixProvider *dpp) { - int ret; - - ret = pthread_mutex_unlock(&mutex); - - if (ret) - ldpp_dout(dpp, 0)<<"pthread_mutex_unlock failed " << dendl; - - return ret; -} DBOp *DB::getDBOp(const DoutPrefixProvider *dpp, string Op, struct DBOpParams *params) { @@ -162,7 +108,10 @@ DBOp *DB::getDBOp(const DoutPrefixProvider *dpp, string Op, struct DBOpParams *p map::iterator iter; class ObjectOp* Ob; - iter = DB::objectmap.find(params->op.bucket.info.bucket.name); + { + const std::lock_guard lk(mtx); + iter = DB::objectmap.find(params->op.bucket.info.bucket.name); + } if (iter == DB::objectmap.end()) { ldpp_dout(dpp, 30)<<"No objectmap found for bucket: " \ @@ -195,20 +144,23 @@ DBOp *DB::getDBOp(const DoutPrefixProvider *dpp, string Op, struct DBOpParams *p return NULL; } -int DB::objectmapInsert(const DoutPrefixProvider *dpp, string bucket, void *ptr) +int DB::objectmapInsert(const DoutPrefixProvider *dpp, string bucket, class ObjectOp* ptr) { map::iterator iter; class ObjectOp *Ob; + const std::lock_guard lk(mtx); iter = DB::objectmap.find(bucket); if (iter != DB::objectmap.end()) { // entry already exists // return success or replace it or // return error ? - // return success for now + // + // return success for now & delete the newly allocated ptr ldpp_dout(dpp, 20)<<"Objectmap entry already exists for bucket("\ <::iterator iter; class ObjectOp *Ob; + const std::lock_guard lk(mtx); iter = DB::objectmap.find(bucket); if (iter == DB::objectmap.end()) { diff --git a/src/rgw/store/dbstore/common/dbstore.h b/src/rgw/store/dbstore/common/dbstore.h index 01b55908c5b73..dd4f2468931d9 100644 --- a/src/rgw/store/dbstore/common/dbstore.h +++ b/src/rgw/store/dbstore/common/dbstore.h @@ -620,8 +620,9 @@ class DBOp { const string ListAllQ = "SELECT * from '{}'"; public: - DBOp() {}; - virtual ~DBOp() {}; + DBOp() {} + virtual ~DBOp() {} + std::mutex mtx; // to protect prepared stmt string CreateTableSchema(string type, DBOpParams *params) { if (!type.compare("User")) @@ -663,10 +664,11 @@ class DBOp { } virtual int Prepare(const DoutPrefixProvider *dpp, DBOpParams *params) { return 0; } + virtual int Bind(const DoutPrefixProvider *dpp, DBOpParams *params) { return 0; } virtual int Execute(const DoutPrefixProvider *dpp, DBOpParams *params) { return 0; } }; -class InsertUserOp : public DBOp { +class InsertUserOp : virtual public DBOp { private: /* For existing entires, - * (1) INSERT or REPLACE - it will delete previous entry and then @@ -711,7 +713,7 @@ class InsertUserOp : public DBOp { }; -class RemoveUserOp: public DBOp { +class RemoveUserOp: virtual public DBOp { private: const string Query = "DELETE from '{}' where UserID = {}"; @@ -725,7 +727,7 @@ class RemoveUserOp: public DBOp { } }; -class GetUserOp: public DBOp { +class GetUserOp: virtual public DBOp { private: /* If below query columns are updated, make sure to update the indexes * in list_user() cbk in sqliteDB.cc */ @@ -786,7 +788,7 @@ class GetUserOp: public DBOp { } }; -class InsertBucketOp: public DBOp { +class InsertBucketOp: virtual public DBOp { private: const string Query = "INSERT OR REPLACE INTO '{}' \ @@ -822,7 +824,7 @@ class InsertBucketOp: public DBOp { } }; -class UpdateBucketOp: public DBOp { +class UpdateBucketOp: virtual public DBOp { private: // Updates Info, Mtime, Version const string InfoQuery = @@ -875,7 +877,7 @@ class UpdateBucketOp: public DBOp { } }; -class RemoveBucketOp: public DBOp { +class RemoveBucketOp: virtual public DBOp { private: const string Query = "DELETE from '{}' where BucketName = {}"; @@ -889,7 +891,7 @@ class RemoveBucketOp: public DBOp { } }; -class GetBucketOp: public DBOp { +class GetBucketOp: virtual public DBOp { private: const string Query = "SELECT \ BucketName, BucketTable.Tenant, Marker, BucketID, Size, SizeRounded, CreationTime, \ @@ -912,7 +914,7 @@ class GetBucketOp: public DBOp { } }; -class ListUserBucketsOp: public DBOp { +class ListUserBucketsOp: virtual public DBOp { private: // once we have stats also stored, may have to update this query to join // these two tables. @@ -935,7 +937,7 @@ class ListUserBucketsOp: public DBOp { } }; -class PutObjectOp: public DBOp { +class PutObjectOp: virtual public DBOp { private: const string Query = "INSERT OR REPLACE INTO '{}' \ @@ -984,7 +986,7 @@ class PutObjectOp: public DBOp { } }; -class DeleteObjectOp: public DBOp { +class DeleteObjectOp: virtual public DBOp { private: const string Query = "DELETE from '{}' where BucketName = {} and ObjName = {} and ObjInstance = {}"; @@ -1000,7 +1002,7 @@ class DeleteObjectOp: public DBOp { } }; -class GetObjectOp: public DBOp { +class GetObjectOp: virtual public DBOp { private: const string Query = "SELECT \ @@ -1027,7 +1029,7 @@ class GetObjectOp: public DBOp { } }; -class ListBucketObjectsOp: public DBOp { +class ListBucketObjectsOp: virtual public DBOp { private: // once we have stats also stored, may have to update this query to join // these two tables. @@ -1056,7 +1058,7 @@ class ListBucketObjectsOp: public DBOp { } }; -class UpdateObjectOp: public DBOp { +class UpdateObjectOp: virtual public DBOp { private: // Updates Omap const string OmapQuery = @@ -1144,7 +1146,7 @@ class UpdateObjectOp: public DBOp { } }; -class PutObjectDataOp: public DBOp { +class PutObjectDataOp: virtual public DBOp { private: const string Query = "INSERT OR REPLACE INTO '{}' \ @@ -1168,7 +1170,7 @@ class PutObjectDataOp: public DBOp { } }; -class UpdateObjectDataOp: public DBOp { +class UpdateObjectDataOp: virtual public DBOp { private: const string Query = "UPDATE '{}' \ @@ -1189,7 +1191,7 @@ class UpdateObjectDataOp: public DBOp { params.op.bucket.bucket_name.c_str()); } }; -class GetObjectDataOp: public DBOp { +class GetObjectDataOp: virtual public DBOp { private: const string Query = "SELECT \ @@ -1208,7 +1210,7 @@ class GetObjectDataOp: public DBOp { } }; -class DeleteObjectDataOp: public DBOp { +class DeleteObjectDataOp: virtual public DBOp { private: const string Query = "DELETE from '{}' where BucketName = {} and ObjName = {} and ObjInstance = {}"; @@ -1225,7 +1227,7 @@ class DeleteObjectDataOp: public DBOp { } }; -class InsertLCEntryOp: public DBOp { +class InsertLCEntryOp: virtual public DBOp { private: const string Query = "INSERT OR REPLACE INTO '{}' \ @@ -1242,7 +1244,7 @@ class InsertLCEntryOp: public DBOp { } }; -class RemoveLCEntryOp: public DBOp { +class RemoveLCEntryOp: virtual public DBOp { private: const string Query = "DELETE from '{}' where LCIndex = {} and BucketName = {}"; @@ -1256,7 +1258,7 @@ class RemoveLCEntryOp: public DBOp { } }; -class GetLCEntryOp: public DBOp { +class GetLCEntryOp: virtual public DBOp { private: const string Query = "SELECT \ LCIndex, BucketName, StartTime, Status \ @@ -1279,7 +1281,7 @@ class GetLCEntryOp: public DBOp { } }; -class ListLCEntriesOp: public DBOp { +class ListLCEntriesOp: virtual public DBOp { private: const string Query = "SELECT \ LCIndex, BucketName, StartTime, Status \ @@ -1295,7 +1297,7 @@ class ListLCEntriesOp: public DBOp { } }; -class InsertLCHeadOp: public DBOp { +class InsertLCHeadOp: virtual public DBOp { private: const string Query = "INSERT OR REPLACE INTO '{}' \ @@ -1312,7 +1314,7 @@ class InsertLCHeadOp: public DBOp { } }; -class RemoveLCHeadOp: public DBOp { +class RemoveLCHeadOp: virtual public DBOp { private: const string Query = "DELETE from '{}' where LCIndex = {}"; @@ -1326,7 +1328,7 @@ class RemoveLCHeadOp: public DBOp { } }; -class GetLCHeadOp: public DBOp { +class GetLCHeadOp: virtual public DBOp { private: const string Query = "SELECT \ LCIndex, Marker, StartDate \ @@ -1372,12 +1374,6 @@ class DB { const string lc_head_table; const string lc_entry_table; static map objectmap; - pthread_mutex_t mutex; // to protect objectmap and other shared - // objects if any. This mutex is taken - // before processing every fop (i.e, in - // ProcessOp()). If required this can be - // made further granular by taking separate - // locks for objectmap and db operations etc. protected: void *db; @@ -1387,6 +1383,9 @@ class DB { // XXX: default ObjStripeSize or ObjChunk size - 4M, make them configurable? uint64_t ObjHeadSize = 1024; /* 1K - default head data size */ uint64_t ObjChunkSize = (get_blob_limit() - 1000); /* 1000 to accommodate other fields */ + // Below mutex is to protect objectmap and other shared + // objects if any. + std::mutex mtx; public: DB(string db_name, CephContext *_cct) : db_name(db_name), @@ -1448,7 +1447,7 @@ class DB { int InitializeParams(const DoutPrefixProvider *dpp, string Op, DBOpParams *params); int ProcessOp(const DoutPrefixProvider *dpp, string Op, DBOpParams *params); DBOp* getDBOp(const DoutPrefixProvider *dpp, string Op, struct DBOpParams *params); - int objectmapInsert(const DoutPrefixProvider *dpp, string bucket, void *ptr); + int objectmapInsert(const DoutPrefixProvider *dpp, string bucket, class ObjectOp* ptr); int objectmapDelete(const DoutPrefixProvider *dpp, string bucket); virtual uint64_t get_blob_limit() { return 0; }; diff --git a/src/rgw/store/dbstore/sqlite/sqliteDB.cc b/src/rgw/store/dbstore/sqlite/sqliteDB.cc index a7979b392d582..f24eb68b2e25a 100644 --- a/src/rgw/store/dbstore/sqlite/sqliteDB.cc +++ b/src/rgw/store/dbstore/sqlite/sqliteDB.cc @@ -118,6 +118,7 @@ #define SQL_EXECUTE(dpp, params, stmt, cbk, args...) \ do{ \ + const std::lock_guard lk(((DBOp*)(this))->mtx); \ if (!stmt) { \ ret = Prepare(dpp, params); \ } \ diff --git a/src/rgw/store/dbstore/sqlite/sqliteDB.h b/src/rgw/store/dbstore/sqlite/sqliteDB.h index 3636cebade629..5bf7dfdd16f78 100644 --- a/src/rgw/store/dbstore/sqlite/sqliteDB.h +++ b/src/rgw/store/dbstore/sqlite/sqliteDB.h @@ -13,7 +13,7 @@ using namespace std; using namespace rgw::store; -class SQLiteDB : public DB, public DBOp{ +class SQLiteDB : public DB, virtual public DBOp { private: sqlite3_mutex *mutex = NULL; -- 2.39.5