[LU-1346] cleanup libcfs

• Type: Improvement
• Resolution: Fixed
• Priority: Minor
• Fix Version/s: Lustre 2.4.0, Lustre 2.5.0
• Affects Version/s: Lustre 2.4.0, Lustre 2.5.0
• Labels: emc patch

Epic

client

• Assignee: Keith Mannthey (Inactive)
• Reporter: Peng Tao

Description

Migrate libcfs to emulate Linux kernel APIs, so that when submitting Linux client to upstream kernel, we don't need the abstraction layer which will be rejected by kernel maintainers. The majority of libcfs are cfs_* or ll_* wrappers, and can be cleaned up with scripts doing like "s/cfs_spin_lock/spin_lock". For other functions that cannot be cleaned up this way, we need to look at them in a case-by-case basis.

Issue Links

• is duplicated by: LU-3963 cleanup libcfs wrappers
• Trackbacks: Lustre Community Development in Progress Features are being developed for future Lustre releases both at Whamcloud and by other organizations in the Lustre community. These will be eligible for inclusion in future Lustre releases as per our processes

Comments

[Alexey Lyashkov]

I created patch with some cleanups need to compile lustre with clang. most of them is c89 style fixes with correct includes.
please inspect it.

http://review.whamcloud.com/7803

[Alexey Lyashkov]

Thanks! I will send maces/freebsd support patches with fuse client son (i hope in next two weeks).

[James A Simmons]

Okay we can revert the last patch. The caps patch had less of a impact on the code than the other patches for LU-1346. Proper capabilities support will need to be explored later.

[Alexey Lyashkov]

OS defined flags should be never applied to the custom type, so CAP_* flags may be applied to the kernel_cap type, other is incorrect by definition, but kernel_cap_t can't be put in _u32 in wire.
reason to have CFS_CAP* to be similar CAP_* defines - capability with older (before patch) clients, but we define only few bits from kernel bitmask, so if we will be need some additional on server side we will be able to pack data from kernel bits to unused bits in wire data.
so avoid a wire protocol changes as long as possible. It's my bad - i forget to add CFS_CAP checks into wiretest script to notify - it's wire specific data and should be don't changed without real reasons.

as about breakage.

In file included from /Users/shadow/work/lustre/work/WC-review/mac-os/libcfs/include/libcfs/posix/libcfs.h:108,
                from /Users/shadow/work/lustre/work/WC-review/mac-os/libcfs/include/libcfs/libcfs.h:45,
                from nidstrings.c:43:
/Users/shadow/work/lustre/work/WC-review/mac-os/libcfs/include/libcfs/user-bitops.h:80: error: conflicting types for ‘fls’
/usr/include/strings.h:90: error: previous declaration of ‘fls’ was here

so part for cfs_fls need to be reverted.

one more additional bug

gcc -DHAVE_CONFIG_H -I. -I../..  -D__arch_lib__ -D_LARGEFILE64_SOURCE=1 -include /Users/shadow/work/lustre/work/WC-review/mac-os/config.h -I/Users/shadow/work/lustre/work/WC-review/mac-os/libcfs/include -I/Users/shadow/work/lustre/work/WC-review/mac-os/lnet/include -I/Users/shadow/work/lustre/work/WC-review/mac-os/lustre/include  -g -Wall -fPIC -D_GNU_SOURCE -g -O2 -Werror -MT libcfs_a-posix-debug.o -MD -MP -MF .deps/libcfs_a-posix-debug.Tpo -c -o libcfs_a-posix-debug.o `test -f 'posix/posix-debug.c' || echo './'`posix/posix-debug.c

cc1: warnings being treated as errors

posix/posix-debug.c: In function ‘libcfs_debug_vmsg2’:

posix/posix-debug.c:210: warning: implicit declaration of function ‘cfs_gettimeofday’

[James A Simmons]

The patch just did a bunch of renaming of variables. cfs_cap_t is still a __u32. Can you point to exactly the break is where you believe it to be.

Please push fixes for the fls and gettimeofday issues. Patches always welcomed

[Alexey Lyashkov]

and PLEASE REMEMBER - any on wire constants MUST be platform independent to avoid situation when it's different on different sides of lustre cluster.

as additional objection about that work - 'fls' cleanup landed some time ago. that is broke lustre to build on system already have these functions but with different arguments as you expected.
and forget to make cfs_gettimeofday to gettimeofday changes in some places.

[Alexey Lyashkov]

please revert
commit b22fb817507ff52c02de38435fe90d758e852105
Author: James Simmons <[e-mail]>
Date: Fri Sep 13 09:44:00 2013 -0400

LU-1346 libcfs: replace CFS_CAP_XXX with kernel definition

that defines needed because Linux capability now have size more then lustre wire protocol so we don't able to pack all capability into wire. That is main reason to introduce these macroses, please look to commit and ticket where it's adds to lustre tree.

I see you are drop compatibility with different OS'es and kernels, but that is very very bad idea.

[Jodi Levi (Inactive)]

Follow on work for this ticket is being tracked in LU-3963

[Cory Spitz]

James posted http://review.whamcloud.com/#/c/7454 for gnilnd

[Peng Tao]

James, do you mean you are working on procfs change in 3.10? It is tracked on LU-3319. I thought that liuying is working on it. You may want to coordinate with each other so don't waste the efforts.