Oh, and another thing: I think you want to make this new code dependent
on the block devie actually supporting REQ_FUA natively. Otherwise
you'll cause a flush for every emulated FUA write, which is only going
make things worse, especially for ATA where FLUSH is not queued. And
last time I check libata still disabled FUA by default.
On Sat, Mar 03, 2018 at 09:53:19AM +1100, Dave Chinner wrote:
> On Fri, Mar 02, 2018 at 11:20:31PM +0100, Christoph Hellwig wrote:
> > > @@ -760,8 +761,19 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
> > > }
> > >
> > > inode_dio_end(file_inode(iocb->ki_filp));
> > > - kfree(dio);
> > >
> > > + /*
> > > + * If a FUA write was done, then that is all we required for datasync
> > > + * semantics -. we don't need to call generic_write_sync() to complete
> > > + * the write.
> > > + */
> > > + if (ret > 0 &&
> > > + (dio->flags & (IOMAP_DIO_WRITE|IOMAP_DIO_WRITE_FUA)) ==
> > > + IOMAP_DIO_WRITE) {
> > > + ret = generic_write_sync(iocb, ret);
> > > + }
> > > +
> > > + kfree(dio);
> >
> > Can please split the move of the generic_write_sync call into
> > generic_write_sync a separate prep patch? It's enough of a logic change
> > on its own that it warrants a separate commit with a separate explanation.
>
> Ok, that makes sense.
>
> > Also I'd be tempted to invert the IOMAP_DIO_WRITE_FUA flag and replace
> > it with an IOMAP_DIO_WRITE_SYNC flag to indicate we need the
> > generic_write_sync call, as that should make the logic much more clear.
>
> Ah, much cleaner.
>
> > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > index 260ff5e5c264..81aa3b73471e 100644
> > > --- a/fs/xfs/xfs_file.c
> > > +++ b/fs/xfs/xfs_file.c
> > > @@ -732,6 +732,11 @@ xfs_file_write_iter(
> > > ret = xfs_file_dio_aio_write(iocb, from);
> > > if (ret == -EREMCHG)
> > > goto buffered;
> > > + /*
> > > + * Direct IO handles sync type writes internally on I/O
> > > + * completion.
> > > + */
> > > + return ret;
> > > } else {
> > > buffered:
> > > ret = xfs_file_buffered_aio_write(iocb, from);
> >
> > The else is not needed and you can now have a much more sensible
> > code flow here:
> >
> > ret = xfs_file_dio_aio_write(iocb, from);
> > if (ret != -EREMCHG))
> > return ret;
> > }
> >
> > ret = xfs_file_buffered_aio_write(iocb, from);
>
> I thought about that, but as you noticed the DAX case gets in the
> way so I didn't bother for a RFC patch.
>
> I'll have a look at making the DAX iomap code do something similar
> with generic_write_sync() so we can simplify this high level code.
>
> Thanks Christoph!
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
---end quoted text---
