Messages in this thread Patch in this message | | | From | Alexander Zarochentsev <> | Subject | Re: [PATCH] not empty pages list after fuse_readpages | Date | Thu, 10 Aug 2006 07:54:35 +0400 |
| |
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; } } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |