improve the performance of large sequential write NFS workloads

Eager Writeback for NFS Clients
-------------------------------
Prevent applications that write large sequential streams of data (like backup, for example) from entering into a memory pressure state, which degrades performance by falling back to synchronous operations (both synchronous writes and additional commits). This is accomplished by preventing the client application from dirtying pages faster than they can be written to the server: clients write pages eagerly instead of lazily.

The eager writeback is controlled by a sysctl: fs.nfs.nfs_max_woutstanding set to 0 disables the feature. Otherwise it contains the maximum number of outstanding NFS writes that can be in flight for a given file. This is used to block the application from dirtying more pages until the writes are complete.

This patch is based heavily (okay, almost entirely) on a prior patch by Peter Staubach. For the original patch, see http://article.gmane.org/gmane.linux.nfs/24323.

The patch below applies to linux-2.6.32-rc7, but it should apply cleanly to vanilla linux-2.6.32.

Performance data and tuning notes can be found on my web site (http://www.nec-labs.com/~sar).
With iozone, I see about 50% improvement for large sequential write workloads over a 1Gb Ethernet. With an in-house micro-benchmark, I see 80% improvement for large, single-stream, sequential workloads (where "large" is defined to be greater than the memory size on the client).

Signed-off-by Steve Rago <[email protected]>

---

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/fs/fs-writeback.c linux-2.6.32-rc7/fs/fs-writeback.c

fs/fs-writeback.c +4 -0

@@ -771,6 +771,8 @@ static long wb_writeback(struct bdi_writ
 		wbc.range_start = 0;
 		wbc.range_end = LLONG_MAX;
 	}
+	if (args->for_background || wbc.for_kupdate)
+		wbc.nonblocking = 1;
 
 	for (;;) {
 		/*

@@ -859,6 +861,8 @@ static long wb_check_old_data_flush(stru
 	unsigned long expired;
 	long nr_pages;
 
+	if (dirty_writeback_interval == 0)
+		return 0;
 	expired = wb->last_old_flush +
 			msecs_to_jiffies(dirty_writeback_interval * 10);
 	if (time_before(jiffies, expired))

fs/fs-writeback.c +0 -0

 		schedule_timeout_interruptible(wait_jiffies);
 		try_to_freeze();
 	}

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/fs/nfs/file.c linux-2.6.32-rc7/fs/nfs/file.c

fs/nfs/file.c +27 -4

@@ -589,11 +589,17 @@ static int nfs_need_sync_write(struct fi
 	return 0;
 }
 
+static int nfs_is_seqwrite(struct inode *inode, loff_t pos)
+{
+	return NFS_I(inode)->wrpos == pos;
+}
+
 static ssize_t nfs_file_write(struct kiocb *iocb, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos)
 {
 	struct dentry * dentry = iocb->ki_filp->f_path.dentry;
 	struct inode * inode = dentry->d_inode;
+	struct nfs_inode *nfsi = NFS_I(inode);
 	ssize_t result;
 	size_t count = iov_length(iov, nr_segs);
 

@@ -607,6 +613,12 @@ static ssize_t nfs_file_write(struct kio
 	result = -EBUSY;
 	if (IS_SWAPFILE(inode))
 		goto out_swapfile;
+
+	result = count;
+	if (!count)
+		goto out;
+	nfs_wait_woutstanding(inode);
+
 	/*
 	 * O_APPEND implies that we must revalidate the file length.
 	 */

@@ -623,10 +635,21 @@ static ssize_t nfs_file_write(struct kio
 	nfs_add_stats(inode, NFSIOS_NORMALWRITTENBYTES, count);
 	result = generic_file_aio_write(iocb, iov, nr_segs, pos);
 	/* Return error values for O_SYNC and IS_SYNC() */
-	if (result >= 0 && nfs_need_sync_write(iocb->ki_filp, inode)) {
-		int err = nfs_do_fsync(nfs_file_open_context(iocb->ki_filp), inode);
-		if (err < 0)
-			result = err;
+	if (result >= 0) {
+		if (nfs_need_sync_write(iocb->ki_filp, inode)) {
+			int err;
+
+			err = nfs_do_fsync(nfs_file_open_context(iocb->ki_filp),
+					   inode);
+			if (err < 0)
+				result = err;
+		} else if (nfs_max_woutstanding != 0 &&
+		     nfs_is_seqwrite(inode, pos) &&
+		     atomic_read(&nfsi->ndirty) >= NFS_SERVER(inode)->wpages) {
+			nfs_wb_eager(inode);
+		}
+		if (result > 0)
+			nfsi->wrpos = pos + result;
 	}
 out:
 	return result;

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/fs/nfs/inode.c linux-2.6.32-rc7/fs/nfs/inode.c

fs/nfs/inode.c +13 -3

@@ -508,7 +508,9 @@ void nfs_setattr_update_inode(struct ino
 int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)
 {
 	struct inode *inode = dentry->d_inode;
-	int need_atime = NFS_I(inode)->cache_validity & NFS_INO_INVALID_ATIME;
+	struct nfs_inode *nfsi = NFS_I(inode);
+	int need_atime = nfsi->cache_validity & NFS_INO_INVALID_ATIME;
+	int woutstanding = nfs_max_woutstanding;
 	int err;
 
 	/*

@@ -519,9 +521,8 @@ int nfs_getattr(struct vfsmount *mnt, st
 	 * nfs_wb_nocommit.
 	 */
 	if (S_ISREG(inode->i_mode)) {
-		mutex_lock(&inode->i_mutex);
+		atomic_add(woutstanding, &nfsi->writes);
 		nfs_wb_nocommit(inode);
-		mutex_unlock(&inode->i_mutex);
 	}
 
 	/*

@@ -545,6 +546,11 @@ int nfs_getattr(struct vfsmount *mnt, st
 		generic_fillattr(inode, stat);
 		stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
 	}
+
+	if (S_ISREG(inode->i_mode)) {
+		atomic_sub(woutstanding, &nfsi->writes);
+		wake_up(&nfsi->writes_wq);
+	}
 	return err;
 }
 

@@ -1418,9 +1424,13 @@ static void init_once(void *foo)
 	INIT_LIST_HEAD(&nfsi->access_cache_inode_lru);
 	INIT_RADIX_TREE(&nfsi->nfs_page_tree, GFP_ATOMIC);
 	nfsi->npages = 0;
+	atomic_set(&nfsi->ndirty, 0);
 	atomic_set(&nfsi->silly_count, 1);
 	INIT_HLIST_HEAD(&nfsi->silly_list);
 	init_waitqueue_head(&nfsi->waitqueue);
+	atomic_set(&nfsi->writes, 0);
+	init_waitqueue_head(&nfsi->writes_wq);
+	nfsi->wrpos = 0;
 	nfs4_init_once(nfsi);
 }
 

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/fs/nfs/sysctl.c linux-2.6.32-rc7/fs/nfs/sysctl.c

fs/nfs/sysctl.c +8 -0

@@ -58,6 +58,14 @@ static ctl_table nfs_cb_sysctls[] = {
 		.mode		= 0644,
 		.proc_handler	= &proc_dointvec,
 	},
+	{
+		.ctl_name       = CTL_UNNUMBERED,
+		.procname       = "nfs_max_woutstanding",
+		.data           = &nfs_max_woutstanding,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = &proc_dointvec,
+	},
 	{ .ctl_name = 0 }
 };
 

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/fs/nfs/write.c linux-2.6.32-rc7/fs/nfs/write.c

fs/nfs/write.c +26 -0

@@ -176,6 +176,8 @@ static void nfs_mark_uptodate(struct pag
 
 static int wb_priority(struct writeback_control *wbc)
 {
+	if (nfs_max_woutstanding != 0)
+		return 0;
 	if (wbc->for_reclaim)
 		return FLUSH_HIGHPRI | FLUSH_STABLE;
 	if (wbc->for_kupdate)

@@ -200,7 +202,9 @@ static int nfs_set_page_writeback(struct
 	if (!ret) {
 		struct inode *inode = page->mapping->host;
 		struct nfs_server *nfss = NFS_SERVER(inode);
+		struct nfs_inode *nfsi = NFS_I(inode);
 
+		atomic_dec(&nfsi->ndirty);
 		if (atomic_long_inc_return(&nfss->writeback) >
 				NFS_CONGESTION_ON_THRESH) {
 			set_bdi_congested(&nfss->backing_dev_info,

@@ -325,6 +329,39 @@ static int nfs_writepages_callback(struc
 	return ret;
 }
 
+int nfs_max_woutstanding = 16;
+
+static void nfs_inc_woutstanding(struct inode *inode)
+{
+	struct nfs_inode *nfsi = NFS_I(inode);
+	atomic_inc(&nfsi->writes);
+}
+
+static void nfs_dec_woutstanding(struct inode *inode)
+{
+	struct nfs_inode *nfsi = NFS_I(inode);
+	if (atomic_dec_return(&nfsi->writes) < nfs_max_woutstanding)
+		wake_up(&nfsi->writes_wq);
+}
+
+void nfs_wait_woutstanding(struct inode *inode)
+{
+	if (nfs_max_woutstanding != 0) {
+		unsigned long background_thresh;
+		unsigned long dirty_thresh;
+		long npages;
+		struct nfs_inode *nfsi = NFS_I(inode);

fs/nfs/write.c +31 -5

+
 int nfs_writepages(struct address_space *mapping, struct writeback_control *wbc)
 {
 	struct inode *inode = mapping->host;

@@ -420,6 +457,9 @@ static void nfs_inode_remove_request(str
 static void
 nfs_mark_request_dirty(struct nfs_page *req)
 {
+	struct inode *inode = req->wb_context->path.dentry->d_inode;
+	struct nfs_inode *nfsi = NFS_I(inode);
+	atomic_inc(&nfsi->ndirty);
 	__set_page_dirty_nobuffers(req->wb_page);
 }
 

@@ -682,16 +722,18 @@ static struct nfs_page * nfs_setup_write
 
 	req = nfs_try_to_update_request(inode, page, offset, bytes);
 	if (req != NULL)
-		goto out;
+		return req;
 	req = nfs_create_request(ctx, inode, page, offset, bytes);
 	if (IS_ERR(req))
-		goto out;
+		return req;
 	error = nfs_inode_add_request(inode, req);
 	if (error != 0) {
 		nfs_release_request(req);
 		req = ERR_PTR(error);
+	} else {
+		struct nfs_inode *nfsi = NFS_I(inode);
+		atomic_inc(&nfsi->ndirty);
 	}
-out:
 	return req;
 }
 

@@ -877,6 +919,7 @@ static int nfs_write_rpcsetup(struct nfs
 		count,
 		(unsigned long long)data->args.offset);
 
+	nfs_inc_woutstanding(inode);
 	task = rpc_run_task(&task_setup_data);
 	if (IS_ERR(task))
 		return PTR_ERR(task);

@@ -1172,7 +1215,7 @@ int nfs_writeback_done(struct rpc_task *
 	 */
 	status = NFS_PROTO(data->inode)->write_done(task, data);
 	if (status != 0)
-		return status;
+		goto out;
 	nfs_add_stats(data->inode, NFSIOS_SERVERWRITTENBYTES, resp->count);
 
 #if defined(CONFIG_NFS_V3) || defined(CONFIG_NFS_V4)

@@ -1229,6 +1272,8 @@ int nfs_writeback_done(struct rpc_task *
 		task->tk_status = -EIO;
 	}
 	nfs4_sequence_free_slot(server->nfs_client, &data->res.seq_res);
+out:
+	nfs_dec_woutstanding(data->inode);
 	return 0;
 }
 

@@ -1591,6 +1636,24 @@ int nfs_wb_page(struct inode *inode, str
 	return nfs_wb_page_priority(inode, page, FLUSH_STABLE);
 }
 
+int nfs_wb_eager(struct inode *inode)
+{
+	struct address_space *mapping = inode->i_mapping;
+	struct writeback_control wbc = {
+		.bdi            = mapping->backing_dev_info,
+		.sync_mode      = WB_SYNC_NONE,
+		.nr_to_write    = LONG_MAX,
+		.range_start    = 0,
+		.range_end      = LLONG_MAX,
+	};
+	int ret;
+
+	ret = nfs_writepages(mapping, &wbc);
+	if (ret < 0)
+		__mark_inode_dirty(inode, I_DIRTY_PAGES);
+	return ret;
+}
+
 #ifdef CONFIG_MIGRATION
 int nfs_migrate_page(struct address_space *mapping, struct page *newpage,
 		struct page *page)

@@ -1674,4 +1737,3 @@ void nfs_destroy_writepagecache(void)
 	mempool_destroy(nfs_wdata_mempool);
 	kmem_cache_destroy(nfs_wdata_cachep);
 }
-

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/include/linux/nfs_fs.h linux-2.6.32-rc7/include/linux/nfs_fs.h

include/linux/nfs_fs.h +9 -0

@@ -166,6 +166,7 @@ struct nfs_inode {
 	struct radix_tree_root	nfs_page_tree;
 
 	unsigned long		npages;
+	atomic_t		ndirty;
 
 	/* Open contexts for shared mmap writes */
 	struct list_head	open_files;

@@ -187,6 +188,11 @@ struct nfs_inode {
 #ifdef CONFIG_NFS_FSCACHE
 	struct fscache_cookie	*fscache;
 #endif
+
+	loff_t			wrpos;
+	atomic_t		writes;
+	wait_queue_head_t	writes_wq;
+
 	struct inode		vfs_inode;
 };
 

@@ -467,11 +473,13 @@ extern void nfs_unblock_sillyrename(stru
  * linux/fs/nfs/write.c
  */
 extern int  nfs_congestion_kb;
+extern int  nfs_max_woutstanding;
 extern int  nfs_writepage(struct page *page, struct writeback_control *wbc);
 extern int  nfs_writepages(struct address_space *, struct writeback_control *);
 extern int  nfs_flush_incompatible(struct file *file, struct page *page);
 extern int  nfs_updatepage(struct file *, struct page *, unsigned int, unsigned int);
 extern int nfs_writeback_done(struct rpc_task *, struct nfs_write_data *);
+extern void nfs_wait_woutstanding(struct inode *);
 
 /*
  * Try to write back everything synchronously (but check the

@@ -482,6 +490,7 @@ extern int nfs_wb_all(struct inode *inod
 extern int nfs_wb_nocommit(struct inode *inode);
 extern int nfs_wb_page(struct inode *inode, struct page* page);
 extern int nfs_wb_page_cancel(struct inode *inode, struct page* page);
+extern int nfs_wb_eager(struct inode *inode);
 #if defined(CONFIG_NFS_V3) || defined(CONFIG_NFS_V4)
 extern int  nfs_commit_inode(struct inode *, int);
 extern struct nfs_write_data *nfs_commitdata_alloc(void);

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/mm/page-writeback.c linux-2.6.32-rc7/mm/page-writeback.c

Quoted 51 lines · Steve Rago

Eager Writeback for NFS Clients
-------------------------------
Prevent applications that write large sequential streams of data (like backup, for example) from entering into a memory pressure state, which degrades performance by falling back to synchronous operations (both synchronous writes and additional commits). This is accomplished by preventing the client application from dirtying pages faster than they can be written to the server: clients write pages eagerly instead of lazily.

The eager writeback is controlled by a sysctl: fs.nfs.nfs_max_woutstanding set to 0 disables the feature. Otherwise it contains the maximum number of outstanding NFS writes that can be in flight for a given file. This is used to block the application from dirtying more pages until the writes are complete.

This patch is based heavily (okay, almost entirely) on a prior patch by Peter Staubach. For the original patch, see http://article.gmane.org/gmane.linux.nfs/24323.

The patch below applies to linux-2.6.32-rc7, but it should apply cleanly to vanilla linux-2.6.32.

Performance data and tuning notes can be found on my web site (http://www.nec-labs.com/~sar).
With iozone, I see about 50% improvement for large sequential write workloads over a 1Gb Ethernet. With an in-house micro-benchmark, I see 80% improvement for large, single-stream, sequential workloads (where "large" is defined to be greater than the memory size on the client).

Signed-off-by Steve Rago <[email protected]>

---

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/fs/fs-writeback.c linux-2.6.32-rc7/fs/fs-writeback.c

fs/fs-writeback.c +4 -0

@@ -771,6 +771,8 @@ static long wb_writeback(struct bdi_writ
 		wbc.range_start = 0;
 		wbc.range_end = LLONG_MAX;
 	}
+	if (args->for_background || wbc.for_kupdate)
+		wbc.nonblocking = 1;
 
 	for (;;) {
 		/*

@@ -859,6 +861,8 @@ static long wb_check_old_data_flush(stru
 	unsigned long expired;
 	long nr_pages;
 
+	if (dirty_writeback_interval == 0)
+		return 0;
 	expired = wb->last_old_flush +
 			msecs_to_jiffies(dirty_writeback_interval * 10);
 	if (time_before(jiffies, expired))

@@ -954,7 +958,11 @@ int bdi_writeback_task(struct bdi_writeb
 				break;
 		}
 

fs/fs-writeback.c int bdi_writeback_task(struct bdi_writeb · line 965

Quoted 6 lines

fs/fs-writeback.c +5 -1

-		wait_jiffies = msecs_to_jiffies(dirty_writeback_interval * 10);
+		if (dirty_writeback_interval == 0)
+			wait_jiffies = msecs_to_jiffies(5000);	/* default */
+		else
+			wait_jiffies =
+				msecs_to_jiffies(dirty_writeback_interval * 10);

I'm not up-to-date on the bdi-writeout stuff, but this just looks wrong.

Quoted 181 lines

fs/fs-writeback.c +0 -0

 		schedule_timeout_interruptible(wait_jiffies);
 		try_to_freeze();
 	}

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/fs/nfs/file.c linux-2.6.32-rc7/fs/nfs/file.c

fs/nfs/file.c +27 -4

@@ -589,11 +589,17 @@ static int nfs_need_sync_write(struct fi
 	return 0;
 }
 
+static int nfs_is_seqwrite(struct inode *inode, loff_t pos)
+{
+	return NFS_I(inode)->wrpos == pos;
+}
+
 static ssize_t nfs_file_write(struct kiocb *iocb, const struct iovec *iov,
 				unsigned long nr_segs, loff_t pos)
 {
 	struct dentry * dentry = iocb->ki_filp->f_path.dentry;
 	struct inode * inode = dentry->d_inode;
+	struct nfs_inode *nfsi = NFS_I(inode);
 	ssize_t result;
 	size_t count = iov_length(iov, nr_segs);
 

@@ -607,6 +613,12 @@ static ssize_t nfs_file_write(struct kio
 	result = -EBUSY;
 	if (IS_SWAPFILE(inode))
 		goto out_swapfile;
+
+	result = count;
+	if (!count)
+		goto out;
+	nfs_wait_woutstanding(inode);
+
 	/*
 	 * O_APPEND implies that we must revalidate the file length.
 	 */

@@ -623,10 +635,21 @@ static ssize_t nfs_file_write(struct kio
 	nfs_add_stats(inode, NFSIOS_NORMALWRITTENBYTES, count);
 	result = generic_file_aio_write(iocb, iov, nr_segs, pos);
 	/* Return error values for O_SYNC and IS_SYNC() */
-	if (result >= 0 && nfs_need_sync_write(iocb->ki_filp, inode)) {
-		int err = nfs_do_fsync(nfs_file_open_context(iocb->ki_filp), inode);
-		if (err < 0)
-			result = err;
+	if (result >= 0) {
+		if (nfs_need_sync_write(iocb->ki_filp, inode)) {
+			int err;
+
+			err = nfs_do_fsync(nfs_file_open_context(iocb->ki_filp),
+					   inode);
+			if (err < 0)
+				result = err;
+		} else if (nfs_max_woutstanding != 0 &&
+		     nfs_is_seqwrite(inode, pos) &&
+		     atomic_read(&nfsi->ndirty) >= NFS_SERVER(inode)->wpages) {
+			nfs_wb_eager(inode);
+		}
+		if (result > 0)
+			nfsi->wrpos = pos + result;
 	}
 out:
 	return result;

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/fs/nfs/inode.c linux-2.6.32-rc7/fs/nfs/inode.c

fs/nfs/inode.c +13 -3

@@ -508,7 +508,9 @@ void nfs_setattr_update_inode(struct ino
 int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)
 {
 	struct inode *inode = dentry->d_inode;
-	int need_atime = NFS_I(inode)->cache_validity & NFS_INO_INVALID_ATIME;
+	struct nfs_inode *nfsi = NFS_I(inode);
+	int need_atime = nfsi->cache_validity & NFS_INO_INVALID_ATIME;
+	int woutstanding = nfs_max_woutstanding;
 	int err;
 
 	/*

@@ -519,9 +521,8 @@ int nfs_getattr(struct vfsmount *mnt, st
 	 * nfs_wb_nocommit.
 	 */
 	if (S_ISREG(inode->i_mode)) {
-		mutex_lock(&inode->i_mutex);
+		atomic_add(woutstanding, &nfsi->writes);
 		nfs_wb_nocommit(inode);
-		mutex_unlock(&inode->i_mutex);
 	}
 
 	/*

@@ -545,6 +546,11 @@ int nfs_getattr(struct vfsmount *mnt, st
 		generic_fillattr(inode, stat);
 		stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
 	}
+
+	if (S_ISREG(inode->i_mode)) {
+		atomic_sub(woutstanding, &nfsi->writes);
+		wake_up(&nfsi->writes_wq);
+	}
 	return err;
 }
 

@@ -1418,9 +1424,13 @@ static void init_once(void *foo)
 	INIT_LIST_HEAD(&nfsi->access_cache_inode_lru);
 	INIT_RADIX_TREE(&nfsi->nfs_page_tree, GFP_ATOMIC);
 	nfsi->npages = 0;
+	atomic_set(&nfsi->ndirty, 0);
 	atomic_set(&nfsi->silly_count, 1);
 	INIT_HLIST_HEAD(&nfsi->silly_list);
 	init_waitqueue_head(&nfsi->waitqueue);
+	atomic_set(&nfsi->writes, 0);
+	init_waitqueue_head(&nfsi->writes_wq);
+	nfsi->wrpos = 0;
 	nfs4_init_once(nfsi);
 }
 

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/fs/nfs/sysctl.c linux-2.6.32-rc7/fs/nfs/sysctl.c

fs/nfs/sysctl.c +8 -0

@@ -58,6 +58,14 @@ static ctl_table nfs_cb_sysctls[] = {
 		.mode		= 0644,
 		.proc_handler	= &proc_dointvec,
 	},
+	{
+		.ctl_name       = CTL_UNNUMBERED,
+		.procname       = "nfs_max_woutstanding",
+		.data           = &nfs_max_woutstanding,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = &proc_dointvec,
+	},
 	{ .ctl_name = 0 }
 };
 

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/fs/nfs/write.c linux-2.6.32-rc7/fs/nfs/write.c

fs/nfs/write.c +26 -0

@@ -176,6 +176,8 @@ static void nfs_mark_uptodate(struct pag
 
 static int wb_priority(struct writeback_control *wbc)
 {
+	if (nfs_max_woutstanding != 0)
+		return 0;
 	if (wbc->for_reclaim)
 		return FLUSH_HIGHPRI | FLUSH_STABLE;
 	if (wbc->for_kupdate)

@@ -200,7 +202,9 @@ static int nfs_set_page_writeback(struct
 	if (!ret) {
 		struct inode *inode = page->mapping->host;
 		struct nfs_server *nfss = NFS_SERVER(inode);
+		struct nfs_inode *nfsi = NFS_I(inode);
 
+		atomic_dec(&nfsi->ndirty);
 		if (atomic_long_inc_return(&nfss->writeback) >
 				NFS_CONGESTION_ON_THRESH) {
 			set_bdi_congested(&nfss->backing_dev_info,

@@ -325,6 +329,39 @@ static int nfs_writepages_callback(struc
 	return ret;
 }
 
+int nfs_max_woutstanding = 16;
+
+static void nfs_inc_woutstanding(struct inode *inode)
+{
+	struct nfs_inode *nfsi = NFS_I(inode);
+	atomic_inc(&nfsi->writes);
+}
+
+static void nfs_dec_woutstanding(struct inode *inode)
+{
+	struct nfs_inode *nfsi = NFS_I(inode);
+	if (atomic_dec_return(&nfsi->writes) < nfs_max_woutstanding)
+		wake_up(&nfsi->writes_wq);
+}
+
+void nfs_wait_woutstanding(struct inode *inode)
+{
+	if (nfs_max_woutstanding != 0) {
+		unsigned long background_thresh;
+		unsigned long dirty_thresh;
+		long npages;
+		struct nfs_inode *nfsi = NFS_I(inode);

fs/nfs/write.c static int nfs_writepages_callback(struc · line 363

Quoted 10 lines

fs/nfs/write.c +10 -0

+
+		get_dirty_limits(&background_thresh, &dirty_thresh, NULL, NULL);
+		npages = global_page_state(NR_FILE_DIRTY) +
+			 global_page_state(NR_UNSTABLE_NFS) +
+			 global_page_state(NR_WRITEBACK);
+		if (npages >= background_thresh)
+			wait_event(nfsi->writes_wq,
+			   atomic_read(&nfsi->writes) < nfs_max_woutstanding);
+	}
+}

This looks utterly busted too, why the global state and not the nfs client's bdi state?

Also, why create this extra workqueue and not simply use the congestion interface that is present? If the congestion stuff doesn't work for you, fix that, don't add extra muck like this.

Quoted 145 lines

fs/nfs/write.c +31 -5

+
 int nfs_writepages(struct address_space *mapping, struct writeback_control *wbc)
 {
 	struct inode *inode = mapping->host;

@@ -420,6 +457,9 @@ static void nfs_inode_remove_request(str
 static void
 nfs_mark_request_dirty(struct nfs_page *req)
 {
+	struct inode *inode = req->wb_context->path.dentry->d_inode;
+	struct nfs_inode *nfsi = NFS_I(inode);
+	atomic_inc(&nfsi->ndirty);
 	__set_page_dirty_nobuffers(req->wb_page);
 }
 

@@ -682,16 +722,18 @@ static struct nfs_page * nfs_setup_write
 
 	req = nfs_try_to_update_request(inode, page, offset, bytes);
 	if (req != NULL)
-		goto out;
+		return req;
 	req = nfs_create_request(ctx, inode, page, offset, bytes);
 	if (IS_ERR(req))
-		goto out;
+		return req;
 	error = nfs_inode_add_request(inode, req);
 	if (error != 0) {
 		nfs_release_request(req);
 		req = ERR_PTR(error);
+	} else {
+		struct nfs_inode *nfsi = NFS_I(inode);
+		atomic_inc(&nfsi->ndirty);
 	}
-out:
 	return req;
 }
 

@@ -877,6 +919,7 @@ static int nfs_write_rpcsetup(struct nfs
 		count,
 		(unsigned long long)data->args.offset);
 
+	nfs_inc_woutstanding(inode);
 	task = rpc_run_task(&task_setup_data);
 	if (IS_ERR(task))
 		return PTR_ERR(task);

@@ -1172,7 +1215,7 @@ int nfs_writeback_done(struct rpc_task *
 	 */
 	status = NFS_PROTO(data->inode)->write_done(task, data);
 	if (status != 0)
-		return status;
+		goto out;
 	nfs_add_stats(data->inode, NFSIOS_SERVERWRITTENBYTES, resp->count);
 
 #if defined(CONFIG_NFS_V3) || defined(CONFIG_NFS_V4)

@@ -1229,6 +1272,8 @@ int nfs_writeback_done(struct rpc_task *
 		task->tk_status = -EIO;
 	}
 	nfs4_sequence_free_slot(server->nfs_client, &data->res.seq_res);
+out:
+	nfs_dec_woutstanding(data->inode);
 	return 0;
 }
 

@@ -1591,6 +1636,24 @@ int nfs_wb_page(struct inode *inode, str
 	return nfs_wb_page_priority(inode, page, FLUSH_STABLE);
 }
 
+int nfs_wb_eager(struct inode *inode)
+{
+	struct address_space *mapping = inode->i_mapping;
+	struct writeback_control wbc = {
+		.bdi            = mapping->backing_dev_info,
+		.sync_mode      = WB_SYNC_NONE,
+		.nr_to_write    = LONG_MAX,
+		.range_start    = 0,
+		.range_end      = LLONG_MAX,
+	};
+	int ret;
+
+	ret = nfs_writepages(mapping, &wbc);
+	if (ret < 0)
+		__mark_inode_dirty(inode, I_DIRTY_PAGES);
+	return ret;
+}
+
 #ifdef CONFIG_MIGRATION
 int nfs_migrate_page(struct address_space *mapping, struct page *newpage,
 		struct page *page)

@@ -1674,4 +1737,3 @@ void nfs_destroy_writepagecache(void)
 	mempool_destroy(nfs_wdata_mempool);
 	kmem_cache_destroy(nfs_wdata_cachep);
 }
-

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/include/linux/nfs_fs.h linux-2.6.32-rc7/include/linux/nfs_fs.h

include/linux/nfs_fs.h +9 -0

@@ -166,6 +166,7 @@ struct nfs_inode {
 	struct radix_tree_root	nfs_page_tree;
 
 	unsigned long		npages;
+	atomic_t		ndirty;
 
 	/* Open contexts for shared mmap writes */
 	struct list_head	open_files;

@@ -187,6 +188,11 @@ struct nfs_inode {
 #ifdef CONFIG_NFS_FSCACHE
 	struct fscache_cookie	*fscache;
 #endif
+
+	loff_t			wrpos;
+	atomic_t		writes;
+	wait_queue_head_t	writes_wq;
+
 	struct inode		vfs_inode;
 };
 

@@ -467,11 +473,13 @@ extern void nfs_unblock_sillyrename(stru
  * linux/fs/nfs/write.c
  */
 extern int  nfs_congestion_kb;
+extern int  nfs_max_woutstanding;
 extern int  nfs_writepage(struct page *page, struct writeback_control *wbc);
 extern int  nfs_writepages(struct address_space *, struct writeback_control *);
 extern int  nfs_flush_incompatible(struct file *file, struct page *page);
 extern int  nfs_updatepage(struct file *, struct page *, unsigned int, unsigned int);
 extern int nfs_writeback_done(struct rpc_task *, struct nfs_write_data *);
+extern void nfs_wait_woutstanding(struct inode *);
 
 /*
  * Try to write back everything synchronously (but check the

@@ -482,6 +490,7 @@ extern int nfs_wb_all(struct inode *inod
 extern int nfs_wb_nocommit(struct inode *inode);
 extern int nfs_wb_page(struct inode *inode, struct page* page);
 extern int nfs_wb_page_cancel(struct inode *inode, struct page* page);
+extern int nfs_wb_eager(struct inode *inode);
 #if defined(CONFIG_NFS_V3) || defined(CONFIG_NFS_V4)
 extern int  nfs_commit_inode(struct inode *, int);
 extern struct nfs_write_data *nfs_commitdata_alloc(void);

diff -rupN -X linux-2.6.32-rc7/Documentation/dontdiff linux-2.6.32-rc7-orig/mm/page-writeback.c linux-2.6.32-rc7/mm/page-writeback.c

mm/page-writeback.c +0 -0

@@ -536,7 +536,7 @@ static void balance_dirty_pages(struct a
 		 * threshold otherwise wait until the disk writes catch
 		 * up.
 		 */

mm/page-writeback.c static void balance_dirty_pages(struct a · line 542

Quoted 5 lines

mm/page-writeback.c +1 -1

-		if (bdi_nr_reclaimable > bdi_thresh) {
+		if (bdi_nr_reclaimable != 0) {
 			writeback_inodes_wbc(&wbc);
 			pages_written += write_chunk - wbc.nr_to_write;
 			get_dirty_limits(&background_thresh, &dirty_thresh,

And I think you just broke regular writeback here, allowing for tons of unneeded writeout of very small chunks.

This really needs to be multiple patches, and a proper changelog describing why you do things. The above, because my benchmark goes faster, just isn't sufficient.

Also, I don't think this needs to have a sysctl, it should just work.

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

Quoted 1 line

Also, I don't think this needs to have a sysctl, it should just work.

The sysctl is a *good thing* in that it allows the eager writeback behavior to be tuned and shut off if need be. I can only test the changes on a finite set of systems, so better safe than sorry.

Quoted 5 lines · Steve Rago

Quoted 1 line

Also, I don't think this needs to have a sysctl, it should just work.

The sysctl is a *good thing* in that it allows the eager writeback behavior to be tuned and shut off if need be. I can only test the changes on a finite set of systems, so better safe than sorry.

This issue has been settled many years ago and that's not what we do in the Linux kernel. We prefer patches to core code where we are reasonably sure they result in good behavior - and then we fix bugs in the new behavior, if any.

(Otherwise odd sysctls would mushroom quickly and the system would become untestable in practice.)

Ingo

Quoted 12 lines · Ingo Molnar

Quoted 5 lines · Steve Rago

Quoted 1 line

Also, I don't think this needs to have a sysctl, it should just work.

The sysctl is a *good thing* in that it allows the eager writeback behavior to be tuned and shut off if need be. I can only test the changes on a finite set of systems, so better safe than sorry.

This issue has been settled many years ago and that's not what we do in the Linux kernel. We prefer patches to core code where we are reasonably sure they result in good behavior - and then we fix bugs in the new behavior, if any.

Quoted 4 lines

(Otherwise odd sysctls would mushroom quickly and the system would become untestable in practice.)

Ingo

I don't disagree, but "that's not what we do" hardly provides insight into making the judgment call. [...]

[...] In this case, the variety of combinations of NFS server speed, NFS client speed, transmission link speed, client memory size, and server memory size argues for a tunable parameter, because one value probably won't work well in all combinations. Making it change dynamically based on these parameters is more complicated than these circumstances call for, IMHO.

Quoted 14 lines · Steve Rago

Quoted 12 lines · Ingo Molnar

Quoted 5 lines · Steve Rago

Quoted 1 line

Also, I don't think this needs to have a sysctl, it should just work.

The sysctl is a *good thing* in that it allows the eager writeback behavior to be tuned and shut off if need be. I can only test the changes on a finite set of systems, so better safe than sorry.

This issue has been settled many years ago and that's not what we do in the Linux kernel. We prefer patches to core code where we are reasonably sure they result in good behavior - and then we fix bugs in the new behavior, if any.

Quoted 7 lines

Quoted 4 lines

(Otherwise odd sysctls would mushroom quickly and the system would become untestable in practice.)

Ingo

I don't disagree, but "that's not what we do" hardly provides insight into making the judgment call. [...]

I gave you an example of the problems that arise, see the last sentence above.

Quoted 5 lines

[...] In this case, the variety of combinations of NFS server speed, NFS client speed, transmission link speed, client memory size, and server memory size argues for a tunable parameter, because one value probably won't work well in all combinations. Making it change dynamically based on these parameters is more complicated than these circumstances call for, IMHO.

So having crappy tunables is the reason to introduce even more tunables? I think you just gave a good second example of why we dont want sysctls for features like this.

Ingo

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

I don't disagree, but "that's not what we do" hardly provides insight into making the judgment call. In this case, the variety of combinations of NFS server speed, NFS client speed, transmission link speed, client memory size, and server memory size argues for a tunable parameter, because one value probably won't work well in all combinations. Making it change dynamically based on these parameters is more complicated than these circumstances call for, IMHO.

Quoted 8 lines · On Fri, 18 Dec 2009 16:20:11 -0500 Steve Rago…

I don't disagree, but "that's not what we do" hardly provides insight into making the judgment call. In this case, the variety of combinations of NFS server speed, NFS client speed, transmission link speed, client memory size, and server memory size argues for a tunable parameter, because one value probably won't work well in all combinations. Making it change dynamically based on these parameters is more complicated than these circumstances call for, IMHO.

if you as the expert do not know how to tune this... how is a sysadmin supposed to know better?

Quoted 21 lines

Quoted 7 lines

fs/nfs/write.c +7 -0

+void nfs_wait_woutstanding(struct inode *inode)
+{
+   if (nfs_max_woutstanding != 0) {
+           unsigned long background_thresh;
+           unsigned long dirty_thresh;
+           long npages;
+           struct nfs_inode *nfsi = NFS_I(inode);

fs/nfs/write.c static int nfs_writepages_callback(struc · line 363

Quoted 10 lines

fs/nfs/write.c +10 -0

+
+           get_dirty_limits(&background_thresh, &dirty_thresh, NULL, NULL);
+           npages = global_page_state(NR_FILE_DIRTY) +
+                    global_page_state(NR_UNSTABLE_NFS) +
+                    global_page_state(NR_WRITEBACK);
+           if (npages >= background_thresh)
+                   wait_event(nfsi->writes_wq,
+                      atomic_read(&nfsi->writes) < nfs_max_woutstanding);
+   }
+}

This looks utterly busted too, why the global state and not the nfs client's bdi state?

Quoted 3 lines

Also, why create this extra workqueue and not simply use the congestion interface that is present? If the congestion stuff doesn't work for you, fix that, don't add extra muck like this.

Pages are a global resource. Once we hit the dirty_threshold, the system is going to work harder to flush the pages out. This code prevents the caller from creating more dirty pages in this state, thereby making matters worse, when eager writeback is enabled.

This wait queue is used for different purposes than the congestion_wait interface. Here we are preventing the caller from proceeding if there are too many NFS writes outstanding for this thread and we are in a memory pressure state. It has nothing to do with the state of the bdi congestion.

Quoted 5 lines · Steve Rago

Eager Writeback for NFS Clients
-------------------------------
Prevent applications that write large sequential streams of data (like backup, for example) from entering into a memory pressure state, which degrades performance by falling back to synchronous operations (both synchronous writes and additional commits).

Quoted 3 lines

This is accomplished by preventing the client application from dirtying pages faster than they can be written to the server:
clients write pages eagerly instead of lazily.

Quoted 4 lines

The eager writeback is controlled by a sysctl: fs.nfs.nfs_max_woutstanding set to 0 disables the feature. Otherwise it contains the maximum number of outstanding NFS writes that can be in flight for a given file. This is used to block the application from dirtying more pages until the writes are complete.

Quoted 9 lines

This patch is based heavily (okay, almost entirely) on a prior patch by Peter Staubach. For the original patch, see http://article.gmane.org/gmane.linux.nfs/24323.

The patch below applies to linux-2.6.32-rc7, but it should apply cleanly to vanilla linux-2.6.32.

Performance data and tuning notes can be found on my web site (http://www.nec-labs.com/~sar).
With iozone, I see about 50% improvement for large sequential write workloads over a 1Gb Ethernet. With an in-house micro-benchmark, I see 80% improvement for large, single-stream, sequential workloads (where "large" is defined to be greater than the memory size on the client).

Quoted 16 lines

fs/nfs/file.c +8 -4

@@ -623,10 +635,21 @@ static ssize_t nfs_file_write(struct kio
      nfs_add_stats(inode, NFSIOS_NORMALWRITTENBYTES, count);
      result = generic_file_aio_write(iocb, iov, nr_segs, pos);
      /* Return error values for O_SYNC and IS_SYNC() */
-     if (result >= 0 && nfs_need_sync_write(iocb->ki_filp, inode)) {
-             int err = nfs_do_fsync(nfs_file_open_context(iocb->ki_filp), inode);
-             if (err < 0)
-                     result = err;
+     if (result >= 0) {
+             if (nfs_need_sync_write(iocb->ki_filp, inode)) {
+                     int err;
+
+                     err = nfs_do_fsync(nfs_file_open_context(iocb->ki_filp),
+                                        inode);
+                     if (err < 0)
+                             result = err;

fs/nfs/file.c static ssize_t nfs_file_write(struct kio · line 653

Quoted 8 lines

fs/nfs/file.c +7 -0

+             } else if (nfs_max_woutstanding != 0 &&
+                  nfs_is_seqwrite(inode, pos) &&
+                  atomic_read(&nfsi->ndirty) >= NFS_SERVER(inode)->wpages) {
+                     nfs_wb_eager(inode);
+             }
+             if (result > 0)
+                     nfsi->wrpos = pos + result;
      }

Hi Steve,

// I should really read the NFS code, but maybe you can help us better // understand the problem :)

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Steve Rago

Eager Writeback for NFS Clients
-------------------------------
Prevent applications that write large sequential streams of data (like backup, for example) from entering into a memory pressure state, which degrades performance by falling back to synchronous operations (both synchronous writes and additional commits).

What exactly is the "memory pressure state" condition? What's the code to do the "synchronous writes and additional commits" and maybe how they are triggered?

Quoted 4 lines

Quoted 4 lines

This is accomplished by preventing the client application from dirtying pages faster than they can be written to the server:
clients write pages eagerly instead of lazily.

We already have the balance_dirty_pages() based global throttling. So what makes the performance difference in your proposed "per-inode" throttling? balance_dirty_pages() does have much larger threshold than yours.

Quoted 5 lines

Quoted 5 lines

The eager writeback is controlled by a sysctl: fs.nfs.nfs_max_woutstanding set to 0 disables the feature. Otherwise it contains the maximum number of outstanding NFS writes that can be in flight for a given file. This is used to block the application from dirtying more pages until the writes are complete.

What if we do heuristic write-behind for sequential NFS writes?

Another related proposal from Peter Staubach is to start async writeback (without the throttle in your proposal) when one inode have enough pages dirtied:

Another approach that I suggested was to keep track of the number of pages which are dirty on a per-inode basis. When enough pages are dirty to fill an over the wire transfer, then schedule an asynchronous write to transmit that data to the server. This ties in with support to ensure that the server/network is not completely overwhelmed by the client by flow controlling the writing application to better match the bandwidth and latencies of the network and server. With this support, the NFS client tends not to fill memory with dirty pages and thus, does not depend upon the other parts of the system to flush these pages.

Can the above alternatives fix the same problem? (or perhaps, is the per-inode throttling really necessary?)

Quoted 6 lines

Quoted 6 lines

This patch is based heavily (okay, almost entirely) on a prior patch by Peter Staubach. For the original patch, see http://article.gmane.org/gmane.linux.nfs/24323.

The patch below applies to linux-2.6.32-rc7, but it should apply cleanly to vanilla linux-2.6.32.

Quoted 4 lines

Quoted 4 lines

Performance data and tuning notes can be found on my web site (http://www.nec-labs.com/~sar).
With iozone, I see about 50% improvement for large sequential write workloads over a 1Gb Ethernet. With an in-house micro-benchmark, I see 80% improvement for large, single-stream, sequential workloads (where "large" is defined to be greater than the memory size on the client).

These are impressive numbers. I wonder what would be the minimal patch (just hacking it to fast, without all the aux bits)? Is it this chunk to call nfs_wb_eager()?

Quoted 17 lines

Quoted 17 lines

fs/nfs/file.c +8 -4

@@ -623,10 +635,21 @@ static ssize_t nfs_file_write(struct kio
      nfs_add_stats(inode, NFSIOS_NORMALWRITTENBYTES, count);
      result = generic_file_aio_write(iocb, iov, nr_segs, pos);
      /* Return error values for O_SYNC and IS_SYNC() */
-     if (result >= 0 && nfs_need_sync_write(iocb->ki_filp, inode)) {
-             int err = nfs_do_fsync(nfs_file_open_context(iocb->ki_filp), inode);
-             if (err < 0)
-                     result = err;
+     if (result >= 0) {
+             if (nfs_need_sync_write(iocb->ki_filp, inode)) {
+                     int err;
+
+                     err = nfs_do_fsync(nfs_file_open_context(iocb->ki_filp),
+                                        inode);
+                     if (err < 0)
+                             result = err;

Quoted 8 lines

fs/nfs/file.c static ssize_t nfs_file_write(struct kio · line 653

Quoted 8 lines

fs/nfs/file.c +7 -0

+             } else if (nfs_max_woutstanding != 0 &&
+                  nfs_is_seqwrite(inode, pos) &&
+                  atomic_read(&nfsi->ndirty) >= NFS_SERVER(inode)->wpages) {
+                     nfs_wb_eager(inode);
+             }
+             if (result > 0)
+                     nfsi->wrpos = pos + result;
      }

Thanks,
Fengguang

Quoted 9 lines · Wu Fengguang

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Steve Rago

Eager Writeback for NFS Clients
-------------------------------
Prevent applications that write large sequential streams of data (like backup, for example) from entering into a memory pressure state, which degrades performance by falling back to synchronous operations (both synchronous writes and additional commits).

Quoted 3 lines

What exactly is the "memory pressure state" condition? What's the code to do the "synchronous writes and additional commits" and maybe how they are triggered?

Memory pressure occurs when most of the client pages have been dirtied by an application (think backup server writing multi-gigabyte files that exceed the size of main memory). The system works harder to be able to free dirty pages so that they can be reused. For a local file system, this means writing the pages to disk. For NFS, however, the writes leave the pages in an "unstable" state until the server responds to a commit request. Generally speaking, commit processing is far more expensive than write processing on the server; both are done with the inode locked, but since the commit takes so long, all writes are blocked, which stalls the pipeline.

2) NFS commit stops pipeline because it sleep&wait inside i_mutex, which blocks all other NFSDs trying to write/writeback the inode.

nfsd_sync:
take i_mutex
filemap_fdatawrite
filemap_fdatawait
drop i_mutex

If filemap_fdatawait() can be moved out of i_mutex (or just remove the lock), we solve the root problem:

nfsd_sync:
[take i_mutex]
filemap_fdatawrite => can also be blocked, but less a problem
[drop i_mutex]
filemap_fdatawait

Maybe it's a dumb question, but what's the purpose of i_mutex here? For correctness or to prevent livelock? I can imagine some livelock problem here (current implementation can easily wait for extra pages), however not too hard to fix.

Quoted 10 lines

nfsd_sync:
[take i_mutex]
filemap_fdatawrite => can also be blocked, but less a problem
[drop i_mutex]
filemap_fdatawait

Maybe it's a dumb question, but what's the purpose of i_mutex here? For correctness or to prevent livelock? I can imagine some livelock problem here (current implementation can easily wait for extra pages), however not too hard to fix.

Generally, most filesystems take i_mutex during fsync to
a) avoid all sorts of livelocking problems
b) serialize fsyncs for one inode (mostly for simplicity)
I don't see what advantage would it bring that we get rid of i_mutex
for fdatawait - only that maybe writers could proceed while we are waiting but is that really the problem?

Quoted 10 lines · Jan Kara

Quoted 10 lines

nfsd_sync:
[take i_mutex]
filemap_fdatawrite => can also be blocked, but less a problem
[drop i_mutex]
filemap_fdatawait

Maybe it's a dumb question, but what's the purpose of i_mutex here? For correctness or to prevent livelock? I can imagine some livelock problem here (current implementation can easily wait for extra pages), however not too hard to fix.

Quoted 6 lines

Generally, most filesystems take i_mutex during fsync to
a) avoid all sorts of livelocking problems
b) serialize fsyncs for one inode (mostly for simplicity)
I don't see what advantage would it bring that we get rid of i_mutex
for fdatawait - only that maybe writers could proceed while we are waiting but is that really the problem?

It would match what we do in vfs_fsync for the non-nfsd path, so it's a no-brainer to do it. In fact I did switch it over to vfs_fsync a while ago but that go reverted because it caused deadlocks for nfsd_sync_dir which for some reason can't take the i_mutex (I'd have to check the archives why).

Here's a RFC patch to make some more sense of the fsync callers in nfsd, including fixing up the data write/wait calling conventions to match the regular fsync path (which might make this a -stable candidate):

Index: linux-2.6/fs/nfsd/vfs.c
===================================================================

fs/nfsd/vfs.c +15 -33

@@ -769,45 +769,27 @@ nfsd_close(struct file *filp)
 }
 
 /*
- * Sync a file
- * As this calls fsync (not fdatasync) there is no need for a write_inode
- * after it.
+ * Sync a directory to disk.
+ *
+ * This is odd compared to all other fsync callers because we
+ *
+ *  a) do not have a file struct available
+ *  b) expect to have i_mutex already held by the caller
  */
-static inline int nfsd_dosync(struct file *filp, struct dentry *dp,
-			      const struct file_operations *fop)
+int
+nfsd_sync_dir(struct dentry *dentry)
 {
-	struct inode *inode = dp->d_inode;
-	int (*fsync) (struct file *, struct dentry *, int);
+	struct inode *inode = dentry->d_inode;
 	int err;
 
-	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);
+	WARN_ON(!mutex_is_locked(&inode->i_mutex));
 
+	err = filemap_write_and_wait(inode->i_mapping);
+	if (err == 0 && inode->i_fop->fsync)
+		err = inode->i_fop->fsync(NULL, dentry, 0);
 	return err;
 }
 
-static int
-nfsd_sync(struct file *filp)
-{
-        int err;
-	struct inode *inode = filp->f_path.dentry->d_inode;
-	dprintk("nfsd: sync file %s\n", filp->f_path.dentry->d_name.name);
-	mutex_lock(&inode->i_mutex);
-	err=nfsd_dosync(filp, filp->f_path.dentry, filp->f_op);
-	mutex_unlock(&inode->i_mutex);
-
-	return err;
-}
-
-int
-nfsd_sync_dir(struct dentry *dp)
-{
-	return nfsd_dosync(NULL, dp, dp->d_inode->i_fop);
-}
-
 /*
  * Obtain the readahead parameters for the file
  * specified by (dev, ino).

@@ -1011,7 +993,7 @@ static int wait_for_concurrent_writes(st
 
 	if (inode->i_state & I_DIRTY) {
 		dprintk("nfsd: write sync %d\n", task_pid_nr(current));
-		err = nfsd_sync(file);
+		err = vfs_fsync(file, file->f_path.dentry, 0);
 	}
 	last_ino = inode->i_ino;
 	last_dev = inode->i_sb->s_dev;

@@ -1180,7 +1162,7 @@ nfsd_commit(struct svc_rqst *rqstp, stru
 		return err;
 	if (EX_ISSYNC(fhp->fh_export)) {
 		if (file->f_op && file->f_op->fsync) {
-			err = nfserrno(nfsd_sync(file));
+			err = nfserrno(vfs_fsync(file, file->f_path.dentry, 0));
 		} else {
 			err = nfserr_notsupp;
 		}

Quoted 24 lines · Christoph Hellwig

Quoted 10 lines · Jan Kara

Quoted 10 lines

nfsd_sync:
[take i_mutex]
filemap_fdatawrite => can also be blocked, but less a problem
[drop i_mutex]
filemap_fdatawait

Maybe it's a dumb question, but what's the purpose of i_mutex here? For correctness or to prevent livelock? I can imagine some livelock problem here (current implementation can easily wait for extra pages), however not too hard to fix.

Quoted 6 lines

Generally, most filesystems take i_mutex during fsync to
a) avoid all sorts of livelocking problems
b) serialize fsyncs for one inode (mostly for simplicity)
I don't see what advantage would it bring that we get rid of i_mutex
for fdatawait - only that maybe writers could proceed while we are waiting but is that really the problem?

It would match what we do in vfs_fsync for the non-nfsd path, so it's a no-brainer to do it. In fact I did switch it over to vfs_fsync a while ago but that go reverted because it caused deadlocks for nfsd_sync_dir which for some reason can't take the i_mutex (I'd have to check the archives why).

Quoted 3 lines

Here's a RFC patch to make some more sense of the fsync callers in nfsd, including fixing up the data write/wait calling conventions to match the regular fsync path (which might make this a -stable candidate):

The patch looks good to me from general soundness point of view :).
Someone with more NFS knowledge should tell whether dropping i_mutex for fdatawrite_and_wait is fine for NFS.

Quoted 66 lines

Index: linux-2.6/fs/nfsd/vfs.c
===================================================================

fs/nfsd/vfs.c +13 -31

@@ -769,45 +769,27 @@ nfsd_close(struct file *filp)
 }
 
 /*
- * Sync a file
- * As this calls fsync (not fdatasync) there is no need for a write_inode
- * after it.
+ * Sync a directory to disk.
+ *
+ * This is odd compared to all other fsync callers because we
+ *
+ *  a) do not have a file struct available
+ *  b) expect to have i_mutex already held by the caller
  */
-static inline int nfsd_dosync(struct file *filp, struct dentry *dp,
-			      const struct file_operations *fop)
+int
+nfsd_sync_dir(struct dentry *dentry)
 {
-	struct inode *inode = dp->d_inode;
-	int (*fsync) (struct file *, struct dentry *, int);
+	struct inode *inode = dentry->d_inode;
 	int err;
 
-	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);
+	WARN_ON(!mutex_is_locked(&inode->i_mutex));
 
+	err = filemap_write_and_wait(inode->i_mapping);
+	if (err == 0 && inode->i_fop->fsync)
+		err = inode->i_fop->fsync(NULL, dentry, 0);
 	return err;
 }
 
-static int
-nfsd_sync(struct file *filp)
-{
-        int err;
-	struct inode *inode = filp->f_path.dentry->d_inode;
-	dprintk("nfsd: sync file %s\n", filp->f_path.dentry->d_name.name);
-	mutex_lock(&inode->i_mutex);
-	err=nfsd_dosync(filp, filp->f_path.dentry, filp->f_op);
-	mutex_unlock(&inode->i_mutex);
-
-	return err;
-}
-
-int
-nfsd_sync_dir(struct dentry *dp)
-{
-	return nfsd_dosync(NULL, dp, dp->d_inode->i_fop);
-}
-
 /*
  * Obtain the readahead parameters for the file
  * specified by (dev, ino).

@@ -1011,7 +993,7 @@ static int wait_for_concurrent_writes(st
 

Quoted 16 lines

fs/nfsd/vfs.c +2 -2

 	if (inode->i_state & I_DIRTY) {
 		dprintk("nfsd: write sync %d\n", task_pid_nr(current));
-		err = nfsd_sync(file);
+		err = vfs_fsync(file, file->f_path.dentry, 0);
 	}
 	last_ino = inode->i_ino;
 	last_dev = inode->i_sb->s_dev;

@@ -1180,7 +1162,7 @@ nfsd_commit(struct svc_rqst *rqstp, stru
 		return err;
 	if (EX_ISSYNC(fhp->fh_export)) {
 		if (file->f_op && file->f_op->fsync) {
-			err = nfserrno(nfsd_sync(file));
+			err = nfserrno(vfs_fsync(file, file->f_path.dentry, 0));
 		} else {
 			err = nfserr_notsupp;
 		}

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 8 lines

2) NFS commit stops pipeline because it sleep&wait inside i_mutex, which blocks all other NFSDs trying to write/writeback the inode.

nfsd_sync:
take i_mutex
filemap_fdatawrite
filemap_fdatawait
drop i_mutex

I believe this is unrelated to the problem Steve is trying to solve.
When we get to doing sync writes the performance is busted so we better shouldn't get to that (unless user asked for that of course).

Quoted 9 lines

If filemap_fdatawait() can be moved out of i_mutex (or just remove the lock), we solve the root problem:

nfsd_sync:
[take i_mutex]
filemap_fdatawrite => can also be blocked, but less a problem
[drop i_mutex]
filemap_fdatawait

Quoted 4 lines

Maybe it's a dumb question, but what's the purpose of i_mutex here? For correctness or to prevent livelock? I can imagine some livelock problem here (current implementation can easily wait for extra pages), however not too hard to fix.

Generally, most filesystems take i_mutex during fsync to
a) avoid all sorts of livelocking problems
b) serialize fsyncs for one inode (mostly for simplicity)
I don't see what advantage would it bring that we get rid of i_mutex
for fdatawait - only that maybe writers could proceed while we are waiting but is that really the problem?

From: Wu Fengguang <[email protected]>
To: Steve Rago <sar-a+KepyhlMvJWk0Htik3J/[email protected]>
Cc: Peter Zijlstra <[email protected]>; "[email protected]" <[email protected]>; "[email protected]" <[email protected]>; "[email protected]" <[email protected]>; jens.axboe <[email protected]>
Sent: Tue, December 22, 2009 2:59:07 AM
Subject: Re: [PATCH] improve the performance of large sequential write NFS workloads

In general it's reasonable to keep NFS per-file nr_dirty low, however questionable to do per-file nr_writeback throttling. This does not work well with the global limits - eg. when there are many dirty files, the summed-up nr_writeback will still grow out of control. And it's more likely to impact user visible responsiveness than a global limit. But my opinion can be biased -- me have a patch to do global NFS nr_writeback limit ;)

----- Original Message ----

Quoted 6 lines

From: Wu Fengguang <[email protected]>
To: Steve Rago <sar-a+KepyhlMvJWk0Htik3J/[email protected]>
Cc: Peter Zijlstra <[email protected]>; "[email protected]" <[email protected]>; "[email protected]" <[email protected]>; "[email protected]" <[email protected]>; jens.axboe <[email protected]>
Sent: Tue, December 22, 2009 2:59:07 AM
Subject: Re: [PATCH] improve the performance of large sequential write NFS workloads

[big snip]

Quoted 9 lines

In general it's reasonable to keep NFS per-file nr_dirty low, however questionable to do per-file nr_writeback throttling. This does not work well with the global limits - eg. when there are many dirty files, the summed-up nr_writeback will still grow out of control. And it's more likely to impact user visible responsiveness than a global limit. But my opinion can be biased -- me have a patch to do global NFS nr_writeback limit ;)

is that "NFS: introduce writeback wait queue", which you sent me a while ago and I did not test until now :-( ?

Steve,

Quoted 15 lines · Steve Rago

Quoted 9 lines · Wu Fengguang

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Steve Rago

Eager Writeback for NFS Clients
-------------------------------
Prevent applications that write large sequential streams of data (like backup, for example) from entering into a memory pressure state, which degrades performance by falling back to synchronous operations (both synchronous writes and additional commits).

Quoted 3 lines

What exactly is the "memory pressure state" condition? What's the code to do the "synchronous writes and additional commits" and maybe how they are triggered?

Quoted 10 lines

Memory pressure occurs when most of the client pages have been dirtied by an application (think backup server writing multi-gigabyte files that exceed the size of main memory). The system works harder to be able to free dirty pages so that they can be reused. For a local file system, this means writing the pages to disk. For NFS, however, the writes leave the pages in an "unstable" state until the server responds to a commit request. Generally speaking, commit processing is far more expensive than write processing on the server; both are done with the inode locked, but since the commit takes so long, all writes are blocked, which stalls the pipeline.

Let me try reiterate the problem with code, please correct me if I'm wrong.

1) normal fs sets I_DIRTY_DATASYNC when extending i_size, however NFS will set the flag for any pages written -- why this trick? To guarantee the call of nfs_commit_inode()? Which unfortunately turns almost every server side NFS write into sync writes..

writeback_single_inode:

    do_writepages
      nfs_writepages
        nfs_writepage ----[short time later]---> nfs_writeback_release*
                                                   nfs_mark_request_commit
                                                     __mark_inode_dirty(I_DIRTY_DATASYNC);
                                    
    if (I_DIRTY_SYNC || I_DIRTY_DATASYNC)  <---- so this will be true for most time
      write_inode
        nfs_write_inode
          nfs_commit_inode

2) NFS commit stops pipeline because it sleep&wait inside i_mutex, which blocks all other NFSDs trying to write/writeback the inode.

nfsd_sync:
take i_mutex
filemap_fdatawrite
filemap_fdatawait
drop i_mutex

If filemap_fdatawait() can be moved out of i_mutex (or just remove the lock), we solve the root problem:

nfsd_sync:
[take i_mutex]
filemap_fdatawrite => can also be blocked, but less a problem
[drop i_mutex]
filemap_fdatawait

Maybe it's a dumb question, but what's the purpose of i_mutex here? For correctness or to prevent livelock? I can imagine some livelock problem here (current implementation can easily wait for extra pages), however not too hard to fix.

The proposed patch essentially takes two actions in nfs_file_write()
- to start writeback when the per-file nr_dirty goes high without committing
- to throttle dirtying when the per-file nr_writeback goes high I guess this effectively prevents pdflush from kicking in with its bad committing behavior

In general it's reasonable to keep NFS per-file nr_dirty low, however questionable to do per-file nr_writeback throttling. This does not work well with the global limits - eg. when there are many dirty files, the summed-up nr_writeback will still grow out of control.

And it's more likely to impact user visible responsiveness than a global limit. But my opinion can be biased -- me have a patch to do global NFS nr_writeback limit ;)

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

Quoted 29 lines · Wu Fengguang

Steve,

Quoted 25 lines · Steve Rago

Quoted 9 lines · Wu Fengguang

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Steve Rago

Eager Writeback for NFS Clients
-------------------------------
Prevent applications that write large sequential streams of data (like backup, for example) from entering into a memory pressure state, which degrades performance by falling back to synchronous operations (both synchronous writes and additional commits).

Quoted 3 lines

What exactly is the "memory pressure state" condition? What's the code to do the "synchronous writes and additional commits" and maybe how they are triggered?

Memory pressure occurs when most of the client pages have been dirtied by an application (think backup server writing multi-gigabyte files that exceed the size of main memory). The system works harder to be able to free dirty pages so that they can be reused. For a local file system, this means writing the pages to disk. For NFS, however, the writes leave the pages in an "unstable" state until the server responds to a commit request. Generally speaking, commit processing is far more expensive than write processing on the server; both are done with the inode locked, but since the commit takes so long, all writes are blocked, which stalls the pipeline.

Quoted 8 lines

Let me try reiterate the problem with code, please correct me if I'm wrong.

1) normal fs sets I_DIRTY_DATASYNC when extending i_size, however NFS will set the flag for any pages written -- why this trick? To guarantee the call of nfs_commit_inode()? Which unfortunately turns

almost every server side NFS write into sync writes..

Not really. The commit needs to be sent, but the writes are still asynchronous. It's just that the pages can't be recycled until they are on stable storage.

Quoted 32 lines

writeback_single_inode:

    do_writepages
      nfs_writepages
        nfs_writepage ----[short time later]---> nfs_writeback_release*
                                                   nfs_mark_request_commit
                                                     __mark_inode_dirty(I_DIRTY_DATASYNC);
                                    
    if (I_DIRTY_SYNC || I_DIRTY_DATASYNC)  <---- so this will be true for most time
      write_inode
        nfs_write_inode
          nfs_commit_inode

2) NFS commit stops pipeline because it sleep&wait inside i_mutex, which blocks all other NFSDs trying to write/writeback the inode.

nfsd_sync:
take i_mutex
filemap_fdatawrite
filemap_fdatawait
drop i_mutex

If filemap_fdatawait() can be moved out of i_mutex (or just remove the lock), we solve the root problem:

nfsd_sync:
[take i_mutex]
filemap_fdatawrite => can also be blocked, but less a problem
[drop i_mutex]
filemap_fdatawait

Quoted 4 lines

Maybe it's a dumb question, but what's the purpose of i_mutex here? For correctness or to prevent livelock? I can imagine some livelock problem here (current implementation can easily wait for extra pages), however not too hard to fix.

Commits and writes on the same inode need to be serialized for consistency (write can change the data and metadata; commit [fsync] needs to provide guarantees that the written data are stable). The performance problem arises because NFS writes are fast (they generally just deposit data into the server's page cache), but commits can take a

long time, especially if there is a lot of cached data to flush to stable storage.

Quoted 7 lines

The proposed patch essentially takes two actions in nfs_file_write()
- to start writeback when the per-file nr_dirty goes high without committing
- to throttle dirtying when the per-file nr_writeback goes high I guess this effectively prevents pdflush from kicking in with its bad committing behavior

Quoted 4 lines

In general it's reasonable to keep NFS per-file nr_dirty low, however questionable to do per-file nr_writeback throttling. This does not work well with the global limits - eg. when there are many dirty files, the summed-up nr_writeback will still grow out of control.

Not with the eager writeback patch. The nr_writeback for NFS is limited by the woutstanding tunable parameter multiplied by the number of active NFS files being written.

Quoted 3 lines

And it's more likely to impact user visible responsiveness than a global limit. But my opinion can be biased -- me have a patch to do global NFS nr_writeback limit ;)

What affects user-visible responsiveness is avoiding long delays and avoiding delays that vary widely. Whether the limit is global or per-file is less important (but I'd be happy to be convinced otherwise).

Quoted 5 lines

Commits and writes on the same inode need to be serialized for consistency (write can change the data and metadata; commit [fsync] needs to provide guarantees that the written data are stable). The performance problem arises because NFS writes are fast (they generally just deposit data into the server's page cache), but commits can take a

Right.

Quoted 2 lines

long time, especially if there is a lot of cached data to flush to stable storage.

"a lot of cached data to flush" is not likely with pdflush, since it roughly send one COMMIT per 4MB WRITEs. So in average each COMMIT syncs 4MB at the server side.

Your patch adds another pre-pdlush async write logic, which greatly reduced the number of COMMITs by pdflush. Can this be the major factor of the performance gain?

Jan has been proposing to change the pdflush logic from

        loop over dirty files {
                writeback 4MB
                write_inode
        }
to
        loop over dirty files {
                writeback all its dirty pages
                write_inode
        }

This should also be able to reduce the COMMIT numbers. I wonder if this (more general) approach can achieve the same performance gain.

Quoted 11 lines · Wu Fengguang

Quoted 5 lines

Commits and writes on the same inode need to be serialized for consistency (write can change the data and metadata; commit [fsync] needs to provide guarantees that the written data are stable). The performance problem arises because NFS writes are fast (they generally just deposit data into the server's page cache), but commits can take a

Right.

Quoted 2 lines

long time, especially if there is a lot of cached data to flush to stable storage.

Quoted 3 lines

"a lot of cached data to flush" is not likely with pdflush, since it roughly send one COMMIT per 4MB WRITEs. So in average each COMMIT syncs 4MB at the server side.

Maybe on paper, but empirically I see anywhere from one commit per 8MB to one commit per 64 MB.

Quoted 4 lines

Your patch adds another pre-pdlush async write logic, which greatly reduced the number of COMMITs by pdflush. Can this be the major factor of the performance gain?

My patch removes pdflush from the picture almost entirely. See my comments below.

Quoted 13 lines

Jan has been proposing to change the pdflush logic from

        loop over dirty files {
                writeback 4MB
                write_inode
        }
to
        loop over dirty files {
                writeback all its dirty pages
                write_inode
        }

Quoted 2 lines

This should also be able to reduce the COMMIT numbers. I wonder if this (more general) approach can achieve the same performance gain.

The pdflush mechanism is fine for random writes and small sequential writes, because it promotes concurrency -- instead of the application blocking while it tries to write and commit its data, the application can go on doing other more useful things, and the data gets flushed in the background. There is also a benefit if the application makes another modification to a page that is already dirty, because then multiple modifications are coalesced into a single write.

However, the pdflush mechanism is wrong for large sequential writes (like a backup stream, for example). First, there is no concurrency to exploit -- the application is only going to dirty more pages, so removing the need for it to block writing the pages out only adds to the problem of memory pressure. Second, the application is not going to go back and modify a page it has already written, so leaving it in the cache for someone else to write provides no additional benefit.

Note that this assumes the application actually cares about the consistency of its data and will call fsync() when it is done. If the application doesn't call fsync(), then it doesn't matter when the pages are written to backing store, because the interface makes no guarantees in this case.

1) normal fs sets I_DIRTY_DATASYNC when extending i_size, however NFS will set the flag for any pages written -- why this trick? To guarantee the call of nfs_commit_inode()? Which unfortunately turns almost every server side NFS write into sync writes..

writeback_single_inode:

    do_writepages
      nfs_writepages
        nfs_writepage ----[short time later]---> nfs_writeback_release*
                                                   nfs_mark_request_commit
                                                     __mark_inode_dirty(I_DIRTY_DATASYNC);
                                    
    if (I_DIRTY_SYNC || I_DIRTY_DATASYNC)  <---- so this will be true for most time
      write_inode
        nfs_write_inode
          nfs_commit_inode

Quoted 6 lines · Wu Fengguang

1) normal fs sets I_DIRTY_DATASYNC when extending i_size, however NFS will set the flag for any pages written -- why this trick? To guarantee the call of nfs_commit_inode()? Which unfortunately turns almost every server side NFS write into sync writes..

writeback_single_inode:

Quoted 10 lines

    do_writepages
      nfs_writepages
        nfs_writepage ----[short time later]---> nfs_writeback_release*
                                                   nfs_mark_request_commit
                                                     __mark_inode_dirty(I_DIRTY_DATASYNC);
                                    
    if (I_DIRTY_SYNC || I_DIRTY_DATASYNC)  <---- so this will be true for most time
      write_inode
        nfs_write_inode
          nfs_commit_inode

I have been working on a fix for this. We basically do want to ensure that NFS calls commit (otherwise we're not finished cleaning the dirty pages), but we want to do it _after_ we've waited for all the writes to complete. See below...

Trond

------------------------------------------------------------------------------------------------------
VFS: Add a new inode state: I_UNSTABLE_PAGES

From: Trond Myklebust <[email protected]>

Add a new inode state to enable the vfs to commit the nfs unstable pages to stable storage once the write back of dirty pages is done.

fs/fs-writeback.c +2 -0

@@ -474,6 +482,18 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	}
 
 	spin_lock(&inode_lock);
+	/*
+	 * Special state for cleaning NFS unstable pages

fs/nfs/inode.c +1 -6

index faa0918..4f129b3 100644
@@ -99,17 +99,14 @@ u64 nfs_compat_user_ino64(u64 fileid)
 
 int nfs_write_inode(struct inode *inode, int sync)
 {
+	int flags = 0;
 	int ret;
 
-	if (sync) {
-		ret = filemap_fdatawait(inode->i_mapping);
-		if (ret == 0)
-			ret = nfs_commit_inode(inode, FLUSH_SYNC);
-	} else
-		ret = nfs_commit_inode(inode, 0);

Quoted 13 lines · Trond Myklebust

fs/fs-writeback.c +9 -0

@@ -474,6 +482,18 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	}
 
 	spin_lock(&inode_lock);
+	/*
+	 * Special state for cleaning NFS unstable pages
+	 */
+	if (inode->i_state & I_UNSTABLE_PAGES) {
+		int err;
+		inode->i_state &= ~I_UNSTABLE_PAGES;
+		spin_unlock(&inode_lock);
+		err = commit_unstable_pages(inode, wait);
+		if (ret == 0)

fs/fs-writeback.c writeback_single_inode(struct inode *inode, struct writeback_control *wbc) · line 496

Quoted 3 lines

fs/fs-writeback.c +3 -0

+			ret = err;
+		spin_lock(&inode_lock);
+	}

I don't quite understand this chunk: We've called writeback_single_inode
because it had some dirty pages. Thus it has I_DIRTY_DATASYNC set and a few lines above your chunk, we've called nfs_write_inode which sent commit to the server. Now here you sometimes send the commit again? What's the purpose?

Quoted 14 lines · Jan Kara

Quoted 13 lines · Trond Myklebust

fs/fs-writeback.c +9 -0

@@ -474,6 +482,18 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	}
 
 	spin_lock(&inode_lock);
+	/*
+	 * Special state for cleaning NFS unstable pages
+	 */
+	if (inode->i_state & I_UNSTABLE_PAGES) {
+		int err;
+		inode->i_state &= ~I_UNSTABLE_PAGES;
+		spin_unlock(&inode_lock);
+		err = commit_unstable_pages(inode, wait);
+		if (ret == 0)

Quoted 8 lines

fs/fs-writeback.c writeback_single_inode(struct inode *inode, struct writeback_control *wbc) · line 496

Quoted 3 lines

fs/fs-writeback.c +3 -0

+			ret = err;
+		spin_lock(&inode_lock);
+	}

I don't quite understand this chunk: We've called writeback_single_inode
because it had some dirty pages. Thus it has I_DIRTY_DATASYNC set and a few lines above your chunk, we've called nfs_write_inode which sent commit to the server. Now here you sometimes send the commit again? What's the purpose?

We no longer set I_DIRTY_DATASYNC. We only set I_DIRTY_PAGES (and later I_UNSTABLE_PAGES).

The point is that we now do the commit only _after_ we've sent all the dirty pages, and waited for writeback to complete, whereas previously we did it in the wrong order.

Trond,

Quoted 24 lines · Trond Myklebust

Quoted 14 lines · Jan Kara

Quoted 13 lines · Trond Myklebust

fs/fs-writeback.c +9 -0

@@ -474,6 +482,18 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	}
 
 	spin_lock(&inode_lock);
+	/*
+	 * Special state for cleaning NFS unstable pages
+	 */
+	if (inode->i_state & I_UNSTABLE_PAGES) {
+		int err;
+		inode->i_state &= ~I_UNSTABLE_PAGES;
+		spin_unlock(&inode_lock);
+		err = commit_unstable_pages(inode, wait);
+		if (ret == 0)

Quoted 8 lines

fs/fs-writeback.c writeback_single_inode(struct inode *inode, struct writeback_control *wbc) · line 496

Quoted 3 lines

fs/fs-writeback.c +3 -0

+			ret = err;
+		spin_lock(&inode_lock);
+	}

I don't quite understand this chunk: We've called writeback_single_inode
because it had some dirty pages. Thus it has I_DIRTY_DATASYNC set and a few lines above your chunk, we've called nfs_write_inode which sent commit to the server. Now here you sometimes send the commit again? What's the purpose?

Quoted 6 lines

We no longer set I_DIRTY_DATASYNC. We only set I_DIRTY_PAGES (and later I_UNSTABLE_PAGES).

The point is that we now do the commit only _after_ we've sent all the dirty pages, and waited for writeback to complete, whereas previously we did it in the wrong order.

Sorry I still don't get it. The timing used to be:

write 4MB ==> WRITE block 0 (ie. first 512KB)
WRITE block 1
WRITE block 2
WRITE block 3 ack from server for WRITE block 0 => mark 0 as unstable (inode marked need-commit)
WRITE block 4 ack from server for WRITE block 1 => mark 1 as unstable
WRITE block 5 ack from server for WRITE block 2 => mark 2 as unstable
WRITE block 6 ack from server for WRITE block 3 => mark 3 as unstable
WRITE block 7 ack from server for WRITE block 4 => mark 4 as unstable
ack from server for WRITE block 5 => mark 5 as unstable
write_inode ==> COMMIT blocks 0-5
ack from server for WRITE block 6 => mark 6 as unstable (inode marked need-commit) ack from server for WRITE block 7 => mark 7 as unstable

ack from server for COMMIT blocks 0-5 => mark 0-5 as clean

write_inode ==> COMMIT blocks 6-7

ack from server for COMMIT blocks 6-7 => mark 6-7 as clean

Note that the first COMMIT is submitted before receiving all ACKs for the previous writes, hence the second COMMIT is necessary. It seems that your patch does not improve the timing at all.

Quoted 53 lines · Wu Fengguang

Trond,

Quoted 24 lines · Trond Myklebust

Quoted 14 lines · Jan Kara

Quoted 13 lines · Trond Myklebust

fs/fs-writeback.c +9 -0

@@ -474,6 +482,18 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	}
 
 	spin_lock(&inode_lock);
+	/*
+	 * Special state for cleaning NFS unstable pages
+	 */
+	if (inode->i_state & I_UNSTABLE_PAGES) {
+		int err;
+		inode->i_state &= ~I_UNSTABLE_PAGES;
+		spin_unlock(&inode_lock);
+		err = commit_unstable_pages(inode, wait);
+		if (ret == 0)

Quoted 8 lines

fs/fs-writeback.c writeback_single_inode(struct inode *inode, struct writeback_control *wbc) · line 496

Quoted 3 lines

fs/fs-writeback.c +3 -0

+			ret = err;
+		spin_lock(&inode_lock);
+	}

I don't quite understand this chunk: We've called writeback_single_inode
because it had some dirty pages. Thus it has I_DIRTY_DATASYNC set and a few lines above your chunk, we've called nfs_write_inode which sent commit to the server. Now here you sometimes send the commit again? What's the purpose?

Quoted 6 lines

We no longer set I_DIRTY_DATASYNC. We only set I_DIRTY_PAGES (and later I_UNSTABLE_PAGES).

The point is that we now do the commit only _after_ we've sent all the dirty pages, and waited for writeback to complete, whereas previously we did it in the wrong order.

Sorry I still don't get it. The timing used to be:

write 4MB ==> WRITE block 0 (ie. first 512KB)
WRITE block 1
WRITE block 2
WRITE block 3 ack from server for WRITE block 0 => mark 0 as unstable (inode marked need-commit)
WRITE block 4 ack from server for WRITE block 1 => mark 1 as unstable
WRITE block 5 ack from server for WRITE block 2 => mark 2 as unstable
WRITE block 6 ack from server for WRITE block 3 => mark 3 as unstable
WRITE block 7 ack from server for WRITE block 4 => mark 4 as unstable
ack from server for WRITE block 5 => mark 5 as unstable
write_inode ==> COMMIT blocks 0-5
ack from server for WRITE block 6 => mark 6 as unstable (inode marked need-commit) ack from server for WRITE block 7 => mark 7 as unstable

ack from server for COMMIT blocks 0-5 => mark 0-5 as clean

write_inode ==> COMMIT blocks 6-7

Quoted 5 lines

ack from server for COMMIT blocks 6-7 => mark 6-7 as clean

Note that the first COMMIT is submitted before receiving all ACKs for the previous writes, hence the second COMMIT is necessary. It seems that your patch does not improve the timing at all.

That would indicate that we're cycling through writeback_single_inode() more than once. Why?

Quoted 2 lines · Trond Myklebust

That would indicate that we're cycling through writeback_single_inode() more than once. Why?

Yes. The above sequence can happen for a 4MB sized dirty file. The first COMMIT is done by L547, while the second COMMIT will be scheduled either by __mark_inode_dirty(), or scheduled by L583 (depending on the time ACKs for L543 but missed L547 arrives:
if an ACK missed L578, the inode will be queued into b_dirty list, but if any ACK arrives between L547 and L578, the inode will enter b_more_io_wait, which is a to-be-introduced new dirty list).

         537         dirty = inode->i_state & I_DIRTY;
         538         inode->i_state |= I_SYNC;
         539         inode->i_state &= ~I_DIRTY;
         540 
         541         spin_unlock(&inode_lock);
         542 
==>      543         ret = do_writepages(mapping, wbc);
         544 
         545         /* Don't write the inode if only I_DIRTY_PAGES was set */
         546         if (dirty & (I_DIRTY_SYNC | I_DIRTY_DATASYNC)) {
==>      547                 int err = write_inode(inode, wait);
         548                 if (ret == 0)
         549                         ret = err;
         550         }
         551 
         552         if (wait) {
         553                 int err = filemap_fdatawait(mapping);
         554                 if (ret == 0)
         555                         ret = err;
         556         }
         557 
         558         spin_lock(&inode_lock);
         559         inode->i_state &= ~I_SYNC;
         560         if (!(inode->i_state & (I_FREEING | I_CLEAR))) {
         561                 if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
         562                         /*
         563                          * We didn't write back all the pages.  nfs_writepages()
         564                          * sometimes bales out without doing anything.
         565                          */
         566                         inode->i_state |= I_DIRTY_PAGES;
         567                         if (wbc->nr_to_write <= 0) {
         568                                 /*
         569                                  * slice used up: queue for next turn
         570                                  */
         571                                 requeue_io(inode);
         572                         } else {
         573                                 /*
         574                                  * somehow blocked: retry later
         575                                  */
         576                                 requeue_io_wait(inode);
         577                         }

==> 578 } else if (inode->i_state & I_DIRTY) {
579 /*
580 * At least XFS will redirty the inode during the
581 * writeback (delalloc) and on io completion (isize).
582 */
==> 583 requeue_io_wait(inode);

it ignores the commit request if the caller is just doing a WB_SYNC_NONE background flush, waiting instead for the ensuing WB_SYNC_ALL request...

---
 fs/nfs/inode.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

fs/nfs/inode.c +0 -0

@@ -105,8 +105,11 @@ int nfs_write_inode(struct inode *inode,
 		ret = filemap_fdatawait(inode->i_mapping);
 		if (ret == 0)

Quoted 18 lines · Wu Fengguang

---
 fs/nfs/inode.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

fs/nfs/inode.c +4 -1

@@ -105,8 +105,11 @@ int nfs_write_inode(struct inode *inode,
 		ret = filemap_fdatawait(inode->i_mapping);
 		if (ret == 0)
 			ret = nfs_commit_inode(inode, FLUSH_SYNC);
-	} else
+	} else if (!radix_tree_tagged(&NFS_I(inode)->nfs_page_tree,
+				      NFS_PAGE_TAG_LOCKED))
 		ret = nfs_commit_inode(inode, 0);
+	else
+		ret = -EAGAIN;
 	if (ret >= 0)
 		return 0;

fs/nfs/inode.c int nfs_write_inode(struct inode *inode, · line 115

Quoted 1 line

fs/nfs/inode.c +0 -0

 	__mark_inode_dirty(inode, I_DIRTY_DATASYNC);

The above change improves on the existing code, but doesn't solve the problem that write_inode() isn't a good match for COMMIT. We need to wait for all the unstable WRITE rpc calls to return before we can know whether or not a COMMIT is needed (some commercial servers never require commit, even if the client requested an unstable write). That was the other reason for the change.

I do, however, agree that the above can provide a nice heuristic for the WB_SYNC_NONE case (minus the -EAGAIN error). Mind if I integrate it?

------------------------------------------------------------------------------------------------------------
VFS: Ensure that writeback_single_inode() commits unstable writes

From: Trond Myklebust <[email protected]>

If the call to do_writepages() succeeded in starting writeback, we do not know whether or not we will need to COMMIT any unstable writes until after the write RPC calls are finished. Currently, we assume that at least one write RPC call will have finished, and set I_DIRTY_DATASYNC by the time do_writepages is done, so that write_inode() is triggered.

In order to ensure reliable operation (i.e. ensure that a single call to writeback_single_inode() with WB_SYNC_ALL set suffices to ensure that pages are on disk) we need to first wait for filemap_fdatawait() to complete, then test for unstable pages.

Since NFS is currently the only filesystem that has unstable pages, we can add a new inode state I_UNSTABLE_PAGES that NFS alone will set. When set, this will trigger a callback to a new address_space_operation to call the COMMIT.

Signed-off-by Trond Myklebust <[email protected]>

---

 fs/fs-writeback.c  |   31 ++++++++++++++++++++++++++++++-
 fs/nfs/file.c      |    1 +
 fs/nfs/inode.c     |   16 ----------------
 fs/nfs/internal.h  |    3 ++-
 fs/nfs/super.c     |    2 --
 fs/nfs/write.c     |   33 ++++++++++++++++++++++++++++++++-
 include/linux/fs.h |    9 +++++++++
 7 files changed, 74 insertions(+), 21 deletions(-)

fs/fs-writeback.c +30 -1

index f6c2155..b25efbb 100644
@@ -388,6 +388,17 @@ static int write_inode(struct inode *inode, int sync)
 }
 
 /*
+ * Commit the NFS unstable pages.
+ */
+static int commit_unstable_pages(struct address_space *mapping,
+		struct writeback_control *wbc)
+{
+	if (mapping->a_ops && mapping->a_ops->commit_unstable_pages)
+		return mapping->a_ops->commit_unstable_pages(mapping, wbc);
+	return 0;
+}
+
+/*
  * Wait for writeback on an inode to complete.
  */
 static void inode_wait_for_writeback(struct inode *inode)

@@ -474,6 +485,18 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	}
 
 	spin_lock(&inode_lock);
+	/*
+	 * Special state for cleaning NFS unstable pages
+	 */
+	if (inode->i_state & I_UNSTABLE_PAGES) {
+		int err;
+		inode->i_state &= ~I_UNSTABLE_PAGES;
+		spin_unlock(&inode_lock);
+		err = commit_unstable_pages(mapping, wbc);
+		if (ret == 0)
+			ret = err;
+		spin_lock(&inode_lock);
+	}
 	inode->i_state &= ~I_SYNC;
 	if (!(inode->i_state & (I_FREEING | I_CLEAR))) {
 		if ((inode->i_state & I_DIRTY_PAGES) && wbc->for_kupdate) {

@@ -532,6 +555,12 @@ select_queue:
 				inode->i_state |= I_DIRTY_PAGES;
 				redirty_tail(inode);
 			}
+		} else if (inode->i_state & I_UNSTABLE_PAGES) {
+			/*
+			 * The inode has got yet more unstable pages to
+			 * commit. Requeue on b_more_io
+			 */
+			requeue_io(inode);
 		} else if (atomic_read(&inode->i_count)) {
 			/*
 			 * The inode is clean, inuse

@@ -1050,7 +1079,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
 
 	spin_lock(&inode_lock);
 	if ((inode->i_state & flags) != flags) {
-		const int was_dirty = inode->i_state & I_DIRTY;
+		const int was_dirty = inode->i_state & (I_DIRTY|I_UNSTABLE_PAGES);
 
 		inode->i_state |= flags;
 

fs/nfs/file.c +1 -0

index 6b89132..67e50ac 100644
@@ -526,6 +526,7 @@ const struct address_space_operations nfs_file_aops = {
 	.migratepage = nfs_migrate_page,
 	.launder_page = nfs_launder_page,
 	.error_remove_page = generic_error_remove_page,
+	.commit_unstable_pages = nfs_commit_unstable_pages,
 };
 
 /*

fs/nfs/inode.c +0 -16

index faa0918..8341709 100644
@@ -97,22 +97,6 @@ u64 nfs_compat_user_ino64(u64 fileid)
 	return ino;
 }
 
-int nfs_write_inode(struct inode *inode, int sync)
-{
-	int ret;
-
-	if (sync) {
-		ret = filemap_fdatawait(inode->i_mapping);
-		if (ret == 0)
-			ret = nfs_commit_inode(inode, FLUSH_SYNC);
-	} else
-		ret = nfs_commit_inode(inode, 0);
-	if (ret >= 0)
-		return 0;
-	__mark_inode_dirty(inode, I_DIRTY_DATASYNC);
-	return ret;
-}
-
 void nfs_clear_inode(struct inode *inode)
 {
 	/*

fs/nfs/internal.h +2 -1

index 29e464d..7bb326f 100644
@@ -211,7 +211,6 @@ extern int nfs_access_cache_shrinker(int nr_to_scan, gfp_t gfp_mask);
 extern struct workqueue_struct *nfsiod_workqueue;
 extern struct inode *nfs_alloc_inode(struct super_block *sb);
 extern void nfs_destroy_inode(struct inode *);
-extern int nfs_write_inode(struct inode *,int);
 extern void nfs_clear_inode(struct inode *);
 #ifdef CONFIG_NFS_V4
 extern void nfs4_clear_inode(struct inode *);

@@ -253,6 +252,8 @@ extern int nfs4_path_walk(struct nfs_server *server,
 extern void nfs_read_prepare(struct rpc_task *task, void *calldata);
 
 /* write.c */
+extern int nfs_commit_unstable_pages(struct address_space *mapping,
+		struct writeback_control *wbc);
 extern void nfs_write_prepare(struct rpc_task *task, void *calldata);
 #ifdef CONFIG_MIGRATION
 extern int nfs_migrate_page(struct address_space *,

fs/nfs/super.c +0 -2

index ce907ef..805c1a0 100644
@@ -265,7 +265,6 @@ struct file_system_type nfs_xdev_fs_type = {
 static const struct super_operations nfs_sops = {
 	.alloc_inode	= nfs_alloc_inode,
 	.destroy_inode	= nfs_destroy_inode,
-	.write_inode	= nfs_write_inode,
 	.statfs		= nfs_statfs,
 	.clear_inode	= nfs_clear_inode,
 	.umount_begin	= nfs_umount_begin,

@@ -334,7 +333,6 @@ struct file_system_type nfs4_referral_fs_type = {
 static const struct super_operations nfs4_sops = {
 	.alloc_inode	= nfs_alloc_inode,
 	.destroy_inode	= nfs_destroy_inode,
-	.write_inode	= nfs_write_inode,
 	.statfs		= nfs_statfs,
 	.clear_inode	= nfs4_clear_inode,
 	.umount_begin	= nfs_umount_begin,

fs/nfs/write.c +32 -1

index d171696..910be28 100644
@@ -441,7 +441,7 @@ nfs_mark_request_commit(struct nfs_page *req)
 	spin_unlock(&inode->i_lock);
 	inc_zone_page_state(req->wb_page, NR_UNSTABLE_NFS);
 	inc_bdi_stat(req->wb_page->mapping->backing_dev_info, BDI_RECLAIMABLE);
-	__mark_inode_dirty(inode, I_DIRTY_DATASYNC);
+	mark_inode_unstable_pages(inode);
 }
 
 static int

@@ -1406,11 +1406,42 @@ int nfs_commit_inode(struct inode *inode, int how)
 	}
 	return res;
 }
+
+int nfs_commit_unstable_pages(struct address_space *mapping,
+		struct writeback_control *wbc)
+{
+	struct inode *inode = mapping->host;
+	int flags = FLUSH_SYNC;
+	int ret;
+
+	/* Don't commit yet if this is a non-blocking flush and there are
+	 * outstanding writes for this mapping.
+	 */
+	if (wbc->sync_mode != WB_SYNC_ALL &&
+	    radix_tree_tagged(&NFS_I(inode)->nfs_page_tree,
+		    NFS_PAGE_TAG_LOCKED)) {
+		mark_inode_unstable_pages(inode);
+		return 0;
+	}
+	if (wbc->nonblocking)
+		flags = 0;
+	ret = nfs_commit_inode(inode, flags);
+	if (ret > 0)
+		ret = 0;
+	return ret;
+}
+
 #else
 static inline int nfs_commit_list(struct inode *inode, struct list_head *head, int how)
 {
 	return 0;
 }
+
+int nfs_commit_unstable_pages(struct address_space *mapping,
+		struct writeback_control *wbc)
+{
+	return 0;
+}
 #endif
 
 long nfs_sync_mapping_wait(struct address_space *mapping, struct writeback_control *wbc, int how)

include/linux/fs.h +9 -0

index 9147ca8..ea0b7a3 100644
@@ -602,6 +602,8 @@ struct address_space_operations {
 	int (*is_partially_uptodate) (struct page *, read_descriptor_t *,
 					unsigned long);
 	int (*error_remove_page)(struct address_space *, struct page *);
+	int (*commit_unstable_pages)(struct address_space *,
+			struct writeback_control *);
 };
 
 /*

@@ -1635,6 +1637,8 @@ struct super_operations {
 #define I_CLEAR			64
 #define __I_SYNC		7
 #define I_SYNC			(1 << __I_SYNC)
+#define __I_UNSTABLE_PAGES	9
+#define I_UNSTABLE_PAGES	(1 << __I_UNSTABLE_PAGES)
 
 #define I_DIRTY (I_DIRTY_SYNC | I_DIRTY_DATASYNC | I_DIRTY_PAGES)
 

@@ -1649,6 +1653,11 @@ static inline void mark_inode_dirty_sync(struct inode *inode)
 	__mark_inode_dirty(inode, I_DIRTY_SYNC);
 }
 
+static inline void mark_inode_unstable_pages(struct inode *inode)
+{
+	__mark_inode_dirty(inode, I_UNSTABLE_PAGES);
+}
+
 /**
  * inc_nlink - directly increment an inode's link count
  * @inode: inode