You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
194 lines
6.5 KiB
194 lines
6.5 KiB
From ad233c1b3abdfe2bdfd1eacc83b5f84b7afa6b46 Mon Sep 17 00:00:00 2001 |
|
From: N Balachandran <nbalacha@redhat.com> |
|
Date: Tue, 1 Oct 2019 17:37:15 +0530 |
|
Subject: [PATCH 304/304] cluster/dht: Correct fd processing loop |
|
|
|
The fd processing loops in the |
|
dht_migration_complete_check_task and the |
|
dht_rebalance_inprogress_task functions were unsafe |
|
and could cause an open to be sent on an already freed |
|
fd. This has been fixed. |
|
|
|
> Change-Id: I0a3c7d2fba314089e03dfd704f9dceb134749540 |
|
> Fixes: bz#1757399 |
|
> Signed-off-by: N Balachandran <nbalacha@redhat.com> |
|
> (Cherry picked from commit 9b15867070b0cc241ab165886292ecffc3bc0aed) |
|
> (Reviewed on upstream link https://review.gluster.org/#/c/glusterfs/+/23506/) |
|
|
|
Change-Id: I0a3c7d2fba314089e03dfd704f9dceb134749540 |
|
BUG: 1756325 |
|
Signed-off-by: Mohit Agrawal <moagrawa@redhat.com> |
|
Reviewed-on: https://code.engineering.redhat.com/gerrit/182826 |
|
Tested-by: RHGS Build Bot <nigelb@redhat.com> |
|
Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com> |
|
--- |
|
xlators/cluster/dht/src/dht-helper.c | 84 ++++++++++++++++++++++++++---------- |
|
1 file changed, 62 insertions(+), 22 deletions(-) |
|
|
|
diff --git a/xlators/cluster/dht/src/dht-helper.c b/xlators/cluster/dht/src/dht-helper.c |
|
index 4c57e0d..1e9fee0 100644 |
|
--- a/xlators/cluster/dht/src/dht-helper.c |
|
+++ b/xlators/cluster/dht/src/dht-helper.c |
|
@@ -1261,6 +1261,7 @@ dht_migration_complete_check_task(void *data) |
|
fd_t *tmp = NULL; |
|
uint64_t tmp_miginfo = 0; |
|
dht_migrate_info_t *miginfo = NULL; |
|
+ gf_boolean_t skip_open = _gf_false; |
|
int open_failed = 0; |
|
|
|
this = THIS; |
|
@@ -1399,24 +1400,34 @@ dht_migration_complete_check_task(void *data) |
|
* the loop will cause the destruction of the fd. So we need to |
|
* iterate the list safely because iter_fd cannot be trusted. |
|
*/ |
|
- list_for_each_entry_safe(iter_fd, tmp, &inode->fd_list, inode_list) |
|
- { |
|
- if (fd_is_anonymous(iter_fd)) |
|
- continue; |
|
- |
|
- if (dht_fd_open_on_dst(this, iter_fd, dst_node)) |
|
- continue; |
|
- |
|
+ iter_fd = list_entry((&inode->fd_list)->next, typeof(*iter_fd), inode_list); |
|
+ while (&iter_fd->inode_list != (&inode->fd_list)) { |
|
+ if (fd_is_anonymous(iter_fd) || |
|
+ (dht_fd_open_on_dst(this, iter_fd, dst_node))) { |
|
+ if (!tmp) { |
|
+ iter_fd = list_entry(iter_fd->inode_list.next, typeof(*iter_fd), |
|
+ inode_list); |
|
+ continue; |
|
+ } |
|
+ skip_open = _gf_true; |
|
+ } |
|
/* We need to release the inode->lock before calling |
|
* syncop_open() to avoid possible deadlocks. However this |
|
* can cause the iter_fd to be released by other threads. |
|
* To avoid this, we take a reference before releasing the |
|
* lock. |
|
*/ |
|
- __fd_ref(iter_fd); |
|
+ fd_ref(iter_fd); |
|
|
|
UNLOCK(&inode->lock); |
|
|
|
+ if (tmp) { |
|
+ fd_unref(tmp); |
|
+ tmp = NULL; |
|
+ } |
|
+ if (skip_open) |
|
+ goto next; |
|
+ |
|
/* flags for open are stripped down to allow following the |
|
* new location of the file, otherwise we can get EEXIST or |
|
* truncate the file again as rebalance is moving the data */ |
|
@@ -1438,9 +1449,11 @@ dht_migration_complete_check_task(void *data) |
|
dht_fd_ctx_set(this, iter_fd, dst_node); |
|
} |
|
|
|
- fd_unref(iter_fd); |
|
- |
|
+ next: |
|
LOCK(&inode->lock); |
|
+ skip_open = _gf_false; |
|
+ tmp = iter_fd; |
|
+ iter_fd = list_entry(tmp->inode_list.next, typeof(*tmp), inode_list); |
|
} |
|
|
|
SYNCTASK_SETID(frame->root->uid, frame->root->gid); |
|
@@ -1453,6 +1466,10 @@ dht_migration_complete_check_task(void *data) |
|
|
|
unlock: |
|
UNLOCK(&inode->lock); |
|
+ if (tmp) { |
|
+ fd_unref(tmp); |
|
+ tmp = NULL; |
|
+ } |
|
|
|
out: |
|
if (dict) { |
|
@@ -1534,6 +1551,7 @@ dht_rebalance_inprogress_task(void *data) |
|
int open_failed = 0; |
|
uint64_t tmp_miginfo = 0; |
|
dht_migrate_info_t *miginfo = NULL; |
|
+ gf_boolean_t skip_open = _gf_false; |
|
|
|
this = THIS; |
|
frame = data; |
|
@@ -1654,24 +1672,40 @@ dht_rebalance_inprogress_task(void *data) |
|
* the loop will cause the destruction of the fd. So we need to |
|
* iterate the list safely because iter_fd cannot be trusted. |
|
*/ |
|
- list_for_each_entry_safe(iter_fd, tmp, &inode->fd_list, inode_list) |
|
- { |
|
- if (fd_is_anonymous(iter_fd)) |
|
- continue; |
|
- |
|
- if (dht_fd_open_on_dst(this, iter_fd, dst_node)) |
|
- continue; |
|
- |
|
+ iter_fd = list_entry((&inode->fd_list)->next, typeof(*iter_fd), inode_list); |
|
+ while (&iter_fd->inode_list != (&inode->fd_list)) { |
|
/* We need to release the inode->lock before calling |
|
* syncop_open() to avoid possible deadlocks. However this |
|
* can cause the iter_fd to be released by other threads. |
|
* To avoid this, we take a reference before releasing the |
|
* lock. |
|
*/ |
|
- __fd_ref(iter_fd); |
|
|
|
+ if (fd_is_anonymous(iter_fd) || |
|
+ (dht_fd_open_on_dst(this, iter_fd, dst_node))) { |
|
+ if (!tmp) { |
|
+ iter_fd = list_entry(iter_fd->inode_list.next, typeof(*iter_fd), |
|
+ inode_list); |
|
+ continue; |
|
+ } |
|
+ skip_open = _gf_true; |
|
+ } |
|
+ |
|
+ /* Yes, this is ugly but there isn't a cleaner way to do this |
|
+ * the fd_ref is an atomic increment so not too bad. We want to |
|
+ * reduce the number of inode locks and unlocks. |
|
+ */ |
|
+ |
|
+ fd_ref(iter_fd); |
|
UNLOCK(&inode->lock); |
|
|
|
+ if (tmp) { |
|
+ fd_unref(tmp); |
|
+ tmp = NULL; |
|
+ } |
|
+ if (skip_open) |
|
+ goto next; |
|
+ |
|
/* flags for open are stripped down to allow following the |
|
* new location of the file, otherwise we can get EEXIST or |
|
* truncate the file again as rebalance is moving the data */ |
|
@@ -1692,9 +1726,11 @@ dht_rebalance_inprogress_task(void *data) |
|
dht_fd_ctx_set(this, iter_fd, dst_node); |
|
} |
|
|
|
- fd_unref(iter_fd); |
|
- |
|
+ next: |
|
LOCK(&inode->lock); |
|
+ skip_open = _gf_false; |
|
+ tmp = iter_fd; |
|
+ iter_fd = list_entry(tmp->inode_list.next, typeof(*tmp), inode_list); |
|
} |
|
|
|
SYNCTASK_SETID(frame->root->uid, frame->root->gid); |
|
@@ -1702,6 +1738,10 @@ dht_rebalance_inprogress_task(void *data) |
|
unlock: |
|
UNLOCK(&inode->lock); |
|
|
|
+ if (tmp) { |
|
+ fd_unref(tmp); |
|
+ tmp = NULL; |
|
+ } |
|
if (open_failed) { |
|
ret = -1; |
|
goto out; |
|
-- |
|
1.8.3.1 |
|
|
|
|