[LU-9490] MPI-IO Lustre ADIO driver gets Lustre layout parameters incorrectly

• Status: Resolved
• Project: Lustre
• Affects Version/s: Lustre 2.10.0
• Fix Version/s: Lustre 2.10.0
• Type: Bug
• Priority: Critical
• Reporter: Andreas Dilger
• Assignee: Emoly Liu
• Resolution: Fixed
• Severity: 3

Description

Looking at the Lustre ADIO driver, it appears that it is retrieving the layout parameters (stripe count, stripe size) incorrectly from the kernel. It is calling an ioctl() directly to the kernel, but then not checking that the returned layout magic is something that it understands before dereferencing the fields of struct lov_mds_md. For composite files that for not have struct lov_mds_md directly at the start of the layout, this will deteference random fields in the layout and return garbage to the library.

Issue Links

• is related to: LU-9349 PFL known issues tracking ticket
• is related to: LU-10232 kernel BUG at cl_object.c:206!
• is related to: LU-7964 tracking whether there is a direct ap...
• is related to: LU-9550 Delete the old Lustre ADIO driver pat...

Comments

[Andreas Dilger]

In particular: https://trac.mpich.org/projects/mpich/browser/src/mpi/romio/adio/ad_lustre/ad_lustre_open.c?rev=f818af4a15b0a168ecab7f73baf0491c2baed26b

void ADIOI_LUSTRE_Open(ADIO_File fd, int *error_code)
{
	:
	:
	/* get file striping information and set it in info */
	lum.lmm_magic = LOV_USER_MAGIC;
	err = ioctl(fd->fd_sys, LL_IOC_LOV_GETSTRIPE, (void *) &lum);

	if (!err) {
		sprintf(value, "%d", lum.lmm_stripe_size);
		MPI_Info_set(fd->info, "striping_unit", value);

		sprintf(value, "%d", lum.lmm_stripe_count);
		MPI_Info_set(fd->info, "striping_factor", value);
	
		sprintf(value, "%d", lum.lmm_stripe_offset);
		MPI_Info_set(fd->info, "start_iodevice", value);
	}

Some options that I see to handle this:

• set LOV_USER_MAGIC = LOV_USER_MAGIC_COMP_V1 in lustreapi and then detect this in the kernel LL_IOC_LOV_GETSTRIPE handling, and return a fake LOV_USER_MAGIC_V1 xattr to the caller. I don't think this will be very robust.
• fix ADIO_Lustre to check the returned magic value and then interpret the returned composite xattr based on the magic
• fix ADIO_Lustre to use llapi_layout_get_by_path(), which creates a dependency on liblustreapi (not sure if this is an issue, is it always installed on Lustre clients?)

It appears that we have similar problems in our own code, for example any program that calls llapi_file_get_stripe() will always return struct lov_user_md to the caller. For better or worse, this will result in lum->lmm_stripe_count, lum->lmm_stripe_size, and lum->lmm_stripe_offset being zero, since these fields overlap with lcm_padding2.

[Peter Jones]

Niu

Could you please assist with this issue?

Thanks

Peter

[Andreas Dilger]

It looks like lfs.c::lfs_migrate() is calling llapi_file_get_stripe() to get the stripe size, and this should be changed to use llapi_layout_get_by_path() and the stripe count extracted from the returned layout. One option is to set the component pointer to be the last initialized component, so that attributes returned are what the user would expect if they don't understand composite files.

[James A Simmons]

Yep we hit this problem during our PFL test shot. Prevented us from running one of our apps.

[Andreas Dilger]

James, could you please include the output from the Cray MPI-IO test run, since it was referencing a function that does not exist in the upstream MPICH code.

[Niu Yawei (Inactive)]

set LOV_USER_MAGIC = LOV_USER_MAGIC_COMP_V1 in lustreapi and then detect this in the kernel LL_IOC_LOV_GETSTRIPE handling, and return a fake LOV_USER_MAGIC_V1 xattr to the caller. I don't think this will be very robust.

Yes, I also don't think it will work well. The fake v1 xattr can't provide accurate stripe information for ADIO to leverage on, imaging FLR or other more complex layout introduced later, it'll be more messy with the fake v1. If some application cares about Lustre file layout, it has to understand the layout anyway.

fix ADIO_Lustre to check the returned magic value and then interpret the returned composite xattr based on the magic
fix ADIO_Lustre to use llapi_layout_get_by_path(), which creates a dependency on liblustreapi (not sure if this is an issue, is it always installed on Lustre clients?)

I vote for using llapi_layout_xxx interfaces, but I don't know how it should be fixed, should we inform the maintainer to update the ADIO driver?

It appears that we have similar problems in our own code, for example any program that calls llapi_file_get_stripe() will always return struct lov_user_md to the caller. For better or worse, this will result in lum->lmm_stripe_count, lum->lmm_stripe_size, and lum->lmm_stripe_offset being zero, since these fields overlap with lcm_padding2.

I see only some places used llapi_file_get_stripe() (or call the ioctl) directly:

• 'lfs find', 'lfs getstripe' calls the ioctl directly, but we've handled it properly.
• 'lfs migrate' calls llapi_file_get_stripe() directly and uses result for v1/v3 only, not an error but can be improved.
• ll_dirstripe_verify.c calls llapi_file_get_stripe() directly, and it's used in sanity.sh, that'll cause test failures when root default layout is set as composite, I think it should be handled by LU-9324. (or already being handled?)

[James A Simmons]

This is the error we saw during the test shot:

Assertion failed in file /notbackedup/users/ulib/mpt_base/mpich2/src/mpi/romio/adio/ad_cray/ad_cray_adio_open.c at line 474: _mpiio_o_lov_delay_create != 0

[Andreas Dilger]

Niu, it looks like ll_dirstripe_verify.c is not handling non-v1/v3 layouts properly. It is probably just comparing "0 == 0" in this case. We should probably deprecate the use of llapi_file_get_stripe() via _attribute_((deprecated)) to start (compile time), then via a message printed out in 2.11.

In the meantime, it probably makes sense to have it return something reasonable for PFL files when used, such as the plain layout of the last component (if file has no size) and the last init component if it has a size.

Separately, Emoly, can you please work with Niu to create and push a patch for MPICH, as I believe you worked on this code originally:
http://git.mpich.org/mpich.git/blob/HEAD:/src/mpi/romio/adio/ad_lustre/ad_lustre_open.c It should really be using the llapi_layout_* interface. I don't know if applications pass in a hint for the file size, but if they do it would make it easy to decide which component to use for the returned layout parameters.

However, for existing applications that call the ioctl directly, we may still need to return something reasonable from the kernel, since otherwise every MPI-IO application running on a new version of Lustre will fail if it hasn't been patched. One option is to detect the magic value passed to the kernel LL_IOC_LOV_GETSTRIPE handling in lov_getstripe(), and if LOV_MAGIC_V1/V3 return the last init component, and only return the full composite layout if the caller passes in LOV_MAGIC_COMP_V1? This would add complexity to the kernel, but would avoid compatibility issues with older applications, at the expense of returning something useful to userspace rather than causing outright application failures.

[Niu Yawei (Inactive)]

Niu, it looks like ll_dirstripe_verify.c is not handling non-v1/v3 layouts properly. It is probably just comparing "0 == 0" in this case. We should probably deprecate the use of llapi_file_get_stripe() via attribute((deprecated)) to start (compile time), then via a message printed out in 2.11.
In the meantime, it probably makes sense to have it return something reasonable for PFL files when used, such as the plain layout of the last component (if file has no size) and the last init component if it has a size.

Ok, sure. What do you mean "via a message printed out in 2.11."?

However, for existing applications that call the ioctl directly, we may still need to return something reasonable from the kernel, since otherwise every MPI-IO application running on a new version of Lustre will fail if it hasn't been patched. One option is to detect the magic value passed to the kernel LL_IOC_LOV_GETSTRIPE handling in lov_getstripe(), and if LOV_MAGIC_V1/V3 return the last init component, and only return the full composite layout if the caller passes in LOV_MAGIC_COMP_V1? This would add complexity to the kernel, but would avoid compatibility issues with older applications, at the expense of returning something useful to userspace rather than causing outright application failures.

I'm wondering if it's better to return error instead of the last instantiated component?

I suppose application usually calls getstripe on open/create file, so the first component will be returned in most cases, then optimizing based on first component's attributes will highly likely get opposite result. If we return an error instead of last instantiated component, app may either fallback to non-optimizing default setting (which is better than optimizing on first component), or error out (so that user will know it's time to upgrade).

BTW, except ADIO, I didn't see other callers passing lmm magic to LL_IOC_LOV_GETSTRIPE ioctl, so this workaround only works for the apps which providing lmm magic.

[Andreas Dilger]

What do you mean "via a message printed out in 2.11."?

I mean that llapi_file_get_stripe() should start to print out a warning about calling this function in 2.11, and that they should change to using llapi_layout_get_by_path() instead.

I'm wondering if it's better to return error instead of the last instantiated component? I suppose application usually calls getstripe on open/create file, so the first component will be returned in most cases, then optimizing based on first component's attributes will highly likely get opposite result.

That's why I suggested to "have it return something reasonable for PFL files when used, such as the plain layout of the last component (if file has no size) and the last init component if it has a size." That would return the last component if the file was just created.

Another alternative is to return the fields of interest in the current padding fields:

struct lov_user_md_v1 {           /* LOV EA user data (host-endian) */
        :
        __u32 lmm_stripe_size;    /* size of stripe in bytes */
        __u16 lmm_stripe_count;   /* num stripes in use for this object */
        __u16 lmm_layout_gen;     /* layout generation number
                                   * used when reading */

These map to the __u64 lcm_padding2 in struct lov_comp_md_v1, which is currently unused. If this ioctl() is called, it would be possible to extract those values from the proper component, temporarily store them into lov_user_md_v1, and return it to userspace. This will likely allow 99% of apps to continue using Lustre without any change.

If course, Ideally we will get a proper fix into the hands of the users, but I think there also needs to be a workaround in place so that sites which don't apply the fix.

[Gerrit Updater]

Niu Yawei ([e-mail]) uploaded a new patch: https://review.whamcloud.com/27180
Subject: LU-9490 tests: ll_dirstripe_verify handles PFL layout
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: ecf66cbfaf2e5734a18098db77882a344c944f9f

[Gerrit Updater]

Niu Yawei ([e-mail]) uploaded a new patch: https://review.whamcloud.com/27183
Subject: LU-9490 llite: return v1/v3 layout for legacy app
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 87da9d4b51f6874412c46ff130b36e938fa85fe4

[Andreas Dilger]

We still need a fix for MPICH ADIO, otherwise it will never work properly with composite layouts. The current solution allows it to function, but it won't generate any optimal IO patterns.

[Eric Bergland (Inactive)]

Emoly to work on ADIO driver ww21

[Emoly Liu]

I will look into this one.

[Emoly Liu]

adilger, I'm cloning MPICH git repo. Do you mean I need to create a patch there?
BTW, the lustre ADIO driver patch in our repo (lustre/contrib/adio_driver_mpich2-1.0.7.patch) is too old, I remembered it was released by MPICH2 about 8 years ago. Should it be removed?

[Andreas Dilger]

Emoly, yes the fix should be submitted to the MPICH Git repo. The ADIO patches in lustre/contrib should be deleted.

[Emoly Liu]

Discussed with Niu and worked on how to apply each component stripe information to the I/O pattern redistribution.

[Gerrit Updater]

Andreas Dilger ([e-mail]) uploaded a new patch: https://review.whamcloud.com/27310
Subject: LU-9490 llite: return v1/v3 layout for legacy app
Project: fs/lustre-release
Branch: pfl
Current Patch Set: 1
Commit: f3e00e27e8f86be9400df19f074b23da6d2b17fb

[Gerrit Updater]

Andreas Dilger ([e-mail]) merged in patch https://review.whamcloud.com/27310/
Subject: LU-9490 llite: return v1/v3 layout for legacy app
Project: fs/lustre-release
Branch: pfl
Current Patch Set:
Commit: fac27756d5f2a283a5ec7a908e6434d30748a6bd

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch https://review.whamcloud.com/27183/
Subject: LU-9490 llite: return v1/v3 layout for legacy app
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 545f464b68ee21ccc425113db9b57c63ac2840b1

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch https://review.whamcloud.com/27180/
Subject: LU-9490 tests: ll_dirstripe_verify handles PFL layout
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: c1359f54314084e0613a5ae70168595f296b7dfc

[Peter Jones]

Lustre changes all landed for 2.10. ADIO changes being tracked separately

[Eric Bergland (Inactive)]

ADIO changes being tracked in LU-9657