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

[Carlos Maiolino <cmaiolino@redhat.com>]

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