xfs
[Top] [All Lists]

Re: [PATCH v11 3/4] xfs: Add proper versioning support to fs_quota_stat

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v11 3/4] xfs: Add proper versioning support to fs_quota_stat
From: Steven Whitehouse <swhiteho@xxxxxxxxxx>
Date: Thu, 11 Jul 2013 09:05:04 +0100
Cc: Chandra Seetharaman <sekharan@xxxxxxxxxx>, xfs@xxxxxxxxxxx, jack@xxxxxxx, adas@xxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130711080537.GC3438@dastard>
Organization: Red Hat UK Ltd
References: <1373518843-1492-1-git-send-email-sekharan@xxxxxxxxxx> <1373518843-1492-4-git-send-email-sekharan@xxxxxxxxxx> <20130711080537.GC3438@dastard>
Hi,

Please keep Abhi cc'd on this thread. Thanks,

Steve.

On Thu, 2013-07-11 at 18:05 +1000, Dave Chinner wrote:
> On Thu, Jul 11, 2013 at 12:00:42AM -0500, Chandra Seetharaman wrote:
> > Added appropriate pads and code for backward compatibility.
> > 
> > Copied over the old version as it is different from the newer padded
> > version.
> > 
> > New callers of the system call have to set the version of the data
> > structure being passed, and kernel will fill as much data as requested.
> > 
> > Signed-off-by: Chandra Seetharaman <sekharan@xxxxxxxxxx>
> > ---
> >  fs/gfs2/quota.c                |    3 --
> >  fs/quota/quota.c               |   67 
> > +++++++++++++++++++++++++++++++++++++--
> >  fs/xfs/xfs_qm_syscalls.c       |    4 --
> >  include/uapi/linux/dqblk_xfs.h |   60 +++++++++++++++++++++++++++++++----
> >  4 files changed, 116 insertions(+), 18 deletions(-)
> > 
> > diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> > index c7c840e..ca0dccd 100644
> > --- a/fs/gfs2/quota.c
> > +++ b/fs/gfs2/quota.c
> > @@ -1443,9 +1443,6 @@ static int gfs2_quota_get_xstate(struct super_block 
> > *sb,
> >  {
> >     struct gfs2_sbd *sdp = sb->s_fs_info;
> >  
> > -   memset(fqs, 0, sizeof(struct fs_quota_stat));
> > -   fqs->qs_version = FS_QSTAT_VERSION;
> > -
> >     switch (sdp->sd_args.ar_quota) {
> >     case GFS2_QUOTA_ON:
> >             fqs->qs_flags |= (FS_QUOTA_UDQ_ENFD | FS_QUOTA_GDQ_ENFD);
> > diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> > index c7314f1..ac5dd3a 100644
> > --- a/fs/quota/quota.c
> > +++ b/fs/quota/quota.c
> > @@ -204,15 +204,72 @@ static int quota_setxstate(struct super_block *sb, 
> > int cmd, void __user *addr)
> >     return sb->s_qcop->set_xstate(sb, flags, cmd);
> >  }
> >  
> > -static int quota_getxstate(struct super_block *sb, void __user *addr)
> > +static int quota_getxstate(struct super_block *sb, void __user *addr,
> > +                      bool older_version)
> 
> I'd suggest that the two API calls should be handled by separate
> functions so there are no overlapping, dependent branches in the
> code.
> 
> >  {
> >     struct fs_quota_stat fqs;
> > -   int ret;
> > +   struct fs_quota_stat_v1 fqs_v1;
> > +   int ret, size;
> > +   void *fqsp;
> >  
> >     if (!sb->s_qcop->get_xstate)
> >             return -ENOSYS;
> 
> I'd also suggest that a sb->s_qcop->get_xstatev vector is added
> so that filesystems keep the separate as well.
> 
> And that means we don't need to play the fs_quota_stat and
> conversion-to-fs_quota_stat_v1 games. i.e. the existing
> quota_getxstate() remains and uses the existing struct
> fs_quota_stat, and the new Q_XGETQSTATV calls quota_getxstatev() and
> uses the newly defined struct fs_quota_statv and ->get_xstatev()
> call....
> 
> > @@ -137,31 +139,75 @@ typedef struct fs_disk_quota {
> >   * Provides a centralized way to get meta information about the quota 
> > subsystem.
> >   * eg. space taken up for user and group quotas, number of dquots currently
> >   * incore.
> > + * With version FS_QSTAT_VERSION, user space can send in an uninitialized
> > + * buffer and data will be filled by the kernel.
> > + * Starting FS_QSTAT_VERSION_2, to be used with Q_XGETQSTATV, user space
> > + * caller should set qs_version to the appropriate version of the
> > + * fs_quota_stat data structure they are providing. On return, user
> > + * space should check qs_version and use appropriate fields supported by
> > + * that version.
> >   */
> >  #define FS_QSTAT_VERSION   1       /* fs_quota_stat.qs_version */
> > +#define FS_QSTAT_VERSION_2 2       /* new field qs_pquota; realignment */
> 
> As per above, this reasoning and logic is all invalid now because we
> aren't trying to maintain compatibility between users of the same
> quotactl.
> 
> We are using a new quotactl, so we do not need to overload the
> existing structure or versions.
> 
> > +/*
> > + * Some basic information about 'quota files'. Version 2.
> > + */
> > +struct fs_qfilestat {
> > +   __u64           qfs_ino;        /* inode number */
> > +   __u64           qfs_nblks;      /* number of BBs 512-byte-blks */
> > +   __u32           qfs_nextents;   /* number of extents */
> > +   __u32           qfs_pad;        /* pad for 8-byte alignment */
> > +};
> > +
> > +struct fs_quota_stat {
> > +   __s8                    qs_version;     /* version for future changes */
> > +   __u8                    qs_pad1;        /* pad for 16bit alignment */
> > +   __u16                   qs_flags;       /* FS_QUOTA_.* flags */
> > +   __u32                   qs_incoredqs;   /* number of dquots incore */
> > +   struct fs_qfilestat     qs_uquota;      /* user quota information */
> > +   struct fs_qfilestat     qs_gquota;      /* group quota information */
> > +   struct fs_qfilestat     qs_pquota;      /* project quota information */
> > +   __s32                   qs_btimelimit;  /* limit for blks timer */
> > +   __s32                   qs_itimelimit;  /* limit for inodes timer */
> > +   __s32                   qs_rtbtimelimit;/* limit for rt blks timer */
> > +   __u16                   qs_bwarnlimit;  /* limit for num warnings */
> > +   __u16                   qs_iwarnlimit;  /* limit for num warnings */
> > +   __u64                   qs_pad2[8];     /* for future proofing */
> > +};
> 
> these are what become fs_quota_statv and fs_qfilestatv structures
> for the new quotactl.
> 
> > +/*
> > + * Since Version 1 did not have padding at appropriate places,
> > + * a new data structure has been defined for the older version to
> > + * provide backward compatibility.
> > + * Future extentions of this data structure won't require new
> > + * data structure definitions as the current one can be extended
> > + * with the logic and padding in place now.
> > + */
> > +#define FS_QSTAT_V1_SIZE   (sizeof(struct fs_quota_stat_v1))
> > +#define FS_QSTAT_V2_SIZE   (sizeof(struct fs_quota_stat))
> 
> And these can go away completely.
> 
> Cheers,
> 
> Dave.


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