Re: [PATCH v9 4/6] xfs: Start using pquotaino from the superblock.

[Dave Chinner <david@fromorbit.com>]

On Sun, Jun 23, 2013 at 09:48:25PM -0500, Chandra Seetharaman wrote:
> Start using pquotino and define a macro to check if the
> superblock has pquotino.
> 
> Keep backward compatibilty by alowing mount of older superblock
> with no separate pquota inode.
> 
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> ---
>  fs/xfs/xfs_fsops.c             |    3 +-
>  fs/xfs/xfs_mount.c             |   51 +++++++++++++++++++++++++++++++--------
>  fs/xfs/xfs_qm.c                |   22 +++++++++--------
>  fs/xfs/xfs_qm_syscalls.c       |   24 ++++++++++++++++--
>  fs/xfs/xfs_quota.h             |    8 ------
>  fs/xfs/xfs_sb.h                |   16 +++++++++++-
>  fs/xfs/xfs_super.c             |   14 ++++++----
>  include/uapi/linux/dqblk_xfs.h |    1 +
>  8 files changed, 99 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 614eb0c..3bf05f4 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -501,7 +501,8 @@ xfs_growfs_data_private(
>  				error, agno);
>  			break;
>  		}
> -		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb, XFS_SB_ALL_BITS);
> +		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb,
> +				xfs_sb_all_bits(&mp->m_sb));

I think you could still pass XFS_SB_ALL_BITS to xfs_sb_to_disk(),
and do the XFS_SB_PQUOTINO filtering inside xfs_sb_quota_to_disk().

Actually xfs_sb_all_bits() is only used here, and it seems to me
that it is not necessary as we do a pquota support check inside
xfs_sb_to_disk() and can handle this in that place...

>  
>  		/*
>  		 * If we get an error writing out the alternate superblocks,
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index e2e14cb..bb7b23e 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -336,12 +336,17 @@ xfs_mount_validate_sb(
>  		return XFS_ERROR(EWRONGFS);
>  	}
>  
> -	if ((sbp->sb_qflags & (XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD)) &&
> -			(sbp->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD |
> -				XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD))) {
> -		xfs_notice(mp,
> -"Super block has XFS_OQUOTA bits along with XFS_PQUOTA and/or XFS_GQUOTA bits.\n");
> -		return XFS_ERROR(EFSCORRUPTED);
> +	if (xfs_sb_version_has_pquota(sbp)) {
> +		if (sbp->sb_qflags & (XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD)) {
> +			xfs_notice(mp,
> +			   "Version 5 of Super block has XFS_OQUOTA bits.\n");
> +			return XFS_ERROR(EFSCORRUPTED);
> +		}
> +	} else if (sbp->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD |
> +				XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD)) {
> +			xfs_notice(mp,
> +"Superblock earlier than Version 5 has XFS_[PQ]UOTA_{ENFD|CHKD} bits.\n");
> +			return XFS_ERROR(EFSCORRUPTED);

[ xfs_alert() for those, I think. ]

> @@ -638,6 +643,13 @@ xfs_handle_quota_to_disk(
>  {
>  	__uint16_t	qflags = from->sb_qflags;
>  
> +	/*
> +	 * We need to do these manipilations only if we are working
> +	 * with an older version of on-disk superblock.
> +	 */
> +	if (xfs_sb_version_has_pquota(from))
> +		return;
> +
>  	if (*fields & XFS_SB_QFLAGS) {
>  		/*
>  		 * The in-core version of sb_qflags do not have

xfs_sb_all_bits() does:

	if (xfs_sb_version_has_pquota(sbp))
		return XFS_SB_ALL_BITS;
	return XFS_SB_ALL_BITS & ~XFS_SB_PQUOTINO;

which means that we enter xfs_handle_quota_to_disk() with
XFS_SB_ALL_BITS intact for the pquota case, but without it in the
non-pquota case. Hence if we don't have pquota set, we continue
onwards and....

> @@ -657,6 +669,10 @@ xfs_handle_quota_to_disk(
>  		to->sb_qflags = cpu_to_be16(qflags);
>  		*fields &= ~XFS_SB_QFLAGS;
>  	}
> +	if (*fields & XFS_SB_PQUOTINO) {
> +		to->sb_gquotino = cpu_to_be64(from->sb_pquotino);
> +		*fields &= ~XFS_SB_PQUOTINO;
> +	}

We will never do this because we've already cleared XFS_SB_PQUOTINO
before we entered xfs_sb_to_disk(). That doesn't seem right to me... 

> @@ -1524,8 +1524,10 @@ xfs_qm_init_quotainos(
>  		flags &= ~XFS_QMOPT_SBVERSION;
>  	}
>  	if (XFS_IS_PQUOTA_ON(mp) && pip == NULL) {
> +		if (!xfs_sb_version_has_pquota(&mp->m_sb))
> +			sbflags &= ~XFS_SB_GQUOTINO;

That's taken me a while to work out - it needs a comment explaining
why XFS_SB_GQUOTINO is being cleared here.

>  		error = xfs_qm_qino_alloc(mp, &pip,
> -					  sbflags | XFS_SB_GQUOTINO,
> +					  sbflags | XFS_SB_PQUOTINO,
>  					  flags | XFS_QMOPT_PQUOTA);
>  		if (error)
>  			goto error_rele;
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index ed7cd55..d664a2d 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -201,8 +201,7 @@ xfs_qm_scall_quotaoff(
>  	/*
>  	 * If quotas is completely disabled, close shop.
>  	 */
> -	if (((flags & XFS_MOUNT_QUOTA_ALL) == XFS_MOUNT_QUOTA_SET1) ||
> -	    ((flags & XFS_MOUNT_QUOTA_ALL) == XFS_MOUNT_QUOTA_SET2)) {
> +	if ((flags & XFS_MOUNT_QUOTA_ALL) == XFS_MOUNT_QUOTA_ALL) {
>  		mutex_unlock(&q->qi_quotaofflock);
>  		xfs_qm_destroy_quotainfo(mp);
>  		return (0);

This makes the assumption that userspace passes all three quota
types into the kernel in a single quota off call as the only way to
turn quotas off completely. What happens if they are turned off one
at a time? Shouldn't we detect when no more quotas are actually
enabled and then do this?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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