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

[Dave Chinner <david@fromorbit.com>]

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@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs