From patchwork Mon Jul 9 14:31:25 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Van Hensbergen X-Patchwork-Id: 1173161 Return-Path: X-Original-To: patchwork-v9fs-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from lists.sourceforge.net (lists.sourceforge.net [216.34.181.88]) by patchwork1.kernel.org (Postfix) with ESMTP id CA8643FC2A for ; Mon, 9 Jul 2012 14:32:19 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=sfs-ml-3.v29.ch3.sourceforge.com) by sfs-ml-3.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1SoF0M-0006KY-3h; Mon, 09 Jul 2012 14:32:02 +0000 Received: from sog-mx-2.v43.ch3.sourceforge.com ([172.29.43.192] helo=mx.sourceforge.net) by sfs-ml-3.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1SoF0K-0006KS-79 for v9fs-developer@lists.sourceforge.net; Mon, 09 Jul 2012 14:32:00 +0000 Received-SPF: pass (sog-mx-2.v43.ch3.sourceforge.com: domain of gmail.com designates 74.125.82.175 as permitted sender) client-ip=74.125.82.175; envelope-from=ericvh@gmail.com; helo=mail-we0-f175.google.com; Received: from mail-we0-f175.google.com ([74.125.82.175]) by sog-mx-2.v43.ch3.sourceforge.com with esmtps (TLSv1:RC4-SHA:128) (Exim 4.76) id 1SoF0E-00088z-2j for v9fs-developer@lists.sourceforge.net; Mon, 09 Jul 2012 14:32:00 +0000 Received: by weyr6 with SMTP id r6so5135655wey.34 for ; Mon, 09 Jul 2012 07:31:45 -0700 (PDT) Received: by 10.180.84.104 with SMTP id x8mr29932372wiy.20.1341844305414; Mon, 09 Jul 2012 07:31:45 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.144.5 with HTTP; Mon, 9 Jul 2012 07:31:25 -0700 (PDT) In-Reply-To: <4FF6EC88.3040107@cea.fr> References: <4FF6EC88.3040107@cea.fr> From: Eric Van Hensbergen Date: Mon, 9 Jul 2012 09:31:25 -0500 Message-ID: To: DENIEL Philippe X-Spam-Score: -1.6 (-) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for sender-domain 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (ericvh[at]gmail.com) -0.0 SPF_PASS SPF: sender matches SPF record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature X-Headers-End: 1SoF0E-00088z-2j Cc: v9fs-developer@lists.sourceforge.net, Jim McKie , Anthony Liguori Subject: Re: [V9fs-developer] TREADDIR: issue when cookie has 64th bit set to 1 X-BeenThere: v9fs-developer@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: v9fs-developer-bounces@lists.sourceforge.net On Fri, Jul 6, 2012 at 8:47 AM, DENIEL Philippe wrote: > Hi, > > I faced a weird issue when running cthon04's test suite. Client was > kernel 3.4.0 and server was my latest version of ganesha with 9p.2000L > support. Hi. Thanks for the report, looking into this today. > You have to know that ganesha uses AVL tree to store directory content. > This means that readdir cookies are in fact keys to access the data in > the AVL tree. This key is computed using the murmur3 algorith, which is > pretty efficient but produces very "big" 64 bits integer (it uses of all > the 64 available bits, including the last one). I use the AVL key as a > readdir cookie in my implementation of TREADDIR. I first had an > unexpected issue: the "telldir" step for the cthon04's special tests > failed. While debugging, I realized that it happened when a TREADDIR was > called with a starting cookie whose value had its 64th bit set to 1. I think I'm having a bit of terminology mismatch in my head. When you say cookie, can you map that to the 9p protocol element? It sounds like maybe you are "overloading" the offset to be this cookie? > Other cookies were OK. So I forced the 64 bits of my cookies to be 0 (it > does not break the internal logic in ganesha) and everything was fixed. > It seems to me that the kernel's 9P implementation does not like 64 bits > long integer whose "sign bit" is set. I suspect something like a > uint64_t managed as a int64_t. Since ganesha has very strange cookies, I > guess I am probably the first one to use v9fs with cookie whose 64bit is > set. I am afraid this is a bug. Looking through the code, the neo-protocol code which I copied from Anthony's original qemu 9p server uses int8_t, int16_t, int32_t, and int64_t for all the protocol conversions. Looking at the Plan 9 code, it seems like most things are handled as uchar, ushort, u32int, and vlongs (so only 64 bit is signed?). A quick search/replace of relevant pieces of client and protocol to convert everything to u8, u16, u32, u64 seems to pass my rudimentary regressions. I'm including the patch here for Daniel to test and copying Anthony to find out what I probably just broke and jmk to get the official Plan 9 read on if there is a good reason why the 64-bit quantities are signed in struct Fcall, but all the other bits are unsigned. And of course I'll blame any stupidity on my part on it being early Monday morning before coffee. -eric ---- Original protocol specs from Plan 9 show most protocol operations as being unsigned. Problems exposed when running with ganesha server may be linked to signed coversion issues on the client, swap all protocol ops back to unsigned. Signed-off-by: Eric Van Hensbergen --- include/net/9p/client.h | 4 ++-- net/9p/client.c | 30 +++++++++++++-------------- net/9p/protocol.c | 52 +++++++++++++++++++++++------------------------ net/9p/protocol.h | 2 +- net/9p/trans_rdma.c | 2 +- 5 files changed, 45 insertions(+), 45 deletions(-) ib_dma_unmap_single(rdma->cm_id->device, c->busa, client->msize, diff --git a/include/net/9p/client.h b/include/net/9p/client.h index fc9b90b..16b1084 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -221,7 +221,7 @@ void p9_client_disconnect(struct p9_client *clnt); void p9_client_begin_disconnect(struct p9_client *clnt); struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid, char *uname, u32 n_uname, char *aname); -struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname, +struct p9_fid *p9_client_walk(struct p9_fid *oldfid, u16 nwname, char **wnames, int clone); int p9_client_open(struct p9_fid *fid, int mode); int p9_client_fcreate(struct p9_fid *fid, char *name, u32 perm, int mode, @@ -258,7 +258,7 @@ int p9_client_getlock_dotl(struct p9_fid *fid, struct p9_getlock *fl); struct p9_req_t *p9_tag_lookup(struct p9_client *, u16); void p9_client_cb(struct p9_client *c, struct p9_req_t *req); -int p9_parse_header(struct p9_fcall *, int32_t *, int8_t *, int16_t *, int); +int p9_parse_header(struct p9_fcall *, u32 *, u8 *, u16 *, int); int p9stat_read(struct p9_client *, char *, int, struct p9_wstat *); void p9stat_free(struct p9_wstat *); diff --git a/net/9p/client.c b/net/9p/client.c index a170893..93b76c1 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -411,12 +411,12 @@ EXPORT_SYMBOL(p9_client_cb); */ int -p9_parse_header(struct p9_fcall *pdu, int32_t *size, int8_t *type, int16_t *tag, +p9_parse_header(struct p9_fcall *pdu, u32 *size, u8 *type, u16 *tag, int rewind) { - int8_t r_type; - int16_t r_tag; - int32_t r_size; + u8 r_type; + u16 r_tag; + u32 r_size; int offset = pdu->offset; int err; @@ -463,9 +463,9 @@ EXPORT_SYMBOL(p9_parse_header); static int p9_check_errors(struct p9_client *c, struct p9_req_t *req) { - int8_t type; + u8 type; int err; - int ecode; + uint ecode; err = p9_parse_header(req->rc, NULL, &type, NULL, 0); /* @@ -529,7 +529,7 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req, { int err; int ecode; - int8_t type; + u8 type; char *ename = NULL; err = p9_parse_header(req->rc, NULL, &type, NULL, 0); @@ -548,7 +548,7 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req, if (!p9_is_proto_dotl(c)) { /* Error is reported in string format */ - uint16_t len; + u16 len; /* 7 = header size for RERROR, 2 is the size of string len; */ int inline_len = in_hdrlen - (7 + 2); @@ -622,7 +622,7 @@ out_err: } static struct p9_req_t * -p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...); +p9_client_rpc(struct p9_client *c, u8 type, const char *fmt, ...); /** * p9_client_flush - flush (cancel) a request @@ -639,7 +639,7 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...); static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq) { struct p9_req_t *req; - int16_t oldtag; + u16 oldtag; int err; err = p9_parse_header(oldreq->tc, NULL, NULL, &oldtag, 1); @@ -665,7 +665,7 @@ static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq) } static struct p9_req_t *p9_client_prepare_req(struct p9_client *c, - int8_t type, int req_size, + u8 type, int req_size, const char *fmt, va_list ap) { int tag, err; @@ -715,7 +715,7 @@ reterr: */ static struct p9_req_t * -p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...) +p9_client_rpc(struct p9_client *c, u8 type, const char *fmt, ...) { va_list ap; int sigpending, err; @@ -798,7 +798,7 @@ reterr: * * Returns request structure (which client must free using p9_free_req) */ -static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type, +static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, u8 type, char *uidata, char *uodata, int inlen, int olen, int in_hdrlen, int kern_buf, const char *fmt, ...) @@ -1132,7 +1132,7 @@ error: } EXPORT_SYMBOL(p9_client_attach); -struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname, +struct p9_fid *p9_client_walk(struct p9_fid *oldfid, u16 nwname, char **wnames, int clone) { int err; @@ -1140,7 +1140,7 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname, struct p9_fid *fid; struct p9_qid *wqids; struct p9_req_t *req; - uint16_t nwqids, count; + u16 nwqids, count; err = 0; wqids = NULL; diff --git a/net/9p/protocol.c b/net/9p/protocol.c index 9ee48cb..18d9b25 100644 --- a/net/9p/protocol.c +++ b/net/9p/protocol.c @@ -80,16 +80,16 @@ pdu_write_u(struct p9_fcall *pdu, const char __user *udata, size_t size) } /* - b - int8_t - w - int16_t - d - int32_t - q - int64_t + b - uu8 + w - u16 + d - u32 + q - u64 s - string S - stat Q - qid - D - data blob (int32_t size followed by void *, results are not freed) - T - array of strings (int16_t count, followed by strings) - R - array of qids (int16_t count, followed by qids) + D - data blob (u32 size followed by void *, results are not freed) + T - array of strings (u16 count, followed by strings) + R - array of qids (u16 count, followed by qids) A - stat for 9p2000.L (p9_stat_dotl) ? - if optional = 1, continue parsing */ @@ -104,7 +104,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, for (ptr = fmt; *ptr; ptr++) { switch (*ptr) { case 'b':{ - int8_t *val = va_arg(ap, int8_t *); + u8 *val = va_arg(ap, u8 *); if (pdu_read(pdu, val, sizeof(*val))) { errcode = -EFAULT; break; @@ -112,7 +112,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, } break; case 'w':{ - int16_t *val = va_arg(ap, int16_t *); + u16 *val = va_arg(ap, u16 *); __le16 le_val; if (pdu_read(pdu, &le_val, sizeof(le_val))) { errcode = -EFAULT; @@ -122,7 +122,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, } break; case 'd':{ - int32_t *val = va_arg(ap, int32_t *); + u32 *val = va_arg(ap, u32 *); __le32 le_val; if (pdu_read(pdu, &le_val, sizeof(le_val))) { errcode = -EFAULT; @@ -132,7 +132,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, } break; case 'q':{ - int64_t *val = va_arg(ap, int64_t *); + u64 *val = va_arg(ap, u64 *); __le64 le_val; if (pdu_read(pdu, &le_val, sizeof(le_val))) { errcode = -EFAULT; @@ -143,7 +143,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, break; case 's':{ char **sptr = va_arg(ap, char **); - uint16_t len; + u16 len; errcode = p9pdu_readf(pdu, proto_version, "w", &len); @@ -196,21 +196,21 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, } break; case 'D':{ - uint32_t *count = va_arg(ap, uint32_t *); + u32 *count = va_arg(ap, u32 *); void **data = va_arg(ap, void **); errcode = p9pdu_readf(pdu, proto_version, "d", count); if (!errcode) { *count = - min_t(uint32_t, *count, + min_t(u32, *count, pdu->size - pdu->offset); *data = &pdu->sdata[pdu->offset]; } } break; case 'T':{ - uint16_t *nwname = va_arg(ap, uint16_t *); + u16 *nwname = va_arg(ap, u16 *); char ***wnames = va_arg(ap, char ***); errcode = p9pdu_readf(pdu, proto_version, @@ -250,7 +250,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, } break; case 'R':{ - int16_t *nwqid = va_arg(ap, int16_t *); + u16 *nwqid = va_arg(ap, u16 *); struct p9_qid **wqids = va_arg(ap, struct p9_qid **); @@ -341,7 +341,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt, for (ptr = fmt; *ptr; ptr++) { switch (*ptr) { case 'b':{ - int8_t val = va_arg(ap, int); + u8 val = va_arg(ap, int); if (pdu_write(pdu, &val, sizeof(val))) errcode = -EFAULT; } @@ -353,22 +353,22 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt, } break; case 'd':{ - __le32 val = cpu_to_le32(va_arg(ap, int32_t)); + __le32 val = cpu_to_le32(va_arg(ap, u32)); if (pdu_write(pdu, &val, sizeof(val))) errcode = -EFAULT; } break; case 'q':{ - __le64 val = cpu_to_le64(va_arg(ap, int64_t)); + __le64 val = cpu_to_le64(va_arg(ap, u64)); if (pdu_write(pdu, &val, sizeof(val))) errcode = -EFAULT; } break; case 's':{ const char *sptr = va_arg(ap, const char *); - uint16_t len = 0; + u16 len = 0; if (sptr) - len = min_t(uint16_t, strlen(sptr), + len = min_t(u16, strlen(sptr), USHRT_MAX); errcode = p9pdu_writef(pdu, proto_version, @@ -401,7 +401,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt, stbuf->n_gid, stbuf->n_muid); } break; case 'D':{ - uint32_t count = va_arg(ap, uint32_t); + u32 count = va_arg(ap, u32); const void *data = va_arg(ap, const void *); errcode = p9pdu_writef(pdu, proto_version, "d", @@ -411,7 +411,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt, } break; case 'U':{ - int32_t count = va_arg(ap, int32_t); + u32 count = va_arg(ap, u32); const char __user *udata = va_arg(ap, const void __user *); errcode = p9pdu_writef(pdu, proto_version, "d", @@ -421,7 +421,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt, } break; case 'T':{ - uint16_t nwname = va_arg(ap, int); + u16 nwname = va_arg(ap, int); const char **wnames = va_arg(ap, const char **); errcode = p9pdu_writef(pdu, proto_version, "w", @@ -442,7 +442,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt, } break; case 'R':{ - int16_t nwqid = va_arg(ap, int); + u16 nwqid = va_arg(ap, int); struct p9_qid *wqids = va_arg(ap, struct p9_qid *); @@ -542,7 +542,7 @@ int p9stat_read(struct p9_client *clnt, char *buf, int len, struct p9_wstat *st) } EXPORT_SYMBOL(p9stat_read); -int p9pdu_prepare(struct p9_fcall *pdu, int16_t tag, int8_t type) +int p9pdu_prepare(struct p9_fcall *pdu, u16 tag, u8 type) { pdu->id = type; return p9pdu_writef(pdu, 0, "dbw", 0, type, tag); diff --git a/net/9p/protocol.h b/net/9p/protocol.h index 2cc525fa..d4aa0a4 100644 --- a/net/9p/protocol.h +++ b/net/9p/protocol.h @@ -28,7 +28,7 @@ int p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt, va_list ap); int p9pdu_readf(struct p9_fcall *pdu, int proto_version, const char *fmt, ...); -int p9pdu_prepare(struct p9_fcall *pdu, int16_t tag, int8_t type); +int p9pdu_prepare(struct p9_fcall *pdu, u16 tag, u8 type); int p9pdu_finalize(struct p9_client *clnt, struct p9_fcall *pdu); void p9pdu_reset(struct p9_fcall *pdu); size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size); diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c index 2c69ddd..3f41a63 100644 --- a/net/9p/trans_rdma.c +++ b/net/9p/trans_rdma.c @@ -279,7 +279,7 @@ handle_recv(struct p9_client *client, struct p9_trans_rdma *rdma, { struct p9_req_t *req; int err = 0; - int16_t tag; + u16 tag; req = NULL;