2011/4/15 Jeff Layton <jlayton@xxxxxxxxxx>: > On Fri, 15 Apr 2011 08:38:59 -0500 > Steve French <smfrench@xxxxxxxxx> wrote: > >> On Fri, Apr 15, 2011 at 5:45 AM, Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote: >> > 2011/4/8 Steve French <smfrench@xxxxxxxxx>: >> >> Update on cifs vs. smb2 mids, and the smb2 sendrcv2. Jeff had >> >> suggested more closely matching the cifs and smb2 mids, in particular >> >> extending the 16 bit cifs mid (multiplex identifier for inflight >> >> network requests) to the 64 bit size needed for smb2 (and thus masking >> >> the mid when used for cifs) and having cifs ignore the various smb2 >> >> unique fields in the mid (which makes the mid larger for cifs). >> >> Since the smb2 code in cifs-2.6.git (put in February and early March) >> >> has now been rereviewed, the next step in the smb2 merge is posting >> >> and reviewing the transport routine for smb2 (smb2_sendrcv2 or reusing >> >> cifs_sendrcv2) - the latter may make more sense if we go to a common >> >> mid for cifs and smb2. At the fs summit, Jeff and Jeremy and I >> >> talked about this, but Pavel and others may have opinions on this >> >> topic. As soon as the cifs merge activity settles down for 2.6.39, I >> >> plan to post sendrcv2 alternatives and then begin work with Pavel on >> >> the superblock, file and inode routines and seeing whether for smb2 >> >> they should be smb2 unique (as we originally expected since smb2 is >> >> handle based, and simpler) and look more like they did in the smb2.ko >> >> work that Pavel did last summer or should be more common with the cifs >> >> routines. >> >> >> >> -- >> >> 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 >> >> >> > >> > I suggest to make cifs and smb2 protocol mid structures use common >> > structure that has equals fields for both and then expand this >> > structure for protocol-dependent things. >> > >> > It can look like this: >> > >> > /* one of these for every pending CIFS request to the server */ >> > struct mid_q_entry { >> > __u8 protocol_id; >> > struct list_head qhead; /* mids waiting on reply from this server */ >> > int midState; /* wish this were enum but can not pass to wait_event */ >> > unsigned long when_alloc; /* when mid was created */ >> > #ifdef CONFIG_CIFS_STATS2 >> > unsigned long when_sent; /* time when smb send finished */ >> > unsigned long when_received; /* when demux complete (taken off wire) */ >> > #endif >> > bool largeBuf:1; /* if valid response, is pointer to large buf */ >> > void *resp_buf; /* response buffer */ >> > mid_callback_t *callback; /* call completion callback */ >> > void *callback_data; /* general purpose pointer for callback */ >> > }; >> > >> > struct cifs_mid_q_entry { >> > struct mid_q_entry mid_q; >> > __u16 mid; /* multiplex id */ >> > __u32 sequence_number; /* for CIFS signing */ >> > __u8 command; /* smb command code */ >> > __u16 pid; /* process id */ >> > bool multiRsp:1; /* multiple trans2 responses for one request */ >> > bool multiEnd:1; /* both received */ >> > }; >> > >> > struct smb2_mid_q_entry { >> > struct mid_q_entry mid_q; >> > __u64 mid; /* multiplex id(s), bigger for smb2 */ >> > __le16 command; /* smb2 command code */ >> > __u32 pid; /* process id - bigger for smb2 than cifs */ >> > }; >> >> I think nested mid structures (around a base of common mid fields) >> like the above is going to be the easiest way to handle the differences. >> >> I don't understand your PROTOCOL_ID #define though - we >> identify smb2 vs. cifs via a bool in the tcp server info struct, >> and presumably if we don't have access to the tcp server >> info struct we would have to add a field in the base mid >> (struct mid_q_entry) that indicates smb2 vs. cifs.. >> > > Why do even that? Why not a common mid struct that's simply a little larger? Sure it means slightly more memory consumption, but we never have that many in flight. Something like: > > struct mid_q_entry { > __u8 protocol_id; > struct list_head qhead; /* mids waiting on reply from this server */ > int midState; /* wish this were enum but can not pass to wait_event */ > unsigned long when_alloc; /* when mid was created */ > #ifdef CONFIG_CIFS_STATS2 > unsigned long when_sent; /* time when smb send finished */ > unsigned long when_received; /* when demux complete (taken off wire) */ > #endif > bool largeBuf:1; /* if valid response, is pointer to large buf */ > void *resp_buf; /* response buffer */ > mid_callback_t *callback; /* call completion callback */ > void *callback_data; /* general purpose pointer for callback */ > __u64 mid; /* multiplex id */ > __le16 command; /* command code */ > __u32 pid; /* process id */ > __u32 sequence_number; /* for CIFS signing */ > bool multiRsp:1; /* multiple trans2 responses for one request */ > bool multiEnd:1; /* both received */ > }; > > Using different structs here means that you need an entirely different > set of code to deal with them. IMO, the memory savings (if any) isn't > worth the duplicate code that'll be necessary. > When we need to work only with common field we can always assign it to a common mid_q_entry pointer and work with it. But when we need to work with protocol dependent fields we will do check like (in both variants - yours and mine) if (protocol == SMB2) smb2_func else cifs_func So, we don't need entirely different set of code. We will have common routines as well as cifs and smb2 specific ones. It seems that you suggest the same - I understand so. Please, sorry me, if I am mistaken. -- Best regards, Pavel Shilovsky. -- 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
