lkml.org 
[lkml]   [2018]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [V9fs-developer] [PATCH v2 3/6] 9p: Replace the fidlist with an IDR
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.
\
 
 \ /
  Last update: 2018-07-15 22:06    [W:0.069 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site