[Cluster-devel] [PATCHv4 v6.5-rc2 3/3] fs: dlm: fix F_CANCELLK to cancel pending request

[Alexander Aring]

Hi,

On Fri, Jul 21, 2023 at 12:25 PM Andreas Gruenbacher
<agruenba at redhat.com> wrote:
>
> On Thu, Jul 20, 2023 at 2:22 PM Alexander Aring <aahringo at redhat.com> wrote:
> > This patch fixes the current handling of F_CANCELLK by not just doing a
> > unlock as we need to try to cancel a lock at first. A unlock makes sense
> > on a non-blocking lock request but if it's a blocking lock request we
> > need to cancel the request until it's not granted yet. This patch is fixing
> > this behaviour by first try to cancel a lock request and if it's failed
> > it's unlocking the lock which seems to be granted.
>
> I don't like how this is implemented, but the problem already starts
> in commit 57e2c2f2d94c ("fs: dlm: fix mismatch of plock results from
> userspace"). That commit relies on how dlm_controld is implemented
> internally; it requires dlm_controld to keep all replies to
> non-blocking requests in perfect order. The correctness of that
> approach is more difficult to argue for than it should be, the code
> might break in non-obvious ways, and it limits the kinds of things
> dlm_controld will be able to do in the future. And this change relies
> on the same flawed design.
>

I agree it is not the best design and I mentioned it in my previous
patch series versions [0]:

TODO we should move to a instance reference from request and reply and
not go over lock states, but it seems going over lock states and get
the instance does not make any problems (at least there were no issues
found yet) but it's much cleaner to not think about all possible
special cases and states to instance has no 1:1 mapping anymore.

> Instead, when noticing that we don't have a way to uniquely identify
> requests, such a way should just have been introduced. The CANCEL
> request would then carry the same unique identifier as the original
> locking request, dlm_controld would reply either to the original
> locking request or to the cancel request, and the kernel would have no
> problem matching the reply to the request.
>
> But without unique request identifiers, we now end up with those
> redundant -ENOENT replies to CANCEL requests and with those
> essentially duplicate entries for the same request on recv_list. Not
> great.
>

There is no reference of lock request instances between kernel and
user yet. It does it by having a match if it's the same lock request.
The whole mechanism works like this, but one reason why we recently
had problems with it. A lock request is the same in the sense that
they are being granted at the same time. So far we did not experience
any problems with that... but as mentioned in [0] you need to think
about all potential cases.

NOTE: that even F_CANCELLK does not give you a unique reference of the
original locking request, it works over matching the field of struct
file_lock... There is already the same problem. The struct file_lock
pointer could be used, but struct file_lock does not represent a lock
request, this does not exist in VFS posix lock API.

- Alex

[0] https://listman.redhat.com/archives/cluster-devel/2023-July/024477.html