Adar Dembo has posted comments on this change. Change subject: env: add RWFile::GetExtentMap for analyzing file extents ......................................................................
Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/6583/1/src/kudu/util/env-test.cc File src/kudu/util/env-test.cc: PS1, Line 896: LOG(INFO) > how big can this get? maybe VLOG? I only get one extent on my laptop. I don't expect it to be big, and I also don't mind logging in tests if it helps debugging. PS1, Line 908: uint64_t found_offset = 0; : for (const auto& e : extents) { : if (e.second >= (fs_block_size * 3)) { : found_offset = e.first + fs_block_size; : break; : } : } : if (found_offset == 0) { : LOG(INFO) << "Couldn't find extent to split, skipping this part of the test"; : return; : } > how likely is this to happen? are we sure we won't be skipping this test mo It's virtually impossible. One of the following would have to be true (assuming a 4096 fs block size): 1. In response to our 1k-at-a-time writes, the fs stupidly allocated one extent (sized to one block) every 4th write. Definitely won't happen with an ext4 that supports delayed allocation. 2. The filesystem was so fragmented that it couldn't even do a single 12k allocation. But, I didn't want to assume anything about the test environment, so the code is forgiving. On second thought, I'll eliminate case #1 by growing the file by preallocation instead of 1k-at-a-time writes. PS1, Line 934: LOG(INFO) > maybe VLOG? Like I wrote above, I don't mind logging tests, and it can be helpful. http://gerrit.cloudera.org:8080/#/c/6583/1/src/kudu/util/env.h File src/kudu/util/env.h: PS1, Line 587: // If the underlying filesystem does not support extents, map entries : // represent runs of adjacent fixed-size filesystem blocks instead. > well in the apple case this does nothing, right? maybe elaborate a bit more Sorry, this is actually intended to cover ext3. I'll add another note for other platforms. PS1, Line 589: typedef std::map<uint64_t, uint64_t> ExtentMap; > I like this way of typedeffing method arguments, maybe we should add this t I'm not actually sure that it's the best approach for C++11. I think we should be doing: using ExtentMap = std::map<uint64_t, uint64_t>; But I didn't want to rock the boat too much. http://gerrit.cloudera.org:8080/#/c/6583/1/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: PS1, Line 732: TRACE_EVENT1("io", "PosixRWFile::GetExtentMap", "path", filename_); > why is this traceable? do we get extent maps online? It's traceable because I believe the convention for all env_posix.cc functions is to trace if it involves any kind of IO (i.e. if it can block). Line 740: struct fiemap* fm = (struct fiemap*)buf.get(); > warning: C-style casts are discouraged; use reinterpret_cast [google-readab Done PS1, Line 764: LOG(WARNING) << Substitute( : "Unexpected extent found at offset $0", fme[i].fe_logical); > log fatal/log error? is the data returned by this method still useful if th I wasn't sure whether to trust that the kernel isn't buggy and crash if something unexpected happened, or whether to be more forgiving. But you're right that it's tough to reason about what to do with the data if the kernel gets it wrong. I'll change this to be more strict. PS1, Line 767: bool inserted = InsertIfNotPresent(&extents, : fme[i].fe_logical, : fme[i].fe_length); > we can get repeated extents? if not InsertOrDie? Done PS1, Line 771: // Two extents at the same logical offset? Shouldn't be possible. : LOG(WARNING) << Substitute( : "Multiple extents found at offset $0. Ignoring", : fme[i].fe_logical); > same comment as the case above Done PS1, Line 776: \n > why the \n? My mistake. -- To view, visit http://gerrit.cloudera.org:8080/6583 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I35bd1bdb9e1a839af2ab95ea73b79217c1f4a2b3 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
