<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
Hi Robert,<br>
<br>
Here is the new lustre adio driver patch. I fixed the following problem
per our discussion:<br>
<ul>
<li>change the hints name</li>
<ul>
<li>from xxx to romio_lustre_xxx<br>
</li>
</ul>
</ul>
<ul>
<li> use the fd->hints structure instead of MPI Info routines</li>
<ul>
<li>define a struct for lustre in ADIOI_Hints_struct in
adioi.h and replace the old MPI_Info_get calls with the new romio
hints<br>
</li>
</ul>
</ul>
<ul>
<li>check lustre/lustre_user.h header file in configure instead of
giving the lustre structs/constants</li>
<ul>
<li>define AC_CHECK_HEADERS in romio/configure.in. If the header
file doesn't exist, report AC_MSG_ERROR<br>
</li>
</ul>
</ul>
<ul>
<li>restore mis-removed comments</li>
</ul>
<ul>
<li>new MPE logging</li>
<ul>
<li>add MPE logging for read/write in ad_lustre_rwconfig.c<br>
</li>
</ul>
</ul>
<ul>
<li>fix the issue in ad_lustre_open.c</li>
</ul>
<br>
I tested the new driver on less than 10 nodes with IOR benchmark.<br>
<br>
Please check and if you have any questions, please let me know.<br>
<br>
<br>
Thanks,<br>
LiuYing<br>
<br>
Robert Latham wrote:
<blockquote cite="mid:20090223230910.GA14717@mcs.anl.gov" type="cite">
<pre wrap="">Hi. Just wanted to see how things are going with this patch. I think
the MPICH2 folks are looking to start the release process in the next
few weeks, and I'd like to get this patchset into that distribution.
Thanks
==rob
On Tue, Jan 13, 2009 at 01:52:59PM -0600, Robert Latham wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi LiuYing. Thanks for giving my comments such consideration. I've
only responded to a few points below inline:
On Wed, Jan 14, 2009 at 12:16:34AM +0800, emoly.liu wrote:
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap=""> ad_lustre/ad_lustre.h
- bring in lustre structs/constants if "lustre_user.h" not getting
brought in: is this really necessary? ROMIO won't try to build
ad_lustre unless 'lustre' is part of "--with-file-systems" at
configure time
</pre>
</blockquote>
</blockquote>
<blockquote type="cite">
<pre wrap="">--with-file-systems=lustre only tells it will build lustre adio, but
it doesn't show where is the lustre_user.h, which is needed by
ad_open.c (for setting stripe stuff). So if there is no
lustre_user.h in /user/include (no lustre-devel rpm in your system),
hmm, maybe we should only give error msg at this moment, instead of
adding this if.
</pre>
</blockquote>
<pre wrap="">This is sort of what we do for PVFS and PVFS2: if the user explicitly
asks for pvfs or pvfs2, and ROMIO configure.in cannot find the
necessary header files, we AC_ERROR. I'd like to see the same
behavior for lustre too.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap=""> ad_lustre/ad_lustre_aggregate.c - Lots of changes. I see you started
from adio/common/ad_aggregate.c
- ADIOI_LUSTRE_Docollect: interesting policy to avoid collective I/O
if any piece bigger than a big_req hint.
- You removed a lot of comments. why?
</pre>
</blockquote>
<pre wrap="">Some comments are not suitable to lustre adio driver and some were
probably removed during the codes cleanup. I will restore the latter.
</pre>
</blockquote>
<pre wrap="">I can certainly appreciate the need to remove misleading comments.
All the ROMIO maintainers will tell you, though, that this code is
tricky and we look at it infrequently enough that it's really pretty
hard to over-comment it.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">- here again, you should refer to ROMIO hint struct instead of calling
MPI_Info functions
</pre>
</blockquote>
<pre wrap="">Sorry, I am not sure what ROMIO hint structure you mean. I assume it is
ADIOI_Hints_struct. I can add lustre hints to that struct, but they are
lustre related, not common. Is that ok?
If I misunderstand, please advise.
</pre>
</blockquote>
<pre wrap="">Sure. I'll be happy to explain further.
You're on the right track. In adioi.h we have ADIOI_Hints_struct.
This struct contains many fs-independent values (cb_buffer_size,
striping_unit), but it also contains a union for fs-specific values.
Right now, that union only has values for pvfs and pvfs2, but we
should add another member for lustre.
Here's an example of ROMIO using the hints in this structure
(adio/ad_pvfs2/ad_pvfs2_read.c)
if (fd->hints->fs_hints.pvfs2.listio_read == ADIOI_HINT_ENABLE) {
ret = ADIOI_PVFS2_ReadStridedListIO(fd, buf, count, datatype,
file_ptr_type, offset, status, error_code);
return;
}
No need to extract the "romio_pvfs2_listio" keyval from the MPI_INFO
member, no need to strcasecmp the info value. Those steps were all
done at MPI_FILE_OPEN or MPI_FILE_SET_VIEW time. It's a small
optimization, and what you do now is correct, but making use of
ADIOI_Hints_struct will make your code more like the other drivers.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">- maybe we need, as wei-keng suggests, a better separation so you
don't need to duplicate Calc_my_req just so you can call the
lustre-sepecific Calc_aggregator. Could a hint do this for you?
</pre>
</blockquote>
<pre wrap="">yes, that would be better if there is a separation.
</pre>
</blockquote>
<pre wrap="">OK, glad we're on the same page. I don't think you need to do this
bit of software engineering for your patch. It's more a note to
ourselves: if we ever redesign ROMIO, we'll have to be sure to make
this part of it.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">- missing the "new MPE" logging anthony put into rest of ROMIO
</pre>
</blockquote>
<pre wrap="">I had a look at MPE events defined in adioi.h. Could you tell me what
else MPE logs you hope to see?
</pre>
</blockquote>
<pre wrap="">Take a look at some of the other files that have
#ifdef ADIOI_MPE_LOGGING
lines.
(Unfortunately, there are still some legacy MPE lines in the code,
too. I'll have to delete them)
The events in adioi.h are exactly what I'd like to see. I see some
lustre files already have some of them.
Here's an example of a modern call (do it like this):
MPE_Log_event( ADIOI_MPE_open_a, 0, NULL );
and here's an ancient call (don't do this):
MPE_Log_event(9, 0, "start close");
In the end, though, this is for developers to see what's going on
inside ROMIO with JUMPSHOT. it's not critical, though you might find
it helpful when you are looking for answers to performance questions.
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">- missing wei-keng's reworked type processing
</pre>
</blockquote>
<pre wrap="">Sorry, I checked the codes but I didn't find anything missed
commented by wei-keng. Could you tell me what type processing is
missed in this file?
</pre>
</blockquote>
<pre wrap="">This one is a bit hard to describe, because the initial work went into
revision 958, but then we did some testing and refining for several
more revisions before settling on a final version. It's ok if you
don't incorporate this change, but if you're motivated, svn revision
1001 makes some changes to ad_read_coll which are demonstrative of the
overall changes also needed in ad_write_coll, ad_write_str, and
ad_read_str.
In ad_lustre, you have one file for "io", so you have already reduced
your work by half. Good.
</pre>
<blockquote type="cite">
<pre wrap="">If I misunderstand or miss anything, please let me know.
Thank you so much!
</pre>
</blockquote>
<pre wrap="">Thank you for the good discussion. I'm looking forward to seeing the
next revision.
==rob
</pre>
</blockquote>
<pre wrap=""><!---->
</pre>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Best regards,
LiuYing
System Software Engineer, Lustre Group
Sun Microsystems ( China ) Co. Limited
</pre>
</body>
</html>