From b3a17b67a69142eef1b4adde3409d5e54dda1e0b Mon Sep 17 00:00:00 2001
From: Amar Tumballi <amarts@redhat.com>
Date: Sat, 9 Feb 2019 13:23:06 +0530
Subject: [PATCH 507/511] inode: make critical section smaller
do all the 'static' tasks outside of locked region.
* hash_dentry() and hash_gfid() are now called outside locked region.
* remove extra __dentry_hash exported in libglusterfs.sym
* avoid checks in locked functions, if the check is done in calling
function.
* implement dentry_destroy(), which handles freeing of dentry separately,
from that of dentry_unset (which takes care of separating dentry from
inode, and table)
> Updates: bz#1670031
> Change-Id: I584213e0748464bb427fbdef3c4ab6615d7d5eb0
> Signed-off-by: Amar Tumballi <amarts@redhat.com>
> (Cherry pick from commit 8a90d346b9d3f69ff11241feb0011c90a8e57e30)
> (Review on upstream link https://review.gluster.org/#/c/glusterfs/+/22184/)
Change-Id: I584213e0748464bb427fbdef3c4ab6615d7d5eb0
BUG: 1898777
Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/221189
Tested-by: RHGS Build Bot <nigelb@redhat.com>
Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com>
---
libglusterfs/src/glusterfs/inode.h | 3 -
libglusterfs/src/inode.c | 323 +++++++++++++------------------------
libglusterfs/src/libglusterfs.sym | 1 -
3 files changed, 111 insertions(+), 216 deletions(-)
diff --git a/libglusterfs/src/glusterfs/inode.h b/libglusterfs/src/glusterfs/inode.h
index 4421c47..c875653 100644
--- a/libglusterfs/src/glusterfs/inode.h
+++ b/libglusterfs/src/glusterfs/inode.h
@@ -167,9 +167,6 @@ inode_rename(inode_table_t *table, inode_t *olddir, const char *oldname,
inode_t *newdir, const char *newname, inode_t *inode,
struct iatt *stbuf);
-dentry_t *
-__dentry_grep(inode_table_t *table, inode_t *parent, const char *name);
-
inode_t *
inode_grep(inode_table_t *table, inode_t *parent, const char *name);
diff --git a/libglusterfs/src/inode.c b/libglusterfs/src/inode.c
index 4c3c546..71b2d2a 100644
--- a/libglusterfs/src/inode.c
+++ b/libglusterfs/src/inode.c
@@ -159,27 +159,15 @@ hash_dentry(inode_t *parent, const char *name, int mod)
static int
hash_gfid(uuid_t uuid, int mod)
{
- int ret = 0;
-
- ret = uuid[15] + (uuid[14] << 8);
-
- return ret;
+ return ((uuid[15] + (uuid[14] << 8)) % mod);
}
static void
-__dentry_hash(dentry_t *dentry)
+__dentry_hash(dentry_t *dentry, const int hash)
{
inode_table_t *table = NULL;
- int hash = 0;
-
- if (!dentry) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_DENTRY_NOT_FOUND,
- "dentry not found");
- return;
- }
table = dentry->inode->table;
- hash = hash_dentry(dentry->parent, dentry->name, table->hashsize);
list_del_init(&dentry->hash);
list_add(&dentry->hash, &table->name_hash[hash]);
@@ -188,49 +176,44 @@ __dentry_hash(dentry_t *dentry)
static int
__is_dentry_hashed(dentry_t *dentry)
{
- if (!dentry) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_DENTRY_NOT_FOUND,
- "dentry not found");
- return 0;
- }
-
return !list_empty(&dentry->hash);
}
static void
__dentry_unhash(dentry_t *dentry)
{
- if (!dentry) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_DENTRY_NOT_FOUND,
- "dentry not found");
- return;
- }
-
list_del_init(&dentry->hash);
}
static void
-__dentry_unset(dentry_t *dentry)
+dentry_destroy(dentry_t *dentry)
{
- if (!dentry) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_DENTRY_NOT_FOUND,
- "dentry not found");
+ if (!dentry)
return;
- }
+
+ GF_FREE(dentry->name);
+ dentry->name = NULL;
+ mem_put(dentry);
+
+ return;
+}
+
+static dentry_t *
+__dentry_unset(dentry_t *dentry)
+{
+ if (!dentry)
+ return NULL;
__dentry_unhash(dentry);
list_del_init(&dentry->inode_list);
- GF_FREE(dentry->name);
- dentry->name = NULL;
-
if (dentry->parent) {
__inode_unref(dentry->parent, false);
dentry->parent = NULL;
}
- mem_put(dentry);
+ return dentry;
}
static int
@@ -289,22 +272,14 @@ static int
__is_dentry_cyclic(dentry_t *dentry)
{
int ret = 0;
- inode_t *inode = NULL;
- char *name = "<nul>";
ret = __foreach_ancestor_dentry(dentry, __check_cycle, dentry->inode);
if (ret) {
- inode = dentry->inode;
-
- if (dentry->name)
- name = dentry->name;
-
gf_msg(dentry->inode->table->name, GF_LOG_CRITICAL, 0,
LG_MSG_DENTRY_CYCLIC_LOOP,
- "detected cyclic loop "
- "formation during inode linkage. inode (%s) linking "
- "under itself as %s",
- uuid_utoa(inode->gfid), name);
+ "detected cyclic loop formation during inode linkage. "
+ "inode (%s) linking under itself as %s",
+ uuid_utoa(dentry->inode->gfid), dentry->name);
}
return ret;
@@ -313,41 +288,19 @@ __is_dentry_cyclic(dentry_t *dentry)
static void
__inode_unhash(inode_t *inode)
{
- if (!inode) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_INODE_NOT_FOUND,
- "inode not found");
- return;
- }
-
list_del_init(&inode->hash);
}
static int
__is_inode_hashed(inode_t *inode)
{
- if (!inode) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_INODE_NOT_FOUND,
- "inode not found");
- return 0;
- }
-
return !list_empty(&inode->hash);
}
static void
-__inode_hash(inode_t *inode)
+__inode_hash(inode_t *inode, const int hash)
{
- inode_table_t *table = NULL;
- int hash = 0;
-
- if (!inode) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_INODE_NOT_FOUND,
- "inode not found");
- return;
- }
-
- table = inode->table;
- hash = hash_gfid(inode->gfid, 65536);
+ inode_table_t *table = inode->table;
list_del_init(&inode->hash);
list_add(&inode->hash, &table->inode_hash[hash]);
@@ -359,12 +312,6 @@ __dentry_search_for_inode(inode_t *inode, uuid_t pargfid, const char *name)
dentry_t *dentry = NULL;
dentry_t *tmp = NULL;
- if (!inode || !name) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, EINVAL, LG_MSG_INVALID_ARG,
- "inode || name not found");
- return NULL;
- }
-
/* earlier, just the ino was sent, which could have been 0, now
we deal with gfid, and if sent gfid is null or 0, no need to
continue with the check */
@@ -390,12 +337,6 @@ __inode_ctx_free(inode_t *inode)
xlator_t *xl = NULL;
xlator_t *old_THIS = NULL;
- if (!inode) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_INODE_NOT_FOUND,
- "inode not found");
- return;
- }
-
if (!inode->_ctx) {
gf_msg(THIS->name, GF_LOG_WARNING, 0, LG_MSG_CTX_NULL,
"_ctx not found");
@@ -424,12 +365,6 @@ noctx:
static void
__inode_destroy(inode_t *inode)
{
- if (!inode) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_INODE_NOT_FOUND,
- "inode not found");
- return;
- }
-
__inode_ctx_free(inode);
LOCK_DESTROY(&inode->lock);
@@ -472,9 +407,6 @@ inode_ctx_merge(fd_t *fd, inode_t *inode, inode_t *linked_inode)
static void
__inode_activate(inode_t *inode)
{
- if (!inode)
- return;
-
list_move(&inode->list, &inode->table->active);
inode->table->active_size++;
}
@@ -485,19 +417,13 @@ __inode_passivate(inode_t *inode)
dentry_t *dentry = NULL;
dentry_t *t = NULL;
- if (!inode) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_INODE_NOT_FOUND,
- "inode not found");
- return;
- }
-
list_move_tail(&inode->list, &inode->table->lru);
inode->table->lru_size++;
list_for_each_entry_safe(dentry, t, &inode->dentry_list, inode_list)
{
if (!__is_dentry_hashed(dentry))
- __dentry_unset(dentry);
+ dentry_destroy(__dentry_unset(dentry));
}
}
@@ -507,12 +433,6 @@ __inode_retire(inode_t *inode)
dentry_t *dentry = NULL;
dentry_t *t = NULL;
- if (!inode) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_INODE_NOT_FOUND,
- "inode not found");
- return;
- }
-
list_move_tail(&inode->list, &inode->table->purge);
inode->table->purge_size++;
@@ -520,7 +440,7 @@ __inode_retire(inode_t *inode)
list_for_each_entry_safe(dentry, t, &inode->dentry_list, inode_list)
{
- __dentry_unset(dentry);
+ dentry_destroy(__dentry_unset(dentry));
}
}
@@ -547,9 +467,6 @@ __inode_unref(inode_t *inode, bool clear)
xlator_t *this = NULL;
uint64_t nlookup = 0;
- if (!inode)
- return NULL;
-
/*
* Root inode should always be in active list of inode table. So unrefs
* on root inode are no-ops.
@@ -677,16 +594,10 @@ inode_ref(inode_t *inode)
}
static dentry_t *
-__dentry_create(inode_t *inode, inode_t *parent, const char *name)
+dentry_create(inode_t *inode, inode_t *parent, const char *name)
{
dentry_t *newd = NULL;
- if (!inode || !parent || !name) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, EINVAL, LG_MSG_INVALID_ARG,
- "inode || parent || name not found");
- return NULL;
- }
-
newd = mem_get0(parent->table->dentry_pool);
if (newd == NULL) {
goto out;
@@ -702,10 +613,6 @@ __dentry_create(inode_t *inode, inode_t *parent, const char *name)
goto out;
}
- if (parent)
- newd->parent = __inode_ref(parent, false);
-
- list_add(&newd->inode_list, &inode->dentry_list);
newd->inode = inode;
out:
@@ -717,14 +624,6 @@ __inode_create(inode_table_t *table)
{
inode_t *newi = NULL;
- if (!table) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0,
- LG_MSG_INODE_TABLE_NOT_FOUND,
- "table not "
- "found");
- return NULL;
- }
-
newi = mem_get0(table->inode_pool);
if (!newi) {
goto out;
@@ -795,9 +694,6 @@ __inode_ref_reduce_by_n(inode_t *inode, uint64_t nref)
{
uint64_t nlookup = 0;
- if (!inode)
- return NULL;
-
GF_ASSERT(inode->ref >= nref);
inode->ref -= nref;
@@ -837,17 +733,12 @@ inode_forget_atomic(inode_t *inode, uint64_t nlookup)
}
dentry_t *
-__dentry_grep(inode_table_t *table, inode_t *parent, const char *name)
+__dentry_grep(inode_table_t *table, inode_t *parent, const char *name,
+ const int hash)
{
- int hash = 0;
dentry_t *dentry = NULL;
dentry_t *tmp = NULL;
- if (!table || !name || !parent)
- return NULL;
-
- hash = hash_dentry(parent, name, table->hashsize);
-
list_for_each_entry(tmp, &table->name_hash[hash], hash)
{
if (tmp->parent == parent && !strcmp(tmp->name, name)) {
@@ -872,15 +763,16 @@ inode_grep(inode_table_t *table, inode_t *parent, const char *name)
return NULL;
}
+ int hash = hash_dentry(parent, name, table->hashsize);
+
pthread_mutex_lock(&table->lock);
{
- dentry = __dentry_grep(table, parent, name);
-
- if (dentry)
+ dentry = __dentry_grep(table, parent, name, hash);
+ if (dentry) {
inode = dentry->inode;
-
- if (inode)
- __inode_ref(inode, false);
+ if (inode)
+ __inode_ref(inode, false);
+ }
}
pthread_mutex_unlock(&table->lock);
@@ -947,17 +839,18 @@ inode_grep_for_gfid(inode_table_t *table, inode_t *parent, const char *name,
return ret;
}
+ int hash = hash_dentry(parent, name, table->hashsize);
+
pthread_mutex_lock(&table->lock);
{
- dentry = __dentry_grep(table, parent, name);
-
- if (dentry)
+ dentry = __dentry_grep(table, parent, name, hash);
+ if (dentry) {
inode = dentry->inode;
-
- if (inode) {
- gf_uuid_copy(gfid, inode->gfid);
- *type = inode->ia_type;
- ret = 0;
+ if (inode) {
+ gf_uuid_copy(gfid, inode->gfid);
+ *type = inode->ia_type;
+ ret = 0;
+ }
}
}
pthread_mutex_unlock(&table->lock);
@@ -978,25 +871,14 @@ __is_root_gfid(uuid_t gfid)
}
inode_t *
-__inode_find(inode_table_t *table, uuid_t gfid)
+__inode_find(inode_table_t *table, uuid_t gfid, const int hash)
{
inode_t *inode = NULL;
inode_t *tmp = NULL;
- int hash = 0;
-
- if (!table) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0,
- LG_MSG_INODE_TABLE_NOT_FOUND,
- "table not "
- "found");
- goto out;
- }
if (__is_root_gfid(gfid))
return table->root;
- hash = hash_gfid(gfid, 65536);
-
list_for_each_entry(tmp, &table->inode_hash[hash], hash)
{
if (gf_uuid_compare(tmp->gfid, gfid) == 0) {
@@ -1005,7 +887,6 @@ __inode_find(inode_table_t *table, uuid_t gfid)
}
}
-out:
return inode;
}
@@ -1022,9 +903,11 @@ inode_find(inode_table_t *table, uuid_t gfid)
return NULL;
}
+ int hash = hash_gfid(gfid, 65536);
+
pthread_mutex_lock(&table->lock);
{
- inode = __inode_find(table, gfid);
+ inode = __inode_find(table, gfid, hash);
if (inode)
__inode_ref(inode, false);
}
@@ -1035,7 +918,7 @@ inode_find(inode_table_t *table, uuid_t gfid)
static inode_t *
__inode_link(inode_t *inode, inode_t *parent, const char *name,
- struct iatt *iatt)
+ struct iatt *iatt, const int dhash)
{
dentry_t *dentry = NULL;
dentry_t *old_dentry = NULL;
@@ -1043,16 +926,7 @@ __inode_link(inode_t *inode, inode_t *parent, const char *name,
inode_table_t *table = NULL;
inode_t *link_inode = NULL;
- if (!inode) {
- errno = EINVAL;
- return NULL;
- }
-
table = inode->table;
- if (!table) {
- errno = EINVAL;
- return NULL;
- }
if (parent) {
/* We should prevent inode linking between different
@@ -1090,14 +964,16 @@ __inode_link(inode_t *inode, inode_t *parent, const char *name,
return NULL;
}
- old_inode = __inode_find(table, iatt->ia_gfid);
+ int ihash = hash_gfid(iatt->ia_gfid, 65536);
+
+ old_inode = __inode_find(table, iatt->ia_gfid, ihash);
if (old_inode) {
link_inode = old_inode;
} else {
gf_uuid_copy(inode->gfid, iatt->ia_gfid);
inode->ia_type = iatt->ia_type;
- __inode_hash(inode);
+ __inode_hash(inode, ihash);
}
} else {
/* @old_inode serves another important purpose - it indicates
@@ -1112,22 +988,16 @@ __inode_link(inode_t *inode, inode_t *parent, const char *name,
old_inode = inode;
}
- if (name) {
- if (!strcmp(name, ".") || !strcmp(name, ".."))
- return link_inode;
-
- if (strchr(name, '/')) {
- GF_ASSERT(!"inode link attempted with '/' in name");
- return NULL;
- }
+ if (name && (!strcmp(name, ".") || !strcmp(name, ".."))) {
+ return link_inode;
}
/* use only link_inode beyond this point */
if (parent) {
- old_dentry = __dentry_grep(table, parent, name);
+ old_dentry = __dentry_grep(table, parent, name, dhash);
if (!old_dentry || old_dentry->inode != link_inode) {
- dentry = __dentry_create(link_inode, parent, name);
+ dentry = dentry_create(link_inode, parent, name);
if (!dentry) {
gf_msg_callingfn(
THIS->name, GF_LOG_ERROR, 0, LG_MSG_DENTRY_CREATE_FAILED,
@@ -1137,15 +1007,20 @@ __inode_link(inode_t *inode, inode_t *parent, const char *name,
errno = ENOMEM;
return NULL;
}
+
+ /* dentry linking needs to happen inside lock */
+ dentry->parent = __inode_ref(parent, false);
+ list_add(&dentry->inode_list, &link_inode->dentry_list);
+
if (old_inode && __is_dentry_cyclic(dentry)) {
errno = ELOOP;
- __dentry_unset(dentry);
+ dentry_destroy(__dentry_unset(dentry));
return NULL;
}
- __dentry_hash(dentry);
+ __dentry_hash(dentry, dhash);
if (old_dentry)
- __dentry_unset(old_dentry);
+ dentry_destroy(__dentry_unset(old_dentry));
}
}
@@ -1155,6 +1030,7 @@ __inode_link(inode_t *inode, inode_t *parent, const char *name,
inode_t *
inode_link(inode_t *inode, inode_t *parent, const char *name, struct iatt *iatt)
{
+ int hash = 0;
inode_table_t *table = NULL;
inode_t *linked_inode = NULL;
@@ -1166,10 +1042,18 @@ inode_link(inode_t *inode, inode_t *parent, const char *name, struct iatt *iatt)
table = inode->table;
+ if (parent && name) {
+ hash = hash_dentry(parent, name, table->hashsize);
+ }
+
+ if (name && strchr(name, '/')) {
+ GF_ASSERT(!"inode link attempted with '/' in name");
+ return NULL;
+ }
+
pthread_mutex_lock(&table->lock);
{
- linked_inode = __inode_link(inode, parent, name, iatt);
-
+ linked_inode = __inode_link(inode, parent, name, iatt, hash);
if (linked_inode)
__inode_ref(linked_inode, false);
}
@@ -1312,48 +1196,47 @@ inode_invalidate(inode_t *inode)
return ret;
}
-static void
+static dentry_t *
__inode_unlink(inode_t *inode, inode_t *parent, const char *name)
{
dentry_t *dentry = NULL;
char pgfid[64] = {0};
char gfid[64] = {0};
- if (!inode || !parent || !name)
- return;
-
dentry = __dentry_search_for_inode(inode, parent->gfid, name);
/* dentry NULL for corrupted backend */
if (dentry) {
- __dentry_unset(dentry);
+ dentry = __dentry_unset(dentry);
} else {
gf_msg("inode", GF_LOG_WARNING, 0, LG_MSG_DENTRY_NOT_FOUND,
"%s/%s: dentry not found in %s",
uuid_utoa_r(parent->gfid, pgfid), name,
uuid_utoa_r(inode->gfid, gfid));
}
+
+ return dentry;
}
void
inode_unlink(inode_t *inode, inode_t *parent, const char *name)
{
- inode_table_t *table = NULL;
+ inode_table_t *table;
+ dentry_t *dentry;
- if (!inode) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_INODE_NOT_FOUND,
- "inode not found");
+ if (!inode || !parent || !name)
return;
- }
table = inode->table;
pthread_mutex_lock(&table->lock);
{
- __inode_unlink(inode, parent, name);
+ dentry = __inode_unlink(inode, parent, name);
}
pthread_mutex_unlock(&table->lock);
+ dentry_destroy(dentry);
+
inode_table_prune(table);
}
@@ -1362,6 +1245,9 @@ inode_rename(inode_table_t *table, inode_t *srcdir, const char *srcname,
inode_t *dstdir, const char *dstname, inode_t *inode,
struct iatt *iatt)
{
+ int hash = 0;
+ dentry_t *dentry = NULL;
+
if (!inode) {
gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_INODE_NOT_FOUND,
"inode not found");
@@ -1370,13 +1256,26 @@ inode_rename(inode_table_t *table, inode_t *srcdir, const char *srcname,
table = inode->table;
+ if (dstname && strchr(dstname, '/')) {
+ GF_ASSERT(!"inode link attempted with '/' in name");
+ return -1;
+ }
+
+ if (dstdir && dstname) {
+ hash = hash_dentry(dstdir, dstname, table->hashsize);
+ }
+
pthread_mutex_lock(&table->lock);
{
- __inode_link(inode, dstdir, dstname, iatt);
- __inode_unlink(inode, srcdir, srcname);
+ __inode_link(inode, dstdir, dstname, iatt, hash);
+ /* pick the old dentry */
+ dentry = __inode_unlink(inode, srcdir, srcname);
}
pthread_mutex_unlock(&table->lock);
+ /* free the old dentry */
+ dentry_destroy(dentry);
+
inode_table_prune(table);
return 0;
@@ -1447,12 +1346,6 @@ inode_parent(inode_t *inode, uuid_t pargfid, const char *name)
static int
__inode_has_dentry(inode_t *inode)
{
- if (!inode) {
- gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_INODE_NOT_FOUND,
- "inode not found");
- return 0;
- }
-
return !list_empty(&inode->dentry_list);
}
@@ -1461,6 +1354,12 @@ inode_has_dentry(inode_t *inode)
{
int dentry_present = 0;
+ if (!inode) {
+ gf_msg_callingfn(THIS->name, GF_LOG_WARNING, 0, LG_MSG_INODE_NOT_FOUND,
+ "inode not found");
+ return 0;
+ }
+
LOCK(&inode->lock);
{
dentry_present = __inode_has_dentry(inode);
@@ -1720,7 +1619,7 @@ __inode_table_init_root(inode_table_t *table)
iatt.ia_ino = 1;
iatt.ia_type = IA_IFDIR;
- __inode_link(root, NULL, NULL, &iatt);
+ __inode_link(root, NULL, NULL, &iatt, 0);
table->root = root;
}
diff --git a/libglusterfs/src/libglusterfs.sym b/libglusterfs/src/libglusterfs.sym
index 5a721e0..d060292 100644
--- a/libglusterfs/src/libglusterfs.sym
+++ b/libglusterfs/src/libglusterfs.sym
@@ -357,7 +357,6 @@ default_copy_file_range
default_copy_file_range_cbk
default_copy_file_range_failure_cbk
default_copy_file_range_resume
-__dentry_grep
dht_is_linkfile
dict_add
dict_addn
--
1.8.3.1