[PATCH] not empty pages list after fuse_readpages

[PATCH] not empty pages list after fuse_readpages

[Alexander Zarochentsev]

Hello,

fuse_readpages looks buggy for me because it doesn't care 
about making the @pages list empty at exit.

If it is indeed a bug, I suggest the following patch:

don't let fuse_readpages leave the @pages list not empty
when exiting on error.

Signed-off-by: Alexander Zarochentsev <zam@namesys.com>
---
 fs/fuse/file.c          |   15 +++++++++++----
 include/linux/pagemap.h |    1 +
 mm/readahead.c          |   24 +++++++++++++++++-------
 3 files changed, 29 insertions(+), 11 deletions(-)

--- linux-2.6-git.orig/fs/fuse/file.c
+++ linux-2.6-git/fs/fuse/file.c
@@ -395,14 +395,18 @@ static int fuse_readpages(struct file *f
 	struct fuse_readpages_data data;
 	int err;
 
-	if (is_bad_inode(inode))
-		return -EIO;
+	if (is_bad_inode(inode)) {
+		err = -EIO;
+		goto clean_pages_up;
+	}
 
 	data.file = file;
 	data.inode = inode;
 	data.req = fuse_get_req(fc);
-	if (IS_ERR(data.req))
-		return PTR_ERR(data.req);
+	if (IS_ERR(data.req)) {
+		err = PTR_ERR(data.req);
+		goto clean_pages_up;
+	}
 
 	err = read_cache_pages(mapping, pages, fuse_readpages_fill, &data);
 	if (!err) {
@@ -411,6 +415,10 @@ static int fuse_readpages(struct file *f
 		else
 			fuse_put_request(fc, data.req);
 	}
+	if (0) {
+clean_pages_up:
+		readpages_cleanup_helper(pages);
+	}
 	return err;
 }
 
--- linux-2.6-git.orig/include/linux/pagemap.h
+++ linux-2.6-git/include/linux/pagemap.h
@@ -100,6 +100,7 @@ extern struct page * read_cache_page(str
 				void *data);
 extern int read_cache_pages(struct address_space *mapping,
 		struct list_head *pages, filler_t *filler, void *data);
+extern void readpages_cleanup_helper(struct list_head *);
 
 static inline struct page *read_mapping_page(struct address_space *mapping,
 					     unsigned long index, void *data)
--- linux-2.6-git.orig/mm/readahead.c
+++ linux-2.6-git/mm/readahead.c
@@ -229,6 +229,22 @@ static inline unsigned long get_next_ra_
 #define list_to_page(head) (list_entry((head)->prev, struct page, lru))
 
 /**
+ * may be useful in foo_fs_readpages method for the page pool cleanup
+ * before exiting on error.
+ */
+void readpages_cleanup_helper(struct list_head *pages)
+{
+	while (!list_empty(pages)) {
+		struct page *victim;
+
+		victim = list_to_page(pages);
+		list_del(&victim->lru);
+		page_cache_release(victim);
+	}
+}
+EXPORT_SYMBOL(readpages_cleanup_helper);
+
+/**
  * read_cache_pages - populate an address space with some pages & start reads against them
  * @mapping: the address_space
  * @pages: The address of a list_head which contains the target pages.  These
@@ -260,13 +276,7 @@ int read_cache_pages(struct address_spac
 			__pagevec_lru_add(&lru_pvec);
 		}
 		if (ret) {
-			while (!list_empty(pages)) {
-				struct page *victim;
-
-				victim = list_to_page(pages);
-				list_del(&victim->lru);
-				page_cache_release(victim);
-			}
+			readpages_cleanup_helper(pages);
 			break;
 		}
 	}

Re: [PATCH] not empty pages list after fuse_readpages

[Dave Hansen]

On Thu, 2006-08-10 at 00:20 +0400, Alexander Zarochentsev wrote:
> 
>         }
> +       if (0) {
> +clean_pages_up:
> +               readpages_cleanup_helper(pages);
> +       }
>         return err;
>  }

If the list is really empty during a normal exit, does it hurt to call
the helper?  The whole goto inside of an if(0) statement looks a little
funky.

The following would be the same number of lines of code, and this is
used pretty commonly in the kernel:

	return err;
clean_pages_up:
	readpages_cleanup_helper(pages);
	return err;

But, I really wonder what is wrong with this:

clean_pages_up:
	readpages_cleanup_helper(pages);
	return err;

-- Dave

Re: [PATCH] not empty pages list after fuse_readpages

[Alexander Zarochentsev]

On 10 August 2006 01:02, Dave Hansen wrote:
> On Thu, 2006-08-10 at 00:20 +0400, Alexander Zarochentsev wrote:
> >         }
> > +       if (0) {
> > +clean_pages_up:
> > +               readpages_cleanup_helper(pages);
> > +       }
> >         return err;
> >  }
>
> If the list is really empty during a normal exit,

normal exit is done after read_cache_pages() 
which always cleans the list.  If read_cache_pages is not called, 
the list is not empty and
__do_page_cache_readahead: BUG_ON(!list_empty(&page_pool)) 
will be triggered.

> does it hurt to 
> call the helper?  The whole goto inside of an if(0) statement looks a
> little funky.

yes, a bit.

but really anybody has problems understanding it?


> The following would be the same number of lines of code, and this is
> used pretty commonly in the kernel:
>
> 	return err;
> clean_pages_up:
> 	readpages_cleanup_helper(pages);
> 	return err;
>
> But, I really wonder what is wrong with this:
>
> clean_pages_up:
> 	readpages_cleanup_helper(pages);
> 	return err;

unneeded function call?

well, the same patch without if(0):

don't let fuse_readpages leave the @pages list not empty
when exiting on error.

Signed-off-by: Alexander Zarochentsev <zam@namesys.com>
---
 fs/fuse/file.c          |   15 +++++++++++----
 include/linux/pagemap.h |    1 +
 mm/readahead.c          |   24 +++++++++++++++++-------
 3 files changed, 29 insertions(+), 11 deletions(-)

--- linux-2.6-git.orig/fs/fuse/file.c
+++ linux-2.6-git/fs/fuse/file.c
@@ -395,14 +395,18 @@ static int fuse_readpages(struct file *f
 	struct fuse_readpages_data data;
 	int err;
 
-	if (is_bad_inode(inode))
-		return -EIO;
+	if (is_bad_inode(inode)) {
+		err = -EIO;
+		goto clean_pages_up;
+	}
 
 	data.file = file;
 	data.inode = inode;
 	data.req = fuse_get_req(fc);
-	if (IS_ERR(data.req))
-		return PTR_ERR(data.req);
+	if (IS_ERR(data.req)) {
+		err = PTR_ERR(data.req);
+		goto clean_pages_up;
+	}
 
 	err = read_cache_pages(mapping, pages, fuse_readpages_fill, &data);
 	if (!err) {
@@ -412,6 +416,9 @@ static int fuse_readpages(struct file *f
 			fuse_put_request(fc, data.req);
 	}
 	return err;
+clean_pages_up:
+	readpages_cleanup_helper(pages);
+	return err;
 }
 
 static size_t fuse_send_write(struct fuse_req *req, struct file *file,
--- linux-2.6-git.orig/include/linux/pagemap.h
+++ linux-2.6-git/include/linux/pagemap.h
@@ -100,6 +100,7 @@ extern struct page * read_cache_page(str
 				void *data);
 extern int read_cache_pages(struct address_space *mapping,
 		struct list_head *pages, filler_t *filler, void *data);
+extern void readpages_cleanup_helper(struct list_head *);
 
 static inline struct page *read_mapping_page(struct address_space *mapping,
 					     unsigned long index, void *data)
--- linux-2.6-git.orig/mm/readahead.c
+++ linux-2.6-git/mm/readahead.c
@@ -229,6 +229,22 @@ static inline unsigned long get_next_ra_
 #define list_to_page(head) (list_entry((head)->prev, struct page, lru))
 
 /**
+ * may be useful in foo_fs_readpages method for the page pool cleanup
+ * before exiting on error.
+ */
+void readpages_cleanup_helper(struct list_head *pages)
+{
+	while (!list_empty(pages)) {
+		struct page *victim;
+
+		victim = list_to_page(pages);
+		list_del(&victim->lru);
+		page_cache_release(victim);
+	}
+}
+EXPORT_SYMBOL(readpages_cleanup_helper);
+
+/**
  * read_cache_pages - populate an address space with some pages & start reads against them
  * @mapping: the address_space
  * @pages: The address of a list_head which contains the target pages.  These
@@ -260,13 +276,7 @@ int read_cache_pages(struct address_spac
 			__pagevec_lru_add(&lru_pvec);
 		}
 		if (ret) {
-			while (!list_empty(pages)) {
-				struct page *victim;
-
-				victim = list_to_page(pages);
-				list_del(&victim->lru);
-				page_cache_release(victim);
-			}
+			readpages_cleanup_helper(pages);
 			break;
 		}
 	}