Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use butil::IOBuf in WriteChunk #81

Merged
merged 2 commits into from
Sep 23, 2020
Merged

use butil::IOBuf in WriteChunk #81

merged 2 commits into from
Sep 23, 2020

Conversation

wu-hanqing
Copy link
Contributor

Change-Id: If514d166a27da3066649820a140f28ec193b41a1

What problem does this PR solve?

Issue Number: None

Problem Summary:

What is changed and how it works?

What's Changed: zeroize memory copy in chunkserver writechunk

How it Works:

Side effects(Breaking backward compatibility? Performance regression?):

Check List

  • Relevant documentation/comments is changed or added
  • I acknowledge that all my contributions will be made under the project's license

@wu-hanqing wu-hanqing force-pushed the chunkserver-zeroize-copy-in-write-chunk branch 3 times, most recently from db62609 to 5477453 Compare August 29, 2020 03:55
bai-charisu
bai-charisu reviewed Sep 21, 2020
View reviewed changes
src/fs/ext4_filesystem_impl.cpp
@@ -329,6 +329,33 @@ int Ext4FileSystemImpl::Write(int fd,
return length;
}

int Ext4FileSystemImpl::Write(int fd,
butil::IOBuf buf,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const &

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里是需要把iobuf写入到fd中，会涉及到对iobuf的修改，所以不能用const引用。
这里只拷贝了一次iobuf，开销不大。

src/fs/ext4_filesystem_impl.cpp Outdated
int retryTimes = 0;

while (remainLength > 0) {
ssize_t ret = buf.pcut_into_file_descriptor(fd, offset, length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

第三个参数应该是remainLength吧

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/fs/ext4_filesystem_impl.h
@@ -52,6 +54,7 @@ class Ext4FileSystemImpl : public LocalFileSystem {
int List(const string& dirPath, vector<std::string>* names) override;
int Read(int fd, char* buf, uint64_t offset, int length) override;
int Write(int fd, const char* buf, uint64_t offset, int length) override;
int Write(int fd, butil::IOBuf buf, uint64_t offset, int length) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const &

src/fs/local_filesystem.h
* @param length：写入数据的长度
* @return 返回成功写入的数据长度，失败返回-1
*/
virtual int Write(int fd, butil::IOBuf buf, uint64_t offset,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同上

test/chunkserver/datastore/chunkfilepool_mock_unittest.cpp
@@ -161,7 +162,7 @@ TEST_F(CSChunkfilePoolMockTest, PersistEnCodeMetaInfoTest) {
{
EXPECT_CALL(*lfs_, Open(poolMetaPath, _))
.WillOnce(Return(-1));
EXPECT_CALL(*lfs_, Write(_, _, _, _))
EXPECT_CALL(*lfs_, Write(_, Matcher<const char*>(_), _, _))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为啥这些还是会带char*的参数？

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里是因为写入操作可能会与快照的metapage有关，更新metapage调用的是Write(_, const char*,....)这个接口。

@wu-hanqing wu-hanqing force-pushed the chunkserver-zeroize-copy-in-write-chunk branch from 5477453 to 8951933 Compare September 21, 2020 14:53
ilixiaocui
ilixiaocui reviewed Sep 23, 2020
View reviewed changes
src/chunkserver/datastore/chunkserver_datastore.h
off_t offset,
size_t length,
uint32_t* cost,
const std::string & cloneSourceLocation = "");

// Deprecated, only use for unit & integration test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecated？ 现在的测试中还是要用的吧

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

测试代码里还是用了很多这个接口，所以暂时没改掉。
这里备注了一下，正常情况下不应该使用这个接口，只有单元测试/集成测试需要。

ilixiaocui
ilixiaocui reviewed Sep 23, 2020
View reviewed changes
src/fs/ext4_filesystem_impl.cpp
int retryTimes = 0;

while (remainLength > 0) {
ssize_t ret = buf.pcut_into_file_descriptor(fd, offset, remainLength);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

会分批写入文件吗

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

正常来说不会。这里内部还是用的pwritev来做的，有可能会返回EINTR。

ilixiaocui
ilixiaocui reviewed Sep 23, 2020
View reviewed changes
src/fs/ext4_filesystem_impl.h
@@ -52,6 +54,7 @@ class Ext4FileSystemImpl : public LocalFileSystem {
int List(const string& dirPath, vector<std::string>* names) override;
int Read(int fd, char* buf, uint64_t offset, int length) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read这里返回也会从char*拷贝一次到iobuf?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

读请求的buffer，会通过iobuf::append_user_data的方式由iobuf管理，这种情况不会拷贝。

@wu-hanqing
use butil::IOBuf in WriteChunk
7306508 
Change-Id: If514d166a27da3066649820a140f28ec193b41a1
@wu-hanqing wu-hanqing force-pushed the chunkserver-zeroize-copy-in-write-chunk branch from 8951933 to 7306508 Compare September 23, 2020 02:23
@wu-hanqing wu-hanqing closed this Sep 23, 2020
@wu-hanqing wu-hanqing reopened this Sep 23, 2020
@wu-hanqing wu-hanqing closed this Sep 23, 2020
@wu-hanqing wu-hanqing reopened this Sep 23, 2020
@ilixiaocui ilixiaocui merged commit 39614a3 into opencurve:master Sep 23, 2020
@wu-hanqing wu-hanqing deleted the chunkserver-zeroize-copy-in-write-chunk branch September 24, 2020 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilixiaocui ilixiaocui ilixiaocui approved these changes

@bai-charisu bai-charisu bai-charisu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wu-hanqing @ilixiaocui @bai-charisu