You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/10/15 01:55:21 UTC

[kudu-CR] KUDU-1508: script for testing presence of bug and finding upper bounds 

Hello Todd Lipcon,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/4730

to review the following change.

Change subject: KUDU-1508: script for testing presence of bug and finding upper bounds
......................................................................

KUDU-1508: script for testing presence of bug and finding upper bounds

This patch introduces a simple script that tests for the presence of
KUDU-1508.

When run on my laptop (Ubuntu 16.04):

  This kernel is not vulnerable to KUDU-1508, skipping test

When run on ve0518 (an el6.6 box):

  This kernel is vulnerable to KUDU-1508, finding per-block size upper bounds
  Block size 1024: searching for block number upper bound (MIN=0,MAX=16384)
  Block size 1024: 8192 bad (MIN=0,MAX=16384)
  Block size 1024: 4095 bad (MIN=0,MAX=8191)
  Block size 1024: 2047 bad (MIN=0,MAX=4094)
  Block size 1024: 1023 bad (MIN=0,MAX=2046)
  Block size 1024: 511 good (MIN=0,MAX=1022)
  Block size 1024: 767 bad (MIN=512,MAX=1022)
  Block size 1024: 639 good (MIN=512,MAX=766)
  Block size 1024: 703 bad (MIN=640,MAX=766)
  Block size 1024: 671 good (MIN=640,MAX=702)
  Block size 1024: 687 bad (MIN=672,MAX=702)
  Block size 1024: 679 bad (MIN=672,MAX=686)
  Block size 1024: 675 bad (MIN=672,MAX=678)
  Block size 1024: 673 good (MIN=672,MAX=674)
  Block size 1024: upper bound found at 673
  Block size 2048: searching for block number upper bound (MIN=0,MAX=16384)
  Block size 2048: 8192 bad (MIN=0,MAX=16384)
  Block size 2048: 4095 bad (MIN=0,MAX=8191)
  Block size 2048: 2047 bad (MIN=0,MAX=4094)
  Block size 2048: 1023 good (MIN=0,MAX=2046)
  Block size 2048: 1535 bad (MIN=1024,MAX=2046)
  Block size 2048: 1279 good (MIN=1024,MAX=1534)
  Block size 2048: 1407 bad (MIN=1280,MAX=1534)
  Block size 2048: 1343 good (MIN=1280,MAX=1406)
  Block size 2048: 1375 bad (MIN=1344,MAX=1406)
  Block size 2048: 1359 bad (MIN=1344,MAX=1374)
  Block size 2048: 1351 good (MIN=1344,MAX=1358)
  Block size 2048: 1355 bad (MIN=1352,MAX=1358)
  Block size 2048: 1353 good (MIN=1352,MAX=1354)
  Block size 2048: upper bound found at 1353
  Block size 4096: searching for block number upper bound (MIN=0,MAX=16384)
  Block size 4096: 8192 bad (MIN=0,MAX=16384)
  Block size 4096: 4095 bad (MIN=0,MAX=8191)
  Block size 4096: 2047 good (MIN=0,MAX=4094)
  Block size 4096: 3071 bad (MIN=2048,MAX=4094)
  Block size 4096: 2559 good (MIN=2048,MAX=3070)
  Block size 4096: 2815 bad (MIN=2560,MAX=3070)
  Block size 4096: 2687 good (MIN=2560,MAX=2814)
  Block size 4096: 2751 bad (MIN=2688,MAX=2814)
  Block size 4096: 2719 good (MIN=2688,MAX=2750)
  Block size 4096: 2735 bad (MIN=2720,MAX=2750)
  Block size 4096: 2727 bad (MIN=2720,MAX=2734)
  Block size 4096: 2723 bad (MIN=2720,MAX=2726)
  Block size 4096: 2721 good (MIN=2720,MAX=2722)
  Block size 4096: upper bound found at 2721

Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
---
A src/kudu/experiments/KUDU-1508/hole_punch_range.c
A src/kudu/experiments/KUDU-1508/run_test.sh
2 files changed, 227 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/4730/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4730
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1508: script for testing presence of bug and finding upper bounds

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-1508: script for testing presence of bug and finding upper bounds
......................................................................


Patch Set 1:

(5 comments)

> can you write in the commit message or in the test header something
 > about what the finding here means? if I'm interpreting it
 > correctly, we're trying to find the max number of extents in the
 > tree before it becomes multi-level, and that's dependent on the
 > file system's block size?

Yes, I'll note that in the commit description.

http://gerrit.cloudera.org:8080/#/c/4730/1/src/kudu/experiments/KUDU-1508/hole_punch_range.c
File src/kudu/experiments/KUDU-1508/hole_punch_range.c:

Line 32:     fprintf(stderr, "usage: %s <path> <start block> <end block> <stride>\n", argv[0]);
> clarify whether the file should exist or not, and how large it should be?
Clarified the existence and the arguments.

Size doesn't matter; fallocate() will no-op if you punch a hole beyond the size of the file.


Line 40:   int fd = open(argv[1], O_EXCL | O_WRONLY, 0644);
> O_EXCL without O_CREAT is undefined
Whoops, dropped O_EXCL.


Line 54:   for (block_num = start_block; block_num < end_block; block_num += stride) {
> any reason not to use C99 here?
Not a huge deal, but I didn't want to have to pass -std=c99 with older compilers (i.e. gcc 4.4 on el6). Given that this is the only C99ism, it seemed like a reasonable trade-off.


http://gerrit.cloudera.org:8080/#/c/4730/1/src/kudu/experiments/KUDU-1508/run_test.sh
File src/kudu/experiments/KUDU-1508/run_test.sh:

PS1, Line 70: 1
> instead of '1' how about using a string like 'fail_ok' so it's clearer
Sure, will use "fail_ok".

I couldn't get "... || :" to work with the run() method, though maybe there's some quoting that would make it work.


PS1, Line 77: -l 
> can you use the longer form (--length) here and elsewhere for flags that ar
Sure, though I think this is the only place I could use a longopt.


-- 
To view, visit http://gerrit.cloudera.org:8080/4730
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1508: script for testing presence of bug and finding upper bounds

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4730

to look at the new patch set (#2).

Change subject: KUDU-1508: script for testing presence of bug and finding upper bounds
......................................................................

KUDU-1508: script for testing presence of bug and finding upper bounds

This patch introduces a script that tests for the presence of KUDU-1508 and
establishes a "safe" upper bound on the number of extents an ext4 file can
have before it becomes vulnerable to the bug. The upper bound is actually a
function of the filesystem's block size, since that's the granularity on
which hole punching operates.

When run on my laptop (Ubuntu 16.04):

  This kernel is not vulnerable to KUDU-1508, skipping test

When run on ve0518 (an el6.6 box):

  This kernel is vulnerable to KUDU-1508, finding per-block size upper bounds
  Block size 1024: searching for block number upper bound (MIN=0,MAX=16384)
  Block size 1024: 8192 bad (MIN=0,MAX=16384)
  Block size 1024: 4095 bad (MIN=0,MAX=8191)
  Block size 1024: 2047 bad (MIN=0,MAX=4094)
  Block size 1024: 1023 bad (MIN=0,MAX=2046)
  Block size 1024: 511 good (MIN=0,MAX=1022)
  Block size 1024: 767 bad (MIN=512,MAX=1022)
  Block size 1024: 639 good (MIN=512,MAX=766)
  Block size 1024: 703 bad (MIN=640,MAX=766)
  Block size 1024: 671 good (MIN=640,MAX=702)
  Block size 1024: 687 bad (MIN=672,MAX=702)
  Block size 1024: 679 bad (MIN=672,MAX=686)
  Block size 1024: 675 bad (MIN=672,MAX=678)
  Block size 1024: 673 good (MIN=672,MAX=674)
  Block size 1024: upper bound found at 673
  Block size 2048: searching for block number upper bound (MIN=0,MAX=16384)
  Block size 2048: 8192 bad (MIN=0,MAX=16384)
  Block size 2048: 4095 bad (MIN=0,MAX=8191)
  Block size 2048: 2047 bad (MIN=0,MAX=4094)
  Block size 2048: 1023 good (MIN=0,MAX=2046)
  Block size 2048: 1535 bad (MIN=1024,MAX=2046)
  Block size 2048: 1279 good (MIN=1024,MAX=1534)
  Block size 2048: 1407 bad (MIN=1280,MAX=1534)
  Block size 2048: 1343 good (MIN=1280,MAX=1406)
  Block size 2048: 1375 bad (MIN=1344,MAX=1406)
  Block size 2048: 1359 bad (MIN=1344,MAX=1374)
  Block size 2048: 1351 good (MIN=1344,MAX=1358)
  Block size 2048: 1355 bad (MIN=1352,MAX=1358)
  Block size 2048: 1353 good (MIN=1352,MAX=1354)
  Block size 2048: upper bound found at 1353
  Block size 4096: searching for block number upper bound (MIN=0,MAX=16384)
  Block size 4096: 8192 bad (MIN=0,MAX=16384)
  Block size 4096: 4095 bad (MIN=0,MAX=8191)
  Block size 4096: 2047 good (MIN=0,MAX=4094)
  Block size 4096: 3071 bad (MIN=2048,MAX=4094)
  Block size 4096: 2559 good (MIN=2048,MAX=3070)
  Block size 4096: 2815 bad (MIN=2560,MAX=3070)
  Block size 4096: 2687 good (MIN=2560,MAX=2814)
  Block size 4096: 2751 bad (MIN=2688,MAX=2814)
  Block size 4096: 2719 good (MIN=2688,MAX=2750)
  Block size 4096: 2735 bad (MIN=2720,MAX=2750)
  Block size 4096: 2727 bad (MIN=2720,MAX=2734)
  Block size 4096: 2723 bad (MIN=2720,MAX=2726)
  Block size 4096: 2721 good (MIN=2720,MAX=2722)
  Block size 4096: upper bound found at 2721

Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
---
A src/kudu/experiments/KUDU-1508/hole_punch_range.c
A src/kudu/experiments/KUDU-1508/run_test.sh
2 files changed, 234 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/4730/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4730
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1508: script for testing presence of bug and finding upper bounds

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.

Change subject: KUDU-1508: script for testing presence of bug and finding upper bounds
......................................................................


KUDU-1508: script for testing presence of bug and finding upper bounds

This patch introduces a script that tests for the presence of KUDU-1508 and
establishes a "safe" upper bound on the number of extents an ext4 file can
have before it becomes vulnerable to the bug. The upper bound is actually a
function of the filesystem's block size, since that's the granularity on
which hole punching operates.

When run on my laptop (Ubuntu 16.04):

  This kernel is not vulnerable to KUDU-1508, skipping test

When run on ve0518 (an el6.6 box):

  This kernel is vulnerable to KUDU-1508, finding per-block size upper bounds
  Block size 1024: searching for block number upper bound (MIN=0,MAX=16384)
  Block size 1024: 8192 bad (MIN=0,MAX=16384)
  Block size 1024: 4095 bad (MIN=0,MAX=8191)
  Block size 1024: 2047 bad (MIN=0,MAX=4094)
  Block size 1024: 1023 bad (MIN=0,MAX=2046)
  Block size 1024: 511 good (MIN=0,MAX=1022)
  Block size 1024: 767 bad (MIN=512,MAX=1022)
  Block size 1024: 639 good (MIN=512,MAX=766)
  Block size 1024: 703 bad (MIN=640,MAX=766)
  Block size 1024: 671 good (MIN=640,MAX=702)
  Block size 1024: 687 bad (MIN=672,MAX=702)
  Block size 1024: 679 bad (MIN=672,MAX=686)
  Block size 1024: 675 bad (MIN=672,MAX=678)
  Block size 1024: 673 good (MIN=672,MAX=674)
  Block size 1024: upper bound found at 673
  Block size 2048: searching for block number upper bound (MIN=0,MAX=16384)
  Block size 2048: 8192 bad (MIN=0,MAX=16384)
  Block size 2048: 4095 bad (MIN=0,MAX=8191)
  Block size 2048: 2047 bad (MIN=0,MAX=4094)
  Block size 2048: 1023 good (MIN=0,MAX=2046)
  Block size 2048: 1535 bad (MIN=1024,MAX=2046)
  Block size 2048: 1279 good (MIN=1024,MAX=1534)
  Block size 2048: 1407 bad (MIN=1280,MAX=1534)
  Block size 2048: 1343 good (MIN=1280,MAX=1406)
  Block size 2048: 1375 bad (MIN=1344,MAX=1406)
  Block size 2048: 1359 bad (MIN=1344,MAX=1374)
  Block size 2048: 1351 good (MIN=1344,MAX=1358)
  Block size 2048: 1355 bad (MIN=1352,MAX=1358)
  Block size 2048: 1353 good (MIN=1352,MAX=1354)
  Block size 2048: upper bound found at 1353
  Block size 4096: searching for block number upper bound (MIN=0,MAX=16384)
  Block size 4096: 8192 bad (MIN=0,MAX=16384)
  Block size 4096: 4095 bad (MIN=0,MAX=8191)
  Block size 4096: 2047 good (MIN=0,MAX=4094)
  Block size 4096: 3071 bad (MIN=2048,MAX=4094)
  Block size 4096: 2559 good (MIN=2048,MAX=3070)
  Block size 4096: 2815 bad (MIN=2560,MAX=3070)
  Block size 4096: 2687 good (MIN=2560,MAX=2814)
  Block size 4096: 2751 bad (MIN=2688,MAX=2814)
  Block size 4096: 2719 good (MIN=2688,MAX=2750)
  Block size 4096: 2735 bad (MIN=2720,MAX=2750)
  Block size 4096: 2727 bad (MIN=2720,MAX=2734)
  Block size 4096: 2723 bad (MIN=2720,MAX=2726)
  Block size 4096: 2721 good (MIN=2720,MAX=2722)
  Block size 4096: upper bound found at 2721

Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
Reviewed-on: http://gerrit.cloudera.org:8080/4730
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
A src/kudu/experiments/KUDU-1508/hole_punch_range.c
A src/kudu/experiments/KUDU-1508/run_test.sh
2 files changed, 234 insertions(+), 0 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4730
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1508: script for testing presence of bug and finding upper bounds

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1508: script for testing presence of bug and finding upper bounds
......................................................................


Patch Set 2: Code-Review+2

Seems fine. I'll admit I didn't review it that carefully, but it's just an experiment, so who cares :)

(btw in the future would prefer this kind of thing in python vs shell!)

-- 
To view, visit http://gerrit.cloudera.org:8080/4730
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1508: script for testing presence of bug and finding upper bounds

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1508: script for testing presence of bug and finding upper bounds
......................................................................


Patch Set 2:

I suppose, except that the shell regexes make my eyes bleed. Plus the fallocate helper script probably could be written in python, too. Anyway water under the bridge, it's just an experiment

-- 
To view, visit http://gerrit.cloudera.org:8080/4730
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1508: script for testing presence of bug and finding upper bounds

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-1508: script for testing presence of bug and finding upper bounds
......................................................................


Patch Set 2:

> (btw in the future would prefer this kind of thing in python vs
 > shell!)

Really? I think the amount of "invoke subprocesses" outweighs the amount of "do stuff with the results"; doesn't that balance favor shell?

-- 
To view, visit http://gerrit.cloudera.org:8080/4730
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1508: script for testing presence of bug and finding upper bounds

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1508: script for testing presence of bug and finding upper bounds
......................................................................


Patch Set 1:

(5 comments)

can you write in the commit message or in the test header something about what the finding here means? if I'm interpreting it correctly, we're trying to find the max number of extents in the tree before it becomes multi-level, and that's dependent on the file system's block size?

http://gerrit.cloudera.org:8080/#/c/4730/1/src/kudu/experiments/KUDU-1508/hole_punch_range.c
File src/kudu/experiments/KUDU-1508/hole_punch_range.c:

Line 32:     fprintf(stderr, "usage: %s <path> <start block> <end block> <stride>\n", argv[0]);
clarify whether the file should exist or not, and how large it should be?


Line 40:   int fd = open(argv[1], O_EXCL | O_WRONLY, 0644);
O_EXCL without O_CREAT is undefined


Line 54:   for (block_num = start_block; block_num < end_block; block_num += stride) {
any reason not to use C99 here?


http://gerrit.cloudera.org:8080/#/c/4730/1/src/kudu/experiments/KUDU-1508/run_test.sh
File src/kudu/experiments/KUDU-1508/run_test.sh:

PS1, Line 70: 1
instead of '1' how about using a string like 'fail_ok' so it's clearer

alternatively what about just using something like 'sudo umount || :'


PS1, Line 77: -l 
can you use the longer form (--length) here and elsewhere for flags that aren't well-known (eg I think mkfs -t or mount -o are commonly used)


-- 
To view, visit http://gerrit.cloudera.org:8080/4730
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I710918a153a9e8e05e989fe63281891c9ebc7178
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes