| Message ID | 20181003123537.30965-3-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs stats fixes | expand |
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)) not work out? > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > fs/xfs/xfs_stats.c | 2 +- > fs/xfs/xfs_stats.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c > index 740ac9674848..09121c36a062 100644 > --- a/fs/xfs/xfs_stats.c > +++ b/fs/xfs/xfs_stats.c > @@ -119,7 +119,7 @@ static int xqmstat_proc_show(struct seq_file *m, void *v) > int j; > > seq_printf(m, "qm"); > - for (j = XFSSTAT_END_REFCOUNT; j < XFSSTAT_END_XQMSTAT; j++) > + for (j = XFSSTAT_START_XQMSTAT; j < XFSSTAT_END_XQMSTAT; j++) > seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j)); > seq_putc(m, '\n'); > return 0; > diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h > index 130db070e4d8..102e794addac 100644 > --- a/fs/xfs/xfs_stats.h > +++ b/fs/xfs/xfs_stats.h > @@ -147,6 +147,7 @@ struct __xfsstats { > uint32_t xs_rmap_2[__XBTS_MAX]; > #define XFSSTAT_END_REFCOUNT (XFSSTAT_END_RMAP_V2 + __XBTS_MAX) > uint32_t xs_refcbt_2[__XBTS_MAX]; > +#define XFSSTAT_START_XQMSTAT XFSSTAT_END_REFCOUNT > #define XFSSTAT_END_XQMSTAT (XFSSTAT_END_REFCOUNT + 6) > uint32_t xs_qm_dqreclaims; > uint32_t xs_qm_dqreclaim_misses; >
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 > > not work out? > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > > --- > > fs/xfs/xfs_stats.c | 2 +- > > fs/xfs/xfs_stats.h | 1 + > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c > > index 740ac9674848..09121c36a062 100644 > > --- a/fs/xfs/xfs_stats.c > > +++ b/fs/xfs/xfs_stats.c > > @@ -119,7 +119,7 @@ static int xqmstat_proc_show(struct seq_file *m, void *v) > > int j; > > > > seq_printf(m, "qm"); > > - for (j = XFSSTAT_END_REFCOUNT; j < XFSSTAT_END_XQMSTAT; j++) > > + for (j = XFSSTAT_START_XQMSTAT; j < XFSSTAT_END_XQMSTAT; j++) > > seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j)); > > seq_putc(m, '\n'); > > return 0; > > diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h > > index 130db070e4d8..102e794addac 100644 > > --- a/fs/xfs/xfs_stats.h > > +++ b/fs/xfs/xfs_stats.h > > @@ -147,6 +147,7 @@ struct __xfsstats { > > uint32_t xs_rmap_2[__XBTS_MAX]; > > #define XFSSTAT_END_REFCOUNT (XFSSTAT_END_RMAP_V2 + __XBTS_MAX) > > uint32_t xs_refcbt_2[__XBTS_MAX]; > > +#define XFSSTAT_START_XQMSTAT XFSSTAT_END_REFCOUNT > > #define XFSSTAT_END_XQMSTAT (XFSSTAT_END_REFCOUNT + 6) > > uint32_t xs_qm_dqreclaims; > > uint32_t xs_qm_dqreclaim_misses; > >
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. 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. But we can see what others think as well. -Eric
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.
diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c index 740ac9674848..09121c36a062 100644 --- a/fs/xfs/xfs_stats.c +++ b/fs/xfs/xfs_stats.c @@ -119,7 +119,7 @@ static int xqmstat_proc_show(struct seq_file *m, void *v) int j; seq_printf(m, "qm"); - for (j = XFSSTAT_END_REFCOUNT; j < XFSSTAT_END_XQMSTAT; j++) + for (j = XFSSTAT_START_XQMSTAT; j < XFSSTAT_END_XQMSTAT; j++) seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j)); seq_putc(m, '\n'); return 0; diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h index 130db070e4d8..102e794addac 100644 --- a/fs/xfs/xfs_stats.h +++ b/fs/xfs/xfs_stats.h @@ -147,6 +147,7 @@ struct __xfsstats { uint32_t xs_rmap_2[__XBTS_MAX]; #define XFSSTAT_END_REFCOUNT (XFSSTAT_END_RMAP_V2 + __XBTS_MAX) uint32_t xs_refcbt_2[__XBTS_MAX]; +#define XFSSTAT_START_XQMSTAT XFSSTAT_END_REFCOUNT #define XFSSTAT_END_XQMSTAT (XFSSTAT_END_REFCOUNT + 6) uint32_t xs_qm_dqreclaims; uint32_t xs_qm_dqreclaim_misses;
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. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- fs/xfs/xfs_stats.c | 2 +- fs/xfs/xfs_stats.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)