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

[Matthew Wilcox]

On Mon, Mar 22, 2021 at 07:27:03PM +0900, Sergey Senozhatsky wrote:
> On (21/03/22 08:34), Matthew Wilcox 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.
> 
> IIRC this ida is per SMB session, so it probably cannot be static.

Depends which IDA you're talking about.

+struct ksmbd_conn *ksmbd_conn_alloc(void)
+	conn->async_ida = ksmbd_ida_alloc();
Embed into 'conn'.

+static struct ksmbd_ida *ida;
+int ksmbd_ipc_init(void)
+	ida = ksmbd_ida_alloc();
Should be static.

> And Windows, IIRC, doesn't like "just any IDs". Some versions of Windows
> would fail the session login if server would return the first id == 0,
> instead of 1. Or vice versa. I don't remember all the details, the last
> time I looked into this was in 2019.

Sure, you can keep that logic.

> > ... 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
> 
> I think cifsd code predates XArray ;)

Sure, but you could have used an IDR ;-)

> > Then you could lookup tree connections in O(log(n)) time instead of
> > O(n) time.
> 
> Agreed. Not sure I remember why the code does list traversal here.