Hi, On Tue, Apr 05, 2011 at 04:05:58PM +1000, Dave Chinner wrote: > Background: the quotaon syscall on XFS can only change whether > quotas are enforced or not in XFS. It can't switch quotas on at > all because that has to be done at mount time. Switching quotas on > at mount time sets the appropriate flags in the superblock, so we > can always rely on a superblock flag check for whether accounting > is enabled or not. Thanks for the explanation. > > > > (mp->m_sb.sb_qflags & XFS_UQUOTA_ACCT) == 0 && > > (flags & XFS_UQUOTA_ENFD)) > > ^^^^^^^^^^^^^^^^^^^^^^^ > > And those are the checks that are meaningful. Should have been the flags & _ACCT check line above, my mistake. > > Besides, there are more cleanups and fixups in this function: > > > > line 331: > > /* No fs can turn on quotas with a delayed effect */ > > ASSERT((flags & XFS_ALL_QUOTA_ACCT) == 0); > > > > seems to be broken too: > > * the acct flags are masked out -> always asserts true > > * doing s/flags/accflags/ does not make sense > > No, that makes perfect sense given the commit above. We use ASSERT() > statements to document constraints and assumptions in the code, and > that is exactly what this does.... Yes, in context of the linked commit. From a technical POV, this assert would only fail when the constants for XFS_ALL_QUOTA_ACCT and XFS_ALL_QUOTA_ENFD overlap. > Well, yes. that's why we have a relatively strict review process - > the reviewer needs to check stuff like this, not just blindly trust > the person who wrote the code did it. That's the essence of the > meaning of the Reviewed-by tag we throw around all the time - it's > not just a rubber stamp. > > Indeed, it's not uncommon for me to spend hours doing this sort of > analysis when reviewing complex changes, and all you might see on > the mailing list is (maybe) a short comment accompanying a > Reviewed-by tag.... Understood and no doubts here. I was missing something like "removing just the variable and will clean the rest later", which Christoph wrote in a later email. dave _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
