Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] cifs: Don't use iov_iter::type directly
@ 2019-11-21  8:13 David Howells
  2019-11-21  8:19 ` Christoph Hellwig
  2019-11-21  9:11 ` David Howells
  0 siblings, 2 replies; 6+ messages in thread
From: David Howells @ 2019-11-21  8:13 UTC (permalink / raw)
  To: sfrench; +Cc: linux-cifs, linux-fsdevel

Don't use iov_iter::type directly, but rather use the new accessor
functions that have been added.  This allows the .type field to be split
and rearranged without the need to update the filesystems.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/cifs/file.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index fa7b0fa72bb3..526f2b95332d 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2833,7 +2833,7 @@ cifs_write_from_iter(loff_t offset, size_t len, struct iov_iter *from,
 					"direct_writev couldn't get user pages "
 					"(rc=%zd) iter type %d iov_offset %zd "
 					"count %zd\n",
-					result, from->type,
+					result, iov_iter_type(from),
 					from->iov_offset, from->count);
 				dump_stack();
 
@@ -3044,7 +3044,7 @@ static ssize_t __cifs_writev(
 	 * In this case, fall back to non-direct write function.
 	 * this could be improved by getting pages directly in ITER_KVEC
 	 */
-	if (direct && from->type & ITER_KVEC) {
+	if (direct && iov_iter_is_kvec(from)) {
 		cifs_dbg(FYI, "use non-direct cifs_writev for kvec I/O\n");
 		direct = false;
 	}
@@ -3556,7 +3556,7 @@ cifs_send_async_read(loff_t offset, size_t len, struct cifsFileInfo *open_file,
 					"couldn't get user pages (rc=%zd)"
 					" iter type %d"
 					" iov_offset %zd count %zd\n",
-					result, direct_iov.type,
+					result, iov_iter_type(&direct_iov),
 					direct_iov.iov_offset,
 					direct_iov.count);
 				dump_stack();
@@ -3767,7 +3767,7 @@ static ssize_t __cifs_readv(
 	 * fall back to data copy read path
 	 * this could be improved by getting pages directly in ITER_KVEC
 	 */
-	if (direct && to->type & ITER_KVEC) {
+	if (direct && iov_iter_is_kvec(to)) {
 		cifs_dbg(FYI, "use non-direct cifs_user_readv for kvec I/O\n");
 		direct = false;
 	}


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cifs: Don't use iov_iter::type directly
  2019-11-21  8:13 [PATCH] cifs: Don't use iov_iter::type directly David Howells
@ 2019-11-21  8:19 ` Christoph Hellwig
  2019-11-21  9:11 ` David Howells
  1 sibling, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2019-11-21  8:19 UTC (permalink / raw)
  To: David Howells; +Cc: sfrench, linux-cifs, linux-fsdevel

On Thu, Nov 21, 2019 at 08:13:58AM +0000, David Howells wrote:
> Don't use iov_iter::type directly, but rather use the new accessor
> functions that have been added.  This allows the .type field to be split
> and rearranged without the need to update the filesystems.

I'd rather get rid of the accessor and access the fields directly, as
that is much easier to follow.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cifs: Don't use iov_iter::type directly
  2019-11-21  8:13 [PATCH] cifs: Don't use iov_iter::type directly David Howells
  2019-11-21  8:19 ` Christoph Hellwig
@ 2019-11-21  9:11 ` David Howells
  2019-11-21 16:07   ` Christoph Hellwig
  2019-12-06  8:35   ` David Howells
  1 sibling, 2 replies; 6+ messages in thread
From: David Howells @ 2019-11-21  9:11 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: dhowells, sfrench, linux-cifs, linux-fsdevel

Christoph Hellwig <hch@infradead.org> wrote:

> I'd rather get rid of the accessor and access the fields directly, as
> that is much easier to follow.

The problem is that the type is arranged as a bunch of bits:

	ITER_IOVEC = 4,
	ITER_KVEC = 8,
	ITER_BVEC = 16,
	ITER_PIPE = 32,
	ITER_DISCARD = 64,

and we end up doing a lot of:

	if (type & TYPE1) {
	} else if (type & TYPE2) {
	} else if (type & TYPE3) {
	} else if (type & TYPE4) {
	} else {
		/* Do ITER_IOVEC */
	}

constructs - which isn't necessarily the most efficient for the CPU,
particularly if we get more iterator types.  Note that ITER_IOVEC (which I
think is the common case) is usually coming last - and the CPU has to do all
the other checks first since the compiler can't know that it might be able to
take a shortcut (ie. rule out all the other types in one check first).

What I've been exploring is moving to:

	ITER_IOVEC = 0
	ITER_KVEC = 1,
	ITER_BVEC = 2,
	ITER_PIPE = 3,
	ITER_DISCARD = 4,

and using switch statements - and then leaving it to the compiler to decide
how best to do things.  In some ways, it might be nice to let the compiler
decide what constants it might use for this so as to best optimise the use
cases, but there's no way to do that at the moment.

However, all the code that is doing direct accesses using '&' has to change to
make that work - so I've converted it all to using accessors so that I only
have to change the header file, except that the patch to do that crossed with
a cifs patch that added more direct accesses, IIRC.

David


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cifs: Don't use iov_iter::type directly
  2019-11-21  9:11 ` David Howells
@ 2019-11-21 16:07   ` Christoph Hellwig
  2019-11-22 16:26     ` Al Viro
  2019-12-06  8:35   ` David Howells
  1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2019-11-21 16:07 UTC (permalink / raw)
  To: David Howells; +Cc: Christoph Hellwig, sfrench, linux-cifs, linux-fsdevel

On Thu, Nov 21, 2019 at 09:11:11AM +0000, David Howells wrote:
> What I've been exploring is moving to:
> 
> 	ITER_IOVEC = 0
> 	ITER_KVEC = 1,
> 	ITER_BVEC = 2,
> 	ITER_PIPE = 3,
> 	ITER_DISCARD = 4,
> 
> and using switch statements - and then leaving it to the compiler to decide
> how best to do things.  In some ways, it might be nice to let the compiler
> decide what constants it might use for this so as to best optimise the use
> cases, but there's no way to do that at the moment.

I'm all in favor of that. 

> However, all the code that is doing direct accesses using '&' has to change to
> make that work - so I've converted it all to using accessors so that I only
> have to change the header file, except that the patch to do that crossed with
> a cifs patch that added more direct accesses, IIRC.

But I still don't really see the point of the wrappers.  Maybe they are
ok as a migration strategy, but in that case this patch mostly makes
sense as part of the series only.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cifs: Don't use iov_iter::type directly
  2019-11-21 16:07   ` Christoph Hellwig
@ 2019-11-22 16:26     ` Al Viro
  0 siblings, 0 replies; 6+ messages in thread
From: Al Viro @ 2019-11-22 16:26 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: David Howells, sfrench, linux-cifs, linux-fsdevel

On Thu, Nov 21, 2019 at 08:07:25AM -0800, Christoph Hellwig wrote:

> > However, all the code that is doing direct accesses using '&' has to change to
> > make that work - so I've converted it all to using accessors so that I only
> > have to change the header file, except that the patch to do that crossed with
> > a cifs patch that added more direct accesses, IIRC.
> 
> But I still don't really see the point of the wrappers.  Maybe they are
> ok as a migration strategy, but in that case this patch mostly makes
> sense as part of the series only.

Wrappers have one benefit, though - they are greppable.  'type' really isn't.
_IF_ we go for "use that field directly", let's rename the damn field.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] cifs: Don't use iov_iter::type directly
  2019-11-21  9:11 ` David Howells
  2019-11-21 16:07   ` Christoph Hellwig
@ 2019-12-06  8:35   ` David Howells
  1 sibling, 0 replies; 6+ messages in thread
From: David Howells @ 2019-12-06  8:35 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: dhowells, sfrench, linux-cifs, linux-fsdevel

Christoph Hellwig <hch@infradead.org> wrote:

> > However, all the code that is doing direct accesses using '&' has to
> > change to make that work - so I've converted it all to using accessors so
> > that I only have to change the header file, except that the patch to do
> > that crossed with a cifs patch that added more direct accesses, IIRC.
> 
> But I still don't really see the point of the wrappers.  Maybe they are
> ok as a migration strategy, but in that case this patch mostly makes
> sense as part of the series only.

Can we at least push this patch?  All the other users have been converted to
use the wrappers upstream, just not these because the patch adding them
crossed with the wrapper patch.  Then everything is consistent (unless more
unwrapped users got added in the merge window).

David


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-21  8:13 [PATCH] cifs: Don't use iov_iter::type directly David Howells
2019-11-21  8:19 ` Christoph Hellwig
2019-11-21  9:11 ` David Howells
2019-11-21 16:07   ` Christoph Hellwig
2019-11-22 16:26     ` Al Viro
2019-12-06  8:35   ` David Howells

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org
	public-inbox-index linux-cifs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git