Re: [PATCH] Extend project quotas to support 32bit project identificators. [version 3]

[Dave Chinner <david@xxxxxxxxxxxxx>]

On Sat, Aug 28, 2010 at 12:39:58AM +0200, Arkadiusz Miśkiewicz wrote:
> This patch adds support for 32bit project quota identificators.
> 
> On disk format is backward compatible with 16bit projid numbers. projid
> on disk is now keept in two 16bit values - di_projid_lo (which holds the
> same position as old 16bit projid value) and new di_projid_hi (takes
> existing padding) and convertes from/to 32bit value on the fly.
> 
> PROJID32BIT feature2 flag is set automaticly when trying to use 32bit
> quota project identificator.
> 
> Signed-off-by: Arkadiusz Miśkiewicz <arekm@xxxxxxxx>
.....
> @@ -946,13 +946,30 @@ xfs_ioctl_setattr(
>               goto error_return;
>       }
>  
> -     /*
> -      * Do a quota reservation only if projid is actually going to change.
> -      */
>       if (mask & FSX_PROJID) {
> +             /*
> +              * Switch on the PROJID32BIT superblock bit when needed
> +              * (implies also FEATURES2)
> +              */
> +             if (!xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb) &&
> +                             fa->fsx_projid > (__uint16_t)-1) {
> +                     spin_lock(&ip->i_mount->m_sb_lock);
> +                     if (!xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb)) 
> {
> +                             
> xfs_sb_version_addprojid32bit(&ip->i_mount->m_sb);
> +                             spin_unlock(&ip->i_mount->m_sb_lock);
> +                             xfs_mod_sb(tp,
> +                                     XFS_SB_VERSIONNUM | XFS_SB_FEATURES2);
> +                     } else
> +                             spin_unlock(&ip->i_mount->m_sb_lock);
> +             }

Can you separate out the superblock feature bit modification into a
helper function?

> @@ -335,6 +336,23 @@ xfs_iflags_test_and_clear(xfs_inode_t *ip, unsigned 
> short flags)
>  }
>  
>  /*
> + * Project quota id helpers
> + */
> +static inline prid_t
> +xfs_get_projid(xfs_inode_t *ip)
> +{
> +     return (prid_t)(ip->i_d.di_projid_hi) << 16 | ip->i_d.di_projid_lo;
> +}

I think the set of parenthesis should be separating to the two
halveѕ of value clearly - the parenthesis around
ip->i_d.di_projid_hi are effectively meaningless. i.e:

        return ((prid_t)ip->i_d.di_projid_hi << 16) | ip->i_d.di_projid_lo;

> +static inline int xfs_sb_version_hasprojid32bit(xfs_sb_t *sbp)
> +{
> +     return xfs_sb_version_hasmorebits(sbp) &&
> +             (sbp->sb_features2 & XFS_SB_VERSION2_PROJID32BIT);
> +}
> +
> +static inline void xfs_sb_version_addprojid32bit(xfs_sb_t *sbp)
> +{
> +     sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
> +     sbp->sb_features2 |= XFS_SB_VERSION2_PROJID32BIT;
> +}

This also needs to set the bit in the sbp->sb_bad_features2 field.

Otherwise it looks ok. I'll throw it in my QA stack and see how it
goes.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx