[patch 1/1] sys_sync_file_range()

[patch 1/1] sys_sync_file_range()

[akpm]

From: Andrew Morton <akpm@osdl.org>

Remove the recently-added LINUX_FADV_ASYNC_WRITE and LINUX_FADV_WRITE_WAIT
fadvise() additions, do it in a new sys_sync_file_range() syscall instead. 
Reasons:

- It's more flexible.  Things which would require two or three syscalls with
  fadvise() can be done in a single syscall.

- Using fadvise() in this manner is something not covered by POSIX.


The patch wires up the syscall for x86.

The sycall is implemented in the new fs/sync.c.  The intention is that we can
move sys_fsync(), sys_fdatasync() and perhaps sys_sync() into there later.

Documentation for the syscall is in fs/sync.c.

A test app (sync_file_range.c) is in
http://www.zip.com.au/~akpm/linux/patches/stuff/ext3-tools.tar.gz.

Note: the `async' writeout mode SYNC_FILE_RANGE_WRITE will turn synchronous if
the queue is congested.  This is trivial to fix: add a new flag bit, set
wbc->nonblocking.  But I'm not sure that we want to expose implementation
details down to that level.

Note: it's notable that we can sync an fd which wasn't opened for writing. 
Same with fsync() and fdatasync()).

Note: the code takes some care to handle attempts to sync file contents
outside the 16TB offset on 32-bit machines.  It makes such attempts appears to
succeed, for best 32-bit/64-bit compatibility.  Perhaps it should make such
requests fail...


Cc: Nick Piggin <nickpiggin@yahoo.com.au>
Cc: Michael Kerrisk <mtk-manpages@gmx.net>
Cc: Ulrich Drepper <drepper@redhat.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 arch/i386/kernel/syscall_table.S |    1 
 fs/Makefile                      |    2 
 fs/sync.c                        |  147 +++++++++++++++++++++++++++++
 include/asm-i386/unistd.h        |    3 
 include/linux/fadvise.h          |    6 -
 include/linux/fs.h               |    5 
 include/linux/syscalls.h         |    2 
 mm/fadvise.c                     |   20 ---
 8 files changed, 158 insertions(+), 28 deletions(-)

diff -puN mm/fadvise.c~sys_sync_file_range mm/fadvise.c
--- devel/mm/fadvise.c~sys_sync_file_range	2006-03-29 23:26:42.000000000 -0800
+++ devel-akpm/mm/fadvise.c	2006-03-29 23:26:42.000000000 -0800
@@ -35,17 +35,6 @@
  *
  * LINUX_FADV_ASYNC_WRITE: push some or all of the dirty pages at the disk.
  *
- * LINUX_FADV_WRITE_WAIT, LINUX_FADV_ASYNC_WRITE: push all of the currently
- * dirty pages at the disk.
- *
- * LINUX_FADV_WRITE_WAIT, LINUX_FADV_ASYNC_WRITE, LINUX_FADV_WRITE_WAIT: push
- * all of the currently dirty pages at the disk, wait until they have been
- * written.
- *
- * It should be noted that none of these operations write out the file's
- * metadata.  So unless the application is strictly performing overwrites of
- * already-instantiated disk blocks, there are no guarantees here that the data
- * will be available after a crash.
  */
 asmlinkage long sys_fadvise64_64(int fd, loff_t offset, loff_t len, int advice)
 {
@@ -129,15 +118,6 @@ asmlinkage long sys_fadvise64_64(int fd,
 			invalidate_mapping_pages(mapping, start_index,
 						end_index);
 		break;
-	case LINUX_FADV_ASYNC_WRITE:
-		ret = __filemap_fdatawrite_range(mapping, offset, endbyte,
-						WB_SYNC_NONE);
-		break;
-	case LINUX_FADV_WRITE_WAIT:
-		ret = wait_on_page_writeback_range(mapping,
-					offset >> PAGE_CACHE_SHIFT,
-					endbyte >> PAGE_CACHE_SHIFT);
-		break;
 	default:
 		ret = -EINVAL;
 	}
diff -puN include/linux/fadvise.h~sys_sync_file_range include/linux/fadvise.h
--- devel/include/linux/fadvise.h~sys_sync_file_range	2006-03-29 23:26:42.000000000 -0800
+++ devel-akpm/include/linux/fadvise.h	2006-03-29 23:26:42.000000000 -0800
@@ -18,10 +18,4 @@
 #define POSIX_FADV_NOREUSE	5 /* Data will be accessed once.  */
 #endif
 
-/*
- * Linux-specific fadvise() extensions:
- */
-#define LINUX_FADV_ASYNC_WRITE	32	/* Start writeout on range */
-#define LINUX_FADV_WRITE_WAIT	33	/* Wait upon writeout to range */
-
 #endif	/* FADVISE_H_INCLUDED */
diff -puN /dev/null fs/sync.c
--- /dev/null	2003-09-15 06:40:47.000000000 -0700
+++ devel-akpm/fs/sync.c	2006-03-29 23:26:42.000000000 -0800
@@ -0,0 +1,147 @@
+/*
+ * High-level sync()-related operations
+ */
+
+#include <linux/kernel.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/writeback.h>
+#include <linux/syscalls.h>
+#include <linux/linkage.h>
+#include <linux/pagemap.h>
+
+#define VALID_FLAGS (SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE| \
+			SYNC_FILE_RANGE_WAIT_AFTER)
+
+/*
+ * sys_sync_file_range() permits finely controlled syncing over a segment of
+ * a file in the range offset .. (offset+nbytes-1) inclusive.  If nbytes is
+ * zero then sys_sync_file_range() will operate from offset out to EOF.
+ *
+ * The flag bits are:
+ *
+ * SYNC_FILE_RANGE_WAIT_BEFORE: wait upon writeout of all pages in the range
+ * before performing the write.
+ *
+ * SYNC_FILE_RANGE_WRITE: initiate writeout of all those dirty pages in the
+ * range which are not presently under writeback.
+ *
+ * SYNC_FILE_RANGE_WAIT_AFTER: wait upon writeout of all pages in the range
+ * after performing the write.
+ *
+ * Useful combinations of the flag bits are:
+ *
+ * SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE: ensures that all pages
+ * in the range which were dirty on entry to sys_sync_file_range() are placed
+ * under writeout.  This is a start-write-for-data-integrity operation.
+ *
+ * SYNC_FILE_RANGE_WRITE: start writeout of all dirty pages in the range which
+ * are not presently under writeout.  This is an asynchronous flush-to-disk
+ * operation.  Not suitable for data integrity operations.
+ *
+ * SYNC_FILE_RANGE_WAIT_BEFORE (or SYNC_FILE_RANGE_WAIT_AFTER): wait for
+ * completion of writeout of all pages in the range.  This will be used after an
+ * earlier SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE operation to wait
+ * for that operation to complete and to return the result.
+ *
+ * SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE|SYNC_FILE_RANGE_WAIT_AFTER:
+ * a traditional sync() operation.  This is a write-for-data-integrity operation
+ * which will ensure that all pages in the range which were dirty on entry to
+ * sys_sync_file_range() are committed to disk.
+ *
+ *
+ * SYNC_FILE_RANGE_WAIT_BEFORE and SYNC_FILE_RANGE_WAIT_AFTER will detect any
+ * I/O errors or ENOSPC conditions and will return those to the caller, after
+ * clearing the EIO and ENOSPC flags in the address_space.
+ *
+ * It should be noted that none of these operations write out the file's
+ * metadata.  So unless the application is strictly performing overwrites of
+ * already-instantiated disk blocks, there are no guarantees here that the data
+ * will be available after a crash.
+ */
+asmlinkage long sys_sync_file_range(int fd, loff_t offset, loff_t nbytes,
+					int flags)
+{
+	int ret;
+	struct file *file;
+	struct address_space *mapping;
+	loff_t endbyte;			/* inclusive */
+
+	ret = -EINVAL;
+	if (flags & ~VALID_FLAGS)
+		goto out;
+
+	endbyte = offset + nbytes;
+
+	if ((s64)offset < 0)
+		goto out;
+	if ((s64)endbyte < 0)
+		goto out;
+	if (endbyte < offset)
+		goto out;
+
+	if (sizeof(pgoff_t) == 4) {
+		if (offset >= (0x100000000ULL << PAGE_CACHE_SHIFT)) {
+			/*
+			 * The range starts outside a 32 bit machine's
+			 * pagecache addressing capabilities.  Let it "succeed"
+			 */
+			ret = 0;
+			goto out;
+		}
+		if (endbyte >= (0x100000000ULL << PAGE_CACHE_SHIFT)) {
+			/*
+			 * Out to EOF
+			 */
+			nbytes = 0;
+		}
+	}
+
+	if (nbytes == 0)
+		endbyte = -1;
+	else
+		endbyte--;		/* inclusive */
+
+	file = fget(fd);
+	if (!file) {
+		ret = -EBADF;
+		goto out;
+	}
+
+	if (S_ISFIFO(file->f_dentry->d_inode->i_mode)) {
+		ret = -ESPIPE;
+		goto out_put;
+	}
+
+	mapping = file->f_mapping;
+	if (!mapping) {
+		ret = -EINVAL;
+		goto out_put;
+	}
+
+	ret = 0;
+	if (flags & SYNC_FILE_RANGE_WAIT_BEFORE) {
+		ret = wait_on_page_writeback_range(mapping,
+					offset >> PAGE_CACHE_SHIFT,
+					endbyte >> PAGE_CACHE_SHIFT);
+		if (ret < 0)
+			goto out_put;
+	}
+
+	if (flags & SYNC_FILE_RANGE_WRITE) {
+		ret = __filemap_fdatawrite_range(mapping, offset, endbyte,
+						WB_SYNC_NONE);
+		if (ret < 0)
+			goto out_put;
+	}
+
+	if (flags & SYNC_FILE_RANGE_WAIT_AFTER) {
+		ret = wait_on_page_writeback_range(mapping,
+					offset >> PAGE_CACHE_SHIFT,
+					endbyte >> PAGE_CACHE_SHIFT);
+	}
+out_put:
+	fput(file);
+out:
+	return ret;
+}
diff -puN include/linux/syscalls.h~sys_sync_file_range include/linux/syscalls.h
--- devel/include/linux/syscalls.h~sys_sync_file_range	2006-03-29 23:26:42.000000000 -0800
+++ devel-akpm/include/linux/syscalls.h	2006-03-29 23:40:57.000000000 -0800
@@ -569,5 +569,7 @@ asmlinkage long compat_sys_newfstatat(un
 asmlinkage long compat_sys_openat(unsigned int dfd, const char __user *filename,
 				   int flags, int mode);
 asmlinkage long sys_unshare(unsigned long unshare_flags);
+asmlinkage long sys_sync_file_range(int fd, loff_t offset, loff_t nbytes,
+					int flags);
 
 #endif
diff -puN include/linux/fs.h~sys_sync_file_range include/linux/fs.h
--- devel/include/linux/fs.h~sys_sync_file_range	2006-03-29 23:26:42.000000000 -0800
+++ devel-akpm/include/linux/fs.h	2006-03-29 23:40:55.000000000 -0800
@@ -757,6 +757,11 @@ extern void send_sigio(struct fown_struc
 extern int fcntl_setlease(unsigned int fd, struct file *filp, long arg);
 extern int fcntl_getlease(struct file *filp);
 
+/* fs/sync.c */
+#define SYNC_FILE_RANGE_WAIT_BEFORE	1
+#define SYNC_FILE_RANGE_WRITE		2
+#define SYNC_FILE_RANGE_WAIT_AFTER	4
+
 /* fs/locks.c */
 extern void locks_init_lock(struct file_lock *);
 extern void locks_copy_lock(struct file_lock *, struct file_lock *);
diff -puN fs/Makefile~sys_sync_file_range fs/Makefile
--- devel/fs/Makefile~sys_sync_file_range	2006-03-29 23:26:42.000000000 -0800
+++ devel-akpm/fs/Makefile	2006-03-29 23:40:54.000000000 -0800
@@ -10,7 +10,7 @@ obj-y :=	open.o read_write.o file_table.
 		ioctl.o readdir.o select.o fifo.o locks.o dcache.o inode.o \
 		attr.o bad_inode.o file.o filesystems.o namespace.o aio.o \
 		seq_file.o xattr.o libfs.o fs-writeback.o mpage.o direct-io.o \
-		ioprio.o pnode.o drop_caches.o
+		ioprio.o pnode.o drop_caches.o sync.o
 
 obj-$(CONFIG_INOTIFY)		+= inotify.o
 obj-$(CONFIG_EPOLL)		+= eventpoll.o
diff -puN include/asm-i386/unistd.h~sys_sync_file_range include/asm-i386/unistd.h
--- devel/include/asm-i386/unistd.h~sys_sync_file_range	2006-03-29 23:26:42.000000000 -0800
+++ devel-akpm/include/asm-i386/unistd.h	2006-03-29 23:26:42.000000000 -0800
@@ -318,8 +318,9 @@
 #define __NR_unshare		310
 #define __NR_set_robust_list	311
 #define __NR_get_robust_list	312
+#define __NR_sys_sync_file_range 313
 
-#define NR_syscalls 313
+#define NR_syscalls 314
 
 /*
  * user-visible error numbers are in the range -1 - -128: see
diff -puN arch/i386/kernel/syscall_table.S~sys_sync_file_range arch/i386/kernel/syscall_table.S
--- devel/arch/i386/kernel/syscall_table.S~sys_sync_file_range	2006-03-29 23:26:42.000000000 -0800
+++ devel-akpm/arch/i386/kernel/syscall_table.S	2006-03-29 23:26:42.000000000 -0800
@@ -312,3 +312,4 @@ ENTRY(sys_call_table)
 	.long sys_unshare		/* 310 */
 	.long sys_set_robust_list
 	.long sys_get_robust_list
+	.long sys_sync_file_range
_

Re: [patch 1/1] sys_sync_file_range()

[Neil Brown]

On Wednesday March 29, akpm@osdl.org wrote:
> 
> From: Andrew Morton <akpm@osdl.org>
> 
> Remove the recently-added LINUX_FADV_ASYNC_WRITE and LINUX_FADV_WRITE_WAIT
> fadvise() additions, do it in a new sys_sync_file_range() syscall
> instead. 

Hmmm... any chance this could be split into a sys_sync_file_range and
a vfs_sync_file_range which takes a 'struct file*' and does less (or
no) sanity checking, so I can call it from nfsd?

Currently I implement COMMIT (which has a range) with a by messing
around with filemap_fdatawrite and filemap_fdatawait (ignoring the
range) and I'd rather than a vfs helper.

And in nfsd I call filp->f_op->fsync between the two.  Doesn't
sys_sync_file_range need to call into the filesystem at all?

NeilBrown

Re: [patch 1/1] sys_sync_file_range()

[Andrew Morton]

Neil Brown <neilb@suse.de> wrote:
>
> On Wednesday March 29, akpm@osdl.org wrote:
> > 
> > From: Andrew Morton <akpm@osdl.org>
> > 
> > Remove the recently-added LINUX_FADV_ASYNC_WRITE and LINUX_FADV_WRITE_WAIT
> > fadvise() additions, do it in a new sys_sync_file_range() syscall
> > instead. 
> 
> Hmmm... any chance this could be split into a sys_sync_file_range and
> a vfs_sync_file_range which takes a 'struct file*' and does less (or
> no) sanity checking, so I can call it from nfsd?

Coming right up.  (Will switch it to fget_light() too)

> Currently I implement COMMIT (which has a range) with a by messing
> around with filemap_fdatawrite and filemap_fdatawait (ignoring the
> range) and I'd rather than a vfs helper.
> 
> And in nfsd I call filp->f_op->fsync between the two.  Doesn't
> sys_sync_file_range need to call into the filesystem at all?

Interesting question.  sync_file_range() is purely a pagecache (ie: file
contents) operation.  It doesn't touch metadata at all.

So if it's being used for data-integrity purposes then it really only makes
sense when it's doing file overwrites.

It does call into the filesystem of course - a_ops.writepages() and perhaps
a_ops.writepage().

Re: [patch 1/1] sys_sync_file_range()

[Andrew Morton]

Neil Brown <neilb@suse.de> wrote:
>
> Hmmm... any chance this could be split into a sys_sync_file_range and
>  a vfs_sync_file_range which takes a 'struct file*' and does less (or
>  no) sanity checking, so I can call it from nfsd?

Problem is, we don't appear to have a way of syncing the file's metadata
without also syncing all of its pagecache.

For example, ext3_sync_file() will run a commit, which will sync all data
and metadata.

ext2_sync_file() will also sync all pagecache as well as metadata.  Even
though do_fsync() already synced the file data (!).


Is the below still useful?


diff -puN mm/fadvise.c~sys_sync_file_range mm/fadvise.c
--- devel/mm/fadvise.c~sys_sync_file_range	2006-03-29 23:48:28.000000000 -0800
+++ devel-akpm/mm/fadvise.c	2006-03-29 23:48:28.000000000 -0800
@@ -35,17 +35,6 @@
  *
  * LINUX_FADV_ASYNC_WRITE: push some or all of the dirty pages at the disk.
  *
- * LINUX_FADV_WRITE_WAIT, LINUX_FADV_ASYNC_WRITE: push all of the currently
- * dirty pages at the disk.
- *
- * LINUX_FADV_WRITE_WAIT, LINUX_FADV_ASYNC_WRITE, LINUX_FADV_WRITE_WAIT: push
- * all of the currently dirty pages at the disk, wait until they have been
- * written.
- *
- * It should be noted that none of these operations write out the file's
- * metadata.  So unless the application is strictly performing overwrites of
- * already-instantiated disk blocks, there are no guarantees here that the data
- * will be available after a crash.
  */
 asmlinkage long sys_fadvise64_64(int fd, loff_t offset, loff_t len, int advice)
 {
@@ -129,15 +118,6 @@ asmlinkage long sys_fadvise64_64(int fd,
 			invalidate_mapping_pages(mapping, start_index,
 						end_index);
 		break;
-	case LINUX_FADV_ASYNC_WRITE:
-		ret = __filemap_fdatawrite_range(mapping, offset, endbyte,
-						WB_SYNC_NONE);
-		break;
-	case LINUX_FADV_WRITE_WAIT:
-		ret = wait_on_page_writeback_range(mapping,
-					offset >> PAGE_CACHE_SHIFT,
-					endbyte >> PAGE_CACHE_SHIFT);
-		break;
 	default:
 		ret = -EINVAL;
 	}
diff -puN include/linux/fadvise.h~sys_sync_file_range include/linux/fadvise.h
--- devel/include/linux/fadvise.h~sys_sync_file_range	2006-03-29 23:48:28.000000000 -0800
+++ devel-akpm/include/linux/fadvise.h	2006-03-29 23:48:28.000000000 -0800
@@ -18,10 +18,4 @@
 #define POSIX_FADV_NOREUSE	5 /* Data will be accessed once.  */
 #endif
 
-/*
- * Linux-specific fadvise() extensions:
- */
-#define LINUX_FADV_ASYNC_WRITE	32	/* Start writeout on range */
-#define LINUX_FADV_WRITE_WAIT	33	/* Wait upon writeout to range */
-
 #endif	/* FADVISE_H_INCLUDED */
diff -puN /dev/null fs/sync.c
--- /dev/null	2003-09-15 06:40:47.000000000 -0700
+++ devel-akpm/fs/sync.c	2006-03-30 00:16:37.000000000 -0800
@@ -0,0 +1,163 @@
+/*
+ * High-level sync()-related operations
+ */
+
+#include <linux/kernel.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/module.h>
+#include <linux/writeback.h>
+#include <linux/syscalls.h>
+#include <linux/linkage.h>
+#include <linux/pagemap.h>
+
+#define VALID_FLAGS (SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE| \
+			SYNC_FILE_RANGE_WAIT_AFTER)
+
+/*
+ * sys_sync_file_range() permits finely controlled syncing over a segment of
+ * a file in the range offset .. (offset+nbytes-1) inclusive.  If nbytes is
+ * zero then sys_sync_file_range() will operate from offset out to EOF.
+ *
+ * The flag bits are:
+ *
+ * SYNC_FILE_RANGE_WAIT_BEFORE: wait upon writeout of all pages in the range
+ * before performing the write.
+ *
+ * SYNC_FILE_RANGE_WRITE: initiate writeout of all those dirty pages in the
+ * range which are not presently under writeback.
+ *
+ * SYNC_FILE_RANGE_WAIT_AFTER: wait upon writeout of all pages in the range
+ * after performing the write.
+ *
+ * Useful combinations of the flag bits are:
+ *
+ * SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE: ensures that all pages
+ * in the range which were dirty on entry to sys_sync_file_range() are placed
+ * under writeout.  This is a start-write-for-data-integrity operation.
+ *
+ * SYNC_FILE_RANGE_WRITE: start writeout of all dirty pages in the range which
+ * are not presently under writeout.  This is an asynchronous flush-to-disk
+ * operation.  Not suitable for data integrity operations.
+ *
+ * SYNC_FILE_RANGE_WAIT_BEFORE (or SYNC_FILE_RANGE_WAIT_AFTER): wait for
+ * completion of writeout of all pages in the range.  This will be used after an
+ * earlier SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE operation to wait
+ * for that operation to complete and to return the result.
+ *
+ * SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE|SYNC_FILE_RANGE_WAIT_AFTER:
+ * a traditional sync() operation.  This is a write-for-data-integrity operation
+ * which will ensure that all pages in the range which were dirty on entry to
+ * sys_sync_file_range() are committed to disk.
+ *
+ *
+ * SYNC_FILE_RANGE_WAIT_BEFORE and SYNC_FILE_RANGE_WAIT_AFTER will detect any
+ * I/O errors or ENOSPC conditions and will return those to the caller, after
+ * clearing the EIO and ENOSPC flags in the address_space.
+ *
+ * It should be noted that none of these operations write out the file's
+ * metadata.  So unless the application is strictly performing overwrites of
+ * already-instantiated disk blocks, there are no guarantees here that the data
+ * will be available after a crash.
+ */
+asmlinkage long sys_sync_file_range(int fd, loff_t offset, loff_t nbytes,
+					int flags)
+{
+	int ret;
+	struct file *file;
+	loff_t endbyte;			/* inclusive */
+	int fput_needed;
+
+	ret = -EINVAL;
+	if (flags & ~VALID_FLAGS)
+		goto out;
+
+	endbyte = offset + nbytes;
+
+	if ((s64)offset < 0)
+		goto out;
+	if ((s64)endbyte < 0)
+		goto out;
+	if (endbyte < offset)
+		goto out;
+
+	if (sizeof(pgoff_t) == 4) {
+		if (offset >= (0x100000000ULL << PAGE_CACHE_SHIFT)) {
+			/*
+			 * The range starts outside a 32 bit machine's
+			 * pagecache addressing capabilities.  Let it "succeed"
+			 */
+			ret = 0;
+			goto out;
+		}
+		if (endbyte >= (0x100000000ULL << PAGE_CACHE_SHIFT)) {
+			/*
+			 * Out to EOF
+			 */
+			nbytes = 0;
+		}
+	}
+
+	if (nbytes == 0)
+		endbyte = -1;
+	else
+		endbyte--;		/* inclusive */
+
+	file = fget_light(fd, &fput_needed);
+	if (!file) {
+		ret = -EBADF;
+		goto out;
+	}
+
+	if (S_ISFIFO(file->f_dentry->d_inode->i_mode)) {
+		ret = -ESPIPE;
+		goto out_put;
+	}
+
+	ret = do_sync_file_range(file, offset, endbyte, flags);
+out_put:
+	fput_light(file, fput_needed);
+out:
+	return ret;
+}
+
+/*
+ * `endbyte' is inclusive
+ */
+int do_sync_file_range(struct file *file, loff_t offset, loff_t endbyte,
+			int flags)
+{
+	int ret;
+	struct address_space *mapping;
+
+	mapping = file->f_mapping;
+	if (!mapping) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = 0;
+	if (flags & SYNC_FILE_RANGE_WAIT_BEFORE) {
+		ret = wait_on_page_writeback_range(mapping,
+					offset >> PAGE_CACHE_SHIFT,
+					endbyte >> PAGE_CACHE_SHIFT);
+		if (ret < 0)
+			goto out;
+	}
+
+	if (flags & SYNC_FILE_RANGE_WRITE) {
+		ret = __filemap_fdatawrite_range(mapping, offset, endbyte,
+						WB_SYNC_NONE);
+		if (ret < 0)
+			goto out;
+	}
+
+	if (flags & SYNC_FILE_RANGE_WAIT_AFTER) {
+		ret = wait_on_page_writeback_range(mapping,
+					offset >> PAGE_CACHE_SHIFT,
+					endbyte >> PAGE_CACHE_SHIFT);
+	}
+out:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(do_sync_file_range);
diff -puN include/linux/syscalls.h~sys_sync_file_range include/linux/syscalls.h
--- devel/include/linux/syscalls.h~sys_sync_file_range	2006-03-29 23:48:28.000000000 -0800
+++ devel-akpm/include/linux/syscalls.h	2006-03-30 00:11:50.000000000 -0800
@@ -569,5 +569,7 @@ asmlinkage long compat_sys_newfstatat(un
 asmlinkage long compat_sys_openat(unsigned int dfd, const char __user *filename,
 				   int flags, int mode);
 asmlinkage long sys_unshare(unsigned long unshare_flags);
+asmlinkage long sys_sync_file_range(int fd, loff_t offset, loff_t nbytes,
+					int flags);
 
 #endif
diff -puN include/linux/fs.h~sys_sync_file_range include/linux/fs.h
--- devel/include/linux/fs.h~sys_sync_file_range	2006-03-29 23:48:28.000000000 -0800
+++ devel-akpm/include/linux/fs.h	2006-03-30 00:15:44.000000000 -0800
@@ -757,6 +757,13 @@ extern void send_sigio(struct fown_struc
 extern int fcntl_setlease(unsigned int fd, struct file *filp, long arg);
 extern int fcntl_getlease(struct file *filp);
 
+/* fs/sync.c */
+#define SYNC_FILE_RANGE_WAIT_BEFORE	1
+#define SYNC_FILE_RANGE_WRITE		2
+#define SYNC_FILE_RANGE_WAIT_AFTER	4
+extern int do_sync_file_range(struct file *file, loff_t offset, loff_t endbyte,
+			int flags);
+
 /* fs/locks.c */
 extern void locks_init_lock(struct file_lock *);
 extern void locks_copy_lock(struct file_lock *, struct file_lock *);
diff -puN fs/Makefile~sys_sync_file_range fs/Makefile
--- devel/fs/Makefile~sys_sync_file_range	2006-03-29 23:48:28.000000000 -0800
+++ devel-akpm/fs/Makefile	2006-03-30 00:11:47.000000000 -0800
@@ -10,7 +10,7 @@ obj-y :=	open.o read_write.o file_table.
 		ioctl.o readdir.o select.o fifo.o locks.o dcache.o inode.o \
 		attr.o bad_inode.o file.o filesystems.o namespace.o aio.o \
 		seq_file.o xattr.o libfs.o fs-writeback.o mpage.o direct-io.o \
-		ioprio.o pnode.o drop_caches.o
+		ioprio.o pnode.o drop_caches.o sync.o
 
 obj-$(CONFIG_INOTIFY)		+= inotify.o
 obj-$(CONFIG_EPOLL)		+= eventpoll.o
diff -puN include/asm-i386/unistd.h~sys_sync_file_range include/asm-i386/unistd.h
--- devel/include/asm-i386/unistd.h~sys_sync_file_range	2006-03-29 23:48:28.000000000 -0800
+++ devel-akpm/include/asm-i386/unistd.h	2006-03-29 23:48:28.000000000 -0800
@@ -318,8 +318,9 @@
 #define __NR_unshare		310
 #define __NR_set_robust_list	311
 #define __NR_get_robust_list	312
+#define __NR_sys_sync_file_range 313
 
-#define NR_syscalls 313
+#define NR_syscalls 314
 
 /*
  * user-visible error numbers are in the range -1 - -128: see
diff -puN arch/i386/kernel/syscall_table.S~sys_sync_file_range arch/i386/kernel/syscall_table.S
--- devel/arch/i386/kernel/syscall_table.S~sys_sync_file_range	2006-03-29 23:48:28.000000000 -0800
+++ devel-akpm/arch/i386/kernel/syscall_table.S	2006-03-29 23:48:28.000000000 -0800
@@ -312,3 +312,4 @@ ENTRY(sys_call_table)
 	.long sys_unshare		/* 310 */
 	.long sys_set_robust_list
 	.long sys_get_robust_list
+	.long sys_sync_file_range
_

Re: [patch 1/1] sys_sync_file_range()

[Neil Brown]

On Thursday March 30, akpm@osdl.org wrote:
> Neil Brown <neilb@suse.de> wrote:
> >
> > Hmmm... any chance this could be split into a sys_sync_file_range and
> >  a vfs_sync_file_range which takes a 'struct file*' and does less (or
> >  no) sanity checking, so I can call it from nfsd?
> 
> Problem is, we don't appear to have a way of syncing the file's metadata
> without also syncing all of its pagecache.
> 
> For example, ext3_sync_file() will run a commit, which will sync all data
> and metadata.
> 
> ext2_sync_file() will also sync all pagecache as well as metadata.  Even
> though do_fsync() already synced the file data (!).
> 
> 
> Is the below still useful?

Yes.  A COMMIT can say NFS_DATA_SYNC or NFS_FILE_SYNC.  I can skip the
->fsync call for NFS_DATA_SYNC which is hopefully the more common.
I didn't before because when I was righting that (2.4 days) there were
a lot fewer options on how to sync things.

So yes, that will be perfect, thanks.

NeilBrown

Re: [patch 1/1] sys_sync_file_range()

[OGAWA Hirofumi]

Andrew Morton <akpm@osdl.org> writes:

> +	if ((s64)offset < 0)
> +		goto out;
> +	if ((s64)endbyte < 0)
> +		goto out;

loff_t is long long on all arch. This is not need?

> +	if (S_ISFIFO(file->f_dentry->d_inode->i_mode)) {
> +		ret = -ESPIPE;
> +		goto out_put;
> +	}

How about to check "if (!file->f_op || !file->f_op->fsync)" or something?

For chrdev is also strange, and in the case of fsync(), it returns -EINVAL.
IMHO it seems there is consistency.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [patch 1/1] sys_sync_file_range()

[Andrew Morton]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
>
> Andrew Morton <akpm@osdl.org> writes:
> 
> > +	if ((s64)offset < 0)
> > +		goto out;
> > +	if ((s64)endbyte < 0)
> > +		goto out;
> 
> loff_t is long long on all arch. This is not need?

True, the casts happen to be unneeded.  But they do set the reader's mind
at ease.

> > +	if (S_ISFIFO(file->f_dentry->d_inode->i_mode)) {
> > +		ret = -ESPIPE;
> > +		goto out_put;
> > +	}
> 
> How about to check "if (!file->f_op || !file->f_op->fsync)" or something?

This syscall won't call ->fsync.

> For chrdev is also strange, and in the case of fsync(), it returns -EINVAL.
> IMHO it seems there is consistency.
> 

I guess so.   Perhaps it should be S_ISREG|S_ISBLK|S_ISDIR|S_ISLNK.

Re: [patch 1/1] sys_sync_file_range()

[Nathan Scott]

On Thu, Mar 30, 2006 at 06:58:46PM +1100, Neil Brown wrote:
> On Wednesday March 29, akpm@osdl.org wrote:
> > Remove the recently-added LINUX_FADV_ASYNC_WRITE and LINUX_FADV_WRITE_WAIT
> > fadvise() additions, do it in a new sys_sync_file_range() syscall
> > instead. 
> 
> Hmmm... any chance this could be split into a sys_sync_file_range and
> a vfs_sync_file_range which takes a 'struct file*' and does less (or
> no) sanity checking, so I can call it from nfsd?
> 
> Currently I implement COMMIT (which has a range) with a by messing
> around with filemap_fdatawrite and filemap_fdatawait (ignoring the
> range) and I'd rather than a vfs helper.

I'm not 100% sure, but it looks like the PF_SYNCWRITE process flag
should be set on the nfsd's while they're doing that, which doesn't
seem to be happening atm.  Looks like a couple of the IO schedulers
will make use of that knowledge now.  All the more reason for a VFS
helper here I guess. ;)

cheers.

-- 
Nathan


Index: 2.6.x-xfs/fs/nfsd/vfs.c
===================================================================
--- 2.6.x-xfs.orig/fs/nfsd/vfs.c
+++ 2.6.x-xfs/fs/nfsd/vfs.c
@@ -712,11 +712,13 @@ static inline int nfsd_dosync(struct fil
 	int (*fsync) (struct file *, struct dentry *, int);
 	int err;
 
+	current->flags |= PF_SYNCWRITE;
 	err = filemap_fdatawrite(inode->i_mapping);
 	if (err == 0 && fop && (fsync = fop->fsync))
 		err = fsync(filp, dp, 0);
 	if (err == 0)
 		err = filemap_fdatawait(inode->i_mapping);
+	current->flags &= ~PF_SYNCWRITE;
 
 	return err;
 }

Re: [patch 1/1] sys_sync_file_range()

[Neil Brown]

On Friday March 31, nathans@sgi.com wrote:
> On Thu, Mar 30, 2006 at 06:58:46PM +1100, Neil Brown wrote:
> > On Wednesday March 29, akpm@osdl.org wrote:
> > > Remove the recently-added LINUX_FADV_ASYNC_WRITE and LINUX_FADV_WRITE_WAIT
> > > fadvise() additions, do it in a new sys_sync_file_range() syscall
> > > instead. 
> > 
> > Hmmm... any chance this could be split into a sys_sync_file_range and
> > a vfs_sync_file_range which takes a 'struct file*' and does less (or
> > no) sanity checking, so I can call it from nfsd?
> > 
> > Currently I implement COMMIT (which has a range) with a by messing
> > around with filemap_fdatawrite and filemap_fdatawait (ignoring the
> > range) and I'd rather than a vfs helper.
> 
> I'm not 100% sure, but it looks like the PF_SYNCWRITE process flag
> should be set on the nfsd's while they're doing that, which doesn't
> seem to be happening atm.  Looks like a couple of the IO schedulers
> will make use of that knowledge now.  All the more reason for a VFS
> helper here I guess. ;)

PF_SYNCWRITE? What's that???

(find | xargs grep ...)
Oh.  The block device schedulers like to know if a request is sync or
async (and all reads are assumed to be sync) - which is reasonable -
and so have a per-task flag to tell them - which isn't (IMO).

md/raid (particularly raid5) often does the write from a different
process than generated the original request, so that will break
completely. 

What is wrong with a bio flag I wonder....

Hopefully this will get wrapped up in a do_X helper so nfsd won't need
to know about it.

Thanks for the heads-up.

NeilBrown

Re: [patch 1/1] sys_sync_file_range()

[Jens Axboe]

On Mon, Apr 03 2006, Neil Brown wrote:
> On Friday March 31, nathans@sgi.com wrote:
> > On Thu, Mar 30, 2006 at 06:58:46PM +1100, Neil Brown wrote:
> > > On Wednesday March 29, akpm@osdl.org wrote:
> > > > Remove the recently-added LINUX_FADV_ASYNC_WRITE and LINUX_FADV_WRITE_WAIT
> > > > fadvise() additions, do it in a new sys_sync_file_range() syscall
> > > > instead. 
> > > 
> > > Hmmm... any chance this could be split into a sys_sync_file_range and
> > > a vfs_sync_file_range which takes a 'struct file*' and does less (or
> > > no) sanity checking, so I can call it from nfsd?
> > > 
> > > Currently I implement COMMIT (which has a range) with a by messing
> > > around with filemap_fdatawrite and filemap_fdatawait (ignoring the
> > > range) and I'd rather than a vfs helper.
> > 
> > I'm not 100% sure, but it looks like the PF_SYNCWRITE process flag
> > should be set on the nfsd's while they're doing that, which doesn't
> > seem to be happening atm.  Looks like a couple of the IO schedulers
> > will make use of that knowledge now.  All the more reason for a VFS
> > helper here I guess. ;)
> 
> PF_SYNCWRITE? What's that???
> 
> (find | xargs grep ...)
> Oh.  The block device schedulers like to know if a request is sync or
> async (and all reads are assumed to be sync) - which is reasonable -
> and so have a per-task flag to tell them - which isn't (IMO).
> 
> md/raid (particularly raid5) often does the write from a different
> process than generated the original request, so that will break
> completely. 

I don't think any disagrees with you, the sync-write process flag is
indeed an atrocious beast...

> What is wrong with a bio flag I wonder....

Nothing, in fact I would love for it to be changed. I'm sure such a
patch would be accepted with open arms! :-)

-- 
Jens Axboe

Re: [patch 1/1] sys_sync_file_range()

[Andrew Morton]

Jens Axboe <axboe@suse.de> wrote:
>
> On Mon, Apr 03 2006, Neil Brown wrote:
> > On Friday March 31, nathans@sgi.com wrote:
> > > On Thu, Mar 30, 2006 at 06:58:46PM +1100, Neil Brown wrote:
> > > > On Wednesday March 29, akpm@osdl.org wrote:
> > > > > Remove the recently-added LINUX_FADV_ASYNC_WRITE and LINUX_FADV_WRITE_WAIT
> > > > > fadvise() additions, do it in a new sys_sync_file_range() syscall
> > > > > instead. 
> > > > 
> > > > Hmmm... any chance this could be split into a sys_sync_file_range and
> > > > a vfs_sync_file_range which takes a 'struct file*' and does less (or
> > > > no) sanity checking, so I can call it from nfsd?
> > > > 
> > > > Currently I implement COMMIT (which has a range) with a by messing
> > > > around with filemap_fdatawrite and filemap_fdatawait (ignoring the
> > > > range) and I'd rather than a vfs helper.
> > > 
> > > I'm not 100% sure, but it looks like the PF_SYNCWRITE process flag
> > > should be set on the nfsd's while they're doing that, which doesn't
> > > seem to be happening atm.  Looks like a couple of the IO schedulers
> > > will make use of that knowledge now.  All the more reason for a VFS
> > > helper here I guess. ;)
> > 
> > PF_SYNCWRITE? What's that???
> > 
> > (find | xargs grep ...)
> > Oh.  The block device schedulers like to know if a request is sync or
> > async (and all reads are assumed to be sync) - which is reasonable -
> > and so have a per-task flag to tell them - which isn't (IMO).
> > 
> > md/raid (particularly raid5) often does the write from a different
> > process than generated the original request, so that will break
> > completely. 
> 
> I don't think any disagrees with you, the sync-write process flag is
> indeed an atrocious beast...

Yeah.  PF_SYNCWRITE was a performance tweak for the anticipatory scheduler.
As cfq is using it as well now (hopefully to good effect) I guess it could
be formalised more.

> > What is wrong with a bio flag I wonder....
> 
> Nothing, in fact I would love for it to be changed. I'm sure such a
> patch would be accepted with open arms! :-)

I think once someone starts coding it, they'll become a big fan of
PF_SYNCWRITE...

For the page writeback functions it's probably possible to use
writeback_control.sync_mode=WB_SYNC_ALL as a trigger, propagate that into
the IO layer.  Maybe that'll always be sufficient - it's hard to tell.  The
writeback paths are twisty and deep...

Then again, using WB_SYNC_ALL as a hint that this process will be waiting
for this writeout to complete is a bit hacky too - it doesn't _really_ mean
that - it just means that I/O should be _started_ against all relevant
dirty data.

Good luck ;)

Re: [patch 1/1] sys_sync_file_range()

[Jens Axboe]

On Mon, Apr 03 2006, Andrew Morton wrote:
> Jens Axboe <axboe@suse.de> wrote:
> >
> > On Mon, Apr 03 2006, Neil Brown wrote:
> > > On Friday March 31, nathans@sgi.com wrote:
> > > > On Thu, Mar 30, 2006 at 06:58:46PM +1100, Neil Brown wrote:
> > > > > On Wednesday March 29, akpm@osdl.org wrote:
> > > > > > Remove the recently-added LINUX_FADV_ASYNC_WRITE and LINUX_FADV_WRITE_WAIT
> > > > > > fadvise() additions, do it in a new sys_sync_file_range() syscall
> > > > > > instead. 
> > > > > 
> > > > > Hmmm... any chance this could be split into a sys_sync_file_range and
> > > > > a vfs_sync_file_range which takes a 'struct file*' and does less (or
> > > > > no) sanity checking, so I can call it from nfsd?
> > > > > 
> > > > > Currently I implement COMMIT (which has a range) with a by messing
> > > > > around with filemap_fdatawrite and filemap_fdatawait (ignoring the
> > > > > range) and I'd rather than a vfs helper.
> > > > 
> > > > I'm not 100% sure, but it looks like the PF_SYNCWRITE process flag
> > > > should be set on the nfsd's while they're doing that, which doesn't
> > > > seem to be happening atm.  Looks like a couple of the IO schedulers
> > > > will make use of that knowledge now.  All the more reason for a VFS
> > > > helper here I guess. ;)
> > > 
> > > PF_SYNCWRITE? What's that???
> > > 
> > > (find | xargs grep ...)
> > > Oh.  The block device schedulers like to know if a request is sync or
> > > async (and all reads are assumed to be sync) - which is reasonable -
> > > and so have a per-task flag to tell them - which isn't (IMO).
> > > 
> > > md/raid (particularly raid5) often does the write from a different
> > > process than generated the original request, so that will break
> > > completely. 
> > 
> > I don't think any disagrees with you, the sync-write process flag is
> > indeed an atrocious beast...
> 
> Yeah.  PF_SYNCWRITE was a performance tweak for the anticipatory scheduler.
> As cfq is using it as well now (hopefully to good effect) I guess it could
> be formalised more.

Yup, both 'as' and 'cfq' would prefer to just look at a SYNC bio flag
instead. But the logic itself is definitely needed.

> > > What is wrong with a bio flag I wonder....
> > 
> > Nothing, in fact I would love for it to be changed. I'm sure such a
> > patch would be accepted with open arms! :-)
> 
> I think once someone starts coding it, they'll become a big fan of
> PF_SYNCWRITE...

They might not become a big fan, but they'll surely appreciate the
simplicity of it :-)

> For the page writeback functions it's probably possible to use
> writeback_control.sync_mode=WB_SYNC_ALL as a trigger, propagate that into
> the IO layer.  Maybe that'll always be sufficient - it's hard to tell.  The
> writeback paths are twisty and deep...
> 
> Then again, using WB_SYNC_ALL as a hint that this process will be waiting
> for this writeout to complete is a bit hacky too - it doesn't _really_ mean
> that - it just means that I/O should be _started_ against all relevant
> dirty data.
> 
> Good luck ;)

It's not a hard problem, but it will definitely cost a little sweat to
go through. I'm sure Neil could pull it off, the question is more if he
wants to :-)

-- 
Jens Axboe

Re: [patch 1/1] sys_sync_file_range()

[Andrew Morton]

Jens Axboe <axboe@suse.de> wrote:
>
> > > I don't think any disagrees with you, the sync-write process flag is
> > > indeed an atrocious beast...
> > 
> > Yeah.  PF_SYNCWRITE was a performance tweak for the anticipatory scheduler.
> > As cfq is using it as well now (hopefully to good effect) I guess it could
> > be formalised more.
> 
> Yup, both 'as' and 'cfq' would prefer to just look at a SYNC bio flag
> instead. But the logic itself is definitely needed.

hm.  I actually thought we were already doing that.  We should at least
tranfer PF_SYNCWRITE into bi_flags at the point where we start to construct
the BIO.

That might well fix RAID, too.  If it's handing work off to another thread
via BIOs.

Re: [patch 1/1] sys_sync_file_range()

[Luck, Tony]

On Wed, Mar 29, 2006 at 11:41:11PM -0800, akpm@osdl.org wrote:
> @@ -318,8 +318,9 @@
>  #define __NR_unshare		310
>  #define __NR_set_robust_list	311
>  #define __NR_get_robust_list	312
> +#define __NR_sys_sync_file_range 313

What's up with the __NR_sys_yada_yada?  Except for recent entries (kexec,
splice, and now sync_file_range) all of the other names in here have
dropped the "sys_".

Is it too late to fix __NR_sys_kexec_load (since it is out in the
wild now?)

-Tony

Re: [patch 1/1] sys_sync_file_range()

[Andrew Morton]

"Luck, Tony" <tony.luck@intel.com> wrote:
>
> On Wed, Mar 29, 2006 at 11:41:11PM -0800, akpm@osdl.org wrote:
> > @@ -318,8 +318,9 @@
> >  #define __NR_unshare		310
> >  #define __NR_set_robust_list	311
> >  #define __NR_get_robust_list	312
> > +#define __NR_sys_sync_file_range 313
> 
> What's up with the __NR_sys_yada_yada?

brainfart.  sync_file_range() and splice() will get fixed once Linus
resurfaces.

>  Except for recent entries (kexec,
> splice, and now sync_file_range) all of the other names in here have
> dropped the "sys_".
> 
> Is it too late to fix __NR_sys_kexec_load (since it is out in the
> wild now?)

It only affects users of the _syscallN macros in unistd.h.  I don't expect
that fixing kexec will break much.

Re: [patch 1/1] sys_sync_file_range()

[Randy.Dunlap]

On Tue, 4 Apr 2006 13:50:55 -0700 Luck, Tony wrote:

> On Wed, Mar 29, 2006 at 11:41:11PM -0800, akpm@osdl.org wrote:
> > @@ -318,8 +318,9 @@
> >  #define __NR_unshare		310
> >  #define __NR_set_robust_list	311
> >  #define __NR_get_robust_list	312
> > +#define __NR_sys_sync_file_range 313
> 
> What's up with the __NR_sys_yada_yada?  Except for recent entries (kexec,
> splice, and now sync_file_range) all of the other names in here have
> dropped the "sys_".
> 
> Is it too late to fix __NR_sys_kexec_load (since it is out in the
> wild now?)

already been done:  (copy-n-paste warning here:)
~~~~~

On i386, we don't use sys_ prefix for __NR_*. This patch removes it.
[FWIW, _syscall*() macros will generate foo() instead of sys_foo().]

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 include/asm-i386/unistd.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN include/asm-i386/unistd.h~remove-sys_-prefix include/asm-i386/unistd.h
--- linux-2.6/include/asm-i386/unistd.h~remove-sys_-prefix	2006-04-02 05:23:57.000000000 +0900
+++ linux-2.6-hirofumi/include/asm-i386/unistd.h	2006-04-02 05:24:10.000000000 +0900
@@ -318,8 +318,8 @@
 #define __NR_unshare		310
 #define __NR_set_robust_list	311
 #define __NR_get_robust_list	312
-#define __NR_sys_splice		313
-#define __NR_sys_sync_file_range 314
+#define __NR_splice		313
+#define __NR_sync_file_range	314
 
 #define NR_syscalls 315


---
~Randy

Re: [patch 1/1] sys_sync_file_range()

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

Luck, Tony wrote:
> Is it too late to fix __NR_sys_kexec_load (since it is out in the
> wild now?)

It's not too late IMO.  The only real users are the libcs.  And in fact,
I'm already checking for __NR_sync_file_range and not the sys_ variant.
 Andrew, please change it.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]