[Lustre-discuss] [ROMIO Req #940] [Fwd: Re: [Lustre-devel] a new Lustre ADIO driver]

[Robert Latham]

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:
> 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:
> >>  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
> 
> > --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.
> 
> 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.  
> 
> >>   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?
> >>   
> > Some comments are not suitable to lustre adio driver and some were  
> > probably removed during the codes cleanup. I will restore the latter.
> 
> 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.
> 
> >> - here again, you should refer to ROMIO hint struct instead of calling
> >>   MPI_Info functions
> >>   
> > 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.
> 
> 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.
> 
> >> - 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?
> >>   
> > yes, that would be better if there is a separation.
> 
> 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.
> 
> >> - missing the "new MPE" logging anthony put into rest of ROMIO
> >>   
> > I had a look at MPE events defined in adioi.h. Could you tell me what  
> > else MPE logs you hope to see?
> 
> 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.
> 
> >> - missing wei-keng's reworked type processing   
> > 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?
> 
> 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.
> 
> > If I misunderstand or miss anything, please let me know.
> > Thank you so much!
> 
> Thank you for the good discussion.  I'm looking forward to seeing the
> next revision.
> 
> ==rob
> 

-- 
Rob Latham
Mathematics and Computer Science Division    A215 0178 EA2D B059 8CDF
Argonne National Lab, IL USA                 B29D F333 664A 4280 315B