Orangefs: the war on some drugs...

Orangefs: the war on some drugs...

[Mike Marshall]

Hi Al...

 AV> The member of that big fat union for getattr has a bit of additional
 AV> insanity - several pointer-sized gaps.  Entirely unused.

 AV> Far be it from me to support The War On Some Drugs, but really,
 AV> staying away from the stuff that induces trips _that_ bad is just
 AV> plain common sense...

Here's a trip (no pun intended) through a getattr request...

  - userspace reads a getattr request from /dev/pvfs2-req,
    basically "tell me about this orangefs_object_kref".

  - userspace fills out a PVFS_sys_attr struct, which contains
    lots of members, some of which aren't relevant to the kernel.

  - userspace fills out a pvfs2_downcall_t struct to use as the response
    to the kernel's getattr request. The pvfs2_downcall_t struct just
    gets a copy (struct assignment) of the already filled out
    PVFS_sys_attr struct in the getattr member of the big fat union.
    The kernel gets back what it needs, plus some stuff it doesn't need.

Userspace still uses "pvfs" everywhere, the upstream version of the
kernel module has been changed over to "orangefs" most everywhere, so
pvfs2_downcall_t in userspace is the same structure as
orangefs_downcall_s in the kernel module...

It appears that the original kernel module developers didn't care
that they were sending some irrelevant stuff back to the kernel when
they used these (and there are others) one-size-fits-all
kernel/userspace structures. I think these structures were
created by the userspace developers and just used by the
original kernel module developers - they existed already, and
they got filled out with the needed attributes during the
course of all getattr requests, not just getattr requests from the
kernel.

In some cases userspace sends back true junk to the kernel, as
in the case of the unused "type" in "struct orangefs_downcall_s",
and the unused "proto_ver" in one of the iovecs written back
to /dev/pvfs2-req.

In other cases, such as the "trailer_buf" member of
"struct orangefs_downcall_s", stuff is sent back that
is used but doesn't need to be sent in the downcall. In
the case of "trailer_buf", a pointer needs to exist, but
it could be declared local to orangefs_devreq_writev, it
doesn't need to be part of the downcall struct.

And then, there's the one-size-fits-all members of the big
fat union in orangefs_downcall_s, some of which contain
stuff that is irrelevant to the kernel.

It it is obvious to me that "true junk" should be identified and
eliminated.

Perhaps items such as "trailer_buf" should be eliminated in
favor of local variables.

It would be possible to create new structures to populate the
big fat union that were kernel specific and copy only the needed
attributes from userspace to the kernel. This change should be
made because:

  - sending irrelevant stuff wastes resources like memory and cpu.

  - sending irrelevant stuff clutters the code and makes it hard to
    read and maintain.

  - better data structures might be able to use techniques like the ones
    in "What every programmer should know about memory" to enhance
    the performance of the kernel module.

  - others reasons?

-Mike