[LU-1575] Add flock policy support

• Status: Resolved
• Project: Lustre
• Affects Version/s: Lustre 2.2.0, Lustre 2.3.0, Lustre 2.1.1, Lustre 1.8.6
• Type: Improvement
• Priority: Minor
• Reporter: Iurii Golovach (Inactive)
• Assignee: Oleg Drokin
• Resolution: Fixed
• Labels: client, server

Sub-Tasks

Key	Summary	Type	Status	Assignee
LU-1770	Introducing OBD_CONNECT_FLOCK_OWNER flag	Technical task	Resolved	Cliff White

• Key: Summary
• Summary: Type
• Type: Status
• Status: Assignee

Description

The goal of this ticket is to add the support for the patched clients 1.8 to be correctly handled on the 2.x fixed side.

The patch has to contains next two parts:
1) client side change (for the https://bugzilla.lustre.org/attachment.cgi?id=30902) - here we need to add new flag for 1.8 clients which has the change from this patch (OBD_CONNECT_FLOCK_OWNER).
2) server side change for the 2.x http://review.whamcloud.com/#patch,unified,2193,2,lustre/ldlm/ldlm_lock.c patch and extend it with the flag OBD_CONNECT_FLOCK_OWNER checking and call the ldlm_flock_policy_wire21_to_local() function for this case.

Issue Links

• is related to: LU-1137 flock owner incorrectly handled on th...

Comments

[Iurii Golovach (Inactive)]

http://review.whamcloud.com/#change,3202 - patch which introduce new flag on the server side (2.x).

http://review.whamcloud.com/3203 - patch which introduce new flag on the client side (1.8).

[Andreas Dilger]

Making this a 2.3 blocker until the interop patches are landed. The original patch in LU-1137 was landed in git commit 7526d21040f4064f83fc4350cd58a8f39de913b3 (v2_2_50_0-50-g7526d21) so the interop issue is only in the current master branch.

[Iurii Golovach (Inactive)]

Patches with OBD_CONNECT_FLAG_OWNER were introduced sepatately according the request from Andreas at LU-1770.

Short explanation of the patch is next:

As you may see at 1.8 there is only introdusion OBD_CONNECT_OWNER flag and sending it to the server. Defines where OBD_CONNECT_OWNER is included are placed at the lustre_idl.h and there is no functional changes on the client side at all.

There is a change on the server side.

Before the patch:

new_client = (exp->exp_connect_flags & OBD_CONNECT_FULL20) != 0;
if (new_client)
convert = ldlm_policy_wire21_to_local[type - LDLM_MIN_TYPE];
else
convert = ldlm_policy_wire18_to_local[type - LDLM_MIN_TYPE];

After the patch:

new_client = (exp->exp_connect_flags & OBD_CONNECT_FULL20) != 0;
if (new_client)
convert = ldlm_policy_wire21_to_local[type - LDLM_MIN_TYPE];
else if ((exp->exp_connect_flags & OBD_CONNECT_FLOCK_OWNER) != 0 &&
type == LDLM_FLOCK)
convert = ldlm_policy_wire21_to_local[type - LDLM_MIN_TYPE];
else
convert = ldlm_policy_wire18_to_local[type - LDLM_MIN_TYPE];

As you may see the new check was introduced:

else if ((exp->exp_connect_flags & OBD_CONNECT_FLOCK_OWNER) != 0 &&
type == LDLM_FLOCK)
convert = ldlm_policy_wire21_to_local[type - LDLM_MIN_TYPE];

There are few ideas in this lines.

1) The funtional change idea:

Without this line server recognize the 1.8 client like a client with old flock policy (wich is native for 1.8.x).
But patch https://bugzilla.lustre.org/attachment.cgi?id=30902 introduce the 2.x flock policy in 1.8 clients.

The difference between 1.8 and 2.x flock policy handling is in assigning flocks to the OWNER instead of using PID.

The issue comes when 1.8 clients (patched with https://bugzilla.lustre.org/attachment.cgi?id=30902) try to work with 2.x server.

Since 2.x server recognize them like 1.8 clients and start working with PID field instead of OWNER field.

2.x server uses the next function:

void ldlm_flock_policy_wire18_to_local(const ldlm_wire_policy_data_t *wpolicy,
ldlm_policy_data_t *lpolicy)
{
.....
lpolicy->l_flock.owner = wpolicy->l_flock.lfw_pid;
}

While it should use the next one:
void ldlm_flock_policy_wire21_to_local(const ldlm_wire_policy_data_t *wpolicy,
ldlm_policy_data_t *lpolicy)
{
.....
lpolicy->l_flock.owner = wpolicy->l_flock.lfw_owner;
}

That's all about functional changes.

Now two ideas in code:

2) It may looks like OBD_CONNECT_FULL20 flag can be used and there is no needs in a new flag.
This is incorrect, since OBD_CONNECT_FULL20 usage adds some additional 2.x related funcitonality which is currently unsupported in 1.8 code.

3) sepatare "else if" is used:
((exp->exp_connect_flags & OBD_CONNECT_FLOCK_OWNER) != 0 &&
type == LDLM_FLOCK)

The reason is that we have use 2.x functionality for the LDLM_FLOCK only. And use 1.8 functionality for other cases.

Andreas, please, let me know if there are any questions which this test doesn't cover.

[Andreas Dilger]

Repeating some of the comments from the inspection of the b1_8 version of the patch, so that everyone is clear on what is going on here:

• if the reservation of OBD_CONNECT_FLOCK_OWNER flag is landing, it should be landed on all of the branches. There are currently patches for b1_8 and master (should be included into b2_3), but patches are also necessary for b2_1 and b2_2 to avoid potential conflicts if someone is developing a new feature against b2_1 and doesn't see this bit reserved.
• the b1_8 version of the patch http://review.whamcloud.com/3203 cannot be landed to our b1_8 Git repo as is, since we do not (AFAICS) include the b=22040/att=30902 change yet. Otherwise, any clients built from that branch (e.g. from Git or 1.8.8) would incorrectly advertise their support for FLOCK_OWNER when they don't actually handle this feature correctly.
• the master version of the fix (http://review.whamcloud.com/3202) needs to be rebased on top of the feature reservation patch (http://review.whamcloud.com/3722).
• I see that LU-1137 (http://review.whamcloud.com/2193) was included into our 2.1.2 release, but since the code is conditional upon the OBD_CONNECT_FULL20 flag, this will only continue working as long as 1.8.x clients do not have the att=30902 fix applied.

So to move forward, either b1_8 should only get the flag reservation patch (http://review.whamcloud.com/3723), or the att=30902 patch needs to be submitted to b1_8 merged together with change 3203 to actually use the flag, so that there is no time when the b1_8 clients are sending the OWNER field without also connecting with the OBD_CONNECT_FLOCK_OWNER feature flag. At the same time (i.e. blocking the 2.1.3 release, which is basically out the door already), b2_1 would also need to be patched to understand this flag, and it would cause 2.1.0/2.1.1/2.1.2/2.2 servers to break against patched 1.8.8 clients.

It isn't clear to me whether there is any benefit of submitting the "fix" to b1_8 at all, since the easiest thing to do would be to leave all 1.8 clients unpatched and handle this at the server. Is there a functional benefit for 1.8 clients if they DO set the OWNER field, as opposed to the 2.x servers with LU-1137 applied handling this themselves?

[Andreas Dilger]

Without a clear functional benefit to applying the original att=30902 change to b1_8, I don't see any value in going down this road at all for our 1.8 clients.

I understand that Xyratex has patched 1.8.x clients in the field using this code, and they will be applying (or already have) http://review.whamcloud.com/3203 in order to declare their use of the flock OWNER field. I think it also makes sense to apply an updated version of 3202/3722 to master (and b2_3 if already forked) and b2_1, to avoid potential interop issues with patched b1_8 clients (Xyratex/Oracle/Cray/etc). However, I don't yet see any benefit for applying att=30902 to our b1_8 at this time, and definite interoperability problems would be introduced for existing deployments.

Having a subtest added to sanity.sh test_105 that actually exercised this code in master would help avoid potential interoperability issues in the future, and would ensure that this code is exercised during our release interoperability testing, which is run regularly by autotest.

[Oleg Drokin]

There is a benefit to b1_8, actually more than one:
1. flock (the bsd whole-file locks, as opposed to posix locks we usually call by this name) starts to really work (things like working across processes and such.
2. locking starts to work in case of nfs4 reexport.

That said, att. 3203 all by itself is harmful, it also needs the actual logic implemented, plus on top of that logic, it needs a bit of interop to now include owner info if the server did not acknowledge the owner flag.

[Cory Spitz]

Can http://review.whamcloud.com/3202 land to master?

[Nathan Rutman]

Xyratex-bug-id: MRP-489

[Iurii Golovach (Inactive)]

Status update.
1) Issue summary:
During usage https://bugzilla.lustre.org/attachment.cgi?id=30902 with 1.8 customer met with an issue when 1.8 client can't be recognized on 2.x server.
2) Resolution:
To fix this issue next patches were created:
1) http://review.whamcloud.com/3203 - for 1.8 clients;
2) http://review.whamcloud.com/3202 - for 2.x servers.
(http://morpheus.xyus.xyratex.com:8443/gerrit/#/c/120/ in Xyratex)
3) Landing History:
Stage 1:
http://review.whamcloud.com/3203 - was created for 1.8 clients
http://review.whamcloud.com/3202 - was created for 2.x servers.
Stage 2:
Andreas requested to provide number of patches to reserve the flag value. So, next patches were added:
http://review.whamcloud.com/#change,3722 (master) - landed
http://review.whamcloud.com/#change,3725 (b2_2) - landed
http://review.whamcloud.com/#change,3727 (b2_1) - approved. not landed yet.
Stage 3 (current):
http://review.whamcloud.com/3203 - Andreas don't want to have this patch, since without https://bugzilla.lustre.org/attachment.cgi?id=30902 it has no sense. But for https://bugzilla.lustre.org/attachment.cgi?id=30902 he see no benefit. Oleg has another vision and now it's up from WC to decide if they want to have these both patches in b1_8
http://review.whamcloud.com/3202 - was rebased after landing 3722. Oleg think that we can't use this patch. I just discussed with him requirements. They are next:
there is a 2.1 version where old policy is used. The patch for b1_8 should contain the ability to understand if the current server version support new policy or not. So, I fixed the situation "new client / new server". Now we have to fix "new client / old server".
Open questions:
1) It's not clear if Andreas will agree with Oleg's recommendation (do they want to have https://bugzilla.lustre.org/attachment.cgi?id=30902).

[Cory Spitz]

Since http://review.whamcloud.com/3203 is abandoned, is there any reason to keep this ticket open?

[Andreas Dilger]

Close old bug.