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

[Robert Latham]

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