[KJ] why does struct inodes_stat_t include "int dummy[5]"?

[KJ] why does struct inodes_stat_t include "int dummy[5]"?

[Robert P. J. Day]

  from include/linux/fs.h:

struct inodes_stat_t {
        int nr_inodes;
        int nr_unused;
        int dummy[5];    <---- ?????????
};

  does that array actually have a purpose?

rday
-- 
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
====================================
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ] why does struct inodes_stat_t include "int dummy[5]"?

[Arnaldo Carvalho de Melo]

On 5/29/07, Robert P. J. Day <rpjday@mindspring.com> wrote:
>
>   from include/linux/fs.h:
>
> struct inodes_stat_t {
>         int nr_inodes;
>         int nr_unused;
>         int dummy[5];    <---- ?????????
> };
>
>   does that array actually have a purpose?

Generally these can be considered as padding, to make sure that the
struct uses 7 * sizeof(int), sometimes this is due to ABI
requirements.

Have you checked if this is not used at all? Or if instances of this
struct aren't casted to something else that uses this area for some
purpose?

- Arnaldo
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ] why does struct inodes_stat_t include "int dummy[5]"?

[Robert P. J. Day]

On Tue, 29 May 2007, Arnaldo Carvalho de Melo wrote:

> On 5/29/07, Robert P. J. Day <rpjday@mindspring.com> wrote:
> >
> >   from include/linux/fs.h:
> >
> > struct inodes_stat_t {
> >         int nr_inodes;
> >         int nr_unused;
> >         int dummy[5];    <---- ?????????
> > };
> >
> >   does that array actually have a purpose?
>
> Generally these can be considered as padding, to make sure that the
> struct uses 7 * sizeof(int), sometimes this is due to ABI
> requirements.
>
> Have you checked if this is not used at all? Or if instances of this
> struct aren't casted to something else that uses this area for some
> purpose?

i just removed it and did "make allyesconfig" and no problem with a
build.  so i'll submit a patch to get rid of it and see if anyone
complains.

rday
-- 
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
====================================
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ] why does struct inodes_stat_t include "int dummy[5]"?

[Matthew Wilcox]

On Tue, May 29, 2007 at 01:10:12PM -0400, Robert P. J. Day wrote:
> > Generally these can be considered as padding, to make sure that the
> > struct uses 7 * sizeof(int), sometimes this is due to ABI
> > requirements.
> >
> > Have you checked if this is not used at all? Or if instances of this
> > struct aren't casted to something else that uses this area for some
> > purpose?
> 
> i just removed it and did "make allyesconfig" and no problem with a
> build.  so i'll submit a patch to get rid of it and see if anyone
> complains.

That's totally insufficient.  If it's part of an ABI, you won't notice
bugs caused until some random application happens to depend on it.  If
that area's being used as scratch space for something else, you'll get
buffer overruns, and subtle corruptions.

You need to go and look at *all* uses of inodes_stat, and figure out
whether removing it is safe.  Don't just post a patch; you've then put
the onus of proving you're right on someone else.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ] why does struct inodes_stat_t include "int dummy[5]"?

[Arnaldo Carvalho de Melo]

On 5/29/07, Matthew Wilcox <matthew@wil.cx> wrote:
> On Tue, May 29, 2007 at 01:10:12PM -0400, Robert P. J. Day wrote:
> > > Generally these can be considered as padding, to make sure that the
> > > struct uses 7 * sizeof(int), sometimes this is due to ABI
> > > requirements.
> > >
> > > Have you checked if this is not used at all? Or if instances of this
> > > struct aren't casted to something else that uses this area for some
> > > purpose?
> >
> > i just removed it and did "make allyesconfig" and no problem with a
> > build.  so i'll submit a patch to get rid of it and see if anyone
> > complains.
>
> That's totally insufficient.  If it's part of an ABI, you won't notice
> bugs caused until some random application happens to depend on it.  If
> that area's being used as scratch space for something else, you'll get
> buffer overruns, and subtle corruptions.
>
> You need to go and look at *all* uses of inodes_stat, and figure out
> whether removing it is safe.  Don't just post a patch; you've then put
> the onus of proving you're right on someone else.

Thanks Willy, that is a more assertive comment 8-)

Robert, I guess you should try to use google codesearch and when
discovering the reason please submit a patch with a comment describing
the purpose for this "dummy" field :-)

Sure, if you feel like pursuing this path...

- Arnaldo
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ] why does struct inodes_stat_t include "int dummy[5]"?

[Robert P. J. Day]

On Tue, 29 May 2007, Matthew Wilcox wrote:

> On Tue, May 29, 2007 at 01:10:12PM -0400, Robert P. J. Day wrote:
> > > Generally these can be considered as padding, to make sure that the
> > > struct uses 7 * sizeof(int), sometimes this is due to ABI
> > > requirements.
> > >
> > > Have you checked if this is not used at all? Or if instances of
> > > this struct aren't casted to something else that uses this area
> > > for some purpose?
> >
> > i just removed it and did "make allyesconfig" and no problem with
> > a build.  so i'll submit a patch to get rid of it and see if
> > anyone complains.
>
> That's totally insufficient.

  um ... no, it's not.  it may not be completely *sufficient*, but
that's a far cry from being "totally insufficient."  let's try to
control the hyperbole, shall we?

  let's review the construct in question:

struct inodes_stat_t {
        int nr_inodes;
        int nr_unused;
        int dummy[5];
};

  in the first place, it seems unlikely that that extra space is used
for padding, since what it does is pad out to a very unusual size of 7
ints.  that's just weird.  not to mention that it's called "dummy",
which is about as worthless and uninformative a variable name as
you're ever likely to find.

> If it's part of an ABI, you won't notice bugs caused until some
> random application happens to depend on it.  If that area's being
> used as scratch space for something else, you'll get buffer
> overruns, and subtle corruptions.

  i'm sure i would, which is why, after *i* spend some time looking at
it, i submit a patch to LKML to see if *someone else* can clear up the
mystery.  and if they can, i drop it and move on.  what's your
problem?

> You need to go and look at *all* uses of inodes_stat, and figure out
> whether removing it is safe.

  in fact, i did just that, grepping through the tree to see if i
could spot any definition of that structure for which the "dummy"
array was referenced, and i didn't find one.  doesn't mean there
isn't one, only that i didn't notice one.

> Don't just post a patch; you've then put the onus of proving you're
> right on someone else.

and the problem with that is ... what exactly?  if i propose removing
something that looks useless, and not a single person on LKML can
explain what it's actually being used for, i think i've made my case.

you seem to be confusing the actions of *submitting* a patch with
forcing the application of one.  there's a difference.

rday
-- 
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
====================================
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ] why does struct inodes_stat_t include "int dummy[5]"?

[Matthew Wilcox]

On Tue, May 29, 2007 at 01:44:21PM -0400, Robert P. J. Day wrote:
> On Tue, 29 May 2007, Matthew Wilcox wrote:
> > On Tue, May 29, 2007 at 01:10:12PM -0400, Robert P. J. Day wrote:
> > > > Generally these can be considered as padding, to make sure that the
> > > > struct uses 7 * sizeof(int), sometimes this is due to ABI
> > > > requirements.
> > > >
> > > > Have you checked if this is not used at all? Or if instances of
> > > > this struct aren't casted to something else that uses this area
> > > > for some purpose?
> > >
> > > i just removed it and did "make allyesconfig" and no problem with
> > > a build.  so i'll submit a patch to get rid of it and see if
> > > anyone complains.
> >
> > That's totally insufficient.
> 
>   um ... no, it's not.  it may not be completely *sufficient*, but
> that's a far cry from being "totally insufficient."  let's try to
> control the hyperbole, shall we?

It's not hyperbole.  You were given two reasons that it might be there.
Your reponse was to do a test that would not determine whether either
reason is true.

> struct inodes_stat_t {
>         int nr_inodes;
>         int nr_unused;
>         int dummy[5];
> };
> 
>   in the first place, it seems unlikely that that extra space is used
> for padding, since what it does is pad out to a very unusual size of 7
> ints.  that's just weird.  not to mention that it's called "dummy",
> which is about as worthless and uninformative a variable name as
> you're ever likely to find.

While it's unusual, that in itself should tell you that it's not for
reasons of cacheline alignment.  It must be something else.  Maybe it's
a struct shared with userspace, and we used to put values in it.  I
don't know, I haven't looked, but a lack of imagination on your part
isn't an excuse to break an ABI.

> > If it's part of an ABI, you won't notice bugs caused until some
> > random application happens to depend on it.  If that area's being
> > used as scratch space for something else, you'll get buffer
> > overruns, and subtle corruptions.
> 
>   i'm sure i would, which is why, after *i* spend some time looking at
> it, i submit a patch to LKML to see if *someone else* can clear up the
> mystery.  and if they can, i drop it and move on.  what's your
> problem?

That you're creating work for someone else to do.  That's not the role
of a janitor.  Your job is to investigate this issue thoroughly.  Become
an expert on this little piece of the puzzle.  Don't look for someone
else to tell you whether or not you can do something -- figure it out!
Argue with people *when you are sure you know better than them*.

> > You need to go and look at *all* uses of inodes_stat, and figure out
> > whether removing it is safe.
> 
>   in fact, i did just that, grepping through the tree to see if i
> could spot any definition of that structure for which the "dummy"
> array was referenced, and i didn't find one.  doesn't mean there
> isn't one, only that i didn't notice one.

It's not a question of whether the dummy array was used directly by
those code paths, it's a question of whether it was subtly, indirectly
used.  Maybe by some piece of code that isn't part of the kernel (the
aforementioned userspace example).

> > Don't just post a patch; you've then put the onus of proving you're
> > right on someone else.
> 
> and the problem with that is ... what exactly?  if i propose removing
> something that looks useless, and not a single person on LKML can
> explain what it's actually being used for, i think i've made my case.

It doesn't scale.  You have to do as much work as possible in order to
avoid creating congestion at higher levels.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ] why does struct inodes_stat_t include "int dummy[5]"?

[Florian Westphal]

Robert P. J. Day <rpjday@mindspring.com> wrote:
>   in fact, i did just that, grepping through the tree to see if i
> could spot any definition of that structure for which the "dummy"
> array was referenced, and i didn't find one.  doesn't mean there
> isn't one, only that i didn't notice one.

inode stats are exported via /proc/sys/fs/inode-stats, see
Documentation/sysctl/fs.txt

(on a side note, the docs say there are four dummy values:
'preshrink' got removed and there are now 5 dummies).
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors