xfstests failures with xfs, dax and v4.4-rc3
diff mbox

Message ID 20151202204502.GI19199@dastard
State New
Headers show

Commit Message

Dave Chinner Dec. 2, 2015, 8:45 p.m. UTC
On Thu, Dec 03, 2015 at 07:29:10AM +1100, Dave Chinner wrote:
> On Wed, Dec 02, 2015 at 11:34:38AM -0700, Ross Zwisler wrote:
> > I'm hitting a few more test failures in my testing setup with v4.4-rc3, xfs
> > and DAX.  My test setup is a pair of 4GiB PMEM partitions in a KVM virtual
> > machine.  Here are the failures:
> 
> Which are caused by commit 1ca1915 ("xfs: Don't use unwritten extents
> for DAX") because of this code for unwritten extent conversion in
> get_blocks:
> 
> 	tp->t_flags |= XFS_TRANS_RESERVE;
> 
> It's a minor problem compared to all the other issues DAX has right
> now, so I ignored it to get the bigger problem solved first.

Patch to fix the problem below.

-Dave.

Comments

Ross Zwisler Dec. 2, 2015, 9:39 p.m. UTC | #1
On Thu, Dec 03, 2015 at 07:45:02AM +1100, Dave Chinner wrote:
> On Thu, Dec 03, 2015 at 07:29:10AM +1100, Dave Chinner wrote:
> > On Wed, Dec 02, 2015 at 11:34:38AM -0700, Ross Zwisler wrote:
> > > I'm hitting a few more test failures in my testing setup with v4.4-rc3, xfs
> > > and DAX.  My test setup is a pair of 4GiB PMEM partitions in a KVM virtual
> > > machine.  Here are the failures:
> > 
> > Which are caused by commit 1ca1915 ("xfs: Don't use unwritten extents
> > for DAX") because of this code for unwritten extent conversion in
> > get_blocks:
> > 
> > 	tp->t_flags |= XFS_TRANS_RESERVE;
> > 
> > It's a minor problem compared to all the other issues DAX has right
> > now, so I ignored it to get the bigger problem solved first.
> 
> Patch to fix the problem below.
> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> xfs: Don't use reserved blocks for data blocks with DAX
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> Commit 1ca1915 ("xfs: Don't use unwritten extents for DAX") enabled
> the DAX allocation call to dip into the reserve pool in case it was
> converting unwritten extents rather than allocating blocks. This was
> a direct copy of the unwritten extent conversion code, but had an
> unintended side effect of allowing normal data block allocation to
> use the reserve pool. Hence normal block allocation could deplete
> the reserve pool and prevent unwritten extent conversion at ENOSPC,
> hence violating fallocate guarantees on preallocated space.
> 
> Fix it by checking whether the incoming map from __xfs_get_blocks()
> spans an unwritten extent and only use the reserve pool if the
> allocation covers an unwritten extent.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>

I've verified that this fixes all three failing xfstests reported in this mail.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ross Zwisler Dec. 10, 2015, 4:54 p.m. UTC | #2
On Wed, Dec 02, 2015 at 02:39:32PM -0700, Ross Zwisler wrote:
> On Thu, Dec 03, 2015 at 07:45:02AM +1100, Dave Chinner wrote:
> > On Thu, Dec 03, 2015 at 07:29:10AM +1100, Dave Chinner wrote:
> > > On Wed, Dec 02, 2015 at 11:34:38AM -0700, Ross Zwisler wrote:
> > > > I'm hitting a few more test failures in my testing setup with v4.4-rc3, xfs
> > > > and DAX.  My test setup is a pair of 4GiB PMEM partitions in a KVM virtual
> > > > machine.  Here are the failures:
> > > 
> > > Which are caused by commit 1ca1915 ("xfs: Don't use unwritten extents
> > > for DAX") because of this code for unwritten extent conversion in
> > > get_blocks:
> > > 
> > > 	tp->t_flags |= XFS_TRANS_RESERVE;
> > > 
> > > It's a minor problem compared to all the other issues DAX has right
> > > now, so I ignored it to get the bigger problem solved first.
> > 
> > Patch to fix the problem below.
> > 
> > -Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 
> > xfs: Don't use reserved blocks for data blocks with DAX
> > 
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Commit 1ca1915 ("xfs: Don't use unwritten extents for DAX") enabled
> > the DAX allocation call to dip into the reserve pool in case it was
> > converting unwritten extents rather than allocating blocks. This was
> > a direct copy of the unwritten extent conversion code, but had an
> > unintended side effect of allowing normal data block allocation to
> > use the reserve pool. Hence normal block allocation could deplete
> > the reserve pool and prevent unwritten extent conversion at ENOSPC,
> > hence violating fallocate guarantees on preallocated space.
> > 
> > Fix it by checking whether the incoming map from __xfs_get_blocks()
> > spans an unwritten extent and only use the reserve pool if the
> > allocation covers an unwritten extent.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> 
> I've verified that this fixes all three failing xfstests reported in this mail.
> Thanks!

Hey Dave,

Are you planning on pushing this fix for v4.4?

- Ross
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Chinner Dec. 10, 2015, 10:33 p.m. UTC | #3
On Thu, Dec 10, 2015 at 09:54:58AM -0700, Ross Zwisler wrote:
> On Wed, Dec 02, 2015 at 02:39:32PM -0700, Ross Zwisler wrote:
> > On Thu, Dec 03, 2015 at 07:45:02AM +1100, Dave Chinner wrote:
> > > On Thu, Dec 03, 2015 at 07:29:10AM +1100, Dave Chinner wrote:
> > > > On Wed, Dec 02, 2015 at 11:34:38AM -0700, Ross Zwisler wrote:
> > > > > I'm hitting a few more test failures in my testing setup with v4.4-rc3, xfs
> > > > > and DAX.  My test setup is a pair of 4GiB PMEM partitions in a KVM virtual
> > > > > machine.  Here are the failures:
> > > > 
> > > > Which are caused by commit 1ca1915 ("xfs: Don't use unwritten extents
> > > > for DAX") because of this code for unwritten extent conversion in
> > > > get_blocks:
> > > > 
> > > > 	tp->t_flags |= XFS_TRANS_RESERVE;
> > > > 
> > > > It's a minor problem compared to all the other issues DAX has right
> > > > now, so I ignored it to get the bigger problem solved first.
> > > 
> > > Patch to fix the problem below.
> > > 
> > > -Dave.
> > > -- 
> > > Dave Chinner
> > > david@fromorbit.com
> > > 
> > > xfs: Don't use reserved blocks for data blocks with DAX
> > > 
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Commit 1ca1915 ("xfs: Don't use unwritten extents for DAX") enabled
> > > the DAX allocation call to dip into the reserve pool in case it was
> > > converting unwritten extents rather than allocating blocks. This was
> > > a direct copy of the unwritten extent conversion code, but had an
> > > unintended side effect of allowing normal data block allocation to
> > > use the reserve pool. Hence normal block allocation could deplete
> > > the reserve pool and prevent unwritten extent conversion at ENOSPC,
> > > hence violating fallocate guarantees on preallocated space.
> > > 
> > > Fix it by checking whether the incoming map from __xfs_get_blocks()
> > > spans an unwritten extent and only use the reserve pool if the
> > > allocation covers an unwritten extent.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > 
> > Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > 
> > I've verified that this fixes all three failing xfstests reported in this mail.
> > Thanks!
> 
> Hey Dave,
> 
> Are you planning on pushing this fix for v4.4?

No plans to right now - ENOSPC is a corner case that most users
won't be anywhere near, especially for experimental functionality on
hardware nobody actually has....

Cheers,

Dave.
Ross Zwisler Dec. 11, 2015, 4:26 a.m. UTC | #4
On Fri, Dec 11, 2015 at 09:33:33AM +1100, Dave Chinner wrote:
> On Thu, Dec 10, 2015 at 09:54:58AM -0700, Ross Zwisler wrote:
> > On Wed, Dec 02, 2015 at 02:39:32PM -0700, Ross Zwisler wrote:
> > > I've verified that this fixes all three failing xfstests reported in this mail.
> > > Thanks!
> > 
> > Hey Dave,
> > 
> > Are you planning on pushing this fix for v4.4?
> 
> No plans to right now - ENOSPC is a corner case that most users
> won't be anywhere near, especially for experimental functionality on
> hardware nobody actually has....

Really?  I realize that it may be a case that most users won't actually hit,
but it is a 5 line change that fixes four xfstests regressions between v4.3 and
v4.4 for my DAX testing...

Is there a strong reason *not* to push it in the v4.4 cycle?  I'm trying to
clear up all xfstests differences between DAX and non-DAX, and this would help
quite a bit.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Chinner Dec. 14, 2015, 12:20 a.m. UTC | #5
On Thu, Dec 10, 2015 at 09:26:09PM -0700, Ross Zwisler wrote:
> On Fri, Dec 11, 2015 at 09:33:33AM +1100, Dave Chinner wrote:
> > On Thu, Dec 10, 2015 at 09:54:58AM -0700, Ross Zwisler wrote:
> > > On Wed, Dec 02, 2015 at 02:39:32PM -0700, Ross Zwisler wrote:
> > > > I've verified that this fixes all three failing xfstests reported in this mail.
> > > > Thanks!
> > > 
> > > Hey Dave,
> > > 
> > > Are you planning on pushing this fix for v4.4?
> > 
> > No plans to right now - ENOSPC is a corner case that most users
> > won't be anywhere near, especially for experimental functionality on
> > hardware nobody actually has....
> 
> Really?  I realize that it may be a case that most users won't actually hit,
> but it is a 5 line change that fixes four xfstests regressions between v4.3 and
> v4.4 for my DAX testing...
> 
> Is there a strong reason *not* to push it in the v4.4 cycle?  I'm trying to
> clear up all xfstests differences between DAX and non-DAX, and this would help
> quite a bit.

Against my better judgement, I committed a largely untested,
fundamental change to allocation policy for DAX-on-XFS *inside the
4.4 merge window* justifying it as "it's experimental code" and that
"we needed to get it out there".  IOWs, I've already stretched the
rules and committed stuff I knew wasn't ready or worked 100%
correctly just to keep you guys happy, but I'm not going to continue
to do so. I'm reverting to usual policy of "soak in for-next, use
"cc: stable" tags to get it backported when upstreamed in the next
merge window.

Indeed, from my point of view it's pretty clear that lots of recent
DAX code has been committed prematurely and without sufficient
review and/or testing. This has lead to having to revert chunks of
code and completely rework algorithms, I really don't care if you're
being pushed to "make DAX shit happen fast" - my only concern here
is *don't screw up production filesystems*.

Hence when it comes to making fundamental changes to allocation
behaviour, I've already bent the policy/rules as far as I'm willing
to.  Changes in this area carry an inherent risk of breaking stuff
unrelated to DAX and that is far more important right now than
whether DAX (and experimental feature) works correctly or not.

Cheers,

Dave.

Patch
diff mbox

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index f4f5b43..9ed146b 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -203,15 +203,20 @@  xfs_iomap_write_direct(
 	 * this outside the transaction context, but if we commit and then crash
 	 * we may not have zeroed the blocks and this will be exposed on
 	 * recovery of the allocation. Hence we must zero before commit.
+	 *
 	 * Further, if we are mapping unwritten extents here, we need to zero
 	 * and convert them to written so that we don't need an unwritten extent
 	 * callback for DAX. This also means that we need to be able to dip into
-	 * the reserve block pool if there is no space left but we need to do
-	 * unwritten extent conversion.
+	 * the reserve block pool for bmbt block allocation if there is no space
+	 * left but we need to do unwritten extent conversion.
 	 */
+
 	if (IS_DAX(VFS_I(ip))) {
 		bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
-		tp->t_flags |= XFS_TRANS_RESERVE;
+		if (ISUNWRITTEN(imap)) {
+			tp->t_flags |= XFS_TRANS_RESERVE;
+			resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0) << 1;
+		}
 	}
 	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_write,
 				  resblks, resrtextents);