[PATCH 0/6 v7] ext4: fix 1k block bugs

[PATCH 0/6 v7] ext4: fix 1k block bugs

[Allison Henderson]

This patch set corrects several bugs that occur
when blocksize = 1k.

The first bug appears in xfstests 75, 112, 127 and occurs
because the partial pages appearing at the head and tail
of a hole are not properly zeroed and unmapped

A second bug in test 127 occurs when i_size and the end of
the hole appear in the same page.  Because punch hole needs
to flush the pages in the hole, it will need to properly
unmap the partial page appearing after i_size.

A similar bug occurred during extra fsx testing for the
first two patches.  This bug happens because truncate also
does not zero and unmap the partial page at the end of the
file.

Similar bugs also occurred when a write operation begins or
ends in a hole or extends EOF.  The partial page of the write
operation needs to be zeroed and unmapped.

These bugs are corrected using a new ext4_discard_partial_page_buffers
routine that zeros a specified region of a page, and unmaps buffer
heads for any block aligned region of the page that was completely
zeroed.

This set also contains a fix for a bug reported by Lukas while
working on a new patch to add discard support to loop devices using
punch hole.  This bug occurs when calculating the byte block offset for
very large holes.  Since the code now needs the byte offset of the
pages instead of the blocks, the problem code is simply removed.

Lastly the set contains a patch to lock i_mutex for punch hole.
Yongquiang noticed that the vfs locks i_mutex for truncate,
but not for fallocate, so fallocate will need to take i_mutex
before calling punch hole.
 
This patch set has passed the following tests
(for both 1k and 4k blocksizes):
xfstests 75, 112, 127, 130, 252 and 256
fsx stress test (12 hours)
 
v1 -> v2
Added EXT4_BLOCK_ZERO_DISCARD_BUFFER flag

v2 -> v3
Moved code out of ext4_zero_block_page_range and in
to new ext4_unmap_page_range function

v3 -> v4
Renamed ext4_unmap_page_range to ext4_unmap_partial_page_buffers
Moved ext4_unmap_partial_page_buffers from inode.c to extents.c
Corrected comments for non block/page aligned handling
Added checks to avoid unnecessary page unmaps
Removed unneeded journaling and mapping from new routine

v4 -> v5
Renamed ext4_unmap_partial_page_buffers to
ext4_discard_partial_page_buffers_no_lock and added
new ext4_discard_partial_page_buffers wrapper function

Modified ext4_discard_partial_page_buffers_no_lock to
zero the page as well as unmap buffers

Moved ext4_discard_partial_page_buffers functions
back to inode.c, and also put in a separate patch.

Added extra patches for write bugs and truncate bugs

Added extra patch for large hole calculation

v5 -> v6
Merged patch 4 into patch 2 since block offsets are
no longer needed

Removed uneeded ClearPageUptodate when unmapping
buffer heads

Patch set now passes test 130.  The
ext4_discard_partial_page_buffers_no_lock routine
was modified to map buffer heads for unblock aligned
regions of the page (if needed) so that they can be
updated with the contents of the block before being
partially zeroed

v6 -> v7
Added feedback to simplify ext4_discard_partial_page_buffers_no_lock

Remove journaling for buffer heads that are block aligned

Do not mark unmapped buffers dirty (causes a warning)

Added patch to lock i_mutex for punch hole

Allison Henderson (6):
  ext4: Add new ext4_discard_partial_page_buffers routines
  ext4: fix xfstests 75, 112, 127 punch hole failure
  ext4: fix 2nd xfstests 127 punch hole failure
  ext4: Lock i_mutex for punch hole
  ext4: fix fsx truncate failure
  ext4: fix partial page writes

 fs/ext4/ext4.h     |   11 +++
 fs/ext4/extents.c  |  126 ++++++++++++++++++++-------
 fs/ext4/indirect.c |   16 +++-
 fs/ext4/inode.c    |  243 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 363 insertions(+), 33 deletions(-)

[PATCH 1/6 v7] ext4: Add new ext4_discard_partial_page_buffers routines

[Allison Henderson]

This patch adds two new routines: ext4_discard_partial_page_buffers
and ext4_discard_partial_page_buffers_no_lock.

The ext4_discard_partial_page_buffers routine is a wrapper
function to ext4_discard_partial_page_buffers_no_lock.
The wrapper function locks the page and passes it to
ext4_discard_partial_page_buffers_no_lock.
Calling functions that already have the page locked can call
ext4_discard_partial_page_buffers_no_lock directly.

The ext4_discard_partial_page_buffers_no_lock function
zeros a specified range in a page, and unmaps the
corresponding buffer heads.  Only block aligned regions of the
page will have their buffer heads unmapped.  Unblock aligned regions
will be mapped if needed so that they can be updated with the
partial zero out.  This function is meant to
be used to update a page and its buffer heads to be zeroed
and unmaped when the corresponding blocks have been released
or will be released.

This routine is used in the following scenarios:
* A hole is punched and the non page aligned regions
  of the head and tail of the hole need to be discarded

* The file is truncated and the partial page beyond EOF needs
  to be discarded

* The end of a hole is in the same page as EOF.  After the
  page is flushed, the partial page beyond EOF needs to be
  discarded.

* A write operation begins or ends inside a hole and the partial
  page appearing before or after the write needs to be discarded

* A write operation extends EOF and the partial page beyond EOF
  needs to be discarded

This function takes a flag EXT4_DSCRD_PARTIAL_PG_ZERO_UNMAPED
which is used when a write operation begins or ends in a hole.
When the EXT4_DSCRD_PARTIAL_PG_ZERO_UNMAPED flag is used, only
buffer heads that are already unmapped will have the corresponding
regions of the page zeroed.

Signed-off-by: Allison Henderson <achender@linux.vnet.ibm.com>
---
:100644 100644 e717dfd... 55a022f... M	fs/ext4/ext4.h
:100644 100644 c4da98a... 8c91991... M	fs/ext4/inode.c
 fs/ext4/ext4.h  |   11 +++
 fs/ext4/inode.c |  224 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 235 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index e717dfd..55a022f 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -529,6 +529,11 @@ struct ext4_new_group_data {
 #define EXT4_FREE_BLOCKS_NO_QUOT_UPDATE	0x0008
 
 /*
+ * Flags used by ext4_discard_partial_page_buffers
+ */
+#define EXT4_DSCRD_PARTIAL_PG_ZERO_UNMAPED	0x0001
+
+/*
  * ioctl commands
  */
 #define	EXT4_IOC_GETFLAGS		FS_IOC_GETFLAGS
@@ -1838,6 +1843,12 @@ extern int ext4_block_truncate_page(handle_t *handle,
 		struct address_space *mapping, loff_t from);
 extern int ext4_block_zero_page_range(handle_t *handle,
 		struct address_space *mapping, loff_t from, loff_t length);
+extern int ext4_discard_partial_page_buffers(handle_t *handle,
+		struct address_space *mapping, loff_t from,
+		loff_t length, int flags);
+extern int ext4_discard_partial_page_buffers_no_lock(handle_t *handle,
+		struct inode *inode, struct page *page, loff_t from,
+		loff_t length, int flags);
 extern int ext4_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf);
 extern qsize_t *ext4_get_reserved_space(struct inode *inode);
 extern void ext4_da_update_reserve_space(struct inode *inode,
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index c4da98a..8c91991 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2962,6 +2962,230 @@ void ext4_set_aops(struct inode *inode)
 		inode->i_mapping->a_ops = &ext4_journalled_aops;
 }
 
+
+/*
+ * ext4_discard_partial_page_buffers()
+ * Wrapper function for ext4_discard_partial_page_buffers_no_lock.
+ * This function finds and locks the page containing the offset
+ * "from" and passes it to ext4_discard_partial_page_buffers_no_lock.
+ * Calling functions that already have the page locked should call
+ * ext4_discard_partial_page_buffers_no_lock directly.
+ */
+int ext4_discard_partial_page_buffers(handle_t *handle,
+		struct address_space *mapping, loff_t from,
+		loff_t length, int flags)
+{
+	struct inode *inode = mapping->host;
+	struct page *page;
+	int err = 0;
+
+	page = find_or_create_page(mapping, from >> PAGE_CACHE_SHIFT,
+				   mapping_gfp_mask(mapping) & ~__GFP_FS);
+	if (!page)
+		return -EINVAL;
+
+	err = ext4_discard_partial_page_buffers_no_lock(handle, inode, page,
+		from, length, flags);
+
+	unlock_page(page);
+	page_cache_release(page);
+	return err;
+}
+
+/*
+ * ext4_discard_partial_page_buffers_no_lock()
+ * Zeros a page range of length 'length' starting from offset 'from'.
+ * Buffer heads that correspond to the block aligned regions of the
+ * zeroed range will be unmapped.  Unblock aligned regions
+ * will have the corresponding buffer head mapped if needed so that
+ * that region of the page can be updated with the partial zero out.
+ *
+ * This function assumes that the page has already been  locked.  The
+ * The range to be discarded must be contained with in the given page.
+ * If the specified range exceeds the end of the page it will be shortened
+ * to the end of the page that corresponds to 'from'.  This function is
+ * appropriate for updateing a page and it buffer heads to be unmaped and
+ * zeroed for blocks that have been either released, or are going to be
+ * released.
+ *
+ * handle: The journal handle
+ * inode:  The files inode
+ * page:   A locked page that contains the offset "from"
+ * from:   The starting byte offset (from the begining of the file)
+ *         to begin discarding
+ * len:    The length of bytes to discard
+ * flags:  Optional flags that may be used:
+ *
+ *         EXT4_DSCRD_PARTIAL_PG_ZERO_UNMAPED
+ *         Only zero the regions of the page whose buffer heads
+ *         have already been unmapped.  This flag is appropriate
+ *         for updateing the contents of a page whose blocks may
+ *         have already been released, and we only want to zero
+ *         out the regions that correspond to those released blocks.
+ *
+ * Returns zero on sucess or negative on failure.
+ */
+int ext4_discard_partial_page_buffers_no_lock(handle_t *handle,
+		struct inode *inode, struct page *page, loff_t from,
+		loff_t length, int flags)
+{
+	ext4_fsblk_t index = from >> PAGE_CACHE_SHIFT;
+	unsigned int offset = from & (PAGE_CACHE_SIZE-1);
+	unsigned int blocksize, max, pos;
+	unsigned int end_of_block, range_to_discard;
+	ext4_lblk_t iblock;
+	struct buffer_head *bh;
+	int err = 0;
+
+	blocksize = inode->i_sb->s_blocksize;
+	max = PAGE_CACHE_SIZE - offset;
+
+	if (index != page->index)
+		return -EINVAL;
+
+	/*
+	 * correct length if it does not fall between
+	 * 'from' and the end of the page
+	 */
+	if (length > max || length < 0)
+		length = max;
+
+	iblock = index << (PAGE_CACHE_SHIFT - inode->i_sb->s_blocksize_bits);
+
+	if (!page_has_buffers(page)) {
+		/*
+		 * If the range to be discarded covers a partial block
+		 * we need to get the page buffers.  This is because
+		 * partial blocks cannot be released and the page needs
+		 * to be updated with the contents of the block before
+		 * we write the zeros on top of it.
+		 */
+		if (!(from & (blocksize - 1)) ||
+		    !((from + length) & (blocksize - 1))) {
+			create_empty_buffers(page, blocksize, 0);
+		} else {
+			/*
+			 * If there are no partial blocks,
+			 * there is nothing to update,
+			 * so we can return now
+			 */
+			return 0;
+		}
+	}
+
+	/* Find the buffer that contains "offset" */
+	bh = page_buffers(page);
+	pos = blocksize;
+	while (offset >= pos) {
+		bh = bh->b_this_page;
+		iblock++;
+		pos += blocksize;
+	}
+
+	pos = offset;
+	while (pos < offset + length) {
+		err = 0;
+
+		/* The length of space left to zero and unmap */
+		range_to_discard = offset + length - pos;
+
+		/* The length of space until the end of the block */
+		end_of_block = blocksize - (pos & (blocksize-1));
+
+		/*
+		 * Do not unmap or zero past end of block
+		 * for this buffer head
+		 */
+		if (range_to_discard > end_of_block)
+			range_to_discard = end_of_block;
+
+
+		/*
+		 * Skip this buffer head if we are only zeroing unampped
+		 * regions of the page
+		 */
+		if (flags & EXT4_DSCRD_PARTIAL_PG_ZERO_UNMAPED &&
+			buffer_mapped(bh))
+				goto next;
+
+		/* If the range is block aligned, unmap */
+		if (range_to_discard == blocksize) {
+			clear_buffer_dirty(bh);
+			bh->b_bdev = NULL;
+			clear_buffer_mapped(bh);
+			clear_buffer_req(bh);
+			clear_buffer_new(bh);
+			clear_buffer_delay(bh);
+			clear_buffer_unwritten(bh);
+			clear_buffer_uptodate(bh);
+			zero_user(page, pos, range_to_discard);
+			BUFFER_TRACE(bh, "Buffer discarded");
+			goto next;
+		}
+
+		/*
+		 * If this block is not completely contained in the range
+		 * to be discarded, then it is not going to be released. Because
+		 * we need to keep this block, we need to make sure this part
+		 * of the page is uptodate before we modify it by writeing
+		 * partial zeros on it.
+		 */
+		if (!buffer_mapped(bh)) {
+			/*
+			 * Buffer head must be mapped before we can read
+			 * from the block
+			 */
+			BUFFER_TRACE(bh, "unmapped");
+			ext4_get_block(inode, iblock, bh, 0);
+			/* unmapped? It's a hole - nothing to do */
+			if (!buffer_mapped(bh)) {
+				BUFFER_TRACE(bh, "still unmapped");
+				goto next;
+			}
+		}
+
+		/* Ok, it's mapped. Make sure it's up-to-date */
+		if (PageUptodate(page))
+			set_buffer_uptodate(bh);
+
+		if (!buffer_uptodate(bh)) {
+			err = -EIO;
+			ll_rw_block(READ, 1, &bh);
+			wait_on_buffer(bh);
+			/* Uhhuh. Read error. Complain and punt.*/
+			if (!buffer_uptodate(bh))
+				goto next;
+		}
+
+		if (ext4_should_journal_data(inode)) {
+			BUFFER_TRACE(bh, "get write access");
+			err = ext4_journal_get_write_access(handle, bh);
+			if (err)
+				goto next;
+		}
+
+		zero_user(page, pos, range_to_discard);
+
+		err = 0;
+		if (ext4_should_journal_data(inode)) {
+			err = ext4_handle_dirty_metadata(handle, inode, bh);
+		} else {
+			if (ext4_should_order_data(inode) &&
+			   EXT4_I(inode)->jinode)
+				err = ext4_jbd2_file_inode(handle, inode);
+			mark_buffer_dirty(bh);
+		}
+
+		BUFFER_TRACE(bh, "Partial buffer zeroed");
+next:
+		bh = bh->b_this_page;
+		iblock++;
+		pos += range_to_discard;
+	}
+
+	return err;
+}
+
 /*
  * ext4_block_truncate_page() zeroes out a mapping from file offset `from'
  * up to the end of the block which corresponds to `from'.
-- 
1.7.1

[PATCH 2/6 v7] ext4: fix xfstests 75, 112, 127 punch hole failure

[Allison Henderson]

This patch addresses a bug found by xfstests 75, 112, 127
when blocksize = 1k

This bug happens because the punch hole code only zeros
out non block aligned regions of the page.  This means that if the
blocks are smaller than a page, then the block aligned regions of
the page inside the hole are left un-zeroed, and their buffer heads
are still mapped.  This bug is corrected by using
ext4_discard_partial_page_buffers to properly zero the partial page
at the head and tail of the hole, and unmap the corresponding buffer
heads

This patch also addresses a bug reported by Lukas while working on a
new patch to add discard support for loop devices using punch hole.
The bug happened because of the first and last block number
needed to be cast to a larger data type before calculating the
byte offset, but since now we only need the byte offsets of the
pages, we no longer even need to be calculating the byte offsets
of the blocks.  The code to do the block offset calculations is
removed in this patch.

Signed-off-by: Allison Henderson <achender@linux.vnet.ibm.com>
---
:100644 100644 57cf568... 18f7e04... M	fs/ext4/extents.c
 fs/ext4/extents.c |   61 +++++++++++++++++++++++++++++++++-------------------
 1 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 57cf568..18f7e04 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4162,17 +4162,14 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length)
 	struct address_space *mapping = inode->i_mapping;
 	struct ext4_map_blocks map;
 	handle_t *handle;
-	loff_t first_block_offset, last_block_offset, block_len;
-	loff_t first_page, last_page, first_page_offset, last_page_offset;
+	loff_t first_page, last_page, page_len;
+	loff_t first_page_offset, last_page_offset;
 	int ret, credits, blocks_released, err = 0;
 
 	first_block = (offset + sb->s_blocksize - 1) >>
 		EXT4_BLOCK_SIZE_BITS(sb);
 	last_block = (offset + length) >> EXT4_BLOCK_SIZE_BITS(sb);
 
-	first_block_offset = first_block << EXT4_BLOCK_SIZE_BITS(sb);
-	last_block_offset = last_block << EXT4_BLOCK_SIZE_BITS(sb);
-
 	first_page = (offset + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
 	last_page = (offset + length) >> PAGE_CACHE_SHIFT;
 
@@ -4211,24 +4208,44 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length)
 		goto out;
 
 	/*
-	 * Now we need to zero out the un block aligned data.
-	 * If the file is smaller than a block, just
-	 * zero out the middle
+	 * Now we need to zero out the non-page-aligned data in the
+	 * pages at the start and tail of the hole, and unmap the buffer
+	 * heads for the block aligned regions of the page that were
+	 * completely zeroed.
 	 */
-	if (first_block > last_block)
-		ext4_block_zero_page_range(handle, mapping, offset, length);
-	else {
-		/* zero out the head of the hole before the first block */
-		block_len  = first_block_offset - offset;
-		if (block_len > 0)
-			ext4_block_zero_page_range(handle, mapping,
-						   offset, block_len);
-
-		/* zero out the tail of the hole after the last block */
-		block_len = offset + length - last_block_offset;
-		if (block_len > 0) {
-			ext4_block_zero_page_range(handle, mapping,
-					last_block_offset, block_len);
+	if (first_page > last_page) {
+		/*
+		 * If the file space being truncated is contained within a page
+		 * just zero out and unmap the middle of that page
+		 */
+		err = ext4_discard_partial_page_buffers(handle,
+			mapping, offset, length, 0);
+
+		if (err)
+			goto out;
+	} else {
+		/*
+		 * zero out and unmap the partial page that contains
+		 * the start of the hole
+		 */
+		page_len  = first_page_offset - offset;
+		if (page_len > 0) {
+			err = ext4_discard_partial_page_buffers(handle, mapping,
+						   offset, page_len, 0);
+			if (err)
+				goto out;
+		}
+
+		/*
+		 * zero out and unmap the partial page that contains
+		 * the end of the hole
+		 */
+		page_len = offset + length - last_page_offset;
+		if (page_len > 0) {
+			err = ext4_discard_partial_page_buffers(handle, mapping,
+					last_page_offset, page_len, 0);
+			if (err)
+				goto out;
 		}
 	}
 
-- 
1.7.1

[PATCH 3/6 v7] ext4: fix 2nd xfstests 127 punch hole failure

[Allison Henderson]

This patch fixes a second punch hole bug found by xfstests 127.

This bug happens because punch hole needs to flush the pages
of the hole to avoid race conditions.  But if the end of the
hole is in the same page as i_size, the buffer heads beyond
i_size need to be unmapped and the page needs to be zeroed
after it is flushed.

To correct this, the new ext4_discard_partial_page_buffers
routine is used to zero and unmap the partial page
beyond i_size if the end of the hole appears in the same
page as i_size.

The code has also been optimized to set the end of the hole
to the page after i_size if the specified hole exceeds i_size,
and the code that flushes the pages has been simplified.

Signed-off-by: Allison Henderson <achender@linux.vnet.ibm.com>
---
:100644 100644 18f7e04... 9124cd2... M	fs/ext4/extents.c
 fs/ext4/extents.c |   41 +++++++++++++++++++++++++++++++++++++----
 1 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 18f7e04..9124cd2 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4166,6 +4166,20 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length)
 	loff_t first_page_offset, last_page_offset;
 	int ret, credits, blocks_released, err = 0;
 
+	/* No need to punch hole beyond i_size */
+	if (offset >= inode->i_size)
+		return 0;
+
+	/*
+	 * If the hole extends beyond i_size, set the hole
+	 * to end after the page that contains i_size
+	 */
+	if (offset + length > inode->i_size) {
+		length = inode->i_size +
+		   PAGE_CACHE_SIZE - (inode->i_size & (PAGE_CACHE_SIZE - 1)) -
+		   offset;
+	}
+
 	first_block = (offset + sb->s_blocksize - 1) >>
 		EXT4_BLOCK_SIZE_BITS(sb);
 	last_block = (offset + length) >> EXT4_BLOCK_SIZE_BITS(sb);
@@ -4182,11 +4196,10 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length)
 	 */
 	if (mapping->nrpages && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
 		err = filemap_write_and_wait_range(mapping,
-			first_page_offset == 0 ? 0 : first_page_offset-1,
-			last_page_offset);
+			offset, offset + length - 1);
 
-			if (err)
-				return err;
+		if (err)
+			return err;
 	}
 
 	/* Now release the pages */
@@ -4249,6 +4262,26 @@ int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length)
 		}
 	}
 
+
+	/*
+	 * If i_size is contained in the last page, we need to
+	 * unmap and zero the partial page after i_size
+	 */
+	if (inode->i_size >> PAGE_CACHE_SHIFT == last_page &&
+	   inode->i_size % PAGE_CACHE_SIZE != 0) {
+
+		page_len = PAGE_CACHE_SIZE -
+			(inode->i_size & (PAGE_CACHE_SIZE - 1));
+
+		if (page_len > 0) {
+			err = ext4_discard_partial_page_buffers(handle,
+			  mapping, inode->i_size, page_len, 0);
+
+			if (err)
+				goto out;
+		}
+	}
+
 	/* If there are no blocks to remove, return now */
 	if (first_block >= last_block)
 		goto out;
-- 
1.7.1

[PATCH 4/6 v7] ext4: Lock i_mutex for punch hole

[Allison Henderson]

This patch modifies the fallocate routine to lock i_mutex during
the punch hole operation.

Yongqiang noticed that the vfs layer locks i_mutex for truncate,
but not fallocate, so the fallocate routine will need to take
care of locking i_mutex.  Otherwise a page may be mapped after
punch hole has released the pages, but before i_data_sem is
locked to release the blocks in the extent tree.

Signed-off-by: Allison Henderson <achender@linux.vnet.ibm.com>
---
:100644 100644 9124cd2... 007fb08... M	fs/ext4/extents.c
 fs/ext4/extents.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 9124cd2..007fb08 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3774,8 +3774,13 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
 		return -EOPNOTSUPP;
 
-	if (mode & FALLOC_FL_PUNCH_HOLE)
-		return ext4_punch_hole(file, offset, len);
+	mutex_lock(&inode->i_mutex);
+
+	if (mode & FALLOC_FL_PUNCH_HOLE) {
+		ret = ext4_punch_hole(file, offset, len);
+		mutex_unlock(&inode->i_mutex);
+		return ret;	
+	}
 
 	trace_ext4_fallocate_enter(inode, offset, len, mode);
 	map.m_lblk = offset >> blkbits;
@@ -3789,7 +3794,6 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	 * credits to insert 1 extent into extent tree
 	 */
 	credits = ext4_chunk_trans_blocks(inode, max_blocks);
-	mutex_lock(&inode->i_mutex);
 	ret = inode_newsize_ok(inode, (len + offset));
 	if (ret) {
 		mutex_unlock(&inode->i_mutex);
-- 
1.7.1

[PATCH 5/6 v7] ext4: fix fsx truncate failure

[Allison Henderson]

While running extended fsx tests to verify the first
two patches, a similar bug was also found in the
truncate operation

This bug happens because the truncate routine only zeros
the unblock aligned portion of the last page.  This means
that the block aligned portions of the page appearing after
i_size are left unzeroed, and the buffer heads still mapped.

This bug is corrected by using ext4_discard_partial_page_buffers
in the truncate routine to zero the partial page and unmap
the buffer headers

Signed-off-by: Allison Henderson <achender@linux.vnet.ibm.com>
---
:100644 100644 007fb08... b6cb686... M	fs/ext4/extents.c
:100644 100644 0962642... 9ae1d44... M	fs/ext4/indirect.c
 fs/ext4/extents.c  |   14 ++++++++++++--
 fs/ext4/indirect.c |   16 ++++++++++++++--
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 007fb08..b6cb686 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3649,6 +3649,7 @@ void ext4_ext_truncate(struct inode *inode)
 	struct super_block *sb = inode->i_sb;
 	ext4_lblk_t last_block;
 	handle_t *handle;
+	loff_t page_len;
 	int err = 0;
 
 	/*
@@ -3665,8 +3666,17 @@ void ext4_ext_truncate(struct inode *inode)
 	if (IS_ERR(handle))
 		return;
 
-	if (inode->i_size & (sb->s_blocksize - 1))
-		ext4_block_truncate_page(handle, mapping, inode->i_size);
+	if (inode->i_size % PAGE_CACHE_SIZE != 0) {
+
+		page_len = PAGE_CACHE_SIZE -
+			(inode->i_size & (PAGE_CACHE_SIZE - 1));
+
+		err = ext4_discard_partial_page_buffers(handle,
+			mapping, inode->i_size, page_len, 0);
+
+		if (err)
+			goto out_stop;
+	}
 
 	if (ext4_orphan_add(handle, inode))
 		goto out_stop;
diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index 0962642..9ae1d44 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -1343,7 +1343,9 @@ void ext4_ind_truncate(struct inode *inode)
 	__le32 nr = 0;
 	int n = 0;
 	ext4_lblk_t last_block, max_block;
+	loff_t page_len;
 	unsigned blocksize = inode->i_sb->s_blocksize;
+	int err;
 
 	handle = start_transaction(inode);
 	if (IS_ERR(handle))
@@ -1354,9 +1356,19 @@ void ext4_ind_truncate(struct inode *inode)
 	max_block = (EXT4_SB(inode->i_sb)->s_bitmap_maxbytes + blocksize-1)
 					>> EXT4_BLOCK_SIZE_BITS(inode->i_sb);
 
-	if (inode->i_size & (blocksize - 1))
-		if (ext4_block_truncate_page(handle, mapping, inode->i_size))
+
+	if (inode->i_size % PAGE_CACHE_SIZE != 0) {
+
+		page_len = PAGE_CACHE_SIZE -
+			(inode->i_size & (PAGE_CACHE_SIZE - 1));
+
+		err = ext4_discard_partial_page_buffers(handle,
+			mapping, inode->i_size, page_len, 0);
+
+		if (err)
 			goto out_stop;
+	}
+
 
 	if (last_block != max_block) {
 		n = ext4_block_to_path(inode, last_block, offsets, NULL);
-- 
1.7.1

[PATCH 6/6 v7] ext4: fix partial page writes

[Allison Henderson]

While running extended fsx tests to verify the preceeding patches,
a similar bug was also found in the write operation

When ever a write operation begins or ends in a hole,
or extends EOF, the partial page contained in the hole
or beyond EOF needs to be zeroed out.

To correct this the new ext4_discard_partial_page_buffers_no_lock
routine is used to zero out the partial page, but only for buffer
heads that are already unmapped.

Signed-off-by: Allison Henderson <achender@linux.vnet.ibm.com>
---
:100644 100644 8c91991... 88db8e5... M	fs/ext4/inode.c
 fs/ext4/inode.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 8c91991..88db8e5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2258,6 +2258,7 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
 	pgoff_t index;
 	struct inode *inode = mapping->host;
 	handle_t *handle;
+	loff_t page_len;
 
 	index = pos >> PAGE_CACHE_SHIFT;
 
@@ -2304,6 +2305,13 @@ retry:
 		 */
 		if (pos + len > inode->i_size)
 			ext4_truncate_failed_write(inode);
+	} else {
+		page_len = pos & (PAGE_CACHE_SIZE - 1);
+		if (page_len > 0) {
+			ret = ext4_discard_partial_page_buffers_no_lock(handle,
+				inode, page, pos - page_len, page_len,
+				EXT4_DSCRD_PARTIAL_PG_ZERO_UNMAPED);
+		}
 	}
 
 	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
@@ -2346,6 +2354,7 @@ static int ext4_da_write_end(struct file *file,
 	loff_t new_i_size;
 	unsigned long start, end;
 	int write_mode = (int)(unsigned long)fsdata;
+	loff_t page_len;
 
 	if (write_mode == FALL_BACK_TO_NONDELALLOC) {
 		if (ext4_should_order_data(inode)) {
@@ -2394,6 +2403,16 @@ static int ext4_da_write_end(struct file *file,
 	}
 	ret2 = generic_write_end(file, mapping, pos, len, copied,
 							page, fsdata);
+
+	page_len = PAGE_CACHE_SIZE -
+			((pos + copied - 1) & (PAGE_CACHE_SIZE - 1));
+
+	if (page_len > 0) {
+		ret = ext4_discard_partial_page_buffers_no_lock(handle,
+			inode, page, pos + copied - 1, page_len,
+			EXT4_DSCRD_PARTIAL_PG_ZERO_UNMAPED);
+	}
+
 	copied = ret2;
 	if (ret2 < 0)
 		ret = ret2;
-- 
1.7.1

Re: [PATCH 4/6 v7] ext4: Lock i_mutex for punch hole

[Andreas Dilger]

On 2011-08-30, at 6:28 PM, Allison Henderson wrote:
> This patch modifies the fallocate routine to lock i_mutex during
> the punch hole operation.
> 
> Yongqiang noticed that the vfs layer locks i_mutex for truncate,
> but not fallocate, so the fallocate routine will need to take
> care of locking i_mutex.  Otherwise a page may be mapped after
> punch hole has released the pages, but before i_data_sem is
> locked to release the blocks in the extent tree.

If the VFS is locking i_mutex for truncate, shouldn't the locking for
fallocate() also be done in the VFS?

> Signed-off-by: Allison Henderson <achender@linux.vnet.ibm.com>
> ---
> :100644 100644 9124cd2... 007fb08... M	fs/ext4/extents.c
> fs/ext4/extents.c |   10 +++++++---
> 1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 9124cd2..007fb08 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3774,8 +3774,13 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> 	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
> 		return -EOPNOTSUPP;
> 
> -	if (mode & FALLOC_FL_PUNCH_HOLE)
> -		return ext4_punch_hole(file, offset, len);
> +	mutex_lock(&inode->i_mutex);
> +
> +	if (mode & FALLOC_FL_PUNCH_HOLE) {
> +		ret = ext4_punch_hole(file, offset, len);
> +		mutex_unlock(&inode->i_mutex);
> +		return ret;	
> +	}
> 
> 	trace_ext4_fallocate_enter(inode, offset, len, mode);
> 	map.m_lblk = offset >> blkbits;
> @@ -3789,7 +3794,6 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> 	 * credits to insert 1 extent into extent tree
> 	 */
> 	credits = ext4_chunk_trans_blocks(inode, max_blocks);
> -	mutex_lock(&inode->i_mutex);
> 	ret = inode_newsize_ok(inode, (len + offset));
> 	if (ret) {
> 		mutex_unlock(&inode->i_mutex);
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas

Re: [PATCH 4/6 v7] ext4: Lock i_mutex for punch hole

[Allison Henderson]

On 08/30/2011 05:31 PM, Andreas Dilger wrote:
> On 2011-08-30, at 6:28 PM, Allison Henderson wrote:
>> This patch modifies the fallocate routine to lock i_mutex during
>> the punch hole operation.
>>
>> Yongqiang noticed that the vfs layer locks i_mutex for truncate,
>> but not fallocate, so the fallocate routine will need to take
>> care of locking i_mutex.  Otherwise a page may be mapped after
>> punch hole has released the pages, but before i_data_sem is
>> locked to release the blocks in the extent tree.
>
> If the VFS is locking i_mutex for truncate, shouldn't the locking for
> fallocate() also be done in the VFS?

Alrighty, I will do some poking around with this idea first.  I dont 
know if other files systems are already locking i_mutex during 
fallocate, and it may be the case that not all filesystems need i_mutex 
locked for fallocate.  So I'll do a little research first, but I will 
post back with what I find.  Thx!

Allison Henderson

>
>> Signed-off-by: Allison Henderson<achender@linux.vnet.ibm.com>
>> ---
>> :100644 100644 9124cd2... 007fb08... M	fs/ext4/extents.c
>> fs/ext4/extents.c |   10 +++++++---
>> 1 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 9124cd2..007fb08 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -3774,8 +3774,13 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>> 	if (mode&  ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
>> 		return -EOPNOTSUPP;
>>
>> -	if (mode&  FALLOC_FL_PUNCH_HOLE)
>> -		return ext4_punch_hole(file, offset, len);
>> +	mutex_lock(&inode->i_mutex);
>> +
>> +	if (mode&  FALLOC_FL_PUNCH_HOLE) {
>> +		ret = ext4_punch_hole(file, offset, len);
>> +		mutex_unlock(&inode->i_mutex);
>> +		return ret;	
>> +	}
>>
>> 	trace_ext4_fallocate_enter(inode, offset, len, mode);
>> 	map.m_lblk = offset>>  blkbits;
>> @@ -3789,7 +3794,6 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>> 	 * credits to insert 1 extent into extent tree
>> 	 */
>> 	credits = ext4_chunk_trans_blocks(inode, max_blocks);
>> -	mutex_lock(&inode->i_mutex);
>> 	ret = inode_newsize_ok(inode, (len + offset));
>> 	if (ret) {
>> 		mutex_unlock(&inode->i_mutex);
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> Cheers, Andreas
>
>
>
>
>

Re: [PATCH 4/6 v7] ext4: Lock i_mutex for punch hole

[Andreas Dilger]

On 2011-08-31, at 12:49 PM, Allison Henderson wrote:
> On 08/30/2011 05:31 PM, Andreas Dilger wrote:
>> On 2011-08-30, at 6:28 PM, Allison Henderson wrote:
>>> This patch modifies the fallocate routine to lock i_mutex during
>>> the punch hole operation.
>>> 
>>> Yongqiang noticed that the vfs layer locks i_mutex for truncate,
>>> but not fallocate, so the fallocate routine will need to take
>>> care of locking i_mutex.  Otherwise a page may be mapped after
>>> punch hole has released the pages, but before i_data_sem is
>>> locked to release the blocks in the extent tree.
>> 
>> If the VFS is locking i_mutex for truncate, shouldn't the locking for
>> fallocate() also be done in the VFS?
> 
> Alrighty, I will do some poking around with this idea first.  I dont know if other files systems are already locking i_mutex during fallocate, and it may be the case that not all filesystems need i_mutex locked for fallocate.  So I'll do a little research first, but I will post back with what I find.  Thx!

Probably a good idea to bring up this issue on linux-fsdevel.

>>> Signed-off-by: Allison Henderson<achender@linux.vnet.ibm.com>
>>> ---
>>> :100644 100644 9124cd2... 007fb08... M	fs/ext4/extents.c
>>> fs/ext4/extents.c |   10 +++++++---
>>> 1 files changed, 7 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>>> index 9124cd2..007fb08 100644
>>> --- a/fs/ext4/extents.c
>>> +++ b/fs/ext4/extents.c
>>> @@ -3774,8 +3774,13 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>>> 	if (mode&  ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
>>> 		return -EOPNOTSUPP;
>>> 
>>> -	if (mode&  FALLOC_FL_PUNCH_HOLE)
>>> -		return ext4_punch_hole(file, offset, len);
>>> +	mutex_lock(&inode->i_mutex);
>>> +
>>> +	if (mode&  FALLOC_FL_PUNCH_HOLE) {
>>> +		ret = ext4_punch_hole(file, offset, len);
>>> +		mutex_unlock(&inode->i_mutex);
>>> +		return ret;	
>>> +	}
>>> 
>>> 	trace_ext4_fallocate_enter(inode, offset, len, mode);
>>> 	map.m_lblk = offset>>  blkbits;
>>> @@ -3789,7 +3794,6 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>>> 	 * credits to insert 1 extent into extent tree
>>> 	 */
>>> 	credits = ext4_chunk_trans_blocks(inode, max_blocks);
>>> -	mutex_lock(&inode->i_mutex);
>>> 	ret = inode_newsize_ok(inode, (len + offset));
>>> 	if (ret) {
>>> 		mutex_unlock(&inode->i_mutex);
>>> --
>>> 1.7.1
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> 
>> Cheers, Andreas
>> 
>> 
>> 
>> 
>> 
> 


Cheers, Andreas

Re: [PATCH 0/6 v7] ext4: fix 1k block bugs

[Ted Ts'o]

Allison,

Many, many thanks for your work on this patch series!!  It's clearly
been a very long slog.

I've pulled in the first three patches, since I think those are
clearly correct at this point.  One minor change I've made was I fixed
the spelling the flag, which I channged to
EXT4_DISCARD_PARTIAL_PG_ZERO_UNMAPPED.  (Sorry, but seeing MAPED
everywhere was just grating on my nerves.  :-)

I didn't want to apply your 4th patch in the series since we're
planning on reducing the usage of i_mutex in the fs/ext4 code proper,
as recommended by Christoph.  It wasn't immediately obvious to me
whether it was safe to apply any of the patches after #4, so for now
I've just merged in the first three, since they clearly fix real
problems that show up in xfstests.

Can you comment on whether patches #5, #6, and #7 depend on #4?

Thanks,

						- Ted

Re: [PATCH 0/6 v7] ext4: fix 1k block bugs

[Allison Henderson]

On 09/05/2011 09:59 PM, Ted Ts'o wrote:
> Allison,
>
> Many, many thanks for your work on this patch series!!  It's clearly
> been a very long slog.
>
> I've pulled in the first three patches, since I think those are
> clearly correct at this point.  One minor change I've made was I fixed
> the spelling the flag, which I channged to
> EXT4_DISCARD_PARTIAL_PG_ZERO_UNMAPPED.  (Sorry, but seeing MAPED
> everywhere was just grating on my nerves.  :-)
>
> I didn't want to apply your 4th patch in the series since we're
> planning on reducing the usage of i_mutex in the fs/ext4 code proper,
> as recommended by Christoph.  It wasn't immediately obvious to me
> whether it was safe to apply any of the patches after #4, so for now
> I've just merged in the first three, since they clearly fix real
> problems that show up in xfstests.
>
> Can you comment on whether patches #5, #6, and #7 depend on #4?
>
> Thanks,
>
> 						- Ted

Hi Ted,

Patches 5 and 6 do not depend on 4, and this set only has 6 patches, so 
no need to worry about patch 7 :)  So it is ok to just skip patch 4, I 
will see if I can find another way to solve the race conditions 
Yongqiang pointed out with out the use of i_mutex.  Many thanks to you 
too for all the help along the way. :)

Allison Henderson

> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6 v7] ext4: fix 1k block bugs

[Allison Henderson]

On 09/06/2011 09:38 AM, Allison Henderson wrote:
> On 09/05/2011 09:59 PM, Ted Ts'o wrote:
>> Allison,
>>
>> Many, many thanks for your work on this patch series!! It's clearly
>> been a very long slog.
>>
>> I've pulled in the first three patches, since I think those are
>> clearly correct at this point. One minor change I've made was I fixed
>> the spelling the flag, which I channged to
>> EXT4_DISCARD_PARTIAL_PG_ZERO_UNMAPPED. (Sorry, but seeing MAPED
>> everywhere was just grating on my nerves. :-)
>>
>> I didn't want to apply your 4th patch in the series since we're
>> planning on reducing the usage of i_mutex in the fs/ext4 code proper,
>> as recommended by Christoph. It wasn't immediately obvious to me
>> whether it was safe to apply any of the patches after #4, so for now
>> I've just merged in the first three, since they clearly fix real
>> problems that show up in xfstests.
>>
>> Can you comment on whether patches #5, #6, and #7 depend on #4?
>>
>> Thanks,
>>
>> - Ted
>
> Hi Ted,
>
> Patches 5 and 6 do not depend on 4, and this set only has 6 patches, so
> no need to worry about patch 7 :) So it is ok to just skip patch 4, I
> will see if I can find another way to solve the race conditions
> Yongqiang pointed out with out the use of i_mutex. Many thanks to you
> too for all the help along the way. :)
>
> Allison Henderson
>

Hi Ted,

I've been working on another way to resolve the punch hole races with 
out i_mutex, and Im starting to think that maybe we're going to need 
another semaphore some where.  Can I ask what the plans are for reducing 
the usage of i_mutex in ext4?  I wasnt sure if someone might already be 
doing this.  Thx!

Allison Henderson

>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html