Hi Christoph,
Thanks for the comments.
See responses inline below.
On Wed, 2011-10-19 at 05:57 -0400, Christoph Hellwig wrote:
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -841,8 +841,10 @@ xfs_qm_dqget(
> > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> > if (type == XFS_DQ_USER)
> > ASSERT(ip->i_udquot == NULL);
> > - else
> > + else if (type == XFS_DQ_GROUP)
> > ASSERT(ip->i_gdquot == NULL);
> > + else
> > + ASSERT(ip->i_pdquot == NULL);
>
> An xfs_inode_dquot(ip, type) macro that gets you the right quota for
> a given type would be highly useful. I'd love to see that as the
> first patch in the series, as it would a) clean up a lot of code like
> this and b) would also help with my TODO list of overlaying the
> user/group dquots with the existing fields in the VFS inode and thus
> shrinking the XFS inode.
>
will do.
> > @@ -933,8 +935,8 @@ xfs_qm_dqget(
> > xfs_dqlock(dqp);
> > goto dqret;
> > }
> > - } else {
> > - if (!XFS_IS_OQUOTA_ON(mp)) {
> > + } else if (type == XFS_DQ_GROUP) {
> > + if (!XFS_IS_GQUOTA_ON(mp)) {
> > /* inode stays locked on return */
> > xfs_qm_dqdestroy(dqp);
> > return XFS_ERROR(ESRCH);
>
> Together with a
>
> XFS_IS_TYPE_QUOTA_ON(mp, type)
>
> that should also really greatly simplify this error path in
> xfs_qm_dqget.
>
> > +#define XFS_IS_THIS_QUOTA_OFF(d) (! ((XFS_QM_ISUDQ(d) && \
> > + XFS_IS_UQUOTA_ON((d)->q_mount)) || \
> > + (XFS_QM_ISGDQ(d) && \
> > + XFS_IS_GQUOTA_ON((d)->q_mount)) || \
> > + (XFS_QM_ISPDQ(d) && \
> > + XFS_IS_PQUOTA_ON((d)->q_mount))))
>
> And the two callers of this could easily be converted to
> !XFS_IS_TYPE_QUOTA_ON.
>
> > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> > index 760140d..26f95a6 100644
> > --- a/fs/xfs/xfs_inode.h
> > +++ b/fs/xfs/xfs_inode.h
> > @@ -231,6 +231,7 @@ typedef struct xfs_inode {
> > struct xfs_mount *i_mount; /* fs mount struct ptr */
> > struct xfs_dquot *i_udquot; /* user dquot */
> > struct xfs_dquot *i_gdquot; /* group dquot */
> > + struct xfs_dquot *i_pdquot; /* project dquot */
>
> I'd really prefer to leave growing the inode until we are fully done
> adding it. So move this part to the back, just add the macro and quota
> on check in the first patch, but always redirect it to i_gdquot for now.
Sure, will do.
>
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -628,6 +628,7 @@ xfs_sb_to_disk(
> > xfs_sb_field_t f;
> > int first;
> > int size;
> > + __be16 saved_qflags = from->sb_qflags;
> >
> > ASSERT(fields);
> > if (!fields)
> > @@ -669,6 +670,7 @@ xfs_sb_to_disk(
> >
> > fields &= ~(1LL << f);
> > }
> > + from->sb_qflags = saved_qflags;
> > }
>
> What is this for? It will need some better documentation.
We want to keep XFS_OQUOTA_.* flags in-disk but on in-core. the while
loop between this save and restore copies the information from in-core
to a buffer destined to go to disk. So, I save the in-core information
then change it which is then copied to the buffer, then I restore the
in-core information back.
I will add some comments to that effect.
>
> > - ASSERT(ip->i_udquot == NULL);
> > + ASSERT_ALWAYS(ip->i_udquot == NULL);
>
> Why the change to ASSERT_ALWAYS here?
oops. debug left over. will remove it.
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
