Re: mids and cifs sendrcv2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux