Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753809AbcLIXYj (ORCPT ); Fri, 9 Dec 2016 18:24:39 -0500 Received: from mail.kernel.org ([198.145.29.136]:45972 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751334AbcLIXYh (ORCPT ); Fri, 9 Dec 2016 18:24:37 -0500 Date: Fri, 9 Dec 2016 15:24:32 -0800 (PST) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-X260 To: Dominique Martinet cc: Stefano Stabellini , v9fs-developer@lists.sourceforge.net, ericvh@gmail.com, rminnich@sandia.gov, linux-kernel@vger.kernel.org, lucho@ionkov.net Subject: Re: [V9fs-developer] [PATCH 2/5] 9p: store req details and callback in struct p9_req_t In-Reply-To: <20161209071824.GC18158@nautica> Message-ID: References: <1481230746-16741-1-git-send-email-sstabellini@kernel.org> <1481230746-16741-2-git-send-email-sstabellini@kernel.org> <20161209071824.GC18158@nautica> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3201 Lines: 93 On Fri, 9 Dec 2016, Dominique Martinet wrote: > Nice. I like the idea of async I/Os :) > > Stefano Stabellini wrote on Thu, Dec 08, 2016: > > Add a few fields to struct p9_req_t. Callback is the function which will > > be called upon requestion completion. offset, rsize, pagevec and kiocb > > store important information regarding the read or write request, > > essential to complete the request. > > > > Currently not utilized, but they will be used in a later patch. > > > > Signed-off-by: Stefano Stabellini > > --- > > include/net/9p/client.h | 8 ++++++++ > > net/9p/client.c | 9 ++++++++- > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/include/net/9p/client.h b/include/net/9p/client.h > > index aef19c6..69fc2f0 100644 > > --- a/include/net/9p/client.h > > +++ b/include/net/9p/client.h > > @@ -110,6 +110,7 @@ enum p9_req_status_t { > > * > > */ > > > > +struct p9_client; > > struct p9_req_t { > > int status; > > int t_err; > > @@ -118,6 +119,13 @@ struct p9_req_t { > > struct p9_fcall *rc; > > void *aux; > > > > + /* Used for async requests */ > > + void (*callback)(struct p9_client *c, struct p9_req_t *req, int status); > > + size_t offset; > > + u64 rsize; > > + struct page **pagevec; > > + struct kiocb *kiocb; > > + > > struct list_head req_list; > > }; > > > > diff --git a/net/9p/client.c b/net/9p/client.c > > index b5ea9a3..bfe1715 100644 > > --- a/net/9p/client.c > > +++ b/net/9p/client.c > > @@ -405,6 +405,10 @@ static void p9_free_req(struct p9_client *c, struct p9_req_t *r) > > int tag = r->tc->tag; > > p9_debug(P9_DEBUG_MUX, "clnt %p req %p tag: %d\n", c, r, tag); > > > > + r->offset = 0; > > + r->rsize = 0; > > + r->kiocb = NULL; > > + r->callback = NULL; > > Probably want to cleanup r->pagevec here too, even if that doesn't seem > to have any implication short-term (e.g. only looked at if callback is > not empty from what I've seen) Thanks, I missed it. > > r->status = REQ_STATUS_IDLE; > > if (tag != P9_NOTAG && p9_idpool_check(tag, c->tagpool)) > > p9_idpool_put(tag, c->tagpool); > > @@ -427,7 +431,10 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status) > > smp_wmb(); > > req->status = status; > > > > - wake_up(req->wq); > > + if (req->callback != NULL) > > + req->callback(c, req, status); > > + else > > + wake_up(req->wq); > > p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc->tag); > > } > > EXPORT_SYMBOL(p9_client_cb); > > Mostly a warning here, but p9_client_cb is called from an interrupt > context in 9P/RDMA. > This has been working up till now because we only do a wake_up and > there's no waiting, but (looking at later patches), > p9_client_read_complete for example does allocations and possibly other > unsafe operations from an interrupt context. > > I don't know if the way forward is to move p9_client_cb from that > context or to have the callback be scheduled in a work queue instead; > but we'll need to fix that later. Either would work. It might be simpler to have the callback run as a work queue. I'll make the change. Maybe I'll use kiocb to figure out if we have to schedule_work.