| Message ID | 20171102070312.18903-2-lsahlber@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: remove rfc1002 header from all smb2 requests | expand |
Ronnie Sahlberg <lsahlber@redhat.com> writes: > + > + new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), GFP_KERNEL); > + if (!new_iov) > + return -ENOMEM; > + > + /* 1st iov is an RFC1002 Session Message length */ > + memcpy(new_iov + 1, iov, (sizeof(struct kvec) * n_vec)); > + > + count = 0; > + for (i = 1; i < n_vec + 1; i++) > + count += new_iov[i].iov_len; > + > + rfc1002_marker = cpu_to_be32(count); > + > + new_iov[0].iov_base = &rfc1002_marker; > + new_iov[0].iov_len = 4; > + > + memset(&rqst, 0, sizeof(struct smb_rqst)); > + rqst.rq_iov = new_iov; > + rqst.rq_nvec = n_vec + 1; > + > + rc = cifs_send_recv(xid, ses, &rqst, resp_buf_type, flags, resp_iov); > + kfree(new_iov); > + return rc; > +} > + I know this is kind of unrelated but I've been thinking to myself we should try to get rid of this dynamic allocation at some point. IIUC the iovec never has more than a couple of elements, so we could have something like a fixed sized stack allocated iovec array + MAX_IOVEC_LENGTH macro. Doing a kmalloc() for every packet defeats the purpose of the memory pool optimization we use for small/large buffers.
On Thu, Nov 02, 2017 at 03:14:26PM +0100, Aurélien Aptel wrote: > I know this is kind of unrelated but I've been thinking to myself we > should try to get rid of this dynamic allocation at some point. > > IIUC the iovec never has more than a couple of elements, so we could > have something like a fixed sized stack allocated iovec array + > MAX_IOVEC_LENGTH macro. > > Doing a kmalloc() for every packet defeats the purpose of the memory > pool optimization we use for small/large buffers. Yes. You probably just want everyone to allocate an additional entry in the vector for the header if needed, and just leave the slot empty and start processing at offset for cases like SMB direct. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 02, 2017 at 06:02:54PM +1100, Ronnie Sahlberg wrote: > This function is similar to SendReceive2 except it does not expect > a 4 byte rfc1002 length header in the first io vector. Can you give it a proper name? cifs_ prefix, lower case with _ as the word delimiter. Also please explain the strategy for phasing out the old API(s) so that we won't have three around forever. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 3, 2017 at 4:27 PM, Christoph Hellwig <hch@infradead.org> wrote: > On Thu, Nov 02, 2017 at 06:02:54PM +1100, Ronnie Sahlberg wrote: >> This function is similar to SendReceive2 except it does not expect >> a 4 byte rfc1002 length header in the first io vector. > > Can you give it a proper name? cifs_ prefix, lower case with _ > as the word delimiter. Since this will now be the _only_ send/receive function SMB2 uses (except for the two async calls) I can rename it s/SendReceive3/SMB2SendReceive/ ? > > Also please explain the strategy for phasing out the old API(s) so > that we won't have three around forever. That is harder and I don't plan to do so. SendReceive2 and SendReceiveNoRsp is after this series no longer used by smb2, but they are still used by SMB1. SMB1 uses a LOT of different send/receive functions. I don't want to touch SMB1. I mostly want it to just go away :-) > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 03, 2017 at 04:37:59PM +1000, ronnie sahlberg wrote: > Since this will now be the _only_ send/receive function SMB2 uses > (except for the two async calls) > I can rename it s/SendReceive3/SMB2SendReceive/ ? smb2_send_recv, please. > > Also please explain the strategy for phasing out the old API(s) so > > that we won't have three around forever. > > That is harder and I don't plan to do so. > SendReceive2 and SendReceiveNoRsp is after this series no longer used by > smb2, but they are still used by SMB1. > SMB1 uses a LOT of different send/receive functions. > > > I don't want to touch SMB1. I mostly want it to just go away :-) Oh well. I guess it's just going to slowly bitrot then.. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index 4143c9dec463..a4e3c6c5aa64 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -106,6 +106,10 @@ extern int SendReceive2(const unsigned int /* xid */ , struct cifs_ses *, struct kvec *, int /* nvec to send */, int * /* type of buf returned */, const int flags, struct kvec * /* resp vec */); +extern int SendReceive3(const unsigned int /* xid */ , struct cifs_ses *, + struct kvec *, int /* nvec to send */, + int * /* type of buf returned */, const int flags, + struct kvec * /* resp vec */); extern int SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *ptcon, struct smb_hdr *in_buf , diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c index 7efbab013957..3f16adec3df5 100644 --- a/fs/cifs/transport.c +++ b/fs/cifs/transport.c @@ -827,6 +827,44 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses, return rc; } +/* Like SendReceive2 but iov[0] does not contain an rfc1002 header */ +int +SendReceive3(const unsigned int xid, struct cifs_ses *ses, + struct kvec *iov, int n_vec, int *resp_buf_type /* ret */, + const int flags, struct kvec *resp_iov) +{ + struct smb_rqst rqst; + struct kvec *new_iov; + int rc; + int i; + __u32 count; + __be32 rfc1002_marker; + + new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), GFP_KERNEL); + if (!new_iov) + return -ENOMEM; + + /* 1st iov is an RFC1002 Session Message length */ + memcpy(new_iov + 1, iov, (sizeof(struct kvec) * n_vec)); + + count = 0; + for (i = 1; i < n_vec + 1; i++) + count += new_iov[i].iov_len; + + rfc1002_marker = cpu_to_be32(count); + + new_iov[0].iov_base = &rfc1002_marker; + new_iov[0].iov_len = 4; + + memset(&rqst, 0, sizeof(struct smb_rqst)); + rqst.rq_iov = new_iov; + rqst.rq_nvec = n_vec + 1; + + rc = cifs_send_recv(xid, ses, &rqst, resp_buf_type, flags, resp_iov); + kfree(new_iov); + return rc; +} + int SendReceive(const unsigned int xid, struct cifs_ses *ses, struct smb_hdr *in_buf, struct smb_hdr *out_buf,
This function is similar to SendReceive2 except it does not expect a 4 byte rfc1002 length header in the first io vector. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/cifsproto.h | 4 ++++ fs/cifs/transport.c | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+)