Re: [PATCH 04/18] xfs: stop using q_core.d_flags in the quota code

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 04/18] xfs: stop using q_core.d_flags in the quota code
Date: Fri, 3 Jul 2020 11:01:32 -0700	[thread overview]
Message-ID: <20200703180132.GE7606@magnolia> (raw)
In-Reply-To: <20200703005850.GJ2005@dread.disaster.area>

On Fri, Jul 03, 2020 at 10:58:50AM +1000, Dave Chinner wrote:
> On Wed, Jul 01, 2020 at 04:50:31PM -0700, Darrick J. Wong wrote:
> > On Thu, Jul 02, 2020 at 09:44:35AM +1000, Dave Chinner wrote:
> > > On Wed, Jul 01, 2020 at 04:19:10PM -0700, Darrick J. Wong wrote:
> > > > On Thu, Jul 02, 2020 at 08:50:53AM +1000, Dave Chinner wrote:
> > > > > On Tue, Jun 30, 2020 at 08:42:16AM -0700, Darrick J. Wong wrote:
> > > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > > 
> > > > > > Use the incore dq_flags to figure out the dquot type.  This is the first
> > > > > > step towards removing xfs_disk_dquot from the incore dquot.
> > > > > > 
> > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > > ---
> > > > > >  fs/xfs/libxfs/xfs_quota_defs.h |    2 ++
> > > > > >  fs/xfs/scrub/quota.c           |    4 ----
> > > > > >  fs/xfs/xfs_dquot.c             |   33 +++++++++++++++++++++++++++++++--
> > > > > >  fs/xfs/xfs_dquot.h             |    2 ++
> > > > > >  fs/xfs/xfs_dquot_item.c        |    6 ++++--
> > > > > >  fs/xfs/xfs_qm.c                |    4 ++--
> > > > > >  fs/xfs/xfs_qm.h                |    2 +-
> > > > > >  fs/xfs/xfs_qm_syscalls.c       |    9 +++------
> > > > > >  8 files changed, 45 insertions(+), 17 deletions(-)
> > > > > > 
> > > > > > 
> > > > > > diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
> > > > > > index 56d9dd787e7b..459023b0a304 100644
> > > > > > --- a/fs/xfs/libxfs/xfs_quota_defs.h
> > > > > > +++ b/fs/xfs/libxfs/xfs_quota_defs.h
> > > > > > @@ -29,6 +29,8 @@ typedef uint16_t	xfs_qwarncnt_t;
> > > > > >  
> > > > > >  #define XFS_DQ_ALLTYPES		(XFS_DQ_USER|XFS_DQ_PROJ|XFS_DQ_GROUP)
> > > > > >  
> > > > > > +#define XFS_DQ_ONDISK		(XFS_DQ_ALLTYPES)
> > > > > 
> > > > > That's used as an on-disk flags mask. Perhaps XFS_DQF_ONDISK_MASK?
> > > > 
> > > > Well, based on Christoph's suggestions I broke the incore dquot flags
> > > > (XFS_DQ_*) apart from the ondisk dquot flags (XFS_DQFLAG_*).  Not sure
> > > > if that's really better, but at least the namespaces are separate now.
> > > 
> > > Sure, but the point I was trying to make is that "XFS_DQ_ONDISK"
> > > doesn't actually indicate what part of the on-disk dquot it refers
> > > to. We use the phrase "on-disk dquot" to refer to the entire on-disk
> > > dquot, not a subset of flags in a flags field in the on-disk
> > > dquot. Hence the name of this variable needs to be more specific as
> > > to what it applies to in the on-disk dquot...
> > 
> > Sorry, I was typing too fast.  xfs_format.h now has:
> > 
> > #define XFS_DQFLAG_USER		0x01		/* user dquot record */
> > #define XFS_DQFLAG_PROJ		0x02		/* project dquot record */
> > #define XFS_DQFLAG_GROUP	0x04		/* group dquot record */
> > 
> > #define XFS_DQFLAG_TYPE_MASK	(XFS_DQFLAG_USER | \
> > 				 XFS_DQFLAG_PROJ | \
> > 				 XFS_DQFLAG_GROUP)
> > 
> > #define XFS_DQFLAG_ALL		(XFS_DQFLAG_TYPE_MASK)
> > 
> > /*
> >  * This is the main portion of the on-disk representation of quota
> >  * information for a user. This is the q_core of the struct xfs_dquot
> >  * that is kept in kernel memory. We pad this with some more expansion
> >  * room to construct the on disk structure.
> >  */
> > struct xfs_disk_dquot {
> > 	__be16		d_magic;	/* dquot magic = XFS_DQUOT_MAGIC */
> > 	__u8		d_version;	/* dquot version */
> > 	__u8		d_flags;	/* XFS_DQFLAG_* */
> > 
> > I'm not particularly thrilled about the DQFLAG/DQ thing though.  DDFLAG?
> > (Also note that the future bigtime series will add a new ondisk flag
> > XFS_DQFLAG_BIGTIME, which ofc will get added to XFS_DQFLAG_ALL.)
> 
> /me shrugs
> 
> I don't have any good ideas, but I think that DQFLAG is a bad choice
> for an on-disk flag namespace because it's way too generic. It's
> more a type/feature indicator so perhaps we should rename d_flags to
> d_type or d_features and use:
> 
> #define XFS_DDQTYPE_USER
> #define XFS_DDQTYPE_PROJ
> #define XFS_DDQTYPE_GROUP
> #define XFS_DDQTYPE_BIGTIME
> 
> #define XFS_DDQTYPE_QUOTA_MASK	(XFS_DDQTYPE_USER | ...
> 
> #define XFS_DDQTYPE_ALL		(XFS_DDQTYPE_QUOTA_MASK | XFS_DDQTYPE_BIGTIME)
> 
> Or something like that...

I prefer d_features/DDQFEAT over d_type/DDQTYPE because I don't want
people to mix up "ondisk dquot type (user/group/proj/bigtime)" with
"quota type (user/group/proj)".

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com