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

Dave Chinner
2015-03-04T13:01:06Z

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