[patch 1/2] fuse: delete dead .write_begin and .write_end aops

[patch 1/2] fuse: delete dead .write_begin and .write_end aops

[Johannes Weiner]

Ever since 'ea9b990 fuse: implement perform_write', the .write_begin
and .write_end aops have been dead code.

Their task - acquiring a page from the page cache, sending out a write
request and releasing the page again - is now done batch-wise to
maximize the number of pages send per userspace request.

Signed-off-by: Johannes Weiner <jweiner@redhat.com>
---
 fs/fuse/file.c |   70 --------------------------------------------------------
 1 files changed, 0 insertions(+), 70 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 82a6646..5c48126 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -743,18 +743,6 @@ static size_t fuse_send_write(struct fuse_req *req, struct file *file,
 	return req->misc.write.out.size;
 }
 
-static int fuse_write_begin(struct file *file, struct address_space *mapping,
-			loff_t pos, unsigned len, unsigned flags,
-			struct page **pagep, void **fsdata)
-{
-	pgoff_t index = pos >> PAGE_CACHE_SHIFT;
-
-	*pagep = grab_cache_page_write_begin(mapping, index, flags);
-	if (!*pagep)
-		return -ENOMEM;
-	return 0;
-}
-
 void fuse_write_update_size(struct inode *inode, loff_t pos)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
@@ -767,62 +755,6 @@ void fuse_write_update_size(struct inode *inode, loff_t pos)
 	spin_unlock(&fc->lock);
 }
 
-static int fuse_buffered_write(struct file *file, struct inode *inode,
-			       loff_t pos, unsigned count, struct page *page)
-{
-	int err;
-	size_t nres;
-	struct fuse_conn *fc = get_fuse_conn(inode);
-	unsigned offset = pos & (PAGE_CACHE_SIZE - 1);
-	struct fuse_req *req;
-
-	if (is_bad_inode(inode))
-		return -EIO;
-
-	/*
-	 * Make sure writepages on the same page are not mixed up with
-	 * plain writes.
-	 */
-	fuse_wait_on_page_writeback(inode, page->index);
-
-	req = fuse_get_req(fc);
-	if (IS_ERR(req))
-		return PTR_ERR(req);
-
-	req->in.argpages = 1;
-	req->num_pages = 1;
-	req->pages[0] = page;
-	req->page_offset = offset;
-	nres = fuse_send_write(req, file, pos, count, NULL);
-	err = req->out.h.error;
-	fuse_put_request(fc, req);
-	if (!err && !nres)
-		err = -EIO;
-	if (!err) {
-		pos += nres;
-		fuse_write_update_size(inode, pos);
-		if (count == PAGE_CACHE_SIZE)
-			SetPageUptodate(page);
-	}
-	fuse_invalidate_attr(inode);
-	return err ? err : nres;
-}
-
-static int fuse_write_end(struct file *file, struct address_space *mapping,
-			loff_t pos, unsigned len, unsigned copied,
-			struct page *page, void *fsdata)
-{
-	struct inode *inode = mapping->host;
-	int res = 0;
-
-	if (copied)
-		res = fuse_buffered_write(file, inode, pos, copied, page);
-
-	unlock_page(page);
-	page_cache_release(page);
-	return res;
-}
-
 static size_t fuse_send_write_pages(struct fuse_req *req, struct file *file,
 				    struct inode *inode, loff_t pos,
 				    size_t count)
@@ -2172,8 +2104,6 @@ static const struct address_space_operations fuse_file_aops  = {
 	.readpage	= fuse_readpage,
 	.writepage	= fuse_writepage,
 	.launder_page	= fuse_launder_page,
-	.write_begin	= fuse_write_begin,
-	.write_end	= fuse_write_end,
 	.readpages	= fuse_readpages,
 	.set_page_dirty	= __set_page_dirty_nobuffers,
 	.bmap		= fuse_bmap,
-- 
1.7.6

[patch 1/2] fuse: delete dead .write_begin and .write_end aops

[Johannes Weiner]

Ever since 'ea9b990 fuse: implement perform_write', the .write_begin
and .write_end aops have been dead code.

Their task - acquiring a page from the page cache, sending out a write
request and releasing the page again - is now done batch-wise to
maximize the number of pages send per userspace request.

Signed-off-by: Johannes Weiner <jweiner@redhat.com>
---
 fs/fuse/file.c |   70 --------------------------------------------------------
 1 files changed, 0 insertions(+), 70 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 82a6646..5c48126 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -743,18 +743,6 @@ static size_t fuse_send_write(struct fuse_req *req, struct file *file,
 	return req->misc.write.out.size;
 }
 
-static int fuse_write_begin(struct file *file, struct address_space *mapping,
-			loff_t pos, unsigned len, unsigned flags,
-			struct page **pagep, void **fsdata)
-{
-	pgoff_t index = pos >> PAGE_CACHE_SHIFT;
-
-	*pagep = grab_cache_page_write_begin(mapping, index, flags);
-	if (!*pagep)
-		return -ENOMEM;
-	return 0;
-}
-
 void fuse_write_update_size(struct inode *inode, loff_t pos)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
@@ -767,62 +755,6 @@ void fuse_write_update_size(struct inode *inode, loff_t pos)
 	spin_unlock(&fc->lock);
 }
 
-static int fuse_buffered_write(struct file *file, struct inode *inode,
-			       loff_t pos, unsigned count, struct page *page)
-{
-	int err;
-	size_t nres;
-	struct fuse_conn *fc = get_fuse_conn(inode);
-	unsigned offset = pos & (PAGE_CACHE_SIZE - 1);
-	struct fuse_req *req;
-
-	if (is_bad_inode(inode))
-		return -EIO;
-
-	/*
-	 * Make sure writepages on the same page are not mixed up with
-	 * plain writes.
-	 */
-	fuse_wait_on_page_writeback(inode, page->index);
-
-	req = fuse_get_req(fc);
-	if (IS_ERR(req))
-		return PTR_ERR(req);
-
-	req->in.argpages = 1;
-	req->num_pages = 1;
-	req->pages[0] = page;
-	req->page_offset = offset;
-	nres = fuse_send_write(req, file, pos, count, NULL);
-	err = req->out.h.error;
-	fuse_put_request(fc, req);
-	if (!err && !nres)
-		err = -EIO;
-	if (!err) {
-		pos += nres;
-		fuse_write_update_size(inode, pos);
-		if (count == PAGE_CACHE_SIZE)
-			SetPageUptodate(page);
-	}
-	fuse_invalidate_attr(inode);
-	return err ? err : nres;
-}
-
-static int fuse_write_end(struct file *file, struct address_space *mapping,
-			loff_t pos, unsigned len, unsigned copied,
-			struct page *page, void *fsdata)
-{
-	struct inode *inode = mapping->host;
-	int res = 0;
-
-	if (copied)
-		res = fuse_buffered_write(file, inode, pos, copied, page);
-
-	unlock_page(page);
-	page_cache_release(page);
-	return res;
-}
-
 static size_t fuse_send_write_pages(struct fuse_req *req, struct file *file,
 				    struct inode *inode, loff_t pos,
 				    size_t count)
@@ -2172,8 +2104,6 @@ static const struct address_space_operations fuse_file_aops  = {
 	.readpage	= fuse_readpage,
 	.writepage	= fuse_writepage,
 	.launder_page	= fuse_launder_page,
-	.write_begin	= fuse_write_begin,
-	.write_end	= fuse_write_end,
 	.readpages	= fuse_readpages,
 	.set_page_dirty	= __set_page_dirty_nobuffers,
 	.bmap		= fuse_bmap,
-- 
1.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[patch 2/2] fuse: mark pages accessed when written to

[Johannes Weiner]

As fuse does not use the page cache library functions when userspace
writes to a file, it did not benefit from 'c8236db mm: mark page
accessed before we write_end()' that made sure pages are properly
marked accessed when written to.

Signed-off-by: Johannes Weiner <jweiner@redhat.com>
---
 fs/fuse/file.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 5c48126..471067e 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -14,6 +14,7 @@
 #include <linux/sched.h>
 #include <linux/module.h>
 #include <linux/compat.h>
+#include <linux/swap.h>
 
 static const struct file_operations fuse_direct_io_file_operations;
 
@@ -828,6 +829,8 @@ static ssize_t fuse_fill_write_pages(struct fuse_req *req,
 		pagefault_enable();
 		flush_dcache_page(page);
 
+		mark_page_accessed(page);
+
 		if (!tmp) {
 			unlock_page(page);
 			page_cache_release(page);
-- 
1.7.6

[patch 2/2] fuse: mark pages accessed when written to

[Johannes Weiner]

As fuse does not use the page cache library functions when userspace
writes to a file, it did not benefit from 'c8236db mm: mark page
accessed before we write_end()' that made sure pages are properly
marked accessed when written to.

Signed-off-by: Johannes Weiner <jweiner@redhat.com>
---
 fs/fuse/file.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 5c48126..471067e 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -14,6 +14,7 @@
 #include <linux/sched.h>
 #include <linux/module.h>
 #include <linux/compat.h>
+#include <linux/swap.h>
 
 static const struct file_operations fuse_direct_io_file_operations;
 
@@ -828,6 +829,8 @@ static ssize_t fuse_fill_write_pages(struct fuse_req *req,
 		pagefault_enable();
 		flush_dcache_page(page);
 
+		mark_page_accessed(page);
+
 		if (!tmp) {
 			unlock_page(page);
 			page_cache_release(page);
-- 
1.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/2] fuse: delete dead .write_begin and .write_end aops

[Christoph Hellwig]

On Mon, Jul 25, 2011 at 10:35:34PM +0200, Johannes Weiner wrote:
> Ever since 'ea9b990 fuse: implement perform_write', the .write_begin
> and .write_end aops have been dead code.
> 
> Their task - acquiring a page from the page cache, sending out a write
> request and releasing the page again - is now done batch-wise to
> maximize the number of pages send per userspace request.

The loop code still calls them uncondtionally.  This actually is a big
as write_begin and write_end require filesystems specific locking,
and might require code in the filesystem to e.g. update the ctime
properly.  I'll let Miklos chime in if leaving them in was intentional,
and if it was a comment is probably justified.

Re: [patch 1/2] fuse: delete dead .write_begin and .write_end aops

[Christoph Hellwig]

On Mon, Jul 25, 2011 at 10:35:34PM +0200, Johannes Weiner wrote:
> Ever since 'ea9b990 fuse: implement perform_write', the .write_begin
> and .write_end aops have been dead code.
> 
> Their task - acquiring a page from the page cache, sending out a write
> request and releasing the page again - is now done batch-wise to
> maximize the number of pages send per userspace request.

The loop code still calls them uncondtionally.  This actually is a big
as write_begin and write_end require filesystems specific locking,
and might require code in the filesystem to e.g. update the ctime
properly.  I'll let Miklos chime in if leaving them in was intentional,
and if it was a comment is probably justified.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/2] fuse: delete dead .write_begin and .write_end aops

[Miklos Szeredi]

Christoph Hellwig <hch@infradead.org> writes:

> On Mon, Jul 25, 2011 at 10:35:34PM +0200, Johannes Weiner wrote:
>> Ever since 'ea9b990 fuse: implement perform_write', the .write_begin
>> and .write_end aops have been dead code.
>> 
>> Their task - acquiring a page from the page cache, sending out a write
>> request and releasing the page again - is now done batch-wise to
>> maximize the number of pages send per userspace request.
>
> The loop code still calls them uncondtionally.  This actually is a big
> as write_begin and write_end require filesystems specific locking,
> and might require code in the filesystem to e.g. update the ctime
> properly.  I'll let Miklos chime in if leaving them in was intentional,
> and if it was a comment is probably justified.

Loop checks for ->write_begin() and falls back to ->write if the former
isn't defined.

So I think the patch is fine.  I tested loop over fuse, and it still
works after the patch.

Added to for-next.

Thanks,
Miklos

Re: [patch 1/2] fuse: delete dead .write_begin and .write_end aops

[Miklos Szeredi]

Christoph Hellwig <hch@infradead.org> writes:

> On Mon, Jul 25, 2011 at 10:35:34PM +0200, Johannes Weiner wrote:
>> Ever since 'ea9b990 fuse: implement perform_write', the .write_begin
>> and .write_end aops have been dead code.
>> 
>> Their task - acquiring a page from the page cache, sending out a write
>> request and releasing the page again - is now done batch-wise to
>> maximize the number of pages send per userspace request.
>
> The loop code still calls them uncondtionally.  This actually is a big
> as write_begin and write_end require filesystems specific locking,
> and might require code in the filesystem to e.g. update the ctime
> properly.  I'll let Miklos chime in if leaving them in was intentional,
> and if it was a comment is probably justified.

Loop checks for ->write_begin() and falls back to ->write if the former
isn't defined.

So I think the patch is fine.  I tested loop over fuse, and it still
works after the patch.

Added to for-next.

Thanks,
Miklos

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 2/2] fuse: mark pages accessed when written to

[Miklos Szeredi]

Johannes Weiner <jweiner@redhat.com> writes:

> As fuse does not use the page cache library functions when userspace
> writes to a file, it did not benefit from 'c8236db mm: mark page
> accessed before we write_end()' that made sure pages are properly
> marked accessed when written to.
>
> Signed-off-by: Johannes Weiner <jweiner@redhat.com>

Thanks, applied.

Miklos

> ---
>  fs/fuse/file.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 5c48126..471067e 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -14,6 +14,7 @@
>  #include <linux/sched.h>
>  #include <linux/module.h>
>  #include <linux/compat.h>
> +#include <linux/swap.h>
>  
>  static const struct file_operations fuse_direct_io_file_operations;
>  
> @@ -828,6 +829,8 @@ static ssize_t fuse_fill_write_pages(struct fuse_req *req,
>  		pagefault_enable();
>  		flush_dcache_page(page);
>  
> +		mark_page_accessed(page);
> +
>  		if (!tmp) {
>  			unlock_page(page);
>  			page_cache_release(page);

Re: [patch 2/2] fuse: mark pages accessed when written to

[Miklos Szeredi]

Johannes Weiner <jweiner@redhat.com> writes:

> As fuse does not use the page cache library functions when userspace
> writes to a file, it did not benefit from 'c8236db mm: mark page
> accessed before we write_end()' that made sure pages are properly
> marked accessed when written to.
>
> Signed-off-by: Johannes Weiner <jweiner@redhat.com>

Thanks, applied.

Miklos

> ---
>  fs/fuse/file.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 5c48126..471067e 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -14,6 +14,7 @@
>  #include <linux/sched.h>
>  #include <linux/module.h>
>  #include <linux/compat.h>
> +#include <linux/swap.h>
>  
>  static const struct file_operations fuse_direct_io_file_operations;
>  
> @@ -828,6 +829,8 @@ static ssize_t fuse_fill_write_pages(struct fuse_req *req,
>  		pagefault_enable();
>  		flush_dcache_page(page);
>  
> +		mark_page_accessed(page);
> +
>  		if (!tmp) {
>  			unlock_page(page);
>  			page_cache_release(page);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/2] fuse: delete dead .write_begin and .write_end aops

[Christoph Hellwig]

On Mon, Aug 08, 2011 at 05:05:20PM +0200, Miklos Szeredi wrote:
> > The loop code still calls them uncondtionally.  This actually is a big
> > as write_begin and write_end require filesystems specific locking,
> > and might require code in the filesystem to e.g. update the ctime
> > properly.  I'll let Miklos chime in if leaving them in was intentional,
> > and if it was a comment is probably justified.
> 
> Loop checks for ->write_begin() and falls back to ->write if the former
> isn't defined.
> 
> So I think the patch is fine.  I tested loop over fuse, and it still
> works after the patch.

It works, but it involves another data copy, which will slow down
various workloads that people at least historically cared about.

And yes, unconditionally above was wrong - calls them if present without
taking care of filesystem specific locking would be the right term.

Re: [patch 1/2] fuse: delete dead .write_begin and .write_end aops

[Christoph Hellwig]

On Mon, Aug 08, 2011 at 05:05:20PM +0200, Miklos Szeredi wrote:
> > The loop code still calls them uncondtionally.  This actually is a big
> > as write_begin and write_end require filesystems specific locking,
> > and might require code in the filesystem to e.g. update the ctime
> > properly.  I'll let Miklos chime in if leaving them in was intentional,
> > and if it was a comment is probably justified.
> 
> Loop checks for ->write_begin() and falls back to ->write if the former
> isn't defined.
> 
> So I think the patch is fine.  I tested loop over fuse, and it still
> works after the patch.

It works, but it involves another data copy, which will slow down
various workloads that people at least historically cared about.

And yes, unconditionally above was wrong - calls them if present without
taking care of filesystem specific locking would be the right term.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/2] fuse: delete dead .write_begin and .write_end aops

[Miklos Szeredi]

Christoph Hellwig <hch@infradead.org> writes:

> On Mon, Aug 08, 2011 at 05:05:20PM +0200, Miklos Szeredi wrote:
>> > The loop code still calls them uncondtionally.  This actually is a big
>> > as write_begin and write_end require filesystems specific locking,
>> > and might require code in the filesystem to e.g. update the ctime
>> > properly.  I'll let Miklos chime in if leaving them in was intentional,
>> > and if it was a comment is probably justified.
>> 
>> Loop checks for ->write_begin() and falls back to ->write if the former
>> isn't defined.
>> 
>> So I think the patch is fine.  I tested loop over fuse, and it still
>> works after the patch.
>
> It works, but it involves another data copy, which will slow down
> various workloads that people at least historically cared about.

AFAICS, normally there isn't an additional copy.  If ->write_begin is
defined the copy from the bio_vec to the filesystem page is done with
transfer_none() in the loop driver.

Otherwise the copy is done by ->write() itself on the kmapped bio.

If there's a crypto transfer function then a temporary page will be used
in the no write_begin case.  But I don't think there the additional copy
makes much difference or that anyone cares.

Thanks,
Miklos

Re: [patch 1/2] fuse: delete dead .write_begin and .write_end aops

[Miklos Szeredi]

Christoph Hellwig <hch@infradead.org> writes:

> On Mon, Aug 08, 2011 at 05:05:20PM +0200, Miklos Szeredi wrote:
>> > The loop code still calls them uncondtionally.  This actually is a big
>> > as write_begin and write_end require filesystems specific locking,
>> > and might require code in the filesystem to e.g. update the ctime
>> > properly.  I'll let Miklos chime in if leaving them in was intentional,
>> > and if it was a comment is probably justified.
>> 
>> Loop checks for ->write_begin() and falls back to ->write if the former
>> isn't defined.
>> 
>> So I think the patch is fine.  I tested loop over fuse, and it still
>> works after the patch.
>
> It works, but it involves another data copy, which will slow down
> various workloads that people at least historically cared about.

AFAICS, normally there isn't an additional copy.  If ->write_begin is
defined the copy from the bio_vec to the filesystem page is done with
transfer_none() in the loop driver.

Otherwise the copy is done by ->write() itself on the kmapped bio.

If there's a crypto transfer function then a temporary page will be used
in the no write_begin case.  But I don't think there the additional copy
makes much difference or that anyone cares.

Thanks,
Miklos

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>