On Fri, 2013-05-17 at 14:23 +1000, Dave Chinner wrote:
> On Fri, May 10, 2013 at 04:21:26PM -0500, Chandra Seetharaman wrote:
> > Add project quota changes to all the places where group quota field
> > is used:
> > * add separate project quota members into various structures
> > * split project quota and group quotas so that instead of overriding
> > the group quota members incore, the new project quota members are
> > used instead
> > * get rid of usage of the OQUOTA flag incore, in favor of separate
> > * group
> > and project quota flags.
> > * add a project dquot argument to various functions.
> >
> > No externally visible interfaces changed.
>
> Looks pretty good. Some relatively minor changes below.
>
> > @@ -568,6 +567,17 @@ xfs_qm_dqrepair(
> > return 0;
> > }
> >
> > +struct xfs_inode *
> > +xfs_dq_to_quota_inode(struct xfs_dquot *dqp)
> > +{
> > + if (XFS_QM_ISUDQ(dqp))
> > + return dqp->q_mount->m_quotainfo->qi_uquotaip;
> > + if (XFS_QM_ISGDQ(dqp))
> > + return dqp->q_mount->m_quotainfo->qi_gquotaip;
> > + ASSERT(XFS_QM_ISPDQ(dqp));
> > + return dqp->q_mount->m_quotainfo->qi_pquotaip;
> > +}
>
> Consolidate this with xfs_dquot_tree() as a static inline function,
> I think. Let's try and keep all these little helpers together so
> they are easy to find ;)
will do.
>
> > +
> > /*
> > * Maps a dquot to the buffer containing its on-disk version.
> > * This returns a ptr to the buffer containing the on-disk dquot
> > @@ -584,7 +594,7 @@ xfs_qm_dqtobp(
> > xfs_bmbt_irec_t map;
> > int nmaps = 1, error;
> > xfs_buf_t *bp;
> > - xfs_inode_t *quotip = XFS_DQ_TO_QIP(dqp);
> > + xfs_inode_t *quotip = xfs_dq_to_quota_inode(dqp);
>
> convert to struct xfs_inode a the same time....
will do
>
> > @@ -52,7 +51,8 @@ typedef struct xfs_dquot {
> > @@ -144,9 +146,6 @@ static inline xfs_dquot_t *xfs_inode_dquot(struct xfs_inode *ip, int type)
> > #define XFS_QM_ISPDQ(dqp) ((dqp)->dq_flags & XFS_DQ_PROJ)
> > #define XFS_QM_ISGDQ(dqp) ((dqp)->dq_flags & XFS_DQ_GROUP)
> > #define XFS_DQ_TO_QINF(dqp) ((dqp)->q_mount->m_quotainfo)
> > -#define XFS_DQ_TO_QIP(dqp) (XFS_QM_ISUDQ(dqp) ? \
> > - XFS_DQ_TO_QINF(dqp)->qi_uquotaip : \
> > - XFS_DQ_TO_QINF(dqp)->qi_gquotaip)
>
> XFS_DQ_TO_QINF can go away, too.
sure
>
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -928,7 +928,7 @@ xfs_ioctl_setattr(
> > struct xfs_trans *tp;
> > unsigned int lock_flags = 0;
> > struct xfs_dquot *udqp = NULL;
> > - struct xfs_dquot *gdqp = NULL;
> > + struct xfs_dquot *pdqp = NULL;
> > struct xfs_dquot *olddquot = NULL;
> > int code;
> >
> > @@ -957,7 +957,7 @@ xfs_ioctl_setattr(
> > if (XFS_IS_QUOTA_ON(mp) && (mask & FSX_PROJID)) {
> > code = xfs_qm_vop_dqalloc(ip, ip->i_d.di_uid,
> > ip->i_d.di_gid, fa->fsx_projid,
> > - XFS_QMOPT_PQUOTA, &udqp, &gdqp);
> > + XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
>
> We're only passing in XFS_QMOPT_PQUOTA, so we do not need to pass in
> uid, gid, udqp or gdqp here....
>
> Can you add a comment here that this may cause user/group quotas
> to be allocated, but we don't need it here in this function because
> we are only specifically changing the project quota via a chown
> operation.
>
> Indeed, the only reason a user quota is passed in is to make use of
> the dquot hint that the udq might hold that points to the other
> dquots on the inode....
will do
>
> > if (code)
> > return code;
> > }
> > @@ -994,8 +994,8 @@ xfs_ioctl_setattr(
> > XFS_IS_PQUOTA_ON(mp) &&
> > xfs_get_projid(ip) != fa->fsx_projid) {
> > ASSERT(tp);
> > - code = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
> > - capable(CAP_FOWNER) ?
> > + code = xfs_qm_vop_chown_reserve(tp, ip, udqp, NULL,
> > + pdqp, capable(CAP_FOWNER) ?
> > XFS_QMOPT_FORCE_RES : 0);
> > @@ -1113,7 +1113,7 @@ xfs_ioctl_setattr(
> > if (xfs_get_projid(ip) != fa->fsx_projid) {
> > if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) {
> > olddquot = xfs_qm_vop_chown(tp, ip,
> > - &ip->i_gdquot, gdqp);
> > + &ip->i_pdquot, pdqp);
>
> xfs_qm_vop_chown() is only called on one dquot at a time, so it can
> only exchange one dquot at a time. and udqp is not used for hints,
> so it looks to me like udqp and gdqp are wholly unnecessary in this
> function....
Did not fully understand this comment. Can you please elaborate ?
>
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index d82efaa..7c54ea4 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -517,7 +517,7 @@ xfs_setattr_nonsize(
> > ASSERT(udqp == NULL);
> > ASSERT(gdqp == NULL);
> > error = xfs_qm_vop_dqalloc(ip, uid, gid, xfs_get_projid(ip),
> > - qflags, &udqp, &gdqp);
> > + qflags, &udqp, &gdqp, NULL);
>
> Why is there no project quota support here, even though we pass in a
> project ID? I know the answer, but please add a comment so that a
> couple of years down the track I don't need to go remind myself of
> why....
Not clear what you are expecting here.
>
> > /*
> > - * Given a udquot and gdquot, attach a ptr to the group dquot in the
> > + * Given a udquot and gdquot, attach a ptr to the group/project dquot in the
> > * udquot as a hint for future lookups.
> > */
> > STATIC void
> > -xfs_qm_dqattach_grouphint(
> > - xfs_dquot_t *udq,
> > - xfs_dquot_t *gdq)
> > +xfs_qm_dqattach_grouphint(xfs_inode_t *ip, int type)
>
> Wrong format for the function header And it's not longer just for
> the group dquot, so I'd rename just to xfs_qm_dqattach_hint. i.e:
will fix.
>
> STATIC void
> xfs_qm_dqattach_hint(
> struct xfs_inode *ip,
> int type)
>
> > {
> > - xfs_dquot_t *tmp;
> > + struct xfs_dquot **dqhint;
> > + struct xfs_dquot *gpdq;
>
> not a group dquot. so perhaps just call it dqp?
sure.
>
> > @@ -1395,19 +1453,27 @@ xfs_qm_init_quotainos(
> > if (XFS_IS_UQUOTA_ON(mp) &&
> > mp->m_sb.sb_uquotino != NULLFSINO) {
> > ASSERT(mp->m_sb.sb_uquotino > 0);
> > - if ((error = xfs_iget(mp, NULL, mp->m_sb.sb_uquotino,
> > - 0, 0, &uip)))
> > + error = xfs_iget(mp, NULL, mp->m_sb.sb_uquotino,
> > + 0, 0, &uip);
> > + if (error)
> > return XFS_ERROR(error);
> > }
> > - if (XFS_IS_OQUOTA_ON(mp) &&
> > + if (XFS_IS_GQUOTA_ON(mp) &&
> > mp->m_sb.sb_gquotino != NULLFSINO) {
> > ASSERT(mp->m_sb.sb_gquotino > 0);
> > - if ((error = xfs_iget(mp, NULL, mp->m_sb.sb_gquotino,
> > - 0, 0, &gip))) {
> > - if (uip)
> > - IRELE(uip);
> > - return XFS_ERROR(error);
> > - }
> > + error = xfs_iget(mp, NULL, mp->m_sb.sb_gquotino,
> > + 0, 0, &gip);
> > + if (error)
> > + goto error_rele;
> > + }
> > + /* Use sb_gquotino for now */
> > + if (XFS_IS_PQUOTA_ON(mp) &&
> > + mp->m_sb.sb_gquotino != NULLFSINO) {
> > + ASSERT(mp->m_sb.sb_gquotino > 0);
> > + error = xfs_iget(mp, NULL, mp->m_sb.sb_gquotino,
> > + 0, 0, &pip);
> > + if (error)
> > + goto error_rele;
>
> There is no need for this trickery, right? All that is needed is:
>
Will be taken care of in the next patch.
>
>
> qino = xfs_sb_version_has_pquota(&mp->m_sb) ?
> mp->m_sb.sb_pquotino :
> mp->m_sb.sb_gquotino;
>
> And the correct on-disk inode is read....
>
The object of this patch is to replace all gquota with pquota stuff. I
am not using the pquotino or xfs_sb_version_has_pquota() until patch 3.
If I get the change forward, I have to merge patch 3 and part of this
patch into patch 1, which adds up lots of work.
The way the patches are split now, IMO, is easy to follow and works
independently.
>
> > @@ -1804,15 +1896,21 @@ xfs_qm_vop_chown(
> > */
> > int
> > xfs_qm_vop_chown_reserve(
> > - xfs_trans_t *tp,
> > - xfs_inode_t *ip,
> > - xfs_dquot_t *udqp,
> > - xfs_dquot_t *gdqp,
> > - uint flags)
> > + xfs_trans_t *tp,
> > + xfs_inode_t *ip,
>
> struct xfs_trans, struct xfs_inode.
>
> > + struct xfs_dquot *udqp,
> > + struct xfs_dquot *gdqp,
> > + struct xfs_dquot *pdqp,
> > + uint flags)
> > {
> > xfs_mount_t *mp = ip->i_mount;
> > uint delblks, blkflags, prjflags = 0;
> > - xfs_dquot_t *unresudq, *unresgdq, *delblksudq, *delblksgdq;
> > + struct xfs_dquot *unresudq = NULL;
> > + struct xfs_dquot *unresgdq = NULL;
> > + struct xfs_dquot *unrespdq = NULL;
> > + struct xfs_dquot *delblksudq = NULL;
> > + struct xfs_dquot *delblksgdq = NULL;
> > + struct xfs_dquot *delblkspdq = NULL;
> > int error;
>
> You may as well line up the other 3 declarations, and make is a
> struct xfs_mount....
>
> .... and I just realised that looking through this code reviewing
> the xfs_ioctl_setattr() changes that I'd not read the existing
> code correctly.
>
> Why not? becuse unresudq and unresgdq are not very different. The
> classic case of the brain not really seeing the difference between
> single letters inside a larger word, and that's where the single
> letter difference in the variables are. i.e: this phenomenon:
>
> http://www.ecenglish.com/learnenglish/lessons/can-you-read
>
> I can read that mess as fast as if it were spelt correctly, hence
> the importance of the first/last letter of variable names...
>
> So, can you rename these variables:
>
> udq_unres
> gdq_unres
> pdq_unres
> udq_delblks
> gdq_delblks
> pdq_delblks
>
> so it's a little more obvious to my easily tricked brain that they
> are different....
>
Why not :)
>
> > @@ -1935,13 +2039,18 @@ xfs_qm_vop_create_dqattach(
> > }
> > if (gdqp) {
> > ASSERT(ip->i_gdquot == NULL);
> > - ASSERT(XFS_IS_OQUOTA_ON(mp));
> > - ASSERT((XFS_IS_GQUOTA_ON(mp) ?
> > - ip->i_d.di_gid : xfs_get_projid(ip)) ==
> > - be32_to_cpu(gdqp->q_core.d_id));
> > -
> > + ASSERT(XFS_IS_GQUOTA_ON(mp));
> > + ASSERT(ip->i_d.di_gid == be32_to_cpu(gdqp->q_core.d_id));
> > ip->i_gdquot = xfs_qm_dqhold(gdqp);
> > xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
> > }
> > + if (pdqp) {
> > + ASSERT(ip->i_pdquot == NULL);
> > + ASSERT(XFS_IS_PQUOTA_ON(mp));
> > + ASSERT(xfs_get_projid(ip) == be32_to_cpu(pdqp->q_core.d_id));
> > +
> > + ip->i_pdquot = xfs_qm_dqhold(pdqp);
> > + xfs_trans_mod_dquot(tp, pdqp, XFS_TRANS_DQ_ICOUNT, 1);
> > + }
> > }
>
> Something this just triggered. All transaction reservations that
> reserve space for dquots need to be upped from 2 to 3 dquots.
Can you elaborate please.
>
> > -extern void xfs_trans_mod_dquot(xfs_trans_t *, xfs_dquot_t *, uint, long);
> > -extern int xfs_trans_reserve_quota_bydquots(xfs_trans_t *, xfs_mount_t *,
> > - xfs_dquot_t *, xfs_dquot_t *, long, long, uint);
> > -extern void xfs_trans_dqjoin(xfs_trans_t *, xfs_dquot_t *);
> > -extern void xfs_trans_log_dquot(xfs_trans_t *, xfs_dquot_t *);
> > +extern void xfs_trans_mod_dquot(xfs_trans_t *, struct xfs_dquot *, uint, long);
> > +extern void xfs_trans_dqjoin(xfs_trans_t *, struct xfs_dquot *);
> > +extern void xfs_trans_log_dquot(xfs_trans_t *, struct xfs_dquot *);
>
> Remove the typedefs at the same time.
sure
>
>
> > /*
> > - * We keep the usr and grp dquots separately so that locking will be easier
> > - * to do at commit time. All transactions that we know of at this point
> > + * We keep the usr, grp, and prj dquots separately so that locking will be
> > + * easier to do at commit time. All transactions that we know of at this point
> > * affect no more than two dquots of one type. Hence, the TRANS_MAXDQS value.
> > */
> > +enum {
> > + XFS_QM_TRANS_USR = 0,
> > + XFS_QM_TRANS_GRP,
> > + XFS_QM_TRANS_PROJ,
> > + XFS_QM_TRANS_DQTYPES
> > +};
> > #define XFS_QM_TRANS_MAXDQS 2
> > -typedef struct xfs_dquot_acct {
> > - xfs_dqtrx_t dqa_usrdquots[XFS_QM_TRANS_MAXDQS];
> > - xfs_dqtrx_t dqa_grpdquots[XFS_QM_TRANS_MAXDQS];
> > -} xfs_dquot_acct_t;
> > +struct xfs_dquot_acct {
> > + struct xfs_dqtrx dqs[XFS_QM_TRANS_DQTYPES][XFS_QM_TRANS_MAXDQS];
> > +};
> >
> > /*
> > * Users are allowed to have a usage exceeding their softlimit for
> > diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
> > index 2d02eac..72a4fdd 100644
> > --- a/fs/xfs/xfs_qm_bhv.c
> > +++ b/fs/xfs/xfs_qm_bhv.c
> > @@ -115,7 +115,7 @@ xfs_qm_newmount(
> > (pquotaondisk && !XFS_IS_PQUOTA_ON(mp)) ||
> > (!pquotaondisk && XFS_IS_PQUOTA_ON(mp)) ||
> > (gquotaondisk && !XFS_IS_GQUOTA_ON(mp)) ||
> > - (!gquotaondisk && XFS_IS_OQUOTA_ON(mp))) &&
> > + (!gquotaondisk && XFS_IS_GQUOTA_ON(mp))) &&
> > xfs_dev_is_read_only(mp, "changing quota state")) {
> > xfs_warn(mp, "please mount with%s%s%s%s.",
> > (!quotaondisk ? "out quota" : ""),
>
> Shouldn't this hunk be in the first patch?
No, the object of the first patch is to just handle on disk .*OQUOTA.*
flags in the sb_flags.
In this patch I replace the rest of OQUOTA
>
>
> > index 1501f4f..cd0d133 100644
> > --- a/fs/xfs/xfs_vnodeops.c
> > +++ b/fs/xfs/xfs_vnodeops.c
> > @@ -498,6 +498,7 @@ xfs_create(
> > prid_t prid;
> > struct xfs_dquot *udqp = NULL;
> > struct xfs_dquot *gdqp = NULL;
> > + struct xfs_dquot *pdqp = NULL;
> > uint resblks;
> > uint log_res;
> > uint log_count;
> > @@ -516,7 +517,7 @@ xfs_create(
> > * Make sure that we have allocated dquot(s) on disk.
> > */
> > error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> > - XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT, &udqp, &gdqp);
> > + XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT, &udqp, &gdqp, &pdqp);
>
> break that into two lines.
>
> XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> &udqp, &gdqp, &pdqp);
>
will do
> Cheers,
>
> Dave.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
