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.
