On Fri, 2013-05-17 at 12:55 +1000, Dave Chinner wrote:
> On Fri, May 10, 2013 at 04:21:25PM -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 and
> > PQUOTA respectively.
> >
> > 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*.
> >
> > Signed-off-by: Chandra Seetharaman <sekharan@xxxxxxxxxx>
> > Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_mount.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > fs/xfs/xfs_qm.c | 9 ++++++---
> > fs/xfs/xfs_qm_syscalls.c | 39 +++++++++++++++++++++------------------
> > fs/xfs/xfs_quota.h | 42 ++++++++++++++++++++++++++++--------------
> > fs/xfs/xfs_quotaops.c | 6 ++++--
> > fs/xfs/xfs_super.c | 16 ++++++++--------
> > fs/xfs/xfs_trans_dquot.c | 4 ++--
> > 7 files changed, 110 insertions(+), 47 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index f6bfbd7..1b79906 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -564,6 +564,8 @@ xfs_sb_from_disk(
> > struct xfs_sb *to,
> > xfs_dsb_t *from)
> > {
> > + bool force_quota_check = false;
> > +
> > to->sb_magicnum = be32_to_cpu(from->sb_magicnum);
> > to->sb_blocksize = be32_to_cpu(from->sb_blocksize);
> > to->sb_dblocks = be64_to_cpu(from->sb_dblocks);
> > @@ -599,6 +601,21 @@ xfs_sb_from_disk(
> > to->sb_uquotino = be64_to_cpu(from->sb_uquotino);
> > to->sb_gquotino = be64_to_cpu(from->sb_gquotino);
> > to->sb_qflags = be16_to_cpu(from->sb_qflags);
> > + if ((to->sb_qflags & (XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD)) &&
> > + (to->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD |
> > + XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD))) {
> > + xfs_notice(NULL, "Super block has XFS_OQUOTA bits along with "
> > + "XFS_PQUOTA and/or XFS_GQUOTA bits. Quota check forced.\n");
> > + force_quota_check = true;
> > + }
>
> We can't do these quota check manipulations to the superblock quota
> flags during disk->incore translation anymore - there is more than
> one place that converts the superblock from on-disk to incore format
> (e.g. the verification callbacks) and so emitting this message when
> we are *writing* the superblock is not good.
>
> Also, if the XFS_PQUOTA_CHKD/XFS_PQUOTA_ENFD flags are set and
> the superblock is not a V5 superblock, then I think we should refuse
> to mount the filesystem because that is an invalid state - those
> flags should only be set now on a V5 superblock, and never at any
> other time. Hence this specific check should probably be moved to
> xfs_mount_validate_sb() and cause an EFSCORRUPTED return if it
> fails. That will catch something setting the flags incorrectly (i.e.
> at superblock write) as well as prevent mounting in this situation.
>
> I know, this is different to what I've said in the past, but the
> on-disk format checking is now a lot stricter and so I think that if
> the filesystem is in some kind of wierd state we should just throw
> an error and let the administrator work out how this problem
> happened and how to resolve it.
>
will do.
> > + 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;
> > + to->sb_qflags &= ~(XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD);
> > +
>
> This translation of the quota flags needs to be external to
> this function, and then only called in the mount path where we are
> initially setting up the in-core superblock (i.e. in xfs_readsb())
> because that is the only place where the incore values are
> permanently stored.
>
> > @@ -636,11 +656,30 @@ xfs_sb_to_disk(
> > xfs_sb_field_t f;
> > int first;
> > int size;
> > + __uint16_t qflags = from->sb_qflags;
> >
> > ASSERT(fields);
> > if (!fields)
> > return;
> >
> > + if (fields & XFS_SB_QFLAGS) {
> > + /*
> > + * The in-core version of sb_qflags do not have
> > + * XFS_OQUOTA_* flags, whereas the on-disk version
> > + * does. So, convert incore XFS_{PG}QUOTA_* flags
> > + * to on-disk XFS_OQUOTA_* flags.
> > + */
> > + qflags &= ~(XFS_PQUOTA_ENFD | XFS_PQUOTA_CHKD |
> > + XFS_GQUOTA_ENFD | XFS_GQUOTA_CHKD);
> > +
> > + if (from->sb_qflags &
> > + (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD))
> > + qflags |= XFS_OQUOTA_ENFD;
> > + if (from->sb_qflags &
> > + (XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD))
> > + qflags |= XFS_OQUOTA_CHKD;
> > + }
>
> Given that we have a new superblock version, we can write the new
> XFS_[PG]QUOTA_CHKD/XFS_[PG]QUOTA_ENFD flags directly into the
> sb_qflags knowing that we can't get an older kernel to interpret
> these new fields because they'll fail the superblck version test. So
> that would mean we only need to do this translation for filesystems
> for non-v5 superblock filesystems.
>
> Ah, I see that in a later patch you introduce
> xfs_sb_version_has_pquota() and modify this code path to do
> something similar, and it becomes rather complex and nasty.
>
> OK, I know this is a bit of work, but can you introduce
> xfs_sb_version_has_pquota() in this patch (as we already have the
> pquota inode defined in the on-disk format) as:
>
> static inline int xfs_sb_version_haspquota(xfs_sb_t *sbp)
> {
> return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> }
>
> and make these OQUOTA flag translations dependent on this not being
> set right from the start?
I am starting to use pquotino only in patch 3, and hence introduced the
above helper in that patch. IMO, it would be helpful w.r.t readability
and patch coherency, if I leave the introduction in patch 3.
>
> Also, given how nasty this manipulation ends up, can you push it out
> into a function that is called from xfs_sb_to_disk(). That way
> xfs_sb_to_disk() doesn't end up mostly being 70% quota field
> manipulation code...
I will talk about this in patch 3.
>
>
> > /*
> > + * Start differentiating group quota and project quota in-core
> > + * using distinct flags, instead of using the combined OQUOTA flags.
> > + *
> > + * Conversion to and from the combined OQUOTA flag (if necessary)
> > + * is done only in xfs_sb_{to,from}_disk()
> > + */
> > +#define XFS_GQUOTA_ENFD 0x0080 /* group quota limits enforced */
> > +#define XFS_GQUOTA_CHKD 0x0100 /* quotacheck run on group quotas */
> > +#define XFS_PQUOTA_ENFD 0x0200 /* project quota limits enforced */
> > +#define XFS_PQUOTA_CHKD 0x0400 /* quotacheck run on project quotas */
>
> These become new on-disk fields for xfs_sb_version_haspquota()
> filesystems, so they are not specifically in-core values.
will fix.
>
> Other than these to/from disk changes, the patch looks fine.
>
> Cheers,
>
> Dave.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
