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 2017/04/07 03:17:05 UTC
[kudu-CR] fs: ensure FS IOC FIEMAP can be used on LBM systems
Hello David Ribeiro Alves, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6584
to review the following change.
Change subject: fs: ensure FS_IOC_FIEMAP can be used on LBM systems
......................................................................
fs: ensure FS_IOC_FIEMAP can be used on LBM systems
This patch generalizes the existing "hole punch test" to include a check
that FS_IOC_FIEMAP works.
I've tested it with ext4 on el6.6 and Ubuntu 16.04. Notably, it doesn't work
with ecryptfs, though neither does hole punching.
Change-Id: Ie027a264473ae5f905f8ad50aa45e810e046a135
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/log_block_manager.cc
3 files changed, 26 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/6584/1
--
To view, visit http://gerrit.cloudera.org:8080/6584
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie027a264473ae5f905f8ad50aa45e810e046a135
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] fs: ensure FS IOC FIEMAP can be used on LBM systems
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: fs: ensure FS_IOC_FIEMAP can be used on LBM systems
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
feel free to ignore the nit or address it
http://gerrit.cloudera.org:8080/#/c/6584/1//COMMIT_MSG
Commit Message:
PS1, Line 13: ecryptfs
nit: eCryptfs
--
To view, visit http://gerrit.cloudera.org:8080/6584
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie027a264473ae5f905f8ad50aa45e810e046a135
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] fs: ensure FS IOC FIEMAP can be used on LBM systems
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: fs: ensure FS_IOC_FIEMAP can be used on LBM systems
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6584/1/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS1, Line 100: Status CheckFsFeatures(Env* env, const string& path) {
: // Arbitrary constants.
: static uint64_t kFileSize = 4096 * 4;
: static uint64_t kHoleOffset = 4096;
: static uint64_t kHoleSize = 8192;
: static uint64_t kPunchedFileSize = kFileSize - kHoleSize;
:
: // Open the test file.
: string filename = JoinPathSegments(path, "hole_punch_test_file");
: unique_ptr<RWFile> file;
: RWFileOptions opts;
: RETURN_NOT_OK(env->NewRWFile(opts, filename, &file));
:
: // The file has been created; delete it on exit no matter what happens.
: ScopedFileDeleter file_deleter(env, filename);
:
: // Preallocate it, making sure the file's size is what we'd expect.
: uint64_t sz;
: RETURN_NOT_OK_PREPEND(file->PreAllocate(
: 0, kFileSize, RWFile::CHANGE_FILE_SIZE), "could not preallocate file");
: RETURN_NOT_OK(env->GetFileSizeOnDisk(filename, &sz));
: if (sz != kFileSize) {
: return Status::IOError(Substitute(
: "unexpected pre-punch file size for $0: expected $1 but got $2",
: filename, kFileSize, sz));
: }
:
: // Punch the hole, testing the file's size again.
: RETURN_NOT_OK_PREPEND(file->PunchHole(kHoleOffset, kHoleSize),
: "could not punch hole in file");
: RETURN_NOT_OK(env->GetFileSizeOnDisk(filename, &sz));
: if (sz != kPunchedFileSize) {
: return Status::IOError(Substitute(
: "unexpected post-punch file size for $0: expected $1 but got $2",
: filename, kPunchedFileSize, sz));
: }
:
: // Make sure we can get at the file's extents.
: RWFile::ExtentMap extents;
: RETURN_NOT_OK_PREPEND(file->GetExtentMap(&extents),
: "could not get file's extent map");
: return Status::OK();
: }
are there cases where people might have data already written to filesystems that don't support at least one of these things and which will become unreadable from now on?
--
To view, visit http://gerrit.cloudera.org:8080/6584
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie027a264473ae5f905f8ad50aa45e810e046a135
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] fs: ensure FS IOC FIEMAP can be used on LBM systems
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has abandoned this change.
Change subject: fs: ensure FS_IOC_FIEMAP can be used on LBM systems
......................................................................
Abandoned
It turns out that aufs does not support FS_IOC_FIEMAP. That's why all of the precommit tests failed: aufs is used by Docker containers.
Rather than force Docker users to work around this, I'll make the use of FS_IOC_FIEMAP in the LBM more opportunistic.
--
To view, visit http://gerrit.cloudera.org:8080/6584
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ie027a264473ae5f905f8ad50aa45e810e046a135
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] fs: ensure FS IOC FIEMAP can be used on LBM systems
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: fs: ensure FS_IOC_FIEMAP can be used on LBM systems
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6584/1//COMMIT_MSG
Commit Message:
PS1, Line 13: ecryptfs
> nit: eCryptfs
Done
http://gerrit.cloudera.org:8080/#/c/6584/1/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS1, Line 100: Status CheckFsFeatures(Env* env, const string& path) {
: // Arbitrary constants.
: static uint64_t kFileSize = 4096 * 4;
: static uint64_t kHoleOffset = 4096;
: static uint64_t kHoleSize = 8192;
: static uint64_t kPunchedFileSize = kFileSize - kHoleSize;
:
: // Open the test file.
: string filename = JoinPathSegments(path, "hole_punch_test_file");
: unique_ptr<RWFile> file;
: RWFileOptions opts;
: RETURN_NOT_OK(env->NewRWFile(opts, filename, &file));
:
: // The file has been created; delete it on exit no matter what happens.
: ScopedFileDeleter file_deleter(env, filename);
:
: // Preallocate it, making sure the file's size is what we'd expect.
: uint64_t sz;
: RETURN_NOT_OK_PREPEND(file->PreAllocate(
: 0, kFileSize, RWFile::CHANGE_FILE_SIZE), "could not preallocate file");
: RETURN_NOT_OK(env->GetFileSizeOnDisk(filename, &sz));
: if (sz != kFileSize) {
: return Status::IOError(Substitute(
: "unexpected pre-punch file size for $0: expected $1 but got $2",
: filename, kFileSize, sz));
: }
:
: // Punch the hole, testing the file's size again.
: RETURN_NOT_OK_PREPEND(file->PunchHole(kHoleOffset, kHoleSize),
: "could not punch hole in file");
: RETURN_NOT_OK(env->GetFileSizeOnDisk(filename, &sz));
: if (sz != kPunchedFileSize) {
: return Status::IOError(Substitute(
: "unexpected post-punch file size for $0: expected $1 but got $2",
: filename, kPunchedFileSize, sz));
: }
:
: // Make sure we can get at the file's extents.
: RWFile::ExtentMap extents;
: RETURN_NOT_OK_PREPEND(file->GetExtentMap(&extents),
: "could not get file's extent map");
: return Status::OK();
: }
> are there cases where people might have data already written to filesystems
That's a good question, and I don't fully know the answer. When I wrote the patch, I tested ext4 on el6.6 and on Ubuntu 16.04. Both supported hole punching as well as FS_IOC_FIEMAP.
Since you asked, I also tested ext4 and xfs on Ubuntu 14.04, SLES 12 and el6.4. Same result.
That's a pretty comprehensive list. What do you think?
http://gerrit.cloudera.org:8080/#/c/6584/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
Line 1306: Status LogBlockManager::Open(FsReport* report) {
> warning: default arguments on virtual or override methods are prohibited [g
Done
--
To view, visit http://gerrit.cloudera.org:8080/6584
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie027a264473ae5f905f8ad50aa45e810e046a135
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes