> On May 12, 2017, 10:13 a.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/xfs/utils.cpp > > Lines 157-158 (original), 155-156 (patched) > > <https://reviews.apache.org/r/55895/diff/6/?file=1713122#file1713122line157> > > > > Sorry I overlooked this in my last around of review but I had this > > question on > > https://reviews.apache.org/r/55897/diff/2/?file=1667426#file1667426line185: > > > > ``` > > I have questions about the need for soft limit below but I don't recall > > the reason for setting the soft limits earlier "just for consistency". Soft > > limits is one of the low-level system's funtionality that we didn't use. If > > we didn't use it, I am not sure about the need for our util or the reader > > to be aware of it (the concept of soft limit)? > > ``` > > > > Can we get rid of the soft limits? > > James Peach wrote: > We set the soft limit because otherwise it would look weird to have a > hard limit but no soft limit. It's easy to set and I don't see any upside in > leaving it unset, do you? > > Jiang Yan Xu wrote: > How is it weird? These are two different concepts and we are not using > soft limits. IIUC to soft limits depend on hard limits but not vice versa. > > I as a reader needed to read additional docs to understand what it is and > as a reviewer had to reason about whether the code is using soft limits > correctly. The behavior of soft limits depend on other varibles which are not > explicitly set or documented here and seemingly it's only because we are > setting the two with the same value that the soft limits don't kick in. > > Code with no effect means redundancy but my point is more this is an > additional concept that we don't need to understand. There are other fields > and concepts in > http://xfs.org/docs/xfsdocs-xml-dev/XFS_Filesystem_Structure/tmp/en-US/html/Internal_Inodes.html > and other features provieded by XFS that I'd be happy to be ignorant of. :)
Chatted offline. IMO the soft limits is a feature that we didn't use and thus could be omitted but jpeach felt it's an integral part of the XFS quota system and the reader should understand. Let's keep the code that sets the soft limit but I feel the following comment would help explain better: a/Functionally all we need is the hard quota./Functionally all we need is the hard limit; the soft limit has no effect when it is the same as the hard limit. I could take care of if no objection. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55895/#review174763 ----------------------------------------------------------- On May 16, 2017, 4:09 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55895/ > ----------------------------------------------------------- > > (Updated May 16, 2017, 4:09 p.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Bugs: MESOS-5116 > https://issues.apache.org/jira/browse/MESOS-5116 > > > Repository: mesos > > > Description > ------- > > Extract a BasicBlocks class to handle disk blocks to clarify block-based > arithmetic in the XFS disk isolator. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/xfs/utils.hpp > eddd4c37fb42339ca21ecb392dea47acf6b277bb > src/slave/containerizer/mesos/isolators/xfs/utils.cpp > 8018ad348d26bd962357543a5fb9f6cb43ff13b1 > src/tests/containerizer/xfs_quota_tests.cpp > 7beb60b059910a0d4451b1ace895a35dc974a043 > > > Diff: https://reviews.apache.org/r/55895/diff/7/ > > > Testing > ------- > > sudo make check (Fedora 25) > > > Thanks, > > James Peach > >
