No subject

root filesystem was special and quota accounting for the root
filesystem was not done at mount time. That was removed in late
2002:

http://oss.sgi.com/cgi-bin/gitweb.cgi?p=archive/xfs-import.git;a=commit;h=3e01b37c1ba0a3839956660b78e09a17ac5ff67b
http://oss.sgi.com/cgi-bin/gitweb.cgi?p=archive/xfs-import.git;a=commitdiff;h=3e01b37c1ba0a3839956660b78e09a17ac5ff67b

so accflags has been unused for almost 10 years. I've gone through
quite a bit more of the history, but thæt commit it the key one.

> and not only this, a few lines below:
> 
>         if (((flags & XFS_UQUOTA_ACCT) == 0 &&
>               ^^^^^^^^^^^^^^^^^^^^^^^

The above commit should have removed these checks are they were not
needed anymore.

> 
>             (mp->m_sb.sb_qflags & XFS_UQUOTA_ACCT) == 0 &&
>             (flags & XFS_UQUOTA_ENFD))
>              ^^^^^^^^^^^^^^^^^^^^^^^

And those are the checks that are meaningful.

> The code is there since ages so I wonder whether anybody hit this bug or
> if I misunderstood xfs/quota documentation.

Not a bug. Crufty, yes, but functioning correctly.

> 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....

> I'd like to ask maintainers to be cautious when accepting these
> simply looking cleanups, they may actually hide bugs.

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....

Cheers,

Dave.
-- 
Dave Chinner
david at fromorbit.com