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
|