osd: introduce sub-chunks to erasure code plugin interface #15193

mynaramana · 2017-05-22T04:26:34Z

Introducing sub-chunks. Added functions decode2, minimum_to_decode2 to the ErasureCodeInterface.
Updated ECBackend, ECUtil to use the new functions.
Authors: Myna, Elita
Related PR: 14300.

Fixes: http://tracker.ceph.com/issues/19278

Signed-off-by: Myna Vajha mynaramana@gmail.com

liewegas · 2017-05-22T13:21:47Z

jenkins test this please

liewegas · 2017-05-23T01:50:59Z

retest this please

liewegas · 2017-05-23T21:27:11Z

this needs to be rebased on latest master, please!

liewegas

Two main things are:

i think a vector will work better than list for these since it'll avoid hammer on the allocator
whitespace cleanup
rebase!

Thanks!

liewegas · 2017-05-23T21:28:17Z

src/erasure-code/ErasureCode.cc

@@ -60,6 +60,28 @@ int ErasureCode::minimum_to_decode(const set<int> &want_to_read,
  return 0;
 }

+int ErasureCode::minimum_to_decode2(const set<int> &want_to_read,
+                                   const set<int> &available_chunks,
+                                   map<int, list<pair<int, int>>> *minimum)


can we replace the list<pair<int,int>> with vector<pair<int,int>>? it'll be a zillion times faster to avoid the memory allocations.

liewegas · 2017-05-23T21:28:55Z

src/erasure-code/ErasureCode.cc

+  default_subchunks.push_back(make_pair(0,get_sub_chunk_count()));
+
+
+  for(set<int>::iterator i=minimum_shard_ids.begin();


for (auto id : minimum_shard_ids) {

liewegas · 2017-05-23T21:29:06Z

src/erasure-code/ErasureCode.cc

+
+  if ( r != 0) return r;
+
+  list<pair<int, int>> default_subchunks;


liewegas · 2017-05-23T21:30:06Z

src/erasure-code/ErasureCode.cc

+  set<int> minimum_shard_ids;
+  int r = minimum_to_decode(want_to_read, available_chunks, &minimum_shard_ids);
+
+  if ( r != 0) return r;


please follow CodingStyle... in this case, no blank like after 'int r = ...', no space after 'if (', braces and newlines around 'return r'

liewegas · 2017-05-23T21:30:23Z

src/erasure-code/ErasureCode.h

+
+    virtual int minimum_to_decode2(const set<int> &want_to_read,
+                                  const set<int> &available,
+                                  map<int, list<pair<int,int>>> *minimum);


list -> vector throughout this patch...

liewegas · 2017-05-23T21:32:03Z

src/osd/ECBackend.cc

-	j->get<1>(),
-	bl, j->get<2>(),
-	true); // Allow EIO return
+      if((op.subchunks.find(i->first)->second.size() == 1) && (op.subchunks.find(i->first)->second.front().second == ec_impl->get_sub_chunk_count())) {


please clean up the whitespace here (space after if, 80 chars per line), etc.

also use the fancy for loop syntax 'for (auto myvar : thecontainer)' where it helps readability

liewegas · 2017-05-23T21:33:06Z

src/osd/ECMsgTypes.cc

  ::encode(from, bl);
  ::encode(tid, bl);
  ::encode(to_read, bl);
  ::encode(attrs_to_read, bl);
+  ::encode(subchunks,bl);


space between , and bl

liewegas · 2017-05-23T21:33:15Z

src/osd/ECMsgTypes.cc

@@ -210,10 +212,17 @@ void ECSubRead::decode(bufferlist::iterator &bl)
      }
      to_read[m->first] = tlist;
    }
-  } else {
+  } else{


keep space between else and {

liewegas · 2017-05-23T21:33:30Z

src/osd/ECMsgTypes.cc

    ::decode(to_read, bl);
  }
  ::decode(attrs_to_read, bl);
+  if((struct_v > 2) && (struct_v > struct_compat) ){


space after if, inner parens are unnecessary

liewegas · 2017-05-23T21:34:07Z

src/osd/ECMsgTypes.cc

+  ::decode(subchunks, bl);
+  } else {
+    for(set<hobject_t>::iterator i = attrs_to_read.begin(); i != attrs_to_read.end(); ++i) {
+      subchunks[*i].push_back(make_pair(0,1));


the 0,1 means a single subchunk covering the whole thing?

mynaramana · 2017-05-27T13:07:02Z

Hi Sage,

I updated the changes requested in the review.

Thanks,
Myna.

liewegas · 2017-09-01T21:20:29Z

see rebased version at #17428

mynaramana · 2017-09-02T10:46:07Z

Hi Sage, I updated the code in this current PR to fix the crash seen there. I don't have push permissions to the branch of new PR (#17428). Is there a way I can trigger the tests or run them locally? Thanks Myna.

…

On Sat, Sep 2, 2017 at 2:50 AM, Sage Weil ***@***.***> wrote: see rebased version at #17428 <#17428> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15193 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGxnvW2jPrdmqGcXeFaz-nQoKRJYZNAsks5seHUygaJpZM4Nh3ie> .

liewegas · 2017-09-02T18:40:19Z

I'll queue up another test run, thanks!

liewegas · 2017-09-06T20:58:43Z

http://pulpito.ceph.com/sage-2017-09-06_14:17:17-rados-wip-sage-testing-2017-09-05-2251-distro-basic-smithi/

looks ok!

liewegas · 2017-09-06T20:59:46Z

@markhpc can you do an a/b test on this vs master with an ec workload (smallish writes preferably)?

markhpc · 2017-09-20T20:11:21Z

With HDDs this PR worked fine and may have provided a slight performance advantage (1-2%). When testing with NVMe, fio regularly locked up. Eventually I was able to get a partial trace from the fio executable:

home/perf/src/markhpc/ceph/src/common/Mutex.cc: In function 'void Mutex::Lock(bool)' thread 7f51b7fff700 time 2017-09-20 15:53:49.073369
/home/perf/src/markhpc/ceph/src/common/Mutex.cc: 110: FAILED assert(r == 0)
 ceph version 10.0.4-29492-ga72ed6e (a72ed6e642521735b3faeeeccf56835a57e3d43f) mimic (dev)
 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x110) [0x7f51e27e5ee0]
 2: (Mutex::Lock(bool)+0x1a4) [0x7f51e27b86e4]
 3: (()+0x11f550) [0x7f51ec73e550]
 4: /home/ubuntu/src/fio/fio() [0x463c54]
 5: (()+0x11eb06) [0x7f51ec73db06]
 6: (()+0x11fbbf) [0x7f51ec73ebbf]
 7: (()+0x136dbd) [0x7f51ec755dbd]
 8: (()+0x12224d) [0x7f51ec74124d]
 9: (()+0x5cc39) [0x7f51ec67bc39]
 10: (()+0x132b57) [0x7f51ec751b57]
 11: (librados::C_AioComplete::finish(int)+0x22) [0x7f51ec363672]
 12: (Context::complete(int)+0x9) [0x7f51ec341079]
 13: (Finisher::finisher_thread_entry()+0x198) [0x7f51e27e3538]
 14: (()+0x7dc5) [0x7f51eb787dc5]
 15: (clone()+0x6d) [0x7f51eb2b273d]
 NOTE: a copy of the executable, or `objdump -rdS ` is needed to interpret this.

I will attempt to reproduce with master as it may not be the fault of this PR.

Edit: I can confirm that the above issue is happening with master, so not this PR's fault. This will block NVMe testing until we fix though.

mynaramana · 2017-09-24T11:56:08Z

Hi Mark, Is the NVMe issue fixed in master now ?

with new decode, minimum_to_decode in ErasureCodeInterface. Updated ECBackend, ECUtil to use the new functions.Fixed the test cases to use the new functions. Fixed the review comments. Authors: Myna, Elita. Signed-off-by: Myna Vajha <mynaramana@gmail.com>

…efore it asserts for non-zeros size. Authors: Myna, Elita. Signed-off-by: Myna Vajha <mynaramana@gmail.com>

tchaikov · 2017-10-30T12:38:32Z

src/osd/ECUtil.cc

@@ -71,28 +68,57 @@ int ECUtil::decode(
    need.insert(i->first);
  }

-  for (uint64_t i = 0; i < total_data_size; i += sinfo.get_chunk_size()) {
+  set<int> avail;
+  for (auto i = to_decode.begin();


nit, use range-based for loop if you please.

@mynaramana could you address this comment also?

tchaikov · 2017-10-30T12:38:59Z

src/osd/ECUtil.cc

+  int repair_data_per_chunk;
+  int subchunk_size = sinfo.get_chunk_size()/ec_impl->get_sub_chunk_count();
+
+  for(auto i=to_decode.begin();


add a space after for.

tchaikov · 2017-10-30T12:39:07Z

src/osd/ECUtil.cc

+  for(auto i=to_decode.begin();
+      i != to_decode.end(); 
+      ++i) {
+    if(min.find(i->first) == min.end()) continue;


add a space after if.

tchaikov · 2017-10-30T12:39:28Z

src/osd/ECUtil.cc

+        repair_subchunk_count += j->second;
+      }
+      repair_data_per_chunk = repair_subchunk_count*subchunk_size;
+      chunks_count = (int) i->second.length() / repair_data_per_chunk;


remove the space after (int).

tchaikov · 2017-10-30T12:40:57Z

src/osd/ECUtil.cc

+            j != min[i->first].end(); ++j) {
+        repair_subchunk_count += j->second;
+      }
+      repair_data_per_chunk = repair_subchunk_count*subchunk_size;


add spaces around *.

mynaramana · 2017-10-31T02:53:20Z

Hi Kefu, I updated the code according to the review comments.
Thanks
Myna.

tchaikov · 2017-10-31T03:30:21Z

src/osd/ECUtil.cc

@@ -71,28 +68,57 @@ int ECUtil::decode(
    need.insert(i->first);
  }

-  for (uint64_t i = 0; i < total_data_size; i += sinfo.get_chunk_size()) {
+  set<int> avail;
+  for (auto i = to_decode.begin();


@mynaramana could you address this comment also?

tchaikov · 2017-10-31T03:32:29Z

src/osd/ECMsgTypes.cc

@@ -214,6 +216,13 @@ void ECSubRead::decode(bufferlist::iterator &bl)
    ::decode(to_read, bl);
  }
  ::decode(attrs_to_read, bl);
+  if (struct_v > 2 && struct_v > struct_compat) {
+  ::decode(subchunks, bl);


wrong indent.

tchaikov · 2017-10-31T03:34:13Z

src/osd/ECMsgTypes.cc

+  if (struct_v > 2 && struct_v > struct_compat) {
+  ::decode(subchunks, bl);
+  } else {
+    for (auto &&i:attrs_to_read) {


add spaces around :.

tchaikov · 2017-10-31T03:34:20Z

src/osd/ECMsgTypes.cc

+  ::decode(subchunks, bl);
+  } else {
+    for (auto &&i:attrs_to_read) {
+      subchunks[i].push_back(make_pair(0,1));


add a space before ,.

tchaikov · 2017-10-31T03:35:10Z

src/osd/ECUtil.cc

+    if (min.find(i.first) == min.end()) continue;
+    else {
+      int repair_subchunk_count = 0;
+      for (auto j = min[i.first].begin();


could use range-based loop here also.

tchaikov · 2017-10-31T03:35:44Z

src/osd/ECUtil.cc

-       i != out.end();
-       ++i) {
-    assert(i->second->length() == total_data_size);
+  for (auto i = out.begin(); i != out.end(); ++i) {


could you switch to range-based for loop, since you are at here.

tchaikov · 2017-10-31T03:41:55Z

src/osd/ECUtil.cc

+  }
+
+  map<int, vector<pair<int, int>>> min;
+  assert(ec_impl->minimum_to_decode(need, avail, &min) == 0);


@mynaramana we should not rely on the side effect in assert(), in future, assert() in Ceph could be optimized out if NDEBUG is defined.

tchaikov · 2017-10-31T03:45:45Z

src/erasure-code/ErasureCode.cc

+  }
+  vector<pair<int, int>> default_subchunks;
+  default_subchunks.push_back(make_pair(0, get_sub_chunk_count()));
+  for(auto &&id:minimum_shard_ids){


add spaces around :

tchaikov · 2017-10-31T03:52:41Z

src/osd/ECUtil.cc

+  int subchunk_size = sinfo.get_chunk_size()/ec_impl->get_sub_chunk_count();
+
+  for (auto &&i : to_decode) {
+    if (min.find(i.first) == min.end()) continue;


nit, this is an anti-pattern:

find and throw away the returned iterator,

use operator[] to locate the element to find again.

also, j deserves a better name. could you change it to something like:

auto found = min.find(i.first); if (found != min.end()) { int repair_subchunk_count = 0; for (auto& subchunks : found) { repair_subchunk_count += subchunks.second; } break; }

tchaikov · 2017-10-31T04:07:07Z

@mynaramana the rados run looks good. most of my comments are just formatting and code style related issues , but the assert() one is a more important one.

tchaikov · 2017-10-31T10:48:25Z

src/osd/ECUtil.cc

  }

  map<int, vector<pair<int, int>>> min;
-  assert(ec_impl->minimum_to_decode(need, avail, &min) == 0);
+  int r = ec_impl->minimum_to_decode(need, avail, &min) == 0;


r is a boolean casted to int, and it should always be 1.

updated this!

tchaikov · 2017-10-31T10:58:44Z

@mynaramana looks great! could you squash the changes addressing review comments into the related commits respectively?

…data than needed. Made all the helper information uniform across all helper nodes. Authors: Myna, Elita. Signed-off-by: Myna Vajha <mynaramana@gmail.com>

tchaikov · 2017-11-01T14:49:29Z

http://pulpito.ceph.com/kchai-2017-11-01_08:15:38-rados-wip-kefu-testing-2017-11-01-1408-distro-basic-smithi/

tchaikov · 2017-11-01T14:50:42Z

the failed test is unrelated, i will take a look at it tmr.

mynaramana mentioned this pull request May 22, 2017

Coupled Layer MSR codes (Optimal Repair Network, Disk Traffic). #14300

Closed

joscollin added feature needs-qa labels May 22, 2017

joscollin changed the title ~~Array Codes (Fractional read during repair)~~ osd: Array Codes (Fractional read during repair) May 22, 2017

liewegas added core and removed needs-qa labels May 23, 2017

liewegas changed the title ~~osd: Array Codes (Fractional read during repair)~~ osd: introduce sub-chunks to erasure code plugin interface May 23, 2017

liewegas requested changes May 23, 2017

View reviewed changes

mynaramana force-pushed the arraycode branch 2 times, most recently from aa321cf to 8726993 Compare May 25, 2017 06:19

mynaramana force-pushed the arraycode branch 3 times, most recently from 649ae79 to 7c96329 Compare September 2, 2017 10:02

liewegas added the wip-sage-testing label Sep 2, 2017

liewegas added needs-review and removed wip-sage-testing labels Sep 6, 2017

liewegas requested a review from jdurgin September 6, 2017 20:59

markhpc self-assigned this Sep 18, 2017

liewegas mentioned this pull request Oct 24, 2017

osd: update erasure code plugin interface to support subchunks #18521

Closed

mynaramana added 2 commits October 30, 2017 07:57

Added sanity checks in ECUtil::decode for input repair buffer sizes b…

1ab883b

…efore it asserts for non-zeros size. Authors: Myna, Elita. Signed-off-by: Myna Vajha <mynaramana@gmail.com>

mynaramana force-pushed the arraycode branch from 7c96329 to 1ab883b Compare October 30, 2017 02:30

tchaikov added needs-qa wip-kefu-testing labels Oct 30, 2017

liewegas approved these changes Oct 30, 2017

View reviewed changes

tchaikov reviewed Oct 30, 2017

View reviewed changes

mynaramana force-pushed the arraycode branch from a6ea055 to 792438f Compare October 30, 2017 15:58

tchaikov requested changes Oct 31, 2017

View reviewed changes

tchaikov removed the wip-kefu-testing label Oct 31, 2017

tchaikov requested changes Oct 31, 2017

View reviewed changes

mynaramana force-pushed the arraycode branch from d2d8c7d to 85dc92e Compare October 31, 2017 10:52

Changed ECUtil::decode to handle the case where there is more helper …

23e8882

…data than needed. Made all the helper information uniform across all helper nodes. Authors: Myna, Elita. Signed-off-by: Myna Vajha <mynaramana@gmail.com>

mynaramana force-pushed the arraycode branch from 85dc92e to 23e8882 Compare October 31, 2017 11:07

tchaikov approved these changes Oct 31, 2017

View reviewed changes

tchaikov added the wip-kefu-testing label Nov 1, 2017

tchaikov merged commit 01a65f0 into ceph:master Nov 1, 2017

dzafman mentioned this pull request May 8, 2018

osd/ECBackend: only check required shards when finishing recovery reads #21873

Closed

		default_subchunks.push_back(make_pair(0,get_sub_chunk_count()));


		for(set<int>::iterator i=minimum_shard_ids.begin();


		if ( r != 0) return r;

		list<pair<int, int>> default_subchunks;

osd: introduce sub-chunks to erasure code plugin interface #15193

osd: introduce sub-chunks to erasure code plugin interface #15193

Conversation

mynaramana commented May 22, 2017 • edited by joscollin

liewegas commented May 22, 2017

liewegas commented May 23, 2017

liewegas commented May 23, 2017

liewegas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mynaramana commented May 27, 2017

liewegas commented Sep 1, 2017

mynaramana commented Sep 2, 2017 via email

liewegas commented Sep 2, 2017

liewegas commented Sep 6, 2017

liewegas commented Sep 6, 2017

markhpc commented Sep 20, 2017 • edited by liewegas

mynaramana commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mynaramana commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Oct 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Oct 31, 2017

tchaikov commented Nov 1, 2017

tchaikov commented Nov 1, 2017

mynaramana commented May 22, 2017 •

edited by joscollin

markhpc commented Sep 20, 2017 •

edited by liewegas

tchaikov commented Oct 31, 2017 •

edited