Thanks for the review...
Comments inline below...
On Wed, 2012-08-15 at 12:09 +1000, Dave Chinner wrote:
> On Fri, Jul 20, 2012 at 06:02:41PM -0500, Chandra Seetharaman wrote:
> > Add a new field to the superblock to add support for seperate pquota
> > with a specific version.
> >
> > Define a macro to differentiate superblock with and without OQUOTA.
> >
> > Keep backward compatibilty by alowing mount of older superblock
> > with no separate pquota inode.
> >
> > Signed-off-by: Chandra Seetharaman <sekharan@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_itable.c | 3 +-
> > fs/xfs/xfs_mount.c | 58 ++++++++++++++++++++++++++++++++++++++++++++-
> > fs/xfs/xfs_qm.c | 18 +++++++++----
> > fs/xfs/xfs_qm_syscalls.c | 30 +++++++++++++++++-----
> > fs/xfs/xfs_quota.h | 8 ------
> > fs/xfs/xfs_sb.h | 20 ++++++++++++---
> > fs/xfs/xfs_super.c | 15 +++++++----
> > fs/xfs/xfs_trans_dquot.c | 4 ++-
> > include/linux/dqblk_xfs.h | 1 +
> > 9 files changed, 123 insertions(+), 34 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> > index eff577a..0fa60b4 100644
> > --- a/fs/xfs/xfs_itable.c
> > +++ b/fs/xfs/xfs_itable.c
> > @@ -42,7 +42,8 @@ xfs_internal_inum(
> > {
> > return (ino == mp->m_sb.sb_rbmino || ino == mp->m_sb.sb_rsumino ||
> > (xfs_sb_version_hasquota(&mp->m_sb) &&
> > - (ino == mp->m_sb.sb_uquotino || ino == mp->m_sb.sb_gquotino)));
> > + (ino == mp->m_sb.sb_uquotino || ino == mp->m_sb.sb_gquotino ||
> > + ino == mp->m_sb.sb_pquotino)));
> > }
> >
> > /*
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index 22f23fd..992ec29 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -108,6 +108,7 @@ static const struct {
> > { offsetof(xfs_sb_t, sb_logsunit), 0 },
> > { offsetof(xfs_sb_t, sb_features2), 0 },
> > { offsetof(xfs_sb_t, sb_bad_features2), 0 },
> > + { offsetof(xfs_sb_t, sb_pquotino), 0 },
>
> Can you line up the "0 }," part with the rest of the structure?
>
> > { sizeof(xfs_sb_t), 0 }
> > };
> >
> > @@ -618,6 +619,35 @@ xfs_sb_from_disk(
> > to->sb_logsunit = be32_to_cpu(from->sb_logsunit);
> > to->sb_features2 = be32_to_cpu(from->sb_features2);
> > to->sb_bad_features2 = be32_to_cpu(from->sb_bad_features2);
> > +
> > + if (xfs_sb_version_has_no_oquota(to)) {
>
> <crash>
>
> My brain just core dumped on that name - "has no old quota"? "old
> quota" could mean anything, and it doesn't describe the change in
> on-disk format being made.
>
> The feature being added is a "separate project quota inode", so the
> feature bit and related functions need to reflect that. Hence I
> think a better name is something like "has project quota inode" i.e
> XFS_SB_VERSION2_PQUOTINO, xfs_sb_version_has_pquotino() and so on,
> which matches the above addition to the superblock fields...
will change it to refer pquotino
>
> > + if (to->sb_qflags & (XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD)) {
> > + xfs_notice(mp, "Super block has XFS_OQUOTA bits with "
> > + "version NO_OQUOTA. Fixing it.\n");
> > + to->sb_qflags &= ~(XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD);
> > + }
> > + to->sb_pquotino = be64_to_cpu(from->sb_pquotino);
>
> So we have a feature bit set, but old quota bits set. How can that
> happen?
>
> If it does occur, doesn't that mean we cannot trust the group or
> project quotas to be correct, so at minimum this case needs to
> trigger a quotacheck for both group and project quotas?
Sure, will do a quotacheck here. I just call xfs_qm_quotacheck() and
fail if it fails ?
>
> And if we can't enumerate how it can happen, then should we even
> allow it to be "fixed" automatically?
>
> > + } else {
> > + if (to->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD |
> > + XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD)) {
> > + xfs_notice(mp, "Super block has XFS_[G|P]UOTA bits in "
> > + "older version. Fixing it.\n");
> > + to->sb_qflags &= ~(XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD |
> > + XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD);
>
> Again, how can that happen, and why isn't it a sign of corruption
> that requires external intervention? If those bits are set, we can't
> trust the quota to be correct, so at minimum we need to force a
> quota check.
same as above.
>
> > + }
> > + if (to->sb_qflags & XFS_OQUOTA_ENFD)
> > + to->sb_qflags |= (to->sb_qflags & XFS_PQUOTA_ACCT) ?
> > + XFS_PQUOTA_ENFD : XFS_GQUOTA_ENFD;
> > + if (to->sb_qflags & XFS_OQUOTA_CHKD)
> > + to->sb_qflags |= (to->sb_qflags & XFS_PQUOTA_ACCT) ?
> > + XFS_PQUOTA_CHKD : XFS_GQUOTA_CHKD;
>
> what do you do if both XFS_PQUOTA_ACCT and XFS_GQUOTA_ACCT are set?
> i.e. both quotas were active even though the feature bit wasn't set?
I will do a check on both flags being set and do a quotacheck ?
>
> This "fix it automatically" issue seems to be problematic to me
> because it isn't clear whether we can get into these states, or if
> we do, what the correct resolution of the problem is....
>
>
> > + to->sb_qflags &= ~(XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD);
> > +
> > + if (to->sb_qflags & XFS_PQUOTA_ACCT) {
> > + to->sb_pquotino = to->sb_gquotino;
> > + to->sb_gquotino = NULLFSINO;
> > + }
>
> This needs a comment explaining that the in-memory code will use the
> sb_pquotino for project quotas (i.e. behave as though the feature
> bit is always set), so we ahve to set up the in-memory image that
> way and convert it back to the old format when writing out the
> superblock.
will do.
>
> > + }
> > }
> >
> > /*
> > @@ -637,11 +667,22 @@ xfs_sb_to_disk(
> > int first;
> > int size;
> > __uint16_t tmp16;
> > + xfs_ino_t gquotino;
> >
> > ASSERT(fields);
> > if (!fields)
> > return;
> >
> > + /*
> > + * On-disk version earlier than NO_OQUOTA doesn't have sb_pquotino.
> > + * so, we need to copy the value to gquotino field.
> > + */
> > + if (!xfs_sb_version_has_no_oquota(from) &&
>
> <boom>
>
> Double negative! (not has no old quota).
>
> Another reason for naming it PQUOTINO....
>
> > + (from->sb_qflags & (XFS_PQUOTA_ENFD | XFS_PQUOTA_CHKD)))
> > + gquotino = from->sb_pquotino;
> > + else
> > + gquotino = from->sb_gquotino;
>
> Also, only do this transformation if we are modifying the group
> quota inode.....
>
will move to code to below.... (1)
> > +
> > while (fields) {
> > f = (xfs_sb_field_t)xfs_lowbit64((__uint64_t)fields);
> > first = xfs_sb_info[f].offset;
> > @@ -651,7 +692,8 @@ 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) {
> > + } else if ((f == XFS_SBS_QFLAGS) &&
>
> [ no need for the extra (). ]
>
> > + !xfs_sb_version_has_no_oquota(from)) {
> > /*
> > * The in-core version of sb_qflags do not have
> > * XFS_OQUOTA_* flags, whereas the on-disk version
> > @@ -671,6 +713,8 @@ xfs_sb_to_disk(
> > tmp16 |= XFS_OQUOTA_CHKD;
> >
> > *(__be16 *)(to_ptr + first) = cpu_to_be16(tmp16);
> > + } else if (f == XFS_SBS_GQUOTINO) {
> > + *(__be64 *)(to_ptr + first) = cpu_to_be64(gquotino);
>
> .... i.e. here.
(1) here, as suggested.
>
> > } else {
> > switch (size) {
> > case 2:
> > @@ -759,6 +803,12 @@ reread:
> > goto reread;
> > }
> >
> > + if (!xfs_sb_version_has_no_oquota(&mp->m_sb) &&
> > + XFS_IS_PQUOTA_ON(mp)) {
> > + mp->m_sb.sb_pquotino = mp->m_sb.sb_gquotino;
> > + mp->m_sb.sb_gquotino = NULLFSINO;
> > + }
>
> why this is necessary? Didn't the earlier xfs_sb_from_disk() call
> deal with this?
The call in xfs_sb_from_disk() only sets if the superblock has pquota
already.
This sets up the fields when superblock didn't have it, but the user
specified pquota as a mount option.
>
> > +
> > /* Initialize per-cpu counters */
> > xfs_icsb_reinit_counters(mp);
> >
> > @@ -1646,6 +1696,12 @@ xfs_mod_sb(xfs_trans_t *tp, __int64_t fields)
> > first = sizeof(xfs_sb_t);
> > last = 0;
> >
> > + if (!xfs_sb_version_has_no_oquota(&mp->m_sb) &&
> > + XFS_IS_PQUOTA_ON(mp)) {
> > + fields &= (__int64_t)~XFS_SB_PQUOTINO;
> > + fields |= (__int64_t)XFS_SB_GQUOTINO;
> > + }
>
> This will set the XFS_SB_GQUOTINO unconditionally. It only needs to
> be set this if the XFS_SB_PQUOTINO field is set.
will fix it.
>
> > @@ -838,19 +840,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));
>
> Did you test this with CONFIG_XFS_DEBUG=y? It will always fail
> because you didn't add XFS_SB_PQUOTINO to the sbfields mask....
In my box, I always had problems with DEBUG :(... So, I stopped testing
with it.
will fix it.
>
> > @@ -1156,7 +1161,8 @@ xfs_qm_dqusage_adjust(
> > * rootino must have its resources accounted for, not so with the quota
> > * inodes.
> > */
> > - if (ino == mp->m_sb.sb_uquotino || ino == mp->m_sb.sb_gquotino) {
> > + if (ino == mp->m_sb.sb_uquotino || ino == mp->m_sb.sb_gquotino ||
> > + ino == mp->m_sb.sb_pquotino) {
>
> if (ino == mp->m_sb.sb_uquotino ||
> ino == mp->m_sb.sb_gquotino ||
> ino == mp->m_sb.sb_pquotino) {
>
> > @@ -413,17 +414,18 @@ xfs_qm_scall_getqstat(
> > struct fs_quota_stat *out)
> > {
> > struct xfs_quotainfo *q = mp->m_quotainfo;
> > - struct xfs_inode *uip, *gip;
> > - boolean_t tempuqip, tempgqip;
> > + struct xfs_inode *uip, *gip, *pip;
> > + boolean_t tempuqip, tempgqip, temppqip;
> >
> > - uip = gip = NULL;
> > - tempuqip = tempgqip = B_FALSE;
> > + uip = gip = pip = NULL;
> > + tempuqip = tempgqip = temppqip = B_FALSE;
>
> Line per declaration and initialisation.
>
> > diff --git a/fs/xfs/xfs_sb.h b/fs/xfs/xfs_sb.h
> > index 8fd7894..7373108 100644
> > --- a/fs/xfs/xfs_sb.h
> > +++ b/fs/xfs/xfs_sb.h
> > @@ -81,11 +81,15 @@ struct xfs_mount;
> > #define XFS_SB_VERSION2_ATTR2BIT 0x00000008 /* Inline attr rework */
> > #define XFS_SB_VERSION2_PARENTBIT 0x00000010 /* parent pointers */
> > #define XFS_SB_VERSION2_PROJID32BIT 0x00000080 /* 32 bit project id */
> > +#define XFS_SB_VERSION2_NO_OQUOTA 0x00000100 /* No OQUOTA and *
> > + * separate project *
> > + * quota field */
>
> see my comments about naming this above.
>
> > @@ -250,7 +255,7 @@ typedef enum {
> > XFS_SBS_GQUOTINO, XFS_SBS_QFLAGS, XFS_SBS_FLAGS, XFS_SBS_SHARED_VN,
> > XFS_SBS_INOALIGNMT, XFS_SBS_UNIT, XFS_SBS_WIDTH, XFS_SBS_DIRBLKLOG,
> > XFS_SBS_LOGSECTLOG, XFS_SBS_LOGSECTSIZE, XFS_SBS_LOGSUNIT,
> > - XFS_SBS_FEATURES2, XFS_SBS_BAD_FEATURES2,
> > + XFS_SBS_FEATURES2, XFS_SBS_BAD_FEATURES2, XFS_SBS_PQUOTINO,
>
> You got the name right here ;)
>
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 595f5ac..b475057 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -399,12 +399,6 @@ xfs_parseargs(
> > }
> > #endif
> >
> > - if ((mp->m_qflags & (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE)) &&
> > - (mp->m_qflags & (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE))) {
> > - xfs_warn(mp, "cannot mount with both project and group quota");
> > - return EINVAL;
> > - }
> > -
> > if ((dsunit && !dswidth) || (!dsunit && dswidth)) {
> > xfs_warn(mp, "sunit and swidth must be specified together");
> > return EINVAL;
> > @@ -1330,6 +1324,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;
> > + }
>
> This check belongs in xfs_finish_flags().
will move it.
>
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
