Re: [PATCH 2/2] xfs: Add new constant to mark start of xqmstat

[Dave Chinner <david@fromorbit.com>]

On Wed, Oct 03, 2018 at 09:04:16AM -0500, Eric Sandeen wrote:
> 
> 
> On 10/3/18 9:00 AM, Carlos Maiolino wrote:
> > On Wed, Oct 03, 2018 at 07:51:43AM -0500, Eric Sandeen wrote:
> >> On 10/3/18 7:35 AM, Carlos Maiolino wrote:
> >>> xqmstat information under __xfsstats is used directly by the
> >>> /proc/fs/xfs/xqmstat show method, which makes use of struct offsets to
> >>> print out the values.
> >>>
> >>> Currently, the function is setup to start printing values from the offset of
> >>> the last marker before xqmstat, so, an update to __xfsstats structure
> >>> may also require an update of xqmstat_proc_show() function.
> >>>
> >>> By adding a new constant, we concentrate any updates do __xfsstats
> >>> structure fields locally to the file xfs_stats.h, reducing the
> >>> likelyhood of future bugs if an update to xqmstat_proc_show is
> >>> forgotten as have happened before.
> >>
> >> did 
> >>
> >> BUILD_BUG_ON(offsetof(struct __xfsstats, xs_qm_dqreclaims)/sizeof(uint32_t)) != XFSSTAT_START_XQMSTAT);
> >>
> >> and/or
> >>
> >> #define XFSSTAT_START_XQMSTAT	(offsetof(struct __xfsstats, xs_qm_dqreclaims)/sizeof(uint32_t))
> > 
> > Yup, it worked, although, it still doesn't really close the possibility for
> > mistakes here, the first field may change for example and we may end up in the
> > same situation.
> > 
> > I think only the 2nd option (defining START via offsetof), doesn't differ much
> > from the approach in the patch, and, using the 1st option (BUILD_BUG_ON), will
> > require an update to xqmstat_proc_show() if by any reason we change the quota
> > values, so, I thought the simpler version as the patch would suffice and be a
> > bit less confusing. But well, feel free to disagree, it's my opinion only and
> > not written on stone :P
> 
> I'd argue that the first field of xqmstats changing not possible, because
> this is essentially a userspacce API.  If we're going to change the format
> of xqmstats, we should only add to the end of it.

That's correct. Once published, the stats in each line are fixed in
place forever. We can extend the stats by appending new stats to the
end of each line, or by adding new lines. But we can't change the
order or meaning of existing entries.

> The thing about using offsetof() is that we'll never have to update the START
> define again, even if we add more records to the larger stats structure.

I think the whole thing needs to be changed to use offsetof() rather
than the messy set of cascading defines we have now.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com