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.

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

cephfs: fix write_buf's _len overflow problem #13587

Merged
merged 1 commit into from Apr 15, 2017

Conversation

Projects
None yet
6 participants
@yanghonggang
Copy link
Contributor

yanghonggang commented Feb 22, 2017

After I have set about 400 64KB xattr kv pair to a file,
mds is crashed. Every time I try to start mds, it will crash again.
The root reason is write_buf._len overflowed when doing
Journaler::append_entry().

This patch try to fix this problem through the following changes:

1. limit file/dir's xattr size
2. throttle journal entry append operations
3. add bufferlist _len overflow checking

Fixes: http://tracker.ceph.com/issues/19033
Signed-off-by: Yang Honggang joseph.yang@xtaotech.com

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

yanghonggang commented Feb 23, 2017

@jcsp Your comments and suggestions will be greatly appreciated.

@ukernel

This comment has been minimized.

Copy link
Member

ukernel commented Feb 23, 2017

please split the change into three commit. one for buffer.{h,cc}, one for Journaler.{h,cc} and one for Server.cc

@yanghonggang yanghonggang force-pushed the yanghonggang:master branch from 34973d4 to 7caa2d7 Feb 24, 2017

@yanghonggang yanghonggang reopened this Feb 24, 2017

@yanghonggang yanghonggang force-pushed the yanghonggang:master branch 2 times, most recently from ec88f41 to 4d1f282 Feb 25, 2017

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

yanghonggang commented Feb 27, 2017

@ukernel done!

@jcsp jcsp requested a review from ukernel Mar 8, 2017

@jcsp

This comment has been minimized.

Copy link
Contributor

jcsp commented Mar 8, 2017

@ukernel does this look good to you?

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

yanghonggang commented Mar 30, 2017

@ukernel @jcsp please help to review. many thanks.

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

yanghonggang commented Apr 12, 2017

@liewegas @ukernel @jcsp @gregsfortytwo

Normal user of our fs can damage the whole system by writing huge xattr kv pairs. I think this is a serious bug. Your quick reply can help me to fix this bug as soon as possible. Thank you for your time to review this patch.

@@ -984,6 +984,7 @@ static simple_spinlock_t buffer_debug_lock = SIMPLE_SPINLOCK_INITIALIZER;
char* ptr = _raw->data + _off + _len;
*ptr = c;
_len++;
assert(_len != 0);

This comment has been minimized.

Copy link
@branch-predictor

branch-predictor Apr 12, 2017

Member

I'd prefer if these asserts are compiled conditionally, for full debug builds only. This is already performance sensitive path that affects everyone.

This comment has been minimized.

Copy link
@yanghonggang

yanghonggang Apr 12, 2017

Author Contributor

@branch-predictor

Yes, this will affect performance. I think assert can be disabled globally by specifing DCMAKE_CXX_FLAGS="-DNDEBUG ..." cmake variable. Any better suggestion?

@jcsp

This comment has been minimized.

Copy link
Contributor

jcsp commented Apr 12, 2017

The Server and Journaler commits look good. After chatting with @liewegas the consensus on the buffer.h asserts is that although we already have lots of asserts there, we probably don't want to add more.

If you could drop the buffer.h commit from this PR, it will be good to merge.

@yanghonggang yanghonggang force-pushed the yanghonggang:master branch from 4d1f282 to fa377e5 Apr 13, 2017

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

yanghonggang commented Apr 13, 2017

@jcsp @branch-predictor @ukernel
Thank you for your suggestions, buffer.h commit is dropped. And commits are squashed together.


// check xattrs kv pairs size
size_t cur_xattrs_size = 0;
for (map<string, bufferptr>::iterator p = pxattrs->begin();

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 13, 2017

Contributor

i'd recommend use the range-based loop when possible.

@@ -8025,7 +8043,7 @@ void Server::handle_client_lssnap(MDRequestRef& mdr)
max_entries = infomap.size();
int max_bytes = req->head.args.readdir.max_bytes;
if (!max_bytes)
max_bytes = 512 << 10;
max_bytes = (512 << 10) + g_conf->mds_max_xattr_pairs_size; // make sure at least one item can be encoded

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 13, 2017

Contributor

please wrap at 80 chars.

cephfs: fix write_buf's _len overflow problem
After I have set about 400 64KB xattr kv pair to a file,
mds is crashed. Every time I try to start mds, it will crash again.
The root reason is write_buf._len overflowed when doing
Journaler::append_entry().

This patch try to fix this problem through the following changes:

 1. limit file/dir's xattr size
 2. throttle journal entry append operations

Fixes: http://tracker.ceph.com/issues/19033
Signed-off-by: Yang Honggang joseph.yang@xtaotech.com

@yanghonggang yanghonggang force-pushed the yanghonggang:master branch from fa377e5 to eb915d0 Apr 13, 2017

@yanghonggang

This comment has been minimized.

Copy link
Contributor Author

yanghonggang commented Apr 13, 2017

@tchaikov
all lines are wrapped at 80 characters and change for to range-based.

@jcsp

jcsp approved these changes Apr 13, 2017

@jcsp jcsp merged commit c65e4fb into ceph:master Apr 15, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.