From 52d71ad0e5c27808e7d8eea8a0920298837e408c Mon Sep 17 00:00:00 2001

From: Xavi Hernandez <xhernandez@redhat.com>

Date: Wed, 17 Jul 2019 14:50:22 +0200

Subject: [PATCH 271/276] cluster/ec: fix EIO error for concurrent writes on

 sparse files

EC doesn't allow concurrent writes on overlapping areas, they are

serialized. However non-overlapping writes are serviced in parallel.

When a write is not aligned, EC first needs to read the entire chunk

from disk, apply the modified fragment and write it again.

The problem appears on sparse files because a write to an offset

implicitly creates data on offsets below it (so, in some way, they

are overlapping). For example, if a file is empty and we read 10 bytes

from offset 10, read() will return 0 bytes. Now, if we write one byte

at offset 1M and retry the same read, the system call will return 10

bytes (all containing 0's).

So if we have two writes, the first one at offset 10 and the second one

at offset 1M, EC will send both in parallel because they do not overlap.

However, the first one will try to read missing data from the first chunk

(i.e. offsets 0 to 9) to recombine the entire chunk and do the final write.

This read will happen in parallel with the write to 1M. What could happen

is that half of the bricks process the write before the read, and the

half do the read before the write. Some bricks will return 10 bytes of

data while the otherw will return 0 bytes (because the file on the brick

has not been expanded yet).

When EC tries to recombine the answers from the bricks, it can't, because

it needs more than half consistent answers to recover the data. So this

read fails with EIO error. This error is propagated to the parent write,

which is aborted and EIO is returned to the application.

The issue happened because EC assumed that a write to a given offset

implies that offsets below it exist.

This fix prevents the read of the chunk from bricks if the current size

of the file is smaller than the read chunk offset. This size is

correctly tracked, so this fixes the issue.

Also modifying ec-stripe.t file for Test #13 within it.

In this patch, if a file size is less than the offset we are writing, we

fill zeros in head and tail and do not consider it strip cache miss.

That actually make sense as we know what data that part holds and there is

no need of reading it from bricks.

Upstream-patch: https://review.gluster.org/c/glusterfs/+/23066

Change-Id: Ic342e8c35c555b8534109e9314c9a0710b6225d6

fixes: bz#1731448

Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>

Reviewed-on: https://code.engineering.redhat.com/gerrit/177975

Tested-by: RHGS Build Bot <nigelb@redhat.com>

Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com>

---

 tests/basic/ec/ec-stripe.t              |  2 +-

 xlators/cluster/ec/src/ec-inode-write.c | 26 +++++++++++++++++---------

 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/tests/basic/ec/ec-stripe.t b/tests/basic/ec/ec-stripe.t

index 1e940eb..98b9229 100644

--- a/tests/basic/ec/ec-stripe.t

+++ b/tests/basic/ec/ec-stripe.t

@@ -202,7 +202,7 @@ TEST truncate -s 0 $B0/test_file

 TEST truncate -s 0 $M0/test_file

 TEST dd if=$B0/misc_file of=$B0/test_file  bs=1022 count=5  oflag=seek_bytes,sync seek=400 conv=notrunc

 TEST dd if=$B0/misc_file of=$M0/test_file  bs=1022 count=5  oflag=seek_bytes,sync seek=400 conv=notrunc

-check_statedump_md5sum 4 5

+check_statedump_md5sum 4 4

 clean_file_unmount

 ### 14 - Truncate to invalidate  all but one the stripe in cache  ####

diff --git a/xlators/cluster/ec/src/ec-inode-write.c b/xlators/cluster/ec/src/ec-inode-write.c

index ea55140..a45e6d6 100644

--- a/xlators/cluster/ec/src/ec-inode-write.c

+++ b/xlators/cluster/ec/src/ec-inode-write.c

@@ -2013,20 +2013,28 @@ ec_writev_start(ec_fop_data_t *fop)

     if (err != 0) {

         goto failed_fd;

     }

+    tail = fop->size - fop->user_size - fop->head;

     if (fop->head > 0) {

-        found_stripe = ec_get_and_merge_stripe(ec, fop, EC_STRIPE_HEAD);

-        if (!found_stripe) {

-            if (ec_make_internal_fop_xdata(&xdata)) {

-                err = -ENOMEM;

-                goto failed_xdata;

+        if (current > fop->offset) {

+            found_stripe = ec_get_and_merge_stripe(ec, fop, EC_STRIPE_HEAD);

+            if (!found_stripe) {

+                if (ec_make_internal_fop_xdata(&xdata)) {

+                    err = -ENOMEM;

+                    goto failed_xdata;

+                }

+                ec_readv(fop->frame, fop->xl, -1, EC_MINIMUM_MIN,

+                         ec_writev_merge_head, NULL, fd, ec->stripe_size,

+                         fop->offset, 0, xdata);

+            }

+        } else {

+            memset(fop->vector[0].iov_base, 0, fop->head);

+            memset(fop->vector[0].iov_base + fop->size - tail, 0, tail);

+            if (ec->stripe_cache && (fop->size <= ec->stripe_size)) {

+                ec_add_stripe_in_cache(ec, fop);

             }

-            ec_readv(fop->frame, fop->xl, -1, EC_MINIMUM_MIN,

-                     ec_writev_merge_head, NULL, fd, ec->stripe_size,

-                     fop->offset, 0, xdata);

         }

     }

-    tail = fop->size - fop->user_size - fop->head;

     if ((tail > 0) && ((fop->head == 0) || (fop->size > ec->stripe_size))) {

         /* Current locking scheme will make sure the 'current' below will

          * never decrease while the fop is in progress, so the checks will

-- 

1.8.3.1