> -----Original Message----- > From: Tom Talpey > Sent: Monday, August 14, 2017 1:44 PM > To: Long Li <longli@xxxxxxxxxxxxx>; Steve French <sfrench@xxxxxxxxx>; > linux-cifs@xxxxxxxxxxxxxxx; samba-technical@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx > Subject: RE: [[PATCH v1] 18/37] [CIFS] SMBD: Implement API for upper layer > to send data > > > -----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. I have fixed that in v3. > > > + 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? I rearranged the shutdown logic in v3. Checking for transport status is still needed, but it checks after checking for other counters on pending activities. For example, on sending code: info->smbd_send_pending++; if (info->transport_status != SMBD_CONNECTED) { info->smbd_send_pending--; wake_up(&info->wait_smbd_send_pending); } On transport shutdown code: info->transport_status = SMBD_DISCONNECTING; ....... ....... ....... log_rdma_event(INFO, "wait for all send to finish\n"); wait_event(info->wait_smbd_send_pending, info->smbd_send_pending == 0); It guarantees no sending code can enter transport after shutdown is finished. Shutdown is running on a separate work queue, so it is needed. > > > + /* 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
