* [PATCH v5 0/5] transport/session code move to ops struct
@ 2012-06-09 6:25 Pavel Shilovsky
[not found] ` <1339223164-9721-1-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Pavel Shilovsky @ 2012-06-09 6:25 UTC (permalink / raw)
To: linux-cifs-u79uwXL29TY76Z2rM5mHXA
This is the rest of the patchset that makes the cifs transport code
work through ops struct of the server.
This version has improved credit patch (#2).
Pavel Shilovsky (5):
CIFS: Move trans2 processing to ops struct
CIFS: Extend credit mechanism to process request type
CIFS: Move protocol specific negotiate code to ops struct
CIFS: Move protocol specific session setup/logoff code to ops struct
CIFS: Move protocol specific tcon/tdis code to ops struct
fs/cifs/cifsglob.h | 38 ++++++-
fs/cifs/cifsproto.h | 20 ++--
fs/cifs/cifssmb.c | 25 +++---
fs/cifs/connect.c | 269 ++++++++++++---------------------------------------
fs/cifs/sess.c | 4 +-
fs/cifs/smb1ops.c | 209 +++++++++++++++++++++++++++++++++++++++-
fs/cifs/transport.c | 54 ++++++-----
7 files changed, 356 insertions(+), 263 deletions(-)
^ permalink raw reply [flat|nested] 17+ messages in thread[parent not found: <1339223164-9721-1-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]
* [PATCH v5 1/5] CIFS: Move trans2 processing to ops struct [not found] ` <1339223164-9721-1-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> @ 2012-06-09 6:26 ` Pavel Shilovsky 2012-06-09 6:26 ` [PATCH v5 2/5] CIFS: Extend credit mechanism to process request type Pavel Shilovsky ` (3 subsequent siblings) 4 siblings, 0 replies; 17+ messages in thread From: Pavel Shilovsky @ 2012-06-09 6:26 UTC (permalink / raw) To: linux-cifs-u79uwXL29TY76Z2rM5mHXA Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> --- fs/cifs/cifsglob.h | 3 + fs/cifs/connect.c | 161 +------------------------------------------------ fs/cifs/smb1ops.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 175 insertions(+), 159 deletions(-) diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 6df0cbe..2aac4e5 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -187,6 +187,9 @@ struct smb_version_operations { /* verify the message */ int (*check_message)(char *, unsigned int); bool (*is_oplock_break)(char *, struct TCP_Server_Info *); + /* process transaction2 response */ + bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *, + char *, int); }; struct smb_version_values { diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 78db68a..ca0288d 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -394,143 +394,6 @@ cifs_reconnect(struct TCP_Server_Info *server) return rc; } -/* - return codes: - 0 not a transact2, or all data present - >0 transact2 with that much data missing - -EINVAL = invalid transact2 - - */ -static int check2ndT2(char *buf) -{ - struct smb_hdr *pSMB = (struct smb_hdr *)buf; - struct smb_t2_rsp *pSMBt; - int remaining; - __u16 total_data_size, data_in_this_rsp; - - if (pSMB->Command != SMB_COM_TRANSACTION2) - return 0; - - /* check for plausible wct, bcc and t2 data and parm sizes */ - /* check for parm and data offset going beyond end of smb */ - if (pSMB->WordCount != 10) { /* coalesce_t2 depends on this */ - cFYI(1, "invalid transact2 word count"); - return -EINVAL; - } - - pSMBt = (struct smb_t2_rsp *)pSMB; - - total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount); - data_in_this_rsp = get_unaligned_le16(&pSMBt->t2_rsp.DataCount); - - if (total_data_size == data_in_this_rsp) - return 0; - else if (total_data_size < data_in_this_rsp) { - cFYI(1, "total data %d smaller than data in frame %d", - total_data_size, data_in_this_rsp); - return -EINVAL; - } - - remaining = total_data_size - data_in_this_rsp; - - cFYI(1, "missing %d bytes from transact2, check next response", - remaining); - if (total_data_size > CIFSMaxBufSize) { - cERROR(1, "TotalDataSize %d is over maximum buffer %d", - total_data_size, CIFSMaxBufSize); - return -EINVAL; - } - return remaining; -} - -static int coalesce_t2(char *second_buf, struct smb_hdr *target_hdr) -{ - struct smb_t2_rsp *pSMBs = (struct smb_t2_rsp *)second_buf; - struct smb_t2_rsp *pSMBt = (struct smb_t2_rsp *)target_hdr; - char *data_area_of_tgt; - char *data_area_of_src; - int remaining; - unsigned int byte_count, total_in_tgt; - __u16 tgt_total_cnt, src_total_cnt, total_in_src; - - src_total_cnt = get_unaligned_le16(&pSMBs->t2_rsp.TotalDataCount); - tgt_total_cnt = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount); - - if (tgt_total_cnt != src_total_cnt) - cFYI(1, "total data count of primary and secondary t2 differ " - "source=%hu target=%hu", src_total_cnt, tgt_total_cnt); - - total_in_tgt = get_unaligned_le16(&pSMBt->t2_rsp.DataCount); - - remaining = tgt_total_cnt - total_in_tgt; - - if (remaining < 0) { - cFYI(1, "Server sent too much data. tgt_total_cnt=%hu " - "total_in_tgt=%hu", tgt_total_cnt, total_in_tgt); - return -EPROTO; - } - - if (remaining == 0) { - /* nothing to do, ignore */ - cFYI(1, "no more data remains"); - return 0; - } - - total_in_src = get_unaligned_le16(&pSMBs->t2_rsp.DataCount); - if (remaining < total_in_src) - cFYI(1, "transact2 2nd response contains too much data"); - - /* find end of first SMB data area */ - data_area_of_tgt = (char *)&pSMBt->hdr.Protocol + - get_unaligned_le16(&pSMBt->t2_rsp.DataOffset); - - /* validate target area */ - data_area_of_src = (char *)&pSMBs->hdr.Protocol + - get_unaligned_le16(&pSMBs->t2_rsp.DataOffset); - - data_area_of_tgt += total_in_tgt; - - total_in_tgt += total_in_src; - /* is the result too big for the field? */ - if (total_in_tgt > USHRT_MAX) { - cFYI(1, "coalesced DataCount too large (%u)", total_in_tgt); - return -EPROTO; - } - put_unaligned_le16(total_in_tgt, &pSMBt->t2_rsp.DataCount); - - /* fix up the BCC */ - byte_count = get_bcc(target_hdr); - byte_count += total_in_src; - /* is the result too big for the field? */ - if (byte_count > USHRT_MAX) { - cFYI(1, "coalesced BCC too large (%u)", byte_count); - return -EPROTO; - } - put_bcc(byte_count, target_hdr); - - byte_count = be32_to_cpu(target_hdr->smb_buf_length); - byte_count += total_in_src; - /* don't allow buffer to overflow */ - if (byte_count > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) { - cFYI(1, "coalesced BCC exceeds buffer size (%u)", byte_count); - return -ENOBUFS; - } - target_hdr->smb_buf_length = cpu_to_be32(byte_count); - - /* copy second buffer into end of first buffer */ - memcpy(data_area_of_tgt, data_area_of_src, total_in_src); - - if (remaining != total_in_src) { - /* more responses to go */ - cFYI(1, "waiting for more secondary responses"); - return 1; - } - - /* we are done */ - cFYI(1, "found the last secondary response"); - return 0; -} - static void cifs_echo_request(struct work_struct *work) { @@ -803,29 +666,9 @@ static void handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server, char *buf, int malformed) { - if (malformed == 0 && check2ndT2(buf) > 0) { - mid->multiRsp = true; - if (mid->resp_buf) { - /* merge response - fix up 1st*/ - malformed = coalesce_t2(buf, mid->resp_buf); - if (malformed > 0) - return; - - /* All parts received or packet is malformed. */ - mid->multiEnd = true; - return dequeue_mid(mid, malformed); - } - if (!server->large_buf) { - /*FIXME: switch to already allocated largebuf?*/ - cERROR(1, "1st trans2 resp needs bigbuf"); - } else { - /* Have first buffer */ - mid->resp_buf = buf; - mid->large_buf = true; - server->bigbuf = NULL; - } + if (server->ops->check_trans2 && + server->ops->check_trans2(mid, server, buf, malformed)) return; - } mid->resp_buf = buf; mid->large_buf = server->large_buf; /* Was previous buf put in mpx struct for multi-rsp? */ diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c index 6dec38f..28359e7 100644 --- a/fs/cifs/smb1ops.c +++ b/fs/cifs/smb1ops.c @@ -213,6 +213,175 @@ cifs_get_next_mid(struct TCP_Server_Info *server) return mid; } +/* + return codes: + 0 not a transact2, or all data present + >0 transact2 with that much data missing + -EINVAL invalid transact2 + */ +static int +check2ndT2(char *buf) +{ + struct smb_hdr *pSMB = (struct smb_hdr *)buf; + struct smb_t2_rsp *pSMBt; + int remaining; + __u16 total_data_size, data_in_this_rsp; + + if (pSMB->Command != SMB_COM_TRANSACTION2) + return 0; + + /* check for plausible wct, bcc and t2 data and parm sizes */ + /* check for parm and data offset going beyond end of smb */ + if (pSMB->WordCount != 10) { /* coalesce_t2 depends on this */ + cFYI(1, "invalid transact2 word count"); + return -EINVAL; + } + + pSMBt = (struct smb_t2_rsp *)pSMB; + + total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount); + data_in_this_rsp = get_unaligned_le16(&pSMBt->t2_rsp.DataCount); + + if (total_data_size == data_in_this_rsp) + return 0; + else if (total_data_size < data_in_this_rsp) { + cFYI(1, "total data %d smaller than data in frame %d", + total_data_size, data_in_this_rsp); + return -EINVAL; + } + + remaining = total_data_size - data_in_this_rsp; + + cFYI(1, "missing %d bytes from transact2, check next response", + remaining); + if (total_data_size > CIFSMaxBufSize) { + cERROR(1, "TotalDataSize %d is over maximum buffer %d", + total_data_size, CIFSMaxBufSize); + return -EINVAL; + } + return remaining; +} + +static int +coalesce_t2(char *second_buf, struct smb_hdr *target_hdr) +{ + struct smb_t2_rsp *pSMBs = (struct smb_t2_rsp *)second_buf; + struct smb_t2_rsp *pSMBt = (struct smb_t2_rsp *)target_hdr; + char *data_area_of_tgt; + char *data_area_of_src; + int remaining; + unsigned int byte_count, total_in_tgt; + __u16 tgt_total_cnt, src_total_cnt, total_in_src; + + src_total_cnt = get_unaligned_le16(&pSMBs->t2_rsp.TotalDataCount); + tgt_total_cnt = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount); + + if (tgt_total_cnt != src_total_cnt) + cFYI(1, "total data count of primary and secondary t2 differ " + "source=%hu target=%hu", src_total_cnt, tgt_total_cnt); + + total_in_tgt = get_unaligned_le16(&pSMBt->t2_rsp.DataCount); + + remaining = tgt_total_cnt - total_in_tgt; + + if (remaining < 0) { + cFYI(1, "Server sent too much data. tgt_total_cnt=%hu " + "total_in_tgt=%hu", tgt_total_cnt, total_in_tgt); + return -EPROTO; + } + + if (remaining == 0) { + /* nothing to do, ignore */ + cFYI(1, "no more data remains"); + return 0; + } + + total_in_src = get_unaligned_le16(&pSMBs->t2_rsp.DataCount); + if (remaining < total_in_src) + cFYI(1, "transact2 2nd response contains too much data"); + + /* find end of first SMB data area */ + data_area_of_tgt = (char *)&pSMBt->hdr.Protocol + + get_unaligned_le16(&pSMBt->t2_rsp.DataOffset); + + /* validate target area */ + data_area_of_src = (char *)&pSMBs->hdr.Protocol + + get_unaligned_le16(&pSMBs->t2_rsp.DataOffset); + + data_area_of_tgt += total_in_tgt; + + total_in_tgt += total_in_src; + /* is the result too big for the field? */ + if (total_in_tgt > USHRT_MAX) { + cFYI(1, "coalesced DataCount too large (%u)", total_in_tgt); + return -EPROTO; + } + put_unaligned_le16(total_in_tgt, &pSMBt->t2_rsp.DataCount); + + /* fix up the BCC */ + byte_count = get_bcc(target_hdr); + byte_count += total_in_src; + /* is the result too big for the field? */ + if (byte_count > USHRT_MAX) { + cFYI(1, "coalesced BCC too large (%u)", byte_count); + return -EPROTO; + } + put_bcc(byte_count, target_hdr); + + byte_count = be32_to_cpu(target_hdr->smb_buf_length); + byte_count += total_in_src; + /* don't allow buffer to overflow */ + if (byte_count > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) { + cFYI(1, "coalesced BCC exceeds buffer size (%u)", byte_count); + return -ENOBUFS; + } + target_hdr->smb_buf_length = cpu_to_be32(byte_count); + + /* copy second buffer into end of first buffer */ + memcpy(data_area_of_tgt, data_area_of_src, total_in_src); + + if (remaining != total_in_src) { + /* more responses to go */ + cFYI(1, "waiting for more secondary responses"); + return 1; + } + + /* we are done */ + cFYI(1, "found the last secondary response"); + return 0; +} + +static bool +cifs_check_trans2(struct mid_q_entry *mid, struct TCP_Server_Info *server, + char *buf, int malformed) +{ + if (malformed) + return false; + if (check2ndT2(buf) <= 0) + return false; + mid->multiRsp = true; + if (mid->resp_buf) { + /* merge response - fix up 1st*/ + malformed = coalesce_t2(buf, mid->resp_buf); + if (malformed > 0) + return true; + /* All parts received or packet is malformed. */ + mid->multiEnd = true; + dequeue_mid(mid, malformed); + return true; + } + if (!server->large_buf) { + /*FIXME: switch to already allocated largebuf?*/ + cERROR(1, "1st trans2 resp needs bigbuf"); + } else { + /* Have first buffer */ + mid->resp_buf = buf; + mid->large_buf = true; + server->bigbuf = NULL; + } + return true; +} + struct smb_version_operations smb1_operations = { .send_cancel = send_nt_cancel, .compare_fids = cifs_compare_fids, @@ -229,6 +398,7 @@ struct smb_version_operations smb1_operations = { .check_message = checkSMB, .dump_detail = cifs_dump_detail, .is_oplock_break = is_valid_oplock_break, + .check_trans2 = cifs_check_trans2, }; struct smb_version_values smb1_values = { -- 1.7.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v5 2/5] CIFS: Extend credit mechanism to process request type [not found] ` <1339223164-9721-1-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> 2012-06-09 6:26 ` [PATCH v5 1/5] CIFS: Move trans2 processing " Pavel Shilovsky @ 2012-06-09 6:26 ` Pavel Shilovsky [not found] ` <1339223164-9721-3-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> 2012-06-09 6:26 ` [PATCH v5 3/5] CIFS: Move protocol specific negotiate code to ops struct Pavel Shilovsky ` (2 subsequent siblings) 4 siblings, 1 reply; 17+ messages in thread From: Pavel Shilovsky @ 2012-06-09 6:26 UTC (permalink / raw) To: linux-cifs-u79uwXL29TY76Z2rM5mHXA Split all requests to echos, oplocks and others - each group uses its own credit slot. This is indicated by new flags CIFS_ECHO_OP and CIFS_OBREAK_OP that are not used now for CIFS. This change is required to support SMB2 protocol because of different processing of these commands. Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> --- fs/cifs/cifsglob.h | 16 +++++++++++--- fs/cifs/cifsproto.h | 2 +- fs/cifs/cifssmb.c | 21 ++++++++++--------- fs/cifs/smb1ops.c | 12 +++++++++- fs/cifs/transport.c | 54 +++++++++++++++++++++++++++++--------------------- 5 files changed, 65 insertions(+), 40 deletions(-) diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 2aac4e5..844b77c 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -171,9 +171,11 @@ struct smb_version_operations { /* check response: verify signature, map error */ int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *, bool); - void (*add_credits)(struct TCP_Server_Info *, const unsigned int); + void (*add_credits)(struct TCP_Server_Info *, const unsigned int, + const int); void (*set_credits)(struct TCP_Server_Info *, const int); - int * (*get_credits_field)(struct TCP_Server_Info *); + int * (*get_credits_field)(struct TCP_Server_Info *, const int); + unsigned int (*get_credits)(struct mid_q_entry *); __u64 (*get_next_mid)(struct TCP_Server_Info *); /* data offset from read response message */ unsigned int (*read_data_offset)(char *); @@ -392,9 +394,10 @@ has_credits(struct TCP_Server_Info *server, int *credits) } static inline void -add_credits(struct TCP_Server_Info *server, const unsigned int add) +add_credits(struct TCP_Server_Info *server, const unsigned int add, + const int optype) { - server->ops->add_credits(server, add); + server->ops->add_credits(server, add, optype); } static inline void @@ -957,6 +960,11 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, #define CIFS_LARGE_BUF_OP 0x020 /* large request buffer */ #define CIFS_NO_RESP 0x040 /* no response buffer required */ +/* Type of request operation */ +#define CIFS_ECHO_OP 0x080 /* echo request */ +#define CIFS_OBREAK_OP 0x0100 /* oplock break request */ +#define CIFS_OP_MASK 0x0180 /* mask request type */ + /* Security Flags: indicate type of session setup needed */ #define CIFSSEC_MAY_SIGN 0x00001 #define CIFSSEC_MAY_NTLM 0x00002 diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index 0a6cbfe..8c1c7c1 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -71,7 +71,7 @@ extern void DeleteMidQEntry(struct mid_q_entry *midEntry); extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, unsigned int nvec, mid_receive_t *receive, mid_callback_t *callback, void *cbdata, - bool ignore_pend); + const int flags); extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *, struct smb_hdr * /* input */ , struct smb_hdr * /* out */ , diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index 5b40073..58f1ab5 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -720,7 +720,7 @@ cifs_echo_callback(struct mid_q_entry *mid) struct TCP_Server_Info *server = mid->callback_data; DeleteMidQEntry(mid); - add_credits(server, 1); + add_credits(server, 1, CIFS_ECHO_OP); } int @@ -747,7 +747,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server) iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4; rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback, - server, true); + server, CIFS_ASYNC_OP | CIFS_ECHO_OP); if (rc) cFYI(1, "Echo request failed: %d", rc); @@ -1563,7 +1563,7 @@ cifs_readv_callback(struct mid_q_entry *mid) queue_work(cifsiod_wq, &rdata->work); DeleteMidQEntry(mid); - add_credits(server, 1); + add_credits(server, 1, 0); } /* cifs_async_readv - send an async write, and set up mid to handle result */ @@ -1619,7 +1619,7 @@ cifs_async_readv(struct cifs_readdata *rdata) kref_get(&rdata->refcount); rc = cifs_call_async(tcon->ses->server, rdata->iov, 1, cifs_readv_receive, cifs_readv_callback, - rdata, false); + rdata, 0); if (rc == 0) cifs_stats_inc(&tcon->num_reads); @@ -2010,7 +2010,7 @@ cifs_writev_callback(struct mid_q_entry *mid) queue_work(cifsiod_wq, &wdata->work); DeleteMidQEntry(mid); - add_credits(tcon->ses->server, 1); + add_credits(tcon->ses->server, 1, 0); } /* cifs_async_writev - send an async write, and set up mid to handle result */ @@ -2090,7 +2090,7 @@ cifs_async_writev(struct cifs_writedata *wdata) kref_get(&wdata->refcount); rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1, - NULL, cifs_writev_callback, wdata, false); + NULL, cifs_writev_callback, wdata, 0); if (rc == 0) cifs_stats_inc(&tcon->num_writes); @@ -2268,7 +2268,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, LOCK_REQ *pSMB = NULL; /* LOCK_RSP *pSMBr = NULL; */ /* No response data other than rc to parse */ int bytes_returned; - int timeout = 0; + int flags = 0; __u16 count; cFYI(1, "CIFSSMBLock timeout %d numLock %d", (int)waitFlag, numLock); @@ -2278,10 +2278,11 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, return rc; if (lockType == LOCKING_ANDX_OPLOCK_RELEASE) { - timeout = CIFS_ASYNC_OP; /* no response expected */ + /* no response expected */ + flags = CIFS_ASYNC_OP | CIFS_OBREAK_OP; pSMB->Timeout = 0; } else if (waitFlag) { - timeout = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ + flags = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ pSMB->Timeout = cpu_to_le32(-1);/* blocking - do not time out */ } else { pSMB->Timeout = 0; @@ -2314,7 +2315,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, (struct smb_hdr *) pSMB, &bytes_returned); cifs_small_buf_release(pSMB); } else { - rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, timeout); + rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, flags); /* SMB buffer freed by function above */ } cifs_stats_inc(&tcon->num_locks); diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c index 28359e7..f4f8394 100644 --- a/fs/cifs/smb1ops.c +++ b/fs/cifs/smb1ops.c @@ -101,7 +101,8 @@ cifs_find_mid(struct TCP_Server_Info *server, char *buffer) } static void -cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add) +cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add, + const int optype) { spin_lock(&server->req_lock); server->credits += add; @@ -120,11 +121,17 @@ cifs_set_credits(struct TCP_Server_Info *server, const int val) } static int * -cifs_get_credits_field(struct TCP_Server_Info *server) +cifs_get_credits_field(struct TCP_Server_Info *server, const int optype) { return &server->credits; } +static unsigned int +cifs_get_credits(struct mid_q_entry *mid) +{ + return 1; +} + /* * Find a free multiplex id (SMB mid). Otherwise there could be * mid collisions which might cause problems, demultiplexing the @@ -390,6 +397,7 @@ struct smb_version_operations smb1_operations = { .add_credits = cifs_add_credits, .set_credits = cifs_set_credits, .get_credits_field = cifs_get_credits_field, + .get_credits = cifs_get_credits, .get_next_mid = cifs_get_next_mid, .read_data_offset = cifs_read_data_offset, .read_data_length = cifs_read_data_length, diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c index 3097ee5..54cd8dd 100644 --- a/fs/cifs/transport.c +++ b/fs/cifs/transport.c @@ -254,13 +254,13 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer, } static int -wait_for_free_credits(struct TCP_Server_Info *server, const int optype, +wait_for_free_credits(struct TCP_Server_Info *server, const int long_op, int *credits) { int rc; spin_lock(&server->req_lock); - if (optype == CIFS_ASYNC_OP) { + if (long_op == CIFS_ASYNC_OP) { /* oplock breaks must not be held up */ server->in_flight++; *credits -= 1; @@ -290,7 +290,7 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int optype, */ /* update # of requests on the wire to server */ - if (optype != CIFS_BLOCKING_OP) { + if (long_op != CIFS_BLOCKING_OP) { *credits -= 1; server->in_flight++; } @@ -302,10 +302,11 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int optype, } static int -wait_for_free_request(struct TCP_Server_Info *server, const int optype) +wait_for_free_request(struct TCP_Server_Info *server, const int long_op, + const int optype) { - return wait_for_free_credits(server, optype, - server->ops->get_credits_field(server)); + return wait_for_free_credits(server, long_op, + server->ops->get_credits_field(server, optype)); } static int allocate_mid(struct cifs_ses *ses, struct smb_hdr *in_buf, @@ -384,12 +385,15 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov, int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, unsigned int nvec, mid_receive_t *receive, - mid_callback_t *callback, void *cbdata, bool ignore_pend) + mid_callback_t *callback, void *cbdata, const int flags) { - int rc; + int rc, long_op, optype; struct mid_q_entry *mid; - rc = wait_for_free_request(server, ignore_pend ? CIFS_ASYNC_OP : 0); + long_op = flags & CIFS_TIMEOUT_MASK; + optype = flags & CIFS_OP_MASK; + + rc = wait_for_free_request(server, long_op, optype); if (rc) return rc; @@ -397,7 +401,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, rc = cifs_setup_async_request(server, iov, nvec, &mid); if (rc) { mutex_unlock(&server->srv_mutex); - add_credits(server, 1); + add_credits(server, 1, optype); wake_up(&server->request_q); return rc; } @@ -419,7 +423,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, return rc; out_err: delete_mid(mid); - add_credits(server, 1); + add_credits(server, 1, optype); wake_up(&server->request_q); return rc; } @@ -539,11 +543,13 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, const int flags) { int rc = 0; - int long_op; + int long_op, optype; struct mid_q_entry *midQ; char *buf = iov[0].iov_base; + unsigned int credits = 1; long_op = flags & CIFS_TIMEOUT_MASK; + optype = flags & CIFS_OP_MASK; *pRespBufType = CIFS_NO_BUFFER; /* no response buf yet */ @@ -564,7 +570,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, * use ses->maxReq. */ - rc = wait_for_free_request(ses->server, long_op); + rc = wait_for_free_request(ses->server, optype, long_op); if (rc) { cifs_small_buf_release(buf); return rc; @@ -583,7 +589,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, mutex_unlock(&ses->server->srv_mutex); cifs_small_buf_release(buf); /* Update # of requests on wire to server */ - add_credits(ses->server, 1); + add_credits(ses->server, 1, optype); return rc; } @@ -613,7 +619,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, midQ->callback = DeleteMidQEntry; spin_unlock(&GlobalMid_Lock); cifs_small_buf_release(buf); - add_credits(ses->server, 1); + add_credits(ses->server, 1, optype); return rc; } spin_unlock(&GlobalMid_Lock); @@ -623,7 +629,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, rc = cifs_sync_mid_result(midQ, ses->server); if (rc != 0) { - add_credits(ses->server, 1); + add_credits(ses->server, 1, optype); return rc; } @@ -641,6 +647,8 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, else *pRespBufType = CIFS_SMALL_BUFFER; + credits = ses->server->ops->get_credits(midQ); + rc = ses->server->ops->check_receive(midQ, ses->server, flags & CIFS_LOG_ERROR); @@ -649,7 +657,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, midQ->resp_buf = NULL; out: delete_mid(midQ); - add_credits(ses->server, 1); + add_credits(ses->server, credits, optype); return rc; } @@ -685,7 +693,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, return -EIO; } - rc = wait_for_free_request(ses->server, long_op); + rc = wait_for_free_request(ses->server, 0, long_op); if (rc) return rc; @@ -699,7 +707,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, if (rc) { mutex_unlock(&ses->server->srv_mutex); /* Update # of requests on wire to server */ - add_credits(ses->server, 1); + add_credits(ses->server, 1, 0); return rc; } @@ -731,7 +739,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, /* no longer considered to be "in-flight" */ midQ->callback = DeleteMidQEntry; spin_unlock(&GlobalMid_Lock); - add_credits(ses->server, 1); + add_credits(ses->server, 1, 0); return rc; } spin_unlock(&GlobalMid_Lock); @@ -739,7 +747,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, rc = cifs_sync_mid_result(midQ, ses->server); if (rc != 0) { - add_credits(ses->server, 1); + add_credits(ses->server, 1, 0); return rc; } @@ -755,7 +763,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, rc = cifs_check_receive(midQ, ses->server, 0); out: delete_mid(midQ); - add_credits(ses->server, 1); + add_credits(ses->server, 1, 0); return rc; } @@ -820,7 +828,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon, return -EIO; } - rc = wait_for_free_request(ses->server, CIFS_BLOCKING_OP); + rc = wait_for_free_request(ses->server, 0, CIFS_BLOCKING_OP); if (rc) return rc; -- 1.7.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
[parent not found: <1339223164-9721-3-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]
* Re: [PATCH v5 2/5] CIFS: Extend credit mechanism to process request type [not found] ` <1339223164-9721-3-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> @ 2012-06-19 15:31 ` Jeff Layton [not found] ` <20120619083156.761f8a31-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org> 0 siblings, 1 reply; 17+ messages in thread From: Jeff Layton @ 2012-06-19 15:31 UTC (permalink / raw) To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA On Sat, 9 Jun 2012 10:26:01 +0400 Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: > Split all requests to echos, oplocks and others - each group uses > its own credit slot. This is indicated by new flags > > CIFS_ECHO_OP and CIFS_OBREAK_OP > > that are not used now for CIFS. This change is required to support > SMB2 protocol because of different processing of these commands. > > Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> > --- > fs/cifs/cifsglob.h | 16 +++++++++++--- > fs/cifs/cifsproto.h | 2 +- > fs/cifs/cifssmb.c | 21 ++++++++++--------- > fs/cifs/smb1ops.c | 12 +++++++++- > fs/cifs/transport.c | 54 +++++++++++++++++++++++++++++--------------------- > 5 files changed, 65 insertions(+), 40 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 2aac4e5..844b77c 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -171,9 +171,11 @@ struct smb_version_operations { > /* check response: verify signature, map error */ > int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *, > bool); > - void (*add_credits)(struct TCP_Server_Info *, const unsigned int); > + void (*add_credits)(struct TCP_Server_Info *, const unsigned int, > + const int); > void (*set_credits)(struct TCP_Server_Info *, const int); > - int * (*get_credits_field)(struct TCP_Server_Info *); > + int * (*get_credits_field)(struct TCP_Server_Info *, const int); > + unsigned int (*get_credits)(struct mid_q_entry *); > __u64 (*get_next_mid)(struct TCP_Server_Info *); > /* data offset from read response message */ > unsigned int (*read_data_offset)(char *); > @@ -392,9 +394,10 @@ has_credits(struct TCP_Server_Info *server, int *credits) > } > > static inline void > -add_credits(struct TCP_Server_Info *server, const unsigned int add) > +add_credits(struct TCP_Server_Info *server, const unsigned int add, > + const int optype) > { > - server->ops->add_credits(server, add); > + server->ops->add_credits(server, add, optype); > } > > static inline void > @@ -957,6 +960,11 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, > #define CIFS_LARGE_BUF_OP 0x020 /* large request buffer */ > #define CIFS_NO_RESP 0x040 /* no response buffer required */ > > +/* Type of request operation */ > +#define CIFS_ECHO_OP 0x080 /* echo request */ > +#define CIFS_OBREAK_OP 0x0100 /* oplock break request */ > +#define CIFS_OP_MASK 0x0180 /* mask request type */ > + > /* Security Flags: indicate type of session setup needed */ > #define CIFSSEC_MAY_SIGN 0x00001 > #define CIFSSEC_MAY_NTLM 0x00002 > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > index 0a6cbfe..8c1c7c1 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -71,7 +71,7 @@ extern void DeleteMidQEntry(struct mid_q_entry *midEntry); > extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, > unsigned int nvec, mid_receive_t *receive, > mid_callback_t *callback, void *cbdata, > - bool ignore_pend); > + const int flags); > extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *, > struct smb_hdr * /* input */ , > struct smb_hdr * /* out */ , > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > index 5b40073..58f1ab5 100644 > --- a/fs/cifs/cifssmb.c > +++ b/fs/cifs/cifssmb.c > @@ -720,7 +720,7 @@ cifs_echo_callback(struct mid_q_entry *mid) > struct TCP_Server_Info *server = mid->callback_data; > > DeleteMidQEntry(mid); > - add_credits(server, 1); > + add_credits(server, 1, CIFS_ECHO_OP); > } > > int > @@ -747,7 +747,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server) > iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4; > > rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback, > - server, true); > + server, CIFS_ASYNC_OP | CIFS_ECHO_OP); > if (rc) > cFYI(1, "Echo request failed: %d", rc); > > @@ -1563,7 +1563,7 @@ cifs_readv_callback(struct mid_q_entry *mid) > > queue_work(cifsiod_wq, &rdata->work); > DeleteMidQEntry(mid); > - add_credits(server, 1); > + add_credits(server, 1, 0); > } > > /* cifs_async_readv - send an async write, and set up mid to handle result */ > @@ -1619,7 +1619,7 @@ cifs_async_readv(struct cifs_readdata *rdata) > kref_get(&rdata->refcount); > rc = cifs_call_async(tcon->ses->server, rdata->iov, 1, > cifs_readv_receive, cifs_readv_callback, > - rdata, false); > + rdata, 0); > > if (rc == 0) > cifs_stats_inc(&tcon->num_reads); > @@ -2010,7 +2010,7 @@ cifs_writev_callback(struct mid_q_entry *mid) > > queue_work(cifsiod_wq, &wdata->work); > DeleteMidQEntry(mid); > - add_credits(tcon->ses->server, 1); > + add_credits(tcon->ses->server, 1, 0); > } > > /* cifs_async_writev - send an async write, and set up mid to handle result */ > @@ -2090,7 +2090,7 @@ cifs_async_writev(struct cifs_writedata *wdata) > > kref_get(&wdata->refcount); > rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1, > - NULL, cifs_writev_callback, wdata, false); > + NULL, cifs_writev_callback, wdata, 0); > > if (rc == 0) > cifs_stats_inc(&tcon->num_writes); > @@ -2268,7 +2268,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, > LOCK_REQ *pSMB = NULL; > /* LOCK_RSP *pSMBr = NULL; */ /* No response data other than rc to parse */ > int bytes_returned; > - int timeout = 0; > + int flags = 0; > __u16 count; > > cFYI(1, "CIFSSMBLock timeout %d numLock %d", (int)waitFlag, numLock); > @@ -2278,10 +2278,11 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, > return rc; > > if (lockType == LOCKING_ANDX_OPLOCK_RELEASE) { > - timeout = CIFS_ASYNC_OP; /* no response expected */ > + /* no response expected */ > + flags = CIFS_ASYNC_OP | CIFS_OBREAK_OP; > pSMB->Timeout = 0; > } else if (waitFlag) { > - timeout = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ > + flags = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ > pSMB->Timeout = cpu_to_le32(-1);/* blocking - do not time out */ > } else { > pSMB->Timeout = 0; > @@ -2314,7 +2315,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, > (struct smb_hdr *) pSMB, &bytes_returned); > cifs_small_buf_release(pSMB); > } else { > - rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, timeout); > + rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, flags); > /* SMB buffer freed by function above */ > } > cifs_stats_inc(&tcon->num_locks); > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > index 28359e7..f4f8394 100644 > --- a/fs/cifs/smb1ops.c > +++ b/fs/cifs/smb1ops.c > @@ -101,7 +101,8 @@ cifs_find_mid(struct TCP_Server_Info *server, char *buffer) > } > > static void > -cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add) > +cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add, > + const int optype) > { > spin_lock(&server->req_lock); > server->credits += add; > @@ -120,11 +121,17 @@ cifs_set_credits(struct TCP_Server_Info *server, const int val) > } > > static int * > -cifs_get_credits_field(struct TCP_Server_Info *server) > +cifs_get_credits_field(struct TCP_Server_Info *server, const int optype) > { > return &server->credits; > } > > +static unsigned int > +cifs_get_credits(struct mid_q_entry *mid) > +{ > + return 1; > +} > + > /* > * Find a free multiplex id (SMB mid). Otherwise there could be > * mid collisions which might cause problems, demultiplexing the > @@ -390,6 +397,7 @@ struct smb_version_operations smb1_operations = { > .add_credits = cifs_add_credits, > .set_credits = cifs_set_credits, > .get_credits_field = cifs_get_credits_field, > + .get_credits = cifs_get_credits, > .get_next_mid = cifs_get_next_mid, > .read_data_offset = cifs_read_data_offset, > .read_data_length = cifs_read_data_length, > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > index 3097ee5..54cd8dd 100644 > --- a/fs/cifs/transport.c > +++ b/fs/cifs/transport.c > @@ -254,13 +254,13 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer, > } > > static int > -wait_for_free_credits(struct TCP_Server_Info *server, const int optype, > +wait_for_free_credits(struct TCP_Server_Info *server, const int long_op, > int *credits) > { > int rc; > > spin_lock(&server->req_lock); > - if (optype == CIFS_ASYNC_OP) { > + if (long_op == CIFS_ASYNC_OP) { > /* oplock breaks must not be held up */ > server->in_flight++; > *credits -= 1; > @@ -290,7 +290,7 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int optype, > */ > > /* update # of requests on the wire to server */ > - if (optype != CIFS_BLOCKING_OP) { > + if (long_op != CIFS_BLOCKING_OP) { > *credits -= 1; > server->in_flight++; > } Why rename the variable in this above function? "optype" seems like a better name... > @@ -302,10 +302,11 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int optype, > } > > static int > -wait_for_free_request(struct TCP_Server_Info *server, const int optype) > +wait_for_free_request(struct TCP_Server_Info *server, const int long_op, > + const int optype) > { > - return wait_for_free_credits(server, optype, > - server->ops->get_credits_field(server)); > + return wait_for_free_credits(server, long_op, > + server->ops->get_credits_field(server, optype)); > } > > static int allocate_mid(struct cifs_ses *ses, struct smb_hdr *in_buf, > @@ -384,12 +385,15 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov, > int > cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, > unsigned int nvec, mid_receive_t *receive, > - mid_callback_t *callback, void *cbdata, bool ignore_pend) > + mid_callback_t *callback, void *cbdata, const int flags) > { > - int rc; > + int rc, long_op, optype; > struct mid_q_entry *mid; > > - rc = wait_for_free_request(server, ignore_pend ? CIFS_ASYNC_OP : 0); > + long_op = flags & CIFS_TIMEOUT_MASK; > + optype = flags & CIFS_OP_MASK; > + > + rc = wait_for_free_request(server, long_op, optype); > if (rc) > return rc; > > @@ -397,7 +401,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, > rc = cifs_setup_async_request(server, iov, nvec, &mid); > if (rc) { > mutex_unlock(&server->srv_mutex); > - add_credits(server, 1); > + add_credits(server, 1, optype); > wake_up(&server->request_q); > return rc; > } > @@ -419,7 +423,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, > return rc; > out_err: > delete_mid(mid); > - add_credits(server, 1); > + add_credits(server, 1, optype); > wake_up(&server->request_q); > return rc; > } > @@ -539,11 +543,13 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, > const int flags) > { > int rc = 0; > - int long_op; > + int long_op, optype; > struct mid_q_entry *midQ; > char *buf = iov[0].iov_base; > + unsigned int credits = 1; > > long_op = flags & CIFS_TIMEOUT_MASK; > + optype = flags & CIFS_OP_MASK; > > *pRespBufType = CIFS_NO_BUFFER; /* no response buf yet */ > > @@ -564,7 +570,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, > * use ses->maxReq. > */ > > - rc = wait_for_free_request(ses->server, long_op); > + rc = wait_for_free_request(ses->server, optype, long_op); > if (rc) { > cifs_small_buf_release(buf); > return rc; > @@ -583,7 +589,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, > mutex_unlock(&ses->server->srv_mutex); > cifs_small_buf_release(buf); > /* Update # of requests on wire to server */ > - add_credits(ses->server, 1); > + add_credits(ses->server, 1, optype); > return rc; > } > > @@ -613,7 +619,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, > midQ->callback = DeleteMidQEntry; > spin_unlock(&GlobalMid_Lock); > cifs_small_buf_release(buf); > - add_credits(ses->server, 1); > + add_credits(ses->server, 1, optype); > return rc; > } > spin_unlock(&GlobalMid_Lock); > @@ -623,7 +629,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, > > rc = cifs_sync_mid_result(midQ, ses->server); > if (rc != 0) { > - add_credits(ses->server, 1); > + add_credits(ses->server, 1, optype); > return rc; > } > > @@ -641,6 +647,8 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, > else > *pRespBufType = CIFS_SMALL_BUFFER; > > + credits = ses->server->ops->get_credits(midQ); > + > rc = ses->server->ops->check_receive(midQ, ses->server, > flags & CIFS_LOG_ERROR); > > @@ -649,7 +657,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, > midQ->resp_buf = NULL; > out: > delete_mid(midQ); > - add_credits(ses->server, 1); > + add_credits(ses->server, credits, optype); > > return rc; > } > @@ -685,7 +693,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, > return -EIO; > } > > - rc = wait_for_free_request(ses->server, long_op); > + rc = wait_for_free_request(ses->server, 0, long_op); > if (rc) > return rc; > > @@ -699,7 +707,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, > if (rc) { > mutex_unlock(&ses->server->srv_mutex); > /* Update # of requests on wire to server */ > - add_credits(ses->server, 1); > + add_credits(ses->server, 1, 0); > return rc; > } > > @@ -731,7 +739,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, > /* no longer considered to be "in-flight" */ > midQ->callback = DeleteMidQEntry; > spin_unlock(&GlobalMid_Lock); > - add_credits(ses->server, 1); > + add_credits(ses->server, 1, 0); > return rc; > } > spin_unlock(&GlobalMid_Lock); > @@ -739,7 +747,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, > > rc = cifs_sync_mid_result(midQ, ses->server); > if (rc != 0) { > - add_credits(ses->server, 1); > + add_credits(ses->server, 1, 0); > return rc; > } > > @@ -755,7 +763,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, > rc = cifs_check_receive(midQ, ses->server, 0); > out: > delete_mid(midQ); > - add_credits(ses->server, 1); > + add_credits(ses->server, 1, 0); > > return rc; > } > @@ -820,7 +828,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon, > return -EIO; > } > > - rc = wait_for_free_request(ses->server, CIFS_BLOCKING_OP); > + rc = wait_for_free_request(ses->server, 0, CIFS_BLOCKING_OP); > if (rc) > return rc; > Other than the nit about renaming that variable, this looks reasonable, at least until we can come up with a better scheme to handle this... Acked-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <20120619083156.761f8a31-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>]
* Re: [PATCH v5 2/5] CIFS: Extend credit mechanism to process request type [not found] ` <20120619083156.761f8a31-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org> @ 2012-06-19 19:53 ` Pavel Shilovsky [not found] ` <CAKywueQw8YaZ0=3Mv-UNZxur=dg_D59m352Msq+hZjsWEHqr1A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 17+ messages in thread From: Pavel Shilovsky @ 2012-06-19 19:53 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA 2012/6/19 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>: > On Sat, 9 Jun 2012 10:26:01 +0400 > Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: > >> Split all requests to echos, oplocks and others - each group uses >> its own credit slot. This is indicated by new flags >> >> CIFS_ECHO_OP and CIFS_OBREAK_OP >> >> that are not used now for CIFS. This change is required to support >> SMB2 protocol because of different processing of these commands. >> >> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> >> --- >> fs/cifs/cifsglob.h | 16 +++++++++++--- >> fs/cifs/cifsproto.h | 2 +- >> fs/cifs/cifssmb.c | 21 ++++++++++--------- >> fs/cifs/smb1ops.c | 12 +++++++++- >> fs/cifs/transport.c | 54 +++++++++++++++++++++++++++++--------------------- >> 5 files changed, 65 insertions(+), 40 deletions(-) >> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h >> index 2aac4e5..844b77c 100644 >> --- a/fs/cifs/cifsglob.h >> +++ b/fs/cifs/cifsglob.h >> @@ -171,9 +171,11 @@ struct smb_version_operations { >> /* check response: verify signature, map error */ >> int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *, >> bool); >> - void (*add_credits)(struct TCP_Server_Info *, const unsigned int); >> + void (*add_credits)(struct TCP_Server_Info *, const unsigned int, >> + const int); >> void (*set_credits)(struct TCP_Server_Info *, const int); >> - int * (*get_credits_field)(struct TCP_Server_Info *); >> + int * (*get_credits_field)(struct TCP_Server_Info *, const int); >> + unsigned int (*get_credits)(struct mid_q_entry *); >> __u64 (*get_next_mid)(struct TCP_Server_Info *); >> /* data offset from read response message */ >> unsigned int (*read_data_offset)(char *); >> @@ -392,9 +394,10 @@ has_credits(struct TCP_Server_Info *server, int *credits) >> } >> >> static inline void >> -add_credits(struct TCP_Server_Info *server, const unsigned int add) >> +add_credits(struct TCP_Server_Info *server, const unsigned int add, >> + const int optype) >> { >> - server->ops->add_credits(server, add); >> + server->ops->add_credits(server, add, optype); >> } >> >> static inline void >> @@ -957,6 +960,11 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, >> #define CIFS_LARGE_BUF_OP 0x020 /* large request buffer */ >> #define CIFS_NO_RESP 0x040 /* no response buffer required */ >> >> +/* Type of request operation */ >> +#define CIFS_ECHO_OP 0x080 /* echo request */ >> +#define CIFS_OBREAK_OP 0x0100 /* oplock break request */ >> +#define CIFS_OP_MASK 0x0180 /* mask request type */ >> + >> /* Security Flags: indicate type of session setup needed */ >> #define CIFSSEC_MAY_SIGN 0x00001 >> #define CIFSSEC_MAY_NTLM 0x00002 >> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h >> index 0a6cbfe..8c1c7c1 100644 >> --- a/fs/cifs/cifsproto.h >> +++ b/fs/cifs/cifsproto.h >> @@ -71,7 +71,7 @@ extern void DeleteMidQEntry(struct mid_q_entry *midEntry); >> extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, >> unsigned int nvec, mid_receive_t *receive, >> mid_callback_t *callback, void *cbdata, >> - bool ignore_pend); >> + const int flags); >> extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *, >> struct smb_hdr * /* input */ , >> struct smb_hdr * /* out */ , >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c >> index 5b40073..58f1ab5 100644 >> --- a/fs/cifs/cifssmb.c >> +++ b/fs/cifs/cifssmb.c >> @@ -720,7 +720,7 @@ cifs_echo_callback(struct mid_q_entry *mid) >> struct TCP_Server_Info *server = mid->callback_data; >> >> DeleteMidQEntry(mid); >> - add_credits(server, 1); >> + add_credits(server, 1, CIFS_ECHO_OP); >> } >> >> int >> @@ -747,7 +747,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server) >> iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4; >> >> rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback, >> - server, true); >> + server, CIFS_ASYNC_OP | CIFS_ECHO_OP); >> if (rc) >> cFYI(1, "Echo request failed: %d", rc); >> >> @@ -1563,7 +1563,7 @@ cifs_readv_callback(struct mid_q_entry *mid) >> >> queue_work(cifsiod_wq, &rdata->work); >> DeleteMidQEntry(mid); >> - add_credits(server, 1); >> + add_credits(server, 1, 0); >> } >> >> /* cifs_async_readv - send an async write, and set up mid to handle result */ >> @@ -1619,7 +1619,7 @@ cifs_async_readv(struct cifs_readdata *rdata) >> kref_get(&rdata->refcount); >> rc = cifs_call_async(tcon->ses->server, rdata->iov, 1, >> cifs_readv_receive, cifs_readv_callback, >> - rdata, false); >> + rdata, 0); >> >> if (rc == 0) >> cifs_stats_inc(&tcon->num_reads); >> @@ -2010,7 +2010,7 @@ cifs_writev_callback(struct mid_q_entry *mid) >> >> queue_work(cifsiod_wq, &wdata->work); >> DeleteMidQEntry(mid); >> - add_credits(tcon->ses->server, 1); >> + add_credits(tcon->ses->server, 1, 0); >> } >> >> /* cifs_async_writev - send an async write, and set up mid to handle result */ >> @@ -2090,7 +2090,7 @@ cifs_async_writev(struct cifs_writedata *wdata) >> >> kref_get(&wdata->refcount); >> rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1, >> - NULL, cifs_writev_callback, wdata, false); >> + NULL, cifs_writev_callback, wdata, 0); >> >> if (rc == 0) >> cifs_stats_inc(&tcon->num_writes); >> @@ -2268,7 +2268,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, >> LOCK_REQ *pSMB = NULL; >> /* LOCK_RSP *pSMBr = NULL; */ /* No response data other than rc to parse */ >> int bytes_returned; >> - int timeout = 0; >> + int flags = 0; >> __u16 count; >> >> cFYI(1, "CIFSSMBLock timeout %d numLock %d", (int)waitFlag, numLock); >> @@ -2278,10 +2278,11 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, >> return rc; >> >> if (lockType == LOCKING_ANDX_OPLOCK_RELEASE) { >> - timeout = CIFS_ASYNC_OP; /* no response expected */ >> + /* no response expected */ >> + flags = CIFS_ASYNC_OP | CIFS_OBREAK_OP; >> pSMB->Timeout = 0; >> } else if (waitFlag) { >> - timeout = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ >> + flags = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ >> pSMB->Timeout = cpu_to_le32(-1);/* blocking - do not time out */ >> } else { >> pSMB->Timeout = 0; >> @@ -2314,7 +2315,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, >> (struct smb_hdr *) pSMB, &bytes_returned); >> cifs_small_buf_release(pSMB); >> } else { >> - rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, timeout); >> + rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, flags); >> /* SMB buffer freed by function above */ >> } >> cifs_stats_inc(&tcon->num_locks); >> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c >> index 28359e7..f4f8394 100644 >> --- a/fs/cifs/smb1ops.c >> +++ b/fs/cifs/smb1ops.c >> @@ -101,7 +101,8 @@ cifs_find_mid(struct TCP_Server_Info *server, char *buffer) >> } >> >> static void >> -cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add) >> +cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add, >> + const int optype) >> { >> spin_lock(&server->req_lock); >> server->credits += add; >> @@ -120,11 +121,17 @@ cifs_set_credits(struct TCP_Server_Info *server, const int val) >> } >> >> static int * >> -cifs_get_credits_field(struct TCP_Server_Info *server) >> +cifs_get_credits_field(struct TCP_Server_Info *server, const int optype) >> { >> return &server->credits; >> } >> >> +static unsigned int >> +cifs_get_credits(struct mid_q_entry *mid) >> +{ >> + return 1; >> +} >> + >> /* >> * Find a free multiplex id (SMB mid). Otherwise there could be >> * mid collisions which might cause problems, demultiplexing the >> @@ -390,6 +397,7 @@ struct smb_version_operations smb1_operations = { >> .add_credits = cifs_add_credits, >> .set_credits = cifs_set_credits, >> .get_credits_field = cifs_get_credits_field, >> + .get_credits = cifs_get_credits, >> .get_next_mid = cifs_get_next_mid, >> .read_data_offset = cifs_read_data_offset, >> .read_data_length = cifs_read_data_length, >> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c >> index 3097ee5..54cd8dd 100644 >> --- a/fs/cifs/transport.c >> +++ b/fs/cifs/transport.c >> @@ -254,13 +254,13 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer, >> } >> >> static int >> -wait_for_free_credits(struct TCP_Server_Info *server, const int optype, >> +wait_for_free_credits(struct TCP_Server_Info *server, const int long_op, >> int *credits) >> { >> int rc; >> >> spin_lock(&server->req_lock); >> - if (optype == CIFS_ASYNC_OP) { >> + if (long_op == CIFS_ASYNC_OP) { >> /* oplock breaks must not be held up */ >> server->in_flight++; >> *credits -= 1; >> @@ -290,7 +290,7 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int optype, >> */ >> >> /* update # of requests on the wire to server */ >> - if (optype != CIFS_BLOCKING_OP) { >> + if (long_op != CIFS_BLOCKING_OP) { >> *credits -= 1; >> server->in_flight++; >> } > > Why rename the variable in this above function? "optype" seems like a better name... That was done to make variable names the same: 'optype' is used to indicate if it is echo, oplock or other operation, 'long_op' is used to indicate if we need any special behavior (aysnc, blocking). wait_for_free_request function (below) takes both 'optype' and 'long_op' and passes 'long_op' to wait_for_free_credits. It is cleaner if wait_for_free_credits uses the same name - 'long_op'. Of course, 'long_op' may be not so good name at all (I took it from the old code) - suggestions are welcome. > >> @@ -302,10 +302,11 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int optype, >> } >> >> static int >> -wait_for_free_request(struct TCP_Server_Info *server, const int optype) >> +wait_for_free_request(struct TCP_Server_Info *server, const int long_op, >> + const int optype) >> { >> - return wait_for_free_credits(server, optype, >> - server->ops->get_credits_field(server)); >> + return wait_for_free_credits(server, long_op, >> + server->ops->get_credits_field(server, optype)); >> } >> >> static int allocate_mid(struct cifs_ses *ses, struct smb_hdr *in_buf, >> @@ -384,12 +385,15 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov, >> int >> cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, >> unsigned int nvec, mid_receive_t *receive, >> - mid_callback_t *callback, void *cbdata, bool ignore_pend) >> + mid_callback_t *callback, void *cbdata, const int flags) >> { >> - int rc; >> + int rc, long_op, optype; >> struct mid_q_entry *mid; >> >> - rc = wait_for_free_request(server, ignore_pend ? CIFS_ASYNC_OP : 0); >> + long_op = flags & CIFS_TIMEOUT_MASK; >> + optype = flags & CIFS_OP_MASK; >> + >> + rc = wait_for_free_request(server, long_op, optype); >> if (rc) >> return rc; >> >> @@ -397,7 +401,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, >> rc = cifs_setup_async_request(server, iov, nvec, &mid); >> if (rc) { >> mutex_unlock(&server->srv_mutex); >> - add_credits(server, 1); >> + add_credits(server, 1, optype); >> wake_up(&server->request_q); >> return rc; >> } >> @@ -419,7 +423,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, >> return rc; >> out_err: >> delete_mid(mid); >> - add_credits(server, 1); >> + add_credits(server, 1, optype); >> wake_up(&server->request_q); >> return rc; >> } >> @@ -539,11 +543,13 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, >> const int flags) >> { >> int rc = 0; >> - int long_op; >> + int long_op, optype; >> struct mid_q_entry *midQ; >> char *buf = iov[0].iov_base; >> + unsigned int credits = 1; >> >> long_op = flags & CIFS_TIMEOUT_MASK; >> + optype = flags & CIFS_OP_MASK; >> >> *pRespBufType = CIFS_NO_BUFFER; /* no response buf yet */ >> >> @@ -564,7 +570,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, >> * use ses->maxReq. >> */ >> >> - rc = wait_for_free_request(ses->server, long_op); >> + rc = wait_for_free_request(ses->server, optype, long_op); >> if (rc) { >> cifs_small_buf_release(buf); >> return rc; >> @@ -583,7 +589,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, >> mutex_unlock(&ses->server->srv_mutex); >> cifs_small_buf_release(buf); >> /* Update # of requests on wire to server */ >> - add_credits(ses->server, 1); >> + add_credits(ses->server, 1, optype); >> return rc; >> } >> >> @@ -613,7 +619,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, >> midQ->callback = DeleteMidQEntry; >> spin_unlock(&GlobalMid_Lock); >> cifs_small_buf_release(buf); >> - add_credits(ses->server, 1); >> + add_credits(ses->server, 1, optype); >> return rc; >> } >> spin_unlock(&GlobalMid_Lock); >> @@ -623,7 +629,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, >> >> rc = cifs_sync_mid_result(midQ, ses->server); >> if (rc != 0) { >> - add_credits(ses->server, 1); >> + add_credits(ses->server, 1, optype); >> return rc; >> } >> >> @@ -641,6 +647,8 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, >> else >> *pRespBufType = CIFS_SMALL_BUFFER; >> >> + credits = ses->server->ops->get_credits(midQ); >> + >> rc = ses->server->ops->check_receive(midQ, ses->server, >> flags & CIFS_LOG_ERROR); >> >> @@ -649,7 +657,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, >> midQ->resp_buf = NULL; >> out: >> delete_mid(midQ); >> - add_credits(ses->server, 1); >> + add_credits(ses->server, credits, optype); >> >> return rc; >> } >> @@ -685,7 +693,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, >> return -EIO; >> } >> >> - rc = wait_for_free_request(ses->server, long_op); >> + rc = wait_for_free_request(ses->server, 0, long_op); >> if (rc) >> return rc; >> >> @@ -699,7 +707,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, >> if (rc) { >> mutex_unlock(&ses->server->srv_mutex); >> /* Update # of requests on wire to server */ >> - add_credits(ses->server, 1); >> + add_credits(ses->server, 1, 0); >> return rc; >> } >> >> @@ -731,7 +739,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, >> /* no longer considered to be "in-flight" */ >> midQ->callback = DeleteMidQEntry; >> spin_unlock(&GlobalMid_Lock); >> - add_credits(ses->server, 1); >> + add_credits(ses->server, 1, 0); >> return rc; >> } >> spin_unlock(&GlobalMid_Lock); >> @@ -739,7 +747,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, >> >> rc = cifs_sync_mid_result(midQ, ses->server); >> if (rc != 0) { >> - add_credits(ses->server, 1); >> + add_credits(ses->server, 1, 0); >> return rc; >> } >> >> @@ -755,7 +763,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses, >> rc = cifs_check_receive(midQ, ses->server, 0); >> out: >> delete_mid(midQ); >> - add_credits(ses->server, 1); >> + add_credits(ses->server, 1, 0); >> >> return rc; >> } >> @@ -820,7 +828,7 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon, >> return -EIO; >> } >> >> - rc = wait_for_free_request(ses->server, CIFS_BLOCKING_OP); >> + rc = wait_for_free_request(ses->server, 0, CIFS_BLOCKING_OP); >> if (rc) >> return rc; >> > > Other than the nit about renaming that variable, this looks reasonable, > at least until we can come up with a better scheme to handle this... > > Acked-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> -- Best regards, Pavel Shilovsky. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CAKywueQw8YaZ0=3Mv-UNZxur=dg_D59m352Msq+hZjsWEHqr1A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH v5 2/5] CIFS: Extend credit mechanism to process request type [not found] ` <CAKywueQw8YaZ0=3Mv-UNZxur=dg_D59m352Msq+hZjsWEHqr1A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2012-06-19 22:00 ` Jeff Layton [not found] ` <20120619150017.73546a39-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org> 0 siblings, 1 reply; 17+ messages in thread From: Jeff Layton @ 2012-06-19 22:00 UTC (permalink / raw) To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA On Tue, 19 Jun 2012 23:53:06 +0400 Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: > 2012/6/19 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>: > > On Sat, 9 Jun 2012 10:26:01 +0400 > > Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: > > > >> Split all requests to echos, oplocks and others - each group uses > >> its own credit slot. This is indicated by new flags > >> > >> CIFS_ECHO_OP and CIFS_OBREAK_OP > >> > >> that are not used now for CIFS. This change is required to support > >> SMB2 protocol because of different processing of these commands. > >> > >> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> > >> --- > >> fs/cifs/cifsglob.h | 16 +++++++++++--- > >> fs/cifs/cifsproto.h | 2 +- > >> fs/cifs/cifssmb.c | 21 ++++++++++--------- > >> fs/cifs/smb1ops.c | 12 +++++++++- > >> fs/cifs/transport.c | 54 +++++++++++++++++++++++++++++--------------------- > >> 5 files changed, 65 insertions(+), 40 deletions(-) > >> > >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > >> index 2aac4e5..844b77c 100644 > >> --- a/fs/cifs/cifsglob.h > >> +++ b/fs/cifs/cifsglob.h > >> @@ -171,9 +171,11 @@ struct smb_version_operations { > >> /* check response: verify signature, map error */ > >> int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *, > >> bool); > >> - void (*add_credits)(struct TCP_Server_Info *, const unsigned int); > >> + void (*add_credits)(struct TCP_Server_Info *, const unsigned int, > >> + const int); > >> void (*set_credits)(struct TCP_Server_Info *, const int); > >> - int * (*get_credits_field)(struct TCP_Server_Info *); > >> + int * (*get_credits_field)(struct TCP_Server_Info *, const int); > >> + unsigned int (*get_credits)(struct mid_q_entry *); > >> __u64 (*get_next_mid)(struct TCP_Server_Info *); > >> /* data offset from read response message */ > >> unsigned int (*read_data_offset)(char *); > >> @@ -392,9 +394,10 @@ has_credits(struct TCP_Server_Info *server, int *credits) > >> } > >> > >> static inline void > >> -add_credits(struct TCP_Server_Info *server, const unsigned int add) > >> +add_credits(struct TCP_Server_Info *server, const unsigned int add, > >> + const int optype) > >> { > >> - server->ops->add_credits(server, add); > >> + server->ops->add_credits(server, add, optype); > >> } > >> > >> static inline void > >> @@ -957,6 +960,11 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, > >> #define CIFS_LARGE_BUF_OP 0x020 /* large request buffer */ > >> #define CIFS_NO_RESP 0x040 /* no response buffer required */ > >> > >> +/* Type of request operation */ > >> +#define CIFS_ECHO_OP 0x080 /* echo request */ > >> +#define CIFS_OBREAK_OP 0x0100 /* oplock break request */ > >> +#define CIFS_OP_MASK 0x0180 /* mask request type */ > >> + > >> /* Security Flags: indicate type of session setup needed */ > >> #define CIFSSEC_MAY_SIGN 0x00001 > >> #define CIFSSEC_MAY_NTLM 0x00002 > >> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > >> index 0a6cbfe..8c1c7c1 100644 > >> --- a/fs/cifs/cifsproto.h > >> +++ b/fs/cifs/cifsproto.h > >> @@ -71,7 +71,7 @@ extern void DeleteMidQEntry(struct mid_q_entry *midEntry); > >> extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, > >> unsigned int nvec, mid_receive_t *receive, > >> mid_callback_t *callback, void *cbdata, > >> - bool ignore_pend); > >> + const int flags); > >> extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *, > >> struct smb_hdr * /* input */ , > >> struct smb_hdr * /* out */ , > >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > >> index 5b40073..58f1ab5 100644 > >> --- a/fs/cifs/cifssmb.c > >> +++ b/fs/cifs/cifssmb.c > >> @@ -720,7 +720,7 @@ cifs_echo_callback(struct mid_q_entry *mid) > >> struct TCP_Server_Info *server = mid->callback_data; > >> > >> DeleteMidQEntry(mid); > >> - add_credits(server, 1); > >> + add_credits(server, 1, CIFS_ECHO_OP); > >> } > >> > >> int > >> @@ -747,7 +747,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server) > >> iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4; > >> > >> rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback, > >> - server, true); > >> + server, CIFS_ASYNC_OP | CIFS_ECHO_OP); > >> if (rc) > >> cFYI(1, "Echo request failed: %d", rc); > >> > >> @@ -1563,7 +1563,7 @@ cifs_readv_callback(struct mid_q_entry *mid) > >> > >> queue_work(cifsiod_wq, &rdata->work); > >> DeleteMidQEntry(mid); > >> - add_credits(server, 1); > >> + add_credits(server, 1, 0); > >> } > >> > >> /* cifs_async_readv - send an async write, and set up mid to handle result */ > >> @@ -1619,7 +1619,7 @@ cifs_async_readv(struct cifs_readdata *rdata) > >> kref_get(&rdata->refcount); > >> rc = cifs_call_async(tcon->ses->server, rdata->iov, 1, > >> cifs_readv_receive, cifs_readv_callback, > >> - rdata, false); > >> + rdata, 0); > >> > >> if (rc == 0) > >> cifs_stats_inc(&tcon->num_reads); > >> @@ -2010,7 +2010,7 @@ cifs_writev_callback(struct mid_q_entry *mid) > >> > >> queue_work(cifsiod_wq, &wdata->work); > >> DeleteMidQEntry(mid); > >> - add_credits(tcon->ses->server, 1); > >> + add_credits(tcon->ses->server, 1, 0); > >> } > >> > >> /* cifs_async_writev - send an async write, and set up mid to handle result */ > >> @@ -2090,7 +2090,7 @@ cifs_async_writev(struct cifs_writedata *wdata) > >> > >> kref_get(&wdata->refcount); > >> rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1, > >> - NULL, cifs_writev_callback, wdata, false); > >> + NULL, cifs_writev_callback, wdata, 0); > >> > >> if (rc == 0) > >> cifs_stats_inc(&tcon->num_writes); > >> @@ -2268,7 +2268,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, > >> LOCK_REQ *pSMB = NULL; > >> /* LOCK_RSP *pSMBr = NULL; */ /* No response data other than rc to parse */ > >> int bytes_returned; > >> - int timeout = 0; > >> + int flags = 0; > >> __u16 count; > >> > >> cFYI(1, "CIFSSMBLock timeout %d numLock %d", (int)waitFlag, numLock); > >> @@ -2278,10 +2278,11 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, > >> return rc; > >> > >> if (lockType == LOCKING_ANDX_OPLOCK_RELEASE) { > >> - timeout = CIFS_ASYNC_OP; /* no response expected */ > >> + /* no response expected */ > >> + flags = CIFS_ASYNC_OP | CIFS_OBREAK_OP; > >> pSMB->Timeout = 0; > >> } else if (waitFlag) { > >> - timeout = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ > >> + flags = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ > >> pSMB->Timeout = cpu_to_le32(-1);/* blocking - do not time out */ > >> } else { > >> pSMB->Timeout = 0; > >> @@ -2314,7 +2315,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, > >> (struct smb_hdr *) pSMB, &bytes_returned); > >> cifs_small_buf_release(pSMB); > >> } else { > >> - rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, timeout); > >> + rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, flags); > >> /* SMB buffer freed by function above */ > >> } > >> cifs_stats_inc(&tcon->num_locks); > >> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > >> index 28359e7..f4f8394 100644 > >> --- a/fs/cifs/smb1ops.c > >> +++ b/fs/cifs/smb1ops.c > >> @@ -101,7 +101,8 @@ cifs_find_mid(struct TCP_Server_Info *server, char *buffer) > >> } > >> > >> static void > >> -cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add) > >> +cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add, > >> + const int optype) > >> { > >> spin_lock(&server->req_lock); > >> server->credits += add; > >> @@ -120,11 +121,17 @@ cifs_set_credits(struct TCP_Server_Info *server, const int val) > >> } > >> > >> static int * > >> -cifs_get_credits_field(struct TCP_Server_Info *server) > >> +cifs_get_credits_field(struct TCP_Server_Info *server, const int optype) > >> { > >> return &server->credits; > >> } > >> > >> +static unsigned int > >> +cifs_get_credits(struct mid_q_entry *mid) > >> +{ > >> + return 1; > >> +} > >> + > >> /* > >> * Find a free multiplex id (SMB mid). Otherwise there could be > >> * mid collisions which might cause problems, demultiplexing the > >> @@ -390,6 +397,7 @@ struct smb_version_operations smb1_operations = { > >> .add_credits = cifs_add_credits, > >> .set_credits = cifs_set_credits, > >> .get_credits_field = cifs_get_credits_field, > >> + .get_credits = cifs_get_credits, > >> .get_next_mid = cifs_get_next_mid, > >> .read_data_offset = cifs_read_data_offset, > >> .read_data_length = cifs_read_data_length, > >> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > >> index 3097ee5..54cd8dd 100644 > >> --- a/fs/cifs/transport.c > >> +++ b/fs/cifs/transport.c > >> @@ -254,13 +254,13 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer, > >> } > >> > >> static int > >> -wait_for_free_credits(struct TCP_Server_Info *server, const int optype, > >> +wait_for_free_credits(struct TCP_Server_Info *server, const int long_op, > >> int *credits) > >> { > >> int rc; > >> > >> spin_lock(&server->req_lock); > >> - if (optype == CIFS_ASYNC_OP) { > >> + if (long_op == CIFS_ASYNC_OP) { > >> /* oplock breaks must not be held up */ > >> server->in_flight++; > >> *credits -= 1; > >> @@ -290,7 +290,7 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int optype, > >> */ > >> > >> /* update # of requests on the wire to server */ > >> - if (optype != CIFS_BLOCKING_OP) { > >> + if (long_op != CIFS_BLOCKING_OP) { > >> *credits -= 1; > >> server->in_flight++; > >> } > > > > Why rename the variable in this above function? "optype" seems like a better name... > > That was done to make variable names the same: 'optype' is used to > indicate if it is echo, oplock or other operation, 'long_op' is used > to indicate if we need any special behavior (aysnc, blocking). > > wait_for_free_request function (below) takes both 'optype' and > 'long_op' and passes 'long_op' to wait_for_free_credits. It is cleaner > if wait_for_free_credits uses the same name - 'long_op'. > > Of course, 'long_op' may be not so good name at all (I took it from > the old code) - suggestions are welcome. > Yeah, I think that's the confusion. "long_op" was from when those flags were supposed to indicate the length of the operation so the code could adjust the timeout. Maybe a better name is something generic like "flags" ? -- Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <20120619150017.73546a39-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>]
* Re: [PATCH v5 2/5] CIFS: Extend credit mechanism to process request type [not found] ` <20120619150017.73546a39-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org> @ 2012-06-19 23:57 ` Steve French [not found] ` <CAH2r5muXdc708-HtceH02y8TBcoAXmi_qjyay0qHeZdtmoxVZw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 17+ messages in thread From: Steve French @ 2012-06-19 23:57 UTC (permalink / raw) To: Jeff Layton; +Cc: Pavel Shilovsky, linux-cifs-u79uwXL29TY76Z2rM5mHXA On Wed, Jun 20, 2012 at 12:00 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > On Tue, 19 Jun 2012 23:53:06 +0400 > Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: > >> 2012/6/19 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>: >> > On Sat, 9 Jun 2012 10:26:01 +0400 >> > Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: >> > >> >> Split all requests to echos, oplocks and others - each group uses >> >> its own credit slot. This is indicated by new flags >> >> >> >> CIFS_ECHO_OP and CIFS_OBREAK_OP >> >> >> >> that are not used now for CIFS. This change is required to support >> >> SMB2 protocol because of different processing of these commands. >> >> >> >> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> >> >> --- >> >> fs/cifs/cifsglob.h | 16 +++++++++++--- >> >> fs/cifs/cifsproto.h | 2 +- >> >> fs/cifs/cifssmb.c | 21 ++++++++++--------- >> >> fs/cifs/smb1ops.c | 12 +++++++++- >> >> fs/cifs/transport.c | 54 +++++++++++++++++++++++++++++--------------------- >> >> 5 files changed, 65 insertions(+), 40 deletions(-) >> >> >> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h >> >> index 2aac4e5..844b77c 100644 >> >> --- a/fs/cifs/cifsglob.h >> >> +++ b/fs/cifs/cifsglob.h >> >> @@ -171,9 +171,11 @@ struct smb_version_operations { >> >> /* check response: verify signature, map error */ >> >> int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *, >> >> bool); >> >> - void (*add_credits)(struct TCP_Server_Info *, const unsigned int); >> >> + void (*add_credits)(struct TCP_Server_Info *, const unsigned int, >> >> + const int); >> >> void (*set_credits)(struct TCP_Server_Info *, const int); >> >> - int * (*get_credits_field)(struct TCP_Server_Info *); >> >> + int * (*get_credits_field)(struct TCP_Server_Info *, const int); >> >> + unsigned int (*get_credits)(struct mid_q_entry *); >> >> __u64 (*get_next_mid)(struct TCP_Server_Info *); >> >> /* data offset from read response message */ >> >> unsigned int (*read_data_offset)(char *); >> >> @@ -392,9 +394,10 @@ has_credits(struct TCP_Server_Info *server, int *credits) >> >> } >> >> >> >> static inline void >> >> -add_credits(struct TCP_Server_Info *server, const unsigned int add) >> >> +add_credits(struct TCP_Server_Info *server, const unsigned int add, >> >> + const int optype) >> >> { >> >> - server->ops->add_credits(server, add); >> >> + server->ops->add_credits(server, add, optype); >> >> } >> >> >> >> static inline void >> >> @@ -957,6 +960,11 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, >> >> #define CIFS_LARGE_BUF_OP 0x020 /* large request buffer */ >> >> #define CIFS_NO_RESP 0x040 /* no response buffer required */ >> >> >> >> +/* Type of request operation */ >> >> +#define CIFS_ECHO_OP 0x080 /* echo request */ >> >> +#define CIFS_OBREAK_OP 0x0100 /* oplock break request */ >> >> +#define CIFS_OP_MASK 0x0180 /* mask request type */ >> >> + >> >> /* Security Flags: indicate type of session setup needed */ >> >> #define CIFSSEC_MAY_SIGN 0x00001 >> >> #define CIFSSEC_MAY_NTLM 0x00002 >> >> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h >> >> index 0a6cbfe..8c1c7c1 100644 >> >> --- a/fs/cifs/cifsproto.h >> >> +++ b/fs/cifs/cifsproto.h >> >> @@ -71,7 +71,7 @@ extern void DeleteMidQEntry(struct mid_q_entry *midEntry); >> >> extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, >> >> unsigned int nvec, mid_receive_t *receive, >> >> mid_callback_t *callback, void *cbdata, >> >> - bool ignore_pend); >> >> + const int flags); >> >> extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *, >> >> struct smb_hdr * /* input */ , >> >> struct smb_hdr * /* out */ , >> >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c >> >> index 5b40073..58f1ab5 100644 >> >> --- a/fs/cifs/cifssmb.c >> >> +++ b/fs/cifs/cifssmb.c >> >> @@ -720,7 +720,7 @@ cifs_echo_callback(struct mid_q_entry *mid) >> >> struct TCP_Server_Info *server = mid->callback_data; >> >> >> >> DeleteMidQEntry(mid); >> >> - add_credits(server, 1); >> >> + add_credits(server, 1, CIFS_ECHO_OP); >> >> } >> >> >> >> int >> >> @@ -747,7 +747,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server) >> >> iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4; >> >> >> >> rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback, >> >> - server, true); >> >> + server, CIFS_ASYNC_OP | CIFS_ECHO_OP); >> >> if (rc) >> >> cFYI(1, "Echo request failed: %d", rc); >> >> >> >> @@ -1563,7 +1563,7 @@ cifs_readv_callback(struct mid_q_entry *mid) >> >> >> >> queue_work(cifsiod_wq, &rdata->work); >> >> DeleteMidQEntry(mid); >> >> - add_credits(server, 1); >> >> + add_credits(server, 1, 0); >> >> } >> >> >> >> /* cifs_async_readv - send an async write, and set up mid to handle result */ >> >> @@ -1619,7 +1619,7 @@ cifs_async_readv(struct cifs_readdata *rdata) >> >> kref_get(&rdata->refcount); >> >> rc = cifs_call_async(tcon->ses->server, rdata->iov, 1, >> >> cifs_readv_receive, cifs_readv_callback, >> >> - rdata, false); >> >> + rdata, 0); >> >> >> >> if (rc == 0) >> >> cifs_stats_inc(&tcon->num_reads); >> >> @@ -2010,7 +2010,7 @@ cifs_writev_callback(struct mid_q_entry *mid) >> >> >> >> queue_work(cifsiod_wq, &wdata->work); >> >> DeleteMidQEntry(mid); >> >> - add_credits(tcon->ses->server, 1); >> >> + add_credits(tcon->ses->server, 1, 0); >> >> } >> >> >> >> /* cifs_async_writev - send an async write, and set up mid to handle result */ >> >> @@ -2090,7 +2090,7 @@ cifs_async_writev(struct cifs_writedata *wdata) >> >> >> >> kref_get(&wdata->refcount); >> >> rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1, >> >> - NULL, cifs_writev_callback, wdata, false); >> >> + NULL, cifs_writev_callback, wdata, 0); >> >> >> >> if (rc == 0) >> >> cifs_stats_inc(&tcon->num_writes); >> >> @@ -2268,7 +2268,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, >> >> LOCK_REQ *pSMB = NULL; >> >> /* LOCK_RSP *pSMBr = NULL; */ /* No response data other than rc to parse */ >> >> int bytes_returned; >> >> - int timeout = 0; >> >> + int flags = 0; >> >> __u16 count; >> >> >> >> cFYI(1, "CIFSSMBLock timeout %d numLock %d", (int)waitFlag, numLock); >> >> @@ -2278,10 +2278,11 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, >> >> return rc; >> >> >> >> if (lockType == LOCKING_ANDX_OPLOCK_RELEASE) { >> >> - timeout = CIFS_ASYNC_OP; /* no response expected */ >> >> + /* no response expected */ >> >> + flags = CIFS_ASYNC_OP | CIFS_OBREAK_OP; >> >> pSMB->Timeout = 0; >> >> } else if (waitFlag) { >> >> - timeout = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ >> >> + flags = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ >> >> pSMB->Timeout = cpu_to_le32(-1);/* blocking - do not time out */ >> >> } else { >> >> pSMB->Timeout = 0; >> >> @@ -2314,7 +2315,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, >> >> (struct smb_hdr *) pSMB, &bytes_returned); >> >> cifs_small_buf_release(pSMB); >> >> } else { >> >> - rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, timeout); >> >> + rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, flags); >> >> /* SMB buffer freed by function above */ >> >> } >> >> cifs_stats_inc(&tcon->num_locks); >> >> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c >> >> index 28359e7..f4f8394 100644 >> >> --- a/fs/cifs/smb1ops.c >> >> +++ b/fs/cifs/smb1ops.c >> >> @@ -101,7 +101,8 @@ cifs_find_mid(struct TCP_Server_Info *server, char *buffer) >> >> } >> >> >> >> static void >> >> -cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add) >> >> +cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add, >> >> + const int optype) >> >> { >> >> spin_lock(&server->req_lock); >> >> server->credits += add; >> >> @@ -120,11 +121,17 @@ cifs_set_credits(struct TCP_Server_Info *server, const int val) >> >> } >> >> >> >> static int * >> >> -cifs_get_credits_field(struct TCP_Server_Info *server) >> >> +cifs_get_credits_field(struct TCP_Server_Info *server, const int optype) >> >> { >> >> return &server->credits; >> >> } >> >> >> >> +static unsigned int >> >> +cifs_get_credits(struct mid_q_entry *mid) >> >> +{ >> >> + return 1; >> >> +} >> >> + >> >> /* >> >> * Find a free multiplex id (SMB mid). Otherwise there could be >> >> * mid collisions which might cause problems, demultiplexing the >> >> @@ -390,6 +397,7 @@ struct smb_version_operations smb1_operations = { >> >> .add_credits = cifs_add_credits, >> >> .set_credits = cifs_set_credits, >> >> .get_credits_field = cifs_get_credits_field, >> >> + .get_credits = cifs_get_credits, >> >> .get_next_mid = cifs_get_next_mid, >> >> .read_data_offset = cifs_read_data_offset, >> >> .read_data_length = cifs_read_data_length, >> >> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c >> >> index 3097ee5..54cd8dd 100644 >> >> --- a/fs/cifs/transport.c >> >> +++ b/fs/cifs/transport.c >> >> @@ -254,13 +254,13 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer, >> >> } >> >> >> >> static int >> >> -wait_for_free_credits(struct TCP_Server_Info *server, const int optype, >> >> +wait_for_free_credits(struct TCP_Server_Info *server, const int long_op, >> >> int *credits) >> >> { >> >> int rc; >> >> >> >> spin_lock(&server->req_lock); >> >> - if (optype == CIFS_ASYNC_OP) { >> >> + if (long_op == CIFS_ASYNC_OP) { >> >> /* oplock breaks must not be held up */ >> >> server->in_flight++; >> >> *credits -= 1; >> >> @@ -290,7 +290,7 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int optype, >> >> */ >> >> >> >> /* update # of requests on the wire to server */ >> >> - if (optype != CIFS_BLOCKING_OP) { >> >> + if (long_op != CIFS_BLOCKING_OP) { >> >> *credits -= 1; >> >> server->in_flight++; >> >> } >> > >> > Why rename the variable in this above function? "optype" seems like a better name... >> >> That was done to make variable names the same: 'optype' is used to >> indicate if it is echo, oplock or other operation, 'long_op' is used >> to indicate if we need any special behavior (aysnc, blocking). >> >> wait_for_free_request function (below) takes both 'optype' and >> 'long_op' and passes 'long_op' to wait_for_free_credits. It is cleaner >> if wait_for_free_credits uses the same name - 'long_op'. >> >> Of course, 'long_op' may be not so good name at all (I took it from >> the old code) - suggestions are welcome. >> > > Yeah, I think that's the confusion. "long_op" was from when those flags > were supposed to indicate the length of the operation so the code could > adjust the timeout. Maybe a better name is something generic like > "flags" ? "flags" is fine with me -- Thanks, Steve ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CAH2r5muXdc708-HtceH02y8TBcoAXmi_qjyay0qHeZdtmoxVZw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH v5 2/5] CIFS: Extend credit mechanism to process request type [not found] ` <CAH2r5muXdc708-HtceH02y8TBcoAXmi_qjyay0qHeZdtmoxVZw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2012-06-20 7:12 ` Pavel Shilovsky 0 siblings, 0 replies; 17+ messages in thread From: Pavel Shilovsky @ 2012-06-20 7:12 UTC (permalink / raw) To: Steve French; +Cc: Jeff Layton, linux-cifs-u79uwXL29TY76Z2rM5mHXA 2012/6/20 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>: > On Wed, Jun 20, 2012 at 12:00 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: >> On Tue, 19 Jun 2012 23:53:06 +0400 >> Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: >> >>> 2012/6/19 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>: >>> > On Sat, 9 Jun 2012 10:26:01 +0400 >>> > Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: >>> > >>> >> Split all requests to echos, oplocks and others - each group uses >>> >> its own credit slot. This is indicated by new flags >>> >> >>> >> CIFS_ECHO_OP and CIFS_OBREAK_OP >>> >> >>> >> that are not used now for CIFS. This change is required to support >>> >> SMB2 protocol because of different processing of these commands. >>> >> >>> >> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> >>> >> --- >>> >> fs/cifs/cifsglob.h | 16 +++++++++++--- >>> >> fs/cifs/cifsproto.h | 2 +- >>> >> fs/cifs/cifssmb.c | 21 ++++++++++--------- >>> >> fs/cifs/smb1ops.c | 12 +++++++++- >>> >> fs/cifs/transport.c | 54 +++++++++++++++++++++++++++++--------------------- >>> >> 5 files changed, 65 insertions(+), 40 deletions(-) >>> >> >>> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h >>> >> index 2aac4e5..844b77c 100644 >>> >> --- a/fs/cifs/cifsglob.h >>> >> +++ b/fs/cifs/cifsglob.h >>> >> @@ -171,9 +171,11 @@ struct smb_version_operations { >>> >> /* check response: verify signature, map error */ >>> >> int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *, >>> >> bool); >>> >> - void (*add_credits)(struct TCP_Server_Info *, const unsigned int); >>> >> + void (*add_credits)(struct TCP_Server_Info *, const unsigned int, >>> >> + const int); >>> >> void (*set_credits)(struct TCP_Server_Info *, const int); >>> >> - int * (*get_credits_field)(struct TCP_Server_Info *); >>> >> + int * (*get_credits_field)(struct TCP_Server_Info *, const int); >>> >> + unsigned int (*get_credits)(struct mid_q_entry *); >>> >> __u64 (*get_next_mid)(struct TCP_Server_Info *); >>> >> /* data offset from read response message */ >>> >> unsigned int (*read_data_offset)(char *); >>> >> @@ -392,9 +394,10 @@ has_credits(struct TCP_Server_Info *server, int *credits) >>> >> } >>> >> >>> >> static inline void >>> >> -add_credits(struct TCP_Server_Info *server, const unsigned int add) >>> >> +add_credits(struct TCP_Server_Info *server, const unsigned int add, >>> >> + const int optype) >>> >> { >>> >> - server->ops->add_credits(server, add); >>> >> + server->ops->add_credits(server, add, optype); >>> >> } >>> >> >>> >> static inline void >>> >> @@ -957,6 +960,11 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param, >>> >> #define CIFS_LARGE_BUF_OP 0x020 /* large request buffer */ >>> >> #define CIFS_NO_RESP 0x040 /* no response buffer required */ >>> >> >>> >> +/* Type of request operation */ >>> >> +#define CIFS_ECHO_OP 0x080 /* echo request */ >>> >> +#define CIFS_OBREAK_OP 0x0100 /* oplock break request */ >>> >> +#define CIFS_OP_MASK 0x0180 /* mask request type */ >>> >> + >>> >> /* Security Flags: indicate type of session setup needed */ >>> >> #define CIFSSEC_MAY_SIGN 0x00001 >>> >> #define CIFSSEC_MAY_NTLM 0x00002 >>> >> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h >>> >> index 0a6cbfe..8c1c7c1 100644 >>> >> --- a/fs/cifs/cifsproto.h >>> >> +++ b/fs/cifs/cifsproto.h >>> >> @@ -71,7 +71,7 @@ extern void DeleteMidQEntry(struct mid_q_entry *midEntry); >>> >> extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov, >>> >> unsigned int nvec, mid_receive_t *receive, >>> >> mid_callback_t *callback, void *cbdata, >>> >> - bool ignore_pend); >>> >> + const int flags); >>> >> extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *, >>> >> struct smb_hdr * /* input */ , >>> >> struct smb_hdr * /* out */ , >>> >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c >>> >> index 5b40073..58f1ab5 100644 >>> >> --- a/fs/cifs/cifssmb.c >>> >> +++ b/fs/cifs/cifssmb.c >>> >> @@ -720,7 +720,7 @@ cifs_echo_callback(struct mid_q_entry *mid) >>> >> struct TCP_Server_Info *server = mid->callback_data; >>> >> >>> >> DeleteMidQEntry(mid); >>> >> - add_credits(server, 1); >>> >> + add_credits(server, 1, CIFS_ECHO_OP); >>> >> } >>> >> >>> >> int >>> >> @@ -747,7 +747,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server) >>> >> iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4; >>> >> >>> >> rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback, >>> >> - server, true); >>> >> + server, CIFS_ASYNC_OP | CIFS_ECHO_OP); >>> >> if (rc) >>> >> cFYI(1, "Echo request failed: %d", rc); >>> >> >>> >> @@ -1563,7 +1563,7 @@ cifs_readv_callback(struct mid_q_entry *mid) >>> >> >>> >> queue_work(cifsiod_wq, &rdata->work); >>> >> DeleteMidQEntry(mid); >>> >> - add_credits(server, 1); >>> >> + add_credits(server, 1, 0); >>> >> } >>> >> >>> >> /* cifs_async_readv - send an async write, and set up mid to handle result */ >>> >> @@ -1619,7 +1619,7 @@ cifs_async_readv(struct cifs_readdata *rdata) >>> >> kref_get(&rdata->refcount); >>> >> rc = cifs_call_async(tcon->ses->server, rdata->iov, 1, >>> >> cifs_readv_receive, cifs_readv_callback, >>> >> - rdata, false); >>> >> + rdata, 0); >>> >> >>> >> if (rc == 0) >>> >> cifs_stats_inc(&tcon->num_reads); >>> >> @@ -2010,7 +2010,7 @@ cifs_writev_callback(struct mid_q_entry *mid) >>> >> >>> >> queue_work(cifsiod_wq, &wdata->work); >>> >> DeleteMidQEntry(mid); >>> >> - add_credits(tcon->ses->server, 1); >>> >> + add_credits(tcon->ses->server, 1, 0); >>> >> } >>> >> >>> >> /* cifs_async_writev - send an async write, and set up mid to handle result */ >>> >> @@ -2090,7 +2090,7 @@ cifs_async_writev(struct cifs_writedata *wdata) >>> >> >>> >> kref_get(&wdata->refcount); >>> >> rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1, >>> >> - NULL, cifs_writev_callback, wdata, false); >>> >> + NULL, cifs_writev_callback, wdata, 0); >>> >> >>> >> if (rc == 0) >>> >> cifs_stats_inc(&tcon->num_writes); >>> >> @@ -2268,7 +2268,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, >>> >> LOCK_REQ *pSMB = NULL; >>> >> /* LOCK_RSP *pSMBr = NULL; */ /* No response data other than rc to parse */ >>> >> int bytes_returned; >>> >> - int timeout = 0; >>> >> + int flags = 0; >>> >> __u16 count; >>> >> >>> >> cFYI(1, "CIFSSMBLock timeout %d numLock %d", (int)waitFlag, numLock); >>> >> @@ -2278,10 +2278,11 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, >>> >> return rc; >>> >> >>> >> if (lockType == LOCKING_ANDX_OPLOCK_RELEASE) { >>> >> - timeout = CIFS_ASYNC_OP; /* no response expected */ >>> >> + /* no response expected */ >>> >> + flags = CIFS_ASYNC_OP | CIFS_OBREAK_OP; >>> >> pSMB->Timeout = 0; >>> >> } else if (waitFlag) { >>> >> - timeout = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ >>> >> + flags = CIFS_BLOCKING_OP; /* blocking operation, no timeout */ >>> >> pSMB->Timeout = cpu_to_le32(-1);/* blocking - do not time out */ >>> >> } else { >>> >> pSMB->Timeout = 0; >>> >> @@ -2314,7 +2315,7 @@ CIFSSMBLock(const int xid, struct cifs_tcon *tcon, >>> >> (struct smb_hdr *) pSMB, &bytes_returned); >>> >> cifs_small_buf_release(pSMB); >>> >> } else { >>> >> - rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, timeout); >>> >> + rc = SendReceiveNoRsp(xid, tcon->ses, (char *)pSMB, flags); >>> >> /* SMB buffer freed by function above */ >>> >> } >>> >> cifs_stats_inc(&tcon->num_locks); >>> >> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c >>> >> index 28359e7..f4f8394 100644 >>> >> --- a/fs/cifs/smb1ops.c >>> >> +++ b/fs/cifs/smb1ops.c >>> >> @@ -101,7 +101,8 @@ cifs_find_mid(struct TCP_Server_Info *server, char *buffer) >>> >> } >>> >> >>> >> static void >>> >> -cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add) >>> >> +cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add, >>> >> + const int optype) >>> >> { >>> >> spin_lock(&server->req_lock); >>> >> server->credits += add; >>> >> @@ -120,11 +121,17 @@ cifs_set_credits(struct TCP_Server_Info *server, const int val) >>> >> } >>> >> >>> >> static int * >>> >> -cifs_get_credits_field(struct TCP_Server_Info *server) >>> >> +cifs_get_credits_field(struct TCP_Server_Info *server, const int optype) >>> >> { >>> >> return &server->credits; >>> >> } >>> >> >>> >> +static unsigned int >>> >> +cifs_get_credits(struct mid_q_entry *mid) >>> >> +{ >>> >> + return 1; >>> >> +} >>> >> + >>> >> /* >>> >> * Find a free multiplex id (SMB mid). Otherwise there could be >>> >> * mid collisions which might cause problems, demultiplexing the >>> >> @@ -390,6 +397,7 @@ struct smb_version_operations smb1_operations = { >>> >> .add_credits = cifs_add_credits, >>> >> .set_credits = cifs_set_credits, >>> >> .get_credits_field = cifs_get_credits_field, >>> >> + .get_credits = cifs_get_credits, >>> >> .get_next_mid = cifs_get_next_mid, >>> >> .read_data_offset = cifs_read_data_offset, >>> >> .read_data_length = cifs_read_data_length, >>> >> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c >>> >> index 3097ee5..54cd8dd 100644 >>> >> --- a/fs/cifs/transport.c >>> >> +++ b/fs/cifs/transport.c >>> >> @@ -254,13 +254,13 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer, >>> >> } >>> >> >>> >> static int >>> >> -wait_for_free_credits(struct TCP_Server_Info *server, const int optype, >>> >> +wait_for_free_credits(struct TCP_Server_Info *server, const int long_op, >>> >> int *credits) >>> >> { >>> >> int rc; >>> >> >>> >> spin_lock(&server->req_lock); >>> >> - if (optype == CIFS_ASYNC_OP) { >>> >> + if (long_op == CIFS_ASYNC_OP) { >>> >> /* oplock breaks must not be held up */ >>> >> server->in_flight++; >>> >> *credits -= 1; >>> >> @@ -290,7 +290,7 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int optype, >>> >> */ >>> >> >>> >> /* update # of requests on the wire to server */ >>> >> - if (optype != CIFS_BLOCKING_OP) { >>> >> + if (long_op != CIFS_BLOCKING_OP) { >>> >> *credits -= 1; >>> >> server->in_flight++; >>> >> } >>> > >>> > Why rename the variable in this above function? "optype" seems like a better name... >>> >>> That was done to make variable names the same: 'optype' is used to >>> indicate if it is echo, oplock or other operation, 'long_op' is used >>> to indicate if we need any special behavior (aysnc, blocking). >>> >>> wait_for_free_request function (below) takes both 'optype' and >>> 'long_op' and passes 'long_op' to wait_for_free_credits. It is cleaner >>> if wait_for_free_credits uses the same name - 'long_op'. >>> >>> Of course, 'long_op' may be not so good name at all (I took it from >>> the old code) - suggestions are welcome. >>> >> >> Yeah, I think that's the confusion. "long_op" was from when those flags >> were supposed to indicate the length of the operation so the code could >> adjust the timeout. Maybe a better name is something generic like >> "flags" ? > > "flags" is fine with me 'flags' is suppose to indicate all request flags (credit slot, timeout, etc). As soon as we mask CIFS_ASYNC_OP and CIFS_BLOCKING_OP with CIFS_TIMEOUT_MASK macro we should use something close to it: e.g. optimeout, timeout, etc. I think we can rethink names of mask macro and variables further - let's not stop on this. -- Best regards, Pavel Shilovsky. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 3/5] CIFS: Move protocol specific negotiate code to ops struct [not found] ` <1339223164-9721-1-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> 2012-06-09 6:26 ` [PATCH v5 1/5] CIFS: Move trans2 processing " Pavel Shilovsky 2012-06-09 6:26 ` [PATCH v5 2/5] CIFS: Extend credit mechanism to process request type Pavel Shilovsky @ 2012-06-09 6:26 ` Pavel Shilovsky [not found] ` <1339223164-9721-4-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> 2012-06-09 6:26 ` [PATCH v5 4/5] CIFS: Move protocol specific session setup/logoff " Pavel Shilovsky 2012-06-09 6:26 ` [PATCH v5 5/5] CIFS: Move protocol specific tcon/tdis " Pavel Shilovsky 4 siblings, 1 reply; 17+ messages in thread From: Pavel Shilovsky @ 2012-06-09 6:26 UTC (permalink / raw) To: linux-cifs-u79uwXL29TY76Z2rM5mHXA Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> --- fs/cifs/cifsglob.h | 8 ++++++-- fs/cifs/cifsproto.h | 5 ++--- fs/cifs/cifssmb.c | 4 ++-- fs/cifs/connect.c | 21 +++++++++------------ fs/cifs/sess.c | 2 +- fs/cifs/smb1ops.c | 23 +++++++++++++++++++++++ 6 files changed, 43 insertions(+), 20 deletions(-) diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 844b77c..669de88 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -192,6 +192,10 @@ struct smb_version_operations { /* process transaction2 response */ bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *, char *, int); + /* check if we need to negotiate */ + bool (*need_neg)(struct TCP_Server_Info *); + /* negotiate to the server */ + int (*negotiate)(const int, struct cifs_ses *); }; struct smb_version_values { @@ -324,7 +328,7 @@ struct TCP_Server_Info { struct mutex srv_mutex; struct task_struct *tsk; char server_GUID[16]; - char sec_mode; + __u16 sec_mode; bool session_estab; /* mark when very first sess is established */ u16 dialect; /* dialect index that server chose */ enum securityEnum secType; @@ -459,7 +463,7 @@ struct cifs_ses { char *serverOS; /* name of operating system underlying server */ char *serverNOS; /* name of network operating system of server */ char *serverDomain; /* security realm of server */ - int Suid; /* remote smb uid */ + __u64 Suid; /* remote smb uid */ uid_t linux_uid; /* overriding owner of files on the mount */ uid_t cred_uid; /* owner of credentials */ int capabilities; diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index 8c1c7c1..32b569f 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -178,11 +178,10 @@ extern void cifs_dfs_release_automount_timer(void); void cifs_proc_init(void); void cifs_proc_clean(void); -extern int cifs_negotiate_protocol(unsigned int xid, - struct cifs_ses *ses); +extern int cifs_negotiate_protocol(const int xid, struct cifs_ses *ses); extern int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, struct nls_table *nls_info); -extern int CIFSSMBNegotiate(unsigned int xid, struct cifs_ses *ses); +extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses); extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses, const char *tree, struct cifs_tcon *tcon, diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index 58f1ab5..fbc5628 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -372,7 +372,7 @@ static inline void inc_rfc1001_len(void *pSMB, int count) } int -CIFSSMBNegotiate(unsigned int xid, struct cifs_ses *ses) +CIFSSMBNegotiate(const int xid, struct cifs_ses *ses) { NEGOTIATE_REQ *pSMB; NEGOTIATE_RSP *pSMBr; @@ -456,7 +456,7 @@ CIFSSMBNegotiate(unsigned int xid, struct cifs_ses *ses) rc = -EOPNOTSUPP; goto neg_err_exit; } - server->sec_mode = (__u8)le16_to_cpu(rsp->SecurityMode); + server->sec_mode = le16_to_cpu(rsp->SecurityMode); server->maxReq = min_t(unsigned int, le16_to_cpu(rsp->MaxMpxCount), cifs_max_pending); diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index ca0288d..4780342 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -406,7 +406,7 @@ cifs_echo_request(struct work_struct *work) * done, which is indicated by maxBuf != 0. Also, no need to ping if * we got a response recently */ - if (server->maxBuf == 0 || + if (!server->ops->need_neg || server->ops->need_neg(server) || time_before(jiffies, server->lstrp + SMB_ECHO_INTERVAL - HZ)) goto requeue_echo; @@ -3942,24 +3942,22 @@ cifs_umount(struct cifs_sb_info *cifs_sb) kfree(cifs_sb); } -int cifs_negotiate_protocol(unsigned int xid, struct cifs_ses *ses) +int +cifs_negotiate_protocol(const int xid, struct cifs_ses *ses) { int rc = 0; struct TCP_Server_Info *server = ses->server; + if (!server->ops->need_neg || !server->ops->negotiate) + return -ENOSYS; + /* only send once per connect */ - if (server->maxBuf != 0) + if (!server->ops->need_neg(server)) return 0; set_credits(server, 1); - rc = CIFSSMBNegotiate(xid, ses); - if (rc == -EAGAIN) { - /* retry only once on 1st time connection */ - set_credits(server, 1); - rc = CIFSSMBNegotiate(xid, ses); - if (rc == -EAGAIN) - rc = -EHOSTDOWN; - } + + rc = server->ops->negotiate(xid, ses); if (rc == 0) { spin_lock(&GlobalMid_Lock); if (server->tcpStatus == CifsNeedNegotiate) @@ -3967,7 +3965,6 @@ int cifs_negotiate_protocol(unsigned int xid, struct cifs_ses *ses) else rc = -EHOSTDOWN; spin_unlock(&GlobalMid_Lock); - } return rc; diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c index 551d0c2..f88fa4d 100644 --- a/fs/cifs/sess.c +++ b/fs/cifs/sess.c @@ -898,7 +898,7 @@ ssetup_ntlmssp_authenticate: if (action & GUEST_LOGIN) cFYI(1, "Guest login"); /* BB mark SesInfo struct? */ ses->Suid = smb_buf->Uid; /* UID left in wire format (le) */ - cFYI(1, "UID = %d ", ses->Suid); + cFYI(1, "UID = %llu ", ses->Suid); /* response can have either 3 or 4 word count - Samba sends 3 */ /* and lanman response is 3 */ bytes_remaining = get_bcc(smb_buf); diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c index f4f8394..5bfc26a 100644 --- a/fs/cifs/smb1ops.c +++ b/fs/cifs/smb1ops.c @@ -389,6 +389,27 @@ cifs_check_trans2(struct mid_q_entry *mid, struct TCP_Server_Info *server, return true; } +static bool +cifs_need_neg(struct TCP_Server_Info *server) +{ + return server->maxBuf == 0; +} + +static int +cifs_negotiate(const int xid, struct cifs_ses *ses) +{ + int rc; + rc = CIFSSMBNegotiate(xid, ses); + if (rc == -EAGAIN) { + /* retry only once on 1st time connection */ + set_credits(ses->server, 1); + rc = CIFSSMBNegotiate(xid, ses); + if (rc == -EAGAIN) + rc = -EHOSTDOWN; + } + return rc; +} + struct smb_version_operations smb1_operations = { .send_cancel = send_nt_cancel, .compare_fids = cifs_compare_fids, @@ -407,6 +428,8 @@ struct smb_version_operations smb1_operations = { .dump_detail = cifs_dump_detail, .is_oplock_break = is_valid_oplock_break, .check_trans2 = cifs_check_trans2, + .need_neg = cifs_need_neg, + .negotiate = cifs_negotiate, }; struct smb_version_values smb1_values = { -- 1.7.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
[parent not found: <1339223164-9721-4-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]
* Re: [PATCH v5 3/5] CIFS: Move protocol specific negotiate code to ops struct [not found] ` <1339223164-9721-4-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> @ 2012-06-19 15:39 ` Jeff Layton 0 siblings, 0 replies; 17+ messages in thread From: Jeff Layton @ 2012-06-19 15:39 UTC (permalink / raw) To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA On Sat, 9 Jun 2012 10:26:02 +0400 Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: > Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> > --- > fs/cifs/cifsglob.h | 8 ++++++-- > fs/cifs/cifsproto.h | 5 ++--- > fs/cifs/cifssmb.c | 4 ++-- > fs/cifs/connect.c | 21 +++++++++------------ > fs/cifs/sess.c | 2 +- > fs/cifs/smb1ops.c | 23 +++++++++++++++++++++++ > 6 files changed, 43 insertions(+), 20 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 844b77c..669de88 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -192,6 +192,10 @@ struct smb_version_operations { > /* process transaction2 response */ > bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *, > char *, int); > + /* check if we need to negotiate */ > + bool (*need_neg)(struct TCP_Server_Info *); > + /* negotiate to the server */ > + int (*negotiate)(const int, struct cifs_ses *); > }; > > struct smb_version_values { > @@ -324,7 +328,7 @@ struct TCP_Server_Info { > struct mutex srv_mutex; > struct task_struct *tsk; > char server_GUID[16]; > - char sec_mode; > + __u16 sec_mode; > bool session_estab; /* mark when very first sess is established */ > u16 dialect; /* dialect index that server chose */ > enum securityEnum secType; > @@ -459,7 +463,7 @@ struct cifs_ses { > char *serverOS; /* name of operating system underlying server */ > char *serverNOS; /* name of network operating system of server */ > char *serverDomain; /* security realm of server */ > - int Suid; /* remote smb uid */ > + __u64 Suid; /* remote smb uid */ > uid_t linux_uid; /* overriding owner of files on the mount */ > uid_t cred_uid; /* owner of credentials */ > int capabilities; > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > index 8c1c7c1..32b569f 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -178,11 +178,10 @@ extern void cifs_dfs_release_automount_timer(void); > void cifs_proc_init(void); > void cifs_proc_clean(void); > > -extern int cifs_negotiate_protocol(unsigned int xid, > - struct cifs_ses *ses); > +extern int cifs_negotiate_protocol(const int xid, struct cifs_ses *ses); > extern int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, > struct nls_table *nls_info); > -extern int CIFSSMBNegotiate(unsigned int xid, struct cifs_ses *ses); > +extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses); > > extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses, > const char *tree, struct cifs_tcon *tcon, > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > index 58f1ab5..fbc5628 100644 > --- a/fs/cifs/cifssmb.c > +++ b/fs/cifs/cifssmb.c > @@ -372,7 +372,7 @@ static inline void inc_rfc1001_len(void *pSMB, int count) > } > > int > -CIFSSMBNegotiate(unsigned int xid, struct cifs_ses *ses) > +CIFSSMBNegotiate(const int xid, struct cifs_ses *ses) > { > NEGOTIATE_REQ *pSMB; > NEGOTIATE_RSP *pSMBr; > @@ -456,7 +456,7 @@ CIFSSMBNegotiate(unsigned int xid, struct cifs_ses *ses) > rc = -EOPNOTSUPP; > goto neg_err_exit; > } > - server->sec_mode = (__u8)le16_to_cpu(rsp->SecurityMode); > + server->sec_mode = le16_to_cpu(rsp->SecurityMode); > server->maxReq = min_t(unsigned int, > le16_to_cpu(rsp->MaxMpxCount), > cifs_max_pending); > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index ca0288d..4780342 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -406,7 +406,7 @@ cifs_echo_request(struct work_struct *work) > * done, which is indicated by maxBuf != 0. Also, no need to ping if > * we got a response recently > */ > - if (server->maxBuf == 0 || > + if (!server->ops->need_neg || server->ops->need_neg(server) || > time_before(jiffies, server->lstrp + SMB_ECHO_INTERVAL - HZ)) > goto requeue_echo; > > @@ -3942,24 +3942,22 @@ cifs_umount(struct cifs_sb_info *cifs_sb) > kfree(cifs_sb); > } > > -int cifs_negotiate_protocol(unsigned int xid, struct cifs_ses *ses) > +int > +cifs_negotiate_protocol(const int xid, struct cifs_ses *ses) > { > int rc = 0; > struct TCP_Server_Info *server = ses->server; > > + if (!server->ops->need_neg || !server->ops->negotiate) > + return -ENOSYS; > + > /* only send once per connect */ > - if (server->maxBuf != 0) > + if (!server->ops->need_neg(server)) > return 0; > > set_credits(server, 1); > - rc = CIFSSMBNegotiate(xid, ses); > - if (rc == -EAGAIN) { > - /* retry only once on 1st time connection */ > - set_credits(server, 1); > - rc = CIFSSMBNegotiate(xid, ses); > - if (rc == -EAGAIN) > - rc = -EHOSTDOWN; > - } > + > + rc = server->ops->negotiate(xid, ses); > if (rc == 0) { > spin_lock(&GlobalMid_Lock); > if (server->tcpStatus == CifsNeedNegotiate) > @@ -3967,7 +3965,6 @@ int cifs_negotiate_protocol(unsigned int xid, struct cifs_ses *ses) > else > rc = -EHOSTDOWN; > spin_unlock(&GlobalMid_Lock); > - > } > > return rc; > diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c > index 551d0c2..f88fa4d 100644 > --- a/fs/cifs/sess.c > +++ b/fs/cifs/sess.c > @@ -898,7 +898,7 @@ ssetup_ntlmssp_authenticate: > if (action & GUEST_LOGIN) > cFYI(1, "Guest login"); /* BB mark SesInfo struct? */ > ses->Suid = smb_buf->Uid; /* UID left in wire format (le) */ > - cFYI(1, "UID = %d ", ses->Suid); > + cFYI(1, "UID = %llu ", ses->Suid); > /* response can have either 3 or 4 word count - Samba sends 3 */ > /* and lanman response is 3 */ > bytes_remaining = get_bcc(smb_buf); > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > index f4f8394..5bfc26a 100644 > --- a/fs/cifs/smb1ops.c > +++ b/fs/cifs/smb1ops.c > @@ -389,6 +389,27 @@ cifs_check_trans2(struct mid_q_entry *mid, struct TCP_Server_Info *server, > return true; > } > > +static bool > +cifs_need_neg(struct TCP_Server_Info *server) > +{ > + return server->maxBuf == 0; > +} > + > +static int > +cifs_negotiate(const int xid, struct cifs_ses *ses) > +{ > + int rc; > + rc = CIFSSMBNegotiate(xid, ses); > + if (rc == -EAGAIN) { > + /* retry only once on 1st time connection */ > + set_credits(ses->server, 1); > + rc = CIFSSMBNegotiate(xid, ses); > + if (rc == -EAGAIN) > + rc = -EHOSTDOWN; > + } > + return rc; > +} > + > struct smb_version_operations smb1_operations = { > .send_cancel = send_nt_cancel, > .compare_fids = cifs_compare_fids, > @@ -407,6 +428,8 @@ struct smb_version_operations smb1_operations = { > .dump_detail = cifs_dump_detail, > .is_oplock_break = is_valid_oplock_break, > .check_trans2 = cifs_check_trans2, > + .need_neg = cifs_need_neg, > + .negotiate = cifs_negotiate, > }; > > struct smb_version_values smb1_values = { Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 4/5] CIFS: Move protocol specific session setup/logoff code to ops struct [not found] ` <1339223164-9721-1-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> ` (2 preceding siblings ...) 2012-06-09 6:26 ` [PATCH v5 3/5] CIFS: Move protocol specific negotiate code to ops struct Pavel Shilovsky @ 2012-06-09 6:26 ` Pavel Shilovsky [not found] ` <1339223164-9721-5-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> 2012-06-09 6:26 ` [PATCH v5 5/5] CIFS: Move protocol specific tcon/tdis " Pavel Shilovsky 4 siblings, 1 reply; 17+ messages in thread From: Pavel Shilovsky @ 2012-06-09 6:26 UTC (permalink / raw) To: linux-cifs-u79uwXL29TY76Z2rM5mHXA Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> --- fs/cifs/cifsglob.h | 5 +++++ fs/cifs/cifsproto.h | 8 ++++---- fs/cifs/connect.c | 16 +++++++++------- fs/cifs/sess.c | 2 +- fs/cifs/smb1ops.c | 2 ++ 5 files changed, 21 insertions(+), 12 deletions(-) diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 669de88..812f27c 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -196,6 +196,11 @@ struct smb_version_operations { bool (*need_neg)(struct TCP_Server_Info *); /* negotiate to the server */ int (*negotiate)(const int, struct cifs_ses *); + /* setup smb sessionn */ + int (*sess_setup)(const int, struct cifs_ses *, + const struct nls_table *); + /* close smb session */ + int (*logoff)(const int, struct cifs_ses *); }; struct smb_version_values { diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index 32b569f..8e6cd38 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -112,8 +112,8 @@ extern void header_assemble(struct smb_hdr *, char /* command */ , extern int small_smb_init_no_tc(const int smb_cmd, const int wct, struct cifs_ses *ses, void **request_buf); -extern int CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses, - const struct nls_table *nls_cp); +extern int CIFS_SessSetup(const int xid, struct cifs_ses *ses, + const struct nls_table *nls_cp); extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601); extern u64 cifs_UnixTimeToNT(struct timespec); extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, @@ -179,8 +179,8 @@ void cifs_proc_init(void); void cifs_proc_clean(void); extern int cifs_negotiate_protocol(const int xid, struct cifs_ses *ses); -extern int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, - struct nls_table *nls_info); +extern int cifs_setup_session(const int xid, struct cifs_ses *ses, + struct nls_table *nls_info); extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses); extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses, diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 4780342..9679352 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -2263,9 +2263,9 @@ cifs_put_smb_ses(struct cifs_ses *ses) list_del_init(&ses->smb_ses_list); spin_unlock(&cifs_tcp_ses_lock); - if (ses->status == CifsGood) { + if (ses->status == CifsGood && server->ops->logoff) { xid = GetXid(); - CIFSSMBLogoff(xid, ses); + server->ops->logoff(xid, ses); _FreeXid(xid); } sesInfoFree(ses); @@ -3970,11 +3970,11 @@ cifs_negotiate_protocol(const int xid, struct cifs_ses *ses) return rc; } - -int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, - struct nls_table *nls_info) +int +cifs_setup_session(const int xid, struct cifs_ses *ses, + struct nls_table *nls_info) { - int rc = 0; + int rc = -ENOSYS; struct TCP_Server_Info *server = ses->server; ses->flags = 0; @@ -3985,7 +3985,9 @@ int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, cFYI(1, "Security Mode: 0x%x Capabilities: 0x%x TimeAdjust: %d", server->sec_mode, server->capabilities, server->timeAdj); - rc = CIFS_SessSetup(xid, ses, nls_info); + if (server->ops->sess_setup) + rc = server->ops->sess_setup(xid, ses, nls_info); + if (rc) { cERROR(1, "Send error in SessSetup = %d", rc); } else { diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c index f88fa4d..16b2083 100644 --- a/fs/cifs/sess.c +++ b/fs/cifs/sess.c @@ -556,7 +556,7 @@ setup_ntlmv2_ret: } int -CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses, +CIFS_SessSetup(const int xid, struct cifs_ses *ses, const struct nls_table *nls_cp) { int rc = 0; diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c index 5bfc26a..ba3071f 100644 --- a/fs/cifs/smb1ops.c +++ b/fs/cifs/smb1ops.c @@ -430,6 +430,8 @@ struct smb_version_operations smb1_operations = { .check_trans2 = cifs_check_trans2, .need_neg = cifs_need_neg, .negotiate = cifs_negotiate, + .sess_setup = CIFS_SessSetup, + .logoff = CIFSSMBLogoff, }; struct smb_version_values smb1_values = { -- 1.7.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
[parent not found: <1339223164-9721-5-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]
* Re: [PATCH v5 4/5] CIFS: Move protocol specific session setup/logoff code to ops struct [not found] ` <1339223164-9721-5-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> @ 2012-06-19 15:51 ` Jeff Layton [not found] ` <20120619085159.6bac54fa-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org> 0 siblings, 1 reply; 17+ messages in thread From: Jeff Layton @ 2012-06-19 15:51 UTC (permalink / raw) To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA On Sat, 9 Jun 2012 10:26:03 +0400 Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: > Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> > --- > fs/cifs/cifsglob.h | 5 +++++ > fs/cifs/cifsproto.h | 8 ++++---- > fs/cifs/connect.c | 16 +++++++++------- > fs/cifs/sess.c | 2 +- > fs/cifs/smb1ops.c | 2 ++ > 5 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 669de88..812f27c 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -196,6 +196,11 @@ struct smb_version_operations { > bool (*need_neg)(struct TCP_Server_Info *); > /* negotiate to the server */ > int (*negotiate)(const int, struct cifs_ses *); > + /* setup smb sessionn */ > + int (*sess_setup)(const int, struct cifs_ses *, > + const struct nls_table *); > + /* close smb session */ > + int (*logoff)(const int, struct cifs_ses *); > }; > > struct smb_version_values { > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > index 32b569f..8e6cd38 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -112,8 +112,8 @@ extern void header_assemble(struct smb_hdr *, char /* command */ , > extern int small_smb_init_no_tc(const int smb_cmd, const int wct, > struct cifs_ses *ses, > void **request_buf); > -extern int CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses, > - const struct nls_table *nls_cp); > +extern int CIFS_SessSetup(const int xid, struct cifs_ses *ses, > + const struct nls_table *nls_cp); > extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601); > extern u64 cifs_UnixTimeToNT(struct timespec); > extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, > @@ -179,8 +179,8 @@ void cifs_proc_init(void); > void cifs_proc_clean(void); > > extern int cifs_negotiate_protocol(const int xid, struct cifs_ses *ses); > -extern int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, > - struct nls_table *nls_info); > +extern int cifs_setup_session(const int xid, struct cifs_ses *ses, > + struct nls_table *nls_info); > extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses); > > extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses, > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 4780342..9679352 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -2263,9 +2263,9 @@ cifs_put_smb_ses(struct cifs_ses *ses) > list_del_init(&ses->smb_ses_list); > spin_unlock(&cifs_tcp_ses_lock); > > - if (ses->status == CifsGood) { > + if (ses->status == CifsGood && server->ops->logoff) { > xid = GetXid(); > - CIFSSMBLogoff(xid, ses); > + server->ops->logoff(xid, ses); > _FreeXid(xid); > } > sesInfoFree(ses); > @@ -3970,11 +3970,11 @@ cifs_negotiate_protocol(const int xid, struct cifs_ses *ses) > return rc; > } > > - > -int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, > - struct nls_table *nls_info) > +int > +cifs_setup_session(const int xid, struct cifs_ses *ses, > + struct nls_table *nls_info) While I think this whole "xid" thing is a bogus idea, if we're going to do it, we should at least try to be consistent about the type. _GetXid returns unsigned int but you're turning that into a signed int here. I suggest that we don't do that. Let's declare henceforth that it's unsigned. Any place that treats it as signed is a bug. We probably have a lot of these bugs. I don't expect you to fix them all up, but you should at least try to fix that up in the code that you're touching in this series. We can get the rest later... Sound ok? > { > - int rc = 0; > + int rc = -ENOSYS; > struct TCP_Server_Info *server = ses->server; > > ses->flags = 0; > @@ -3985,7 +3985,9 @@ int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, > cFYI(1, "Security Mode: 0x%x Capabilities: 0x%x TimeAdjust: %d", > server->sec_mode, server->capabilities, server->timeAdj); > > - rc = CIFS_SessSetup(xid, ses, nls_info); > + if (server->ops->sess_setup) > + rc = server->ops->sess_setup(xid, ses, nls_info); > + > if (rc) { > cERROR(1, "Send error in SessSetup = %d", rc); > } else { > diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c > index f88fa4d..16b2083 100644 > --- a/fs/cifs/sess.c > +++ b/fs/cifs/sess.c > @@ -556,7 +556,7 @@ setup_ntlmv2_ret: > } > > int > -CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses, > +CIFS_SessSetup(const int xid, struct cifs_ses *ses, > const struct nls_table *nls_cp) > { > int rc = 0; > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > index 5bfc26a..ba3071f 100644 > --- a/fs/cifs/smb1ops.c > +++ b/fs/cifs/smb1ops.c > @@ -430,6 +430,8 @@ struct smb_version_operations smb1_operations = { > .check_trans2 = cifs_check_trans2, > .need_neg = cifs_need_neg, > .negotiate = cifs_negotiate, > + .sess_setup = CIFS_SessSetup, > + .logoff = CIFSSMBLogoff, > }; > > struct smb_version_values smb1_values = { -- Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <20120619085159.6bac54fa-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>]
* Re: [PATCH v5 4/5] CIFS: Move protocol specific session setup/logoff code to ops struct [not found] ` <20120619085159.6bac54fa-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org> @ 2012-06-19 19:21 ` Pavel Shilovsky [not found] ` <CAKywueSt2b5jYMNnnuGK+r_FPXifATBFfPGq9OeyycK8Y=sgGQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 17+ messages in thread From: Pavel Shilovsky @ 2012-06-19 19:21 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA 2012/6/19 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>: > On Sat, 9 Jun 2012 10:26:03 +0400 > Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: > >> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> >> --- >> fs/cifs/cifsglob.h | 5 +++++ >> fs/cifs/cifsproto.h | 8 ++++---- >> fs/cifs/connect.c | 16 +++++++++------- >> fs/cifs/sess.c | 2 +- >> fs/cifs/smb1ops.c | 2 ++ >> 5 files changed, 21 insertions(+), 12 deletions(-) >> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h >> index 669de88..812f27c 100644 >> --- a/fs/cifs/cifsglob.h >> +++ b/fs/cifs/cifsglob.h >> @@ -196,6 +196,11 @@ struct smb_version_operations { >> bool (*need_neg)(struct TCP_Server_Info *); >> /* negotiate to the server */ >> int (*negotiate)(const int, struct cifs_ses *); >> + /* setup smb sessionn */ >> + int (*sess_setup)(const int, struct cifs_ses *, >> + const struct nls_table *); >> + /* close smb session */ >> + int (*logoff)(const int, struct cifs_ses *); >> }; >> >> struct smb_version_values { >> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h >> index 32b569f..8e6cd38 100644 >> --- a/fs/cifs/cifsproto.h >> +++ b/fs/cifs/cifsproto.h >> @@ -112,8 +112,8 @@ extern void header_assemble(struct smb_hdr *, char /* command */ , >> extern int small_smb_init_no_tc(const int smb_cmd, const int wct, >> struct cifs_ses *ses, >> void **request_buf); >> -extern int CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses, >> - const struct nls_table *nls_cp); >> +extern int CIFS_SessSetup(const int xid, struct cifs_ses *ses, >> + const struct nls_table *nls_cp); >> extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601); >> extern u64 cifs_UnixTimeToNT(struct timespec); >> extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, >> @@ -179,8 +179,8 @@ void cifs_proc_init(void); >> void cifs_proc_clean(void); >> >> extern int cifs_negotiate_protocol(const int xid, struct cifs_ses *ses); >> -extern int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, >> - struct nls_table *nls_info); >> +extern int cifs_setup_session(const int xid, struct cifs_ses *ses, >> + struct nls_table *nls_info); >> extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses); >> >> extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses, >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c >> index 4780342..9679352 100644 >> --- a/fs/cifs/connect.c >> +++ b/fs/cifs/connect.c >> @@ -2263,9 +2263,9 @@ cifs_put_smb_ses(struct cifs_ses *ses) >> list_del_init(&ses->smb_ses_list); >> spin_unlock(&cifs_tcp_ses_lock); >> >> - if (ses->status == CifsGood) { >> + if (ses->status == CifsGood && server->ops->logoff) { >> xid = GetXid(); >> - CIFSSMBLogoff(xid, ses); >> + server->ops->logoff(xid, ses); >> _FreeXid(xid); >> } >> sesInfoFree(ses); >> @@ -3970,11 +3970,11 @@ cifs_negotiate_protocol(const int xid, struct cifs_ses *ses) >> return rc; >> } >> >> - >> -int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, >> - struct nls_table *nls_info) >> +int >> +cifs_setup_session(const int xid, struct cifs_ses *ses, >> + struct nls_table *nls_info) > Thanks for the review. > > While I think this whole "xid" thing is a bogus idea, if we're going to > do it, we should at least try to be consistent about the type. _GetXid > returns unsigned int but you're turning that into a signed int here. > > I suggest that we don't do that. Let's declare henceforth that it's > unsigned. Any place that treats it as signed is a bug. We probably have > a lot of these bugs. I don't expect you to fix them all up, but you > should at least try to fix that up in the code that you're touching in > this series. We can get the rest later... > > Sound ok? > cifs_get_smb_ses has int xid; and does xid = GetXid(); that returns 'int' - then it calls cifs_setup_session with this xid. It seems no problems with my patch here that converts cifs_setup_session to take an int. But GedXid() is 42 #define GetXid()>------->------->------->------->------->-------\ 43 ({>----->------->------->------->------->------->------->-------\ 44 >-------int __xid = (int)_GetXid();>---->------->------->-------\ 45 >-------cFYI(1, "CIFS VFS: in %s as Xid: %d with uid: %d",>-----\ 46 >------- __func__, __xid, current_fsuid());>>------->-------\ 47 >-------__xid;>->------->------->------->------->------->-------\ 48 }) so, it assigns _GetXid to 'int' and returns sign int value - this confuse me a little. Why don't we return unsigned int here? -- Best regards, Pavel Shilovsky. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CAKywueSt2b5jYMNnnuGK+r_FPXifATBFfPGq9OeyycK8Y=sgGQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH v5 4/5] CIFS: Move protocol specific session setup/logoff code to ops struct [not found] ` <CAKywueSt2b5jYMNnnuGK+r_FPXifATBFfPGq9OeyycK8Y=sgGQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2012-06-19 19:31 ` Steve French 0 siblings, 0 replies; 17+ messages in thread From: Steve French @ 2012-06-19 19:31 UTC (permalink / raw) To: Pavel Shilovsky; +Cc: Jeff Layton, linux-cifs-u79uwXL29TY76Z2rM5mHXA On Tue, Jun 19, 2012 at 12:21 PM, Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: > 2012/6/19 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>: >> On Sat, 9 Jun 2012 10:26:03 +0400 >> Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: >> >>> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> >>> --- >>> fs/cifs/cifsglob.h | 5 +++++ >>> fs/cifs/cifsproto.h | 8 ++++---- >>> fs/cifs/connect.c | 16 +++++++++------- >>> fs/cifs/sess.c | 2 +- >>> fs/cifs/smb1ops.c | 2 ++ >>> 5 files changed, 21 insertions(+), 12 deletions(-) >>> >>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h >>> index 669de88..812f27c 100644 >>> --- a/fs/cifs/cifsglob.h >>> +++ b/fs/cifs/cifsglob.h >>> @@ -196,6 +196,11 @@ struct smb_version_operations { >>> bool (*need_neg)(struct TCP_Server_Info *); >>> /* negotiate to the server */ >>> int (*negotiate)(const int, struct cifs_ses *); >>> + /* setup smb sessionn */ >>> + int (*sess_setup)(const int, struct cifs_ses *, >>> + const struct nls_table *); >>> + /* close smb session */ >>> + int (*logoff)(const int, struct cifs_ses *); >>> }; >>> >>> struct smb_version_values { >>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h >>> index 32b569f..8e6cd38 100644 >>> --- a/fs/cifs/cifsproto.h >>> +++ b/fs/cifs/cifsproto.h >>> @@ -112,8 +112,8 @@ extern void header_assemble(struct smb_hdr *, char /* command */ , >>> extern int small_smb_init_no_tc(const int smb_cmd, const int wct, >>> struct cifs_ses *ses, >>> void **request_buf); >>> -extern int CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses, >>> - const struct nls_table *nls_cp); >>> +extern int CIFS_SessSetup(const int xid, struct cifs_ses *ses, >>> + const struct nls_table *nls_cp); >>> extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601); >>> extern u64 cifs_UnixTimeToNT(struct timespec); >>> extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time, >>> @@ -179,8 +179,8 @@ void cifs_proc_init(void); >>> void cifs_proc_clean(void); >>> >>> extern int cifs_negotiate_protocol(const int xid, struct cifs_ses *ses); >>> -extern int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, >>> - struct nls_table *nls_info); >>> +extern int cifs_setup_session(const int xid, struct cifs_ses *ses, >>> + struct nls_table *nls_info); >>> extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses); >>> >>> extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses, >>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c >>> index 4780342..9679352 100644 >>> --- a/fs/cifs/connect.c >>> +++ b/fs/cifs/connect.c >>> @@ -2263,9 +2263,9 @@ cifs_put_smb_ses(struct cifs_ses *ses) >>> list_del_init(&ses->smb_ses_list); >>> spin_unlock(&cifs_tcp_ses_lock); >>> >>> - if (ses->status == CifsGood) { >>> + if (ses->status == CifsGood && server->ops->logoff) { >>> xid = GetXid(); >>> - CIFSSMBLogoff(xid, ses); >>> + server->ops->logoff(xid, ses); >>> _FreeXid(xid); >>> } >>> sesInfoFree(ses); >>> @@ -3970,11 +3970,11 @@ cifs_negotiate_protocol(const int xid, struct cifs_ses *ses) >>> return rc; >>> } >>> >>> - >>> -int cifs_setup_session(unsigned int xid, struct cifs_ses *ses, >>> - struct nls_table *nls_info) >>> +int >>> +cifs_setup_session(const int xid, struct cifs_ses *ses, >>> + struct nls_table *nls_info) >> > > Thanks for the review. > >> >> While I think this whole "xid" thing is a bogus idea, if we're going to >> do it, we should at least try to be consistent about the type. _GetXid >> returns unsigned int but you're turning that into a signed int here. >> >> I suggest that we don't do that. Let's declare henceforth that it's >> unsigned. Any place that treats it as signed is a bug. We probably have >> a lot of these bugs. I don't expect you to fix them all up, but you >> should at least try to fix that up in the code that you're touching in >> this series. We can get the rest later... >> >> Sound ok? >> > > cifs_get_smb_ses has > > int xid; > > and does > > xid = GetXid(); > > that returns 'int' - then it calls cifs_setup_session with this xid. > It seems no problems with my patch here that converts > cifs_setup_session to take an int. > > But GedXid() is > > 42 #define GetXid()>------->------->------->------->------->-------\ > 43 ({>----->------->------->------->------->------->------->-------\ > 44 >-------int __xid = (int)_GetXid();>---->------->------->-------\ > 45 >-------cFYI(1, "CIFS VFS: in %s as Xid: %d with uid: %d",>-----\ > 46 >------- __func__, __xid, current_fsuid());>>------->-------\ > 47 >-------__xid;>->------->------->------->------->------->-------\ > 48 }) > > so, it assigns _GetXid to 'int' and returns sign int value - this > confuse me a little. Why don't we return unsigned int here? uint to int change in GetXid probably was an incorrect fix for some warning (and probably an old change). Why not simply fix this to return unsigned int in GetXid to be consistent. As you noted _GetXid returns unsigned int (and FreeXid wants an unsigned int). If we do a global fix to change GetXid to be consistent - we can fix the name at the same time (to remove camel case) if camel case still bugs people. Of course - much of the GetXid and cFYI stuff could be removed if we had dynamic trace points and a howto on how to use them for cifs - someone started looking at this for various fs three or four years ago but don't think they finished. -- Thanks, Steve ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 5/5] CIFS: Move protocol specific tcon/tdis code to ops struct [not found] ` <1339223164-9721-1-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> ` (3 preceding siblings ...) 2012-06-09 6:26 ` [PATCH v5 4/5] CIFS: Move protocol specific session setup/logoff " Pavel Shilovsky @ 2012-06-09 6:26 ` Pavel Shilovsky [not found] ` <1339223164-9721-6-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> 4 siblings, 1 reply; 17+ messages in thread From: Pavel Shilovsky @ 2012-06-09 6:26 UTC (permalink / raw) To: linux-cifs-u79uwXL29TY76Z2rM5mHXA and rename variables around the code changes. Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> --- fs/cifs/cifsglob.h | 6 ++++ fs/cifs/cifsproto.h | 5 +-- fs/cifs/connect.c | 71 ++++++++++++++++++++++++++++++--------------------- fs/cifs/smb1ops.c | 2 + 4 files changed, 52 insertions(+), 32 deletions(-) diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 812f27c..1266e2e 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -160,6 +160,7 @@ struct mid_q_entry; struct TCP_Server_Info; struct cifsFileInfo; struct cifs_ses; +struct cifs_tcon; struct smb_version_operations { int (*send_cancel)(struct TCP_Server_Info *, void *, @@ -201,6 +202,11 @@ struct smb_version_operations { const struct nls_table *); /* close smb session */ int (*logoff)(const int, struct cifs_ses *); + /* connect to a server share */ + int (*tree_connect)(const int, struct cifs_ses *, const char *, + struct cifs_tcon *, const struct nls_table *); + /* close tree connecion */ + int (*tree_disconnect)(const int, struct cifs_tcon *); }; struct smb_version_values { diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index 8e6cd38..2f4f661 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -183,9 +183,8 @@ extern int cifs_setup_session(const int xid, struct cifs_ses *ses, struct nls_table *nls_info); extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses); -extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses, - const char *tree, struct cifs_tcon *tcon, - const struct nls_table *); +extern int CIFSTCon(const int xid, struct cifs_ses *ses, const char *tree, + struct cifs_tcon *tcon, const struct nls_table *); extern int CIFSFindFirst(const int xid, struct cifs_tcon *tcon, const char *searchName, const struct nls_table *nls_codepage, diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 9679352..d14bd45 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -2552,7 +2552,8 @@ cifs_put_tcon(struct cifs_tcon *tcon) spin_unlock(&cifs_tcp_ses_lock); xid = GetXid(); - CIFSSMBTDis(xid, tcon); + if (ses->server->ops->tree_disconnect) + ses->server->ops->tree_disconnect(xid, tcon); _FreeXid(xid); cifs_fscache_release_super_cookie(tcon); @@ -2577,6 +2578,11 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info) return tcon; } + if (!ses->server->ops->tree_connect) { + rc = -ENOSYS; + goto out_fail; + } + tcon = tconInfoAlloc(); if (tcon == NULL) { rc = -ENOMEM; @@ -2599,13 +2605,15 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info) goto out_fail; } - /* BB Do we need to wrap session_mutex around - * this TCon call and Unix SetFS as - * we do on SessSetup and reconnect? */ + /* + * BB Do we need to wrap session_mutex around this TCon call and Unix + * SetFS as we do on SessSetup and reconnect? + */ xid = GetXid(); - rc = CIFSTCon(xid, ses, volume_info->UNC, tcon, volume_info->local_nls); + rc = ses->server->ops->tree_connect(xid, ses, volume_info->UNC, tcon, + volume_info->local_nls); FreeXid(xid); - cFYI(1, "CIFS Tcon rc = %d", rc); + cFYI(1, "Tcon rc = %d", rc); if (rc) goto out_fail; @@ -2614,10 +2622,11 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info) cFYI(1, "DFS disabled (%d)", tcon->Flags); } tcon->seal = volume_info->seal; - /* we can have only one retry value for a connection - to a share so for resources mounted more than once - to the same server share the last value passed in - for the retry flag is used */ + /* + * We can have only one retry value for a connection to a share so for + * resources mounted more than once to the same server share the last + * value passed in for the retry flag is used. + */ tcon->retry = volume_info->retry; tcon->nocase = volume_info->nocase; tcon->local_lease = volume_info->local_lease; @@ -2751,37 +2760,41 @@ out: } int -get_dfs_path(int xid, struct cifs_ses *pSesInfo, const char *old_path, - const struct nls_table *nls_codepage, unsigned int *pnum_referrals, - struct dfs_info3_param **preferrals, int remap) +get_dfs_path(int xid, struct cifs_ses *ses, const char *old_path, + const struct nls_table *nls_codepage, unsigned int *num_referrals, + struct dfs_info3_param **referrals, int remap) { char *temp_unc; int rc = 0; - *pnum_referrals = 0; - *preferrals = NULL; + if (!ses->server->ops->tree_connect) + return -ENOSYS; + + *num_referrals = 0; + *referrals = NULL; - if (pSesInfo->ipc_tid == 0) { + if (ses->ipc_tid == 0) { temp_unc = kmalloc(2 /* for slashes */ + - strnlen(pSesInfo->serverName, - SERVER_NAME_LEN_WITH_NULL * 2) - + 1 + 4 /* slash IPC$ */ + 2, - GFP_KERNEL); + strnlen(ses->serverName, SERVER_NAME_LEN_WITH_NULL * 2) + + 1 + 4 /* slash IPC$ */ + 2, GFP_KERNEL); if (temp_unc == NULL) return -ENOMEM; temp_unc[0] = '\\'; temp_unc[1] = '\\'; - strcpy(temp_unc + 2, pSesInfo->serverName); - strcpy(temp_unc + 2 + strlen(pSesInfo->serverName), "\\IPC$"); - rc = CIFSTCon(xid, pSesInfo, temp_unc, NULL, nls_codepage); - cFYI(1, "CIFS Tcon rc = %d ipc_tid = %d", rc, pSesInfo->ipc_tid); + strcpy(temp_unc + 2, ses->serverName); + strcpy(temp_unc + 2 + strlen(ses->serverName), "\\IPC$"); + rc = ses->server->ops->tree_connect(xid, ses, temp_unc, NULL, + nls_codepage); + cFYI(1, "Tcon rc = %d ipc_tid = %d", rc, ses->ipc_tid); kfree(temp_unc); } if (rc == 0) - rc = CIFSGetDFSRefer(xid, pSesInfo, old_path, preferrals, - pnum_referrals, nls_codepage, remap); - /* BB map targetUNCs to dfs_info3 structures, here or - in CIFSGetDFSRefer BB */ + rc = CIFSGetDFSRefer(xid, ses, old_path, referrals, + num_referrals, nls_codepage, remap); + /* + * BB - map targetUNCs to dfs_info3 structures, here or in + * CIFSGetDFSRefer. + */ return rc; } @@ -3758,7 +3771,7 @@ out: * pointer may be NULL. */ int -CIFSTCon(unsigned int xid, struct cifs_ses *ses, +CIFSTCon(const int xid, struct cifs_ses *ses, const char *tree, struct cifs_tcon *tcon, const struct nls_table *nls_codepage) { diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c index ba3071f..1a3f08c 100644 --- a/fs/cifs/smb1ops.c +++ b/fs/cifs/smb1ops.c @@ -432,6 +432,8 @@ struct smb_version_operations smb1_operations = { .negotiate = cifs_negotiate, .sess_setup = CIFS_SessSetup, .logoff = CIFSSMBLogoff, + .tree_connect = CIFSTCon, + .tree_disconnect = CIFSSMBTDis, }; struct smb_version_values smb1_values = { -- 1.7.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
[parent not found: <1339223164-9721-6-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]
* Re: [PATCH v5 5/5] CIFS: Move protocol specific tcon/tdis code to ops struct [not found] ` <1339223164-9721-6-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> @ 2012-06-19 15:59 ` Jeff Layton [not found] ` <20120619085940.337536ed-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org> 0 siblings, 1 reply; 17+ messages in thread From: Jeff Layton @ 2012-06-19 15:59 UTC (permalink / raw) To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA On Sat, 9 Jun 2012 10:26:04 +0400 Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: > and rename variables around the code changes. > > Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> > --- > fs/cifs/cifsglob.h | 6 ++++ > fs/cifs/cifsproto.h | 5 +-- > fs/cifs/connect.c | 71 ++++++++++++++++++++++++++++++--------------------- > fs/cifs/smb1ops.c | 2 + > 4 files changed, 52 insertions(+), 32 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 812f27c..1266e2e 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -160,6 +160,7 @@ struct mid_q_entry; > struct TCP_Server_Info; > struct cifsFileInfo; > struct cifs_ses; > +struct cifs_tcon; > > struct smb_version_operations { > int (*send_cancel)(struct TCP_Server_Info *, void *, > @@ -201,6 +202,11 @@ struct smb_version_operations { > const struct nls_table *); > /* close smb session */ > int (*logoff)(const int, struct cifs_ses *); > + /* connect to a server share */ > + int (*tree_connect)(const int, struct cifs_ses *, const char *, > + struct cifs_tcon *, const struct nls_table *); > + /* close tree connecion */ > + int (*tree_disconnect)(const int, struct cifs_tcon *); > }; > > struct smb_version_values { > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > index 8e6cd38..2f4f661 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -183,9 +183,8 @@ extern int cifs_setup_session(const int xid, struct cifs_ses *ses, > struct nls_table *nls_info); > extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses); > > -extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses, > - const char *tree, struct cifs_tcon *tcon, > - const struct nls_table *); > +extern int CIFSTCon(const int xid, struct cifs_ses *ses, const char *tree, > + struct cifs_tcon *tcon, const struct nls_table *); > > extern int CIFSFindFirst(const int xid, struct cifs_tcon *tcon, > const char *searchName, const struct nls_table *nls_codepage, > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 9679352..d14bd45 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -2552,7 +2552,8 @@ cifs_put_tcon(struct cifs_tcon *tcon) > spin_unlock(&cifs_tcp_ses_lock); > > xid = GetXid(); > - CIFSSMBTDis(xid, tcon); > + if (ses->server->ops->tree_disconnect) > + ses->server->ops->tree_disconnect(xid, tcon); > _FreeXid(xid); > > cifs_fscache_release_super_cookie(tcon); > @@ -2577,6 +2578,11 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info) > return tcon; > } > > + if (!ses->server->ops->tree_connect) { > + rc = -ENOSYS; > + goto out_fail; > + } > + > tcon = tconInfoAlloc(); > if (tcon == NULL) { > rc = -ENOMEM; > @@ -2599,13 +2605,15 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info) > goto out_fail; > } > > - /* BB Do we need to wrap session_mutex around > - * this TCon call and Unix SetFS as > - * we do on SessSetup and reconnect? */ > + /* > + * BB Do we need to wrap session_mutex around this TCon call and Unix > + * SetFS as we do on SessSetup and reconnect? > + */ > xid = GetXid(); > - rc = CIFSTCon(xid, ses, volume_info->UNC, tcon, volume_info->local_nls); > + rc = ses->server->ops->tree_connect(xid, ses, volume_info->UNC, tcon, > + volume_info->local_nls); > FreeXid(xid); > - cFYI(1, "CIFS Tcon rc = %d", rc); > + cFYI(1, "Tcon rc = %d", rc); > if (rc) > goto out_fail; > > @@ -2614,10 +2622,11 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info) > cFYI(1, "DFS disabled (%d)", tcon->Flags); > } > tcon->seal = volume_info->seal; > - /* we can have only one retry value for a connection > - to a share so for resources mounted more than once > - to the same server share the last value passed in > - for the retry flag is used */ > + /* > + * We can have only one retry value for a connection to a share so for > + * resources mounted more than once to the same server share the last > + * value passed in for the retry flag is used. > + */ > tcon->retry = volume_info->retry; > tcon->nocase = volume_info->nocase; > tcon->local_lease = volume_info->local_lease; > @@ -2751,37 +2760,41 @@ out: > } > > int > -get_dfs_path(int xid, struct cifs_ses *pSesInfo, const char *old_path, > - const struct nls_table *nls_codepage, unsigned int *pnum_referrals, > - struct dfs_info3_param **preferrals, int remap) > +get_dfs_path(int xid, struct cifs_ses *ses, const char *old_path, > + const struct nls_table *nls_codepage, unsigned int *num_referrals, > + struct dfs_info3_param **referrals, int remap) > { > char *temp_unc; > int rc = 0; > > - *pnum_referrals = 0; > - *preferrals = NULL; > + if (!ses->server->ops->tree_connect) > + return -ENOSYS; > + > + *num_referrals = 0; > + *referrals = NULL; > > - if (pSesInfo->ipc_tid == 0) { > + if (ses->ipc_tid == 0) { > temp_unc = kmalloc(2 /* for slashes */ + > - strnlen(pSesInfo->serverName, > - SERVER_NAME_LEN_WITH_NULL * 2) > - + 1 + 4 /* slash IPC$ */ + 2, > - GFP_KERNEL); > + strnlen(ses->serverName, SERVER_NAME_LEN_WITH_NULL * 2) > + + 1 + 4 /* slash IPC$ */ + 2, GFP_KERNEL); > if (temp_unc == NULL) > return -ENOMEM; > temp_unc[0] = '\\'; > temp_unc[1] = '\\'; > - strcpy(temp_unc + 2, pSesInfo->serverName); > - strcpy(temp_unc + 2 + strlen(pSesInfo->serverName), "\\IPC$"); > - rc = CIFSTCon(xid, pSesInfo, temp_unc, NULL, nls_codepage); > - cFYI(1, "CIFS Tcon rc = %d ipc_tid = %d", rc, pSesInfo->ipc_tid); > + strcpy(temp_unc + 2, ses->serverName); > + strcpy(temp_unc + 2 + strlen(ses->serverName), "\\IPC$"); > + rc = ses->server->ops->tree_connect(xid, ses, temp_unc, NULL, > + nls_codepage); > + cFYI(1, "Tcon rc = %d ipc_tid = %d", rc, ses->ipc_tid); > kfree(temp_unc); > } > if (rc == 0) > - rc = CIFSGetDFSRefer(xid, pSesInfo, old_path, preferrals, > - pnum_referrals, nls_codepage, remap); > - /* BB map targetUNCs to dfs_info3 structures, here or > - in CIFSGetDFSRefer BB */ > + rc = CIFSGetDFSRefer(xid, ses, old_path, referrals, > + num_referrals, nls_codepage, remap); > + /* > + * BB - map targetUNCs to dfs_info3 structures, here or in > + * CIFSGetDFSRefer. > + */ > > return rc; > } > @@ -3758,7 +3771,7 @@ out: > * pointer may be NULL. > */ > int > -CIFSTCon(unsigned int xid, struct cifs_ses *ses, > +CIFSTCon(const int xid, struct cifs_ses *ses, > const char *tree, struct cifs_tcon *tcon, > const struct nls_table *nls_codepage) > { > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > index ba3071f..1a3f08c 100644 > --- a/fs/cifs/smb1ops.c > +++ b/fs/cifs/smb1ops.c > @@ -432,6 +432,8 @@ struct smb_version_operations smb1_operations = { > .negotiate = cifs_negotiate, > .sess_setup = CIFS_SessSetup, > .logoff = CIFSSMBLogoff, > + .tree_connect = CIFSTCon, > + .tree_disconnect = CIFSSMBTDis, > }; > > struct smb_version_values smb1_values = { Looks fine other than the type change on the xid. Note too, that if you wanted to just remove all of that xid stuff, I'd be even happier. Steve seems to like it for some reason though, so we'll need to convince him. IMO, it's just unnecessary clutter that gets in the way of the code clarity. It's also very unclear that FreeXid() has a side effect (a debug printk). A better scheme would be to turn all of those FreeXid calls into an explict cFYI that prints the name of the function and maybe the return code. The xid could be replaced with current->pid, which would eliminate GetXid. -- Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <20120619085940.337536ed-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>]
* Re: [PATCH v5 5/5] CIFS: Move protocol specific tcon/tdis code to ops struct [not found] ` <20120619085940.337536ed-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org> @ 2012-06-19 16:15 ` Steve French 0 siblings, 0 replies; 17+ messages in thread From: Steve French @ 2012-06-19 16:15 UTC (permalink / raw) To: Jeff Layton; +Cc: Pavel Shilovsky, linux-cifs-u79uwXL29TY76Z2rM5mHXA The main debugging benefit I get from the xid is simply being able to track in logs which operations I am looking at when running something like dbench with lots of processes in parallel or even when one process but log entries got lost. Pid isn't always as helpful since log entries get dropped - it is not always clear which call is failing when pid does multiple operations. Quick modifications of other log entries to add xid is pretty easy when the xid in a particular important cFYI or cERROR is not clear in a complex trace. On Tue, Jun 19, 2012 at 5:59 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > On Sat, 9 Jun 2012 10:26:04 +0400 > Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote: > >> and rename variables around the code changes. >> >> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> >> --- >> fs/cifs/cifsglob.h | 6 ++++ >> fs/cifs/cifsproto.h | 5 +-- >> fs/cifs/connect.c | 71 ++++++++++++++++++++++++++++++--------------------- >> fs/cifs/smb1ops.c | 2 + >> 4 files changed, 52 insertions(+), 32 deletions(-) >> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h >> index 812f27c..1266e2e 100644 >> --- a/fs/cifs/cifsglob.h >> +++ b/fs/cifs/cifsglob.h >> @@ -160,6 +160,7 @@ struct mid_q_entry; >> struct TCP_Server_Info; >> struct cifsFileInfo; >> struct cifs_ses; >> +struct cifs_tcon; >> >> struct smb_version_operations { >> int (*send_cancel)(struct TCP_Server_Info *, void *, >> @@ -201,6 +202,11 @@ struct smb_version_operations { >> const struct nls_table *); >> /* close smb session */ >> int (*logoff)(const int, struct cifs_ses *); >> + /* connect to a server share */ >> + int (*tree_connect)(const int, struct cifs_ses *, const char *, >> + struct cifs_tcon *, const struct nls_table *); >> + /* close tree connecion */ >> + int (*tree_disconnect)(const int, struct cifs_tcon *); >> }; >> >> struct smb_version_values { >> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h >> index 8e6cd38..2f4f661 100644 >> --- a/fs/cifs/cifsproto.h >> +++ b/fs/cifs/cifsproto.h >> @@ -183,9 +183,8 @@ extern int cifs_setup_session(const int xid, struct cifs_ses *ses, >> struct nls_table *nls_info); >> extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses); >> >> -extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses, >> - const char *tree, struct cifs_tcon *tcon, >> - const struct nls_table *); >> +extern int CIFSTCon(const int xid, struct cifs_ses *ses, const char *tree, >> + struct cifs_tcon *tcon, const struct nls_table *); >> >> extern int CIFSFindFirst(const int xid, struct cifs_tcon *tcon, >> const char *searchName, const struct nls_table *nls_codepage, >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c >> index 9679352..d14bd45 100644 >> --- a/fs/cifs/connect.c >> +++ b/fs/cifs/connect.c >> @@ -2552,7 +2552,8 @@ cifs_put_tcon(struct cifs_tcon *tcon) >> spin_unlock(&cifs_tcp_ses_lock); >> >> xid = GetXid(); >> - CIFSSMBTDis(xid, tcon); >> + if (ses->server->ops->tree_disconnect) >> + ses->server->ops->tree_disconnect(xid, tcon); >> _FreeXid(xid); >> >> cifs_fscache_release_super_cookie(tcon); >> @@ -2577,6 +2578,11 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info) >> return tcon; >> } >> >> + if (!ses->server->ops->tree_connect) { >> + rc = -ENOSYS; >> + goto out_fail; >> + } >> + >> tcon = tconInfoAlloc(); >> if (tcon == NULL) { >> rc = -ENOMEM; >> @@ -2599,13 +2605,15 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info) >> goto out_fail; >> } >> >> - /* BB Do we need to wrap session_mutex around >> - * this TCon call and Unix SetFS as >> - * we do on SessSetup and reconnect? */ >> + /* >> + * BB Do we need to wrap session_mutex around this TCon call and Unix >> + * SetFS as we do on SessSetup and reconnect? >> + */ >> xid = GetXid(); >> - rc = CIFSTCon(xid, ses, volume_info->UNC, tcon, volume_info->local_nls); >> + rc = ses->server->ops->tree_connect(xid, ses, volume_info->UNC, tcon, >> + volume_info->local_nls); >> FreeXid(xid); >> - cFYI(1, "CIFS Tcon rc = %d", rc); >> + cFYI(1, "Tcon rc = %d", rc); >> if (rc) >> goto out_fail; >> >> @@ -2614,10 +2622,11 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info) >> cFYI(1, "DFS disabled (%d)", tcon->Flags); >> } >> tcon->seal = volume_info->seal; >> - /* we can have only one retry value for a connection >> - to a share so for resources mounted more than once >> - to the same server share the last value passed in >> - for the retry flag is used */ >> + /* >> + * We can have only one retry value for a connection to a share so for >> + * resources mounted more than once to the same server share the last >> + * value passed in for the retry flag is used. >> + */ >> tcon->retry = volume_info->retry; >> tcon->nocase = volume_info->nocase; >> tcon->local_lease = volume_info->local_lease; >> @@ -2751,37 +2760,41 @@ out: >> } >> >> int >> -get_dfs_path(int xid, struct cifs_ses *pSesInfo, const char *old_path, >> - const struct nls_table *nls_codepage, unsigned int *pnum_referrals, >> - struct dfs_info3_param **preferrals, int remap) >> +get_dfs_path(int xid, struct cifs_ses *ses, const char *old_path, >> + const struct nls_table *nls_codepage, unsigned int *num_referrals, >> + struct dfs_info3_param **referrals, int remap) >> { >> char *temp_unc; >> int rc = 0; >> >> - *pnum_referrals = 0; >> - *preferrals = NULL; >> + if (!ses->server->ops->tree_connect) >> + return -ENOSYS; >> + >> + *num_referrals = 0; >> + *referrals = NULL; >> >> - if (pSesInfo->ipc_tid == 0) { >> + if (ses->ipc_tid == 0) { >> temp_unc = kmalloc(2 /* for slashes */ + >> - strnlen(pSesInfo->serverName, >> - SERVER_NAME_LEN_WITH_NULL * 2) >> - + 1 + 4 /* slash IPC$ */ + 2, >> - GFP_KERNEL); >> + strnlen(ses->serverName, SERVER_NAME_LEN_WITH_NULL * 2) >> + + 1 + 4 /* slash IPC$ */ + 2, GFP_KERNEL); >> if (temp_unc == NULL) >> return -ENOMEM; >> temp_unc[0] = '\\'; >> temp_unc[1] = '\\'; >> - strcpy(temp_unc + 2, pSesInfo->serverName); >> - strcpy(temp_unc + 2 + strlen(pSesInfo->serverName), "\\IPC$"); >> - rc = CIFSTCon(xid, pSesInfo, temp_unc, NULL, nls_codepage); >> - cFYI(1, "CIFS Tcon rc = %d ipc_tid = %d", rc, pSesInfo->ipc_tid); >> + strcpy(temp_unc + 2, ses->serverName); >> + strcpy(temp_unc + 2 + strlen(ses->serverName), "\\IPC$"); >> + rc = ses->server->ops->tree_connect(xid, ses, temp_unc, NULL, >> + nls_codepage); >> + cFYI(1, "Tcon rc = %d ipc_tid = %d", rc, ses->ipc_tid); >> kfree(temp_unc); >> } >> if (rc == 0) >> - rc = CIFSGetDFSRefer(xid, pSesInfo, old_path, preferrals, >> - pnum_referrals, nls_codepage, remap); >> - /* BB map targetUNCs to dfs_info3 structures, here or >> - in CIFSGetDFSRefer BB */ >> + rc = CIFSGetDFSRefer(xid, ses, old_path, referrals, >> + num_referrals, nls_codepage, remap); >> + /* >> + * BB - map targetUNCs to dfs_info3 structures, here or in >> + * CIFSGetDFSRefer. >> + */ >> >> return rc; >> } >> @@ -3758,7 +3771,7 @@ out: >> * pointer may be NULL. >> */ >> int >> -CIFSTCon(unsigned int xid, struct cifs_ses *ses, >> +CIFSTCon(const int xid, struct cifs_ses *ses, >> const char *tree, struct cifs_tcon *tcon, >> const struct nls_table *nls_codepage) >> { >> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c >> index ba3071f..1a3f08c 100644 >> --- a/fs/cifs/smb1ops.c >> +++ b/fs/cifs/smb1ops.c >> @@ -432,6 +432,8 @@ struct smb_version_operations smb1_operations = { >> .negotiate = cifs_negotiate, >> .sess_setup = CIFS_SessSetup, >> .logoff = CIFSSMBLogoff, >> + .tree_connect = CIFSTCon, >> + .tree_disconnect = CIFSSMBTDis, >> }; >> >> struct smb_version_values smb1_values = { > > Looks fine other than the type change on the xid. Note too, that if you > wanted to just remove all of that xid stuff, I'd be even happier. Steve > seems to like it for some reason though, so we'll need to convince him. > > IMO, it's just unnecessary clutter that gets in the way of the code > clarity. It's also very unclear that FreeXid() has a side effect (a > debug printk). > > A better scheme would be to turn all of those FreeXid calls into an > explict cFYI that prints the name of the function and maybe the return > code. The xid could be replaced with current->pid, which would > eliminate GetXid. > > -- > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Thanks, Steve ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2012-06-20 7:12 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-09 6:25 [PATCH v5 0/5] transport/session code move to ops struct Pavel Shilovsky
[not found] ` <1339223164-9721-1-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
2012-06-09 6:26 ` [PATCH v5 1/5] CIFS: Move trans2 processing " Pavel Shilovsky
2012-06-09 6:26 ` [PATCH v5 2/5] CIFS: Extend credit mechanism to process request type Pavel Shilovsky
[not found] ` <1339223164-9721-3-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
2012-06-19 15:31 ` Jeff Layton
[not found] ` <20120619083156.761f8a31-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2012-06-19 19:53 ` Pavel Shilovsky
[not found] ` <CAKywueQw8YaZ0=3Mv-UNZxur=dg_D59m352Msq+hZjsWEHqr1A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-06-19 22:00 ` Jeff Layton
[not found] ` <20120619150017.73546a39-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2012-06-19 23:57 ` Steve French
[not found] ` <CAH2r5muXdc708-HtceH02y8TBcoAXmi_qjyay0qHeZdtmoxVZw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-06-20 7:12 ` Pavel Shilovsky
2012-06-09 6:26 ` [PATCH v5 3/5] CIFS: Move protocol specific negotiate code to ops struct Pavel Shilovsky
[not found] ` <1339223164-9721-4-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
2012-06-19 15:39 ` Jeff Layton
2012-06-09 6:26 ` [PATCH v5 4/5] CIFS: Move protocol specific session setup/logoff " Pavel Shilovsky
[not found] ` <1339223164-9721-5-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
2012-06-19 15:51 ` Jeff Layton
[not found] ` <20120619085159.6bac54fa-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2012-06-19 19:21 ` Pavel Shilovsky
[not found] ` <CAKywueSt2b5jYMNnnuGK+r_FPXifATBFfPGq9OeyycK8Y=sgGQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-06-19 19:31 ` Steve French
2012-06-09 6:26 ` [PATCH v5 5/5] CIFS: Move protocol specific tcon/tdis " Pavel Shilovsky
[not found] ` <1339223164-9721-6-git-send-email-pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
2012-06-19 15:59 ` Jeff Layton
[not found] ` <20120619085940.337536ed-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2012-06-19 16:15 ` Steve French
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox