| Message ID | 157432403818.17624.9300948341879954830.stgit@warthog.procyon.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: Don't use iov_iter::type directly | expand |
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.
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
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.
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.
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
tentatively merged into cifs-2.6.git for-next (pending more of the usual automated testing we do with the buildbot) On Thu, Nov 21, 2019 at 2:14 AM David Howells <dhowells@redhat.com> 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. > > 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; > } >
Steve French <smfrench@gmail.com> wrote: > tentatively merged into cifs-2.6.git for-next (pending more of the > usual automated testing we do with the buildbot) Thanks. David
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; }
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(-)