LinuxLists.cc - [PATCH 1/3] 9p/net: implement asynchronous rpc

2018-12-11 12:43:53

Subject: [PATCH 1/3] 9p/net: implement asynchronous rpc

From: Dominique Martinet <[email protected]>

Add p9_client_async_rpc that will let us send an rpc without waiting
for the reply, as well as an async handler hook.

The work done in the hook could mostly be done in the client callback,
but I prefered not to for a couple of reasons:
- the callback can be called in an IRQ for some transports, and the
handler needs to call p9_tag_remove which takes the client lock that has
recently been reworked to not be irq-compatible (as it was never taken
in IRQs until now)
- the handled replies can be handled anytime, there is nothing the
userspace would care about and want to be notified of quickly
- the async request list and this function would be needed anyway for
when we disconnect the client, in order to not leak async requests that
haven't been replied to

Signed-off-by: Dominique Martinet <[email protected]>
Cc: Eric Van Hensbergen <[email protected]>
Cc: Latchesar Ionkov <[email protected]>
Cc: Tomas Bortoli <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
---

I've been sitting on these patches for almost a month now because I
wanted to fix the cancel race, but I think it's what the most recent
syzbot email is about so if it could find it without this it probably
won't hurt to at least get some reviews for these three patches first.

I won't submit these for next cycle, so will only put them into -next
after 4.20 is released; hopefully I'll have time to look at the two
pending refcount races around that time.
Until then, comments please!

include/net/9p/client.h | 9 +++++-
net/9p/client.c | 64 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index 947a570307a6..a4ded7666c73 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -89,7 +89,8 @@ enum p9_req_status_t {
* @tc: the request fcall structure
* @rc: the response fcall structure
* @aux: transport specific data (provided for trans_fd migration)
- * @req_list: link for higher level objects to chain requests
+ * @req_list: link used by trans_fd
+ * @async_list: link used to check on async requests
*/
struct p9_req_t {
int status;
@@ -100,6 +101,7 @@ struct p9_req_t {
struct p9_fcall rc;
void *aux;
struct list_head req_list;
+ struct list_head async_list;
};

/**
@@ -110,6 +112,9 @@ struct p9_req_t {
* @trans_mod: module API instantiated with this client
* @status: connection state
* @trans: tranport instance state and API
+ * @fcall_cache: kmem cache for client request data (msize-specific)
+ * @async_req_lock: protects @async_req_list
+ * @async_req_list: list of requests waiting a reply
* @fids: All active FID handles
* @reqs: All active requests.
* @name: node name used as client id
@@ -125,6 +130,8 @@ struct p9_client {
enum p9_trans_status status;
void *trans;
struct kmem_cache *fcall_cache;
+ spinlock_t async_req_lock;
+ struct list_head async_req_list;

union {
struct {
diff --git a/net/9p/client.c b/net/9p/client.c
index 357214a51f13..0a67c3ccd4fd 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -729,6 +729,62 @@ static struct p9_req_t *p9_client_prepare_req(struct p9_client *c,
return ERR_PTR(err);
}

+static struct p9_req_t *
+p9_client_async_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
+{
+ va_list ap;
+ int err;
+ struct p9_req_t *req;
+
+ va_start(ap, fmt);
+ req = p9_client_prepare_req(c, type, c->msize, fmt, ap);
+ va_end(ap);
+ if (IS_ERR(req))
+ return req;
+
+ err = c->trans_mod->request(c, req);
+ if (err < 0) {
+ /* bail out without flushing... */
+ p9_req_put(req);
+ p9_tag_remove(c, req);
+ if (err != -ERESTARTSYS && err != -EFAULT)
+ c->status = Disconnected;
+ return ERR_PTR(safe_errno(err));
+ }
+
+ return req;
+}
+
+static void p9_client_handle_async(struct p9_client *c, bool free_all)
+{
+ struct p9_req_t *req, *nreq;
+
+ /* it's ok to miss some async replies here, do a quick check without
+ * lock first unless called with free_all
+ */
+ if (!free_all && list_empty(&c->async_req_list))
+ return;
+
+ spin_lock_irq(&c->async_req_lock);
+ list_for_each_entry_safe(req, nreq, &c->async_req_list, async_list) {
+ if (free_all && req->status < REQ_STATUS_RCVD) {
+ p9_debug(P9_DEBUG_ERROR,
+ "final async handler found req tag %d type %d status %d\n",
+ req->tc.tag, req->tc.id, req->status);
+ if (c->trans_mod->cancelled)
+ c->trans_mod->cancelled(c, req);
+ /* won't receive reply now */
+ p9_req_put(req);
+ }
+ if (free_all || req->status >= REQ_STATUS_RCVD) {
+ /* Put old refs whatever reqs actually returned */
+ list_del(&req->async_list);
+ p9_tag_remove(c, req);
+ }
+ }
+ spin_unlock_irq(&c->async_req_lock);
+}
+
/**
* p9_client_rpc - issue a request and wait for a response
* @c: client session
@@ -746,6 +802,8 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
unsigned long flags;
struct p9_req_t *req;

+ p9_client_handle_async(c, 0);
+
va_start(ap, fmt);
req = p9_client_prepare_req(c, type, c->msize, fmt, ap);
va_end(ap);
@@ -841,6 +899,8 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
unsigned long flags;
struct p9_req_t *req;

+ p9_client_handle_async(c, 0);
+
va_start(ap, fmt);
/*
* We allocate a inline protocol data of only 4k bytes.
@@ -1030,6 +1090,8 @@ 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);
+ spin_lock_init(&clnt->async_req_lock);
+ INIT_LIST_HEAD(&clnt->async_req_list);
idr_init(&clnt->fids);
idr_init(&clnt->reqs);

@@ -1101,6 +1163,8 @@ void p9_client_destroy(struct p9_client *clnt)

v9fs_put_trans(clnt->trans_mod);

+ p9_client_handle_async(clnt, 1);
+
idr_for_each_entry(&clnt->fids, fid, id) {
pr_info("Found fid %d not clunked\n", fid->fid);
p9_fid_destroy(fid);
--
2.19.2

2018-12-11 12:43:33

by Dominique Martinet

[permalink] [raw]

Subject: [PATCH 3/3] 9p/net: make flush asynchronous

From: Dominique Martinet <[email protected]>

Make the client flush asynchronous so we don't need to ignore signals in
rpc functions:
- on signal, send a flush request as we previously did but use the new
asynchronous helper and return immediately
- when the reply has been received or connection is destroyed, free
both tags in the handler

Signed-off-by: Dominique Martinet <[email protected]>
Cc: Eric Van Hensbergen <[email protected]>
Cc: Latchesar Ionkov <[email protected]>
Cc: Tomas Bortoli <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
---
include/net/9p/client.h | 2 +
net/9p/client.c | 172 ++++++++++++++++++----------------------
2 files changed, 78 insertions(+), 96 deletions(-)

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index 75d7f83e5b94..dcd40e7ef202 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -91,6 +91,7 @@ enum p9_req_status_t {
* @aux: transport specific data (provided for trans_fd migration)
* @req_list: link used by trans_fd
* @async_list: link used to check on async requests
+ * @flushed_req: for flush, points to matching flushed req
* @clunked_fid: for clunk, points to fid
*/
struct p9_req_t {
@@ -104,6 +105,7 @@ struct p9_req_t {
struct list_head req_list;
struct list_head async_list;
union {
+ struct p9_req_t *flushed_req;
struct p9_fid *clunked_fid;
};
};
diff --git a/net/9p/client.c b/net/9p/client.c
index a47b5a54573d..666a722088e9 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -694,50 +694,6 @@ static void p9_fid_destroy(struct p9_fid *fid)
kfree(fid);
}

-static struct p9_req_t *
-p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...);
-
-/**
- * p9_client_flush - flush (cancel) a request
- * @c: client state
- * @oldreq: request to cancel
- *
- * This sents a flush for a particular request and links
- * the flush request to the original request. The current
- * code only supports a single flush request although the protocol
- * allows for multiple flush requests to be sent for a single request.
- *
- */
-
-static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq)
-{
- struct p9_req_t *req;
- int16_t oldtag;
- int err;
-
- err = p9_parse_header(&oldreq->tc, NULL, NULL, &oldtag, 1);
- if (err)
- return err;
-
- p9_debug(P9_DEBUG_9P, ">>> TFLUSH tag %d\n", oldtag);
-
- req = p9_client_rpc(c, P9_TFLUSH, "w", oldtag);
- if (IS_ERR(req))
- return PTR_ERR(req);
-
- /*
- * if we haven't received a response for oldreq,
- * remove it from the list
- */
- if (oldreq->status == REQ_STATUS_SENT) {
- if (c->trans_mod->cancelled)
- c->trans_mod->cancelled(c, oldreq);
- }
-
- p9_tag_remove(c, req);
- return 0;
-}
-
static struct p9_req_t *p9_client_prepare_req(struct p9_client *c,
int8_t type, int req_size,
const char *fmt, va_list ap)
@@ -800,6 +756,39 @@ p9_client_async_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
return req;
}

+/**
+ * p9_client_flush - flush (cancel) a request
+ * @c: client state
+ * @oldreq: request to cancel
+ *
+ * This sents a flush for a particular request and links
+ * the flush request to the original request. The current
+ * code only supports a single flush request although the protocol
+ * allows for multiple flush requests to be sent for a single request.
+ *
+ */
+
+static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq)
+{
+ struct p9_req_t *req;
+ int16_t oldtag = oldreq->tc.tag;
+
+ p9_debug(P9_DEBUG_9P, ">>> TFLUSH tag %d\n", oldtag);
+ req = p9_client_async_rpc(c, P9_TFLUSH, "w", oldtag);
+ if (IS_ERR(req)) {
+ return PTR_ERR(req);
+ }
+
+ p9_debug(P9_DEBUG_MUX, "sent flush for oldreq %d type %d with flush tag %d\n",
+ oldtag, oldreq->tc.id, req->tc.tag);
+ req->flushed_req = oldreq;
+ spin_lock_irq(&c->lock);
+ list_add(&req->async_list, &c->async_req_list);
+ spin_unlock_irq(&c->lock);
+
+ return 0;
+}
+
static void p9_client_handle_async(struct p9_client *c, bool free_all)
{
struct p9_req_t *req, *nreq;
@@ -823,6 +812,24 @@ static void p9_client_handle_async(struct p9_client *c, bool free_all)
}
if (free_all || req->status >= REQ_STATUS_RCVD) {
/* Put old refs whatever reqs actually returned */
+ if (req->tc.id == P9_TFLUSH) {
+ p9_debug(P9_DEBUG_MUX,
+ "flushing oldreq tag %d status %d, flush_req tag %d\n",
+ req->flushed_req->tc.tag,
+ req->flushed_req->status,
+ req->tc.tag);
+ if (req->flushed_req->status < REQ_STATUS_RCVD) {
+ /* won't receive reply now */
+ if (c->trans_mod->cancelled)
+ c->trans_mod->cancelled(c, req);
+ p9_req_put(req->flushed_req);
+ }
+ if (!p9_tag_remove(c, req->flushed_req))
+ p9_debug(P9_DEBUG_ERROR,
+ "oldreq tag %d status %d still has ref\n",
+ req->flushed_req->tc.tag,
+ req->flushed_req->status);
+ }
if (req->tc.id == P9_TCLUNK) {
p9_fid_destroy(req->clunked_fid);
}
@@ -846,8 +853,8 @@ static struct p9_req_t *
p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
{
va_list ap;
- int sigpending, err;
- unsigned long flags;
+ int err;
+ int flushing = 0;
struct p9_req_t *req;

p9_client_handle_async(c, 0);
@@ -859,10 +866,11 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
return req;

if (signal_pending(current)) {
- sigpending = 1;
- clear_thread_flag(TIF_SIGPENDING);
- } else
- sigpending = 0;
+ err = -ERESTARTSYS;
+ /* write won't happen */
+ p9_req_put(req);
+ goto reterr;
+ }

err = c->trans_mod->request(c, req);
if (err < 0) {
@@ -870,9 +878,9 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
p9_req_put(req);
if (err != -ERESTARTSYS && err != -EFAULT)
c->status = Disconnected;
- goto recalc_sigpending;
+ goto reterr;
}
-again:
+
/* Wait for the response */
err = wait_event_killable(req->wq, req->status >= REQ_STATUS_RCVD);

@@ -882,34 +890,15 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
*/
smp_rmb();

- if ((err == -ERESTARTSYS) && (c->status == Connected)
- && (type == P9_TFLUSH)) {
- sigpending = 1;
- clear_thread_flag(TIF_SIGPENDING);
- goto again;
- }
-
if (req->status == REQ_STATUS_ERROR) {
p9_debug(P9_DEBUG_ERROR, "req_status error %d\n", req->t_err);
err = req->t_err;
}
if ((err == -ERESTARTSYS) && (c->status == Connected)) {
p9_debug(P9_DEBUG_MUX, "flushing\n");
- sigpending = 1;
- clear_thread_flag(TIF_SIGPENDING);
-
- if (c->trans_mod->cancel(c, req))
- p9_client_flush(c, req);

- /* if we received the response anyway, don't signal error */
- if (req->status == REQ_STATUS_RCVD)
- err = 0;
- }
-recalc_sigpending:
- if (sigpending) {
- spin_lock_irqsave(&current->sighand->siglock, flags);
- recalc_sigpending();
- spin_unlock_irqrestore(&current->sighand->siglock, flags);
+ if (c->trans_mod->cancel(c, req) && !p9_client_flush(c, req))
+ flushing = 1;
}
if (err < 0)
goto reterr;
@@ -919,7 +908,8 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
if (!err)
return req;
reterr:
- p9_tag_remove(c, req);
+ if (!flushing)
+ p9_tag_remove(c, req);
return ERR_PTR(safe_errno(err));
}

@@ -943,8 +933,8 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
const char *fmt, ...)
{
va_list ap;
- int sigpending, err;
- unsigned long flags;
+ int err;
+ int flushing = 0;
struct p9_req_t *req;

p9_client_handle_async(c, 0);
@@ -960,10 +950,11 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
return req;

if (signal_pending(current)) {
- sigpending = 1;
- clear_thread_flag(TIF_SIGPENDING);
- } else
- sigpending = 0;
+ err = -ERESTARTSYS;
+ /* write won't happen */
+ p9_req_put(req);
+ goto reterr;
+ }

err = c->trans_mod->zc_request(c, req, uidata, uodata,
inlen, olen, in_hdrlen);
@@ -971,7 +962,7 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
if (err == -EIO)
c->status = Disconnected;
if (err != -ERESTARTSYS)
- goto recalc_sigpending;
+ goto reterr;
}
if (req->status == REQ_STATUS_ERROR) {
p9_debug(P9_DEBUG_ERROR, "req_status error %d\n", req->t_err);
@@ -979,21 +970,9 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
}
if ((err == -ERESTARTSYS) && (c->status == Connected)) {
p9_debug(P9_DEBUG_MUX, "flushing\n");
- sigpending = 1;
- clear_thread_flag(TIF_SIGPENDING);
-
- if (c->trans_mod->cancel(c, req))
- p9_client_flush(c, req);

- /* if we received the response anyway, don't signal error */
- if (req->status == REQ_STATUS_RCVD)
- err = 0;
- }
-recalc_sigpending:
- if (sigpending) {
- spin_lock_irqsave(&current->sighand->siglock, flags);
- recalc_sigpending();
- spin_unlock_irqrestore(&current->sighand->siglock, flags);
+ if (c->trans_mod->cancel(c, req) && !p9_client_flush(c, req))
+ flushing = 1;
}
if (err < 0)
goto reterr;
@@ -1003,7 +982,8 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
if (!err)
return req;
reterr:
- p9_tag_remove(c, req);
+ if (!flushing)
+ p9_tag_remove(c, req);
return ERR_PTR(safe_errno(err));
}

--
2.19.2

2018-12-11 12:43:35

by Dominique Martinet

[permalink] [raw]

Subject: [PATCH 2/3] 9p/net: make clunk asynchronous

From: Dominique Martinet <[email protected]>

clunk is defined as making the fid invalid whatever the server returns,
and we should ignore errors so it is a good candidate for async call.

The change should make 9p slightly faster (many vfs systeme calls won't
need to wait for that clunk), but more importantly the flush rework
means we won't clear pending signals and the current implementation of
"retry twice then leak the fid" will stop working, so this needed
improving.

Signed-off-by: Dominique Martinet <[email protected]>
Cc: Eric Van Hensbergen <[email protected]>
Cc: Latchesar Ionkov <[email protected]>
Cc: Tomas Bortoli <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
---
include/net/9p/client.h | 4 ++
net/9p/client.c | 124 +++++++++++++++++++---------------------
2 files changed, 62 insertions(+), 66 deletions(-)

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index a4ded7666c73..75d7f83e5b94 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -91,6 +91,7 @@ enum p9_req_status_t {
* @aux: transport specific data (provided for trans_fd migration)
* @req_list: link used by trans_fd
* @async_list: link used to check on async requests
+ * @clunked_fid: for clunk, points to fid
*/
struct p9_req_t {
int status;
@@ -102,6 +103,9 @@ struct p9_req_t {
void *aux;
struct list_head req_list;
struct list_head async_list;
+ union {
+ struct p9_fid *clunked_fid;
+ };
};

/**
diff --git a/net/9p/client.c b/net/9p/client.c
index 0a67c3ccd4fd..a47b5a54573d 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -649,6 +649,51 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req,
return err;
}

+static struct p9_fid *p9_fid_create(struct p9_client *clnt)
+{
+ int ret;
+ struct p9_fid *fid;
+
+ p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
+ fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);
+ if (!fid)
+ return NULL;
+
+ memset(&fid->qid, 0, sizeof(struct p9_qid));
+ fid->mode = -1;
+ fid->uid = current_fsuid();
+ fid->clnt = clnt;
+ fid->rdir = NULL;
+ fid->fid = 0;
+
+ idr_preload(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);
+ idr_preload_end();
+
+ if (!ret)
+ return fid;
+
+ kfree(fid);
+ return NULL;
+}
+
+static void p9_fid_destroy(struct p9_fid *fid)
+{
+ struct p9_client *clnt;
+ unsigned long flags;
+
+ p9_debug(P9_DEBUG_FID, "fid %d\n", fid->fid);
+ clnt = fid->clnt;
+ spin_lock_irqsave(&clnt->lock, flags);
+ idr_remove(&clnt->fids, fid->fid);
+ spin_unlock_irqrestore(&clnt->lock, flags);
+ kfree(fid->rdir);
+ kfree(fid);
+}
+
static struct p9_req_t *
p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...);

@@ -778,6 +823,9 @@ static void p9_client_handle_async(struct p9_client *c, bool free_all)
}
if (free_all || req->status >= REQ_STATUS_RCVD) {
/* Put old refs whatever reqs actually returned */
+ if (req->tc.id == P9_TCLUNK) {
+ p9_fid_destroy(req->clunked_fid);
+ }
list_del(&req->async_list);
p9_tag_remove(c, req);
}
@@ -959,51 +1007,6 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
return ERR_PTR(safe_errno(err));
}

-static struct p9_fid *p9_fid_create(struct p9_client *clnt)
-{
- int ret;
- struct p9_fid *fid;
-
- p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
- fid = kmalloc(sizeof(struct p9_fid), GFP_KERNEL);
- if (!fid)
- return NULL;
-
- memset(&fid->qid, 0, sizeof(struct p9_qid));
- fid->mode = -1;
- fid->uid = current_fsuid();
- fid->clnt = clnt;
- fid->rdir = NULL;
- fid->fid = 0;
-
- idr_preload(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);
- idr_preload_end();
-
- if (!ret)
- return fid;
-
- kfree(fid);
- return NULL;
-}
-
-static void p9_fid_destroy(struct p9_fid *fid)
-{
- struct p9_client *clnt;
- unsigned long flags;
-
- p9_debug(P9_DEBUG_FID, "fid %d\n", fid->fid);
- clnt = fid->clnt;
- spin_lock_irqsave(&clnt->lock, flags);
- idr_remove(&clnt->fids, fid->fid);
- spin_unlock_irqrestore(&clnt->lock, flags);
- kfree(fid->rdir);
- kfree(fid);
-}
-
static int p9_client_version(struct p9_client *c)
{
int err = 0;
@@ -1534,7 +1537,6 @@ int p9_client_clunk(struct p9_fid *fid)
int err;
struct p9_client *clnt;
struct p9_req_t *req;
- int retries = 0;

if (!fid) {
pr_warn("%s (%d): Trying to clunk with NULL fid\n",
@@ -1543,33 +1545,23 @@ int p9_client_clunk(struct p9_fid *fid)
return 0;
}

-again:
- p9_debug(P9_DEBUG_9P, ">>> TCLUNK fid %d (try %d)\n", fid->fid,
- retries);
+ p9_debug(P9_DEBUG_9P, ">>> TCLUNK fid %d\n", fid->fid);
err = 0;
clnt = fid->clnt;

- req = p9_client_rpc(clnt, P9_TCLUNK, "d", fid->fid);
+ req = p9_client_async_rpc(clnt, P9_TCLUNK, "d", fid->fid);
if (IS_ERR(req)) {
- err = PTR_ERR(req);
- goto error;
+ return PTR_ERR(req);
}

- p9_debug(P9_DEBUG_9P, "<<< RCLUNK fid %d\n", fid->fid);
+ p9_debug(P9_DEBUG_MUX, "sent clunk for fid %d, tag %d\n",
+ fid->fid, req->tc.tag);
+ req->clunked_fid = fid;
+ spin_lock_irq(&clnt->lock);
+ list_add(&req->async_list, &clnt->async_req_list);
+ spin_unlock_irq(&clnt->lock);

- p9_tag_remove(clnt, req);
-error:
- /*
- * Fid is not valid even after a failed clunk
- * If interrupted, retry once then give up and
- * leak fid until umount.
- */
- if (err == -ERESTARTSYS) {
- if (retries++ == 0)
- goto again;
- } else
- p9_fid_destroy(fid);
- return err;
+ return 0;
}
EXPORT_SYMBOL(p9_client_clunk);

--
2.19.2

2018-12-17 07:41:30

by Tomas Bortoli

[permalink] [raw]

Subject: Re: [PATCH 1/3] 9p/net: implement asynchronous rpc

Hey Dominique,

sorry for the delay, I've been quite busy these days.

The patches looks good to me and should indeed speed up the code a bit.
I quickly tested them against Syzkaller tuned for the 9p subsystem and
everything seems fine.

And by the way, which refcount races?

Cheers,
Tomas

On 12/11/18 1:41 PM, Dominique Martinet wrote:
> From: Dominique Martinet <[email protected]>
>
> Add p9_client_async_rpc that will let us send an rpc without waiting
> for the reply, as well as an async handler hook.
>
> The work done in the hook could mostly be done in the client callback,
> but I prefered not to for a couple of reasons:
> - the callback can be called in an IRQ for some transports, and the
> handler needs to call p9_tag_remove which takes the client lock that has
> recently been reworked to not be irq-compatible (as it was never taken
> in IRQs until now)
> - the handled replies can be handled anytime, there is nothing the
> userspace would care about and want to be notified of quickly
> - the async request list and this function would be needed anyway for
> when we disconnect the client, in order to not leak async requests that
> haven't been replied to
>
> Signed-off-by: Dominique Martinet <[email protected]>
> Cc: Eric Van Hensbergen <[email protected]>
> Cc: Latchesar Ionkov <[email protected]>
> Cc: Tomas Bortoli <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> ---
>
> I've been sitting on these patches for almost a month now because I
> wanted to fix the cancel race, but I think it's what the most recent
> syzbot email is about so if it could find it without this it probably
> won't hurt to at least get some reviews for these three patches first.
>
> I won't submit these for next cycle, so will only put them into -next
> after 4.20 is released; hopefully I'll have time to look at the two
> pending refcount races around that time.

> Until then, comments please!
>
>
> include/net/9p/client.h | 9 +++++-
> net/9p/client.c | 64 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/9p/client.h b/include/net/9p/client.h
> index 947a570307a6..a4ded7666c73 100644
> --- a/include/net/9p/client.h
> +++ b/include/net/9p/client.h
> @@ -89,7 +89,8 @@ enum p9_req_status_t {
> * @tc: the request fcall structure
> * @rc: the response fcall structure
> * @aux: transport specific data (provided for trans_fd migration)
> - * @req_list: link for higher level objects to chain requests
> + * @req_list: link used by trans_fd
> + * @async_list: link used to check on async requests
> */
> struct p9_req_t {
> int status;
> @@ -100,6 +101,7 @@ struct p9_req_t {
> struct p9_fcall rc;
> void *aux;
> struct list_head req_list;
> + struct list_head async_list;
> };
>
> /**
> @@ -110,6 +112,9 @@ struct p9_req_t {
> * @trans_mod: module API instantiated with this client
> * @status: connection state
> * @trans: tranport instance state and API
> + * @fcall_cache: kmem cache for client request data (msize-specific)
> + * @async_req_lock: protects @async_req_list
> + * @async_req_list: list of requests waiting a reply
> * @fids: All active FID handles
> * @reqs: All active requests.
> * @name: node name used as client id
> @@ -125,6 +130,8 @@ struct p9_client {
> enum p9_trans_status status;
> void *trans;
> struct kmem_cache *fcall_cache;
> + spinlock_t async_req_lock;
> + struct list_head async_req_list;
>
> union {
> struct {
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 357214a51f13..0a67c3ccd4fd 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -729,6 +729,62 @@ static struct p9_req_t *p9_client_prepare_req(struct p9_client *c,
> return ERR_PTR(err);
> }
>
> +static struct p9_req_t *
> +p9_client_async_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
> +{
> + va_list ap;
> + int err;
> + struct p9_req_t *req;
> +
> + va_start(ap, fmt);
> + req = p9_client_prepare_req(c, type, c->msize, fmt, ap);
> + va_end(ap);
> + if (IS_ERR(req))
> + return req;
> +
> + err = c->trans_mod->request(c, req);
> + if (err < 0) {
> + /* bail out without flushing... */
> + p9_req_put(req);
> + p9_tag_remove(c, req);
> + if (err != -ERESTARTSYS && err != -EFAULT)
> + c->status = Disconnected;
> + return ERR_PTR(safe_errno(err));
> + }
> +
> + return req;
> +}
> +
> +static void p9_client_handle_async(struct p9_client *c, bool free_all)
> +{
> + struct p9_req_t *req, *nreq;
> +
> + /* it's ok to miss some async replies here, do a quick check without
> + * lock first unless called with free_all
> + */
> + if (!free_all && list_empty(&c->async_req_list))
> + return;
> +
> + spin_lock_irq(&c->async_req_lock);
> + list_for_each_entry_safe(req, nreq, &c->async_req_list, async_list) {
> + if (free_all && req->status < REQ_STATUS_RCVD) {
> + p9_debug(P9_DEBUG_ERROR,
> + "final async handler found req tag %d type %d status %d\n",
> + req->tc.tag, req->tc.id, req->status);
> + if (c->trans_mod->cancelled)
> + c->trans_mod->cancelled(c, req);
> + /* won't receive reply now */
> + p9_req_put(req);
> + }
> + if (free_all || req->status >= REQ_STATUS_RCVD) {
> + /* Put old refs whatever reqs actually returned */
> + list_del(&req->async_list);
> + p9_tag_remove(c, req);
> + }
> + }
> + spin_unlock_irq(&c->async_req_lock);
> +}
> +
> /**
> * p9_client_rpc - issue a request and wait for a response
> * @c: client session
> @@ -746,6 +802,8 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
> unsigned long flags;
> struct p9_req_t *req;
>
> + p9_client_handle_async(c, 0);
> +
> va_start(ap, fmt);
> req = p9_client_prepare_req(c, type, c->msize, fmt, ap);
> va_end(ap);
> @@ -841,6 +899,8 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
> unsigned long flags;
> struct p9_req_t *req;
>
> + p9_client_handle_async(c, 0);
> +
> va_start(ap, fmt);
> /*
> * We allocate a inline protocol data of only 4k bytes.
> @@ -1030,6 +1090,8 @@ 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);
> + spin_lock_init(&clnt->async_req_lock);
> + INIT_LIST_HEAD(&clnt->async_req_list);
> idr_init(&clnt->fids);
> idr_init(&clnt->reqs);
>
> @@ -1101,6 +1163,8 @@ void p9_client_destroy(struct p9_client *clnt)
>
> v9fs_put_trans(clnt->trans_mod);
>
> + p9_client_handle_async(clnt, 1);
> +
> idr_for_each_entry(&clnt->fids, fid, id) {
> pr_info("Found fid %d not clunked\n", fid->fid);
> p9_fid_destroy(fid);
>

2018-12-17 08:14:15

by Tomas Bortoli

[permalink] [raw]

Subject: Re: [PATCH 1/3] 9p/net: implement asynchronous rpc

2018-12-17 11:06:56

by Dominique Martinet

[permalink] [raw]

Subject: Re: [PATCH 1/3] 9p/net: implement asynchronous rpc

Tomas Bortoli wrote on Mon, Dec 17, 2018:
> sorry for the delay, I've been quite busy these days.

No problem.

> The patches looks good to me and should indeed speed up the code a bit.
> I quickly tested them against Syzkaller tuned for the 9p subsystem and
> everything seems fine.

Thanks, can I add your Reviewed-by on all three?

> And by the way, which refcount races?

There's a problem with trans_fd read_work and cancelled callback; I'm
not so sure about refcount but we can definitely get double list_del
as we're not checking the status. I think when we incorrectly remove
from the list we also mismanage the refcount, but honestly need to
test..

--
Dominique

2018-12-17 23:25:07

by Tomas Bortoli

[permalink] [raw]

Subject: Re: [PATCH 1/3] 9p/net: implement asynchronous rpc

On 12/17/18 12:01 PM, Dominique Martinet wrote:
> Tomas Bortoli wrote on Mon, Dec 17, 2018:
>> sorry for the delay, I've been quite busy these days.
>
> No problem.
>
>> The patches looks good to me and should indeed speed up the code a bit.
>> I quickly tested them against Syzkaller tuned for the 9p subsystem and
>> everything seems fine.
>
> Thanks, can I add your Reviewed-by on all three?
>

Sure, FWIW.

>> And by the way, which refcount races?
>
> There's a problem with trans_fd read_work and cancelled callback; I'm
> not so sure about refcount but we can definitely get double list_del
> as we're not checking the status. I think when we incorrectly remove
> from the list we also mismanage the refcount, but honestly need to
> test..
>

Yeah, definitely it needs to check the status. Btw, if a double
list_del happens the kernel should crash.