Support ext4 on NV-DIMMs

Pagecache faults recheck i_size after taking the page lock to ensure that the fault didn't race against a truncate. We don't have a page to lock in the XIP case, so use the i_mmap_mutex instead. It is locked in the truncate path in unmap_mapping_range() after updating i_size. So while we hold it in the fault path, we are guaranteed that either i_size has already been updated in the truncate path, or that the truncate will subsequently call zap_page_range_single() and so remove the mapping we have just inserted.

There is a window of time in which i_size has been reduced and the thread has a mapping to a page which will be removed from the file, but this is harmless as the page will not be allocated to a different purpose before the thread's access to it is revoked.

Signed-off-by Matthew Wilcox <[email protected]>

---
 mm/filemap_xip.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

mm/filemap_xip.c +22 -2

index d8d9fe3..c8d23e9 100644
@@ -260,8 +260,17 @@ again:
 		__xip_unmap(mapping, vmf->pgoff);
 
 found:
+		/* We must recheck i_size under i_mmap_mutex */
+		mutex_lock(&mapping->i_mmap_mutex);
+		size = (i_size_read(inode) + PAGE_CACHE_SIZE - 1) >>
+							PAGE_CACHE_SHIFT;
+		if (unlikely(vmf->pgoff >= size)) {
+			mutex_unlock(&mapping->i_mmap_mutex);
+			return VM_FAULT_SIGBUS;
+		}
 		err = vm_insert_mixed(vma, (unsigned long)vmf->virtual_address,
 							xip_pfn);
+		mutex_unlock(&mapping->i_mmap_mutex);
 		if (err == -ENOMEM)
 			return VM_FAULT_OOM;
 		/*

@@ -285,16 +294,27 @@ found:
 		}
 		if (error != -ENODATA)
 			goto out;
+
+		/* We must recheck i_size under i_mmap_mutex */
+		mutex_lock(&mapping->i_mmap_mutex);
+		size = (i_size_read(inode) + PAGE_CACHE_SIZE - 1) >>
+							PAGE_CACHE_SHIFT;
+		if (unlikely(vmf->pgoff >= size)) {
+			ret = VM_FAULT_SIGBUS;
+			goto unlock;
+		}
 		/* not shared and writable, use xip_sparse_page() */
 		page = xip_sparse_page();
 		if (!page)
-			goto out;
+			goto unlock;
 		err = vm_insert_page(vma, (unsigned long)vmf->virtual_address,
 							page);
 		if (err == -ENOMEM)
-			goto out;
+			goto unlock;
 
 		ret = VM_FAULT_NOPAGE;
+unlock:
+		mutex_unlock(&mapping->i_mmap_mutex);
 out:
 		write_seqcount_end(&xip_sparse_seq);
 		mutex_unlock(&xip_sparse_mutex);

Signature · 6 lines

1.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Currently COW of an XIP file is done by first bringing in a read-only mapping, then retrying the fault and copying the page. It is much more efficient to tell the fault handler that a COW is being attempted (by passing in the pre-allocated page in the vm_fault structure), and allow the handler to perform the COW operation itself.

Where the filemap code protects against truncation of the file until the PTE has been installed with the page lock, the XIP code use the i_mmap_mutex instead. We must therefore unlock the i_mmap_mutex after inserting the PTE.

Signed-off-by Matthew Wilcox <[email protected]>

---
 include/linux/mm.h |  2 ++
 mm/memory.c        | 45 +++++++++++++++++++++++++++++++++------------
 2 files changed, 35 insertions(+), 12 deletions(-)

include/linux/mm.h +2 -0

index c1b7414..513b78a 100644
@@ -205,6 +205,7 @@ struct vm_fault {
 	pgoff_t pgoff;			/* Logical page offset based on vma */
 	void __user *virtual_address;	/* Faulting virtual address */
 
+	struct page *cow_page;		/* Handler may choose to COW */
 	struct page *page;		/* ->fault handlers should return a
 					 * page here, unless VM_FAULT_NOPAGE
 					 * is set (which is also implied by

@@ -1010,6 +1011,7 @@ static inline int page_mapped(struct page *page)
 #define VM_FAULT_HWPOISON 0x0010	/* Hit poisoned small page */
 #define VM_FAULT_HWPOISON_LARGE 0x0020  /* Hit poisoned large page. Index encoded in upper bits */
 
+#define VM_FAULT_COWED	0x0080	/* ->fault COWed the page instead */
 #define VM_FAULT_NOPAGE	0x0100	/* ->fault installed the pte, not return page */
 #define VM_FAULT_LOCKED	0x0200	/* ->fault locked the returned page */
 #define VM_FAULT_RETRY	0x0400	/* ->fault blocked, must retry */

mm/memory.c +33 -12

index 07b4287..2a2ecac 100644
@@ -2602,6 +2602,7 @@ static int do_page_mkwrite(struct vm_area_struct *vma, struct page *page,
 	vmf.pgoff = page->index;
 	vmf.flags = FAULT_FLAG_WRITE|FAULT_FLAG_MKWRITE;
 	vmf.page = page;
+	vmf.cow_page = NULL;
 
 	ret = vma->vm_ops->page_mkwrite(vma, &vmf);
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))

@@ -3288,7 +3289,8 @@ oom:
 }
 
 static int __do_fault(struct vm_area_struct *vma, unsigned long address,
-		pgoff_t pgoff, unsigned int flags, struct page **page)
+			pgoff_t pgoff, unsigned int flags,
+			struct page *cow_page, struct page **page)
 {
 	struct vm_fault vmf;
 	int ret;

@@ -3297,10 +3299,13 @@ static int __do_fault(struct vm_area_struct *vma, unsigned long address,
 	vmf.pgoff = pgoff;
 	vmf.flags = flags;
 	vmf.page = NULL;
+	vmf.cow_page = cow_page;
 
 	ret = vma->vm_ops->fault(vma, &vmf);
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY)))
 		return ret;
+	if (unlikely(ret & VM_FAULT_COWED))
+		goto out;
 
 	if (unlikely(PageHWPoison(vmf.page))) {
 		if (ret & VM_FAULT_LOCKED)

@@ -3314,6 +3319,7 @@ static int __do_fault(struct vm_area_struct *vma, unsigned long address,
 	else
 		VM_BUG_ON_PAGE(!PageLocked(vmf.page), vmf.page);
 
+ out:
 	*page = vmf.page;
 	return ret;
 }

@@ -3351,7 +3357,7 @@ static int do_read_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 	pte_t *pte;
 	int ret;
 
-	ret = __do_fault(vma, address, pgoff, flags, &fault_page);
+	ret = __do_fault(vma, address, pgoff, flags, NULL, &fault_page);
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY)))
 		return ret;
 

@@ -3368,6 +3374,12 @@ static int do_read_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 	return ret;
 }
 
+/*
+ * If the fault handler performs the COW, it does not return a page,
+ * so cannot use the page's lock to protect against a concurrent truncate
+ * operation.  Instead it returns with the i_mmap_mutex held, which must
+ * be released after the PTE has been inserted.
+ */
 static int do_cow_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 		unsigned long address, pmd_t *pmd,
 		pgoff_t pgoff, unsigned int flags, pte_t orig_pte)

@@ -3389,25 +3401,34 @@ static int do_cow_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 		return VM_FAULT_OOM;
 	}
 
-	ret = __do_fault(vma, address, pgoff, flags, &fault_page);
+	ret = __do_fault(vma, address, pgoff, flags, new_page, &fault_page);
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY)))
 		goto uncharge_out;
 
-	copy_user_highpage(new_page, fault_page, address, vma);
+	if (!(ret & VM_FAULT_COWED))
+		copy_user_highpage(new_page, fault_page, address, vma);
 	__SetPageUptodate(new_page);
 
 	pte = pte_offset_map_lock(mm, pmd, address, &ptl);
-	if (unlikely(!pte_same(*pte, orig_pte))) {
-		pte_unmap_unlock(pte, ptl);
+	if (unlikely(!pte_same(*pte, orig_pte)))
+		goto unlock_out;
+	do_set_pte(vma, address, new_page, pte, true, true);
+	pte_unmap_unlock(pte, ptl);
+	if (ret & VM_FAULT_COWED) {
+		mutex_unlock(&vma->vm_file->f_mapping->i_mmap_mutex);
+	} else {
 		unlock_page(fault_page);
 		page_cache_release(fault_page);
-		goto uncharge_out;
 	}
-	do_set_pte(vma, address, new_page, pte, true, true);
-	pte_unmap_unlock(pte, ptl);
-	unlock_page(fault_page);
-	page_cache_release(fault_page);
 	return ret;
+unlock_out:
+	pte_unmap_unlock(pte, ptl);
+	if (ret & VM_FAULT_COWED) {
+		mutex_unlock(&vma->vm_file->f_mapping->i_mmap_mutex);
+	} else {
+		unlock_page(fault_page);
+		page_cache_release(fault_page);
+	}
 uncharge_out:
 	mem_cgroup_uncharge_page(new_page);
 	page_cache_release(new_page);

@@ -3424,7 +3445,7 @@ static int do_shared_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 	int dirtied = 0;
 	int ret, tmp;
 
-	ret = __do_fault(vma, address, pgoff, flags, &fault_page);
+	ret = __do_fault(vma, address, pgoff, flags, NULL, &fault_page);
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY)))
 		return ret;
 

Signature · 6 lines

1.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The 'pfn' returned by axonram was completely bogus, and has been since 2008.

Signed-off-by Matthew Wilcox <[email protected]>

---
 arch/powerpc/sysdev/axonram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/powerpc/sysdev/axonram.c +1 -1

index 47b6b9f..830edc8 100644
@@ -156,7 +156,7 @@ axon_ram_direct_access(struct block_device *device, sector_t sector,
 	}
 
 	*kaddr = (void *)(bank->ph_addr + offset);
-	*pfn = virt_to_phys(kaddr) >> PAGE_SHIFT;
+	*pfn = virt_to_phys(*kaddr) >> PAGE_SHIFT;
 
 	return 0;
 }

Signature · 6 lines

1.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 2 lines · Matthew Wilcox

The 'pfn' returned by axonram was completely bogus, and has been since 2008.

Maybe time to drop the driver instead? When noone noticed for 6 years, it
seems pretty much dead... Or is there some possibility the driver can get reused for new HW?

In order to support accesses to larger chunks of memory, pass in a 'size' parameter (counted in bytes), and return the amount available at that address.

Signed-off-by Matthew Wilcox <[email protected]>

---
 Documentation/filesystems/xip.txt | 15 +++++++++------
 arch/powerpc/sysdev/axonram.c     |  6 +++---
 drivers/block/brd.c               |  8 +++++---
 drivers/s390/block/dcssblk.c      | 19 ++++++++++---------
 fs/ext2/xip.c                     | 30 +++++++++++++-----------------
 include/linux/blkdev.h            |  4 ++--
 6 files changed, 42 insertions(+), 40 deletions(-)

drivers/block/brd.c +2 -2

index e73b85c..00da60d 100644
@@ -361,8 +361,8 @@ out:
 }
 
 #ifdef CONFIG_BLK_DEV_XIP
-static int brd_direct_access(struct block_device *bdev, sector_t sector,
-			void **kaddr, unsigned long *pfn)
+static long brd_direct_access(struct block_device *bdev, sector_t sector,
+			void **kaddr, unsigned long *pfn, long size)
 {
 	struct brd_device *brd = bdev->bd_disk->private_data;
 	struct page *page;

drivers/block/brd.c +0 -0

 }
 #endif
 

drivers/s390/block/dcssblk.c +6 -6

index ebf41e2..da914b2 100644
@@ -28,8 +28,8 @@
 static int dcssblk_open(struct block_device *bdev, fmode_t mode);
 static void dcssblk_release(struct gendisk *disk, fmode_t mode);
 static void dcssblk_make_request(struct request_queue *q, struct bio *bio);
-static int dcssblk_direct_access(struct block_device *bdev, sector_t secnum,
-				 void **kaddr, unsigned long *pfn);
+static long dcssblk_direct_access(struct block_device *bdev, sector_t secnum,
+				 void **kaddr, unsigned long *pfn, long size);
 
 static char dcssblk_segments[DCSSBLK_PARM_LEN] = "\0";
 

@@ -866,25 +866,26 @@ fail:
 	bio_io_error(bio);
 }
 
-static int
+static long
 dcssblk_direct_access (struct block_device *bdev, sector_t secnum,
-			void **kaddr, unsigned long *pfn)
+			void **kaddr, unsigned long *pfn, long size)
 {
 	struct dcssblk_dev_info *dev_info;
-	unsigned long pgoff;
+	unsigned long offset, dev_sz;
 
 	dev_info = bdev->bd_disk->private_data;
 	if (!dev_info)
 		return -ENODEV;
+	dev_sz = dev_info->end - dev_info->start;
 	if (secnum % (PAGE_SIZE/512))
 		return -EINVAL;
-	pgoff = secnum / (PAGE_SIZE / 512);

drivers/block/brd.c static int brd_direct_access(struct block_device *bdev, sector_t sector, · line 384

Quoted 8 lines

drivers/block/brd.c +3 -1

@@ -379,7 +379,9 @@ static int brd_direct_access(struct block_device *bdev, sector_t sector,
 	*kaddr = page_address(page);
 	*pfn = page_to_pfn(page);
 
-	return 0;
+	/* Could optimistically check to see if the next page in the
+	 * file is mapped to the next page of physical RAM */
+	return PAGE_SIZE;

This should be min_t(long, PAGE_SIZE, size), shouldn't it?

Quoted 13 lines

drivers/s390/block/dcssblk.c +3 -3

@@ -866,25 +866,26 @@ fail:
 	bio_io_error(bio);
 }
 
-static int
+static long
 dcssblk_direct_access (struct block_device *bdev, sector_t secnum,
-			void **kaddr, unsigned long *pfn)
+			void **kaddr, unsigned long *pfn, long size)
 {
 	struct dcssblk_dev_info *dev_info;
-	unsigned long pgoff;
+	unsigned long offset, dev_sz;

drivers/s390/block/dcssblk.c fail: · line 888

Quoted 2 lines

drivers/s390/block/dcssblk.c +1 -1

-	return 0;
+	return min_t(unsigned long, size, dev_sz - offset);

^^^ Why unsigned? Everything seems to be long...

Use an inode flag to tag inodes which should avoid using the page cache. Convert ext2 to use it instead of mapping_is_xip().

Signed-off-by Matthew Wilcox <[email protected]>

---
 fs/ext2/inode.c    | 9 ++++++---
 fs/ext2/xip.h      | 2 --
 include/linux/fs.h | 6 ++++++
 3 files changed, 12 insertions(+), 5 deletions(-)

fs/ext2/inode.c +6 -3

index 94ed3684..e7d3192 100644
@@ -731,7 +731,7 @@ static int ext2_get_blocks(struct inode *inode,
 		goto cleanup;
 	}
 
-	if (ext2_use_xip(inode->i_sb)) {
+	if (IS_DAX(inode)) {
 		/*
 		 * we need to clear the block
 		 */

@@ -1201,7 +1201,7 @@ static int ext2_setsize(struct inode *inode, loff_t newsize)
 
 	inode_dio_wait(inode);
 
-	if (mapping_is_xip(inode->i_mapping))
+	if (IS_DAX(inode))
 		error = xip_truncate_page(inode->i_mapping, newsize);
 	else if (test_opt(inode->i_sb, NOBH))
 		error = nobh_truncate_page(inode->i_mapping,

@@ -1273,7 +1273,8 @@ void ext2_set_inode_flags(struct inode *inode)
 {
 	unsigned int flags = EXT2_I(inode)->i_flags;
 
-	inode->i_flags &= ~(S_SYNC|S_APPEND|S_IMMUTABLE|S_NOATIME|S_DIRSYNC);
+	inode->i_flags &= ~(S_SYNC | S_APPEND | S_IMMUTABLE | S_NOATIME |
+				S_DIRSYNC | S_DAX);
 	if (flags & EXT2_SYNC_FL)
 		inode->i_flags |= S_SYNC;
 	if (flags & EXT2_APPEND_FL)

@@ -1284,6 +1285,8 @@ void ext2_set_inode_flags(struct inode *inode)
 		inode->i_flags |= S_NOATIME;
 	if (flags & EXT2_DIRSYNC_FL)
 		inode->i_flags |= S_DIRSYNC;
+	if (test_opt(inode->i_sb, XIP))
+		inode->i_flags |= S_DAX;
 }
 
 /* Propagate flags from i_flags to EXT2_I(inode)->i_flags */

fs/ext2/xip.h +0 -2

index 18b34d2..29be737 100644
@@ -16,9 +16,7 @@ static inline int ext2_use_xip (struct super_block *sb)
 }
 int ext2_get_xip_mem(struct address_space *, pgoff_t, int,
 				void **, unsigned long *);
-#define mapping_is_xip(map) unlikely(map->a_ops->get_xip_mem)
 #else
-#define mapping_is_xip(map)			0
 #define ext2_xip_verify_sb(sb)			do { } while (0)
 #define ext2_use_xip(sb)			0
 #define ext2_clear_xip_target(inode, chain)	0

include/linux/fs.h +6 -0

index 23b2a35..47fd219 100644
@@ -1644,6 +1644,7 @@ struct super_operations {
 #define S_IMA		1024	/* Inode has an associated IMA struct */
 #define S_AUTOMOUNT	2048	/* Automount/referral quasi-directory */
 #define S_NOSEC		4096	/* no suid or xattr security attributes */
+#define S_DAX		8192	/* Direct Access, avoiding the page cache */
 
 /*
  * Note that nosuid etc flags are inode-specific: setting some file-system

@@ -1681,6 +1682,11 @@ struct super_operations {
 #define IS_IMA(inode)		((inode)->i_flags & S_IMA)
 #define IS_AUTOMOUNT(inode)	((inode)->i_flags & S_AUTOMOUNT)
 #define IS_NOSEC(inode)		((inode)->i_flags & S_NOSEC)
+#ifdef CONFIG_FS_XIP
+#define IS_DAX(inode)		((inode)->i_flags & S_DAX)
+#else
+#define IS_DAX(inode)		0
+#endif
 
 /*
  * Inode state bits.  Protected by inode->i_lock

Signature · 6 lines

1.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Use the generic AIO infrastructure instead of custom read and write methods. In addition to giving us support for AIO, this adds the missing locking between read() and truncate().

Signed-off-by Matthew Wilcox <[email protected]>

Reviewed-by Ross Zwisler <[email protected]>

---
 fs/Makefile        |   1 +
 fs/dax.c           | 216 +++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ext2/file.c     |   6 +-
 fs/ext2/inode.c    |   7 +-
 include/linux/fs.h |  18 ++++-
 mm/filemap.c       |   6 +-
 mm/filemap_xip.c   | 234 -----------------------------------------------------
 7 files changed, 243 insertions(+), 245 deletions(-)
 create mode 100644 fs/dax.c

fs/Makefile +1 -0

index 47ac07b..2f194cd 100644
@@ -29,6 +29,7 @@ obj-$(CONFIG_SIGNALFD)		+= signalfd.o
 obj-$(CONFIG_TIMERFD)		+= timerfd.o
 obj-$(CONFIG_EVENTFD)		+= eventfd.o
 obj-$(CONFIG_AIO)               += aio.o
+obj-$(CONFIG_FS_XIP)		+= dax.o
 obj-$(CONFIG_FILE_LOCKING)      += locks.o
 obj-$(CONFIG_COMPAT)		+= compat.o compat_ioctl.o
 obj-$(CONFIG_BINFMT_AOUT)	+= binfmt_aout.o

fs/dax.c new file +32 -0

new file mode 100644
index 0000000..66a6bda
@@ -0,0 +1,216 @@
+/*
+ * fs/dax.c - Direct Access filesystem code
+ * Copyright (c) 2013-2014 Intel Corporation
+ * Author: Matthew Wilcox <[email protected]>
+ * Author: Ross Zwisler <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/atomic.h>
+#include <linux/blkdev.h>
+#include <linux/buffer_head.h>
+#include <linux/fs.h>
+#include <linux/genhd.h>
+#include <linux/mutex.h>
+#include <linux/uio.h>
+
+static long dax_get_addr(struct inode *inode, struct buffer_head *bh,
+								void **addr)
+{
+	struct block_device *bdev = bh->b_bdev;
+	const struct block_device_operations *ops = bdev->bd_disk->fops;
+	unsigned long pfn;
+	sector_t sector = bh->b_blocknr << (inode->i_blkbits - 9);
+	return ops->direct_access(bdev, sector, addr, &pfn, bh->b_size);

fs/dax.c new file +9 -0

+
+	if (first > 0)
+		memset(addr, 0, first);
+	if (final < size)
+		memset(addr + final, 0, size - final);
+}
+
+static bool buffer_written(struct buffer_head *bh)
+{

fs/dax.c new file +36 -0

+ */
+static bool buffer_size_valid(struct buffer_head *bh)
+{
+	return bh->b_state != 0;
+}
+
+static ssize_t dax_io(int rw, struct inode *inode, const struct iovec *iov,
+			loff_t start, loff_t end, get_block_t get_block,
+			struct buffer_head *bh)
+{
+	ssize_t retval = 0;
+	unsigned seg = 0;
+	unsigned len;
+	unsigned copied = 0;
+	loff_t offset = start;
+	loff_t max = start;
+	loff_t bh_max = start;
+	void *addr;
+	bool hole = false;
+
+	if (rw != WRITE)
+		end = min(end, i_size_read(inode));
+
+	while (offset < end) {
+		void __user *buf = iov[seg].iov_base + copied;
+
+		if (offset == max) {
+			sector_t block = offset >> inode->i_blkbits;
+			unsigned first = offset - (block << inode->i_blkbits);
+			long size;
+
+			if (offset == bh_max) {
+				bh->b_size = PAGE_ALIGN(end - offset);
+				bh->b_state = 0;
+				retval = get_block(inode, block, bh,
+								rw == WRITE);

fs/dax.c new file +76 -0

+				}
+				hole = false;
+			} else {
+				hole = !buffer_written(bh);
+			}
+
+			if (hole) {
+				addr = NULL;
+				size = bh->b_size - first;
+			} else {
+				retval = dax_get_addr(inode, bh, &addr);
+				if (retval < 0)
+					break;
+				if (buffer_unwritten(bh) || buffer_new(bh))
+					dax_new_buf(addr, retval, first,
+						   offset, end, rw);
+				addr += first;
+				size = retval - first;
+			}
+			max = min(offset + size, end);
+		}
+
+		len = min_t(unsigned, iov[seg].iov_len - copied, max - offset);
+
+		if (rw == WRITE)
+			len -= __copy_from_user_nocache(addr, buf, len);
+		else if (!hole)
+			len -= __copy_to_user(buf, addr, len);
+		else
+			len -= __clear_user(buf, len);
+
+		if (!len)
+			break;
+
+		offset += len;
+		copied += len;
+		addr += len;
+		if (copied == iov[seg].iov_len) {
+			seg++;
+			copied = 0;
+		}
+	}
+
+	return (offset == start) ? retval : offset - start;
+}
+
+/**
+ * dax_do_io - Perform I/O to a DAX file
+ * @rw: READ to read or WRITE to write
+ * @iocb: The control block for this I/O
+ * @inode: The file which the I/O is directed at
+ * @iov: The user addresses to do I/O from or to
+ * @offset: The file offset where the I/O starts
+ * @nr_segs: The length of the iov array
+ * @get_block: The filesystem method used to translate file offsets to blocks
+ * @end_io: A filesystem callback for I/O completion
+ * @flags: See below
+ *
+ * This function uses the same locking scheme as do_blockdev_direct_IO:
+ * If @flags has DIO_LOCKING set, we assume that the i_mutex is held by the
+ * caller for writes.  For reads, we take and release the i_mutex ourselves.
+ * If DIO_LOCKING is not set, the filesystem takes care of its own locking.
+ * As with do_blockdev_direct_IO(), we increment i_dio_count while the I/O
+ * is in progress.
+ */
+ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode,
+		const struct iovec *iov, loff_t offset, unsigned nr_segs,
+		get_block_t get_block, dio_iodone_t end_io, int flags)
+{
+	struct buffer_head bh;
+	unsigned seg;
+	ssize_t retval = -EINVAL;
+	loff_t end = offset;
+
+	memset(&bh, 0, sizeof(bh));
+	for (seg = 0; seg < nr_segs; seg++)

fs/dax.c new file +18 -0

+	}
+
+	/* Protects against truncate */
+	atomic_inc(&inode->i_dio_count);
+
+	retval = dax_io(rw, inode, iov, offset, end, get_block, &bh);
+
+	if ((flags & DIO_LOCKING) && (rw == READ))
+		mutex_unlock(&inode->i_mutex);
+
+	inode_dio_done(inode);
+
+	if ((retval > 0) && end_io)
+		end_io(iocb, offset, retval, bh.b_private);
+ out:
+	return retval;
+}
+EXPORT_SYMBOL_GPL(dax_do_io);

fs/ext2/file.c +4 -2

index 44c36e5..ef5cf96 100644
@@ -81,8 +81,10 @@ const struct file_operations ext2_file_operations = {
 #ifdef CONFIG_EXT2_FS_XIP
 const struct file_operations ext2_xip_file_operations = {
 	.llseek		= generic_file_llseek,
-	.read		= xip_file_read,
-	.write		= xip_file_write,
+	.read		= do_sync_read,
+	.write		= do_sync_write,
+	.aio_read	= generic_file_aio_read,
+	.aio_write	= generic_file_aio_write,
 	.unlocked_ioctl = ext2_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= ext2_compat_ioctl,

fs/ext2/inode.c +0 -0

index e7d3192..f128ebf 100644

fs/ext2/inode.c +1 -0

 	if (ret < 0 && (rw & WRITE))
 		ext2_write_failed(mapping, offset + iov_length(iov, nr_segs));

@@ -888,6 +892,7 @@ const struct address_space_operations ext2_aops = {
 const struct address_space_operations ext2_aops_xip = {
 	.bmap			= ext2_bmap,
 	.get_xip_mem		= ext2_get_xip_mem,
+	.direct_IO		= ext2_direct_IO,
 };
 
 const struct address_space_operations ext2_nobh_aops = {

include/linux/fs.h +2 -4

index 47fd219..dabc601 100644
@@ -2521,17 +2521,22 @@ extern int generic_file_open(struct inode * inode, struct file * filp);
 extern int nonseekable_open(struct inode * inode, struct file * filp);
 
 #ifdef CONFIG_FS_XIP
-extern ssize_t xip_file_read(struct file *filp, char __user *buf, size_t len,
-			     loff_t *ppos);
 extern int xip_file_mmap(struct file * file, struct vm_area_struct * vma);
-extern ssize_t xip_file_write(struct file *filp, const char __user *buf,
-			      size_t len, loff_t *ppos);
 extern int xip_truncate_page(struct address_space *mapping, loff_t from);
+ssize_t dax_do_io(int rw, struct kiocb *, struct inode *, const struct iovec *,
+		loff_t, unsigned segs, get_block_t, dio_iodone_t, int flags);
 #else

include/linux/fs.h +0 -0

 static inline ino_t parent_ino(struct dentry *dentry)
 {
 	ino_t res;

mm/filemap.c +1 -2

index 7a13f6a..1b7dff6 100644
@@ -1417,8 +1417,7 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
 	if (retval)
 		return retval;
 
-	/* coalesce the iovecs and go direct-to-BIO for O_DIRECT */
-	if (filp->f_flags & O_DIRECT) {
+	if (io_is_direct(filp)) {
 		loff_t size;
 		struct address_space *mapping;
 		struct inode *inode;

Quoted 5 lines

fs/dax.c new file +5 -0

+static void dax_new_buf(void *addr, unsigned size, unsigned first,
+					loff_t offset, loff_t end, int rw)
+{
+	loff_t final = end - offset + first; /* The final byte of the buffer */
+	if (rw != WRITE) {

fs/dax.c line 42

Quoted 3 lines

fs/dax.c new file +3 -0

+		memset(addr, 0, size);
+		return;
+	}

It seems counterintuitive to zero out "on-disk" blocks (it seems you'd do
this for unwritten blocks) when reading from them. Presumably it could also have undesired effects on endurance of persistent memory. Instead I'd expect that you simply zero out user provided buffer the same way as you do it for holes.

Quoted 7 lines

+/*
+ * When ext4 encounters a hole, it likes to return without modifying the
+ * buffer_head which means that we can't trust b_size. To cope with this,
+ * we set b_state to 0 before calling get_block and, if any bit is set, we
+ * know we can trust b_size. Unfortunate, really, since ext4 does know
+ * precisely how long a hole is and would save us time calling get_block
+ * repeatedly.

Well, this is really a problem of get_blocks() returning the result in
struct buffer_head which is used for input as well. I don't think it is actually ext4 specific.

Quoted 18 lines

fs/dax.c new file +18 -0

+	while (offset < end) {
+		void __user *buf = iov[seg].iov_base + copied;
+
+		if (offset == max) {
+			sector_t block = offset >> inode->i_blkbits;
+			unsigned first = offset - (block << inode->i_blkbits);
+			long size;
+
+			if (offset == bh_max) {
+				bh->b_size = PAGE_ALIGN(end - offset);
+				bh->b_state = 0;
+				retval = get_block(inode, block, bh,
+								rw == WRITE);
+				if (retval)
+					break;
+				if (!buffer_size_valid(bh))
+					bh->b_size = 1 << inode->i_blkbits;
+				bh_max = offset - first + bh->b_size;

fs/dax.c line 107

Quoted 5 lines

fs/dax.c new file +5 -0

+			} else {
+				unsigned done = bh->b_size - (bh_max -
+							(offset - first));
+				bh->b_blocknr += done >> inode->i_blkbits;
+				bh->b_size -= done;

It took me quite some time to figure out what this does and whether it is
correct :). Why isn't this at the place where we advance all other iterators like offset, addr, etc.?

fs/dax.c line 112

Quoted 4 lines

fs/dax.c new file +4 -0

+			if (rw == WRITE) {
+				if (!buffer_mapped(bh)) {
+					retval = -EIO;
+					break;

-EIO looks like a wrong error here. Or maybe it is the right one and it

only needs some explanation? The thing is that for direct IO some filesystems choose not to fill holes for direct IO and fall back to buffered IO instead (to avoid exposure of uninitialized blocks if the system crashes after blocks have been added to a file but before they were written out). For DAX you are pretty much free to define what you ask from the get_blocks() (and this fallback behavior is somewhat disputed behavior in direct IO case so you might want to differ here) but you should document it somewhere.

fs/dax.c line 198

Quoted 8 lines

fs/dax.c new file +8 -0

+	if ((flags & DIO_LOCKING) && (rw == READ)) {
+		struct address_space *mapping = inode->i_mapping;
+		mutex_lock(&inode->i_mutex);
+		retval = filemap_write_and_wait_range(mapping, offset, end - 1);
+		if (retval) {
+			mutex_unlock(&inode->i_mutex);
+			goto out;
+		}

Is there a reason for this? I'd assume DAX has no pages in pagecache...

Quoted 4 lines

fs/ext2/inode.c +0 -0

@@ -858,7 +858,11 @@ ext2_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov,
 	struct inode *inode = mapping->host;
 	ssize_t ret;
 

fs/ext2/inode.c ext2_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov, · line 866

Quoted 7 lines

fs/ext2/inode.c +5 -1

-	ret = blockdev_direct_IO(rw, iocb, inode, iov, offset, nr_segs,
+	if (IS_DAX(inode))
+		ret = dax_do_io(rw, iocb, inode, iov, offset, nr_segs,
+				ext2_get_block, NULL, DIO_LOCKING);
+	else
+		ret = blockdev_direct_IO(rw, iocb, inode, iov, offset, nr_segs,
 				 ext2_get_block);

I'd somewhat prefer to have a ext2_direct_IO() as is and have
ext2_dax_IO() call only dax_do_io() (and use that as .direct_io in ext2_aops_xip). Then there's no need to check IS_DAX() and the code would look more obvious to me. But I don't feel strongly about it.

include/linux/fs.h extern int generic_show_options(struct seq_file *m, struct dentry *root); · line 2693

Quoted 9 lines

include/linux/fs.h +5 -0

@@ -2681,6 +2686,11 @@ extern int generic_show_options(struct seq_file *m, struct dentry *root);
 extern void save_mount_options(struct super_block *sb, char *options);
 extern void replace_mount_options(struct super_block *sb, char *options);
 
+static inline bool io_is_direct(struct file *filp)
+{
+	return (filp->f_flags & O_DIRECT) || IS_DAX(file_inode(filp));
+}
+

BTW: It seems fs/open.c: open_check_o_direct() can be simplified to not
check for get_xip_mem(), cannot it?

Quoted 5 lines · Jan Kara

Quoted 5 lines

fs/dax.c new file +5 -0

+static void dax_new_buf(void *addr, unsigned size, unsigned first,
+					loff_t offset, loff_t end, int rw)
+{
+	loff_t final = end - offset + first; /* The final byte of the buffer */
+	if (rw != WRITE) {

Quoted 8 lines

fs/dax.c line 42

Quoted 3 lines

fs/dax.c new file +3 -0

+		memset(addr, 0, size);
+		return;
+	}

It seems counterintuitive to zero out "on-disk" blocks (it seems you'd do
this for unwritten blocks) when reading from them. Presumably it could also have undesired effects on endurance of persistent memory. Instead I'd expect that you simply zero out user provided buffer the same way as you do it for holes.

I think we have to zero it here, because the second time we call get_block() for a given block, it won't be BH_New any more, so we won't know that it's supposed to be zeroed.

Quoted 7 lines

Quoted 7 lines

+/*
+ * When ext4 encounters a hole, it likes to return without modifying the
+ * buffer_head which means that we can't trust b_size. To cope with this,
+ * we set b_state to 0 before calling get_block and, if any bit is set, we
+ * know we can trust b_size. Unfortunate, really, since ext4 does know
+ * precisely how long a hole is and would save us time calling get_block
+ * repeatedly.

Quoted 3 lines

Well, this is really a problem of get_blocks() returning the result in
struct buffer_head which is used for input as well. I don't think it is actually ext4 specific.

Of course it's ext4 specific! It's the ext4_get_block() implementation which is choosing not to return the length of the hole. XFS does return the length of the hole. I think something like this would fix it:

+++ b/fs/ext4/inode.c

(quoted diff) +3 -0

@@ -727,14 +727,14 @@ static int _ext4_get_block(struct inode *inode, sector_t i
        }
 
        ret = ext4_map_blocks(handle, inode, &map, flags);
+       map_bh(bh, inode->i_sb, map.m_pblk);
+       bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags;
+       bh->b_size = inode->i_sb->s_blocksize * map.m_len;
        if (ret > 0) {

Quoted 18 lines

Quoted 18 lines

fs/dax.c new file +18 -0

+	while (offset < end) {
+		void __user *buf = iov[seg].iov_base + copied;
+
+		if (offset == max) {
+			sector_t block = offset >> inode->i_blkbits;
+			unsigned first = offset - (block << inode->i_blkbits);
+			long size;
+
+			if (offset == bh_max) {
+				bh->b_size = PAGE_ALIGN(end - offset);
+				bh->b_state = 0;
+				retval = get_block(inode, block, bh,
+								rw == WRITE);
+				if (retval)
+					break;
+				if (!buffer_size_valid(bh))
+					bh->b_size = 1 << inode->i_blkbits;
+				bh_max = offset - first + bh->b_size;

Quoted 8 lines

fs/dax.c line 107

Quoted 5 lines

fs/dax.c new file +5 -0

+			} else {
+				unsigned done = bh->b_size - (bh_max -
+							(offset - first));
+				bh->b_blocknr += done >> inode->i_blkbits;
+				bh->b_size -= done;

It took me quite some time to figure out what this does and whether it is
correct :). Why isn't this at the place where we advance all other iterators like offset, addr, etc.?

It'll be kind of tricky to move it because 'len' is not necessarily a multiple of i_blkbits, so we can't necessarily maintain b_blocknr accurately.

Quoted 5 lines

fs/dax.c line 112

Quoted 4 lines

fs/dax.c new file +4 -0

+			if (rw == WRITE) {
+				if (!buffer_mapped(bh)) {
+					retval = -EIO;
+					break;

-EIO looks like a wrong error here. Or maybe it is the right one and it

Quoted 8 lines

only needs some explanation? The thing is that for direct IO some filesystems choose not to fill holes for direct IO and fall back to buffered IO instead (to avoid exposure of uninitialized blocks if the system crashes after blocks have been added to a file but before they were written out). For DAX you are pretty much free to define what you ask from the get_blocks() (and this fallback behavior is somewhat disputed behavior in direct IO case so you might want to differ here) but you should document it somewhere.

Hmm ... I thought that calling get_block() with the create argument would force the return of a bh with the Mapped bit set. Did I misunderstand that aspect of the undocumented get_block() API too?

Quoted 9 lines

fs/dax.c line 198

Quoted 8 lines

fs/dax.c new file +8 -0

+	if ((flags & DIO_LOCKING) && (rw == READ)) {
+		struct address_space *mapping = inode->i_mapping;
+		mutex_lock(&inode->i_mutex);
+		retval = filemap_write_and_wait_range(mapping, offset, end - 1);
+		if (retval) {
+			mutex_unlock(&inode->i_mutex);
+			goto out;
+		}

Is there a reason for this? I'd assume DAX has no pages in pagecache...

There will be pages in the page cache for holes that we page faulted on. They must go! :-)

Quoted 4 lines

Quoted 4 lines

fs/ext2/inode.c +0 -0

@@ -858,7 +858,11 @@ ext2_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov,
 	struct inode *inode = mapping->host;
 	ssize_t ret;
 

Quoted 11 lines

fs/ext2/inode.c ext2_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov, · line 866

Quoted 7 lines

fs/ext2/inode.c +5 -1

-	ret = blockdev_direct_IO(rw, iocb, inode, iov, offset, nr_segs,
+	if (IS_DAX(inode))
+		ret = dax_do_io(rw, iocb, inode, iov, offset, nr_segs,
+				ext2_get_block, NULL, DIO_LOCKING);
+	else
+		ret = blockdev_direct_IO(rw, iocb, inode, iov, offset, nr_segs,
 				 ext2_get_block);

I'd somewhat prefer to have a ext2_direct_IO() as is and have
ext2_dax_IO() call only dax_do_io() (and use that as .direct_io in ext2_aops_xip). Then there's no need to check IS_DAX() and the code would look more obvious to me. But I don't feel strongly about it.

I can look at that ... but I was hoping to not have separate aops for XIP and non-XIP files.

Quoted 15 lines · Matthew Wilcox

Quoted 13 lines · Jan Kara

fs/dax.c line 42

Quoted 8 lines

fs/dax.c new file +8 -0

+static void dax_new_buf(void *addr, unsigned size, unsigned first,
+					loff_t offset, loff_t end, int rw)
+{
+	loff_t final = end - offset + first; /* The final byte of the buffer */
+	if (rw != WRITE) {
+		memset(addr, 0, size);
+		return;
+	}

It seems counterintuitive to zero out "on-disk" blocks (it seems you'd do
this for unwritten blocks) when reading from them. Presumably it could also have undesired effects on endurance of persistent memory. Instead I'd expect that you simply zero out user provided buffer the same way as you do it for holes.

Quoted 3 lines

I think we have to zero it here, because the second time we call get_block() for a given block, it won't be BH_New any more, so we won't know that it's supposed to be zeroed.

But how can you have BH_New buffer when you didn't ask get_blocks() to
create any block? That would be a bug in the get_blocks() implementation... Or am I missing something?

OK, but there are filesystems which do the same thing as ext4 (e.g.
btrfs) and historically noone really cared. E.g. direct IO code advances only by a single block regardless of what filesystem returns when the buffer is unmapped. As you correctly mention, get_blocks() API isn't really documented so noone has really defined what should happen when you ask filesystem to map some blocks and there's a hole. I agree what XFS does looks sensible and ext4 can do the same. Hopefully this gets cleaned up when Dave finishes his new block mapping interface.

This wouldn't quite work because even ext4_map_blocks() doesn't bother to
fill in 'map' when it finds a hole. But it won't be complicated to propagate the information.

Quoted 3 lines

It'll be kind of tricky to move it because 'len' is not necessarily a multiple of i_blkbits, so we can't necessarily maintain b_blocknr accurately.

Yeah, after I understood the code I also understood why you do it the way
you did. But we could do something like:
...

fs/dax.c new file +3 -0

+               if (!len)
+                       break;
+ 

		blocks = ((offset + len) >> inode->i_blkbits) - 
				(offset >> inode->i_blkbits);
		bh->b_blocknr += blocks;
		bh->b_size -= blocks << inode->i_blkbits;

BTW: it might be good to store inode->i_blkbits in a local variable. It makes some expressions shorter.

BTW2: although direct IO uses 'offset' for position in file, the rest of VFS uses 'pos' for that and that seems to be less overloaded term so for me it would be easier if you used 'pos' instead of 'offset'. Just a suggestion.

Quoted 14 lines

Quoted 5 lines

fs/dax.c line 112

Quoted 4 lines

fs/dax.c new file +4 -0

+			if (rw == WRITE) {
+				if (!buffer_mapped(bh)) {
+					retval = -EIO;
+					break;

-EIO looks like a wrong error here. Or maybe it is the right one and it

Quoted 8 lines

only needs some explanation? The thing is that for direct IO some filesystems choose not to fill holes for direct IO and fall back to buffered IO instead (to avoid exposure of uninitialized blocks if the system crashes after blocks have been added to a file but before they were written out). For DAX you are pretty much free to define what you ask from the get_blocks() (and this fallback behavior is somewhat disputed behavior in direct IO case so you might want to differ here) but you should document it somewhere.

Quoted 3 lines

Hmm ... I thought that calling get_block() with the create argument would force the return of a bh with the Mapped bit set. Did I misunderstand that aspect of the undocumented get_block() API too?

As you mention the API is undocumented and not really designed. So
filesystems do whatever causes the generic code to do what they want (it's a mess I know). In this case, I'm warning you there are filesystems which refuse to fill in holes from the get_blocks() function passed to blockdev_direct_IO() (even ext4 does this for inodes with old indirect-block based on disk format). You can just define DAX fails horribly in these case and I'm fine with that at least in this stage. If someone bothers later, fallback to buffered IO can be implemented. But we should document this somewhere.

Quoted 10 lines

Quoted 9 lines

fs/dax.c line 198

Quoted 8 lines

fs/dax.c new file +8 -0

+	if ((flags & DIO_LOCKING) && (rw == READ)) {
+		struct address_space *mapping = inode->i_mapping;
+		mutex_lock(&inode->i_mutex);
+		retval = filemap_write_and_wait_range(mapping, offset, end - 1);
+		if (retval) {
+			mutex_unlock(&inode->i_mutex);
+			goto out;
+		}

Is there a reason for this? I'd assume DAX has no pages in pagecache...

Quoted 2 lines

There will be pages in the page cache for holes that we page faulted on. They must go! :-)

Well, but this will only writeback dirty pages and if I read the code
correctly those pages will never be dirty since dax_mkwrite() will replace them. Or am I missing something?

Quoted 16 lines · Jan Kara

Quoted 15 lines · Matthew Wilcox

Quoted 13 lines · Jan Kara

fs/dax.c line 42

Quoted 8 lines

fs/dax.c new file +8 -0

+static void dax_new_buf(void *addr, unsigned size, unsigned first,
+					loff_t offset, loff_t end, int rw)
+{
+	loff_t final = end - offset + first; /* The final byte of the buffer */
+	if (rw != WRITE) {
+		memset(addr, 0, size);
+		return;
+	}

It seems counterintuitive to zero out "on-disk" blocks (it seems you'd do
this for unwritten blocks) when reading from them. Presumably it could also have undesired effects on endurance of persistent memory. Instead I'd expect that you simply zero out user provided buffer the same way as you do it for holes.

Quoted 6 lines

Quoted 3 lines

I think we have to zero it here, because the second time we call get_block() for a given block, it won't be BH_New any more, so we won't know that it's supposed to be zeroed.

But how can you have BH_New buffer when you didn't ask get_blocks() to
create any block? That would be a bug in the get_blocks() implementation... Or am I missing something?

Oh ... right. So just to be clear, we're looking at the case where we're doing a read of a filesystem block which is BH_Unwritten, but isn't a hole ... so it's been allocated on storage and not yet written. That's already treated as a hole:

                        if (rw == WRITE) {
...
                        } else {
                                hole = !buffer_written(bh);
                        }

and dax_new_buf is only called in the !hole case.

Quoted 12 lines

Quoted 12 lines

Quoted 9 lines

fs/dax.c line 198

Quoted 8 lines

fs/dax.c new file +8 -0

+	if ((flags & DIO_LOCKING) && (rw == READ)) {
+		struct address_space *mapping = inode->i_mapping;
+		mutex_lock(&inode->i_mutex);
+		retval = filemap_write_and_wait_range(mapping, offset, end - 1);
+		if (retval) {
+			mutex_unlock(&inode->i_mutex);
+			goto out;
+		}

Is there a reason for this? I'd assume DAX has no pages in pagecache...

There will be pages in the page cache for holes that we page faulted on. They must go! :-)

Quoted 3 lines

Well, but this will only writeback dirty pages and if I read the code
correctly those pages will never be dirty since dax_mkwrite() will replace them. Or am I missing something?

In addition to writing back dirty pages, filemap_write_and_wait_range() will evict clean pages. Unintuitive, I know, but it matches what the direct I/O path does. Plus, if we fall back to buffered I/O for holes (see above), then this will do the right thing at that time.

Quoted 4 lines

In addition to writing back dirty pages, filemap_write_and_wait_range() will evict clean pages. Unintuitive, I know, but it matches what the direct I/O path does. Plus, if we fall back to buffered I/O for holes (see above), then this will do the right thing at that time.

Ugh, I'm pretty certain filemap_write_and_wait_range() doesn't evict
anything ;). Direct IO path calls that function so that direct IO read after buffered write returns the written data. In that case we don't evict anything from page cache because direct IO read doesn't invalidate any information we have cached. Only direct IO write does that and for that we call invalidate_inode_pages2_range() after writing the pages. So I maintain that what you do doesn't make sense to me. You might need to do some invalidation of hole pages. But note that generic_file_direct_write() does that for you and even though that isn't serialized in any way with page faults which can instantiate the hole pages again, things should work out fine for you since that function also invalidates the range again after ->direct_IO callback is done. So AFAICT you don't have to do anything except writing some nice comment about this ;).

fs/dax.c new file +4 -0

+ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode,
+		const struct iovec *iov, loff_t offset, unsigned nr_segs,
+		get_block_t get_block, dio_iodone_t end_io, int flags)
+{

Instead of calling aops->get_xip_mem from the fault handler, the filesystem passes a get_block_t that is used to find the appropriate blocks.

Signed-off-by Matthew Wilcox <[email protected]>

---
 fs/dax.c           | 207 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ext2/file.c     |  35 ++++++++-
 include/linux/fs.h |   4 +-
 mm/filemap_xip.c   | 206 ----------------------------------------------------
 4 files changed, 243 insertions(+), 209 deletions(-)

fs/dax.c +25 -0

index 66a6bda..863749c 100644
@@ -19,8 +19,12 @@
 #include <linux/buffer_head.h>
 #include <linux/fs.h>
 #include <linux/genhd.h>
+#include <linux/highmem.h>
+#include <linux/memcontrol.h>
+#include <linux/mm.h>
 #include <linux/mutex.h>
 #include <linux/uio.h>
+#include <linux/vmstat.h>
 
 static long dax_get_addr(struct inode *inode, struct buffer_head *bh,
 								void **addr)

@@ -32,6 +36,16 @@ static long dax_get_addr(struct inode *inode, struct buffer_head *bh,
 	return ops->direct_access(bdev, sector, addr, &pfn, bh->b_size);
 }
 
+static long dax_get_pfn(struct inode *inode, struct buffer_head *bh,
+							unsigned long *pfn)
+{
+	struct block_device *bdev = bh->b_bdev;
+	const struct block_device_operations *ops = bdev->bd_disk->fops;
+	void *addr;
+	sector_t sector = bh->b_blocknr << (inode->i_blkbits - 9);
+	return ops->direct_access(bdev, sector, &addr, pfn, bh->b_size);
+}
+
 static void dax_new_buf(void *addr, unsigned size, unsigned first,
 					loff_t offset, loff_t end, int rw)
 {

@@ -214,3 +228,196 @@ ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode,
 	return retval;
 }
 EXPORT_SYMBOL_GPL(dax_do_io);
+
+/*
+ * The user has performed a load from a hole in the file.  Allocating
+ * a new page in the file would cause excessive storage usage for
+ * workloads with sparse files.  We allocate a page cache page instead.
+ * We'll kick it out of the page cache if it's ever written to,
+ * otherwise it will simply fall out of the page cache under memory
+ * pressure without ever having been dirtied.
+ */
+static int dax_load_hole(struct address_space *mapping, struct page *page,
+							struct vm_fault *vmf)

fs/dax.c +4 -0

+		unlock_page(page);
+		page_cache_release(page);
+		return VM_FAULT_SIGBUS;
+	}

fs/dax.c +71 -0

+	vto = kmap_atomic(to);
+	copy_user_page(vto, vfrom, vaddr, to);
+	kunmap_atomic(vto);
+}
+
+static int do_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
+			get_block_t get_block)
+{
+	struct file *file = vma->vm_file;
+	struct inode *inode = file_inode(file);
+	struct address_space *mapping = file->f_mapping;
+	struct page *page;
+	struct buffer_head bh;
+	unsigned long vaddr = (unsigned long)vmf->virtual_address;
+	sector_t block;
+	pgoff_t size;
+	unsigned long pfn;
+	int error;
+	int major = 0;
+
+	size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	if (vmf->pgoff >= size)
+		return VM_FAULT_SIGBUS;
+
+	memset(&bh, 0, sizeof(bh));
+	block = (sector_t)vmf->pgoff << (PAGE_SHIFT - inode->i_blkbits);
+	bh.b_size = PAGE_SIZE;
+
+ repeat:
+	page = find_get_page(mapping, vmf->pgoff);
+	if (page) {
+		if (!lock_page_or_retry(page, vma->vm_mm, vmf->flags)) {
+			page_cache_release(page);
+			return VM_FAULT_RETRY;
+		}
+		if (unlikely(page->mapping != mapping)) {
+			unlock_page(page);
+			page_cache_release(page);
+			goto repeat;
+		}
+	}
+
+	error = get_block(inode, block, &bh, 0);
+	if (error || bh.b_size < PAGE_SIZE)
+		goto sigbus;
+
+	if (!buffer_written(&bh) && !vmf->cow_page) {
+		if (vmf->flags & FAULT_FLAG_WRITE) {
+			error = get_block(inode, block, &bh, 1);
+			count_vm_event(PGMAJFAULT);
+			mem_cgroup_count_vm_event(vma->vm_mm, PGMAJFAULT);
+			major = VM_FAULT_MAJOR;
+			if (error || bh.b_size < PAGE_SIZE)
+				goto sigbus;
+		} else {
+			return dax_load_hole(mapping, page, vmf);
+		}
+	}
+
+	/* Recheck i_size under i_mmap_mutex */
+	mutex_lock(&mapping->i_mmap_mutex);
+	size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	if (unlikely(vmf->pgoff >= size)) {
+		mutex_unlock(&mapping->i_mmap_mutex);
+		goto sigbus;
+	}
+	if (vmf->cow_page) {
+		if (buffer_written(&bh))
+			copy_user_bh(vmf->cow_page, inode, &bh, vaddr);
+		else
+			clear_user_highpage(vmf->cow_page, vaddr);

fs/dax.c +25 -0

+		unlock_page(page);
+		page_cache_release(page);
+	}
+
+	if (error == -ENOMEM)
+		return VM_FAULT_OOM;
+	/* -EBUSY is fine, somebody else faulted on the same PTE */
+	if (error != -EBUSY)
+		BUG_ON(error);
+	return VM_FAULT_NOPAGE | major;
+
+ sigbus:
+	if (page) {
+		unlock_page(page);
+		page_cache_release(page);
+	}
+	return VM_FAULT_SIGBUS;
+}
+
+/**
+ * dax_fault - handle a page fault on an XIP file
+ * @vma: The virtual memory area where the fault occurred
+ * @vmf: The description of the fault
+ * @get_block: The filesystem method used to translate file offsets to blocks
+ *

fs/dax.c +31 -0

+	result = do_dax_fault(vma, vmf, get_block);
+	sb_end_pagefault(sb);
+
+	return result;
+}
+EXPORT_SYMBOL_GPL(dax_fault);
+
+/**
+ * dax_mkwrite - convert a read-only page to read-write in an XIP file
+ * @vma: The virtual memory area where the fault occurred
+ * @vmf: The description of the fault
+ * @get_block: The filesystem method used to translate file offsets to blocks
+ *
+ * XIP handles reads of holes by adding pages full of zeroes into the
+ * mapping.  If the page is subsequenty written to, we have to allocate
+ * the page on media and free the page that was in the cache.
+ */
+int dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
+			get_block_t get_block)
+{
+	int result;
+	struct super_block *sb = file_inode(vma->vm_file)->i_sb;
+
+	sb_start_pagefault(sb);
+	file_update_time(vma->vm_file);
+	result = do_dax_fault(vma, vmf, get_block);
+	sb_end_pagefault(sb);
+
+	return result;
+}
+EXPORT_SYMBOL_GPL(dax_mkwrite);

fs/ext2/file.c +31 -0

index ef5cf96..e3ce10d 100644
@@ -25,6 +25,37 @@
 #include "xattr.h"
 #include "acl.h"
 
+#ifdef CONFIG_EXT2_FS_XIP
+static int ext2_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	return dax_fault(vma, vmf, ext2_get_block);
+}
+
+static int ext2_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	return dax_mkwrite(vma, vmf, ext2_get_block);
+}
+
+static const struct vm_operations_struct ext2_dax_vm_ops = {
+	.fault		= ext2_dax_fault,
+	.page_mkwrite	= ext2_dax_mkwrite,
+	.remap_pages	= generic_file_remap_pages,
+};
+
+static int ext2_file_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	if (!IS_DAX(file_inode(file)))
+		return generic_file_mmap(file, vma);
+
+	file_accessed(file);
+	vma->vm_ops = &ext2_dax_vm_ops;
+	vma->vm_flags |= VM_MIXEDMAP;
+	return 0;
+}
+#else
+#define ext2_file_mmap	generic_file_mmap
+#endif
+
 /*
  * Called when filp is released. This happens when all file descriptors
  * for a single struct file are closed. Note that different open() calls

fs/dax.c ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode, · line 251

Quoted 4 lines

fs/dax.c +4 -0

+	if (!page)
+		return VM_FAULT_OOM;
+	size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	if (vmf->pgoff >= size) {

Maybe comment here that we have to recheck i_size so that we don't create
pages in the area truncate_pagecache() has already evicted.

fs/dax.c ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode, · line 265

Quoted 1 line

fs/dax.c +1 -0

+	dax_get_addr(inode, bh, &vfrom);	/* XXX: error handling */

The error handling here is missing as the comment suggests :)

fs/dax.c ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode, · line 346

Quoted 2 lines

fs/dax.c +2 -0

+	if (buffer_unwritten(&bh) || buffer_new(&bh))
+		dax_clear_blocks(inode, bh.b_blocknr, bh.b_size);

Where is dax_clear_blocks() defined?

fs/dax.c ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode, · line 350

Quoted 4 lines

fs/dax.c +4 -0

+
+	error = dax_get_pfn(inode, &bh, &pfn);
+	if (error > 0)
+		error = vm_insert_mixed(vma, vaddr, pfn);

When there's a hole (thus page != NULL) and we are called from
dax_mkwrite(), this will always return EBUSY, correct?

Quoted 4 lines

fs/dax.c +4 -0

+	mutex_unlock(&mapping->i_mmap_mutex);
+
+	if (page) {
+		delete_from_page_cache(page);

fs/dax.c ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode, · line 356

Quoted 2 lines

fs/dax.c +2 -0

+		unmap_mapping_range(mapping, vmf->pgoff << PAGE_SHIFT,
+							PAGE_CACHE_SIZE, 0);

Here we unmap the PTE pointing to the hole page but then we'll have to
retry the fault again to fill in the pfn we've got? This seems wrong. I'd say we want to remap the PTE from the hole page to a pfn we've got while holding i_mmap_mutex. remap_pfn_range() almost does what you need, except that you also need that to work for normal pages. So you might need to create a new helper in mm layer for that.

fs/dax.c ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode, · line 391

Quoted 7 lines

fs/dax.c +7 -0

+int dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
+			get_block_t get_block)
+{
+	int result;
+	struct super_block *sb = file_inode(vma->vm_file)->i_sb;
+
+	sb_start_pagefault(sb);

You don't need any filesystem freeze protection for the fault handler
since that's not going to modify the filesystem.

Review

Quoted 1 line

(quoted diff) +1 -0

+	file_update_time(vma->vm_file);

Why do you update m/ctime? We are only reading the file...

fs/ext2/file.c const struct file_operations ext2_file_operations = { · line 104

Quoted 6 lines

fs/ext2/file.c +1 -1

@@ -70,7 +101,7 @@ const struct file_operations ext2_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= ext2_compat_ioctl,
 #endif
-	.mmap		= generic_file_mmap,
+	.mmap		= ext2_file_mmap,

So what's the point of ext2_file_operations ever handling IS_DAX()
inodes? Actually ext2_file_operations and ext2_xip_file_operations seem to be the same after this patch so either you drop ext2_xip_file_operations (I'm for this) or you can leave generic_file_mmap here and assume ext2_file_mmap is always called for IS_DAX() inodes.

Quoted 6 lines · Jan Kara

fs/dax.c ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode, · line 350

Quoted 4 lines

fs/dax.c +4 -0

+
+	error = dax_get_pfn(inode, &bh, &pfn);
+	if (error > 0)
+		error = vm_insert_mixed(vma, vaddr, pfn);

When there's a hole (thus page != NULL) and we are called from
dax_mkwrite(), this will always return EBUSY, correct?

Erm ... it will return -EBUSY if this was the task that previously faulted on it. Drat. See below.

Quoted 4 lines

Quoted 4 lines

fs/dax.c +4 -0

+	mutex_unlock(&mapping->i_mmap_mutex);
+
+	if (page) {
+		delete_from_page_cache(page);

Quoted 8 lines

fs/dax.c ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode, · line 356

Quoted 2 lines

fs/dax.c +2 -0

+		unmap_mapping_range(mapping, vmf->pgoff << PAGE_SHIFT,
+							PAGE_CACHE_SIZE, 0);

Here we unmap the PTE pointing to the hole page but then we'll have to
retry the fault again to fill in the pfn we've got? This seems wrong. I'd say we want to remap the PTE from the hole page to a pfn we've got while holding i_mmap_mutex. remap_pfn_range() almost does what you need, except that you also need that to work for normal pages. So you might need to create a new helper in mm layer for that.

I think it's easier than that. How does this look?

fs/dax.c +2 -3

@@ -390,9 +389,8 @@ static int do_dax_fault(struct vm_area_struct *vma, struct v
                dax_clear_blocks(inode, bh.b_blocknr, bh.b_size);
 
        error = dax_get_pfn(&bh, &pfn, blkbits);
-       if (error > 0)
-               error = vm_insert_mixed(vma, vaddr, pfn);
-       mutex_unlock(&mapping->i_mmap_mutex);
+       if (error <= 0)
+               goto unlock;
 
        if (page) {
                delete_from_page_cache(page);

(quoted diff) +2 -0

        if (error == -ENOMEM)
                return VM_FAULT_OOM;
        /* -EBUSY is fine, somebody else faulted on the same PTE */

@@ -409,6 +410,8 @@ static int do_dax_fault(struct vm_area_struct *vma, struct v
                BUG_ON(error);
        return VM_FAULT_NOPAGE | major;
 
+ unlock:
+       mutex_unlock(&mapping->i_mmap_mutex);
  sigbus:
        if (page) {
                unlock_page(page);

Quoted 11 lines

fs/dax.c ssize_t dax_do_io(int rw, struct kiocb *iocb, struct inode *inode, · line 391

Quoted 9 lines

fs/dax.c +7 -0

+int dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
+			get_block_t get_block)
+{
+	int result;
+	struct super_block *sb = file_inode(vma->vm_file)->i_sb;
+
+	sb_start_pagefault(sb);

You don't need any filesystem freeze protection for the fault handler
since that's not going to modify the filesystem.

Err ... we might allocate a block as a result of doing a write to a hole. Or does that not count as 'modifying the filesystem' in this context?

Quoted 2 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	file_update_time(vma->vm_file);

Why do you update m/ctime? We are only reading the file...

... except that it might be a write fault. I think we modify the file iff we return VM_FAULT_MAJOR from do_dax_fault(). So I'd be open to something like this:

	sb_start_pagefault(sb);
	result = do_dax_fault(vma, vmf, get_block);
	if (result & VM_FAULT_MAJOR)
		file_update_time(vma->vm_file);
	sb_end_pagefault(sb);

Would that work better for you?

Quoted 11 lines

fs/ext2/file.c const struct file_operations ext2_file_operations = { · line 104

Quoted 6 lines

fs/ext2/file.c +1 -1

@@ -70,7 +101,7 @@ const struct file_operations ext2_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= ext2_compat_ioctl,
 #endif
-	.mmap		= generic_file_mmap,
+	.mmap		= ext2_file_mmap,

So what's the point of ext2_file_operations ever handling IS_DAX()
inodes? Actually ext2_file_operations and ext2_xip_file_operations seem to be the same after this patch so either you drop ext2_xip_file_operations (I'm for this) or you can leave generic_file_mmap here and assume ext2_file_mmap is always called for IS_DAX() inodes.

The goal is to get them the same. At this point, the only sticky point is:

.splice_read = generic_file_splice_read,
.splice_write = generic_file_splice_write,

And splice is pretty damn sticky for DAX.

This would be fine except that unmap_mapping_range() grabs i_mmap_mutex
again :-|. But it might be easier to provide a version of that function which assumes i_mmap_mutex is already locked than what I was suggesting.

Quoted 10 lines

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+int dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
+			get_block_t get_block)
+{
+	int result;
+	struct super_block *sb = file_inode(vma->vm_file)->i_sb;
+
+	sb_start_pagefault(sb);

You don't need any filesystem freeze protection for the fault handler
since that's not going to modify the filesystem.

Quoted 2 lines

Err ... we might allocate a block as a result of doing a write to a hole. Or does that not count as 'modifying the filesystem' in this context?

Ah, it does. But it would be nice to avoid doing sb_start_pagefault() if
it's not a write fault - because you don't want to block reading from a frozen filesystem (imagine what would happen when you freeze your root filesystem to do a snapshot...).

I have somewhat a mindset of standard pagecache mmap where filemap_fault() only reads in data regardless of FAULT_FLAG_WRITE setting so I was confused by your difference :).