Re: batched write

["Vladimir V. Saveliev" <vs@namesys.com>]

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

Hello

On Tue, 2006-06-20 at 00:26 -0700, Andrew Morton wrote:
> On Tue, 20 Jun 2006 00:19:24 -0700
> Hans Reiser <reiser@namesys.com> wrote:
> 
> > So far we have XFS, FUSE, and reiser4 benefiting from the potential
> > ability to process more than 4k at a time.  Is it enough?
> 
> Spose so.  Let's see what the diff looks like?
> 
ok, the first draft version for evaluation is here.

[-- Attachment #2: batched-write.patch --]
[-- Type: text/x-patch, Size: 8721 bytes --]


This patch adds a method fill_pages to struct address_space_operations.
A filesystem may want to implement this operation to improve write performance.
Generic implementation for the method is made by cut-n-paste off generic_file_buffered_write:
it writes one page using prepare_write and commit_write address space operations.

NOTE: just for evaluation, compiliable only yet. 



diff -puN mm/filemap.c~batched-write mm/filemap.c
--- linux-2.6.17-rc6-mm2/mm/filemap.c~batched-write	2006-06-12 16:06:43.000000000 +0400
+++ linux-2.6.17-rc6-mm2-vs/mm/filemap.c	2006-06-20 19:52:53.000000000 +0400
@@ -2093,68 +2093,78 @@ generic_file_direct_write(struct kiocb *
 }
 EXPORT_SYMBOL(generic_file_direct_write);
 
-ssize_t
-generic_file_buffered_write(struct kiocb *iocb, const struct iovec *iov,
-		unsigned long nr_segs, loff_t pos, loff_t *ppos,
-		size_t count, ssize_t written)
+typedef size_t (*write_actor_t)(struct page *, unsigned long, size_t,
+				const write_descriptor_t *);
+struct write_descriptor {
+	loff_t pos;
+	size_t count;
+	const struct iovec *cur_iov;	/* current iovec */
+	size_t iov_base;		/* offset in the current iovec */
+	char __user *buf;
+	write_actor_t actor;
+};
+
+static size_t write_actor(struct page *page, unsigned long offset, size_t bytes,
+			  const write_descriptor_t *desc)
 {
-	struct file *file = iocb->ki_filp;
-	struct address_space * mapping = file->f_mapping;
-	const struct address_space_operations *a_ops = mapping->a_ops;
-	struct inode 	*inode = mapping->host;
-	long		status = 0;
-	struct page	*page;
-	struct page	*cached_page = NULL;
-	size_t		bytes;
-	struct pagevec	lru_pvec;
-	const struct iovec *cur_iov = iov; /* current iovec */
-	size_t		iov_base = 0;	   /* offset in the current iovec */
-	char __user	*buf;
+	return filemap_copy_from_user(page, offset, desc->buf, bytes);
+}
 
-	pagevec_init(&lru_pvec, 0);
+static size_t write_iovec_actor(struct page *page, unsigned long offset,
+				size_t bytes, const write_descriptor_t *desc)
+{
+	return filemap_copy_from_user_iovec(page, offset, desc->cur_iov,
+					    desc->iov_base, bytes);
+}
+
+/**
+ * generic_fill_pages
+ * @file:
+ * @desc:
+ * @lru_pvec:
+ * @cached_page:
+ * @copied:
+ *
+ * Number of bytes written is returned via @copied.
+ */
+long generic_fill_pages(struct file *file, const write_descriptor_t *desc,
+			struct pagevec *lru_pvec, struct page **cached_page,
+			size_t *copied)
+{
+	const struct address_space_operations *a_ops = file->f_mapping->a_ops;
+	struct page *page;
+	unsigned long index;
+	size_t bytes;
+	unsigned long offset;
+	unsigned long maxlen;
+	long status;
+
+	offset = (desc->pos & (PAGE_CACHE_SIZE - 1)); /* Within page */
+	index = desc->pos >> PAGE_CACHE_SHIFT;
+	bytes = PAGE_CACHE_SIZE - offset;
+	if (bytes > desc->count)
+		bytes = desc->count;
 
 	/*
-	 * handle partial DIO write.  Adjust cur_iov if needed.
+	 * Bring in the user page that we will copy from _first_.
+	 * Otherwise there's a nasty deadlock on copying from the same
+	 * page as we're writing to, without it being marked
+	 * up-to-date.
 	 */
-	if (likely(nr_segs == 1))
-		buf = iov->iov_base + written;
-	else {
-		filemap_set_next_iovec(&cur_iov, &iov_base, written);
-		buf = cur_iov->iov_base + iov_base;
-	}
-
-	do {
-		unsigned long index;
-		unsigned long offset;
-		unsigned long maxlen;
-		size_t copied;
-
-		offset = (pos & (PAGE_CACHE_SIZE -1)); /* Within page */
-		index = pos >> PAGE_CACHE_SHIFT;
-		bytes = PAGE_CACHE_SIZE - offset;
-		if (bytes > count)
-			bytes = count;
-
-		/*
-		 * Bring in the user page that we will copy from _first_.
-		 * Otherwise there's a nasty deadlock on copying from the
-		 * same page as we're writing to, without it being marked
-		 * up-to-date.
-		 */
-		maxlen = cur_iov->iov_len - iov_base;
-		if (maxlen > bytes)
-			maxlen = bytes;
-		fault_in_pages_readable(buf, maxlen);
-
-		page = __grab_cache_page(mapping,index,&cached_page,&lru_pvec);
-		if (!page) {
-			status = -ENOMEM;
-			break;
-		}
+	maxlen = desc->cur_iov->iov_len - desc->iov_base;
+	if (maxlen > bytes)
+		maxlen = bytes;
+
+	while (1) {
+		fault_in_pages_readable(desc->buf, maxlen);
+
+		page = __grab_cache_page(file->f_mapping, index, cached_page, lru_pvec);
+		if (!page)
+			return -ENOMEM;
 
 		status = a_ops->prepare_write(file, page, offset, offset+bytes);
 		if (unlikely(status)) {
-			loff_t isize = i_size_read(inode);
+			loff_t isize = i_size_read(file->f_mapping->host);
 
 			if (status != AOP_TRUNCATED_PAGE)
 				unlock_page(page);
@@ -2162,51 +2172,87 @@ generic_file_buffered_write(struct kiocb
 			if (status == AOP_TRUNCATED_PAGE)
 				continue;
 			/*
-			 * prepare_write() may have instantiated a few blocks
-			 * outside i_size.  Trim these off again.
+			 * prepare_write() may have instantiated a few
+			 * blocks outside i_size.  Trim these off
+			 * again.
 			 */
-			if (pos + bytes > isize)
-				vmtruncate(inode, isize);
-			break;
+			if (desc->pos + bytes > isize)
+				vmtruncate(file->f_mapping->host, isize);
+			return status;
 		}
-		if (likely(nr_segs == 1))
-			copied = filemap_copy_from_user(page, offset,
-							buf, bytes);
-		else
-			copied = filemap_copy_from_user_iovec(page, offset,
-						cur_iov, iov_base, bytes);
+
+		*copied = desc->actor(page, offset, bytes, desc);
+
 		flush_dcache_page(page);
 		status = a_ops->commit_write(file, page, offset, offset+bytes);
 		if (status == AOP_TRUNCATED_PAGE) {
 			page_cache_release(page);
 			continue;
 		}
-		if (likely(copied > 0)) {
-			if (!status)
-				status = copied;
 
-			if (status >= 0) {
-				written += status;
-				count -= status;
-				pos += status;
-				buf += status;
-				if (unlikely(nr_segs > 1)) {
-					filemap_set_next_iovec(&cur_iov,
-							&iov_base, status);
-					if (count)
-						buf = cur_iov->iov_base +
-							iov_base;
-				} else {
-					iov_base += status;
-				}
-			}
-		}
-		if (unlikely(copied != bytes))
-			if (status >= 0)
-				status = -EFAULT;
 		unlock_page(page);
 		mark_page_accessed(page);
 		page_cache_release(page);
+		break;
+	}
+	if (status)
+		*copied = 0;
+	else if (*copied != bytes)
+		status = -EFAULT;
+	return status;
+}
+
+ssize_t
+generic_file_buffered_write(struct kiocb *iocb, const struct iovec *iov,
+			    unsigned long nr_segs, loff_t pos, loff_t *ppos,
+			    size_t count, ssize_t written)
+{
+	struct file *file = iocb->ki_filp;
+	struct address_space * mapping = file->f_mapping;
+	const struct address_space_operations *a_ops = mapping->a_ops;
+	struct inode 	*inode = mapping->host;
+	long		status;
+	struct page	*cached_page = NULL;
+	struct pagevec	lru_pvec;
+	write_descriptor_t desc;
+	size_t copied = 0;
+
+	pagevec_init(&lru_pvec, 0);
+
+	desc.pos = pos;
+	desc.count = count;
+	desc.cur_iov = iov;
+	desc.iov_base = 0;
+
+	/*
+	 * handle partial DIO write.  Adjust cur_iov if needed.
+	 */
+	if (likely(nr_segs == 1)) {
+		desc.buf = iov->iov_base + written;
+		desc.actor = write_actor;
+	} else {
+		filemap_set_next_iovec(&desc.cur_iov, &desc.iov_base, written);
+		desc.buf = desc.cur_iov->iov_base + desc.iov_base;
+		desc.actor = write_iovec_actor;
+	}
+
+	do {
+		status = mapping->a_ops->fill_pages(file, &desc,
+						    &lru_pvec, &cached_page, &copied);
+		if (likely(copied > 0)) {
+			written += copied;
+			desc.count -= copied;
+			desc.pos += copied;
+			desc.buf += copied;
+			if (unlikely(nr_segs > 1)) {
+				filemap_set_next_iovec(&desc.cur_iov,
+						       &desc.iov_base, copied);
+				if (count)
+					desc.buf = desc.cur_iov->iov_base + desc.iov_base;
+			} else {
+				desc.iov_base += copied;
+			}
+		}
 		if (status < 0)
 			break;
 		balance_dirty_pages_ratelimited(mapping);
diff -puN include/linux/fs.h~batched-write include/linux/fs.h
--- linux-2.6.17-rc6-mm2/include/linux/fs.h~batched-write	2006-06-16 19:12:47.000000000 +0400
+++ linux-2.6.17-rc6-mm2-vs/include/linux/fs.h	2006-06-20 19:40:26.000000000 +0400
@@ -346,6 +346,9 @@ enum positive_aop_returns {
 struct page;
 struct address_space;
 struct writeback_control;
+typedef struct write_descriptor write_descriptor_t;
+
+#include <linux/pagevec.h>
 
 struct address_space_operations {
 	int (*writepage)(struct page *page, struct writeback_control *wbc);
@@ -367,6 +370,9 @@ struct address_space_operations {
 	 */
 	int (*prepare_write)(struct file *, struct page *, unsigned, unsigned);
 	int (*commit_write)(struct file *, struct page *, unsigned, unsigned);
+	long (*fill_pages)(struct file *, const write_descriptor_t *,
+			   struct pagevec *, struct page **, size_t *copied);
+
 	/* Unfortunately this kludge is needed for FIBMAP. Don't use it */
 	sector_t (*bmap)(struct address_space *, sector_t);
 	void (*invalidatepage) (struct page *, unsigned long);

_