xfs
[Top] [All Lists]

Re: [RFC v6 PATCH 1/5] xfs: Remove incore use of XFS_OQUOTA_ENFD and XFS

To: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Subject: Re: [RFC v6 PATCH 1/5] xfs: Remove incore use of XFS_OQUOTA_ENFD and XFS_OQUOTA_CHKD
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 15 Aug 2012 08:46:45 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120720230208.20477.49663.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <20120720230202.20477.69766.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20120720230208.20477.49663.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Jul 20, 2012 at 06:02:08PM -0500, Chandra Seetharaman wrote:
> Remove all incore use of XFS_OQUOTA_ENFD and XFS_OQUOTA_CHKD. Instead,
> start using XFS_GQUOTA_.* XFS_PQUOTA_.* counterparts for GQUOTA nd
> PQUOTA respectively.
> 
> No changes are made to the on-disk version of the superblock yet. On-disk
> copy still uses XFS_OQUOTA_ENFD and XFS_OQUOTA_CHKD.
> 
> Read and write of the superblock does the conversion from *OQUOTA*
> to *[PG]QUOTA*.

Couple of minor style things....

> @@ -622,6 +636,7 @@ xfs_sb_to_disk(
>       xfs_sb_field_t  f;
>       int             first;
>       int             size;
> +      __uint16_t     tmp16;

tmp16 is a bad name for a temporary variable. Move it to the scope
that uses it, and name it for it's purpose. i.e:


>  
>       ASSERT(fields);
>       if (!fields)
> @@ -636,6 +651,26 @@ xfs_sb_to_disk(
>  
>               if (size == 1 || xfs_sb_info[f].type == 1) {
>                       memcpy(to_ptr + first, from_ptr + first, size);
> +             } else if (f == XFS_SBS_QFLAGS) {

                        __uint16_t      qflags;

> +                     /*
> +                      * The in-core version of sb_qflags do not have
> +                      * XFS_OQUOTA_* flags, whereas the on-disk version
> +                      * does.  Save the in-core sb_qflags temporarily,
> +                      * removing the new XFS_{PG}QUOTA_* flags and re-apply
> +                      * the old on-disk flags.
> +                      */
> +                     tmp16 = from->sb_qflags &

                        qflags = from->sb_qflags & ....

> +                                     ~(XFS_PQUOTA_ENFD | XFS_PQUOTA_CHKD |
> +                                     XFS_GQUOTA_ENFD | XFS_GQUOTA_CHKD);
> +
> +                     if (from->sb_qflags &
> +                                     (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD))
> +                             tmp16 |= XFS_OQUOTA_ENFD;
> +                     if (from->sb_qflags &
> +                                     (XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD))
> +                             tmp16 |= XFS_OQUOTA_CHKD;
> +
> +                     *(__be16 *)(to_ptr + first) = cpu_to_be16(tmp16);
>               } else {
>                       switch (size) {
>                       case 2:

....

> @@ -339,9 +339,11 @@ xfs_qm_scall_quotaon(
>           ||
>           ((flags & XFS_PQUOTA_ACCT) == 0 &&
>           (mp->m_sb.sb_qflags & XFS_PQUOTA_ACCT) == 0 &&
> -         (flags & XFS_GQUOTA_ACCT) == 0 &&
> +         (flags & XFS_PQUOTA_ENFD))
> +         ||
> +         ((flags & XFS_GQUOTA_ACCT) == 0 &&
>           (mp->m_sb.sb_qflags & XFS_GQUOTA_ACCT) == 0 &&
> -         (flags & XFS_OQUOTA_ENFD))) {
> +         (flags & XFS_GQUOTA_ENFD))) {

Can you fix the flat indenting here at the same time so that the
logic is obvious at a glance and consistent with the rest of the XFS
code? i.e.

        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_PQUOTA_ENFD)) ||
            (flags & XFS_GQUOTA_ACCT) == 0 &&
             (mp->m_sb.sb_qflags & XFS_GQUOTA_ACCT) == 0 &&
             (flags & XFS_GQUOTA_ENFD))) {

>               xfs_debug(mp,
>                       "%s: Can't enforce without acct, flags=%x sbflags=%x\n",
>                       __func__, flags, mp->m_sb.sb_qflags);
> @@ -771,8 +773,10 @@ xfs_qm_scall_getquota(
>        * so return zeroes in that case.
>        */
>       if ((!XFS_IS_UQUOTA_ENFORCED(mp) && dqp->q_core.d_flags == XFS_DQ_USER) 
> ||
> -         (!XFS_IS_OQUOTA_ENFORCED(mp) &&
> -                     (dqp->q_core.d_flags & (XFS_DQ_PROJ | XFS_DQ_GROUP)))) {
> +         (!XFS_IS_PQUOTA_ENFORCED(mp)
> +                     && dqp->q_core.d_flags == XFS_DQ_PROJ) ||
> +         (!XFS_IS_GQUOTA_ENFORCED(mp)
> +                     && dqp->q_core.d_flags == XFS_DQ_GROUP)) {

Same here:

        if ((!XFS_IS_UQUOTA_ENFORCED(mp) &&
             dqp->q_core.d_flags == XFS_DQ_USER) ||
            (!XFS_IS_PQUOTA_ENFORCED(mp) &&
             dqp->q_core.d_flags == XFS_DQ_PROJ) ||
            (!XFS_IS_GQUOTA_ENFORCED(mp) &&
             dqp->q_core.d_flags == XFS_DQ_GROUP)) {

Otherwise it looks good. Fix the above and you can add a

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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