On Fri, Mar 11, 2011 at 2:04 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Fri, 11 Mar 2011 14:59:17 -0500
> Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
>> On Fri, 25 Feb 2011 23:25:08 -0600
>> Steve French <smfrench@xxxxxxxxx> wrote:
>> +
>> > +/* one of these for every pending SMB2 request to the server */
>> > +struct smb2_mid_entry {
>> > + struct list_head qhead; /* mids waiting on reply from this server */
>> > + __u64 mid; /* multiplex id(s) */
>> > + __u16 pid; /* process id */
>> > + __u32 sequence_number; /* for signing */ /* BB check if needed */
>> > + 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
>> > + struct task_struct *tsk; /* task waiting for response */
>> > + struct smb2_hdr *resp_buf; /* response buffer */
>> > + char **pagebuf_list; /* response buffer */
>> > + int num_pages;
>> > + int mid_state; /* wish this were enum but can not pass to wait_event */
>> > + __le16 command; /* smb command code */
>> > + bool async_resp_rcvd:1; /* if server has responded with interim resp */
>> > + bool large_buf:1; /* if valid response, is pointer to large buf */
>> > + bool is_kmap_buf:1;
>> > +/* bool multi_rsp:1; BB do we have to account for something in SMB2 like
>> > + we saw multiple trans2 responses for one request (possible in CIFS) */
>> > + /* Async things */
>> > + __u64 *mid_list; /* multiplex id(s) */
>> > + int *mid_state_list;
>> > + short int *large_buf_list;
>> > + unsigned int num_mid;
>> > + unsigned int act_num_mid;
>> > + unsigned int num_received;
>> > + unsigned int cur_id;
>> > + struct smb2_hdr **resp_buf_list; /* response buffer */
>> > + __le16 *command_list;
>> > + bool async:1;
>> > + bool complex_mid:1; /* complex entry - consists of several messages */
>> > + int result;
>> > + unsigned long last_rsp_time;
>> > + int (*callback)(struct smb2_mid_entry * , void *);
>> > + void *callback_data;
>> > +};
>> > +
>> >
>>
>> Holy cow, this thing is a porker.
>>
>> Can we eliminate some of these fields? For instance, you have a
>> callback and callback_data field like in the cifs variant, but you also
>> have a tsk pointer. That should no longer be needed. There also seem to
>> be a lot of fields that are not used in any of the code that has yet
>> been merged. Much of it is really unclear.
>>
>> This seems like a real red flag to me. Dumping code into the tree like
>> this en-masse is a recipe for bloat and waste. Adding structs for
>> protocol definitions is one thing, adding things like this that seem to
>> have no rhyme or reason is quite another.
>>
>
> To be clear...
>
> It's impossible for anyone to reasonably review this. There's no
> context. Adding structures like this only makes sense if there is
> actual code to use them.
I sent you and a few others the mid allocation code (it is reasonably
close to what cifs does so should not be an issue). The struct itself
has fields which are different for the reasons I noted (the other few
fields were added during the Google Summer of Code async writepages
work that Pavel did last summer in the smb2.git tree).
--
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
