Re: [V9fs-developer] [PATCH v2 3/6] 9p: Replace the fidlist with an IDR

[Matthew Wilcox <willy@infradead.org>]

On Fri, Jul 13, 2018 at 10:05:50AM +0800, jiangyiwen wrote:
> > @@ -908,30 +908,29 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
> >  {
> >  	int ret;
> >  	struct p9_fid *fid;
> > -	unsigned long flags;
> >  
> >  	p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
> >  	fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);
> >  	if (!fid)
> >  		return NULL;
> >  
> > -	ret = p9_idpool_get(clnt->fidpool);
> > -	if (ret < 0)
> > -		goto error;
> > -	fid->fid = ret;
> > -
> >  	memset(&fid->qid, 0, sizeof(struct p9_qid));
> >  	fid->mode = -1;
> >  	fid->uid = current_fsuid();
> >  	fid->clnt = clnt;
> >  	fid->rdir = NULL;
> > -	spin_lock_irqsave(&clnt->lock, flags);
> > -	list_add(&fid->flist, &clnt->fidlist);
> > -	spin_unlock_irqrestore(&clnt->lock, flags);
> > +	fid->fid = 0;
> >  
> > -	return fid;
> > +	idr_preload(GFP_KERNEL);
> 
> It is best to use GFP_NOFS instead, or else it may cause some
> unpredictable problem, because when out of memory it will
> reclaim memory from v9fs.

Earlier in this function, fid was allocated with GFP_KERNEL:

> >  	fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);


> > +	spin_lock_irq(&clnt->lock);
> > +	ret = idr_alloc_u32(&clnt->fids, fid, &fid->fid, P9_NOFID - 1,
> > +			GFP_NOWAIT);
> > +	spin_unlock_irq(&clnt->lock);
> 
> use spin_lock instead, clnt->lock is not used in irq context.

I don't think that's right.  What about p9_fid_destroy?  It was already
using spin_lock_irqsave(), so I just assumed that whoever wrote that
code at least considered that it might be called from interrupt context.

Also consider p9_free_req() which shares the same lock.  We could get
rid of clnt->lock altogether as there's a lock embedded in each IDR,
but that'll introduce an unwanted dependence on the RDMA tree in this
merge window.

> > @@ -1095,14 +1086,11 @@ void p9_client_destroy(struct p9_client *clnt)
> >  
> >  	v9fs_put_trans(clnt->trans_mod);
> >  
> > -	list_for_each_entry_safe(fid, fidptr, &clnt->fidlist, flist) {
> > +	idr_for_each_entry(&clnt->fids, fid, id) {
> >  		pr_info("Found fid %d not clunked\n", fid->fid);
> >  		p9_fid_destroy(fid);
> >  	}
> >  
> > -	if (clnt->fidpool)
> > -		p9_idpool_destroy(clnt->fidpool);
> > -
> 
> I suggest add idr_destroy in the end.

Why?  p9_fid_destroy calls idr_remove() for each fid, so it'll already
be empty.

Thanks for all the review, to everyone who's submitted review.  This is
a really healthy community.