----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27111/#review58418 -----------------------------------------------------------
Close to ready. Major points are cleaning up the cgroups statistics functions and making the test child safe. src/linux/cgroups.hpp <https://reviews.apache.org/r/27111/#comment99413> s/The a pair/The pair/ src/linux/cgroups.cpp <https://reviews.apache.org/r/27111/#comment99414> We accept ">>" now. src/linux/cgroups.cpp <https://reviews.apache.org/r/27111/#comment99481> I think it's error-prone to have the implicit <read, write> pairing and specifying the file. What about hiding this function in an internal namespace and exposing the various statistics explicitly under a descriptive namespace, e.g., perhaps something like this: Bytes blkio::statistics::bytes::{read,write}() uint64_t blkio::statistics::iops::{read,write}() Yes, converting to Bytes to return is unnecessary in your usage but it avoids any ambiguity for the user of this library function. src/linux/fs.cpp <https://reviews.apache.org/r/27111/#comment99419> Any tests for this? src/linux/fs.cpp <https://reviews.apache.org/r/27111/#comment99417> move this to after the file.fail() check? src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/27111/#comment99473> kill newline src/slave/containerizer/isolators/cgroups/blkio.cpp <https://reviews.apache.org/r/27111/#comment99482> What's the difference between blkio.throttle.io_service_bytes and blkio.io_service_bytes? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/27111/#comment99490> s/alligned/aligned/ src/tests/isolator_tests.cpp <https://reviews.apache.org/r/27111/#comment99492> posix_memalign is definitely not async-signal-safe src/tests/isolator_tests.cpp <https://reviews.apache.org/r/27111/#comment99488> s/ret/write/? or count or ... something other than 'ret', please :-) pwrite() is not listed as async-signal-safe: http://man7.org/linux/man-pages/man7/signal.7.html src/tests/isolator_tests.cpp <https://reviews.apache.org/r/27111/#comment99489> ditto src/tests/isolator_tests.cpp <https://reviews.apache.org/r/27111/#comment99484> This "setup" function must be async-signal-safe as it's called between the fork and exec, and we're forking from a threaded parent. http://man7.org/linux/man-pages/man7/signal.7.html Can you verify that everything in there is safe (and comment that it is) or put this code into a separate helper binary? - Ian Downes On Oct. 23, 2014, 2:58 p.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27111/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2014, 2:58 p.m.) > > > Review request for mesos, Benjamin Hindman and Ian Downes. > > > Bugs: MESOS-1977 > https://issues.apache.org/jira/browse/MESOS-1977 > > > Repository: mesos-git > > > Description > ------- > > Implement a block io isolator that just publishes read / write, bytes / > operations per second. > A split of r25922 as suggested by Ian. > > > Diffs > ----- > > include/mesos/mesos.proto 6b93e90 > src/Makefile.am 2617f77 > src/linux/cgroups.hpp abf31df > src/linux/cgroups.cpp 62df4b7 > src/linux/fs.hpp ac8b5f4 > src/linux/fs.cpp b01d14c > src/slave/containerizer/isolators/cgroups/blkio.hpp PRE-CREATION > src/slave/containerizer/isolators/cgroups/blkio.cpp PRE-CREATION > src/slave/containerizer/mesos/containerizer.cpp 9d08329 > src/tests/isolator_tests.cpp 52b38a3 > > Diff: https://reviews.apache.org/r/27111/diff/ > > > Testing > ------- > > make check > sudo ./mesos-tests --gtest_filter="BlkIOIsolatorTest*" > > > Thanks, > > Joris Van Remoortere > >
