Re: [PATCH 2/5] cifsd: add server-side procedures for SMB3

[Matthew Wilcox]

On Mon, Mar 22, 2021 at 02:13:41PM +0900, Namjae Jeon wrote:
> +++ b/fs/cifsd/mgmt/ksmbd_ida.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *   Copyright (C) 2018 Samsung Electronics Co., Ltd.
> + */
> +
> +#include "ksmbd_ida.h"
> +
> +struct ksmbd_ida *ksmbd_ida_alloc(void)
> +{
> +	struct ksmbd_ida *ida;
> +
> +	ida = kmalloc(sizeof(struct ksmbd_ida), GFP_KERNEL);
> +	if (!ida)
> +		return NULL;
> +
> +	ida_init(&ida->map);
> +	return ida;
> +}

... why?  Everywhere that you call ksmbd_ida_alloc(), you would
be better off just embedding the struct ida into the struct that
currently has a pointer to it.  Or declaring it statically.  Then
you can even initialise it statically using DEFINE_IDA() and
eliminate the initialiser functions.

I'd remove the ksmbd_ida abstraction, although I like this wrapper:

> +int ksmbd_acquire_smb2_tid(struct ksmbd_ida *ida)
> +{
> +	int id;
> +
> +	do {
> +		id = __acquire_id(ida, 0, 0);
> +	} while (id == 0xFFFF);
> +
> +	return id;

Very clever, given your constraint.  I might do it as:

	int id = ida_alloc(ida, GFP_KERNEL);
	if (id == 0xffff)
		id = ida_alloc(ida, GFP_KERNEL);
	return id;

Although ...

> +	tree_conn = ksmbd_alloc(sizeof(struct ksmbd_tree_connect));
> +	if (!tree_conn) {
> +		status.ret = -ENOMEM;
> +		goto out_error;
> +	}
> +
> +	tree_conn->id = ksmbd_acquire_tree_conn_id(sess);
> +	if (tree_conn->id < 0) {
> +		status.ret = -EINVAL;
> +		goto out_error;
> +	}
> +
> +	peer_addr = KSMBD_TCP_PEER_SOCKADDR(sess->conn);
> +	resp = ksmbd_ipc_tree_connect_request(sess,
> +					      sc,
> +					      tree_conn,
> +					      peer_addr);
> +	if (!resp) {
> +		status.ret = -EINVAL;
> +		goto out_error;
> +	}
> +
> +	status.ret = resp->status;
> +	if (status.ret != KSMBD_TREE_CONN_STATUS_OK)
> +		goto out_error;
> +
> +	tree_conn->flags = resp->connection_flags;
> +	tree_conn->user = sess->user;
> +	tree_conn->share_conf = sc;
> +	status.tree_conn = tree_conn;
> +
> +	list_add(&tree_conn->list, &sess->tree_conn_list);

This is basically the only function which calls that, and this is a relatively
common anti-pattern when using the IDA -- you've allocated a unique ID,
but then you stuff the object in a list and ...

> +struct ksmbd_tree_connect *ksmbd_tree_conn_lookup(struct ksmbd_session *sess,
> +						  unsigned int id)
> +{
> +	struct ksmbd_tree_connect *tree_conn;
> +	struct list_head *tmp;
> +
> +	list_for_each(tmp, &sess->tree_conn_list) {
> +		tree_conn = list_entry(tmp, struct ksmbd_tree_connect, list);
> +		if (tree_conn->id == id)
> +			return tree_conn;
> +	}

... walk the linked list looking for an ID match.  You'd be much better
off using an allocating XArray:
https://www.kernel.org/doc/html/latest/core-api/xarray.html

Then you could lookup tree connections in O(log(n)) time instead of
O(n) time.