[RFC] ext4: simplify the code a bit

[RFC] ext4: simplify the code a bit

[Stephen Rothwell]

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

Hi guys,

I noticed the following warning in a linux-next build:

fs/ext4/inode.c: In function 'ext4_da_writepages':
fs/ext4/inode.c:2212:6: warning: 'err' may be used uninitialized in this function [-Wmaybe-uninitialized]

In tracking this down, I followed the call chains and discovered that
io_submit_init_bio() only ever returned 0.  Switching that to return
void and following back up the chain lead to the following patch.  This
makes it far more obvious that by the end of the loop in
mpage_map_and_submit_extent(), err must be zero, which allows the final
removal of err2 and the check that caused the above warning.

This does remove the above warning and simplifies the code a bit, but may
be removing error infrastructure that may be used in the future.

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 fs/ext4/ext4.h    |  8 ++++----
 fs/ext4/inode.c   | 46 +++++++++++++---------------------------------
 fs/ext4/page-io.c | 35 ++++++-----------------------------
 3 files changed, 23 insertions(+), 66 deletions(-)

This is against today's ext4 tree dev branch.

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index bd9890f..1341452 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2660,10 +2660,10 @@ extern void ext4_io_submit_init(struct ext4_io_submit *io,
 extern void ext4_end_io_rsv_work(struct work_struct *work);
 extern void ext4_end_io_unrsv_work(struct work_struct *work);
 extern void ext4_io_submit(struct ext4_io_submit *io);
-extern int ext4_bio_write_page(struct ext4_io_submit *io,
-			       struct page *page,
-			       int len,
-			       struct writeback_control *wbc);
+extern void ext4_bio_write_page(struct ext4_io_submit *io,
+				struct page *page,
+				int len,
+				struct writeback_control *wbc);
 
 /* mmp.c */
 extern int ext4_multi_mount_protect(struct super_block *, ext4_fsblk_t);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 442c5d2..80bc416 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1832,7 +1832,6 @@ out:
 static int ext4_writepage(struct page *page,
 			  struct writeback_control *wbc)
 {
-	int ret = 0;
 	loff_t size;
 	unsigned int len;
 	struct buffer_head *page_bufs = NULL;
@@ -1884,11 +1883,11 @@ static int ext4_writepage(struct page *page,
 		unlock_page(page);
 		return -ENOMEM;
 	}
-	ret = ext4_bio_write_page(&io_submit, page, len, wbc);
+	ext4_bio_write_page(&io_submit, page, len, wbc);
 	ext4_io_submit(&io_submit);
 	/* Drop io_end reference we got from init */
 	ext4_put_io_end_defer(io_submit.io_end);
-	return ret;
+	return 0;
 }
 
 #define BH_FLAGS ((1 << BH_Unwritten) | (1 << BH_Delay))
@@ -1963,11 +1962,10 @@ static bool add_page_bufs_to_extent(struct mpage_da_data *mpd,
 	return true;
 }
 
-static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
+static void mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
 {
 	int len;
 	loff_t size = i_size_read(mpd->inode);
-	int err;
 
 	BUG_ON(page->index != mpd->first_page);
 	if (page->index == size >> PAGE_CACHE_SHIFT)
@@ -1975,12 +1973,9 @@ static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
 	else
 		len = PAGE_CACHE_SIZE;
 	clear_page_dirty_for_io(page);
-	err = ext4_bio_write_page(&mpd->io_submit, page, len, mpd->wbc);
-	if (!err)
-		mpd->wbc->nr_to_write--;
+	ext4_bio_write_page(&mpd->io_submit, page, len, mpd->wbc);
+	mpd->wbc->nr_to_write--;
 	mpd->first_page++;
-
-	return err;
 }
 
 /*
@@ -1997,7 +1992,7 @@ static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
  * mapped, we update @map to the next extent in the last page that needs
  * mapping. Otherwise we submit the page for IO.
  */
-static int mpage_map_and_submit_buffers(struct mpage_da_data *mpd)
+static void mpage_map_and_submit_buffers(struct mpage_da_data *mpd)
 {
 	struct pagevec pvec;
 	int nr_pages, i;
@@ -2009,7 +2004,6 @@ static int mpage_map_and_submit_buffers(struct mpage_da_data *mpd)
 	pgoff_t start, end;
 	ext4_lblk_t lblk;
 	sector_t pblock;
-	int err;
 
 	start = mpd->map.m_lblk >> bpp_bits;
 	end = (mpd->map.m_lblk + mpd->map.m_len - 1) >> bpp_bits;
@@ -2043,7 +2037,7 @@ static int mpage_map_and_submit_buffers(struct mpage_da_data *mpd)
 					add_page_bufs_to_extent(mpd, head, bh,
 								lblk);
 					pagevec_release(&pvec);
-					return 0;
+					return;
 				}
 				if (buffer_delay(bh)) {
 					clear_buffer_delay(bh);
@@ -2060,11 +2054,7 @@ static int mpage_map_and_submit_buffers(struct mpage_da_data *mpd)
 			 */
 			mpd->io_submit.io_end->size += PAGE_CACHE_SIZE;
 			/* Page fully mapped - let IO run! */
-			err = mpage_submit_page(mpd, page);
-			if (err < 0) {
-				pagevec_release(&pvec);
-				return err;
-			}
+			mpage_submit_page(mpd, page);
 			start++;
 		}
 		pagevec_release(&pvec);
@@ -2072,7 +2062,6 @@ static int mpage_map_and_submit_buffers(struct mpage_da_data *mpd)
 	/* Extent fully mapped and matches with page boundary. We are done. */
 	mpd->map.m_len = 0;
 	mpd->map.m_flags = 0;
-	return 0;
 }
 
 static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
@@ -2191,9 +2180,7 @@ static int mpage_map_and_submit_extent(handle_t *handle,
 		 * Update buffer state, submit mapped pages, and get us new
 		 * extent to map
 		 */
-		err = mpage_map_and_submit_buffers(mpd);
-		if (err < 0)
-			return err;
+		mpage_map_and_submit_buffers(mpd);
 	}
 
 	/* Update on-disk size after IO is submitted */
@@ -2201,16 +2188,12 @@ static int mpage_map_and_submit_extent(handle_t *handle,
 	if (disksize > i_size_read(inode))
 		disksize = i_size_read(inode);
 	if (disksize > EXT4_I(inode)->i_disksize) {
-		int err2;
-
 		ext4_update_i_disksize(inode, disksize);
-		err2 = ext4_mark_inode_dirty(handle, inode);
-		if (err2)
+		err = ext4_mark_inode_dirty(handle, inode);
+		if (err)
 			ext4_error(inode->i_sb,
 				   "Failed to mark inode %lu dirty",
 				   inode->i_ino);
-		if (!err)
-			err = err2;
 	}
 	return err;
 }
@@ -2321,11 +2304,8 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
 			if (!add_page_bufs_to_extent(mpd, head, head, lblk))
 				goto out;
 			/* So far everything mapped? Submit the page for IO. */
-			if (mpd->map.m_len == 0) {
-				err = mpage_submit_page(mpd, page);
-				if (err < 0)
-					goto out;
-			}
+			if (mpd->map.m_len == 0)
+				mpage_submit_page(mpd, page);
 
 			/*
 			 * Accumulated enough dirty pages? This doesn't apply
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index ce8c15a..530ccb0 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -365,7 +365,7 @@ void ext4_io_submit_init(struct ext4_io_submit *io,
 	io->io_end = NULL;
 }
 
-static int io_submit_init_bio(struct ext4_io_submit *io,
+static void io_submit_init_bio(struct ext4_io_submit *io,
 			      struct buffer_head *bh)
 {
 	int nvecs = bio_get_nr_vecs(bh->b_bdev);
@@ -378,10 +378,9 @@ static int io_submit_init_bio(struct ext4_io_submit *io,
 	bio->bi_private = ext4_get_io_end(io->io_end);
 	io->io_bio = bio;
 	io->io_next_block = bh->b_blocknr;
-	return 0;
 }
 
-static int io_submit_add_bh(struct ext4_io_submit *io,
+static void io_submit_add_bh(struct ext4_io_submit *io,
 			    struct inode *inode,
 			    struct buffer_head *bh)
 {
@@ -391,19 +390,15 @@ static int io_submit_add_bh(struct ext4_io_submit *io,
 submit_and_retry:
 		ext4_io_submit(io);
 	}
-	if (io->io_bio == NULL) {
-		ret = io_submit_init_bio(io, bh);
-		if (ret)
-			return ret;
-	}
+	if (io->io_bio == NULL)
+		io_submit_init_bio(io, bh);
 	ret = bio_add_page(io->io_bio, bh->b_page, bh->b_size, bh_offset(bh));
 	if (ret != bh->b_size)
 		goto submit_and_retry;
 	io->io_next_block++;
-	return 0;
 }
 
-int ext4_bio_write_page(struct ext4_io_submit *io,
+void ext4_bio_write_page(struct ext4_io_submit *io,
 			struct page *page,
 			int len,
 			struct writeback_control *wbc)
@@ -411,7 +406,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 	struct inode *inode = page->mapping->host;
 	unsigned block_start, blocksize;
 	struct buffer_head *bh, *head;
-	int ret = 0;
 	int nr_submitted = 0;
 
 	blocksize = 1 << inode->i_blkbits;
@@ -471,30 +465,13 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 	do {
 		if (!buffer_async_write(bh))
 			continue;
-		ret = io_submit_add_bh(io, inode, bh);
-		if (ret) {
-			/*
-			 * We only get here on ENOMEM.  Not much else
-			 * we can do but mark the page as dirty, and
-			 * better luck next time.
-			 */
-			redirty_page_for_writepage(wbc, page);
-			break;
-		}
+		io_submit_add_bh(io, inode, bh);
 		nr_submitted++;
 		clear_buffer_dirty(bh);
 	} while ((bh = bh->b_this_page) != head);
 
-	/* Error stopped previous loop? Clean up buffers... */
-	if (ret) {
-		do {
-			clear_buffer_async_write(bh);
-			bh = bh->b_this_page;
-		} while (bh != head);
-	}
 	unlock_page(page);
 	/* Nothing submitted - we have to end page writeback */
 	if (!nr_submitted)
 		end_page_writeback(page);
-	return ret;
 }
-- 
1.8.1

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] ext4: add check to io_submit_init_bio

[Theodore Ts'o]

On Thu, Jun 06, 2013 at 11:56:53AM +1000, Stephen Rothwell wrote:
> 
> In tracking this down, I followed the call chains and discovered that
> io_submit_init_bio() only ever returned 0.  Switching that to return
> void and following back up the chain lead to the following patch.

Actually, that's a bug.  We're missing an check in that function.
Thanks for looking at this code.  (I'll fix the uninitialized err
warning another way.)

						- Ted

commit 7745883f99c0e81e6bc71527d23ca363eb125a3f
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Thu Jun 6 10:18:22 2013 -0400

    ext4: add check to io_submit_init_bio
    
    The bio_alloc() function can return NULL if the memory allocation
    fails.  So we need to check for this.
    
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index ce8c15a..48786cd 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -372,6 +372,8 @@ static int io_submit_init_bio(struct ext4_io_submit *io,
 	struct bio *bio;
 
 	bio = bio_alloc(GFP_NOIO, min(nvecs, BIO_MAX_PAGES));
+	if (!bio)
+		return -ENOMEM;
 	bio->bi_sector = bh->b_blocknr * (bh->b_size >> 9);
 	bio->bi_bdev = bh->b_bdev;
 	bio->bi_end_io = ext4_end_bio;

Re: [PATCH] ext4: add check to io_submit_init_bio

[Stephen Rothwell]

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

Hi Ted,

On Thu, 6 Jun 2013 10:19:41 -0400 Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Thu, Jun 06, 2013 at 11:56:53AM +1000, Stephen Rothwell wrote:
> > 
> > In tracking this down, I followed the call chains and discovered that
> > io_submit_init_bio() only ever returned 0.  Switching that to return
> > void and following back up the chain lead to the following patch.
> 
> Actually, that's a bug.  We're missing an check in that function.
> Thanks for looking at this code.  (I'll fix the uninitialized err
> warning another way.)

Glad to be of service.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]