diff mbox series

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

Message ID 20181003123537.30965-2-cmaiolino@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series xfs stats fixes | expand

Commit Message

Carlos Maiolino Oct. 3, 2018, 12:35 p.m. UTC
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(-)

Comments

Eric Sandeen Oct. 3, 2018, 12:47 p.m. UTC | #1
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;
>
Carlos Maiolino Oct. 3, 2018, 1:39 p.m. UTC | #2
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;
> >
Eric Sandeen Oct. 3, 2018, 1:59 p.m. UTC | #3
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
Darrick J. Wong Oct. 3, 2018, 3:18 p.m. UTC | #4
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
>
Darrick J. Wong Oct. 3, 2018, 3:20 p.m. UTC | #5
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
> >
Carlos Maiolino Oct. 4, 2018, 8:01 a.m. UTC | #6
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
diff mbox series

Patch

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;