|
 |
233933 |
From 9912a432dc3493007462f76c5933d04a160814ae Mon Sep 17 00:00:00 2001
|
|
|
233933 |
From: Pranith Kumar K <pkarampu@redhat.com>
|
|
|
233933 |
Date: Thu, 20 Jun 2019 17:05:49 +0530
|
|
|
233933 |
Subject: [PATCH 198/221] cluster/ec: Prevent double pre-op xattrops
|
|
|
233933 |
|
|
|
233933 |
Problem:
|
|
|
233933 |
Race:
|
|
|
233933 |
Thread-1 Thread-2
|
|
|
233933 |
1) Does ec_get_size_version() to perform
|
|
|
233933 |
pre-op fxattrop as part of write-1
|
|
|
233933 |
2) Calls ec_set_dirty_flag() in
|
|
|
233933 |
ec_get_size_version() for write-2.
|
|
|
233933 |
This sets dirty[] to 1
|
|
|
233933 |
3) Completes executing
|
|
|
233933 |
ec_prepare_update_cbk leading to
|
|
|
233933 |
ctx->dirty[] = '1'
|
|
|
233933 |
4) Takes LOCK(inode->lock) to check if there are
|
|
|
233933 |
any flags and sets dirty-flag because
|
|
|
233933 |
lock->waiting_flag is 0 now. This leads to
|
|
|
233933 |
fxattrop to increment on-disk dirty[] to '2'
|
|
|
233933 |
|
|
|
233933 |
At the end of the writes the file will be marked for heal even when it doesn't need heal.
|
|
|
233933 |
|
|
|
233933 |
Fix:
|
|
|
233933 |
Perform ec_set_dirty_flag() and other checks inside LOCK() to prevent dirty[] to be marked
|
|
|
233933 |
as '1' in step 2) above
|
|
|
233933 |
|
|
|
233933 |
> Upstream-patch: https://review.gluster.org/c/glusterfs/+/22907
|
|
|
233933 |
|
|
|
233933 |
fixes: bz#1600918
|
|
|
233933 |
Change-Id: Icac2ab39c0b1e7e154387800fbededc561612865
|
|
|
233933 |
Signed-off-by: Pranith Kumar K <pkarampu@redhat.com>
|
|
|
233933 |
Reviewed-on: https://code.engineering.redhat.com/gerrit/174385
|
|
|
233933 |
Reviewed-by: Atin Mukherjee <amukherj@redhat.com>
|
|
|
233933 |
Tested-by: Atin Mukherjee <amukherj@redhat.com>
|
|
|
233933 |
---
|
|
|
233933 |
tests/basic/ec/ec-dirty-flags.t | 23 +++++++++++++++++++++++
|
|
|
233933 |
xlators/cluster/ec/src/ec-common.c | 13 +++++++------
|
|
|
233933 |
2 files changed, 30 insertions(+), 6 deletions(-)
|
|
|
233933 |
create mode 100644 tests/basic/ec/ec-dirty-flags.t
|
|
|
233933 |
|
|
|
233933 |
diff --git a/tests/basic/ec/ec-dirty-flags.t b/tests/basic/ec/ec-dirty-flags.t
|
|
|
233933 |
new file mode 100644
|
|
|
233933 |
index 0000000..68e6610
|
|
|
233933 |
--- /dev/null
|
|
|
233933 |
+++ b/tests/basic/ec/ec-dirty-flags.t
|
|
|
233933 |
@@ -0,0 +1,23 @@
|
|
|
233933 |
+#!/bin/bash
|
|
|
233933 |
+
|
|
|
233933 |
+. $(dirname $0)/../../include.rc
|
|
|
233933 |
+. $(dirname $0)/../../volume.rc
|
|
|
233933 |
+
|
|
|
233933 |
+# This checks if the fop keeps the dirty flags settings correctly after
|
|
|
233933 |
+# finishing the fop.
|
|
|
233933 |
+
|
|
|
233933 |
+cleanup
|
|
|
233933 |
+TEST glusterd
|
|
|
233933 |
+TEST pidof glusterd
|
|
|
233933 |
+TEST $CLI volume create $V0 disperse 3 redundancy 1 $H0:$B0/${V0}{0..2}
|
|
|
233933 |
+TEST $CLI volume heal $V0 disable
|
|
|
233933 |
+TEST $CLI volume start $V0
|
|
|
233933 |
+
|
|
|
233933 |
+TEST $GFS --volfile-id=/$V0 --volfile-server=$H0 $M0;
|
|
|
233933 |
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "3" ec_child_up_count $V0 0
|
|
|
233933 |
+cd $M0
|
|
|
233933 |
+for i in {1..1000}; do dd if=/dev/zero of=file-${i} bs=512k count=2; done
|
|
|
233933 |
+cd -
|
|
|
233933 |
+EXPECT "^0$" get_pending_heal_count $V0
|
|
|
233933 |
+
|
|
|
233933 |
+cleanup
|
|
|
233933 |
diff --git a/xlators/cluster/ec/src/ec-common.c b/xlators/cluster/ec/src/ec-common.c
|
|
|
233933 |
index 9cc6395..35c2256 100644
|
|
|
233933 |
--- a/xlators/cluster/ec/src/ec-common.c
|
|
|
233933 |
+++ b/xlators/cluster/ec/src/ec-common.c
|
|
|
233933 |
@@ -1405,6 +1405,10 @@ ec_get_size_version(ec_lock_link_t *link)
|
|
|
233933 |
!ec_is_data_fop(fop->id))
|
|
|
233933 |
link->optimistic_changelog = _gf_true;
|
|
|
233933 |
|
|
|
233933 |
+ memset(&loc, 0, sizeof(loc));
|
|
|
233933 |
+
|
|
|
233933 |
+ LOCK(&lock->loc.inode->lock);
|
|
|
233933 |
+
|
|
|
233933 |
set_dirty = ec_set_dirty_flag(link, ctx, dirty);
|
|
|
233933 |
|
|
|
233933 |
/* If ec metadata has already been retrieved, do not try again. */
|
|
|
233933 |
@@ -1412,20 +1416,16 @@ ec_get_size_version(ec_lock_link_t *link)
|
|
|
233933 |
if (ec_is_data_fop(fop->id)) {
|
|
|
233933 |
fop->healing |= lock->healing;
|
|
|
233933 |
}
|
|
|
233933 |
- return;
|
|
|
233933 |
+ goto unlock;
|
|
|
233933 |
}
|
|
|
233933 |
|
|
|
233933 |
/* Determine if there's something we need to retrieve for the current
|
|
|
233933 |
* operation. */
|
|
|
233933 |
if (!set_dirty && !lock->query && (lock->loc.inode->ia_type != IA_IFREG) &&
|
|
|
233933 |
(lock->loc.inode->ia_type != IA_INVAL)) {
|
|
|
233933 |
- return;
|
|
|
233933 |
+ goto unlock;
|
|
|
233933 |
}
|
|
|
233933 |
|
|
|
233933 |
- memset(&loc, 0, sizeof(loc));
|
|
|
233933 |
-
|
|
|
233933 |
- LOCK(&lock->loc.inode->lock);
|
|
|
233933 |
-
|
|
|
233933 |
changed_flags = ec_set_xattrop_flags_and_params(lock, link, dirty);
|
|
|
233933 |
if (link->waiting_flags) {
|
|
|
233933 |
/* This fop needs to wait until all its flags are cleared which
|
|
|
233933 |
@@ -1436,6 +1436,7 @@ ec_get_size_version(ec_lock_link_t *link)
|
|
|
233933 |
GF_ASSERT(!changed_flags);
|
|
|
233933 |
}
|
|
|
233933 |
|
|
|
233933 |
+unlock:
|
|
|
233933 |
UNLOCK(&lock->loc.inode->lock);
|
|
|
233933 |
|
|
|
233933 |
if (!changed_flags)
|
|
|
233933 |
--
|
|
|
233933 |
1.8.3.1
|
|
|
233933 |
|