On Tue, 20 Mar 2012 11:37:27 +0400
Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> 20 марта 2012 г. 0:48 пользователь Steve French <smfrench@xxxxxxxxx> написал:
> > On Mon, Mar 19, 2012 at 3:24 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >> On Fri, 16 Mar 2012 18:09:33 +0300
> >> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> >>
> >>> While in CIFS/SMB we have 16 bit mid, in SMB2 it is 64 bit.
> >>> Convert the existing field to 64 bit and mask off higher bits
> >>> for CIFS/SMB.
> >>>
> >>> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
> >>> ---
> >>> fs/cifs/cifsglob.h | 2 +-
> >>> fs/cifs/cifsproto.h | 2 +-
> >>> fs/cifs/misc.c | 84 ++++++++++++++++++++++++++++-----------------------
> >>> 3 files changed, 48 insertions(+), 40 deletions(-)
> >>>
> >>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >>> index a403398..b213458 100644
> >>> --- a/fs/cifs/cifsglob.h
> >>> +++ b/fs/cifs/cifsglob.h
> >>> @@ -282,7 +282,7 @@ struct TCP_Server_Info {
> >>> vcnumbers */
> >>> int capabilities; /* allow selective disabling of caps by smb sess */
> >>> int timeAdj; /* Adjust for difference in server time zone in sec */
> >>> - __u16 CurrentMid; /* multiplex id - rotating counter */
> >>> + __u64 CurrentMid; /* multiplex id - rotating counter */
> >>
> >> It occurs to me that a simpler way to do this might be to turn this
> >> into a union with a u16 and u64 field. This works just as well though...
> >>
> >>> char cryptkey[CIFS_CRYPTO_KEY_SIZE]; /* used by ntlm, ntlmv2 etc */
> >>> /* 16th byte of RFC1001 workstation name is always null */
> >>> char workstation_RFC1001_name[RFC1001_NAME_LEN_WITH_NULL];
> >>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> >>> index db38a40..8958721 100644
> >>> --- a/fs/cifs/cifsproto.h
> >>> +++ b/fs/cifs/cifsproto.h
> >>> @@ -115,7 +115,7 @@ extern int small_smb_init_no_tc(const int smb_cmd, const int wct,
> >>> void **request_buf);
> >>> extern int CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses,
> >>> const struct nls_table *nls_cp);
> >>> -extern __u16 GetNextMid(struct TCP_Server_Info *server);
> >>> +extern __u64 GetNextMid(struct TCP_Server_Info *server);
> >>> 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,
> >>> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> >>> index 88459d0..0b743b7 100644
> >>> --- a/fs/cifs/misc.c
> >>> +++ b/fs/cifs/misc.c
> >>> @@ -213,54 +213,61 @@ cifs_small_buf_release(void *buf_to_free)
> >>> }
> >>>
> >>> /*
> >>> - Find a free multiplex id (SMB mid). Otherwise there could be
> >>> - mid collisions which might cause problems, demultiplexing the
> >>> - wrong response to this request. Multiplex ids could collide if
> >>> - one of a series requests takes much longer than the others, or
> >>> - if a very large number of long lived requests (byte range
> >>> - locks or FindNotify requests) are pending. No more than
> >>> - 64K-1 requests can be outstanding at one time. If no
> >>> - mids are available, return zero. A future optimization
> >>> - could make the combination of mids and uid the key we use
> >>> - to demultiplex on (rather than mid alone).
> >>> - In addition to the above check, the cifs demultiplex
> >>> - code already used the command code as a secondary
> >>> - check of the frame and if signing is negotiated the
> >>> - response would be discarded if the mid were the same
> >>> - but the signature was wrong. Since the mid is not put in the
> >>> - pending queue until later (when it is about to be dispatched)
> >>> - we do have to limit the number of outstanding requests
> >>> - to somewhat less than 64K-1 although it is hard to imagine
> >>> - so many threads being in the vfs at one time.
> >>> -*/
> >>> -__u16 GetNextMid(struct TCP_Server_Info *server)
> >>> + * Find a free multiplex id (SMB mid). Otherwise there could be
> >>> + * mid collisions which might cause problems, demultiplexing the
> >>> + * wrong response to this request. Multiplex ids could collide if
> >>> + * one of a series requests takes much longer than the others, or
> >>> + * if a very large number of long lived requests (byte range
> >>> + * locks or FindNotify requests) are pending. No more than
> >>> + * 64K-1 requests can be outstanding at one time. If no
> >>> + * mids are available, return zero. A future optimization
> >>> + * could make the combination of mids and uid the key we use
> >>> + * to demultiplex on (rather than mid alone).
> >>> + * In addition to the above check, the cifs demultiplex
> >>> + * code already used the command code as a secondary
> >>> + * check of the frame and if signing is negotiated the
> >>> + * response would be discarded if the mid were the same
> >>> + * but the signature was wrong. Since the mid is not put in the
> >>> + * pending queue until later (when it is about to be dispatched)
> >>> + * we do have to limit the number of outstanding requests
> >>> + * to somewhat less than 64K-1 although it is hard to imagine
> >>> + * so many threads being in the vfs at one time.
> >>> + */
> >>> +__u64 GetNextMid(struct TCP_Server_Info *server)
> >>> {
> >>> - __u16 mid = 0;
> >>> - __u16 last_mid;
> >>> + __u64 mid = 0;
> >>> + __u16 last_mid, cur_mid;
> >>> bool collision;
> >>>
> >>> spin_lock(&GlobalMid_Lock);
> >>> - last_mid = server->CurrentMid; /* we do not want to loop forever */
> >>> - server->CurrentMid++;
> >>> - /* This nested loop looks more expensive than it is.
> >>> - In practice the list of pending requests is short,
> >>> - fewer than 50, and the mids are likely to be unique
> >>> - on the first pass through the loop unless some request
> >>> - takes longer than the 64 thousand requests before it
> >>> - (and it would also have to have been a request that
> >>> - did not time out) */
> >>> - while (server->CurrentMid != last_mid) {
> >>> +
> >>> + /* mid is 16 bit only for CIFS/SMB */
> >>> + cur_mid = (__u16)((server->CurrentMid) & 0xffff);
> >>> + /* we do not want to loop forever */
> >>> + last_mid = cur_mid;
> >>> + cur_mid++;
> >>> +
> >>> + /*
> >>> + * This nested loop looks more expensive than it is.
> >>> + * In practice the list of pending requests is short,
> >>> + * fewer than 50, and the mids are likely to be unique
> >>> + * on the first pass through the loop unless some request
> >>> + * takes longer than the 64 thousand requests before it
> >>> + * (and it would also have to have been a request that
> >>> + * did not time out).
> >>> + */
> >>> + while (cur_mid != last_mid) {
> >>> struct mid_q_entry *mid_entry;
> >>> unsigned int num_mids;
> >>>
> >>> collision = false;
> >>> - if (server->CurrentMid == 0)
> >>> - server->CurrentMid++;
> >>> + if (cur_mid == 0)
> >>> + cur_mid++;
> >>>
> >>> num_mids = 0;
> >>> list_for_each_entry(mid_entry, &server->pending_mid_q, qhead) {
> >>> ++num_mids;
> >>> - if (mid_entry->mid == server->CurrentMid &&
> >>> + if (mid_entry->mid == cur_mid &&
> >>> mid_entry->midState == MID_REQUEST_SUBMITTED) {
> >>> /* This mid is in use, try a different one */
> >>> collision = true;
> >>> @@ -282,10 +289,11 @@ __u16 GetNextMid(struct TCP_Server_Info *server)
> >>> server->tcpStatus = CifsNeedReconnect;
> >>>
> >>> if (!collision) {
> >>> - mid = server->CurrentMid;
> >>> + mid = (__u64)cur_mid;
> >>> + server->CurrentMid = mid;
> >>> break;
> >>> }
> >>> - server->CurrentMid++;
> >>> + cur_mid++;
> >>> }
> >>> spin_unlock(&GlobalMid_Lock);
> >>> return mid;
> >>
> >> Not directly related to this patch, but should we move all of these mid
> >> operations under the req_lock instead of the GlobalMid_Lock? The global
> >> spinlock is a bottleneck and all of the structures involved should be
> >> per-server anyway.
> >>
> >> Anyway, I think this looks ok
> >>
> >> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
> >
> > The idea of a union for these two key fields is worth thinking about
> > more - any more opinions from others on this?
> >
>
> I think our main goal is to make to code is cleaner as possible. If we
> change this to a union we can't use the same codepath for both
> protocols - more protocol specific code - harder to understand, fix
> bugs, etc.
>
>
You're going to need some bit of separate code in order to deal with
the differences in field width in things like the MID. The question is
how to abstract that code out. It's probably cleaner to do so with an
ops struct of some sort rather than sprinkling "if (server->is_smb2)"
all over the code.
Once you design the code around that sort of idea, then using a union
may be cleaner, but you'll have to make that judgment as you proceed...
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
