> -----Original Message----- > From: linux-cifs-owner@xxxxxxxxxxxxxxx [mailto:linux-cifs- > owner@xxxxxxxxxxxxxxx] On Behalf Of Long Li > Sent: Wednesday, August 2, 2017 4:10 PM > To: Steve French <sfrench@xxxxxxxxx>; linux-cifs@xxxxxxxxxxxxxxx; samba- > technical@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Cc: Long Li <longli@xxxxxxxxxxxxx> > Subject: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer to > send data > > +/* > + * Write data to transport > + * Each rqst is transported as a SMBDirect payload > + * rqst: the data to write > + * return value: 0 if successfully write, otherwise error code > + */ > +int cifs_rdma_write(struct cifs_rdma_info *info, struct smb_rqst *rqst) > +{ !!! This is a VERY confusing name. It is not sending an RDMA Write, which will confuse any RDMA-enlightened reader. It's performing an RDMA Send, so that name is perhaps one possibility. > + if (info->transport_status != CIFS_RDMA_CONNECTED) { > + log_cifs_write("disconnected returning -EIO\n"); > + return -EIO; > + } Isn't this optimizing the error case? There's no guarantee it's still connected by the time the following request construction occurs. Why not just proceed without the check? > + /* Strip the first 4 bytes MS-SMB2 section 2.1 > + * they are used only for TCP transport */ > + iov[0].iov_base = (char*)rqst->rq_iov[0].iov_base + 4; > + iov[0].iov_len = rqst->rq_iov[0].iov_len - 4; > + buflen += iov[0].iov_len; Ok, that layering choice in the cifs.ko client code needs to be corrected. After all, it will need to be RDMA-aware to build the SMB3 read/write channel structures. And, the code in cifs_post_send_data() is allocating and building a structure that could have been accounted for much earlier, avoiding the extra overhead. That change could happen later, the hack is mostly ok for now. But something needs to be said in a comment. Tom. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
