Re: [PATCH 01/24] iov_iter: Separate type from direction and use accessor functions

From: David Howells <dhowells@redhat.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: dhowells@redhat.com, linux-afs@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/24] iov_iter: Separate type from direction and use accessor functions
Date: Mon, 22 Oct 2018 14:00:45 +0100	[thread overview]
Message-ID: <10856.1540213245@warthog.procyon.org.uk> (raw)
In-Reply-To: <20181020045608.GH32577@ZenIV.linux.org.uk>

Al Viro <viro@ZenIV.linux.org.uk> wrote:

> One general comment: I would strongly recommend splitting the iov_iter
> initializers change into a separate patch.

Done.

> >  			void *addr = kmap_atomic(page);
> >  
> >  			written = copy_to_iter(addr, copy, iter);
> 
> FWIW, I wonder if that one is actually a missing primitive getting open-coded...

Ummm...  You mean the combination of kmap_atomic() and copy_to_iter()?  Can I
leave these for another time?

> >  		memcpy(&ctx->iter, iter, sizeof(struct iov_iter));
> >  		ctx->len = count;
> >  		iov_iter_advance(iter, count);
> 
> ... and so, to much greater extent, is this.

And combining memcpy() and iov_iter_advance()?

> > -	dio->should_dirty = (iter->type == ITER_IOVEC);
> > +	dio->should_dirty = iter_is_iovec(iter);
> 
> Nope.  This path *can* get both read and write iov_iter.  Not an equivalent
> change.

This should really have been (iter->type == (ITER_IOVEC | READ)).

Fixed to:

	dio->should_dirty = iter_is_iovec(iter) && iov_iter_rw(iter) == READ;

> > -		if (iter->type == ITER_IOVEC)
> > +		if (iter_is_iovec(iter))
> >  			dio->flags |= IOMAP_DIO_DIRTY;
> 
> Ditto.

This also should've had "| READ" in there.

Fixed to:

		if (iter_is_iovec(iter) && iov_iter_rw(iter) == READ)
 			dio->flags |= IOMAP_DIO_DIRTY;

> > @@ -417,28 +417,35 @@ int iov_iter_fault_in_readable(struct iov_iter *i, size_t bytes)
> >  	int err;
> >  	struct iovec v;
> >  
> > -	if (!(i->type & (ITER_BVEC|ITER_KVEC))) {
> > +	switch (iov_iter_type(i)) {
> > +	case ITER_IOVEC:
> > +	case ITER_PIPE:
> >  		iterate_iovec(i, bytes, v, iov, skip, ({
> >  			err = fault_in_pages_readable(v.iov_base, v.iov_len);
> >  			if (unlikely(err))
> >  			return err;
> >  		0;}))
> > +		break;
> > +	case ITER_KVEC:
> > +	case ITER_BVEC:
> > +		break;
> >  	}
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(iov_iter_fault_in_readable);
> 
> Huh?  That makes no sense whatsoever - ITER_PIPE ones are write-only in the
> first place, so they won't be passed to that one, but feeding ITER_PIPE to
> iterate_iovec() is insane.  And even if they copy-from ITER_PIPES would
> appear, why the devil would we want to fault-in anything?

Note that the condition "!(i->type & (ITER_BVEC|ITER_KVEC))" is true if type
is ITER_PIPE or ITER_IOVEC.

That said, I can make it BUG if it encounters a pipe iterator.

> > @@ -987,7 +1003,7 @@ void iov_iter_revert(struct iov_iter *i, size_t unroll)
> >  		return;
> >  	i->count += unroll;
> > -	if (unlikely(i->type & ITER_PIPE)) {
> > +	if (unlikely(iov_iter_is_pipe(i))) {
> >  		struct pipe_inode_info *pipe = i->pipe;
> ...
> > +	case ITER_PIPE:
> > +		BUG();
> > +	}
> >  }
> >  EXPORT_SYMBOL(iov_iter_revert);
> 
> Wha...?

It should never get to the BUG() because of the earlier if-statement.
However, the compiler gets picky sometimes, but isn't necessarily good enough
to see that this particular case should never occur, so to avoid getting a
warning about missing cases I put in a BUG.  I can stick a comment on it and
make it just break.

David