Message ID | 20230125214543.2337639-9-dhowells@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp500846wrn; Wed, 25 Jan 2023 13:49:49 -0800 (PST) X-Google-Smtp-Source: AMrXdXvLhjstnoKASiI0iZmJ7FRnqstgJikEWx3AhRTYgv4BwRDP/E1K1+4KbYTA2nKwMyGZqrlI X-Received: by 2002:a05:6a21:2d8e:b0:b8:4cc9:16d9 with SMTP id ty14-20020a056a212d8e00b000b84cc916d9mr36348167pzb.46.1674683389297; Wed, 25 Jan 2023 13:49:49 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1674683389; cv=none; d=google.com; s=arc-20160816; b=QWZZGvlz2msMkkuLGrAZV9z2egqVYP5A0+kXFuROecHZXL+O5pWleXOV7NVBfHiq0E vhd3BwO9IF1awhdz44CnLLw5u4ubnzx8DCs6lRK9J0XTkpS08QNc5r+vbl9U44ePk21s XMREbT7IWpaw3ZXKQizML+/7NWOIGkjHh/6pyT2+twmoLq/qjyO1L5Dt/jlFsr0bvVLz KqLDMFFuJXaKPJMjSQYaPRt2Id2uw92igs84gnXGkg30cIPAaNODKLVEjntmz10ioMXw UVYsy6d/L7A2OyA00+ToBNK+Sds1/BoRbPQ08Mb4SBIeGyQKf7/PR94Fv7/OSShK17iO yTCw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=pR9HJOLLxdNpGESJrnGvB1rp48wDlAuqgCCdDVT8evM=; b=tlBVDzANc8HZ6l5DHEB+2+2ECwH12htbR5ihBwVRh5ZEILOFf7d8D9UJ8Qa32NbWjN MqhE8wHCUwCp6Sg6xQIfdFBdaVyrC4AgT6zOLFKztojm05B6K3Gy7tVOKNZvTmKcvEYA fQhzvnHzJTlgB8JqbDYa2flSv/cg2RMDjG445i7LdMbS7IRx2kUBU1/HCaRz4SyAY0We Me8Qv7r3PifpFfCKV2hnYYD05OXnyfUSNviF2U9QtB8Y93cPvkaagWc55DfC6Mbc+oaU HRu6lJzfRBdkj7KhvlRSnYXFfzIww1TgehUWJudjwgevJlngQ+zfG4k8ueybY8jyNYYJ HgFg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=J9kqRttd; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id e22-20020a637456000000b004822db58bd4si671067pgn.282.2023.01.25.13.49.37; Wed, 25 Jan 2023 13:49:49 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=J9kqRttd; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236397AbjAYVrb (ORCPT <rfc822;lekhanya01809@gmail.com> + 99 others); Wed, 25 Jan 2023 16:47:31 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40242 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235791AbjAYVra (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 25 Jan 2023 16:47:30 -0500 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4AF165867A for <linux-kernel@vger.kernel.org>; Wed, 25 Jan 2023 13:46:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1674683170; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=pR9HJOLLxdNpGESJrnGvB1rp48wDlAuqgCCdDVT8evM=; b=J9kqRttdZUyEhhdYAqbTJi/bFr7d3lU3HkNCOePs0WRpkvRXkGFJhr5QpeONQ8Te5U297B tYJ+WR/IeXu43yvQOl7Cx/2jAMO79VJVLBV26QAllAlnJaxR2SV+gLNKinVAQv3e2GE7i/ 8apEuiZEw7LPQt1td62Gma7hmAMjbjI= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-353-6N70a4U5Mj-_qoeHx5nMow-1; Wed, 25 Jan 2023 16:46:09 -0500 X-MC-Unique: 6N70a4U5Mj-_qoeHx5nMow-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 33A653C01DE1; Wed, 25 Jan 2023 21:46:08 +0000 (UTC) Received: from warthog.procyon.org.uk.com (unknown [10.33.36.97]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6DAB41121330; Wed, 25 Jan 2023 21:46:06 +0000 (UTC) From: David Howells <dhowells@redhat.com> To: Steve French <smfrench@gmail.com> Cc: David Howells <dhowells@redhat.com>, Al Viro <viro@zeniv.linux.org.uk>, Shyam Prasad N <nspmangalore@gmail.com>, Rohith Surabattula <rohiths.msft@gmail.com>, Tom Talpey <tom@talpey.com>, Stefan Metzmacher <metze@samba.org>, Christoph Hellwig <hch@infradead.org>, Matthew Wilcox <willy@infradead.org>, Jeff Layton <jlayton@kernel.org>, linux-cifs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Steve French <sfrench@samba.org> Subject: [RFC 08/13] cifs: Add a function to read into an iter from a socket Date: Wed, 25 Jan 2023 21:45:38 +0000 Message-Id: <20230125214543.2337639-9-dhowells@redhat.com> In-Reply-To: <20230125214543.2337639-1-dhowells@redhat.com> References: <20230125214543.2337639-1-dhowells@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1756032809363347379?= X-GMAIL-MSGID: =?utf-8?q?1756032809363347379?= |
Series |
smb3: Use iov_iters down to the network transport and fix DIO page pinning
|
|
Commit Message
David Howells
Jan. 25, 2023, 9:45 p.m. UTC
Add a helper function to read data from a socket into the given iterator.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Steve French <sfrench@samba.org>
cc: Shyam Prasad N <nspmangalore@gmail.com>
cc: Rohith Surabattula <rohiths.msft@gmail.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-cifs@vger.kernel.org
Link: https://lore.kernel.org/r/164928617874.457102.10021662143234315566.stgit@warthog.procyon.org.uk/ # v1
Link: https://lore.kernel.org/r/165211419563.3154751.18431990381145195050.stgit@warthog.procyon.org.uk/ # v1
Link: https://lore.kernel.org/r/165348879662.2106726.16881134187242702351.stgit@warthog.procyon.org.uk/ # v1
Link: https://lore.kernel.org/r/165364826398.3334034.12541600783145647319.stgit@warthog.procyon.org.uk/ # v3
Link: https://lore.kernel.org/r/166126395495.708021.12328677373159554478.stgit@warthog.procyon.org.uk/ # v1
Link: https://lore.kernel.org/r/166697258876.61150.3530237818849429372.stgit@warthog.procyon.org.uk/ # rfc
Link: https://lore.kernel.org/r/166732031039.3186319.10691316510079412635.stgit@warthog.procyon.org.uk/ # rfc
---
fs/cifs/cifsproto.h | 3 +++
fs/cifs/connect.c | 16 ++++++++++++++++
2 files changed, 19 insertions(+)
Comments
From: David Howells > Sent: 25 January 2023 21:46 > Add a helper function to read data from a socket into the given iterator. ... > +int > +cifs_read_iter_from_socket(struct TCP_Server_Info *server, struct iov_iter *iter, > + unsigned int to_read) > +{ > + struct msghdr smb_msg; > + int ret; > + > + smb_msg.msg_iter = *iter; > + if (smb_msg.msg_iter.count > to_read) > + smb_msg.msg_iter.count = to_read; > + ret = cifs_readv_from_socket(server, &smb_msg); > + if (ret > 0) > + iov_iter_advance(iter, ret); > + return ret; > +} On the face of it that passes a largely uninitialised 'struct msghdr' to cifs_readv_from_socket() in order to pass an iov_iter. That seems to be asking for trouble. I'm also not 100% sure that taking a copy of an iov_iter is a good idea. If cifs_readv_from_socket() only needs the iov_iter then wouldn't it be better to do the wrapper the other way around? (Probably as an inline function) Something like: int cifs_readv_from_socket(struct TCP_Server_Info *server, struct msghdr *smb_msg) { return cifs_read_iter_from_socket(server, &smb_msg->msg_iter, smb_msg->msg_iter.count); } and then changing cifs_readv_from_socket() to just use the iov_iter. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
David Laight <David.Laight@ACULAB.COM> wrote: > On the face of it that passes a largely uninitialised 'struct msghdr' > to cifs_readv_from_socket() in order to pass an iov_iter. > That seems to be asking for trouble. > > If cifs_readv_from_socket() only needs the iov_iter then wouldn't > it be better to do the wrapper the other way around? > (Probably as an inline function) > Something like: > > int > cifs_readv_from_socket(struct TCP_Server_Info *server, struct msghdr *smb_msg) > { > return cifs_read_iter_from_socket(server, &smb_msg->msg_iter, smb_msg->msg_iter.count); > } > > and then changing cifs_readv_from_socket() to just use the iov_iter. Yeah. And smbd_recv() only cares about the iterator too. > I'm also not 100% sure that taking a copy of an iov_iter is a good idea. It shouldn't matter as the only problematic iterator is ITER_PIPE (advancing that has side effects) - and splice_read is handled specially by patch 4. The problem with splice_read with the way cifs works is that it likes to subdivide its read/write requests across multiple reqs and then subsubdivide them if certain types of failure occur. But you can't do that with ITER_PIPE. I build an ITER_BVEC from ITER_PIPE, ITER_UBUF and ITER_IOVEC in the top levels with pins inserted as appropriate and hand the ITER_BVEC down. For user-backed iterators it has to be done this way because the I/O may get shuffled off to a different thread. Reqs can then just copy the BVEC/XARRAY/KVEC and narrow the region because the master request at the top does holds the vector list and the top cifs level or the caller above the vfs (eg. sys_execve) does what is necessary to retain the pages. David
From: David Howells > Sent: 26 January 2023 15:44 ... > > I'm also not 100% sure that taking a copy of an iov_iter is a good idea. > > It shouldn't matter as the only problematic iterator is ITER_PIPE (advancing > that has side effects) - and splice_read is handled specially by patch 4. The > problem with splice_read with the way cifs works is that it likes to subdivide > its read/write requests across multiple reqs and then subsubdivide them if > certain types of failure occur. But you can't do that with ITER_PIPE. I was thinking that even if ok at the moment it might be troublesome later. Somewhere I started writing a patch to put the iov_cache[] for user requests into the same structure as the iterator. Copying those might cause oddities. > I build an ITER_BVEC from ITER_PIPE, ITER_UBUF and ITER_IOVEC in the top > levels with pins inserted as appropriate and hand the ITER_BVEC down. For > user-backed iterators it has to be done this way because the I/O may get > shuffled off to a different thread. For sub-page sided transfers it is probably worth doing a bounce buffer copy of user requests - just to save all the complex page pinning code. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
David Laight <David.Laight@ACULAB.COM> wrote: > > It shouldn't matter as the only problematic iterator is ITER_PIPE > > (advancing that has side effects) - and splice_read is handled specially > > by patch 4. The problem with splice_read with the way cifs works is that > > it likes to subdivide its read/write requests across multiple reqs and > > then subsubdivide them if certain types of failure occur. But you can't > > do that with ITER_PIPE. > > I was thinking that even if ok at the moment it might be troublesome later. > Somewhere I started writing a patch to put the iov_cache[] for user > requests into the same structure as the iterator. > Copying those might cause oddities. Well, there is dup_iter(), but that copies the vector table, which isn't what we want in a number of cases. You probably need to come up with a wrapper for that. But we copy iters by assignment in a lot of places. With regards to msg_hdr, it might be worth giving it an iterator pointer rather than its own iterator. I've just had a go at attempting to modify the code. cifs_read_iter_from_socket() wants to copy the iterator and truncate the copy, which makes things slightly trickier. For both of the call sites, receive_encrypted_read() and cifs_readv_receive(), it can do the truncation before calling cifs_read_iter_from_socket(), I think - but it may have to undo the truncation afterwards. > > I build an ITER_BVEC from ITER_PIPE, ITER_UBUF and ITER_IOVEC in the top > > levels with pins inserted as appropriate and hand the ITER_BVEC down. For > > user-backed iterators it has to be done this way because the I/O may get > > shuffled off to a different thread. > > For sub-page sided transfers it is probably worth doing a bounce buffer > copy of user requests - just to save all the complex page pinning code. You can't avoid it for async DIO reads. But that sort of thing I'm intending to do in netfslib. David
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index 1207b39686fb..cb7a3fe89278 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -244,6 +244,9 @@ extern int cifs_read_page_from_socket(struct TCP_Server_Info *server, struct page *page, unsigned int page_offset, unsigned int to_read); +int cifs_read_iter_from_socket(struct TCP_Server_Info *server, + struct iov_iter *iter, + unsigned int to_read); extern int cifs_setup_cifs_sb(struct cifs_sb_info *cifs_sb); void cifs_mount_put_conns(struct cifs_mount_ctx *mnt_ctx); int cifs_mount_get_session(struct cifs_mount_ctx *mnt_ctx); diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index b2a04b4e89a5..5bdabbb6c45c 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -765,6 +765,22 @@ cifs_read_page_from_socket(struct TCP_Server_Info *server, struct page *page, return cifs_readv_from_socket(server, &smb_msg); } +int +cifs_read_iter_from_socket(struct TCP_Server_Info *server, struct iov_iter *iter, + unsigned int to_read) +{ + struct msghdr smb_msg; + int ret; + + smb_msg.msg_iter = *iter; + if (smb_msg.msg_iter.count > to_read) + smb_msg.msg_iter.count = to_read; + ret = cifs_readv_from_socket(server, &smb_msg); + if (ret > 0) + iov_iter_advance(iter, ret); + return ret; +} + static bool is_smb_response(struct TCP_Server_Info *server, unsigned char type) {