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.
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
