xfs
[Top] [All Lists]

Re: [RFC v6 PATCH 4/5] xfs: Add proper versioning support to fs_quota_st

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [RFC v6 PATCH 4/5] xfs: Add proper versioning support to fs_quota_stat
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Wed, 22 Aug 2012 18:32:32 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120815023206.GT2877@dastard>
Organization: IBM
References: <20120720230202.20477.69766.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20120720230247.20477.68011.sendpatchset@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20120815023206.GT2877@dastard>
Reply-to: sekharan@xxxxxxxxxx
Will change as suggested.

Thanks for the review and clarification.
On Wed, 2012-08-15 at 12:32 +1000, Dave Chinner wrote:
> On Fri, Jul 20, 2012 at 06:02:47PM -0500, Chandra Seetharaman wrote:
> > Add proper versioning support for fs_quota_stat.
> > 
> > Leave the earlier version as version 1 and add version 2 to add a new
> > field "fs_qfilestat_t qs_pquota"
> > 
> > Callers of the system call have to just set the version of the data
> > structure being passed, and kernel will fill as much data as possible.
> 
> Userspace Interface Traps for the Unwary 101, below.
> 
> >  /*
> >   * Disk quota - quotactl(2) commands for the XFS Quota Manager (XQM).
> > @@ -139,6 +140,7 @@ typedef struct fs_disk_quota {
> >   * incore.
> >   */
> >  #define FS_QSTAT_VERSION   1       /* fs_quota_stat.qs_version */
> > +#define FS_QSTAT_VERSION_2 2       /* new field qs_pquota */
> >  
> >  /*
> >   * Some basic information about 'quota files'.
> > @@ -155,13 +157,37 @@ 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 */    
> >     __s32           qs_rtbtimelimit;/* limit for rt blks timer */   
> >     __u16           qs_bwarnlimit;  /* limit for num warnings */
> >     __u16           qs_iwarnlimit;  /* limit for num warnings */
> > +   fs_qfilestat_t  qs_pquota;      /* project quota storage information */
> >  } fs_quota_stat_t;
> >  
> > +#define FS_QSTAT_V1_SIZE   (offsetof(fs_quota_stat_t, qs_pquota))
> > +#define FS_QSTAT_V2_SIZE   (FS_QSTAT_V1_SIZE + sizeof (fs_qfilestat_t))
> 
> I don't expect that will work on all arches. pahole (everyone needs
> to know about pahole!) tells me the original structure on x86_64
> looks like:
> 
> struct fs_quota_stat {
>         __s8                       qs_version;           /*     0     1 */
> 
>         /* XXX 1 byte hole, try to pack */
> 
>         __u16                      qs_flags;             /*     2     2 */
>         __s8                       qs_pad;               /*     4     1 */
> 
>         /* XXX 3 bytes hole, try to pack */
> 
>         fs_qfilestat_t             qs_uquota;            /*     8    24 */
>         fs_qfilestat_t             qs_gquota;            /*    32    24 */
>         __u32                      qs_incoredqs;         /*    56     4 */
>         __s32                      qs_btimelimit;        /*    60     4 */
>         /* --- cacheline 1 boundary (64 bytes) --- */
>         __s32                      qs_itimelimit;        /*    64     4 */
>         __s32                      qs_rtbtimelimit;      /*    68     4 */
>         __u16                      qs_bwarnlimit;        /*    72     2 */
>         __u16                      qs_iwarnlimit;        /*    74     2 */
> 
>         /* size: 80, cachelines: 2, members: 11 */
>         /* sum members: 72, holes: 2, sum holes: 4 */
>         /* padding: 4 */
>         /* last cacheline: 16 bytes */
> }
> 
> and that qs_iwarnlimit does not end on a 8 byte boundary. If we then
> look at fs_qfilestat:
> 
> typedef struct fs_qfilestat {
>         __u64           qfs_ino;        /* inode number */
>         __u64           qfs_nblks;      /* number of BBs 512-byte-blks */
>         __u32           qfs_nextents;   /* number of extents */
> } fs_qfilestat_t;
> 
> it has an 8 byte alignment, so the above FS_QSTAT_V2_SIZE
> calculation is wrong because it doesn't take into account holes in
> the structure and there is 4 bytes of hole between qs_iwarnlimit and
> qs_pquota. It's entirely possible that this might require a compat
> handler as some platforms might pack the structure differently in 32
> vs 64 bit binaries..
> 
> IOWs, you ned to explicitly add padding to thie structure, like
> someone tried to do in the past a screwed it up completely.
> Basically, the structure needs to be padded like this:
> 
> typedef struct fs_quota_stat {
>       __s8            qs_version;     /* version number for future changes */
>       __u8            qs_pad1;        /* unused */
>       __u16           qs_flags;       /* FS_QUOTA_{U,P,G}DQ_{ACCT,ENFD} */
>       __u8            qs_pad2[3];     /* unused */
>       fs_qfilestat_t  qs_uquota;      /* user quota storage information */
>       fs_qfilestat_t  qs_gquota;      /* group quota storage information */
>       __u32           qs_incoredqs;   /* number of dquots incore */
>       __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 */
>       __u8            qs_pad3[4];     /* unused */
>       fs_qfilestat_t  qs_pquota;      /* project quota storage information */
> } fs_quota_stat_t;
> 
> > +
> > +static inline int valid_qstat_version(int version)
> > +{
> > +   switch (version) {
> > +   case FS_QSTAT_VERSION:
> > +   case FS_QSTAT_VERSION_2:
> > +           return 1;
> > +   default:
> > +           return 0;
> > +   }
> > +}
> > +static inline int qstatsize(int version)
> > +{
> > +   switch (version) {
> > +   case FS_QSTAT_VERSION_2:
> > +           return FS_QSTAT_V2_SIZE;
> > +   case FS_QSTAT_VERSION:
> > +   default:
> > +           return FS_QSTAT_V1_SIZE;
> > +   }
> > +}
> 
> I don't see much point in these helpers - just put them in line in
> the quota_getxstate() code. i.e.
> 
>       switch (version) {
>       case FS_QSTAT_VERSION_2:
>               size = FS_QSTAT_V2_SIZE;
>               break;
>       default:
>               version = FS_QSTAT_VERSION;
>               /* fallthrough */
>       case FS_QSTAT_VERSION:
>               size = FS_QSTAT_V1_SIZE;
>               break;
>       }
> 
> Cheers,
> 
> Dave.
> 


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