Re: [V9fs-developer] [PATCH 5/6] 9p: Use a slab for allocating requests

[Matthew Wilcox <willy@infradead.org>]

On Wed, Jul 11, 2018 at 03:33:13PM +0200, Dominique Martinet wrote:
> Matthew Wilcox wrote on Thu, Jun 28, 2018:
> >  /**
> >   * struct p9_client - per client instance state
> > - * @lock: protect @fidlist
> > + * @lock: protect @fids and @reqs
> >   * @msize: maximum data size negotiated by protocol
> > - * @dotu: extension flags negotiated by protocol
> >   * @proto_version: 9P protocol version to use
> >   * @trans_mod: module API instantiated with this client
> > + * @status: XXX
> 
> Let's give this a proper comment; something like 'connection state
> machine' ? (this contains Connected/BeginDisconnect/Disconnected/Hung)

Sure!  Will add.

> >  /**
> > - * p9_tag_alloc - lookup/allocate a request by tag
> > - * @c: client session to lookup tag within
> > - * @tag: numeric id for transaction
> > - *
> > - * this is a simple array lookup, but will grow the
> > - * request_slots as necessary to accommodate transaction
> > - * ids which did not previously have a slot.
> > - *
> > - * this code relies on the client spinlock to manage locks, its
> > - * possible we should switch to something else, but I'd rather
> > - * stick with something low-overhead for the common case.
> > + * p9_req_alloc - Allocate a new request.
> > + * @c: Client session.
> > + * @type: Transaction type.
> > + * @max_size: Maximum packet size for this request.
> >   *
> > + * Context: Process context.  What mutex might we be holding that
> > + * requires GFP_NOFS?
> 
> Good question, but p9_tag_alloc happens on every single client request
> so the fs/9p functions might be trying to do something and the alloc
> request here comes in and could try to destroy the inode that is
> currently used in the request -- I'm not sure how likely this is, but
> I'd rather not tempt fate :p

Fair.  I'll remove the question.

> > +	INIT_LIST_HEAD(&req->req_list);
> > +
> > +	idr_preload(GFP_NOFS);
> > +	spin_lock_irq(&c->lock);
> > +	if (type == P9_TVERSION)
> > +		tag = idr_alloc(&c->reqs, req, P9_NOTAG, P9_NOTAG + 1,
> > +				GFP_NOWAIT);
> 
> Well this appears to work but P9_NOTAG being '(u16)(~0)' I'm not too
> confident with P9_NOTAG + 1. . . it doesn't look like it's overflowing
> before the cast on my laptop but is that guaranteed?

By my understanding of n1256.pdf ... this falls under 6.3.1.8 ("Usual
arithmetic conversions").  We have a u16 and an int.  Therefore this
rule applies:

    Otherwise, if the type of the operand with signed integer type can
    represent all of the values of the type of the operand with unsigned
    integer type, then the operand with unsigned integer type is converted
    to the type of the operand with signed integer type.

> > [..]
> > @@ -1012,14 +940,11 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
> >  
> >  	spin_lock_init(&clnt->lock);
> >  	idr_init(&clnt->fids);
> > -
> > -	err = p9_tag_init(clnt);
> > -	if (err < 0)
> > -		goto free_client;
> > +	idr_init(&clnt->reqs);
> 
> I do not see any call to idr_destroy, is that OK?

Yes, that's fine.  It used to be (back in 2013) that one had to call
idr_destroy() in order to free the preallocated idr data structures.
Now it's a no-op if called on an empty IDR, and I would expect that both
IDRs are empty at the time that it comes to unloading the module (and if
they aren't, we probably have bigger problems than a small memory leak).
Some users like to assert that the IDR is empty; most do not go to that
extent of defensive programming.