xfs
[Top] [All Lists]

Re: [RFC PATCH 2/3] Add pquota fields where gquota is used

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [RFC PATCH 2/3] Add pquota fields where gquota is used
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Wed, 19 Oct 2011 10:17:05 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20111019095742.GA20622@xxxxxxxxxxxxx>
Organization: IBM
References: <20111018000932.14942.11597.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20111018000943.14942.24899.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20111019095742.GA20622@xxxxxxxxxxxxx>
Reply-to: sekharan@xxxxxxxxxx
Hi Christoph,

Thanks for the comments.

See responses inline below.

On Wed, 2011-10-19 at 05:57 -0400, Christoph Hellwig wrote:
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -841,8 +841,10 @@ xfs_qm_dqget(
> >             ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> >             if (type == XFS_DQ_USER)
> >                     ASSERT(ip->i_udquot == NULL);
> > -           else
> > +           else if (type == XFS_DQ_GROUP)
> >                     ASSERT(ip->i_gdquot == NULL);
> > +           else
> > +                   ASSERT(ip->i_pdquot == NULL);
> 
> An xfs_inode_dquot(ip, type) macro that gets you the right quota for
> a given type would be highly useful.  I'd love to see that as the
> first patch in the series, as it would a) clean up a lot of code like
> this and b) would also help with my TODO list of overlaying the
> user/group dquots with the existing fields in the VFS inode and thus
> shrinking the XFS inode.
> 
will do.

> > @@ -933,8 +935,8 @@ xfs_qm_dqget(
> >                             xfs_dqlock(dqp);
> >                             goto dqret;
> >                     }
> > -           } else {
> > -                   if (!XFS_IS_OQUOTA_ON(mp)) {
> > +           } else if (type == XFS_DQ_GROUP) {
> > +                   if (!XFS_IS_GQUOTA_ON(mp)) {
> >                             /* inode stays locked on return */
> >                             xfs_qm_dqdestroy(dqp);
> >                             return XFS_ERROR(ESRCH);
> 
> Together with a
> 
>       XFS_IS_TYPE_QUOTA_ON(mp, type)
> 
> that should also really greatly simplify this error path in
> xfs_qm_dqget.
> 
> > +#define XFS_IS_THIS_QUOTA_OFF(d) (! ((XFS_QM_ISUDQ(d) && \
> > +                                XFS_IS_UQUOTA_ON((d)->q_mount)) || \
> > +                                (XFS_QM_ISGDQ(d) && \
> > +                                XFS_IS_GQUOTA_ON((d)->q_mount)) || \
> > +                                (XFS_QM_ISPDQ(d) && \
> > +                                XFS_IS_PQUOTA_ON((d)->q_mount))))
> 
> And the two callers of this could easily be converted to
> !XFS_IS_TYPE_QUOTA_ON.
> 
> > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> > index 760140d..26f95a6 100644
> > --- a/fs/xfs/xfs_inode.h
> > +++ b/fs/xfs/xfs_inode.h
> > @@ -231,6 +231,7 @@ typedef struct xfs_inode {
> >     struct xfs_mount        *i_mount;       /* fs mount struct ptr */
> >     struct xfs_dquot        *i_udquot;      /* user dquot */
> >     struct xfs_dquot        *i_gdquot;      /* group dquot */
> > +   struct xfs_dquot        *i_pdquot;      /* project dquot */
> 
> I'd really prefer to leave growing the inode until we are fully done
> adding it.  So move this part to the back, just add the macro and quota
> on check in the first patch, but always redirect it to i_gdquot for now.

Sure, will do.

> 
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -628,6 +628,7 @@ xfs_sb_to_disk(
> >     xfs_sb_field_t  f;
> >     int             first;
> >     int             size;
> > +   __be16          saved_qflags = from->sb_qflags;
> >  
> >     ASSERT(fields);
> >     if (!fields)
> > @@ -669,6 +670,7 @@ xfs_sb_to_disk(
> >  
> >             fields &= ~(1LL << f);
> >     }
> > +   from->sb_qflags = saved_qflags;
> >  }
> 
> What is this for?  It will need some better documentation.

We want to keep XFS_OQUOTA_.* flags in-disk but on in-core. the while
loop between this save and restore copies the information from in-core
to a buffer destined to go to disk. So, I save the in-core information
then change it which is then copied to the buffer, then I restore the
in-core information back.

I will add some comments to that effect.
> 
> > -           ASSERT(ip->i_udquot == NULL);
> > +           ASSERT_ALWAYS(ip->i_udquot == NULL);
> 
> Why the change to ASSERT_ALWAYS here?

oops. debug left over. will remove it.
> 


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