diff mbox

[RESEND,2/2,v2] ext4: let us fully support punching hole feature in fallocate

Message ID 1358493381-20150-2-git-send-email-wenqing.lz@taobao.com
State Accepted, archived
Headers show

Commit Message

Zheng Liu Jan. 18, 2013, 7:16 a.m. UTC 
From: Zheng Liu <wenqing.lz@taobao.com>

After adding indirect punching hole feature, we need to enable it in fallocate.
For this purpose, some sanity checks need to be adjusted.  Currently we need to
check FALLOC_FL_PUNCH_HOLE flag before other sanity checks.

CC: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
---
 fs/ext4/extents.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Theodore Ts'o Jan. 25, 2013, 3:32 a.m. UTC | #1 
On Fri, Jan 18, 2013 at 03:16:21PM +0800, Zheng Liu wrote:
> From: Zheng Liu <wenqing.lz@taobao.com>
> 
> After adding indirect punching hole feature, we need to enable it in fallocate.
> For this purpose, some sanity checks need to be adjusted.  Currently we need to
> check FALLOC_FL_PUNCH_HOLE flag before other sanity checks.

I've folded these two patches into one since the first patch in this
series since there won't be any way to exercise the new code paths
until we enable this in fallocate(), and it doesn't really change
existing code paths.  If it did make huge changes in the normal code
path, it might be useful to keep the two commits separate, but that's
not the case here.

Anyway, they look good so I've checked them into my tree.  I'm
currently kicking off a test run to make sure there aren't any
problems except for the test 255 failure when testing w/o extents, but
I don't really anticipate any issues.

Thanks!!

							- Ted
--
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
Zheng Liu Jan. 25, 2013, 3:59 a.m. UTC | #2 
On Thu, Jan 24, 2013 at 10:32:01PM -0500, Theodore Ts'o wrote:
> On Fri, Jan 18, 2013 at 03:16:21PM +0800, Zheng Liu wrote:
> > From: Zheng Liu <wenqing.lz@taobao.com>
> > 
> > After adding indirect punching hole feature, we need to enable it in fallocate.
> > For this purpose, some sanity checks need to be adjusted.  Currently we need to
> > check FALLOC_FL_PUNCH_HOLE flag before other sanity checks.
> 
> I've folded these two patches into one since the first patch in this
> series since there won't be any way to exercise the new code paths
> until we enable this in fallocate(), and it doesn't really change
> existing code paths.  If it did make huge changes in the normal code
> path, it might be useful to keep the two commits separate, but that's
> not the case here.

Thanks for pointing out. :-)

> 
> Anyway, they look good so I've checked them into my tree.  I'm
> currently kicking off a test run to make sure there aren't any
> problems except for the test 255 failure when testing w/o extents, but
> I don't really anticipate any issues.

I wonder that maybe we need to submit a patch to let xfstest understand
that a filesystem supports extents or not because after applied this
patch indirect-based file in ext4 has supported seek_data/hole and hole
punching.  I usually run xfstest automatically, and every time I need
to check the result of #255 and #285 manually.  That is annoying for me.

Regards,
                                                - Zheng
--
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
Theodore Ts'o Jan. 25, 2013, 4:21 a.m. UTC | #3 
On Fri, Jan 25, 2013 at 11:59:12AM +0800, Zheng Liu wrote:
> 
> I wonder that maybe we need to submit a patch to let xfstest understand
> that a filesystem supports extents or not because after applied this
> patch indirect-based file in ext4 has supported seek_data/hole and hole
> punching.  I usually run xfstest automatically, and every time I need
> to check the result of #255 and #285 manually.  That is annoying for me.

I would think the right thing to do is to have xfstests make sure it
understands that fallocate working with FALLOC_FL_PUNCH_HOLE does not
imply that fallocate without the FALLOC_FL_PUNCH_HOLE flag OR'ed in
will work.  It should test for support for preallocation and hole
punching separately, and do tests accordingly.

That way we don't have to add explicit ext4 knowledge/logic to
xfstests.  (Maybe in the future there will be some other file system
which supports punch hole but not preallocate, and it might not be
based on whether or not the file is using ext4 extents or not.)

Cheers,

						- Ted
--
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
Dave Chinner Jan. 26, 2013, 12:47 a.m. UTC | #4 
[cc xfs@oss.sgi.com]

On Thu, Jan 24, 2013 at 11:21:22PM -0500, Theodore Ts'o wrote:
> On Fri, Jan 25, 2013 at 11:59:12AM +0800, Zheng Liu wrote:
> > 
> > I wonder that maybe we need to submit a patch to let xfstest understand
> > that a filesystem supports extents or not because after applied this
> > patch indirect-based file in ext4 has supported seek_data/hole and hole
> > punching.  I usually run xfstest automatically, and every time I need
> > to check the result of #255 and #285 manually.  That is annoying for me.
> 
> I would think the right thing to do is to have xfstests make sure it
> understands that fallocate working with FALLOC_FL_PUNCH_HOLE does not
> imply that fallocate without the FALLOC_FL_PUNCH_HOLE flag OR'ed in
> will work.

We already have this capabiity in xfstests via
_require_xfs_io_falloc_punch and _require_xfs_io_falloc. That,
however,  doesn't mean the tests that use these calls do the correct
requirement checks. That's the problem with 255 - it doesn't call
_require_xfs_io_falloc.

As to 285, the seek_sanity_test does it's own check for seek
hole/data support, and error out if it fails. This needs to be
turned into an equivalent _require_seek_hole_data (e.g. by running
"seek_sanity_test -t" to test for support) and 285 needs to the call
the _require_seek_hole_data before running the test proper.

Please send patches to xfs@oss.sgi.com....

Cheers,

Dave.
Theodore Ts'o Jan. 28, 2013, 4:57 a.m. UTC | #5 
On Mon, Jan 28, 2013 at 01:04:42PM +0800, Zheng Liu wrote:
> 
> Sorry, my apology.  I found a bug in indirect-based hole punching patch
> when I tried to create a new test case in xfstest.  A patch has been
> sent out.  Please look at it.

Ah yes, this must be the patch found at:

   http://patchwork.ozlabs.org/patch/216105/

It would it make sense for me to fold this into the punch hole patch,
yes?  (Since I haven't pushed out the patch yet, I might as well just
fold in the bugfix into the feature patch.)

     	    	   	    	    - Ted
--
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
Zheng Liu Jan. 28, 2013, 5:04 a.m. UTC | #6 
On Thu, Jan 24, 2013 at 10:32:01PM -0500, Theodore Ts'o wrote:
> On Fri, Jan 18, 2013 at 03:16:21PM +0800, Zheng Liu wrote:
> > From: Zheng Liu <wenqing.lz@taobao.com>
> > 
> > After adding indirect punching hole feature, we need to enable it in fallocate.
> > For this purpose, some sanity checks need to be adjusted.  Currently we need to
> > check FALLOC_FL_PUNCH_HOLE flag before other sanity checks.
> 
> I've folded these two patches into one since the first patch in this
> series since there won't be any way to exercise the new code paths
> until we enable this in fallocate(), and it doesn't really change
> existing code paths.  If it did make huge changes in the normal code
> path, it might be useful to keep the two commits separate, but that's
> not the case here.
> 
> Anyway, they look good so I've checked them into my tree.  I'm
> currently kicking off a test run to make sure there aren't any
> problems except for the test 255 failure when testing w/o extents, but
> I don't really anticipate any issues.

Hi Ted,

Sorry, my apology.  I found a bug in indirect-based hole punching patch
when I tried to create a new test case in xfstest.  A patch has been
sent out.  Please look at it.

Thanks,
                                                - Zheng
--
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
Zheng Liu Jan. 28, 2013, 5:16 a.m. UTC | #7 
On Sun, Jan 27, 2013 at 11:57:13PM -0500, Theodore Ts'o wrote:
> On Mon, Jan 28, 2013 at 01:04:42PM +0800, Zheng Liu wrote:
> > 
> > Sorry, my apology.  I found a bug in indirect-based hole punching patch
> > when I tried to create a new test case in xfstest.  A patch has been
> > sent out.  Please look at it.
> 
> Ah yes, this must be the patch found at:
> 
>    http://patchwork.ozlabs.org/patch/216105/


Yes, it is.

> 
> It would it make sense for me to fold this into the punch hole patch,
> yes?  (Since I haven't pushed out the patch yet, I might as well just
> fold in the bugfix into the feature patch.)

No problem.

Thanks,
                                                - Zheng
--
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
diff mbox

Patch

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 5ae1674..76643fd 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4397,13 +4397,6 @@  long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	struct ext4_map_blocks map;
 	unsigned int credits, blkbits = inode->i_blkbits;
 
-	/*
-	 * currently supporting (pre)allocate mode for extent-based
-	 * files _only_
-	 */
-	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
-		return -EOPNOTSUPP;
-
 	/* Return error if mode is not supported */
 	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
 		return -EOPNOTSUPP;
@@ -4415,6 +4408,13 @@  long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (ret)
 		return ret;
 
+	/*
+	 * currently supporting (pre)allocate mode for extent-based
+	 * files _only_
+	 */
+	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
+		return -EOPNOTSUPP;
+
 	trace_ext4_fallocate_enter(inode, offset, len, mode);
 	map.m_lblk = offset >> blkbits;
 	/*