+ fs-introduce-write_begin-write_end-and-perform_write-aops.patch added to -mm tree

The patch titled
fs: introduce write_begin, write_end, and perform_write aops
has been added to the -mm tree. Its filename is
fs-introduce-write_begin-write_end-and-perform_write-aops.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find out what to do about this

------------------------------------------------------
Subject: fs: introduce write_begin, write_end, and perform_write aops
From: Nick Piggin <[email protected]>

These are intended to replace prepare_write and commit_write with more flexible alternatives that are also able to avoid the buffered write deadlock problems efficiently (which prepare_write is unable to do).

Quoted 11 lines · Andrew Morton

The patch titled
fs: introduce write_begin, write_end, and perform_write aops
has been added to the -mm tree. Its filename is
fs-introduce-write_begin-write_end-and-perform_write-aops.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find out what to do about this

Quoted 7 lines

------------------------------------------------------
Subject: fs: introduce write_begin, write_end, and perform_write aops
From: Nick Piggin <[email protected]>

These are intended to replace prepare_write and commit_write with more flexible alternatives that are also able to avoid the buffered write deadlock problems efficiently (which prepare_write is unable to do).

OK, well now Andrew's merged a significant chunk of this work, I would like to try getting the clustered filesystem patches back in too (Steven, the last GFS2 patch you sent had rejects against this tree, so I dropped it... hope it isn't too much work to bring it back uptodate?).

The cluster filesystems aren't 100% happy with the backward-compat code, because pagecache_write_end cannot handle AOP_TRUNCATED_PAGE from ->commit_write... so if you were to try using loop over GFS2, it might go BUG. This is a bit bad of me, however the compat code would have been a whole lot uglier to support that, and I figure the cluster filesystems want to convert to the new aops ASAP anyway.

I doubt anybody but the filesystem developers would be using -mm in such a way, but even so I hope we can fix this before long.

Meanwhile, I'll look at redoing the rest of the filesystems that got left behind.

hm, I suppose that means I need to undrop git-ocfs2.patch. It has a mild disagreeement with the fault-vs-invalidate patches which I didn't feel like fixing.

Quoted 3 lines · Andrew Morton

hm, I suppose that means I need to undrop git-ocfs2.patch. It has a mild disagreeement with the fault-vs-invalidate patches which I didn't feel like fixing.

Erf, somehow I missed that it was dropped.

I guess I'll take a look...

Quoted 11 lines · Andrew Morton

The patch titled
fs: introduce write_begin, write_end, and perform_write aops
has been added to the -mm tree. Its filename is
fs-introduce-write_begin-write_end-and-perform_write-aops.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find out what to do about this

Quoted 7 lines

------------------------------------------------------
Subject: fs: introduce write_begin, write_end, and perform_write aops
From: Nick Piggin <[email protected]>

These are intended to replace prepare_write and commit_write with more flexible alternatives that are also able to avoid the buffered write deadlock problems efficiently (which prepare_write is unable to do).

OK, well now Andrew's merged a significant chunk of this work, I would like to try getting the clustered filesystem patches back in too (Steven, the last GFS2 patch you sent had rejects against this tree, so I dropped it... hope it isn't too much work to bring it back uptodate?).

Hi,

Quoted 20 lines · Nick Piggin

Quoted 11 lines · Andrew Morton

The patch titled
fs: introduce write_begin, write_end, and perform_write aops
has been added to the -mm tree. Its filename is
fs-introduce-write_begin-write_end-and-perform_write-aops.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find out what to do about this

Quoted 7 lines

------------------------------------------------------
Subject: fs: introduce write_begin, write_end, and perform_write aops
From: Nick Piggin <[email protected]>

These are intended to replace prepare_write and commit_write with more flexible alternatives that are also able to avoid the buffered write deadlock problems efficiently (which prepare_write is unable to do).

Quoted 6 lines

OK, well now Andrew's merged a significant chunk of this work, I would like to try getting the clustered filesystem patches back in too (Steven, the last GFS2 patch you sent had rejects against this tree, so I dropped it... hope it isn't too much work to bring it back uptodate?).

I think the following should do the trick... sorry for the delay, I was on holiday last week and I'm just catching up again. There is not a lot of change from the previous version, just a few small changes in the upstream code which had caused one chunk not to apply,

The patch titled
fs: introduce write_begin, write_end, and perform_write aops
has been added to the -mm tree. Its filename is
fs-introduce-write_begin-write_end-and-perform_write-aops.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find out what to do about this

------------------------------------------------------
Subject: fs: introduce write_begin, write_end, and perform_write aops
From: Nick Piggin <[email protected]>

These are intended to replace prepare_write and commit_write with more flexible alternatives that are also able to avoid the buffered write deadlock problems efficiently (which prepare_write is unable to do).

[[email protected]: API design contributions, code review and fixes]

Signed-off-by Nick Piggin <[email protected]>

Signed-off-by Mark Fasheh <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

	/* TESTCASE BEGIN */
	#include <stdio.h>
	#include <sys/types.h>
	#include <sys/stat.h>
	#include <fcntl.h>
	#include <sys/uio.h>
	#include <sys/mman.h>
	#define SIZE  (4096 * 2)
	int main(int argc, char* argv[])
	{	
		char* ptr[4];
		struct iovec iov[2];
		int fd, ret;
		ptr[0] = mmap(NULL, SIZE, PROT_READ|PROT_WRITE,
			MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
		ptr[1] = mmap(NULL, SIZE, PROT_NONE,
			MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
		ptr[2] = mmap(NULL, SIZE, PROT_READ|PROT_WRITE,
			MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
		ptr[3] = mmap(NULL, SIZE, PROT_NONE, 
			MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);

		iov[0].iov_base = ptr[0] + (SIZE -1);
		iov[0].iov_len = 1;
		memset(ptr[0], 1, SIZE);

		iov[1].iov_base = ptr[2];
		iov[1].iov_len = SIZE;
		memset(ptr[2], 2, SIZE);

		fd = open(argv[1], O_CREAT|O_RDWR|O_TRUNC, 0666);
		ret = writev(fd, iov, sizeof(iov) / sizeof(struct iovec));
		return 0;
	}	
	/* TESTCASE END*/
	We will get folowing result:
		writev(3, [{"\1", 1}, {"\2"..., 8192}], 2) = -1 EFAULT (Bad address)
 	
	this is hidden bug, and it was invisiable because XXXX_fault_in_readable

(quoted diff) +0 -0

 	return value was ignored before. Lets iov_iter_fault_in_readable
 	perform checks for all segments.

Signed-off-by Dmitriy Monakhov <[email protected]>

include/linux/fs.h +1 -1

index fef19fc..7e025ea 100644
@@ -433,7 +433,7 @@ size_t iov_iter_copy_from_user_atomic(struct page *page,
 size_t iov_iter_copy_from_user(struct page *page,
 		struct iov_iter *i, unsigned long offset, size_t bytes);
 void iov_iter_advance(struct iov_iter *i, size_t bytes);
-int iov_iter_fault_in_readable(struct iov_iter *i, size_t bytes);
+int iov_iter_fault_in_readable(struct iov_iter *i, size_t *bytes);
 size_t iov_iter_single_seg_count(struct iov_iter *i);
 
 static inline void iov_iter_init(struct iov_iter *i,

mm/filemap.c +27 -6

index 8d59ed9..8600c3e 100644
@@ -1817,10 +1817,32 @@ void iov_iter_advance(struct iov_iter *i, size_t bytes)
 }
 EXPORT_SYMBOL(iov_iter_advance);
 
-int iov_iter_fault_in_readable(struct iov_iter *i, size_t bytes)
+int iov_iter_fault_in_readable(struct iov_iter *i, size_t* bytes)
 {
-	char __user *buf = i->iov->iov_base + i->iov_offset;
-	return fault_in_pages_readable(buf, bytes);
+	size_t len = *bytes;
+	int ret;
+	if (likely(i->nr_segs == 1)) {
+		ret = fault_in_pages_readable(i->iov->iov_base, len);
+		if (ret)
+			*bytes = 0;
+	} else {
+		const struct iovec *iov = i->iov;
+		size_t base = i->iov_offset;
+		*bytes = 0;
+		while (len) {
+			int copy = min(len, iov->iov_len - base);
+			if ((ret = fault_in_pages_readable(iov->iov_base + base, copy)))
+				break;
+			*bytes += copy;
+			len -= copy;
+			base += copy;
+			if (iov->iov_len == base) {
+				iov++;
+				base = 0;
+			}
+		}
+	}
+	return ret;	
 }
 EXPORT_SYMBOL(iov_iter_fault_in_readable);
 

@@ -2110,7 +2132,7 @@ static ssize_t generic_perform_write_2copy(struct file *file,
 		 * to check that the address is actually valid, when atomic
 		 * usercopies are used, below.
 		 */
-		if (unlikely(iov_iter_fault_in_readable(i, bytes))) {
+		if (unlikely(iov_iter_fault_in_readable(i, &bytes) && !bytes)) {
 			status = -EFAULT;
 			break;
 		}

@@ -2284,7 +2306,7 @@ again:
 		 * to check that the address is actually valid, when atomic
 		 * usercopies are used, below.
 		 */
-		if (unlikely(iov_iter_fault_in_readable(i, bytes))) {
+		if (unlikely(iov_iter_fault_in_readable(i, &bytes)&& !bytes)) {
 			status = -EFAULT;
 			break;
 		}
 
 
-

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/

Review

Quoted 2 lines · Dmitriy Monakhov

(quoted diff) +0 -0

 	Function prerform check for signgle region, with out respect to
 	segment nature of iovec, For example writev no longer works :)

Btw, could someone please start to collect all sniplets like this in a nice simple regression test suite? If no one wants to start a new one we should probably just put it into xfsqa (which should be useable for other filesystems aswell despite the name)

Yeah, it can run a subset of the tests on NFS and UDF filesystems as well and there are some specific UDF-only tests in it too. I think the NFS test group is mostly generic tests that don't use or test specific XFS features.

Review

Quoted 2 lines · Dmitriy Monakhov

(quoted diff) +0 -0

 	Function prerform check for signgle region, with out respect to
 	segment nature of iovec, For example writev no longer works :)

Btw, could someone please start to collect all sniplets like this in a nice simple regression test suite? If no one wants to start a new one we should probably just put it into xfsqa (which should be useable for other filesystems aswell despite the name)

Quoted 19 lines · On 14:19 ?????? 29 ?????? , [email protected] wrote:

The patch titled
fs: introduce write_begin, write_end, and perform_write aops
has been added to the -mm tree. Its filename is
fs-introduce-write_begin-write_end-and-perform_write-aops.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find out what to do about this

------------------------------------------------------
Subject: fs: introduce write_begin, write_end, and perform_write aops
From: Nick Piggin <[email protected]>

These are intended to replace prepare_write and commit_write with more flexible alternatives that are also able to avoid the buffered write deadlock problems efficiently (which prepare_write is unable to do).

Quoted 4 lines

[[email protected]: API design contributions, code review and fixes]

Signed-off-by Nick Piggin <[email protected]>

Signed-off-by Mark Fasheh <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

I've finaly find time to review Nick's "write_begin/write_end aop" patch set. And i have some fixes and questions. My be i've missed somthing and it was already disscussed, but i cant find in LKML.

1) loop dev:
loop.c code itself is not perfect. In fact before Nick's patch partial write was't possible. Assumption what write chunks are always page aligned is realy weird ( see "index++" line). Fixed by "new aop loop fix" patch

2)block_write_begin:

	After we enter to block_write_begin with *pagep == NULL and
	some page was grabed we remember this page in *pagep
	And if __block_prepare_write() we have to clear *pagep , as 
	it was before. Because this may confuse caller.
	for example caller may have folowing code:
		ret = block_write_begin(..., pagep,...)
		if (ret && *pagep != NULL) {
			unlock_page(*pagep);
			page_cache_release(*pagep);
		}
	Fixed my "new aop block_write_begin fix" patch

3) __page_symlink:
Nick's patch add folowing code:
+ err = pagecache_write_begin(NULL, mapping, 0,PAGE_CACHE_SIZE,
+ AOP_FLAG_UNINTERRUPTIBLE, &page,&fsdata);
symlink now consume whole page. I have only one question "WHY???". I don't see any advantages, but where are huge list of dissdvantages:
a) fs with blksize == 1k and pagesize == 16k after this patch waste more than 10x times disk space for nothing.
b) What happends if we want use fs with blksize == 4k on i386 after it was used by ia64 ??? (before this patch it was possible).

I dont prepare patch for this because i dont understand issue witch Nick aimed to fix.

4) iov_iter_fault_in_readable:
Function prerform check for signgle region, with out respect to segment nature of iovec, For example writev no longer works :) :
writev(3, [{"\1", 1}, {"\2"..., 4096}], 2) = -1 EFAULT (Bad address) this hidden bug, and it was invisiable because XXXX_fault_in_readable return value was ignored before. Lets iov_iter_fault_in_readable perform checks for all segments.
Fixed by :"iov_iter_fault_in_readable fix"

5) ext3_write_end:

	Before  write_begin/write_end patch set we have folowing locking
	order:
		stop_journal(handle);
		unlock_page(page);
	But now order is oposite:
		unlock_page(page);
		stop_journal(handle);
	Can we got any race condition now? I'm not sure is it actual problem,
	may be somebody cant describe this.

Quoted 20 lines · Dmitriy Monakhov

Quoted 19 lines · On 14:19 ?????? 29 ?????? , [email protected] wrote:

The patch titled
fs: introduce write_begin, write_end, and perform_write aops
has been added to the -mm tree. Its filename is
fs-introduce-write_begin-write_end-and-perform_write-aops.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find out what to do about this

------------------------------------------------------
Subject: fs: introduce write_begin, write_end, and perform_write aops
From: Nick Piggin <[email protected]>

These are intended to replace prepare_writ eand commit_write with more flexible alternatives that are also able to avoid the buffered write deadlock problems efficiently (which prepare_write is unable to do).

Quoted 7 lines

Quoted 4 lines

[[email protected]: API design contributions, code review and fixes]

Signed-off-by Nick Piggin <[email protected]>

Signed-off-by Mark Fasheh <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

I've finaly find time to review Nick's "write_begin/write_end aop" patch set. And i have some fixes and questions. My be i've missed somthing and it was already disscussed, but i cant find in LKML.

Thanks, that's really helpful.

Quoted 5 lines

1) loop dev:
loop.c code itself is not perfect. In fact before Nick's patch partial write was't possible. Assumption what write chunks are always page aligned is realy weird ( see "index++" line). Fixed by "new aop loop fix" patch

I think you're right, fix looks good.

Quoted 12 lines

2)block_write_begin:

	After we enter to block_write_begin with *pagep == NULL and
	some page was grabed we remember this page in *pagep
	And if __block_prepare_write() we have to clear *pagep , as 
	it was before. Because this may confuse caller.
	for example caller may have folowing code:
		ret = block_write_begin(..., pagep,...)
		if (ret && *pagep != NULL) {
			unlock_page(*pagep);
			page_cache_release(*pagep);
		}
	Fixed my "new aop block_write_begin fix" patch

I don't think the caller can rely on that if it returns failure. But that is more defensive I guess. Maybe setting it to 1 or so would catch abusers.

Quoted 12 lines

3) __page_symlink:
Nick's patch add folowing code:
+ err = pagecache_write_begin(NULL, mapping, 0,PAGE_CACHE_SIZE,
+ AOP_FLAG_UNINTERRUPTIBLE, &page,&fsdata);
symlink now consume whole page. I have only one question "WHY???". I don't see any advantages, but where are huge list of dissdvantages:
a) fs with blksize == 1k and pagesize == 16k after this patch waste more than 10x times disk space for nothing.
b) What happends if we want use fs with blksize == 4k on i386 after it was used by ia64 ??? (before this patch it was possible).

Quoted 3 lines

I dont prepare patch for this because i dont understand issue witch Nick aimed to fix.

I don't know why myself :P I think it would be just fine to use len-1 as it did previously, so it must have been a typo?

Quoted 8 lines

4) iov_iter_fault_in_readable:
Function prerform check for signgle region, with out respect to segment nature of iovec, For example writev no longer works :) :
writev(3, [{"\1", 1}, {"\2"..., 4096}], 2) = -1 EFAULT (Bad address) this hidden bug, and it was invisiable because XXXX_fault_in_readable return value was ignored before. Lets iov_iter_fault_in_readable perform checks for all segments.
Fixed by :"iov_iter_fault_in_readable fix"

OK thanks. I would rather just change this to use the length of the first iovec always (prefaulting multiple iovecs would have to be benchmarked on real apps that make heavy use of writev, I suspect).

Quoted 10 lines

5) ext3_write_end:

	Before  write_begin/write_end patch set we have folowing locking
	order:
		stop_journal(handle);
		unlock_page(page);
	But now order is oposite:
		unlock_page(page);
		stop_journal(handle);
	Can we got any race condition now? I'm not sure is it actual problem,
	may be somebody cant describe this.

Can we just change it to the original order? That would seem to be safest unless one of the ext3 devs explicitly acks it.
-
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 11 lines

5) ext3_write_end:

	Before  write_begin/write_end patch set we have folowing locking
	order:
		stop_journal(handle);
		unlock_page(page);
	But now order is oposite:
		unlock_page(page);
		stop_journal(handle);
	Can we got any race condition now? I'm not sure is it actual problem,
	may be somebody cant describe this.

Can we just change it to the original order? That would seem to be safest unless one of the ext3 devs explicitly acks it.

On Wed, 2007-06-13 at 13:43 +0200, Nick Piggin wrote:
..

Quoted 12 lines

Quoted 11 lines

5) ext3_write_end:

	Before  write_begin/write_end patch set we have folowing locking
	order:
		stop_journal(handle);
		unlock_page(page);
	But now order is oposite:
		unlock_page(page);
		stop_journal(handle);
	Can we got any race condition now? I'm not sure is it actual problem,
	may be somebody cant describe this.

Quoted 2 lines

Can we just change it to the original order? That would seem to be safest unless one of the ext3 devs explicitly acks it.

It would be nice to go back to original order, but its not that simple with current structure of the code. With Nick's patches unlock_page() happens in generic_write_end(). journal_stop() needs to happen after generic_write_end(). :(

Mingming, can you take a look at the current & proposed order ? I ran into bunch of races when I tried to change the order for ->writepages() support earlier :(

..

Quoted 12 lines

Quoted 11 lines

5) ext3_write_end:

	Before  write_begin/write_end patch set we have folowing locking
	order:
		stop_journal(handle);
		unlock_page(page);
	But now order is oposite:
		unlock_page(page);
		stop_journal(handle);
	Can we got any race condition now? I'm not sure is it actual problem,
	may be somebody cant describe this.

Quoted 2 lines

Can we just change it to the original order? That would seem to be safest unless one of the ext3 devs explicitly acks it.

Quoted 14 lines · Nick Piggin

..

Quoted 12 lines

Quoted 11 lines

5) ext3_write_end:

	Before  write_begin/write_end patch set we have folowing locking
	order:
		stop_journal(handle);
		unlock_page(page);
	But now order is oposite:
		unlock_page(page);
		stop_journal(handle);
	Can we got any race condition now? I'm not sure is it actual problem,
	may be somebody cant describe this.

Quoted 2 lines

Quoted 2 lines

Can we just change it to the original order? That would seem to be safest unless one of the ext3 devs explicitly acks it.

Sorry, I've missed beginning of this thread. But what problems can
exactly cause this ordering change? ext3_journal_stop has no need to be protected by the page lock - it can be even better that it's not protected as it can trigger commit and all that would happen unnecessarily under page lock...