> 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. :) > > Jiang Yan Xu wrote: > 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.
I revised it to: ``` // Set both the hard and the soft limit to the same quota. Functionally // all we need is the hard limit. The soft limit has no effect when it // is the same as the hard limit but we set it for explicitness. ``` Let me know if it can be improved. - 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 > >
