| Message ID | 20230623114425.2150536-12-dhowells@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,v4,01/15] tcp_bpf, smc, tls, espintcp, siw: Reduce MSG_SENDPAGE_NOTLAST usage | expand |
On 6/23/23 6:44 AM, David Howells wrote: > Use sendmsg() with MSG_SPLICE_PAGES rather than sendpage. This allows > multiple pages and multipage folios to be passed through. > > TODO: iscsit_fe_sendpage_sg() should perhaps set up a bio_vec array for the > entire set of pages it's going to transfer plus two for the header and > trailer and page fragments to hold the header and trailer - and then call > sendmsg once for the entire message. > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Lee Duncan <lduncan@suse.com> > cc: Chris Leech <cleech@redhat.com> > cc: Mike Christie <michael.christie@oracle.com> > cc: Maurizio Lombardi <mlombard@redhat.com> > cc: "James E.J. Bottomley" <jejb@linux.ibm.com> > cc: "Martin K. Petersen" <martin.petersen@oracle.com> > cc: "David S. Miller" <davem@davemloft.net> > cc: Eric Dumazet <edumazet@google.com> > cc: Jakub Kicinski <kuba@kernel.org> > cc: Paolo Abeni <pabeni@redhat.com> > cc: Jens Axboe <axboe@kernel.dk> > cc: Matthew Wilcox <willy@infradead.org> > cc: Al Viro <viro@zeniv.linux.org.uk> > cc: open-iscsi@googlegroups.com > cc: linux-scsi@vger.kernel.org > cc: target-devel@vger.kernel.org > cc: netdev@vger.kernel.org > --- > > Notes: > ver #2) > - Wrap lines at 80. > > drivers/scsi/iscsi_tcp.c | 26 +++++++++--------------- > drivers/scsi/iscsi_tcp.h | 2 -- > drivers/target/iscsi/iscsi_target_util.c | 15 ++++++++------ When you send this again can you split it into 2 patches? drivers/scsi/iscsi_tcp.* is one driver. It's similar to a NFS client and it has a set of maintainers that you saw in the MAINTAINER file. drivers/target/iscsi/iscsi_target_util.c is another driver which is similar to a NFS server. Martin is the overall maintainer but it's a group effort to review patches. I've tested and reviewed the iscsi_tcp parts. You can add my: Reviewed-by: Mike Christie <michael.christie@oracle.com> For the iscsi_target_util.c part, I'm reviewing it now and hoping Maurizio and/or Dimitry might review as well. One question on the target part I had is about the TODO above. Is that something you were going to do, or is it something you are asking the target people to do?
Mike Christie <michael.christie@oracle.com> wrote: > > One question on the target part I had is about the TODO above. Is that > something you were going to do, or is it something you are asking the target > people to do? I've got an in-progress patch for that, but it's not the simplest code to modify. I'm holding off on completing it till the simpler cleanup is in. I might end up having to push the incomplete patch your way to ask for advice on how to complete it. David
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index 9637d4bc2bc9..9ab8555180a3 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -301,35 +301,32 @@ static int iscsi_sw_tcp_xmit_segment(struct iscsi_tcp_conn *tcp_conn, while (!iscsi_tcp_segment_done(tcp_conn, segment, 0, r)) { struct scatterlist *sg; + struct msghdr msg = {}; + struct bio_vec bv; unsigned int offset, copy; - int flags = 0; r = 0; offset = segment->copied; copy = segment->size - offset; if (segment->total_copied + segment->size < segment->total_size) - flags |= MSG_MORE | MSG_SENDPAGE_NOTLAST; + msg.msg_flags |= MSG_MORE; if (tcp_sw_conn->queue_recv) - flags |= MSG_DONTWAIT; + msg.msg_flags |= MSG_DONTWAIT; - /* Use sendpage if we can; else fall back to sendmsg */ if (!segment->data) { + if (!tcp_conn->iscsi_conn->datadgst_en) + msg.msg_flags |= MSG_SPLICE_PAGES; sg = segment->sg; offset += segment->sg_offset + sg->offset; - r = tcp_sw_conn->sendpage(sk, sg_page(sg), offset, - copy, flags); + bvec_set_page(&bv, sg_page(sg), copy, offset); } else { - struct msghdr msg = { .msg_flags = flags }; - struct kvec iov = { - .iov_base = segment->data + offset, - .iov_len = copy - }; - - r = kernel_sendmsg(sk, &msg, &iov, 1, copy); + bvec_set_virt(&bv, segment->data + offset, copy); } + iov_iter_bvec(&msg.msg_iter, ITER_SOURCE, &bv, 1, copy); + r = sock_sendmsg(sk, &msg); if (r < 0) { iscsi_tcp_segment_unmap(segment); return r; @@ -746,7 +743,6 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session *cls_session, sock_no_linger(sk); iscsi_sw_tcp_conn_set_callbacks(conn); - tcp_sw_conn->sendpage = tcp_sw_conn->sock->ops->sendpage; /* * set receive state machine into initial state */ @@ -777,8 +773,6 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn, return -ENOTCONN; } iscsi_set_param(cls_conn, param, buf, buflen); - tcp_sw_conn->sendpage = conn->datadgst_en ? - sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage; mutex_unlock(&tcp_sw_conn->sock_lock); break; case ISCSI_PARAM_MAX_R2T: diff --git a/drivers/scsi/iscsi_tcp.h b/drivers/scsi/iscsi_tcp.h index 68e14a344904..89a6fc552f0b 100644 --- a/drivers/scsi/iscsi_tcp.h +++ b/drivers/scsi/iscsi_tcp.h @@ -47,8 +47,6 @@ struct iscsi_sw_tcp_conn { /* MIB custom statistics */ uint32_t sendpage_failures_cnt; uint32_t discontiguous_hdr_cnt; - - ssize_t (*sendpage)(struct socket *, struct page *, int, size_t, int); }; struct iscsi_sw_tcp_host { diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c index b14835fcb033..6231fa4ef5c6 100644 --- a/drivers/target/iscsi/iscsi_target_util.c +++ b/drivers/target/iscsi/iscsi_target_util.c @@ -1129,6 +1129,8 @@ int iscsit_fe_sendpage_sg( struct iscsit_conn *conn) { struct scatterlist *sg = cmd->first_data_sg; + struct bio_vec bvec; + struct msghdr msghdr = { .msg_flags = MSG_SPLICE_PAGES, }; struct kvec iov; u32 tx_hdr_size, data_len; u32 offset = cmd->first_data_sg_off; @@ -1172,17 +1174,18 @@ int iscsit_fe_sendpage_sg( u32 space = (sg->length - offset); u32 sub_len = min_t(u32, data_len, space); send_pg: - tx_sent = conn->sock->ops->sendpage(conn->sock, - sg_page(sg), sg->offset + offset, sub_len, 0); + bvec_set_page(&bvec, sg_page(sg), sub_len, sg->offset + offset); + iov_iter_bvec(&msghdr.msg_iter, ITER_SOURCE, &bvec, 1, sub_len); + + tx_sent = conn->sock->ops->sendmsg(conn->sock, &msghdr, + sub_len); if (tx_sent != sub_len) { if (tx_sent == -EAGAIN) { - pr_err("tcp_sendpage() returned" - " -EAGAIN\n"); + pr_err("sendmsg/splice returned -EAGAIN\n"); goto send_pg; } - pr_err("tcp_sendpage() failure: %d\n", - tx_sent); + pr_err("sendmsg/splice failure: %d\n", tx_sent); return -1; }
Use sendmsg() with MSG_SPLICE_PAGES rather than sendpage. This allows multiple pages and multipage folios to be passed through. TODO: iscsit_fe_sendpage_sg() should perhaps set up a bio_vec array for the entire set of pages it's going to transfer plus two for the header and trailer and page fragments to hold the header and trailer - and then call sendmsg once for the entire message. Signed-off-by: David Howells <dhowells@redhat.com> cc: Lee Duncan <lduncan@suse.com> cc: Chris Leech <cleech@redhat.com> cc: Mike Christie <michael.christie@oracle.com> cc: Maurizio Lombardi <mlombard@redhat.com> cc: "James E.J. Bottomley" <jejb@linux.ibm.com> cc: "Martin K. Petersen" <martin.petersen@oracle.com> cc: "David S. Miller" <davem@davemloft.net> cc: Eric Dumazet <edumazet@google.com> cc: Jakub Kicinski <kuba@kernel.org> cc: Paolo Abeni <pabeni@redhat.com> cc: Jens Axboe <axboe@kernel.dk> cc: Matthew Wilcox <willy@infradead.org> cc: Al Viro <viro@zeniv.linux.org.uk> cc: open-iscsi@googlegroups.com cc: linux-scsi@vger.kernel.org cc: target-devel@vger.kernel.org cc: netdev@vger.kernel.org --- Notes: ver #2) - Wrap lines at 80. drivers/scsi/iscsi_tcp.c | 26 +++++++++--------------- drivers/scsi/iscsi_tcp.h | 2 -- drivers/target/iscsi/iscsi_target_util.c | 15 ++++++++------ 3 files changed, 19 insertions(+), 24 deletions(-)