Messages in this thread |  | | From | Long Li <> | Subject | RE: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer to send data | Date | Sat, 19 Aug 2017 23:41:45 +0000 |
| |
> -----Original Message----- > From: Tom Talpey > Sent: Monday, August 14, 2017 1:44 PM > To: Long Li <longli@microsoft.com>; Steve French <sfrench@samba.org>; > linux-cifs@vger.kernel.org; samba-technical@lists.samba.org; linux- > kernel@vger.kernel.org; linux-rdma@vger.kernel.org > Subject: RE: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer > to send data > > > -----Original Message----- > > From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs- > > owner@vger.kernel.org] On Behalf Of Long Li > > Sent: Wednesday, August 2, 2017 4:10 PM > > To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; > > samba- technical@lists.samba.org; linux-kernel@vger.kernel.org > > Cc: Long Li <longli@microsoft.com> > > 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.
Will address those in v2.
> > Tom.
|  |