xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [RFC v6 PATCH 1/5] xfs: Remove incore use of XFS_OQUOTA_ENFD and XFS_OQUOTA_CHKD
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Wed, 22 Aug 2012 17:53:22 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120814224645.GQ2877@dastard>
Organization: IBM
References: <20120720230202.20477.69766.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20120720230208.20477.49663.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20120814224645.GQ2877@dastard>
Reply-to: sekharan@xxxxxxxxxx
will fix them as suggested.

Thanks for the review.

Chandra
On Wed, 2012-08-15 at 08:46 +1000, Dave Chinner wrote:
> 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>
> 


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