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.
> > + 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.
>
> > + if (may_zero) {
> > + pad = pos & (fs_block_size - 1);
> > + if (pad)
> > + iomap_dio_zero(dio, iomap, pos, fs_block_size - pad);
> > + }
>
> Repeated zeroing code. helper function?
The actual repeated code is in iomap_dio_zero. Because we once zero
the beginning of a block and once the end the arithmetics looks somewhat
similar but actually are different. We could do a trick like the end
parameter to dio_zero_block in the old dio code to save a line of code
or two, but I think it's highly confusing to the reader.
> > + do {
> > + ret = iomap_apply(inode, pos, count, flags, ops, dio,
> > + iomap_dio_actor);
> > + 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")
> Comment decribing use?
Sure.
> Comment on the context the new flags are used under and what they
> mean?
Ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
