Hi,
I've seen this one too and I think this may not be a trivialy removable one, at
least not without some analysis.
On Mon, Apr 04, 2011 at 01:21:20PM -0500, Alex Elder wrote:
> > Index: linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c
> > ===================================================================
> > --- linux-2.6.orig/fs/xfs/quota/xfs_qm_syscalls.c 2011-04-03 06:40:45.399789765 -0700
> > +++ linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c 2011-04-03 06:43:00.219782939 -0700
> > @@ -313,14 +313,12 @@ xfs_qm_scall_quotaon(
> > {
> > int error;
> > uint qf;
> > - uint accflags;
> > __int64_t sbflags;
> >
> > flags &= (XFS_ALL_QUOTA_ACCT | XFS_ALL_QUOTA_ENFD);
> > /*
> > * Switching on quota accounting must be done at mount time.
> > */
> > - accflags = flags & XFS_ALL_QUOTA_ACCT;
> > flags &= ~(XFS_ALL_QUOTA_ACCT);
>
> Unrelated, but isn't the effect of this line plus the one a few
> lines up the same as this?
>
> flags &= XFS_ALL_QUOTA_ENFD;
yes it its, but then why was the separate accflags there? That's why I'm not
sure it should go away so easily.
>
> >
> > sbflags = 0;
and not only this, a few lines below:
if (((flags & XFS_UQUOTA_ACCT) == 0 &&
^^^^^^^^^^^^^^^^^^^^^^^
(mp->m_sb.sb_qflags & XFS_UQUOTA_ACCT) == 0 &&
(flags & XFS_UQUOTA_ENFD))
^^^^^^^^^^^^^^^^^^^^^^^
||
((flags & XFS_PQUOTA_ACCT) == 0 &&
^^^^^^^^^^^^^^^^^^^^^^^
(mp->m_sb.sb_qflags & XFS_PQUOTA_ACCT) == 0 &&
(flags & XFS_GQUOTA_ACCT) == 0 &&
^^^^^^^^^^^^^^^^^^^^^^^
(mp->m_sb.sb_qflags & XFS_GQUOTA_ACCT) == 0 &&
(flags & XFS_OQUOTA_ENFD))) {
xfs_debug(mp,
"%s: Can't enforce without acct, flags=%x sbflags=%x\n",
__func__, flags, mp->m_sb.sb_qflags);
return XFS_ERROR(EINVAL);
}
(the GQUOTA/PQUOTA/UQUOTA are covered by XFS_ALL_QUOTA_ACCT, masked above)
all the underlined expressions will be always zero, leading to a true ==
condition and leaving only the superblock flags to be checked.
Callchain of xfs_qm_scall_quotaon() goes up to
quotactl
do_quotactl
quota_setxstate
.set_xstate
xfs_fs_set_xstate
xfs_qm_scall_quotaon
where the final flags/accflags are passed from ioctl uflags and then
converted to XFS quota flags. Ie., the caller of the quotactl has to set
the ACCT flags, therefore I'd say the checked flags should state
accflags & XFS_GQUOTA_ACCT (...etc)
else, if the ACCT flags are not specified via superblock, the quotaon
sysctl will always fail with "Can't enforce without acct", even when
the accounting flags are specified via a mount option (speculated and
not tested myself).
The code is there since ages so I wonder whether anybody hit this bug or
if I misunderstood xfs/quota documentation.
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
line 374:
if ((error = xfs_qm_write_sb_changes(mp, sbflags)))
return (error);
convert to:
error = xfs_qm_...
if (error) ...
line 395:
mp->m_qflags |= (flags & XFS_ALL_QUOTA_ENFD);
->
mp->m_qflags |= flags;
I could send a patch, but I want to be clear on the accounting flags.
I'd like to ask maintainers to be cautious when accepting these
simply looking cleanups, they may actually hide bugs.
dave
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
