| Message ID | 20181003123537.30965-2-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs stats fixes | expand |
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; >
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; > >
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
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 >
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 > >
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 --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;
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(-)