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@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
