[LU-4023] wrong type used for ldlm flag

• Type: Bug
• Resolution: Fixed
• Priority: Blocker
• Fix Version/s: Lustre 2.5.0, Lustre 2.6.0, Lustre 2.4.2
• Affects Version/s: Lustre 2.4.0, Lustre 2.5.0
• Environment: clang 3.4
• Severity: 3
• Assignee: Dmitry Eremin (Inactive)
• Reporter: Alexey Lyashkov

Description

While porting lustre to macos i found a some strange behavior - likely wrong type used for variable.

osc_request.c:2503:41: error: implicit conversion from 'unsigned long long' to 'int' changes value from 2199023255552 to 0 [-Werror,-Wconstant-conversion]
        int match_lvb = (agl != 0 ? 0 : LDLM_FL_LVB_READY);
            ~~~~~~~~~                   ^~~~~~~~~~~~~~~~~
/Users/shadow/work/lustre/work/WC-review/mac-os/lustre/include/lustre_dlm_flags.h:244:41: note: expanded from macro 'LDLM_FL_LVB_READY'
#define LDLM_FL_LVB_READY               0x0000020000000000ULL // bit  41

so if we assign 64bit data to 32bit variable - we will be have zero always and that logic newer work.

Comments

[Alexey Lyashkov]

Andreas,

first, i think lack a response to inspector question - it's not a good style. you have a different opinion? if yes - please elaborate why Intel developers allowed to do it, but you strongly agains it from other developers.
If Dmitry was post a reply and create a new ticket with that cleanup it's make different response from my side.
I understand about code freeze, but i don't have guaranteed we don't have other places with same bugs, as i don't able to build kernel part with clang now. It's main objection if we have such bug in two releases - we don't think it's big regression in past. so have some changes in that area?

second - i have plan to test patch with clang to see none breakage introduced, but none interested with verification i see.

third - i know about -1 in gerrit, but i don't think i need set to -1 if i have any questions to the author, but now i will set -1 for any patch with any questions and clear only when finished a review - it's easy when see - none interested with review results.

as about patch - that patch is not a small change as you think. previously we have drop working any logic related to the alg as match_lvb always zero in past. did you inspect all related code - how it work with LDLM_FL_LVB_READY passed in down call? so that patch may change flag to correct mask but introduce a new bugs due LDLM_FL_LVB_READY will passed to argument of ldlm_lock_match?

[Andreas Dilger]

Shadow, I agree that changing the LDLM flags to be an enum is a good code cleanup and will help avoid bugs like this in the future, but I don't think this should prevent the current fix from landing. That is something that should be done as a separate patch, since it will touch a huge amount of code, and that would hide the actual important fix that is in http://review.whamcloud.com/7799. The master branch is currently in code freeze, so only critical bugs are being fixed in the branch, not huge code cleanups.

As for the patch landing to master without all reviewers confirmed - that would make it impossible to land any patch. It isn't possible to remove reviewers from a patch unless they do it themselves, and while we invite many people to inspect patches it is rare that all of the reviewers review every patch (Oleg and I are the only lucky ones that have to review all of the patches). We require two inspections for any patch (at least one must be a more senior developer, as has always been documented at https://wiki.hpdd.intel.com/display/PUB/Submitting+Changes).

If you have a serious objection to the correctness of a patch, it should be marked -1. If needed it is always possible for you to change your review to +1 after discussing the issue with the developer. To me your comment was more of a feature request than an objection to the patch, especially since you didn't mark it -1. In any case it makes sense to separate out patches so each one is just fixing one thing at a time. If you are interested to change the LDLM flags to an enum, please note the comment at the top of lustre_dlm_flags.h that Bruce Korb from Xyratex has changed this to be automatically generated from ./contrib/bit-masks/lustre_dlm_flags.def, so this should be updated instead of editing lustre_dlm_flags.h directly.

[Alexey Lyashkov]

stop talking about blockers. we need proper patch at first place and don't need rewrite a single fix for different branches it's add many problems with supporting that code.

current patch don't fix all similar issues in future and fix just one instance. if we have lost one point in code review where 32bit -> 64bit conversion is lost - you really think we don't have similar bugs in different places? because you review a code? but as you see review isn't guaranteed to kill all bugs.

changes define to enum and use correct type for a ldlm flags avoid bugs at all, because introduce are compile time checking and allocating a property type for all variables and that is provide better code and more optimization for compiler if need.

[Dmitry Eremin (Inactive)]

It's blocker and required immediate action for 2.5 release. So, the current patch fix the issue and then we can elaborate other solution.

[Alexey Lyashkov]

I isn't really understand - it's new feature to ignore a requests one of reviewers and push patch anyway?

i put comment in gerrit before new patch test start

Alexey Lyashkov		Oct 3 2:15 PM
Patch Set 4:
may we converted #define to enum to avoid such errors in feature ?

but none response - and patch landed to master without all reviewers confirmed - that patch is OK.

PS. sorry for english - autocompletion was rewrite a future to feature.

[Dmitry Eremin (Inactive)]

Patch was landed to master. New patch is for b2_4 http://review.whamcloud.com/#/c/7849/

[Andreas Dilger]

This code has been broken since Vitaly landed the patch b58a810 in 2.3.54. It isn't clear why we didn't notice this failure, but it seems that the "obvious" fix is introducing some kind of breakage and we need a closer look at what is going wrong.

[Dmitry Eremin (Inactive)]

patch is http://review.whamcloud.com/#/c/7799/