Hi, We at Xyratex are needing to develop some auxiliary utilities that need internal data structures from lustre. To accomplish this, our sources have been reaching into the lustre build tree for the necessary headers. Not being a particular fan of that approach, I developed a patch for lustre that will create another RPM: a lustre-devel package. It builds the packages and drops the headers into a tree that looks like this: usr/include/lustre usr/include/lustre/darwin usr/include/lustre/linux usr/include/lustre/lnet usr/include/lustre/lnet/darwin usr/include/lustre/lnet/linux usr/include/lustre/lnet/winnt usr/include/lustre/libcfs usr/include/lustre/libcfs/darwin usr/include/lustre/libcfs/linux usr/include/lustre/libcfs/winnt usr/include/lustre/libcfs/util usr/include/lustre/libcfs/posix usr/include/linux usr/include/libcfs/posix one file is dropped into include/libcfs/posix and one file into include/linux. I''d have preferred not doing that, but the old code already did. 186 headers get installed. The other files installed as part of lustre-devel are object archives and library interface man pages: usr/lib64/libcfsutil.a usr/lib64/liblustreapi.a usr/lib64/liblustre.a usr/lib64/libptlctl.a usr/lib64/libiam.a usr/share/lustre/mpich-1.2.6-lustre.patch usr/share/man/man3/llapi_quotactl.3.gz usr/share/man/man3/llapi_file_open.3.gz usr/share/man/man3/llapi_file_create.3.gz usr/share/man/man3/llapi_file_get_stripe.3.gz The rest of the lustre installation is left with the main package. Below is my git-commit message. Under separate cover, I''ll post the git patch under the subject "[PATCH] lustre-devel packaging" * autoMakefile.am Constrain lines to 80 columns (readability) Per automake maintainers, configurables should be set to a make file macro before use. Enables "make" time replacement. * build/autoMakefile.am.toplevel Constrain lines to 80 columns (readability) * libcfs/include/Makefile.am Put every header under libcfs/include into nobase_pkginclude_HEADERS. * lnet/autoconf/lustre-lnet.m4 The body of an m4 (autoconf) macro is indented (readability). "LNET_MAX_PAYLOAD" is now a configured value lnet/include/lnet/types.h is now a configured file Alphabetize and indent the configured files * lnet/include/Makefile.am Collect the neaders into nobase_pkginclude_HEADERS and nodist_nobase_pkginclude_HEADERS (just lnet/types.h) * lnet/include/lnet/Makefile.am lnet/include/lnet/types.h is now a configured file * lnet/include/lnet/types.h Renamed to: * lnet/include/lnet/types.h.in Add configured #define for LNET_MAX_PAYLOAD and adjust CPP testing. * lustre.spec.in 354 columns in a single line is too much. Trim the egregious ones. add a "%package devel" directive In %install section, separate "devel" files from non-devel and list them in lustre-devel.files and lustre.files. * lustre/autoconf/lustre-core.m4 Add config.h #define for OBD_MAX_IOCTL_BUFFER and make it a substitution value as well. lustre/include/lustre/Makefile.am is removed. Do not configure. * lustre/include/Makefile.am The lustre/include/lustre/*.h headers get installed in pkginclude. Drop the rest into nodist_pkginclude_HEADER (lustre_ver.h) or nobase_pkginclude_HEADER (everything else). * lustre/include/linux/Makefile.am The EXTRA_DIST value is obsolete. * lustre/include/linux/lustre_acl.h Fix spelling of "Should not include directly". * lustre/include/lustre/Makefile.am Obsolete * lustre/include/lustre_sec.h Prefer "__u32" to "uint32_t". Eliminates need for inttypes.h. * lustre/include/lustre_ver.h.in Add configured #define OBD_MAX_IOCTL_BUFFER * lustre/utils/Makefile.am Move obd.c and lustre_cfg.c sources to convenience library. --- autoMakefile.am | 36 ++- build/autoMakefile.am.toplevel | 17 +- libcfs/include/Makefile.am | 8 + lnet/autoconf/lustre-lnet.m4 | 102 ++++---- lnet/include/Makefile.am | 10 + lnet/include/lnet/Makefile.am | 2 +- lnet/include/lnet/types.h | 509 ------------------------------------- lnet/include/lnet/types.h.in | 505 ++++++++++++++++++++++++++++++++++++ lustre.spec.in | 156 ++++++++---- lustre/autoconf/lustre-core.m4 | 13 +- lustre/include/Makefile.am | 71 +++++- lustre/include/linux/Makefile.am | 7 - lustre/include/linux/lustre_acl.h | 2 +- lustre/include/lustre/Makefile.am | 42 --- lustre/include/lustre_sec.h | 4 +- lustre/include/lustre_ver.h.in | 23 +- lustre/utils/Makefile.am | 12 +- 17 files changed, 814 insertions(+), 705 deletions(-) === 500 insertions and 500 deletions from file move. === patch actually 300 inserts and 200 deletes.
On 2011-11-14, at 7:11 AM, Bruce Korb wrote:> We at Xyratex are needing to develop some auxiliary utilities that > need internal data structures from lustre. To accomplish this, our > sources have been reaching into the lustre build tree for the > necessary headers.That is messy, I agree.> Not being a particular fan of that approach, I developed a patch for > lustre that will create another RPM: a lustre-devel package.I like the idea of having a separate lustre-devel package, since this is fairly standard practise for other libraries and such. My main question would be - do you _need_ to have access to all of the headers that you included, or did you simply include all of the headers because that was the easiest thing to do? Doing a simple check on the current master tree, it appears you just copied all of the headers in libcfs/include, lnet/include, and lustre/include (which total 186 files). By just copying everything into "public" headers, it is introducing a maintenance nightmare, because it is no longer clear which APIs, structs, constants, ioctls, etc. are private to Lustre or specific tools, and which ones might possibly be used by external tools and would cause those tools to break if they were changed for some reason.> The other files installed as part of lustre-devel are object > archives and library interface man pages: > > usr/lib64/libcfsutil.a > usr/lib64/liblustreapi.a > usr/lib64/liblustre.a > usr/lib64/libptlctl.a > usr/lib64/libiam.aNow that you post this list of libraries, it in fact appears that libiam is not even used by userspace at all anymore. From simple checking of lustre/utils/Makefile.am, it only shows mkfs_lustre.c as a potential user, and that doesn''t seem to use it at all. The creation of the object index "oi.16", which is the only IAM-format index in the filesystem, is done in the kernel since before Lustre 2.0 was released. So, rather than simply copying everything that is available, it would be much better to have a discussion about what APIs you are using (or which ones you wish would be available), and then implement llapi_* wrappers for those interfaces. This would at least give some level of abstraction from the low-level interfaces, and makes it much clearer which interfaces are actually in use by tools that are not part of the Lustre package, (and should try to remain relatively stable), and which interfaces, ioctls, structures, etc. are for internal use only (or sometimes not in use at all).> usr/share/lustre/mpich-1.2.6-lustre.patchI think a significantly improved version this patch was already included in the MPICH upstream release, and this one is obsolete.> usr/share/man/man3/llapi_quotactl.3.gz > usr/share/man/man3/llapi_file_open.3.gz > usr/share/man/man3/llapi_file_create.3.gz > usr/share/man/man3/llapi_file_get_stripe.3.gz> > Below is my git-commit message. Under separate cover, I''ll post the > git patch under the subject "[PATCH] lustre-devel packaging"Looking at the descriptions, the patches look quite reasonable. However, you need to submit patches to Gerrit in order to get them inspected, tested, and landed. Please see: http://wiki.whamcloud.com/display/PUB/Submitting+Changes and in particular: http://wiki.whamcloud.com/display/PUB/Using+Gerrit There are a number of people at Xyratex already following this process that you can likely ask for guidance. Cheers, Andreas -- Andreas Dilger Principal Engineer Whamcloud, Inc.
Hi Adreas, On 11/15/11 1:47 AM, Andreas Dilger wrote:>On 2011-11-14, at 7:11 AM, Bruce Korb wrote: >My main question would be - do you _need_ to have access to all of the >headers that you included, or did you simply include all of the headers >because that was the easiest thing to do? Doing a simple check on the >current master tree, it appears you just copied all of the headers in >libcfs/include, lnet/include, and lustre/include (which total 186 files).Lets assume I did a minimal approach and only included the necessary headers. Then, someone oblivious to which headers get exported and which do not, then added a new dependency into the headers. Everything builds and checks out so it looks right and you ship the new version. Except it isn''t because of the new inadvertent dependency. Oops. The correct and proper solution is for each component of lustre to explicitly copy interface headers into an include staging area with everything under that getting installed. I''m not going there. That is a future exercise. Probably ought to be done, though. OK, *surely* ought to be done. :)>By just copying everything into "public" headers, it is introducing a >maintenance nightmare, because it is no longer clear which APIs, structs, >constants, ioctls, etc. are private to Lustre or specific tools, and >which ones might possibly be used by external tools and would cause those >tools to break if they were changed for some reason.Were the private vs. public headers separated in some way, then the unwanted exports could be removed. At the moment, I think the assumption has to be that clients of the lustre-devel package would have to be friendly clients. Very friendly. Right now, we have actual clients grubbing around all over the lustre source tree in a very unfriendly way.>> The other files installed as part of lustre-devel are?.. >So, rather than simply copying everything that is available, it would be >much better to have a discussion about what APIs you are using (or which >ones you wish would be available), and then implement llapi_* wrappersUltimately, that is completely correct. For now, I''m mostly interested in getting headers and libraries installed in a way where I''m not dependent upon the build tree layout. It is already understood that if lustre changes internals these utilities have to adapt. Adapting semi-private interfaces to a coherent framework would be such a change. This change simply isolates our auxiliary utilities from changes in build layout. That''s step 1. I (we at Xyratex) would be completely okay with a usage caveat that the interfaces exposed are subject to change while the process of working out exported vs. completely private interfaces goes on.>>usr/share/lustre/mpich-1.2.6-lustre.patch > >I think a significantly improved version this patch was already included >in the MPICH upstream release, and this one is obsolete.The .spec file needs to adapt to whatever gets staged into BUILDROOT. Some of that may be hard coded and need changing, but the .spec file ought to be as scripted as possible, minimizing the need for any changes when the lustre subcomponents change the set of files they install.>> >> Below is my git-commit message. Under separate cover, I''ll post the >> git patch under the subject "[PATCH] lustre-devel packaging" > >Looking at the descriptions, the patches look quite reasonable. However, >you need to submit patches to Gerrit in order to get them inspected, >tested, and landed.I was actually starting with email before going to a formal review request. You have already seen the review request for changes I consider less controversial (see "P.S." below), even if the changes were too unfocused as a single patch.>Cheers, AndreasThank you!! Regards, Bruce P.S. the other issue (LU-483) got combined because of procedural issues. Sorry about that. I will do as you ask within a few days and break it up into several independent patches, but still under LU-483, yes?
Nathan Rutman
2011-Nov-15 15:43 UTC
[Lustre-devel] [lustre-devel] lustre-devel packaging - LU-482
On Nov 15, 2011, at 7:22 AM, "Bruce Korb" <bruce_korb at xyratex.com> wrote:> Hi Adreas, > > On 11/15/11 1:47 AM, Andreas Dilger wrote: > >> On 2011-11-14, at 7:11 AM, Bruce Korb wrote: >> My main question would be - do you _need_ to have access to all of the >> headers that you included, or did you simply include all of the headers >> because that was the easiest thing to do? Doing a simple check on the >> current master tree, it appears you just copied all of the headers in >> libcfs/include, lnet/include, and lustre/include (which total 186 files). > > > Were the private vs. public headers separated in some way, then > the unwanted exports could be removed. At the moment, I think > the assumption has to be that clients of the lustre-devel package > would have to be friendly clients. Very friendly. Right now, we > have actual clients grubbing around all over the lustre source > tree in a very unfriendly way.Actually I thought we were careful to only expose llapi_* functions. So what''s doing the grubbing?
Hi Andreas, I did do a little more due diligence: On 11/15/11 7:22 AM, Bruce Korb wrote:>>My main question would be - do you _need_ to have access to all of the >>headers that you included, or did you simply include all of the headers >>because that was the easiest thing to do? Doing a simple check on the >>current master tree, it appears you just copied all of the headers in >>libcfs/include, lnet/include, and lustre/include (which total 186 files). > >Lets assume I did a minimal approach and only included the necessarylibcfs/libcfsutil.h lnet/lnetctl.h lustre/liblustreapi.h lustre/lustre_idl.h test.h utils/obdctl.h These are the headers directly #include-d by our "utility" that live in the lustre tree. They likely pull in several more. As I said elsewhere somewhere, unless the libcfs, lnet, lustre and utils components of lustre know that these are exported via an unambiguous mechanism, installing only the minimal subset will be prone to problems. A better way is to have them stage the headers and run a validation that they are all both self-sufficient and idempotent, meaning that any requisite headers are also staged and they are all guarded with duplicate inclusion guards. This can be done with a trivial script. I can make this part of the "lustre-devel package" project. I was trying to limit scope by installing everything in sight. At least for now.
On 2011-11-16, at 14:22, Bruce Korb <bruce_korb at xyratex.com> wrote:> > I did do a little more due diligence: > > On 11/15/11 7:22 AM, Bruce Korb wrote: >>> My main question would be - do you _need_ to have access to all of the >>> headers that you included, or did you simply include all of the headers >>> because that was the easiest thing to do? Doing a simple check on the >>> current master tree, it appears you just copied all of the headers in >>> libcfs/include, lnet/include, and lustre/include (which total 186 files). >> >> Lets assume I did a minimal approach and only included the necessary > > libcfs/libcfsutil.h > lnet/lnetctl.h > lustre/liblustreapi.h > lustre/lustre_idl.h > test.h > utils/obdctl.h > > These are the headers directly #include-d by our "utility"Including liblustrapi.h is expected, since this is the entry point for the Lustre wrappers, and this header is already installed. I did some work several months ago to make lustre_idl.h usable from userspace for lfsck, which works OK except for the use of __u32/__u64 and friends, which needs the "types.h" header to define. I haven''t looked at the other headers (just about to get on a plane) but I think they might be acceptable for low-level Lustre specific applications. That said, the only previous user of these headers is probably lctl, and I''m hesitant to expose them as an "API". As Nathan wrote, we''ve tried in the past to do the right thing and create llapi wrappers for code that needs to poke into the Lustre kernel interfaces. This allows things like e.g. fixing the ioctl numbers, changing the data structures used with the kernel, etc. without having to modify the applications that are using these wrappers. I''m the first one to admit that the llapi_* wrappers do not address a large number of use cases, but that also won''t get better if nobody works to improve them. I guess a reasonable question at this point is what specifically you are trying to access? Is it (essentially) trying to link directly into lctl, lfs, etc? Access to wire protocol structures (that lustre_idl.h should handle), or something else entirely?> As I said elsewhere somewhere, unless the libcfs, lnet, lustre and utils > components of lustre know that these are exported via an unambiguous > mechanism, installing only the minimal subset will be prone to problems. > A better way is to have them stage the headers and run a validation that > they are all both self-sufficient and idempotent, meaning that any > requisite headers are also staged and they are all guarded with duplicate > inclusion guards. This can be done with a trivial script. > > I can make this part of the "lustre-devel package" project. I was trying > to limit scope by installing everything in sight. At least for now. > >
On Nov 17, 2011, at 05:35, Andreas Dilger wrote:> On 2011-11-16, at 14:22, Bruce Korb <bruce_korb at xyratex.com> wrote: >> >> I did do a little more due diligence: >> >> On 11/15/11 7:22 AM, Bruce Korb wrote: >>>> My main question would be - do you _need_ to have access to all of the >>>> headers that you included, or did you simply include all of the headers >>>> because that was the easiest thing to do? Doing a simple check on the >>>> current master tree, it appears you just copied all of the headers in >>>> libcfs/include, lnet/include, and lustre/include (which total 186 files). >>> >>> Lets assume I did a minimal approach and only included the necessary >> >> libcfs/libcfsutil.h >> lnet/lnetctl.h >> lustre/liblustreapi.h >> lustre/lustre_idl.h >> test.h >> utils/obdctl.h >> >> These are the headers directly #include-d by our "utility" > > Including liblustrapi.h is expected, since this is the entry point for the Lustre wrappers, and this header is already installed. > > I did some work several months ago to make lustre_idl.h usable from userspace for lfsck, which works OK except for the use of __u32/__u64 and friends, which needs the "types.h" header to define.that should be done via libcfs.h i think ?