On Tue, Mar 20, 2012 at 5:28 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> 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...
Yes - probably right that this will change a few times as we figure
out what is cleaner.
--
Thanks,
Steve
--
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
