<!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 rob,<br>
<br>
Thanks for reviewing the patch so carefully. Sorry for this delay reply.<br>
<br>
Robert Latham wrote:
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">On Fri, Nov 28, 2008 at 01:33:39PM +0800, emoly.liu wrote:
  </pre>
  <blockquote type="cite">
    <pre wrap="">Hello,

Per Rob's comment, I send the Lustre ADIO patch here. FYI, the brief  
introduction is given below.
Any advice is welcome.
    </pre>
  </blockquote>
  <pre wrap=""><!---->
Hi LiuYing

I finally had a chance to look at your patch.  Sorry it took me so
long.  I'm glad to see the Lustre folks taking MPI-IO more seriously.  

First off, the patch looks pretty good.  It's clear why the approaches
you're taking give you the improvements in MPI-IO performance.
Naturally, I've got some comments.

Your patch modifies or creates 11 files.  I'll go through it file by
file and tell you what I think.    I've also copied Wei-keng Liao from
Northwestern.  He has done a lot of work tuning ROMIO for lustre and
he might have some comments or suggestions, as he has done some of
these things in a slightly different way.  Weikuan, I presume you
worked with LiuYing on this patch?  If not, any comments from you?

OK, here we go!

 ad_lustre/Makefile.in
- just fine. building new objects

 ad_lustre/README 
- I'm happy to have this, and you might want to add even more content
  </pre>
</blockquote>
yes<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap=""> ad_lustre/ad_lustre.c
- Just a small change to call the two new lustre-specific strided and
  strided-collective routines

 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>
<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 cite="mid:20090108012145.GQ9086@mcs.anl.gov" 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>
Some comments are not suitable to lustre adio driver and some were
probably removed during the codes cleanup. I will restore the latter.<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">- here again, you should refer to ROMIO hint struct instead of calling
  MPI_Info functions
  </pre>
</blockquote>
<font color="#000000">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?<br>
If I misunderstand, please advise.<br>
</font>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" 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>
yes, that would be better if there is a separation. <br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">- same for Calc_others_req.  I see you want to change ROMIO policy,
  and that makes sense for you, but too bad we don't have a better way
  than for you to copy huge chunks of romio code only to make small
  modifications.
  </pre>
</blockquote>
yes, in Calc_others_req we try to reduce MPI_Alltoall operations to get
some performance improvement.<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap=""> ad_lustre/ad_lustre_hints.c
- new hints: please prefix lustre-specific with 'lustre' or
  'romio_lustre'  Yes, we are inconsistent about this, so you probably
  learned this bad habbit from us.  Please do as I say, not as I do
  (or have done in the past :> )
  </pre>
</blockquote>
sure, I will. <span class="moz-smiley-s1"><span> :-) </span></span><br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">- Also, note that for all the ROMIO hints, we add to an internal-to
  ROMIO hint structure.  It's a bit faster to read from these
  "natural" hints instead of going through the MPI_Info interface and
  parsing strings.
  </pre>
</blockquote>
mentioned above<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap=""> ad_lustre/ad_lustre_open.c
-  looks fine.  I'm a little concerned about rank 0 setting
   stripe/stride with ioctl while everyone sits in barrier.  ADIO_Open
   will call lustre_open with COMM_SELF in create case, but that
   doesn't help since you need to be able to adjust on open.  What
   happens when you adjust an already-created file?
  </pre>
</blockquote>
<font color="#000000">You're right.<br>
We shouldn't set lustre striping info only according to O_CREAT flag
because fd will return a wrong value for an existing file.<br>
I will fix it. Thanks.<br>
</font>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">  ad_lustre/ad_lustre_rwcontig.c  
- just new copyright... but nothing changed?
  </pre>
</blockquote>
Sorry, I will remove it.<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap=""> ad_lustre/ad_lustre_wrcoll.c

- this is more like a fork of adio/common/ad_write_str.c and not a
  copy.  Watch out for changes we've made in adio/common. I can make
  an honest effort to change ad_lustre when we change adio/common, but
  we don't have a lustre test system.
- missing some 64-bit Aint casting
  </pre>
</blockquote>
I will add it.<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">- took out most of the comments from adio/common/ad_write_coll.c -- why?
  </pre>
</blockquote>
I will restore some.<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">- missing the "new MPE" logging anthony put into rest of ROMIO
  </pre>
</blockquote>
I had a look at MPE events defined in adioi.h. Could you tell me what
else MPE logs you hope to see?<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">- yet another implementation of "rounding domains to stripe size".  
- interesting: disable data sieving in two-phase
  </pre>
</blockquote>
In our tests, data sieving showed bad
collective write performance for some kinds of workloads. To avoid
this, we define ds_in_coll hint, distinguished from the one in
independent I/O.<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">- optimization: instead of MPI_Info_get, use ROMIO hint struct.
  </pre>
</blockquote>
fd->hints<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">- missing wei-keng's reworked type processing 
  </pre>
</blockquote>
<font color="#000000">Sorry, I checked the codes but I didn't find
anything missed commented by wei-keng.<br>
Could you tell me what type processing is missed in this file?<br>
</font>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap=""> 

 ad_lustre/ad_lustre_wrstr.c
- some formating changes
- also missing some 64-bit Aint casts
  </pre>
</blockquote>
I will add it.<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap=""> common/ad_write_coll.c          
- make ADIOI_Heap_merge "public" to ROMIO.  That's fine: sure beats copying it
  </pre>
</blockquote>
<span class="moz-smiley-s1"><span> :-) </span></span><br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">So, for the next steps, could you at the very least do the following:
- change the hint names 
- use the fd->hints structure instead of MPI Info routines when you
  are strictly internal to ROMIO.  Continue to populate the users
  MPI_INFO.
  </pre>
</blockquote>
OK, I will refine the patch per your comments and send it to you later.<br>
<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">Other than that, I'll commit this to our trunk.  I will need Sun and
Oak Ridge's help to keep the ad_lustre driver up to date.   

Ideally, we would have most of the ad_lustre functionality as "tunable
knobs" in the common/ code.  That's a matter of software engineering
we just haven't tackled yet, so a separate ad_lustre is just fine for
now.

Thanks for the patch!
  </pre>
</blockquote>
If I misunderstand or miss anything, please let me know.<br>
Thank you so much! <br>
<br>
LiuYing<br>
<blockquote cite="mid:20090108012145.GQ9086@mcs.anl.gov" type="cite">
  <pre wrap="">==rob

  </pre>
  <blockquote type="cite">
    <pre wrap="">-------- Original Message --------
Subject:        Re: [Lustre-discuss] [Lustre-devel] a new Lustre ADIO driver
Date:   Wed, 12 Nov 2008 11:35:48 -0600
From:   Robert Latham <a class="moz-txt-link-rfc2396E"
 href="mailto:robl@mcs.anl.gov"><robl@mcs.anl.gov></a>
To:     emoly.liu <a class="moz-txt-link-rfc2396E"
 href="mailto:Emoly.Liu@Sun.COM"><Emoly.Liu@Sun.COM></a>
CC:     lustre-discuss <a class="moz-txt-link-rfc2396E"
 href="mailto:lustre-discuss@lists.lustre.org"><lustre-discuss@lists.lustre.org></a>,  
<a class="moz-txt-link-abbreviated"
 href="mailto:lustre-devel@lists.lustre.org">lustre-devel@lists.lustre.org</a>
References:     <a class="moz-txt-link-rfc2396E"
 href="mailto:49110B1A.5030201@sun.com"><49110B1A.5030201@sun.com></a> <a
 class="moz-txt-link-rfc2396E" href="mailto:49114151.10908@sun.com"><49114151.10908@sun.com></a>



On Wed, Nov 05, 2008 at 02:46:41PM +0800, emoly.liu wrote:
    </pre>
    <blockquote type="cite">
      <pre wrap="">Hi,

Here is some description and performance results for the new Lustre 
ADIO  driver. You might be interested.

The main purpose of the new lustre adio driver is to collect the small  
size I/O requests into fewer number of larger ones according to 
Lustre's  striping feature with low overhead automatically. We also 
provide some  hints for performance tuning.

In the following graph, the recent FLASH I/O benchmarking result shows  
that the new driver performs much better than the old one, especially   
for a large-scale system.
In this test, FLASH I/O blocksize is 4*4*4 that means each 3 clients   
write 40KB, 40.5KB and 42KB respectively. The I/O operation iteration   
(nvar) is set to 1024.
      </pre>
    </blockquote>
    <pre wrap="">
Hi LiuYing

Since this patch is for ROMIO and not really for Lustre, could you
send this to <a class="moz-txt-link-abbreviated"
 href="mailto:romio-maint@mcs.anl.gov">romio-maint@mcs.anl.gov</a> ?  It will reach more of the
ROMIO developers that way.

Thanks
==rob

    </pre>
  </blockquote>
  <blockquote type="cite"> </blockquote>
  <pre wrap=""><!----> </pre>
</blockquote>
<pre class="moz-signature" cols="72">-- 
Best regards,

LiuYing
System Software Engineer, Lustre Group
Sun Microsystems ( China ) Co. Limited
</pre>
</body>
</html>