On Thu, Jul 13, 2023 at 4:40 PM Alexander Aring <aahringo@xxxxxxxxxx> wrote:
> This patch introduces a new flag DLM_PLOCK_FL_NO_REPLY in case an dlm
> plock operation should not send a reply back. Currently this is kind of
> being handled in DLM_PLOCK_FL_CLOSE, but DLM_PLOCK_FL_CLOSE has more
> meanings that it will remove all waiters for a specific nodeid/owner
> values in by doing a unlock operation. In case of an error in dlm user
> space software e.g. dlm_controld we get an reply with an error back.
> This cannot be matched because there is no op to match in recv_list. We
> filter now on DLM_PLOCK_FL_NO_REPLY in case we had an error back as
> reply. In newer dlm_controld version it will never send a result back
> when DLM_PLOCK_FL_NO_REPLY is set. This filter is a workaround to handle
> older dlm_controld versions.
I don't think this makes sense. If dlm_controld understands a
particular request, it already knows whether or not that request
should receive a reply. On the other hand, if dlm_controld doesn't
understand a particular request, it should communicate that fact back
to the kernel so that the kernel will know. The kernel knows which
kinds of requests should and shouldn't receive replies, so when it is
sent a reply it doesn't expect, it knows that dlm_controld didn't
understand the request and is either outdated or plain broken. The
kernel doesn't need to pipe a flag through dlm_controld for figuring
that out.
Thanks,
Andreas
> Fixes: 901025d2f319 ("dlm: make plock operation killable")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Alexander Aring <aahringo@xxxxxxxxxx>
> ---
> fs/dlm/plock.c | 23 +++++++++++++++++++----
> include/uapi/linux/dlm_plock.h | 1 +
> 2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> index 70a4752ed913..7fe9f4b922d3 100644
> --- a/fs/dlm/plock.c
> +++ b/fs/dlm/plock.c
> @@ -96,7 +96,7 @@ static void do_unlock_close(const struct dlm_plock_info *info)
> op->info.end = OFFSET_MAX;
> op->info.owner = info->owner;
>
> - op->info.flags |= DLM_PLOCK_FL_CLOSE;
> + op->info.flags |= (DLM_PLOCK_FL_CLOSE | DLM_PLOCK_FL_NO_REPLY);
> send_op(op);
> }
>
> @@ -293,7 +293,7 @@ int dlm_posix_unlock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
> op->info.owner = (__u64)(long) fl->fl_owner;
>
> if (fl->fl_flags & FL_CLOSE) {
> - op->info.flags |= DLM_PLOCK_FL_CLOSE;
> + op->info.flags |= (DLM_PLOCK_FL_CLOSE | DLM_PLOCK_FL_NO_REPLY);
> send_op(op);
> rv = 0;
> goto out;
> @@ -392,7 +392,7 @@ static ssize_t dev_read(struct file *file, char __user *u, size_t count,
> spin_lock(&ops_lock);
> if (!list_empty(&send_list)) {
> op = list_first_entry(&send_list, struct plock_op, list);
> - if (op->info.flags & DLM_PLOCK_FL_CLOSE)
> + if (op->info.flags & DLM_PLOCK_FL_NO_REPLY)
> list_del(&op->list);
> else
> list_move_tail(&op->list, &recv_list);
> @@ -407,7 +407,7 @@ static ssize_t dev_read(struct file *file, char __user *u, size_t count,
> that were generated by the vfs cleaning up for a close
> (the process did not make an unlock call). */
>
> - if (op->info.flags & DLM_PLOCK_FL_CLOSE)
> + if (op->info.flags & DLM_PLOCK_FL_NO_REPLY)
> dlm_release_plock_op(op);
>
> if (copy_to_user(u, &info, sizeof(info)))
> @@ -433,6 +433,21 @@ static ssize_t dev_write(struct file *file, const char __user *u, size_t count,
> if (check_version(&info))
> return -EINVAL;
>
> + /* Some old dlm user space software will send replies back,
> + * even if DLM_PLOCK_FL_NO_REPLY is set (because the flag is
> + * new) e.g. if a error occur. We can't match them in recv_list
> + * because they were never be part of it. We filter it here,
> + * new dlm user space software will filter it in user space.
> + *
> + * In future this handling can be removed.
> + */
> + if (info.flags & DLM_PLOCK_FL_NO_REPLY) {
> + pr_info("Received unexpected reply from op %d, "
> + "please update DLM user space software!\n",
> + info.optype);
> + return count;
> + }
> +
> /*
> * The results for waiting ops (SETLKW) can be returned in any
> * order, so match all fields to find the op. The results for
> diff --git a/include/uapi/linux/dlm_plock.h b/include/uapi/linux/dlm_plock.h
> index 63b6c1fd9169..8dfa272c929a 100644
> --- a/include/uapi/linux/dlm_plock.h
> +++ b/include/uapi/linux/dlm_plock.h
> @@ -25,6 +25,7 @@ enum {
> };
>
> #define DLM_PLOCK_FL_CLOSE 1
> +#define DLM_PLOCK_FL_NO_REPLY 2
>
> struct dlm_plock_info {
> __u32 version[3];
> --
> 2.31.1
>
