From: Dave Chinner <david@fromorbit.com>
To: Chandra Seetharaman <sekharan@us.ibm.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH v8 3/5] xfs: Start using pquotaino from the superblock
Date: Fri, 17 May 2013 14:46:04 +1000 [thread overview]
Message-ID: <20130517044604.GO24635@dastard> (raw)
In-Reply-To: <1368220889-25188-4-git-send-email-sekharan@us.ibm.com>
On Fri, May 10, 2013 at 04:21:27PM -0500, Chandra Seetharaman wrote:
> Define a macro to check if the superblock has pquotino.
>
> Keep backward compatibilty by alowing mount of older superblock
> with no separate pquota inode.
....
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 1b79906..233f88e 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -601,21 +601,6 @@ 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;
> - }
> - 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);
> -
> to->sb_flags = from->sb_flags;
> to->sb_shared_vn = from->sb_shared_vn;
> to->sb_inoalignmt = be32_to_cpu(from->sb_inoalignmt);
> @@ -636,6 +621,44 @@ xfs_sb_from_disk(
> to->sb_pquotino = be64_to_cpu(from->sb_pquotino);
> to->sb_lsn = be64_to_cpu(from->sb_lsn);
>
> + if (xfs_sb_version_has_pquota(to)) {
> + if (to->sb_qflags & (XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD)) {
> + xfs_notice(NULL, "Super block has XFS_OQUOTA bits "
> + "with version PQUOTINO. Quota check forced.\n");
> + to->sb_qflags &= ~(XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD);
> + force_quota_check = true;
> + }
> + to->sb_pquotino = be64_to_cpu(from->sb_pquotino);
> + } else {
> + if (to->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD |
> + XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD)) {
> + xfs_notice(NULL, "Super block has XFS_[G|P]UOTA "
> + "bits in version older than PQUOTINO. "
> + "Quota check forced.\n");
> +
> + to->sb_qflags &= ~(XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD |
> + XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD);
> + force_quota_check = true;
> + }
> +
> + /*
> + * On disk superblock qflags uses XFS_OQUOTA.* to support
> + * either PQUOTA or GQUOTA. But, in memory qflags uses
> + * XFS_PQUOTA.* or XFS_GQUOTA.* depending on which quota
> + * is used.
> + * Following block translates XFS_OQUOTA.* to either
> + * GQUOTA or PQUOTA.
> + */
> + 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);
> +
> + }
> +
> if (force_quota_check)
> to->sb_qflags &= ~(XFS_GQUOTA_CHKD | XFS_PQUOTA_CHKD);
> }
This all gets restructured here - as per my previous comments, this
probably should all be in xfs_readsb(), not here.
> @@ -657,27 +680,51 @@ xfs_sb_to_disk(
> int first;
> int size;
> __uint16_t qflags = from->sb_qflags;
> + xfs_ino_t gquotino = from->sb_gquotino;
>
> ASSERT(fields);
> if (!fields)
> return;
>
> - if (fields & XFS_SB_QFLAGS) {
> + if (!xfs_sb_version_has_pquota(from)) {
This should be moved to a separate function, I think.
> + 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;
Now that we have the new flags set correct, write it directly to
to->sb_qflags and clear XFS_SB_QFLAGS from the fields varaible.
> + }
> +
> /*
> - * 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.
> + * On-disk version earlier than pquota doesn't have
> + * sb_pquotino. so, we need to copy the value to gquotino.
> */
> - qflags &= ~(XFS_PQUOTA_ENFD | XFS_PQUOTA_CHKD |
> - XFS_GQUOTA_ENFD | XFS_GQUOTA_CHKD);
> + if (fields & XFS_SB_PQUOTINO) {
> + fields &= (__int64_t)~XFS_SB_PQUOTINO;
> + fields |= (__int64_t)XFS_SB_GQUOTINO;
> + gquotino = from->sb_pquotino;
> + }
Same here - write the inode number directly to the to->sb_gquotino
field and clear the XFS_SB_PQUOTINO flag.
>
> - 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;
> + /* If any quota inodes are written, write all quota inodes */
> + if (fields & (XFS_SB_UQUOTINO|XFS_SB_GQUOTINO))
> + fields |= (XFS_SB_UQUOTINO|XFS_SB_GQUOTINO);
Why write things that haven't been modified?
> +
> + } else {
> + /* If any quota inodes are written, write all quota inodes */
> + if (fields & (XFS_SB_UQUOTINO | XFS_SB_GQUOTINO
> + | XFS_SB_PQUOTINO))
> + fields |= (XFS_SB_UQUOTINO | XFS_SB_GQUOTINO
> + | XFS_SB_PQUOTINO);
Same - the flags pssed in are supposed to document everything that
has been modified. If the flags passed in are wrong, then fix the
bad callers.....
> }
>
> while (fields) {
> @@ -691,6 +738,8 @@ xfs_sb_to_disk(
> memcpy(to_ptr + first, from_ptr + first, size);
> } else if (f == XFS_SBS_QFLAGS) {
> *(__be16 *)(to_ptr + first) = cpu_to_be16(qflags);
> + } else if (f == XFS_SBS_GQUOTINO) {
> + *(__be64 *)(to_ptr + first) = cpu_to_be64(gquotino);
If we clear the XFS_SBS_GQUOTINO/XFS_SBS_QFLAGS flags like I
suggested above, this grot can go away and the loop remain
completely generic.
> } else {
> switch (size) {
> case 2:
> @@ -872,6 +921,18 @@ reread:
> */
> xfs_sb_from_disk(&mp->m_sb, XFS_BUF_TO_SBP(bp));
>
> + if (!xfs_sb_version_has_pquota(&mp->m_sb) && XFS_IS_PQUOTA_ON(mp)) {
> + /*
> + * On disk superblock only has sb_gquotino, and in memory
> + * superblock has both sb_gquotino and sb_pquotino. But,
> + * only one them is supported at any point of time.
> + * So, if PQUOTA is set in disk superblock, copy over
> + * sb_gquotino to sb_pquotino.
> + */
> + mp->m_sb.sb_pquotino = mp->m_sb.sb_gquotino;
> + mp->m_sb.sb_gquotino = NULLFSINO;
> + }
Yup, that's the right place for doing all this on-disk->incore
translation stuff at mount time ;)
> - 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) {
Seeing how often this is repeated, perhaps we need a helper function
called "xfs_is_quota_inode(mp, ino)"?
> @@ -1505,7 +1506,7 @@ xfs_qm_init_quotainos(
> }
> if (XFS_IS_PQUOTA_ON(mp) && pip == NULL) {
> error = xfs_qm_qino_alloc(mp, &pip,
> - sbflags | XFS_SB_GQUOTINO,
> + sbflags | XFS_SB_PQUOTINO,
> flags | XFS_QMOPT_PQUOTA);
Isn't that fixing a bug in the previous patch?
> @@ -412,17 +413,20 @@ xfs_qm_scall_getqstat(
> struct fs_quota_stat *out)
> {
> struct xfs_quotainfo *q = mp->m_quotainfo;
> - struct xfs_inode *uip, *gip;
> - bool tempuqip, tempgqip;
> + struct xfs_inode *uip = NULL;
> + struct xfs_inode *gip = NULL;
> + struct xfs_inode *pip = NULL;
> + bool tempuqip = false;
> + bool tempgqip = false;
> + bool temppqip = false;
See my previous comments about naming variables. At least
add an underscore into the name like temp_uqip....
> @@ -420,12 +420,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;
> @@ -1388,6 +1382,14 @@ xfs_finish_flags(
> return XFS_ERROR(EROFS);
> }
>
> + if ((mp->m_qflags & (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE)) &&
> + (mp->m_qflags & (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE)) &&
> + !xfs_sb_version_has_pquota(&mp->m_sb)) {
> + xfs_warn(mp, "Super block does not support "
> + "project and group quota together");
> + return XFS_ERROR(EINVAL);
> + }
Why move this check? just leave it where it is and add the extra
check to it...
> @@ -157,7 +157,8 @@ xfs_trans_mod_dquot_byino(
> if (!XFS_IS_QUOTA_RUNNING(mp) ||
> !XFS_IS_QUOTA_ON(mp) ||
> ip->i_ino == mp->m_sb.sb_uquotino ||
> - ip->i_ino == mp->m_sb.sb_gquotino)
> + ip->i_ino == mp->m_sb.sb_gquotino ||
> + ip->i_ino == mp->m_sb.sb_pquotino)
> return;
I see a helper....
> if (tp->t_dqinfo == NULL)
> @@ -825,6 +826,7 @@ xfs_trans_reserve_quota_nblks(
>
> ASSERT(ip->i_ino != mp->m_sb.sb_uquotino);
> ASSERT(ip->i_ino != mp->m_sb.sb_gquotino);
> + ASSERT(ip->i_ino != mp->m_sb.sb_pquotino);
And that becomes ASSERT(!xfs_is_quota_inode(mp, ip->i_ino))....
> diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
> index 8655280..f17e3bb 100644
> --- a/include/uapi/linux/dqblk_xfs.h
> +++ b/include/uapi/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
> __u32 qs_incoredqs; /* number of dquots incore */
> __s32 qs_btimelimit; /* limit for blks timer */
> __s32 qs_itimelimit; /* limit for inodes timer */
Ugly, but will do until the next patch ;)
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
next prev parent reply other threads:[~2013-05-17 4:46 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-10 21:21 [PATCH v8 0/5] xfs: Allow pquota and gquota to be used together Chandra Seetharaman
2013-05-10 21:21 ` [PATCH v8 1/5] xfs: Remove incore use of XFS_OQUOTA_ENFD and XFS_OQUOTA_CHKD Chandra Seetharaman
2013-05-13 3:15 ` Jeff Liu
2013-05-14 22:19 ` Chandra Seetharaman
2013-05-17 2:55 ` Dave Chinner
2013-05-24 21:45 ` Chandra Seetharaman
2013-05-10 21:21 ` [PATCH v8 2/5] xfs: Add pquota fields where gquota is used Chandra Seetharaman
2013-05-13 3:59 ` Jeff Liu
2013-05-17 3:01 ` Dave Chinner
2013-05-17 5:40 ` Jeff Liu
2013-05-17 21:15 ` Chandra Seetharaman
2013-05-17 4:23 ` Dave Chinner
2013-05-24 21:57 ` Chandra Seetharaman
2013-06-10 23:17 ` Dave Chinner
2013-06-11 23:08 ` Chandra Seetharaman
2013-05-10 21:21 ` [PATCH v8 3/5] xfs: Start using pquotaino from the superblock Chandra Seetharaman
2013-05-13 4:24 ` Jeff Liu
2013-05-17 4:46 ` Dave Chinner [this message]
2013-05-24 22:09 ` Chandra Seetharaman
2013-06-10 23:20 ` Dave Chinner
2013-05-10 21:21 ` [PATCH v8 4/5] xfs: Add proper versioning support to fs_quota_stat Chandra Seetharaman
2013-05-13 4:29 ` Jeff Liu
2013-05-17 5:10 ` Dave Chinner
2013-05-24 22:17 ` Chandra Seetharaman
2013-06-10 23:27 ` Dave Chinner
2013-06-11 23:13 ` Chandra Seetharaman
2013-05-10 21:21 ` [PATCH v8 5/5] xfs: Use new qs_pquota field in fs_quota_stat for Q_XGETQSTAT Chandra Seetharaman
2013-05-17 5:14 ` Dave Chinner
2013-05-24 22:17 ` Chandra Seetharaman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130517044604.GO24635@dastard \
--to=david@fromorbit.com \
--cc=sekharan@us.ibm.com \
--cc=xfs@oss.sgi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.