xfs
[Top] [All Lists]

Re: [PATCH v8 3/5] xfs: Start using pquotaino from the superblock

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v8 3/5] xfs: Start using pquotaino from the superblock
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Fri, 24 May 2013 17:09:03 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130517044604.GO24635@dastard>
Organization: IBM
References: <1368220889-25188-1-git-send-email-sekharan@xxxxxxxxxx> <1368220889-25188-4-git-send-email-sekharan@xxxxxxxxxx> <20130517044604.GO24635@dastard>
Reply-to: sekharan@xxxxxxxxxx
On Fri, 2013-05-17 at 14:46 +1000, Dave Chinner wrote:
> 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.

So, you are suggesting to move this block of functionality to a new
function, and set the fields in the "to" data structure in that function
and clear the appropriate bits in the same function ?
  
> 
> > +           }
> > +
> >             /*
> > -            * 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?

fixed it
> 
> > +
> > +   } 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.....

fixed it.
> >     }
> 
> >  
> >     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.

sure.
> 
> >             } 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)"?

will do.
> 
> > @@ -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?

No, pquotino is not being used until this function.

> 
> > @@ -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....
> 

why not.

> > @@ -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...

Superblock hasn't been read yet at the old place. So, this block has to
be moved to a place after superblock is read.

> 
> > @@ -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.


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