From d544399c921895b81240d2efd9371d2c8139faac Mon Sep 17 00:00:00 2001
From: Ashish Pandey <aspandey@redhat.com>
Date: Mon, 3 Apr 2017 12:46:29 +0530
Subject: [PATCH 491/509] cluster/ec: Update xattr and heal size properly
Problem-1 : Recursive healing of same file is happening
when IO is going on even after data heal completes.
Solution:
RCA: At the end of the write, when ec_update_size_version
gets called, we send it only on good bricks and not
on healing brick. Due to this, xattr on healing brick
will always remain out of sync and when the background
heal check source and sink, it finds this brick to be
healed and start healing from scratch. That involve
ftruncate and writing all of the data again.
To solve this, send xattrop on all the good bricks as
well as healing bricks.
Problem-2: The above fix exposes the data corruption
during heal. If the write on a file is going on and
heal finishes, we find that the file gets corrupted.
RCA:
The real problem happens in ec_rebuild_data(). Here we receive the
'size' argument which contains the real file size at the time of
starting self-heal and it's assigned to heal->total_size.
After that, a sequence of calls to ec_sync_heal_block() are done. Each
call ends up calling ec_manager_heal_block(), which does the actual work
of healing a block.
First a lock on the inode is taken in state EC_STATE_INIT using
ec_heal_inodelk(). When the lock is acquired, ec_heal_lock_cbk() is
called. This function calls ec_set_inode_size() to store the real size
of the inode (it uses heal->total_size).
The next step is to read the block to be healed. This is done using a
regular ec_readv(). One of the things this call does is to trim the
returned size if the file is smaller than the requested size.
In our case, when we read the last block of a file whose size was = 512
mod 1024 at the time of starting self-heal, ec_readv() will return only
the first 512 bytes, not the whole 1024 bytes.
This isn't a problem since the following ec_writev() sent from the heal
code only attempts to write the amount of data read, so it shouldn't
modify the remaining 512 bytes.
However ec_writev() also checks the file size. If we are writing the
last block of the file (determined by the size stored on the inode that
we have set to heal->total_size), any data beyond the (imposed) end of
file will be cleared with 0's. This causes the 512 bytes after the
heal->total_size to be cleared. Since the file was written after heal
started, the these bytes contained data, so the block written to the
damaged brick will be incorrect.
Solution:
Align heal->total_size to a multiple of the stripe size.
Thanks "Xavier Hernandez" <xhernandez@datalab.es>
to find out the root cause and to fix the issue.
>Change-Id: I6c9f37b3ff9dd7f5dc1858ad6f9845c05b4e204e
>BUG: 1428673
>Signed-off-by: Ashish Pandey <aspandey@redhat.com>
>Reviewed-on: https://review.gluster.org/16985
>Smoke: Gluster Build System <jenkins@build.gluster.org>
>NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
>CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
>Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
>Reviewed-by: Xavier Hernandez <xhernandez@datalab.es>
>Signed-off-by: Ashish Pandey <aspandey@redhat.com>
Change-Id: I6c9f37b3ff9dd7f5dc1858ad6f9845c05b4e204e
BUG: 1427159
Signed-off-by: Ashish Pandey <aspandey@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/108397
Reviewed-by: Atin Mukherjee <amukherj@redhat.com>
---
tests/basic/ec/ec-data-heal.t | 87 ++++++++++++++++++++++++++++++++++++++
tests/include.rc | 1 +
xlators/cluster/ec/src/ec-common.c | 11 ++++-
xlators/cluster/ec/src/ec-heal.c | 17 +++++---
4 files changed, 107 insertions(+), 9 deletions(-)
create mode 100755 tests/basic/ec/ec-data-heal.t
diff --git a/tests/basic/ec/ec-data-heal.t b/tests/basic/ec/ec-data-heal.t
new file mode 100755
index 0000000..4599c8a
--- /dev/null
+++ b/tests/basic/ec/ec-data-heal.t
@@ -0,0 +1,87 @@
+#!/bin/bash
+
+. $(dirname $0)/../../include.rc
+. $(dirname $0)/../../volume.rc
+
+#This test checks data corruption after heal while IO is going on
+
+cleanup
+TEST glusterd
+TEST pidof glusterd
+TEST $CLI volume create $V0 disperse 3 redundancy 1 $H0:$B0/${V0}{0..2}
+TEST $CLI volume start $V0
+
+TEST $GFS --volfile-id=/$V0 --volfile-server=$H0 $M0;
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "3" ec_child_up_count $V0 0
+
+############ Start IO ###########
+TEST touch $M0/file
+#start background IO on file
+dd if=/dev/urandom of=$M0/file conv=fdatasync &
+iopid=$(echo $!)
+
+
+############ Kill and start brick0 for heal ###########
+brick0=$(ps -p $(get_brick_pid $V0 $H0 $B0/${V0}0) -o args)
+WORDTOREMOVE=COMMAND
+brick0=${brick0//$WORDTOREMOVE/}
+TEST kill_brick $V0 $H0 $B0/${V0}0
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "2" ec_child_up_count $V0 0
+#sleep so that data can be written which will be healed later
+sleep 10
+TEST eval $brick0
+##wait for heal info to become 0 and kill IO
+EXPECT_WITHIN $IO_HEAL_TIMEOUT "^0$" get_pending_heal_count $V0
+kill $iopid
+EXPECT_WITHIN $IO_HEAL_TIMEOUT "^0$" get_pending_heal_count $V0
+
+############### Check md5sum #########################
+
+## unmount and mount get md5sum after killing brick0
+
+brick0=$(ps -p $(get_brick_pid $V0 $H0 $B0/${V0}0) -o args)
+WORDTOREMOVE=COMMAND
+brick0=${brick0//$WORDTOREMOVE/}
+TEST kill_brick $V0 $H0 $B0/${V0}0
+
+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0
+TEST $GFS --volfile-id=/$V0 --volfile-server=$H0 $M0;
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "2" ec_child_up_count $V0 0
+mdsum0=`md5sum $M0/file | awk '{print $1}'`
+TEST eval $brick0
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "3" ec_child_up_count $V0 0
+
+## unmount and mount get md5sum after killing brick1
+
+brick1=$(ps -p $(get_brick_pid $V0 $H0 $B0/${V0}1) -o args)
+WORDTOREMOVE=COMMAND
+brick1=${brick1//$WORDTOREMOVE/}
+TEST kill_brick $V0 $H0 $B0/${V0}1
+
+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0
+TEST $GFS --volfile-id=/$V0 --volfile-server=$H0 $M0;
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "2" ec_child_up_count $V0 0
+mdsum1=`md5sum $M0/file | awk '{print $1}'`
+TEST eval $brick1
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "3" ec_child_up_count $V0 0
+
+## unmount and mount get md5sum after killing brick2
+
+brick2=$(ps -p $(get_brick_pid $V0 $H0 $B0/${V0}2) -o args)
+WORDTOREMOVE=COMMAND
+brick2=${brick2//$WORDTOREMOVE/}
+TEST kill_brick $V0 $H0 $B0/${V0}2
+
+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0
+TEST $GFS --volfile-id=/$V0 --volfile-server=$H0 $M0;
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "2" ec_child_up_count $V0 0
+mdsum2=`md5sum $M0/file | awk '{print $1}'`
+TEST eval $brick2
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "3" ec_child_up_count $V0 0
+
+# compare all the three md5sums
+EXPECT "$mdsum0" echo $mdsum1
+EXPECT "$mdsum0" echo $mdsum2
+EXPECT "$mdsum1" echo $mdsum2
+
+cleanup
diff --git a/tests/include.rc b/tests/include.rc
index ce2085f..21aabf5 100644
--- a/tests/include.rc
+++ b/tests/include.rc
@@ -70,6 +70,7 @@ PROBE_TIMEOUT=60
REBALANCE_TIMEOUT=360
REOPEN_TIMEOUT=20
HEAL_TIMEOUT=80
+IO_HEAL_TIMEOUT=120
MARKER_UPDATE_TIMEOUT=20
JANITOR_TIMEOUT=60
UMOUNT_TIMEOUT=5
diff --git a/xlators/cluster/ec/src/ec-common.c b/xlators/cluster/ec/src/ec-common.c
index 1720d88..9a977d7 100644
--- a/xlators/cluster/ec/src/ec-common.c
+++ b/xlators/cluster/ec/src/ec-common.c
@@ -1755,6 +1755,9 @@ ec_lock_next_owner(ec_lock_link_t *link, ec_cbk_data_t *cbk,
}
}
+ if (fop->healing) {
+ lock->healing = fop->healing & (fop->good | fop->remaining);
+ }
ec_lock_update_good(lock, fop);
lock->exclusive -= (fop->flags & EC_FLAG_LOCK_SHARED) == 0;
@@ -1952,6 +1955,8 @@ ec_update_size_version(ec_lock_link_t *link, uint64_t *version,
ec_lock_t *lock;
ec_inode_t *ctx;
dict_t * dict;
+ uintptr_t update_on = 0;
+
int32_t err = -ENOMEM;
fop = link->fop;
@@ -2006,12 +2011,14 @@ ec_update_size_version(ec_lock_link_t *link, uint64_t *version,
fop->frame->root->uid = 0;
fop->frame->root->gid = 0;
+ update_on = lock->good_mask | lock->healing;
+
if (link->lock->fd == NULL) {
- ec_xattrop(fop->frame, fop->xl, lock->good_mask, EC_MINIMUM_MIN,
+ ec_xattrop(fop->frame, fop->xl, update_on, EC_MINIMUM_MIN,
ec_update_size_version_done, link, &link->lock->loc,
GF_XATTROP_ADD_ARRAY64, dict, NULL);
} else {
- ec_fxattrop(fop->frame, fop->xl, lock->good_mask, EC_MINIMUM_MIN,
+ ec_fxattrop(fop->frame, fop->xl, update_on, EC_MINIMUM_MIN,
ec_update_size_version_done, link, link->lock->fd,
GF_XATTROP_ADD_ARRAY64, dict, NULL);
}
diff --git a/xlators/cluster/ec/src/ec-heal.c b/xlators/cluster/ec/src/ec-heal.c
index 02e8123..ae60d2f 100644
--- a/xlators/cluster/ec/src/ec-heal.c
+++ b/xlators/cluster/ec/src/ec-heal.c
@@ -1908,8 +1908,8 @@ ec_rebuild_data (call_frame_t *frame, ec_t *ec, fd_t *fd, uint64_t size,
heal->fd = fd_ref (fd);
heal->xl = ec->xl;
heal->data = &barrier;
- syncbarrier_init (heal->data);
pool = ec->xl->ctx->iobuf_pool;
+ size = ec_adjust_size (ec, size, 0);
heal->total_size = size;
heal->size = iobpool_default_pagesize (pool);
/* We need to adjust the size to a multiple of the stripe size of the
@@ -1947,13 +1947,15 @@ ec_rebuild_data (call_frame_t *frame, ec_t *ec, fd_t *fd, uint64_t size,
}
int
-__ec_heal_trim_sinks (call_frame_t *frame, ec_t *ec, fd_t *fd,
- unsigned char *healed_sinks, unsigned char *trim)
+__ec_heal_trim_sinks (call_frame_t *frame, ec_t *ec,
+ fd_t *fd, unsigned char *healed_sinks,
+ unsigned char *trim, uint64_t size)
{
default_args_cbk_t *replies = NULL;
unsigned char *output = NULL;
int ret = 0;
int i = 0;
+ off_t trim_offset = 0;
EC_REPLIES_ALLOC (replies, ec->nodes);
output = alloca0 (ec->nodes);
@@ -1962,9 +1964,9 @@ __ec_heal_trim_sinks (call_frame_t *frame, ec_t *ec, fd_t *fd,
ret = 0;
goto out;
}
-
+ trim_offset = ec_adjust_size (ec, size, 1);
ret = cluster_ftruncate (ec->xl_list, trim, ec->nodes, replies, output,
- frame, ec->xl, fd, 0, NULL);
+ frame, ec->xl, fd, trim_offset, NULL);
for (i = 0; i < ec->nodes; i++) {
if (!output[i] && trim[i])
healed_sinks[i] = 0;
@@ -2018,7 +2020,7 @@ ec_data_undo_pending (call_frame_t *frame, ec_t *ec, fd_t *fd, dict_t *xattr,
if ((memcmp (versions_xattr, allzero, sizeof (allzero)) == 0) &&
(memcmp (dirty_xattr, allzero, sizeof (allzero)) == 0) &&
- (size == 0)) {
+ (size_xattr == 0)) {
ret = 0;
goto out;
}
@@ -2224,7 +2226,8 @@ __ec_heal_data (call_frame_t *frame, ec_t *ec, fd_t *fd, unsigned char *heal_on,
if (ret < 0)
goto unlock;
- ret = __ec_heal_trim_sinks (frame, ec, fd, healed_sinks, trim);
+ ret = __ec_heal_trim_sinks (frame, ec, fd, healed_sinks,
+ trim, size[source]);
}
unlock:
cluster_uninodelk (ec->xl_list, locked_on, ec->nodes, replies, output,
--
1.8.3.1