Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

[Dave Chinner <david@fromorbit.com>]

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@fromorbit.com