Re: [PATCH 3/6] xfs: add DAX file operations support

[Dave Chinner <david@fromorbit.com>]

On Wed, Mar 04, 2015 at 12:09:40PM +0200, Boaz Harrosh wrote:
> On 03/04/2015 01:30 AM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Add the initial support for DAX file operations to XFS. This
> > includes the necessary block allocation and mmap page fault hooks
> > for DAX to function.
> > 
> > Note that the current block allocation code abuses the mapping
> > buffer head to provide a completion callback for unwritten extent
> > allocation when DAX is clearing blocks. The DAX interface needs to
> > be changed to provide a callback similar to get_blocks for this
> > callback.
> 
> It looks like this comment is stale for this set
> 
> A question below
.....

> >  	.fsync		= xfs_dir_fsync,
> >  };
> >  
> > -static const struct vm_operations_struct xfs_file_vm_ops = {
> > -	.fault		= xfs_filemap_fault,
> > -	.map_pages	= filemap_map_pages,
> > -	.page_mkwrite	= xfs_filemap_page_mkwrite,
> > +#ifdef CONFIG_FS_DAX
> > +const struct file_operations xfs_file_dax_operations = {
> > +	.llseek		= xfs_file_llseek,
> > +	.read		= new_sync_read,
> > +	.write		= new_sync_write,
> > +	.read_iter	= xfs_file_read_iter,
> > +	.write_iter	= xfs_file_write_iter,
> > +	.unlocked_ioctl	= xfs_file_ioctl,
> > +#ifdef CONFIG_COMPAT
> > +	.compat_ioctl	= xfs_file_compat_ioctl,
> > +#endif
> > +	.mmap		= xfs_file_mmap,
> > +	.open		= xfs_file_open,
> > +	.release	= xfs_file_release,
> > +	.fsync		= xfs_file_fsync,
> > +	.fallocate	= xfs_file_fallocate,
> >  };
> 
> sigh, The same problem was in ext4, the reason you need
> a second xfs_file_operations vector is because of the minus
> 	.splice_read	= xfs_file_splice_read,
> 	.splice_write	= iter_file_splice_write,
> Which do not exist for DAX

Right, because they use buffered IO, and DAX doesn't do buffered
IO.

> Would it be cleaner to call default_file_splice_write/read
> directly from xfs_file_splice_read/write in the DAX case
> and only keep one vector?

Umm, looking at the code, that is different ot what I remember. it
used to be if the filesystem did not support splice, then it did not
set a vector. That has changed - it now calls a generic splice path
instead.

So, we definitely need splice to/from DAX enabled inodes to be
rejected. I'll have a look at that...

> I have looked through the code, nothing stands out that I
> can see. Do you have a public tree I can pull for easy
> testing?

No, not for a small RFC patchset. git am is your friend ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com