xfs
[Top] [All Lists]

Re: [PATCH v10 08/11] xfs: Add pquota fields where gquota is used.

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH v10 08/11] xfs: Add pquota fields where gquota is used.
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Wed, 10 Jul 2013 16:46:38 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130710213907.GZ20932@xxxxxxx>
Organization: IBM
References: <1372371914-11370-1-git-send-email-sekharan@xxxxxxxxxx> <1372371914-11370-9-git-send-email-sekharan@xxxxxxxxxx> <20130710213907.GZ20932@xxxxxxx>
Reply-to: sekharan@xxxxxxxxxx
I am rolling up a new set with changes based on David's comments and
Jan's comments.

Will address your comments too.
On Wed, 2013-07-10 at 16:39 -0500, Ben Myers wrote:
> Hi Chandra,
> 
> On Thu, Jun 27, 2013 at 05:25:11PM -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.
> 
>      ^ * is extra?

will fix
> 
> >    * add a project dquot argument to various functions.
> > 
> > Not using the pquotino field from superblock yet.
> > 
> > Signed-off-by: Chandra Seetharaman <sekharan@xxxxxxxxxx>
> 
> Patch looks good to me.
> 
> Reviewed-by: Ben Myers <bpm@xxxxxxx>
> 
> I had just a few tiny nits and questions I found in review.  If you were not
> already planning to repost I suggest you ignore them.  ;)
> 
> Regards,
>       Ben
> 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 5e99968..71a8bc5 100644
> > --- 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);
> >             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);
> >                     if (code)       /* out of quota */
> >                             goto error_return;
> > @@ -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_set_projid(ip, fa->fsx_projid);
> >  
> > @@ -1160,13 +1160,13 @@ xfs_ioctl_setattr(
> >      */
> >     xfs_qm_dqrele(olddquot);
> >     xfs_qm_dqrele(udqp);
> > -   xfs_qm_dqrele(gdqp);
> > +   xfs_qm_dqrele(pdqp);
> >  
> >     return code;
> >  
> >   error_return:
> >     xfs_qm_dqrele(udqp);
> > -   xfs_qm_dqrele(gdqp);
> > +   xfs_qm_dqrele(pdqp);
> >     xfs_trans_cancel(tp, 0);
> >     if (lock_flags)
> >             xfs_iunlock(ip, lock_flags);
> 
> Here in xfs_ioctl_setattr, I'm not clear on why we're messing with the user
> dquot at all.  Could it be removed entirely?  A change in project id doesn't
> effect user quota, right?
> 
> Maybe that's an idea for a separate patch.

I will look into that after these get in :)
> 
> >  STATIC void
> > -xfs_qm_dqattach_grouphint(
> > -   xfs_dquot_t     *udq,
> > -   xfs_dquot_t     *gdq)
> > +xfs_qm_dqattach_hint(
> > +   struct xfs_inode        *ip,
> > +   int                     type)
> >  {
> > -   xfs_dquot_t     *tmp;
> > +   struct xfs_dquot **dqhint;
> 
> Maybe clearer as dqhintp.
> 
sure.
> > +   struct xfs_dquot *dqp;
> > +   struct xfs_dquot *udq = ip->i_udquot;
> >  
> >     xfs_dqlock(udq);
> >  
> > -   tmp = udq->q_gdquot;
> > -   if (tmp) {
> > -           if (tmp == gdq)
> 
> If I understand this xfs_qm_dqattach_hint correctly, this might be nice:
> 
>       ASSERT(type == XFS_DQ_GROUP || type == XFS_DQ_PROJ);

will do.

> 
> > 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)))  &&
> 
> These are not in the usual user/group/project order, and so are some other
> checks in this function.  Could be cleaned up.  Maybe a different patch too.
> 
I thought I fixed all of them around. Will fix this too.
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 


<Prev in Thread] Current Thread [Next in Thread>