| Message ID | 20180628132629.3148-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Matthew Wilcox wrote on Thu, Jun 28, 2018: > diff --git a/net/9p/client.c b/net/9p/client.c > index f8d58b0852fe..bbab82f22c20 100644 > --- a/net/9p/client.c > +++ b/net/9p/client.c > @@ -908,30 +908,27 @@ 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); > > - return fid; > + idr_preload(GFP_KERNEL); > + spin_lock_irq(&clnt->lock); > + ret = idr_alloc_u32(&clnt->fids, fid, &fid->fid, UINT_MAX, GFP_NOWAIT); There's also a P9_NOFID that we shouldn't use, so the max here should be P9_NOFID - 1 That aside this introduces a change of behaviour that fid used to be alloc'd linearily from 0 which no longer holds true, that breaks one serveur (nfs-ganesha just returns ERANGE) but others seem to handle this just fine so they'll just need to fix that server. max aside this looks good.
On Wed, Jul 11, 2018 at 02:40:38PM +0200, Dominique Martinet wrote: > Matthew Wilcox wrote on Thu, Jun 28, 2018: > > diff --git a/net/9p/client.c b/net/9p/client.c > > index f8d58b0852fe..bbab82f22c20 100644 > > --- a/net/9p/client.c > > +++ b/net/9p/client.c > > @@ -908,30 +908,27 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt) > > - ret = p9_idpool_get(clnt->fidpool); > > - if (ret < 0) > > - goto error; > > - fid->fid = ret; > > - ... > > + idr_preload(GFP_KERNEL); > > + spin_lock_irq(&clnt->lock); > > + ret = idr_alloc_u32(&clnt->fids, fid, &fid->fid, UINT_MAX, GFP_NOWAIT); > > There's also a P9_NOFID that we shouldn't use, so the max here should be > P9_NOFID - 1 Happy to fix that. It shouldn't actually happen, of course. I can't imagine us having 4 billion FIDs in use at once. > That aside this introduces a change of behaviour that fid used to be > alloc'd linearily from 0 which no longer holds true, that breaks one > serveur (nfs-ganesha just returns ERANGE) but others seem to handle this > just fine so they'll just need to fix that server. > max aside this looks good. I don't understand your assertion that this is a change of behaviour. The implementation of p9_idpool_get() uses idr_alloc(), not idr_alloc_cyclic(), so I don't believe I've changed which FID would be allocated.
Matthew Wilcox wrote on Wed, Jul 11, 2018: > On Wed, Jul 11, 2018 at 02:40:38PM +0200, Dominique Martinet wrote: > > > + idr_preload(GFP_KERNEL); > > > + spin_lock_irq(&clnt->lock); > > > + ret = idr_alloc_u32(&clnt->fids, fid, &fid->fid, UINT_MAX, GFP_NOWAIT); > > > > There's also a P9_NOFID that we shouldn't use, so the max here should be > > P9_NOFID - 1 > > Happy to fix that. It shouldn't actually happen, of course. I can't > imagine us having 4 billion FIDs in use at once. Oh, I said that assuming that it was random.. But I guess we might as well fix it. > > That aside this introduces a change of behaviour that fid used to be > > alloc'd linearily from 0 which no longer holds true, that breaks one > > serveur (nfs-ganesha just returns ERANGE) but others seem to handle this > > just fine so they'll just need to fix that server. > > max aside this looks good. > > I don't understand your assertion that this is a change of behaviour. > The implementation of p9_idpool_get() uses idr_alloc(), not > idr_alloc_cyclic(), so I don't believe I've changed which FID would > be allocated. Hmm, I just tried mounting something with ganesha and that broke because it received fid=1730858632 in a TATTACH request, so something in the patch series made some big fid happens. If you say this isn't intented though this needs debugging, I'm glad I brought this up :P (Note that it really should be fine if it is random within the pool, I have notified the current developpers of the problem)
On Wed, Jul 11, 2018 at 02:58:12PM +0200, Dominique Martinet wrote: > > I don't understand your assertion that this is a change of behaviour. > > The implementation of p9_idpool_get() uses idr_alloc(), not > > idr_alloc_cyclic(), so I don't believe I've changed which FID would > > be allocated. > > Hmm, I just tried mounting something with ganesha and that broke because > it received fid=1730858632 in a TATTACH request, so something in the > patch series made some big fid happens. > > If you say this isn't intented though this needs debugging, I'm glad I > brought this up :P > > (Note that it really should be fine if it is random within the pool, I > have notified the current developpers of the problem) Heh, unintended protocol fuzzing ;-) I see the problem; need to initialise fid->fid to 0 before calling idr_alloc_u32. I'd misread: memset(&fid->qid, 0, sizeof(struct p9_qid)); as memset(fid, 0, sizeof(struct p9_fid)); so I thought fid->fid was already 0. I'll fix that up, thanks!
diff --git a/include/net/9p/client.h b/include/net/9p/client.h index 7af9d769b97d..e405729cd1c7 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -27,6 +27,7 @@ #define NET_9P_CLIENT_H #include <linux/utsname.h> +#include <linux/idr.h> /* Number of requests per row */ #define P9_ROW_MAXTAG 255 @@ -128,8 +129,7 @@ struct p9_req_t { * @proto_version: 9P protocol version to use * @trans_mod: module API instantiated with this client * @trans: tranport instance state and API - * @fidpool: fid handle accounting for session - * @fidlist: List of active fid handles + * @fids: All active FID handles * @tagpool - transaction id accounting for session * @reqs - 2D array of requests * @max_tag - current maximum tag id allocated @@ -169,8 +169,7 @@ struct p9_client { } tcp; } trans_opts; - struct p9_idpool *fidpool; - struct list_head fidlist; + struct idr fids; struct p9_idpool *tagpool; struct p9_req_t *reqs[P9_ROW_MAXTAG]; @@ -188,7 +187,6 @@ struct p9_client { * @iounit: the server reported maximum transaction size for this file * @uid: the numeric uid of the local user who owns this handle * @rdir: readdir accounting structure (allocated on demand) - * @flist: per-client-instance fid tracking * @dlist: per-dentry fid tracking * * TODO: This needs lots of explanation. @@ -204,7 +202,6 @@ struct p9_fid { void *rdir; - struct list_head flist; struct hlist_node dlist; /* list of all fids attached to a dentry */ }; diff --git a/net/9p/client.c b/net/9p/client.c index f8d58b0852fe..bbab82f22c20 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -908,30 +908,27 @@ 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); - return fid; + idr_preload(GFP_KERNEL); + spin_lock_irq(&clnt->lock); + ret = idr_alloc_u32(&clnt->fids, fid, &fid->fid, UINT_MAX, GFP_NOWAIT); + spin_unlock_irq(&clnt->lock); + idr_preload_end(); + + if (!ret) + return fid; -error: kfree(fid); return NULL; } @@ -943,9 +940,8 @@ static void p9_fid_destroy(struct p9_fid *fid) p9_debug(P9_DEBUG_FID, "fid %d\n", fid->fid); clnt = fid->clnt; - p9_idpool_put(fid->fid, clnt->fidpool); spin_lock_irqsave(&clnt->lock, flags); - list_del(&fid->flist); + idr_remove(&clnt->fids, fid->fid); spin_unlock_irqrestore(&clnt->lock, flags); kfree(fid->rdir); kfree(fid); @@ -1028,7 +1024,7 @@ struct p9_client *p9_client_create(const char *dev_name, char *options) memcpy(clnt->name, client_id, strlen(client_id) + 1); spin_lock_init(&clnt->lock); - INIT_LIST_HEAD(&clnt->fidlist); + idr_init(&clnt->fids); err = p9_tag_init(clnt); if (err < 0) @@ -1048,18 +1044,12 @@ struct p9_client *p9_client_create(const char *dev_name, char *options) goto destroy_tagpool; } - clnt->fidpool = p9_idpool_create(); - if (IS_ERR(clnt->fidpool)) { - err = PTR_ERR(clnt->fidpool); - goto put_trans; - } - p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n", clnt, clnt->trans_mod, clnt->msize, clnt->proto_version); err = clnt->trans_mod->create(clnt, dev_name, options); if (err) - goto destroy_fidpool; + goto put_trans; if (clnt->msize > clnt->trans_mod->maxsize) clnt->msize = clnt->trans_mod->maxsize; @@ -1072,8 +1062,6 @@ struct p9_client *p9_client_create(const char *dev_name, char *options) close_trans: clnt->trans_mod->close(clnt); -destroy_fidpool: - p9_idpool_destroy(clnt->fidpool); put_trans: v9fs_put_trans(clnt->trans_mod); destroy_tagpool: @@ -1086,7 +1074,8 @@ EXPORT_SYMBOL(p9_client_create); void p9_client_destroy(struct p9_client *clnt) { - struct p9_fid *fid, *fidptr; + struct p9_fid *fid; + int id; p9_debug(P9_DEBUG_MUX, "clnt %p\n", clnt); @@ -1095,14 +1084,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); - p9_tag_cleanup(clnt); kfree(clnt);
The p9_idpool being used to allocate the IDs uses an IDR to allocate the IDs ... which we then keep in a doubly-linked list, rather than in the IDR which allocated them. We can use an IDR directly which saves two pointers per p9_fid, and a tiny memory allocation per p9_client. Signed-off-by: Matthew Wilcox <willy@infradead.org> --- include/net/9p/client.h | 9 +++------ net/9p/client.c | 42 ++++++++++++++--------------------------- 2 files changed, 17 insertions(+), 34 deletions(-)