Re: [PATCH 4/5] iomap: implement direct I/O

[Dave Chinner <david@fromorbit.com>]

On Mon, Nov 07, 2016 at 04:08:07PM +0100, Christoph Hellwig wrote:
> On Mon, Nov 07, 2016 at 09:40:49AM +1100, Dave Chinner wrote:
> > > +	case IOMAP_HOLE:
> > > +		/*
> > > +		 * We return -ENOTBLK to fall back to buffered I/O for file
> > > +		 * systems that can't fill holes from direct writes.
> > > +		 */
> > > +		if (dio->flags & IOMAP_DIO_WRITE)
> > > +			return -ENOTBLK;
> > > +		/*FALLTHRU*/
> > 
> > This is preventing direct IO writes from being done into holes for
> > all filesystems.
> 
> It's not.  Hint:  the whole iomap code very much assumes a file system
> fills holes before applying the actor on writes.
> 
> That being said I should remove this check - as-is it's dead, untested
> code that I only used for my aborted ext2 conversion, so we're better
> off not having it.

Yup, agreed.

> > > +	iov_iter_truncate(&iter, length);
> > 
> > Won't this truncate the entire DIO down to the length of the first
> > extent that is mapped? 
> 
> It truncates a copy of the main iter down to the length of the extent
> we're working on.  That allows us to limit all the iov_iter based helper
> (most importantly get_user_pages) to only operate on a given extent.
> Later in the function we then advance the primary iter when moving to
> the next extent.

Hmmm, I must be missing something here. iomap_dio_rw() stores a
pointer to the primary iter in the dio structure, and that gets
passed to the actor function, and then it....

Oh, bloody hell, Christoph! :/ You hid a structure copy in the
variable initialisations and used the same variable name for the
copy as the primary pointer:

	struct iov_iter iter = *dio->submit.iter;

That's really subtle and easy for idiots like me to miss when
reading the code. Please make it clear that we're working on
a copy of the primary iter here, not the primary iter itself.

> > > +		if (ret <= 0) {
> > > +			/* magic error code to fall back to buffered I/O */
> > > +			if (ret == -ENOTBLK)
> > > +				ret = 0;
> > > +			break;
> > > +		}
> > > +		pos += ret;
> > > +	} while ((count = iov_iter_count(iter)) > 0);
> > > +	blk_finish_plug(&plug);
> > > +
> > > +	if (ret < 0)
> > > +		cmpxchg(&dio->error, 0, ret);
> > 
> > Why cmpxchg? What are we racing with here? Helper (e.g.
> > dio_set_error())?
> 
> The submission thread against I/O completions (which in the worst
> case could come from multiple threads as well).  Same reason as
> the one in xfs_buf_bio_end_io added in commit 9bdd9bd69b
> ("xfs: buffer ->bi_end_io function requires irq-safe lock")

Yup, that's what I suspected - a comment is needed at least, though,
IMO, a helper w/ comment is the most maintainable approach here.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com