xfs
[Top] [All Lists]

Re: [RFC v5 PATCH 3/4] xfs: Add pquotaino to on-disk super block

To: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Subject: Re: [RFC v5 PATCH 3/4] xfs: Add pquotaino to on-disk super block
From: Ben Myers <bpm@xxxxxxx>
Date: Thu, 3 May 2012 12:16:09 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120314202641.17044.32835.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <20120314202623.17044.90045.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20120314202641.17044.32835.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
Chandra,

On Wed, Mar 14, 2012 at 03:26:42PM -0500, Chandra Seetharaman wrote:
> >From 9d32afb9cc284b2511951c45b47f6a73dfd0a7e1 Mon Sep 17 00:00:00 2001
> From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
> Date: Wed, 14 Mar 2012 14:26:20 -0500
> Subject: [PATCH 3/4] Add a new field to the superblock to add support for 
> seperate pquota
>  with a specific version.
> 
> No change made yet to gather both project and group quota with quotactl.

This note that you're going to do in the next patch is useful, but please also
put a description of what you're doing in this patch.  ;)

> Signed-off-by: Chandra Seetharaman <sekharan@xxxxxxxxxx>

...

> @@ -1055,19 +1057,22 @@ xfs_qm_qino_alloc(
>               ASSERT((sbfields & (XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO |
>                                  XFS_SB_GQUOTINO | XFS_SB_QFLAGS)) ==
>                      (XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO |
> -                     XFS_SB_GQUOTINO | XFS_SB_QFLAGS));
> +                     XFS_SB_GQUOTINO | XFS_SB_PQUOTINO | XFS_SB_QFLAGS));
>  
>               xfs_sb_version_addquota(&mp->m_sb);
>               mp->m_sb.sb_uquotino = NULLFSINO;
>               mp->m_sb.sb_gquotino = NULLFSINO;
> +             mp->m_sb.sb_pquotino = NULLFSINO;
>  
>               /* qflags will get updated _after_ quotacheck */
>               mp->m_sb.sb_qflags = 0;
>       }
>       if (flags & XFS_QMOPT_UQUOTA)
>               mp->m_sb.sb_uquotino = (*ip)->i_ino;
> -     else
> +     else if (flags & XFS_QMOPT_GQUOTA)
>               mp->m_sb.sb_gquotino = (*ip)->i_ino;
> +     else

Maybe?
             {
                ASSERT(flags & XFS_QMOPT_PQUOTA);
> +             mp->m_sb.sb_pquotino = (*ip)->i_ino;
        }
...

> @@ -439,10 +441,13 @@ xfs_qm_scall_getqstat(
>       out->qs_pad = 0;
>       out->qs_uquota.qfs_ino = mp->m_sb.sb_uquotino;
>       out->qs_gquota.qfs_ino = mp->m_sb.sb_gquotino;
> +     if (&out->qs_gquota != &out->qs_pquota)
> +             out->qs_pquota.qfs_ino = mp->m_sb.sb_pquotino;

I was a little surprised not to get a warning for that test.  

...

> @@ -1317,6 +1311,15 @@ xfs_fs_fill_super(
>       if (error)
>               goto out_destroy_counters;
>  
> +     if ((mp->m_qflags & (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE)) &&
> +         (mp->m_qflags & (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE)) &&
> +         !xfs_sb_version_has_no_oquota(&mp->m_sb)) {
> +             xfs_warn(mp, "Super block does not support "
> +                              "project and group quota together");
> +             error = EINVAL;
> +             goto out_free_sb;
> +     }
> +

You'd see this warning if you have an old superblock with the new flags.  Maybe
it is worth writing a test for that.

...

> diff --git a/include/linux/dqblk_xfs.h b/include/linux/dqblk_xfs.h
> index 8655280..f17e3bb 100644
> --- a/include/linux/dqblk_xfs.h
> +++ b/include/linux/dqblk_xfs.h
> @@ -155,6 +155,7 @@ typedef struct fs_quota_stat {
>       __s8            qs_pad;         /* unused */
>       fs_qfilestat_t  qs_uquota;      /* user quota storage information */
>       fs_qfilestat_t  qs_gquota;      /* group quota storage information */
> +#define qs_pquota    qs_gquota

Cool.  This #define goes away in the next patch.

I've been over this and it is looking pretty good to me...  I'm thinking about
what basic xfstests would be good to have for this feature.

Old sb_version with new flags, 
new sb_version with old flags, etc.

I'll revisit that shortly.

-Ben

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