xfs
[Top] [All Lists]

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

To: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Subject: Re: [PATCH v10 08/11] xfs: Add pquota fields where gquota is used.
From: Ben Myers <bpm@xxxxxxx>
Date: Wed, 10 Jul 2013 16:39:07 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1372371914-11370-9-git-send-email-sekharan@xxxxxxxxxx>
References: <1372371914-11370-1-git-send-email-sekharan@xxxxxxxxxx> <1372371914-11370-9-git-send-email-sekharan@xxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
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?

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

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

> +     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);

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

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