Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 22 Mar 2021 08:34:45 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     Namjae Jeon <namjae.jeon@samsung.com>
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-cifs@vger.kernel.org,
        linux-cifsd-devel@lists.sourceforge.net, smfrench@gmail.com,
        senozhatsky@chromium.org, hyc.lee@gmail.com,
        viro@zeniv.linux.org.uk, hch@lst.de, hch@infradead.org,
        ronniesahlberg@gmail.com, aurelien.aptel@gmail.com,
        aaptel@suse.com, sandeen@sandeen.net, dan.carpenter@oracle.com,
        colin.king@canonical.com, rdunlap@infradead.org,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Steve French <stfrench@microsoft.com>
Subject: Re: [PATCH 2/5] cifsd: add server-side procedures for SMB3
Message-ID: <20210322083445.GJ1719932@casper.infradead.org>
References: <20210322051344.1706-1-namjae.jeon@samsung.com>
 <CGME20210322052206epcas1p438f15851216f07540537c5547a0a2c02@epcas1p4.samsung.com>
 <20210322051344.1706-3-namjae.jeon@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210322051344.1706-3-namjae.jeon@samsung.com>
Precedence: bulk

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.