[PATCH 0/2] xfs stats fixes

[PATCH 0/2] xfs stats fixes

[Carlos Maiolino]

Hi,

this week I found a bug in the code for xqmstat file in procfs, caused by wrong
offsets while reading the __xfsstats structure fields in xqmstat_proc_show.

This patchset aims to fix the bug (patch 1) and add a way to reduce the
likelyhood of creating the same error again.

xqmstat_proc_show() uses a loop to walk through the quota-specific fields in
__xfsstats when printing the information. The problem is, since the start/end
offsets are hardcoded, any update to __xfsstats may also require an update on
the start offset in xqmstat_proc_show().

I decided to split it into two patches, because I think it is easier to let the
bugfix itself in a separated patch, and the approach on how to avoid it in the
future can be discussed in a separated patch.

My first idea was to try to add a method to check it at compile time, and I spent
some time discussing with Eric what would be the best way to avoid this problem
in the future.
We end up with an possible solution using offsetof() and a new constant in
xfs_stats.h to properly check the offsets at compile time.
Although, after giving it some extra thought, I found it better to only use a
new constant to mark the start of xqmstat in xfs_stats.h and not add any fancy
calculation in xqmstat_proc_show itself.
History told me that all updates to __xfsstats and its respective content/uses,
were properly done when needed, only the xqmstat was left behind because it's
unusual direct usage of the offset markers. So, I believe that the addition of a
new marker to specify the start offset of the quota stat fields is enough to
avoid future problems here, once no more changes will be needed in xqmstat code
whenever __xfsstats requires to be updated.

My idea though is still to move this file to sysfs together with the rest of
statistic code, but by now, I believe we should fix this bug.

Comments?

Cheers.


Carlos Maiolino (2):
  xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat
  xfs: Add new constant to mark start of xqmstat

 fs/xfs/xfs_stats.c | 2 +-
 fs/xfs/xfs_stats.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat

[Carlos Maiolino]

The addition of FIBT, RMAP and REFCOUNT changed the offsets into
__xfssats structure.

Although this didn't cause any direct issue,  xqmstat_proc_show() relied
on the old offsets to display the xqm statistics.

Fix it.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_stats.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
index 4e4423153071..740ac9674848 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_IBT_V2; j < XFSSTAT_END_XQMSTAT; j++)
+	for (j = XFSSTAT_END_REFCOUNT; j < XFSSTAT_END_XQMSTAT; j++)
 		seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j));
 	seq_putc(m, '\n');
 	return 0;
-- 
2.17.1

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

[Carlos Maiolino]

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(-)

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;
-- 
2.17.1

Re: [PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat

[Eric Sandeen]

On 10/3/18 7:35 AM, Carlos Maiolino wrote:
> The addition of FIBT, RMAP and REFCOUNT changed the offsets into
> __xfssats structure.
> 
> Although this didn't cause any direct issue,  xqmstat_proc_show() relied
> on the old offsets to display the xqm statistics.

Well, it caused /proc/fs/xfs/xqmstat to display garbage data, right?
That seems worth highlighting in the changelog, and not glossing over.

Could maybe use Fixes: tags for:

00f4e4f9 xfs: add rmap btree stats infrastructure
aafc3c24 xfs: support the XFS_BTNUM_FINOBT free inode btree type
46eeb521 xfs: introduce refcount btree definitions

and a stable tag as well?  Though stable is tricky because different patches
would be required for different points along the stats structure evolution...
maybe best to ignore it, chances of auto-applying it correctly are slim to
none.

As for the change itself,

Reviewed-by: Eric Sandeen <sandeen@redhat.com>



> Fix it.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_stats.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index 4e4423153071..740ac9674848 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_IBT_V2; j < XFSSTAT_END_XQMSTAT; j++)
> +	for (j = XFSSTAT_END_REFCOUNT; j < XFSSTAT_END_XQMSTAT; j++)
>  		seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j));
>  	seq_putc(m, '\n');
>  	return 0;
> 

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

[Eric Sandeen]

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;
> 

Re: [PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat

[Carlos Maiolino]

On Wed, Oct 03, 2018 at 07:47:46AM -0500, Eric Sandeen wrote:
> On 10/3/18 7:35 AM, Carlos Maiolino wrote:
> > The addition of FIBT, RMAP and REFCOUNT changed the offsets into
> > __xfssats structure.
> > 
> > Although this didn't cause any direct issue,  xqmstat_proc_show() relied
> > on the old offsets to display the xqm statistics.
> 
> Well, it caused /proc/fs/xfs/xqmstat to display garbage data, right?
> That seems worth highlighting in the changelog, and not glossing over.

Well, I wouldn't say 'garbage' data. I'd say data from other fields in the
structure (at this point, specifically fino btree data) :P, but sure, I can add
it to the changelog.

> 
> Could maybe use Fixes: tags for:
> 
> 00f4e4f9 xfs: add rmap btree stats infrastructure
> aafc3c24 xfs: support the XFS_BTNUM_FINOBT free inode btree type
> 46eeb521 xfs: introduce refcount btree definitions
> 

No objection.

> and a stable tag as well?  Though stable is tricky because different patches
> would be required for different points along the stats structure evolution...
> maybe best to ignore it, chances of auto-applying it correctly are slim to
> none.

Indeed, this may not apply to stable trees, unless the tree itself contains all
three data structures (rmap finobtree and refcount).

> 
> As for the change itself,
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> 
> 
> > Fix it.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  fs/xfs/xfs_stats.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> > index 4e4423153071..740ac9674848 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_IBT_V2; j < XFSSTAT_END_XQMSTAT; j++)
> > +	for (j = XFSSTAT_END_REFCOUNT; j < XFSSTAT_END_XQMSTAT; j++)
> >  		seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j));
> >  	seq_putc(m, '\n');
> >  	return 0;
> > 

-- 
Carlos

Re: [PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat

[Eric Sandeen]

On 10/3/18 8:39 AM, Carlos Maiolino wrote:
> On Wed, Oct 03, 2018 at 07:47:46AM -0500, Eric Sandeen wrote:
>> On 10/3/18 7:35 AM, Carlos Maiolino wrote:
>>> The addition of FIBT, RMAP and REFCOUNT changed the offsets into
>>> __xfssats structure.
>>>
>>> Although this didn't cause any direct issue,  xqmstat_proc_show() relied
>>> on the old offsets to display the xqm statistics.
>>
>> Well, it caused /proc/fs/xfs/xqmstat to display garbage data, right?
>> That seems worth highlighting in the changelog, and not glossing over.
> 
> Well, I wouldn't say 'garbage' data. I'd say data from other fields in the
> structure (at this point, specifically fino btree data) :P, but sure, I can add
> it to the changelog.

That's kind of like saying stale data exposure or kernel memory leaks isn't
garbage data, it's just someone /else's/ data.  ;)

Anyway, however you want to phrase it, aI think the change needs to highlight
that there /is/ a failure and it's not an irrelevant fix.

>>
>> Could maybe use Fixes: tags for:
>>
>> 00f4e4f9 xfs: add rmap btree stats infrastructure
>> aafc3c24 xfs: support the XFS_BTNUM_FINOBT free inode btree type
>> 46eeb521 xfs: introduce refcount btree definitions
>>
> 
> No objection.
> 
>> and a stable tag as well?  Though stable is tricky because different patches
>> would be required for different points along the stats structure evolution...
>> maybe best to ignore it, chances of auto-applying it correctly are slim to
>> none.
> 
> Indeed, this may not apply to stable trees, unless the tree itself contains all
> three data structures (rmap finobtree and refcount).

Yeah.  It would go back to 4.10 cleanly, tho.  Just a thought.

-Eric

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

[Carlos Maiolino]

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;
> > 

-- 
Carlos

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

[Eric Sandeen]

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

Re: [PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat

[Darrick J. Wong]

On Wed, Oct 03, 2018 at 02:35:36PM +0200, Carlos Maiolino wrote:
> The addition of FIBT, RMAP and REFCOUNT changed the offsets into
> __xfssats structure.
> 
> Although this didn't cause any direct issue,  xqmstat_proc_show() relied
> on the old offsets to display the xqm statistics.
> 
> Fix it.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_stats.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index 4e4423153071..740ac9674848 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_IBT_V2; j < XFSSTAT_END_XQMSTAT; j++)
> +	for (j = XFSSTAT_END_REFCOUNT; j < XFSSTAT_END_XQMSTAT; j++)

/me totally missed that when I was writing reflink, sorry. :(

Can we have a:

  #define XFSSTAT_START_XQMSTAT (XFSSTAT_END_REFCOUNT)

in xfs_stats.h right above the XFSSTAT_END_XQMSTAT definition so that
future authors don't have to remember that some code use _END_REFCOUNT
to mean "start of next thing"?

--D

>  		seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j));
>  	seq_putc(m, '\n');
>  	return 0;
> -- 
> 2.17.1
> 

Re: [PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat

[Darrick J. Wong]

On Wed, Oct 03, 2018 at 08:18:08AM -0700, Darrick J. Wong wrote:
> On Wed, Oct 03, 2018 at 02:35:36PM +0200, Carlos Maiolino wrote:
> > The addition of FIBT, RMAP and REFCOUNT changed the offsets into
> > __xfssats structure.
> > 
> > Although this didn't cause any direct issue,  xqmstat_proc_show() relied
> > on the old offsets to display the xqm statistics.
> > 
> > Fix it.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  fs/xfs/xfs_stats.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> > index 4e4423153071..740ac9674848 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_IBT_V2; j < XFSSTAT_END_XQMSTAT; j++)
> > +	for (j = XFSSTAT_END_REFCOUNT; j < XFSSTAT_END_XQMSTAT; j++)
> 
> /me totally missed that when I was writing reflink, sorry. :(
> 
> Can we have a:
> 
>   #define XFSSTAT_START_XQMSTAT (XFSSTAT_END_REFCOUNT)
> 
> in xfs_stats.h right above the XFSSTAT_END_XQMSTAT definition so that
> future authors don't have to remember that some code use _END_REFCOUNT
> to mean "start of next thing"?

Oh, that is patch 2.  Sorry, disregard message.

> --D
> 
> >  		seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j));
> >  	seq_putc(m, '\n');
> >  	return 0;
> > -- 
> > 2.17.1
> > 

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

[Dave Chinner]

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

Re: [PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat

[Carlos Maiolino]

On Wed, Oct 03, 2018 at 08:59:52AM -0500, Eric Sandeen wrote:
> On 10/3/18 8:39 AM, Carlos Maiolino wrote:
> > On Wed, Oct 03, 2018 at 07:47:46AM -0500, Eric Sandeen wrote:
> >> On 10/3/18 7:35 AM, Carlos Maiolino wrote:
> >>> The addition of FIBT, RMAP and REFCOUNT changed the offsets into
> >>> __xfssats structure.
> >>>
> >>> Although this didn't cause any direct issue,  xqmstat_proc_show() relied
> >>> on the old offsets to display the xqm statistics.
> >>
> >> Well, it caused /proc/fs/xfs/xqmstat to display garbage data, right?
> >> That seems worth highlighting in the changelog, and not glossing over.
> > 
> > Well, I wouldn't say 'garbage' data. I'd say data from other fields in the
> > structure (at this point, specifically fino btree data) :P, but sure, I can add
> > it to the changelog.
> 
> That's kind of like saying stale data exposure or kernel memory leaks isn't
> garbage data, it's just someone /else's/ data.  ;)
> 
> Anyway, however you want to phrase it, aI think the change needs to highlight
> that there /is/ a failure and it's not an irrelevant fix.
> 

Sure, I'll resubmit the patch again with updated description  together with a
new version of patch 2, I am considering to do what Dave suggested and replace
the defines by offsetof() macros, but well, will see.

I'll add your reviewed-by flag to the patch 1, is it ok? I think that's what you
meant with your previous reply?

> >>
> >> Could maybe use Fixes: tags for:
> >>
> >> 00f4e4f9 xfs: add rmap btree stats infrastructure
> >> aafc3c24 xfs: support the XFS_BTNUM_FINOBT free inode btree type
> >> 46eeb521 xfs: introduce refcount btree definitions
> >>
> > 
> > No objection.
> > 
> >> and a stable tag as well?  Though stable is tricky because different patches
> >> would be required for different points along the stats structure evolution...
> >> maybe best to ignore it, chances of auto-applying it correctly are slim to
> >> none.
> > 
> > Indeed, this may not apply to stable trees, unless the tree itself contains all
> > three data structures (rmap finobtree and refcount).
> 
> Yeah.  It would go back to 4.10 cleanly, tho.  Just a thought.

Ok,
Cheers

> 
> -Eric

-- 
Carlos