xfs: upfront block zeroing for DAX

From: Dave Chinner <[email protected]>

Both direct IO and DAX pass an offset and count into get_blocks that will overflow a s64 variable when an IO goes into the last supported block in a file (i.e. at offset 2^63 - 1FSB bytes). This can be seen from the tracing:

xfs_get_blocks_alloc: [...] offset 0x7ffffffffffff000 count 4096
xfs_gbmap_direct:     [...] offset 0x7ffffffffffff000 count 4096

xfs_gbmap_direct_none:[...] offset 0x7ffffffffffff000 count 4096

0x7ffffffffffff000 + 4096 = 0x8000000000000000, and hence that overflows the s64 offset and we fail to detect the need for a filesize update and an ioend is not allocated.

This is *mostly* avoided for direct IO because such extending IOs occur with full block allocation, and so the "IS_UNWRITTEN()" check still evaluates as true and we get an ioend that way. However, doing single sector extending IOs to this last block will expose the fact that file size updates will not occur after the first allocating direct IO as the overflow will then be exposed.

There is one further complexity: the DAX page fault path also exposes the same issue in block allocation. However, page faults cannot extend the file size, so in this case we want to allocate the block but do not want to allocate an ioend to enable file size update at IO completion. Hence we now need to distinguish between the direct IO patch allocation and dax fault path allocation to avoid leaking ioend structures.

Signed-off-by Dave Chinner <[email protected]>

---

 fs/xfs/xfs_aops.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
 fs/xfs/xfs_aops.h |  2 ++
 fs/xfs/xfs_file.c |  6 +++---
 3 files changed, 49 insertions(+), 9 deletions(-)

fs/xfs/xfs_aops.c +44 -6

index e4fff58..366e41eb 100644
@@ -1259,13 +1259,28 @@ xfs_vm_releasepage(
  * the DIO. There is only going to be one reference to the ioend and its life
  * cycle is constrained by the DIO completion code. hence we don't need
  * reference counting here.
+ *
+ * Note that for DIO, an IO to the highest supported file block offset (i.e.
+ * 2^63 - 1FSB bytes) will result in the offset + count overflowing a signed 64
+ * bit variable. Hence if we see this overflow, we have to assume that the IO is
+ * extending the file size. We won't know for sure until IO completion is run
+ * and the actual max write offset is communicated to the IO completion
+ * routine.
+ *
+ * For DAX page faults, we are preparing to never see unwritten extents here,
+ * nor should we ever extend the inode size. Hence we will soon have nothing to
+ * do here for this case, ensuring we don't have to provide an IO completion
+ * callback to free an ioend that we don't actually need for a fault into the
+ * page at offset (2^63 - 1FSB) bytes.
  */
+
 static void
 xfs_map_direct(
 	struct inode		*inode,
 	struct buffer_head	*bh_result,
 	struct xfs_bmbt_irec	*imap,
-	xfs_off_t		offset)
+	xfs_off_t		offset,
+	bool			dax_fault)
 {
 	struct xfs_ioend	*ioend;
 	xfs_off_t		size = bh_result->b_size;

@@ -1278,6 +1293,16 @@ xfs_map_direct(
 
 	trace_xfs_gbmap_direct(XFS_I(inode), offset, size, type, imap);
 
+	/* XXX: preparation for removing unwritten extents in DAX */
+#if 0
+	if (dax_fault) {
+		ASSERT(type == XFS_IO_OVERWRITE);
+		trace_xfs_gbmap_direct_none(XFS_I(inode), offset, size, type,
+					    imap);
+		return;
+	}
+#endif
+
 	if (bh_result->b_private) {
 		ioend = bh_result->b_private;
 		ASSERT(ioend->io_size > 0);

@@ -1292,7 +1317,8 @@ xfs_map_direct(
 					      ioend->io_size, ioend->io_type,
 					      imap);
 	} else if (type == XFS_IO_UNWRITTEN ||
-		   offset + size > i_size_read(inode)) {
+		   offset + size > i_size_read(inode) ||
+		   offset + size < 0) {
 		ioend = xfs_alloc_ioend(inode, type);
 		ioend->io_offset = offset;
 		ioend->io_size = size;

@@ -1354,7 +1380,8 @@ __xfs_get_blocks(
 	sector_t		iblock,
 	struct buffer_head	*bh_result,
 	int			create,
-	bool			direct)
+	bool			direct,
+	bool			dax_fault)
 {
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;

@@ -1467,7 +1494,8 @@ __xfs_get_blocks(
 			set_buffer_unwritten(bh_result);
 		/* direct IO needs special help */
 		if (create && direct)
-			xfs_map_direct(inode, bh_result, &imap, offset);
+			xfs_map_direct(inode, bh_result, &imap, offset,
+				       dax_fault);
 	}
 
 	/*

@@ -1514,7 +1542,7 @@ xfs_get_blocks(
 	struct buffer_head	*bh_result,
 	int			create)
 {
-	return __xfs_get_blocks(inode, iblock, bh_result, create, false);
+	return __xfs_get_blocks(inode, iblock, bh_result, create, false, false);
 }
 
 int

@@ -1524,7 +1552,17 @@ xfs_get_blocks_direct(
 	struct buffer_head	*bh_result,
 	int			create)
 {
-	return __xfs_get_blocks(inode, iblock, bh_result, create, true);
+	return __xfs_get_blocks(inode, iblock, bh_result, create, true, false);
+}
+
+int
+xfs_get_blocks_dax_fault(
+	struct inode		*inode,
+	sector_t		iblock,
+	struct buffer_head	*bh_result,
+	int			create)
+{
+	return __xfs_get_blocks(inode, iblock, bh_result, create, true, true);
 }
 
 static void

fs/xfs/xfs_aops.h +2 -0

index 86afd1a..d39ba25 100644
@@ -58,6 +58,8 @@ int	xfs_get_blocks(struct inode *inode, sector_t offset,
 		       struct buffer_head *map_bh, int create);
 int	xfs_get_blocks_direct(struct inode *inode, sector_t offset,
 			      struct buffer_head *map_bh, int create);
+int	xfs_get_blocks_dax_fault(struct inode *inode, sector_t offset,
+			         struct buffer_head *map_bh, int create);
 void	xfs_end_io_dax_write(struct buffer_head *bh, int uptodate);
 
 extern void xfs_count_page_state(struct page *, int *, int *);

fs/xfs/xfs_file.c +3 -3

index 2f7b6bd..7f873bc 100644
@@ -1508,7 +1508,7 @@ xfs_filemap_page_mkwrite(
 	xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
 
 	if (IS_DAX(inode)) {
-		ret = __dax_mkwrite(vma, vmf, xfs_get_blocks_direct,
+		ret = __dax_mkwrite(vma, vmf, xfs_get_blocks_dax_fault,
 				    xfs_end_io_dax_write);
 	} else {
 		ret = __block_page_mkwrite(vma, vmf, xfs_get_blocks);

@@ -1543,7 +1543,7 @@ xfs_filemap_fault(
 		 * changes to xfs_get_blocks_direct() to map unwritten extent
 		 * ioend for conversion on read-only mappings.
 		 */
-		ret = __dax_fault(vma, vmf, xfs_get_blocks_direct, NULL);
+		ret = __dax_fault(vma, vmf, xfs_get_blocks_dax_fault, NULL);
 	} else
 		ret = filemap_fault(vma, vmf);
 	xfs_iunlock(XFS_I(inode), XFS_MMAPLOCK_SHARED);

@@ -1570,7 +1570,7 @@ xfs_filemap_pmd_fault(
 	sb_start_pagefault(inode->i_sb);
 	file_update_time(vma->vm_file);
 	xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
-	ret = __dax_pmd_fault(vma, addr, pmd, flags, xfs_get_blocks_direct,
+	ret = __dax_pmd_fault(vma, addr, pmd, flags, xfs_get_blocks_dax_fault,
 				    xfs_end_io_dax_write);
 	xfs_iunlock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
 	sb_end_pagefault(inode->i_sb);

Signature · 6 lines

2.5.0

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

From: Dave Chinner <[email protected]>

To enable DAX to do atomic allocation of zeroed extents, we need to drive the block zeroing deep into the allocator. Because xfs_bmapi_write() can return merged extents on allocation that were only partially allocated (i.e. requested range spans allocated and hole regions, allocation into the hole was contiguous), we cannot zero the extent returned from xfs_bmapi_write() as that can overwrite existing data with zeros.

Hence we have to drive the extent zeroing into the allocation code, prior to where we merge the extents into the BMBT and return the resultant map. This means we need to propagate this need down to the xfs_alloc_vextent() and issue the block zeroing at this point.

While this functionality is being introduced for DAX, there is no reason why it is specific to DAX - we can per-zero blocks during the allocation transaction on any type of device. It's just slow (and usually slower than unwritten allocation and conversion) on traditional block devices so doesn't tend to get used. We can, however, hook hardware zeroing optimisations via sb_issue_zeroout() to this operation, so it may be useful in future and hence the "allocate zeroed blocks" API needs to be implementation neutral.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/libxfs/xfs_alloc.c | 10 +++++++++-
 fs/xfs/libxfs/xfs_alloc.h |  8 +++++---
 fs/xfs/libxfs/xfs_bmap.c  | 25 +++++++++++++++++++++++--
 fs/xfs/libxfs/xfs_bmap.h  | 13 +++++++++++--
 fs/xfs/xfs_bmap_util.c    | 36 ++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_mount.h        |  3 +++
 6 files changed, 87 insertions(+), 8 deletions(-)

fs/xfs/libxfs/xfs_alloc.c +1 -1

index e926197..3479294 100644
@@ -2509,7 +2509,7 @@ xfs_alloc_vextent(
 		 * Try near allocation first, then anywhere-in-ag after
 		 * the first a.g. fails.
 		 */
-		if ((args->userdata  == XFS_ALLOC_INITIAL_USER_DATA) &&
+		if ((args->userdata & XFS_ALLOC_INITIAL_USER_DATA) &&
 		    (mp->m_flags & XFS_MOUNT_32BITINODES)) {
 			args->fsbno = XFS_AGB_TO_FSB(mp,
 					((mp->m_agfrotor / rotorstep) %

@@ -2640,6 +2640,14 @@ xfs_alloc_vextent(
 		XFS_AG_CHECK_DADDR(mp, XFS_FSB_TO_DADDR(mp, args->fsbno),

fs/xfs/libxfs/xfs_alloc.c +0 -0

 	}
 	xfs_perag_put(args->pag);
 	return 0;

fs/xfs/libxfs/xfs_alloc.h +5 -3

index ca1c816..0ecde4d 100644
@@ -101,6 +101,7 @@ typedef struct xfs_alloc_arg {
 	struct xfs_mount *mp;		/* file system mount point */
 	struct xfs_buf	*agbp;		/* buffer for a.g. freelist header */
 	struct xfs_perag *pag;		/* per-ag struct for this agno */
+	struct xfs_inode *ip;		/* for userdata zeroing method */
 	xfs_fsblock_t	fsbno;		/* file system block number */
 	xfs_agnumber_t	agno;		/* allocation group number */
 	xfs_agblock_t	agbno;		/* allocation group-relative block # */

@@ -120,15 +121,16 @@ typedef struct xfs_alloc_arg {
 	char		wasdel;		/* set if allocation was prev delayed */
 	char		wasfromfl;	/* set if allocation is from freelist */
 	char		isfl;		/* set if is freelist blocks - !acctg */
-	char		userdata;	/* set if this is user data */
+	char		userdata;	/* mask defining userdata treatment */
 	xfs_fsblock_t	firstblock;	/* io first block allocated */
 } xfs_alloc_arg_t;
 
 /*
  * Defines for userdata
  */
-#define XFS_ALLOC_USERDATA		1	/* allocation is for user data*/
-#define XFS_ALLOC_INITIAL_USER_DATA	2	/* special case start of file */
+#define XFS_ALLOC_USERDATA		(1 << 0)/* allocation is for user data*/
+#define XFS_ALLOC_INITIAL_USER_DATA	(1 << 1)/* special case start of file */
+#define XFS_ALLOC_USERDATA_ZERO		(1 << 2)/* zero extent on allocation */
 
 xfs_extlen_t xfs_alloc_longest_free_extent(struct xfs_mount *mp,
 		struct xfs_perag *pag, xfs_extlen_t need);

fs/xfs/libxfs/xfs_bmap.c +11 -2

index ab92d10..590cbec 100644
@@ -3802,8 +3802,13 @@ xfs_bmap_btalloc(
 	args.wasdel = ap->wasdel;
 	args.isfl = 0;
 	args.userdata = ap->userdata;
-	if ((error = xfs_alloc_vextent(&args)))
+	if (ap->userdata & XFS_ALLOC_USERDATA_ZERO)
+		args.ip = ap->ip;
+
+	error = xfs_alloc_vextent(&args);
+	if (error)
 		return error;
+
 	if (tryagain && args.fsbno == NULLFSBLOCK) {
 		/*
 		 * Exact allocation failed. Now try with alignment

@@ -4302,11 +4307,14 @@ xfs_bmapi_allocate(
 
 	/*
 	 * Indicate if this is the first user data in the file, or just any
-	 * user data.
+	 * user data. And if it is userdata, indicate whether it needs to
+	 * be initialised to zero during allocation.
 	 */
 	if (!(bma->flags & XFS_BMAPI_METADATA)) {
 		bma->userdata = (bma->offset == 0) ?
 			XFS_ALLOC_INITIAL_USER_DATA : XFS_ALLOC_USERDATA;
+		if (bma->flags & XFS_BMAPI_ZERO)
+			bma->userdata |= XFS_ALLOC_USERDATA_ZERO;
 	}
 
 	bma->minlen = (bma->flags & XFS_BMAPI_CONTIG) ? bma->length : 1;

@@ -4421,6 +4429,17 @@ xfs_bmapi_convert_unwritten(
 	mval->br_state = (mval->br_state == XFS_EXT_UNWRITTEN)
 				? XFS_EXT_NORM : XFS_EXT_UNWRITTEN;
 
+	/*

fs/xfs/libxfs/xfs_bmap.c +2 -0

 	error = xfs_bmap_add_extent_unwritten_real(bma->tp, bma->ip, &bma->idx,
 			&bma->cur, mval, bma->firstblock, bma->flist,
 			&tmp_logflags);

@@ -4513,6 +4532,8 @@ xfs_bmapi_write(
 	ASSERT(len > 0);
 	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT((flags & (XFS_BMAPI_METADATA | XFS_BMAPI_ZERO)) !=
+			(XFS_BMAPI_METADATA | XFS_BMAPI_ZERO));
 
 	if (unlikely(XFS_TEST_ERROR(
 	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&

fs/xfs/libxfs/xfs_bmap.h +11 -2

index 6aaa0c1..a160f8a 100644
@@ -52,9 +52,9 @@ struct xfs_bmalloca {
 	xfs_extlen_t		minleft; /* amount must be left after alloc */
 	bool			eof;	/* set if allocating past last extent */
 	bool			wasdel;	/* replacing a delayed allocation */
-	bool			userdata;/* set if is user data */
 	bool			aeof;	/* allocated space at eof */
 	bool			conv;	/* overwriting unwritten extents */
+	char			userdata;/* userdata mask */
 	int			flags;
 };
 

@@ -109,6 +109,14 @@ typedef	struct xfs_bmap_free
  */
 #define XFS_BMAPI_CONVERT	0x040
 
+/*
+ * allocate zeroed extents - this requires all newly allocated user data extents
+ * to be initialised to zero. It will be ignored if XFS_BMAPI_METADATA is set.
+ * Use in conjunction with XFS_BMAPI_CONVERT to convert unwritten extents found
+ * during the allocation range to zeroed written extents.
+ */
+#define XFS_BMAPI_ZERO		0x080
+
 #define XFS_BMAPI_FLAGS \
 	{ XFS_BMAPI_ENTIRE,	"ENTIRE" }, \
 	{ XFS_BMAPI_METADATA,	"METADATA" }, \

@@ -116,7 +124,8 @@ typedef	struct xfs_bmap_free
 	{ XFS_BMAPI_PREALLOC,	"PREALLOC" }, \
 	{ XFS_BMAPI_IGSTATE,	"IGSTATE" }, \
 	{ XFS_BMAPI_CONTIG,	"CONTIG" }, \
-	{ XFS_BMAPI_CONVERT,	"CONVERT" }
+	{ XFS_BMAPI_CONVERT,	"CONVERT" }, \
+	{ XFS_BMAPI_ZERO,	"ZERO" }
 
 
 static inline int xfs_bmapi_aflag(int w)

fs/xfs/xfs_bmap_util.c +15 -0

index eca325e..dbae649 100644
@@ -57,6 +57,35 @@ xfs_fsb_to_db(struct xfs_inode *ip, xfs_fsblock_t fsb)
 }
 
 /*
+ * Routine to zero an extent on disk allocated to the specific inode.
+ *
+ * The VFS functions take a linearised filesystem block offset, so we have to
+ * convert the sparse xfs fsb to the right format first.
+ * VFS types are real funky, too.
+ */
+int
+xfs_zero_extent(
+	struct xfs_inode *ip,
+	xfs_fsblock_t	start_fsb,
+	xfs_off_t	count_fsb)
+{
+	struct xfs_mount *mp = ip->i_mount;
+	xfs_daddr_t	sector = xfs_fsb_to_db(ip, start_fsb);
+	sector_t	block = XFS_BB_TO_FSBT(mp, sector);

fs/xfs/xfs_bmap_util.c +11 -0

+
+}
+
+/*
  * Routine to be called at transaction's end by xfs_bmapi, xfs_bunmapi
  * caller.  Frees all the extents that need freeing, which must be done
  * last due to locking considerations.  We never free any extents in

@@ -229,6 +258,13 @@ xfs_bmap_rtalloc(
 		xfs_trans_mod_dquot_byino(ap->tp, ap->ip,
 			ap->wasdel ? XFS_TRANS_DQ_DELRTBCOUNT :
 					XFS_TRANS_DQ_RTBCOUNT, (long) ralen);
+
+		/* Zero the extent if we were asked to do so */
+		if (ap->userdata & XFS_ALLOC_USERDATA_ZERO) {
+			error = xfs_zero_extent(ap->ip, ap->blkno, ap->length);
+			if (error)
+				return error;
+		}
 	} else {
 		ap->length = 0;
 	}

fs/xfs/xfs_mount.h +3 -0

index 8795272..f20e5de 100644
@@ -337,4 +337,7 @@ extern int	xfs_dev_is_read_only(struct xfs_mount *, char *);
 
 extern void	xfs_set_low_space_thresholds(struct xfs_mount *);
 
+int	xfs_zero_extent(struct xfs_inode *ip, xfs_fsblock_t start_fsb,
+			xfs_off_t count_fsb);
+
 #endif	/* __XFS_MOUNT_H__ */

Signature · 6 lines

2.5.0

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 25 lines · Dave Chinner

From: Dave Chinner <[email protected]>

To enable DAX to do atomic allocation of zeroed extents, we need to drive the block zeroing deep into the allocator. Because xfs_bmapi_write() can return merged extents on allocation that were only partially allocated (i.e. requested range spans allocated and hole regions, allocation into the hole was contiguous), we cannot zero the extent returned from xfs_bmapi_write() as that can overwrite existing data with zeros.

Hence we have to drive the extent zeroing into the allocation code, prior to where we merge the extents into the BMBT and return the resultant map. This means we need to propagate this need down to the xfs_alloc_vextent() and issue the block zeroing at this point.

While this functionality is being introduced for DAX, there is no reason why it is specific to DAX - we can per-zero blocks during the allocation transaction on any type of device. It's just slow (and usually slower than unwritten allocation and conversion) on traditional block devices so doesn't tend to get used. We can, however, hook hardware zeroing optimisations via sb_issue_zeroout() to this operation, so it may be useful in future and hence the "allocate zeroed blocks" API needs to be implementation neutral.

Signed-off-by Dave Chinner <[email protected]>

fs/xfs/libxfs/xfs_alloc.c xfs_alloc_vextent( · line 2650

Quoted 9 lines

#endif

fs/xfs/libxfs/xfs_alloc.c +8 -0

+
+		/* Zero the extent if we were asked to do so */
+		if (args->userdata & XFS_ALLOC_USERDATA_ZERO) {
+			error = xfs_zero_extent(args->ip, args->fsbno, args->len);
+			if (error)
+				goto error0;
+		}
+

Ok, so we wire up zeroing to the actual block allocation here...

Quoted 4 lines

fs/xfs/libxfs/xfs_bmap.c +0 -0

@@ -4421,6 +4429,17 @@ xfs_bmapi_convert_unwritten(
 	mval->br_state = (mval->br_state == XFS_EXT_UNWRITTEN)
 				? XFS_EXT_NORM : XFS_EXT_UNWRITTEN;
 

fs/xfs/libxfs/xfs_bmap.c xfs_bmapi_convert_unwritten( · line 4442

Quoted 11 lines

fs/xfs/libxfs/xfs_bmap.c +11 -0

+	/*
+	 * Before insertion into the bmbt, zero the range being converted
+	 * if required.
+	 */
+	if (flags & XFS_BMAPI_ZERO) {
+		error = xfs_zero_extent(bma->ip, mval->br_startblock,
+					mval->br_blockcount);
+		if (error)
+			return error;
+	}
+

... and also zero the extent on unwritten conversion, if necessary.

I suspect there's no use case to pass XFS_BMAPI_ZERO for new unwritten allocations, but if the flag is passed, doesn't this cause duplicate block zeroing?

Perhaps we should drop the zero flag from 'flags' after allocation in xfs_bmapi_write() just to ensure this executes in one place or the other..?

Quoted 7 lines

fs/xfs/xfs_bmap_util.c +7 -0

+	xfs_daddr_t	sector = xfs_fsb_to_db(ip, start_fsb);
+	sector_t	block = XFS_BB_TO_FSBT(mp, sector);
+	ssize_t		size = XFS_FSB_TO_B(mp, count_fsb);
+
+	if (IS_DAX(VFS_I(ip)))
+		return dax_clear_blocks(VFS_I(ip), block, size);
+

fs/xfs/xfs_bmap_util.c xfs_fsb_to_db(struct xfs_inode *ip, xfs_fsblock_t fsb) · line 84

Quoted 5 lines

fs/xfs/xfs_bmap_util.c +5 -0

+	/*
+	 * let the block layer decide on the fastest method of
+	 * implementing the zeroing.
+	 */
+	return sb_issue_zeroout(mp->m_super, block, count_fsb, GFP_NOFS);

The count param to sb_issue_zeroout() is a sector_t and we're passing an FSB.

Quoted 3 lines · Brian Foster

Quoted 2 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Quoted 4 lines

I suspect there's no use case to pass XFS_BMAPI_ZERO for new unwritten allocations, but if the flag is passed, doesn't this cause duplicate block zeroing?

It probably would, but....

Quoted 3 lines

Perhaps we should drop the zero flag from 'flags' after allocation in xfs_bmapi_write() just to ensure this executes in one place or the other..?

I think that if we hit this, we're doing something else wrong - why would we allocate unwritten extents and still need to initialise them to zero?

Quoted 7 lines

Quoted 7 lines

fs/xfs/xfs_bmap_util.c +7 -0

+	xfs_daddr_t	sector = xfs_fsb_to_db(ip, start_fsb);
+	sector_t	block = XFS_BB_TO_FSBT(mp, sector);
+	ssize_t		size = XFS_FSB_TO_B(mp, count_fsb);
+
+	if (IS_DAX(VFS_I(ip)))
+		return dax_clear_blocks(VFS_I(ip), block, size);
+

Quoted 8 lines

fs/xfs/xfs_bmap_util.c xfs_fsb_to_db(struct xfs_inode *ip, xfs_fsblock_t fsb) · line 84

Quoted 5 lines

fs/xfs/xfs_bmap_util.c +5 -0

+	/*
+	 * let the block layer decide on the fastest method of
+	 * implementing the zeroing.
+	 */
+	return sb_issue_zeroout(mp->m_super, block, count_fsb, GFP_NOFS);

The count param to sb_issue_zeroout() is a sector_t and we're passing an FSB.

"sector_t" does not mean the function takes parameters in units of sectors. It's the only variable that you can guarantee will be sized correctly to the kernel's configured block device capacity support. Indeed:

static inline int sb_issue_zeroout(struct super_block *sb, sector_t block,
sector_t nr_blocks, gfp_t gfp_mask)

{
        return blkdev_issue_zeroout(sb->s_bdev,
                                    block << (sb->s_blocksize_bits - 9),
                                    nr_blocks << (sb->s_blocksize_bits - 9),
                                    gfp_mask, true);
}

sb_issue_zeroout() takes a block device offset and length in filesystem block size units and converts them back to sectors to pass it to the block layer.

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 4 lines · Dave Chinner

Quoted 3 lines · Brian Foster

Quoted 2 lines · Dave Chinner

From: Dave Chinner <[email protected]>

...

Quoted 7 lines

Quoted 4 lines

I suspect there's no use case to pass XFS_BMAPI_ZERO for new unwritten allocations, but if the flag is passed, doesn't this cause duplicate block zeroing?

It probably would, but....

Quoted 8 lines

Quoted 3 lines

Perhaps we should drop the zero flag from 'flags' after allocation in xfs_bmapi_write() just to ensure this executes in one place or the other..?

I think that if we hit this, we're doing something else wrong - why would we allocate unwritten extents and still need to initialise them to zero?

No idea, really (as noted above). ;) It just looked like it could be invoked twice per bmapi call, nothing else I saw prevented it, and it looks easily avoidable. Maybe somebody down the road decides to turn on block zeroing unconditionally in the block allocator due to hardware support or some such. Or maybe we'll never hit the problem. The point is that this code will inevitably be modified/enhanced down the road and nobody is going to remember that the zeroing is invoked twice in a particular prealloc codepath.

If we don't want to mess with the flags, how about an assert somewhere so it's explicit the bmapi implementation doesn't expect this combination of flags?

From: Dave Chinner <[email protected]>

DAX has a page fault serialisation problem with block allocation. Because it allows concurrent page faults and does not have a page lock to serialise faults to the same page, it can get two concurrent faults to the page that race.

When two read faults race, this isn't a huge problem as the data underlying the page is not changing and so "detect and drop" works just fine. The issues are to do with write faults.

When two write faults occur, we serialise block allocation in get_blocks() so only one faul will allocate the extent. It will, however, be marked as an unwritten extent, and that is where the problem lies - the DAX fault code cannot differentiate between a block that was just allocated and a block that was preallocated and needs zeroing. The result is that both write faults end up zeroing the block and attempting to convert it back to written.

The problem is that the first fault can zero and convert before the second fault starts zeroing, resulting in the zeroing for the second fault overwriting the data that the first fault wrote with zeros. The second fault then attempts to convert the unwritten extent, which is then a no-op because it's already written. Data loss occurs as a result of this race.

Because there is no sane locking construct in the page fault code that we can use for serialisation across the page faults, we need to ensure block allocation and zeroing occurs atomically in the filesystem. This means we can still take concurrent page faults and the only time they will serialise is in the filesystem mapping/allocation callback. The page fault code will always see written, initialised extents, so we will be able to remove the unwritten extent handling from the DAX code when all filesystems are converted.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/dax.c           |  5 +++++
 fs/xfs/xfs_aops.c  | 13 +++++++++----
 fs/xfs/xfs_iomap.c | 21 ++++++++++++++++++++-
 3 files changed, 34 insertions(+), 5 deletions(-)

fs/xfs/xfs_iomap.c +0 -0

index c3cb5a5..f4f5b43 100644

fs/xfs/xfs_iomap.c +7 -0

 	tp = xfs_trans_alloc(mp, XFS_TRANS_DIOSTRAT);
+
+	/*
+	 * For DAX, we do not allocate unwritten extents, but instead we zero
+	 * the block before we commit the transaction.  Ideally we'd like to do
+	 * 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.

fs/xfs/xfs_iomap.c +2 -1

 	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_write,
 				  resblks, resrtextents);
 	/*

@@ -221,7 +239,7 @@ xfs_iomap_write_direct(
 	xfs_bmap_init(&free_list, &firstfsb);
 	nimaps = 1;
 	error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb,
-				XFS_BMAPI_PREALLOC, &firstfsb, resblks, imap,
+				bmapi_flags, &firstfsb, resblks, imap,
 				&nimaps, &free_list);
 	if (error)
 		goto out_bmap_cancel;

@@ -232,6 +250,7 @@ xfs_iomap_write_direct(
 	error = xfs_bmap_finish(&tp, &free_list, &committed);
 	if (error)
 		goto out_bmap_cancel;
+
 	error = xfs_trans_commit(tp);
 	if (error)
 		goto out_unlock;

Signature · 6 lines

2.5.0

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 43 lines · Dave Chinner

From: Dave Chinner <[email protected]>

DAX has a page fault serialisation problem with block allocation. Because it allows concurrent page faults and does not have a page lock to serialise faults to the same page, it can get two concurrent faults to the page that race.

When two read faults race, this isn't a huge problem as the data underlying the page is not changing and so "detect and drop" works just fine. The issues are to do with write faults.

When two write faults occur, we serialise block allocation in get_blocks() so only one faul will allocate the extent. It will, however, be marked as an unwritten extent, and that is where the problem lies - the DAX fault code cannot differentiate between a block that was just allocated and a block that was preallocated and needs zeroing. The result is that both write faults end up zeroing the block and attempting to convert it back to written.

The problem is that the first fault can zero and convert before the second fault starts zeroing, resulting in the zeroing for the second fault overwriting the data that the first fault wrote with zeros. The second fault then attempts to convert the unwritten extent, which is then a no-op because it's already written. Data loss occurs as a result of this race.

Because there is no sane locking construct in the page fault code that we can use for serialisation across the page faults, we need to ensure block allocation and zeroing occurs atomically in the filesystem. This means we can still take concurrent page faults and the only time they will serialise is in the filesystem mapping/allocation callback. The page fault code will always see written, initialised extents, so we will be able to remove the unwritten extent handling from the DAX code when all filesystems are converted.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/dax.c           |  5 +++++
 fs/xfs/xfs_aops.c  | 13 +++++++++----
 fs/xfs/xfs_iomap.c | 21 ++++++++++++++++++++-
 3 files changed, 34 insertions(+), 5 deletions(-)

...

Quoted 4 lines

fs/xfs/xfs_iomap.c +0 -0

index c3cb5a5..f4f5b43 100644

...

Quoted 15 lines

fs/xfs/xfs_iomap.c +14 -0

 	tp = xfs_trans_alloc(mp, XFS_TRANS_DIOSTRAT);
+
+	/*
+	 * For DAX, we do not allocate unwritten extents, but instead we zero
+	 * the block before we commit the transaction.  Ideally we'd like to do
+	 * 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.
+	 */
+	if (IS_DAX(VFS_I(ip))) {

fs/xfs/xfs_iomap.c xfs_iomap_write_direct( · line 215

Quoted 3 lines

fs/xfs/xfs_iomap.c +3 -0

+		bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
+		tp->t_flags |= XFS_TRANS_RESERVE;
+	}

Am I following the commit log description correctly in that block zeroing is only required for DAX faults? Do we zero blocks for DAX DIO as well to be consistent, or is that also required (because it looks like we still have end_io completion for dio writes anyways)?

Quoted 3 lines · Brian Foster

Quoted 2 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Quoted 13 lines

Quoted 13 lines

fs/xfs/xfs_iomap.c +13 -0

+	/*
+	 * For DAX, we do not allocate unwritten extents, but instead we zero
+	 * the block before we commit the transaction.  Ideally we'd like to do
+	 * 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.
+	 */
+	if (IS_DAX(VFS_I(ip))) {

Quoted 8 lines

fs/xfs/xfs_iomap.c xfs_iomap_write_direct( · line 215

Quoted 3 lines

fs/xfs/xfs_iomap.c +3 -0

+		bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
+		tp->t_flags |= XFS_TRANS_RESERVE;
+	}

Am I following the commit log description correctly in that block zeroing is only required for DAX faults? Do we zero blocks for DAX DIO as well to be consistent, or is that also required (because it looks like we still have end_io completion for dio writes anyways)?

DAX DIO will do the zeroing rather than using unwritten extents, too. But we still have DIO IO completion as that needs to do file size updates.

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 4 lines · Dave Chinner

Quoted 3 lines · Brian Foster

Quoted 2 lines · Dave Chinner

From: Dave Chinner <[email protected]>

...

Quoted 22 lines

Quoted 13 lines

Quoted 13 lines

fs/xfs/xfs_iomap.c +13 -0

+	/*
+	 * For DAX, we do not allocate unwritten extents, but instead we zero
+	 * the block before we commit the transaction.  Ideally we'd like to do
+	 * 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.
+	 */
+	if (IS_DAX(VFS_I(ip))) {

Quoted 8 lines

fs/xfs/xfs_iomap.c xfs_iomap_write_direct( · line 215

Quoted 3 lines

fs/xfs/xfs_iomap.c +3 -0

+		bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
+		tp->t_flags |= XFS_TRANS_RESERVE;
+	}

Am I following the commit log description correctly in that block zeroing is only required for DAX faults? Do we zero blocks for DAX DIO as well to be consistent, or is that also required (because it looks like we still have end_io completion for dio writes anyways)?

Quoted 4 lines

DAX DIO will do the zeroing rather than using unwritten extents, too. But we still have DIO IO completion as that needs to do file size updates.

Right, my question is: is the DAX DIO zeroing required to avoid the races described as the purpose for this patch, or is this just here as a simplification? In other words, why not do block zeroing only for DAX faults and not DAX/DIO?

I ask because my understanding is the purpose of this patch is a special atomic zeroed allocation requirement just for mmap.

Unless there is some
special mixed dio/mmap case I'm missing, doing so for DAX/DIO basically causes a clear_pmem() over every page sized chunk of the target I/O range for which we already have the data.

Quoted 33 lines · Brian Foster

Quoted 4 lines · Dave Chinner

Quoted 3 lines · Brian Foster

Quoted 2 lines · Dave Chinner

From: Dave Chinner <[email protected]>

...

Quoted 22 lines

Quoted 13 lines

Quoted 13 lines

fs/xfs/xfs_iomap.c +13 -0

+	/*
+	 * For DAX, we do not allocate unwritten extents, but instead we zero
+	 * the block before we commit the transaction.  Ideally we'd like to do
+	 * 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.
+	 */
+	if (IS_DAX(VFS_I(ip))) {

Quoted 8 lines

fs/xfs/xfs_iomap.c xfs_iomap_write_direct( · line 215

Quoted 3 lines

fs/xfs/xfs_iomap.c +3 -0

+		bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
+		tp->t_flags |= XFS_TRANS_RESERVE;
+	}

Am I following the commit log description correctly in that block zeroing is only required for DAX faults? Do we zero blocks for DAX DIO as well to be consistent, or is that also required (because it looks like we still have end_io completion for dio writes anyways)?

Quoted 4 lines

DAX DIO will do the zeroing rather than using unwritten extents, too. But we still have DIO IO completion as that needs to do file size updates.

Quoted 4 lines

Right, my question is: is the DAX DIO zeroing required to avoid the races described as the purpose for this patch, or is this just here as a simplification? In other words, why not do block zeroing only for DAX faults and not DAX/DIO?

Because the only reason the DIO code does 'allocate unwritten; convert unwritten on IO completion' is so that if we have:

	allocate
	trans_commit
	....				log force
					journal IO submit
	....				journal IO completion
	submit data io
	crash

We don't expose allocated blocks containing stale data to userspace via recovery. The allcoation uses unwritten extents to ensure that if the allocation is recovered without the correspending completion, it reads as zeros rather whatever was previously on disk in taht location.

For DAX, we can zero the blocks inside the allocation transaction for direct IO, and hence even if we have the above happen, we'll only ever expose zeros. Hence we don't need unwritten extents in the DIO path to avoid stale data exposure, and so we can simply avoid all that extra overhead of unwritten extent conversion on completion...

Quoted 2 lines

I ask because my understanding is the purpose of this patch is a special atomic zeroed allocation requirement just for mmap.

The requirement is set by DAX+mmap; the implementation is a generic "allocate zeroed blocks" mechanism that can be applied to any allocation that uses unwritten extents to allocate zeroed blocks if zeroing is more efficient than using unwritten extents....

Quoted 4 lines

Unless there is some
special mixed dio/mmap case I'm missing, doing so for DAX/DIO basically causes a clear_pmem() over every page sized chunk of the target I/O range for which we already have the data.

I don't follow - this only zeros blocks when we do allocation of new blocks or overwrite unwritten extents, not on blocks which we already have written data extents allocated for...

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 10 lines · Dave Chinner

Quoted 4 lines · Brian Foster

Unless there is some
special mixed dio/mmap case I'm missing, doing so for DAX/DIO basically causes a clear_pmem() over every page sized chunk of the target I/O range for which we already have the data.

I don't follow - this only zeros blocks when we do allocation of new blocks or overwrite unwritten extents, not on blocks which we already have written data extents allocated for...

Why are we assuming that block zeroing is more efficient than unwritten extents for DAX/dio? I haven't played with pmem enough to know for sure one way or another (or if hw support is imminent), but I'd expect the latter to be more efficient in general without any kind of hardware support.

Just as an example, here's an 8GB pwrite test, large buffer size, to XFS on a ramdisk mounted with '-o dax:'

- Before this series:

# xfs_io -fc "truncate 0" -c "pwrite -b 10m 0 8g" /mnt/file
wrote 8589934592/8589934592 bytes at offset 0
8.000 GiB, 820 ops; 0:00:04.00 (1.909 GiB/sec and 195.6591 ops/sec)

- After this series:

# xfs_io -fc "truncate 0" -c "pwrite -b 10m 0 8g" /mnt/file
wrote 8589934592/8589934592 bytes at offset 0
8.000 GiB, 820 ops; 0:00:12.00 (659.790 MiB/sec and 66.0435 ops/sec)

The impact is less with a smaller buffer size so the above is just meant to illustrate the point. FWIW, I'm also fine with getting this in as a matter of "correctness before performance" since this stuff is clearly still under development, but as far as I can see so far we should probably ultimately prefer unwritten extents for DAX/DIO (or at least plan to run some similar tests on real pmem hw). Thoughts?

[add people to the cc list]

Quoted 27 lines · Brian Foster

Quoted 10 lines · Dave Chinner

Quoted 4 lines · Brian Foster

Unless there is some
special mixed dio/mmap case I'm missing, doing so for DAX/DIO basically causes a clear_pmem() over every page sized chunk of the target I/O range for which we already have the data.

I don't follow - this only zeros blocks when we do allocation of new blocks or overwrite unwritten extents, not on blocks which we already have written data extents allocated for...

Why are we assuming that block zeroing is more efficient than unwritten extents for DAX/dio? I haven't played with pmem enough to know for sure one way or another (or if hw support is imminent), but I'd expect the latter to be more efficient in general without any kind of hardware support.

Just as an example, here's an 8GB pwrite test, large buffer size, to XFS on a ramdisk mounted with '-o dax:'

- Before this series:

# xfs_io -fc "truncate 0" -c "pwrite -b 10m 0 8g" /mnt/file
wrote 8589934592/8589934592 bytes at offset 0
8.000 GiB, 820 ops; 0:00:04.00 (1.909 GiB/sec and 195.6591 ops/sec)

Quoted 5 lines

- After this series:

# xfs_io -fc "truncate 0" -c "pwrite -b 10m 0 8g" /mnt/file
wrote 8589934592/8589934592 bytes at offset 0
8.000 GiB, 820 ops; 0:00:12.00 (659.790 MiB/sec and 66.0435 ops/sec)

That looks wrong. Much, much slower than it should be just zeroing pages and then writing to them again while cache hot.

Oh, hell - dax_clear_blocks() is stupidly slow. A profile shows this loop spending most of the CPU time:

¿ ¿ jbe ea
¿ de: clflus %ds:(%rax)

 84.67 ¿       add    %rsi,%rax
       ¿       cmp    %rax,%rdx
       ¿     ¿ ja     de
       ¿ ea:   add    %r13,-0x38(%rbp)
       ¿       sub    %r12,%r14
       ¿       sub    %r12,-0x40(%rbp)

That is the overhead of __arch_wb_cache_pmem() i.e. issuing CPU cache flushes after each memset.

None of these pmem memory operations are optimised yet - the implementations are correct, but performance still needs work. The conversion to non-temporal stores should get rid of this cache flush overhead (somewhat), but I was still expecting this code to be much closer to memset speed and not reduce performance to bugger all...

Quoted 6 lines

The impact is less with a smaller buffer size so the above is just meant to illustrate the point. FWIW, I'm also fine with getting this in as a matter of "correctness before performance" since this stuff is clearly still under development, but as far as I can see so far we should probably ultimately prefer unwritten extents for DAX/DIO (or at least plan to run some similar tests on real pmem hw). Thoughts?

We're going to have these problems initially, but from the XFS persepective those problems don't matter because we have a different problem to solve. That is, we need to move towards an architecture that is highly efficient for low latency, high IOPS storage subsystems. The first step towards that is to be able to offload block zeroing to the hardware so we can avoid using unwritten extents.

In the long run, we don't want to use unwritten extents on DAX if we can avoid it - the CPU overhead of unwritten extent conversion isn't constant (i.e. it grows with the size of the BMBT) and it isn't deterministic (e.g. split/merge take much more CPU than a simple record write to clear an unwritten flag). We don't notice this overhead much with normal IO because of the fact that the CPU time for conversion is much less than the CPU time for the IO to complete, hence it's a win.

But for PMEM, directly zeroing a 4k chunk of data should take *much less CPU* than synchronously starting a transaction, reserving space, looking up the extent in the BMBT, loading the buffers from cache, modifying the buffers, logging the changes and committing the transaction (which in itself will typically copy more than a single page of metadata into the CIL buffers).

Realistically, dax_clear_blocks() should probably be implemented at the pmem driver layer through blkdev_issue_zeroout() because all it does is directly map the sector/len to pfn via bdev_direct_access() and then zero it - it's a sector based, block device operation. We don't actually need a special case path for DAX here. Optimisation of this operation has little to do with the filesystem.

This comes back to the comments I made w.r.t. the pmem driver implementation doing synchronous IO by immediately forcing CPU cache flushes and barriers. it's obviously correct, but it looks like there's going to be a major performance penalty associated with it. This is why I recently suggested that a pmem driver that doesn't do CPU cache writeback during IO but does it on REQ_FLUSH is an architecture we'll likely have to support.

In this case, the block device won't need to flush CPU cache lines for the zeroed blocks until the allocation transaction is actually committed to the journal. In that case, there's a good chance that we'd end up also committing the new data as well, hence avoiding two synchronous memory writes. i.e. the "big hammer" REQ_FLUSH implementation may well be *much* faster than fine-grained synchronous "stable on completion" writes for persistent memory.

This, however, is not really a problem for the filesystem - it's a pmem driver architecture problem. ;)

Quoted 51 lines · Dave Chinner

[add people to the cc list]

Quoted 27 lines · Brian Foster

Quoted 10 lines · Dave Chinner

Quoted 4 lines · Brian Foster

Unless there is some
special mixed dio/mmap case I'm missing, doing so for DAX/DIO basically causes a clear_pmem() over every page sized chunk of the target I/O range for which we already have the data.

I don't follow - this only zeros blocks when we do allocation of new blocks or overwrite unwritten extents, not on blocks which we already have written data extents allocated for...

Why are we assuming that block zeroing is more efficient than unwritten extents for DAX/dio? I haven't played with pmem enough to know for sure one way or another (or if hw support is imminent), but I'd expect the latter to be more efficient in general without any kind of hardware support.

Just as an example, here's an 8GB pwrite test, large buffer size, to XFS on a ramdisk mounted with '-o dax:'

- Before this series:

# xfs_io -fc "truncate 0" -c "pwrite -b 10m 0 8g" /mnt/file
wrote 8589934592/8589934592 bytes at offset 0
8.000 GiB, 820 ops; 0:00:04.00 (1.909 GiB/sec and 195.6591 ops/sec)

Quoted 5 lines

- After this series:

# xfs_io -fc "truncate 0" -c "pwrite -b 10m 0 8g" /mnt/file
wrote 8589934592/8589934592 bytes at offset 0
8.000 GiB, 820 ops; 0:00:12.00 (659.790 MiB/sec and 66.0435 ops/sec)

That looks wrong. Much, much slower than it should be just zeroing pages and then writing to them again while cache hot.

Oh, hell - dax_clear_blocks() is stupidly slow. A profile shows this loop spending most of the CPU time:

¿ ¿ jbe ea
¿ de: clflus %ds:(%rax)

 84.67 ¿       add    %rsi,%rax
       ¿       cmp    %rax,%rdx
       ¿     ¿ ja     de
       ¿ ea:   add    %r13,-0x38(%rbp)
       ¿       sub    %r12,%r14
       ¿       sub    %r12,-0x40(%rbp)

Quoted 2 lines

That is the overhead of __arch_wb_cache_pmem() i.e. issuing CPU cache flushes after each memset.

Ideally this would be non-temporal and skip the second flush loop altogether. Outside of that another problem is that this cpu does not support the clwb instruction and is instead using the serializing and invalidating clflush instruction.

Quoted 8 lines

This comes back to the comments I made w.r.t. the pmem driver implementation doing synchronous IO by immediately forcing CPU cache flushes and barriers. it's obviously correct, but it looks like there's going to be a major performance penalty associated with it. This is why I recently suggested that a pmem driver that doesn't do CPU cache writeback during IO but does it on REQ_FLUSH is an architecture we'll likely have to support.

The only thing we can realistically delay is wmb_pmem() i.e. the final sync waiting for data that has *left* the cpu cache. Unless/until we get a architecturally guaranteed method to write-back the entire cache, or flush the cache by physical-cache-way we're stuck with either non-temporal cycles or looping on potentially huge virtual address ranges.

Quoted 3 lines

This, however, is not really a problem for the filesystem - it's a pmem driver architecture problem. ;)

It's a platform problem. Let's see how this looks when not using clflush instructions.

Also, another benefit of pushing zeroing down into the driver is that for brd, as used in this example, it will rightly be a nop because there's no persistence to guarantee there.

Quoted 1 line · Dan Williams

On Mon, Nov 2, 2015 at 1:44 PM, Dave Chinner <[email protected]> wrote:

Quoted 9 lines

Quoted 8 lines

This comes back to the comments I made w.r.t. the pmem driver implementation doing synchronous IO by immediately forcing CPU cache flushes and barriers. it's obviously correct, but it looks like there's going to be a major performance penalty associated with it. This is why I recently suggested that a pmem driver that doesn't do CPU cache writeback during IO but does it on REQ_FLUSH is an architecture we'll likely have to support.

Quoted 6 lines

The only thing we can realistically delay is wmb_pmem() i.e. the final sync waiting for data that has *left* the cpu cache. Unless/until we get a architecturally guaranteed method to write-back the entire cache, or flush the cache by physical-cache-way we're stuck with either non-temporal cycles or looping on potentially huge virtual address ranges.

I'm missing something: why won't flushing the address range returned by bdev_direct_access() during a fsync operation work? i.e. we're working with exactly the same address as dax_clear_blocks() and dax_do_io() use, so why can't we look up that address and flush it from fsync?

Quoted 2 lines · Dave Chinner

Quoted 1 line · Dan Williams

On Mon, Nov 2, 2015 at 1:44 PM, Dave Chinner <[email protected]> wrote:

<>

Quoted 16 lines

Quoted 9 lines

Quoted 8 lines

This comes back to the comments I made w.r.t. the pmem driver implementation doing synchronous IO by immediately forcing CPU cache flushes and barriers. it's obviously correct, but it looks like there's going to be a major performance penalty associated with it. This is why I recently suggested that a pmem driver that doesn't do CPU cache writeback during IO but does it on REQ_FLUSH is an architecture we'll likely have to support.

Quoted 6 lines

The only thing we can realistically delay is wmb_pmem() i.e. the final sync waiting for data that has *left* the cpu cache. Unless/until we get a architecturally guaranteed method to write-back the entire cache, or flush the cache by physical-cache-way we're stuck with either non-temporal cycles or looping on potentially huge virtual address ranges.

Quoted 5 lines

I'm missing something: why won't flushing the address range returned by bdev_direct_access() during a fsync operation work? i.e. we're working with exactly the same address as dax_clear_blocks() and dax_do_io() use, so why can't we look up that address and flush it from fsync?

I could be wrong, but I don't see a reason why DAX can't use the strategy of writing data and marking it dirty in one step and then flushing later in response to fsync/msync. I think this could be used everywhere we write or zero data - dax_clear_blocks(), dax_io() etc. (I believe that lots of the block zeroing code will go away once we have the XFS and ext4 patches in that guarantee we will only get written and zeroed extents from the filesystem in response to get_block().) I think the PMEM driver, lacking the ability to mark things as dirty in the radix tree, etc, will need to keep doing things synchronously.

Hmm...if we go this path, though, is that an argument against moving the zeroing from DAX down into the driver? True, with BRD it makes things nice and efficient because you can zero and never flush, and the driver knows there's nothing else to do.

For PMEM, though, you lose the ability to zero the data and then queue the flushing for later, as you would be able to do if you left the zeroing code in DAX. The benefit of this is that if you are going to immediately re-write the newly zeroed data (which seems common), PMEM will end up doing an extra cache flush of the zeroes, only to have them overwritten and marked as dirty by DAX. If we leave the zeroing to DAX we can mark it dirty once, zero it once, write it once, and flush it once.

This would make us lose the ability to do hardware-assisted flushing in the future that requires driver specific knowledge, though I don't think that exists yet.

Perhaps we should leave the zeroing in DAX for now to take advantage of the single flush, and then move it down if a driver can improve performance with hardware assisted PMEM zeroing?

Quoted 26 lines · Ross Zwisler

Quoted 2 lines · Dave Chinner

Quoted 1 line · Dan Williams

On Mon, Nov 2, 2015 at 1:44 PM, Dave Chinner <[email protected]> wrote:

<>

Quoted 16 lines

Quoted 9 lines

Quoted 8 lines

This comes back to the comments I made w.r.t. the pmem driver implementation doing synchronous IO by immediately forcing CPU cache flushes and barriers. it's obviously correct, but it looks like there's going to be a major performance penalty associated with it. This is why I recently suggested that a pmem driver that doesn't do CPU cache writeback during IO but does it on REQ_FLUSH is an architecture we'll likely have to support.

Quoted 6 lines

The only thing we can realistically delay is wmb_pmem() i.e. the final sync waiting for data that has *left* the cpu cache. Unless/until we get a architecturally guaranteed method to write-back the entire cache, or flush the cache by physical-cache-way we're stuck with either non-temporal cycles or looping on potentially huge virtual address ranges.

Quoted 5 lines

I'm missing something: why won't flushing the address range returned by bdev_direct_access() during a fsync operation work? i.e. we're working with exactly the same address as dax_clear_blocks() and dax_do_io() use, so why can't we look up that address and flush it from fsync?

Quoted 9 lines

I could be wrong, but I don't see a reason why DAX can't use the strategy of writing data and marking it dirty in one step and then flushing later in response to fsync/msync. I think this could be used everywhere we write or zero data - dax_clear_blocks(), dax_io() etc. (I believe that lots of the block zeroing code will go away once we have the XFS and ext4 patches in that guarantee we will only get written and zeroed extents from the filesystem in response to get_block().) I think the PMEM driver, lacking the ability to mark things as dirty in the radix tree, etc, will need to keep doing things synchronously.

Not without numbers showing the relative performance of dirtying cache followed by flushing vs non-temporal + pcommit.

Quoted 5 lines

Hmm...if we go this path, though, is that an argument against moving the zeroing from DAX down into the driver? True, with BRD it makes things nice and efficient because you can zero and never flush, and the driver knows there's nothing else to do.

Quoted 7 lines

For PMEM, though, you lose the ability to zero the data and then queue the flushing for later, as you would be able to do if you left the zeroing code in DAX. The benefit of this is that if you are going to immediately re-write the newly zeroed data (which seems common), PMEM will end up doing an extra cache flush of the zeroes, only to have them overwritten and marked as dirty by DAX. If we leave the zeroing to DAX we can mark it dirty once, zero it once, write it once, and flush it once.

Why do we lose the ability to flush later if the driver supports blkdev_issue_zeroout?

Quoted 3 lines

This would make us lose the ability to do hardware-assisted flushing in the future that requires driver specific knowledge, though I don't think that exists yet.

ioatdma has supported memset() for a while now, but I would prioritize a non-temporal SIMD implementation first.

Quoted 3 lines

Perhaps we should leave the zeroing in DAX for now to take advantage of the single flush, and then move it down if a driver can improve performance with hardware assisted PMEM zeroing?

Not convinced. I think we should implement the driver zeroing solution and take a look at performance.

Quoted 13 lines · Dan Williams

Quoted 5 lines

Hmm...if we go this path, though, is that an argument against moving the zeroing from DAX down into the driver? True, with BRD it makes things nice and efficient because you can zero and never flush, and the driver knows there's nothing else to do.

Quoted 7 lines

For PMEM, though, you lose the ability to zero the data and then queue the flushing for later, as you would be able to do if you left the zeroing code in DAX. The benefit of this is that if you are going to immediately re-write the newly zeroed data (which seems common), PMEM will end up doing an extra cache flush of the zeroes, only to have them overwritten and marked as dirty by DAX. If we leave the zeroing to DAX we can mark it dirty once, zero it once, write it once, and flush it once.

Quoted 2 lines

Why do we lose the ability to flush later if the driver supports blkdev_issue_zeroout?

I think that if you implement zeroing in the driver you'd need to also flush in the driver because you wouldn't have access to the radix tree to be able to mark entries as dirty so you can flush them later.

As I think about this more, though, I'm not sure that having the zeroing flush later could work. I'm guessing that the filesystem must require a sync point between the zeroing and the subsequent follow-up writes so that you can sync metadata for the block allocation. Otherwise you could end up in a situation where you've got your metadata pointing at newly allocated blocks but the new zeros are still in the processor cache - if
you lose power you've just created an information leak. Dave, Jan, does
this make sense?

Quoted 21 lines · Ross Zwisler

Quoted 13 lines · Dan Williams

Quoted 5 lines

Hmm...if we go this path, though, is that an argument against moving the zeroing from DAX down into the driver? True, with BRD it makes things nice and efficient because you can zero and never flush, and the driver knows there's nothing else to do.

Quoted 7 lines

For PMEM, though, you lose the ability to zero the data and then queue the flushing for later, as you would be able to do if you left the zeroing code in DAX. The benefit of this is that if you are going to immediately re-write the newly zeroed data (which seems common), PMEM will end up doing an extra cache flush of the zeroes, only to have them overwritten and marked as dirty by DAX. If we leave the zeroing to DAX we can mark it dirty once, zero it once, write it once, and flush it once.

Quoted 2 lines

Why do we lose the ability to flush later if the driver supports blkdev_issue_zeroout?

I think that if you implement zeroing in the driver you'd need to also flush in the driver because you wouldn't have access to the radix tree to be able to mark entries as dirty so you can flush them later.

Quoted 8 lines

As I think about this more, though, I'm not sure that having the zeroing flush later could work. I'm guessing that the filesystem must require a sync point between the zeroing and the subsequent follow-up writes so that you can sync metadata for the block allocation. Otherwise you could end up in a situation where you've got your metadata pointing at newly allocated blocks but the new zeros are still in the processor cache - if
you lose power you've just created an information leak. Dave, Jan, does
this make sense?

So the problem you describe does not exist. Thing to keep in mind is that filesystem are designed to work reliably with 'non-persistent' cache in the disk which is common these days. That's why we bother with all that REQ_FLUSH | REQ_FUA and blkdev_issue_flush() stuff after all. Processor cache is exactly that kind of the cache attached to the PMEM storage. And Dave and I try to steer you to a solution that would also treat it equally in DAX filesystems as well :).

Now how the problem is currently solved: When we allocate blocks, we just record that information in a transaction in the journal. For DAX case we also submit the IO zeroing those blocks and wait for it. Now if we crash before the transaction gets committed, blocks won't be seen in the inode after a journal recovery and thus no data exposure can happen. As a part of transaction commit, we call blkdev_issue_flush() (or submit REQ_FLUSH request). We expect that to force out all the IO in volatile caches into the persistent storage. So this will also force the zeroing into persistent storage for normal disks and AFAIU if you do zeroing with non-temporal writes in pmem driver and then do wmb_pmem() in response to a flush request we get the same persistency guarantee in pmem case as well. So after a transaction commit we are guaranteed to see zeros in those allocated blocks.

So the transaction commit and the corresponding flush request in particular is the sync point you speak about above but the good thing is that in most cases this will happen after real data gets written into those blocks so we save the unnecessary flush.

Quoted 14 lines · Jan Kara

Now how the problem is currently solved: When we allocate blocks, we just record that information in a transaction in the journal. For DAX case we also submit the IO zeroing those blocks and wait for it. Now if we crash before the transaction gets committed, blocks won't be seen in the inode after a journal recovery and thus no data exposure can happen. As a part of transaction commit, we call blkdev_issue_flush() (or submit REQ_FLUSH request). We expect that to force out all the IO in volatile caches into the persistent storage. So this will also force the zeroing into persistent storage for normal disks and AFAIU if you do zeroing with non-temporal writes in pmem driver and then do wmb_pmem() in response to a flush request we get the same persistency guarantee in pmem case as well. So after a transaction commit we are guaranteed to see zeros in those allocated blocks.

Quoted 4 lines

So the transaction commit and the corresponding flush request in particular is the sync point you speak about above but the good thing is that in most cases this will happen after real data gets written into those blocks so we save the unnecessary flush.

Cool, thank you for the explanation, that makes sense to me.

When dealing with normal SSDs and the page cache, does the filesystem keep the zeroes in the page cache, or does it issue it directly to the driver via sb_issue_zeroout()/blkdev_issue_zeroout()? If we keep it in the page cache so

that the follow-up writes just update the dirty pages and we end up writing to media once, this seems like it would flow nicely into the idea of zeroing new blocks at the DAX level without flushing and just marking them as dirty in the radix tree.

If the zeroing happens via sb_issue_zeroout() then this probably doesn't make sense because the existing flow won't include a fsync/msync type step of the newly zeroed data in the page cache.

Realistically, dax_clear_blocks() should probably be implemented at the pmem driver layer through blkdev_issue_zeroout() because all it does is directly map the sector/len to pfn via bdev_direct_access() and then zero it - it's a sector based, block device operation. We don't actually need a special case path for DAX here. Optimisation of this operation has little to do with the filesystem.

From: Dave Chinner <[email protected]>

For DAX, we are now doing block zeroing and
we are updating the file size during allocation. This means we no longer need an IO completion callback to do these things, so remove the completion callbacks from the __dax_fault and __dax_mkwrite calls.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_aops.c | 39 ---------------------------------------
 fs/xfs/xfs_aops.h |  1 -
 fs/xfs/xfs_file.c |  5 ++---
 3 files changed, 2 insertions(+), 43 deletions(-)

fs/xfs/xfs_aops.c +0 -39

index 7b4f849..29e7e5d 100644
@@ -1666,45 +1666,6 @@ xfs_end_io_direct_write(
 	__xfs_end_io_direct_write(inode, ioend, offset, size);
 }
 
-/*
- * For DAX we need a mapping buffer callback for unwritten extent conversion
- * when page faults allocate blocks and then zero them. Note that in this
- * case the mapping indicated by the ioend may extend beyond EOF. We most
- * definitely do not want to extend EOF here, so we trim back the ioend size to
- * EOF.
- */
-#ifdef CONFIG_FS_DAX
-void
-xfs_end_io_dax_write(
-	struct buffer_head	*bh,
-	int			uptodate)
-{
-	struct xfs_ioend	*ioend = bh->b_private;
-	struct inode		*inode = ioend->io_inode;
-	ssize_t			size = ioend->io_size;
-
-	ASSERT(IS_DAX(ioend->io_inode));
-
-	/* if there was an error zeroing, then don't convert it */
-	if (!uptodate)
-		ioend->io_error = -EIO;
-
-	/*
-	 * Trim update to EOF, so we don't extend EOF during unwritten extent
-	 * conversion of partial EOF blocks.
-	 */
-	spin_lock(&XFS_I(inode)->i_flags_lock);
-	if (ioend->io_offset + size > i_size_read(inode))
-		size = i_size_read(inode) - ioend->io_offset;
-	spin_unlock(&XFS_I(inode)->i_flags_lock);
-
-	__xfs_end_io_direct_write(inode, ioend, ioend->io_offset, size);
-
-}
-#else
-void xfs_end_io_dax_write(struct buffer_head *bh, int uptodate) { }
-#endif
-
 static inline ssize_t
 xfs_vm_do_dio(
 	struct inode		*inode,

fs/xfs/xfs_aops.h +0 -1

index d39ba25..f6ffc9a 100644
@@ -60,7 +60,6 @@ int	xfs_get_blocks_direct(struct inode *inode, sector_t offset,
 			      struct buffer_head *map_bh, int create);
 int	xfs_get_blocks_dax_fault(struct inode *inode, sector_t offset,
 			         struct buffer_head *map_bh, int create);
-void	xfs_end_io_dax_write(struct buffer_head *bh, int uptodate);
 
 extern void xfs_count_page_state(struct page *, int *, int *);
 

fs/xfs/xfs_file.c +2 -3

index 7f873bc..403151a 100644
@@ -1508,8 +1508,7 @@ xfs_filemap_page_mkwrite(
 	xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
 
 	if (IS_DAX(inode)) {
-		ret = __dax_mkwrite(vma, vmf, xfs_get_blocks_dax_fault,
-				    xfs_end_io_dax_write);
+		ret = __dax_mkwrite(vma, vmf, xfs_get_blocks_dax_fault, NULL);
 	} else {
 		ret = __block_page_mkwrite(vma, vmf, xfs_get_blocks);
 		ret = block_page_mkwrite_return(ret);

@@ -1571,7 +1570,7 @@ xfs_filemap_pmd_fault(
 	file_update_time(vma->vm_file);
 	xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
 	ret = __dax_pmd_fault(vma, addr, pmd, flags, xfs_get_blocks_dax_fault,
-				    xfs_end_io_dax_write);
+			      NULL);
 	xfs_iunlock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
 	sb_end_pagefault(inode->i_sb);
 

Signature · 6 lines

2.5.0

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 8 lines · Dave Chinner

From: Dave Chinner <[email protected]>

For DAX, we are now doing block zeroing and
we are updating the file size during allocation. This means we no longer need an IO completion callback to do these things, so remove the completion callbacks from the __dax_fault and __dax_mkwrite calls.

Where do we "update the file size during allocation?"

Quoted 10 lines · Brian Foster

Quoted 8 lines · Dave Chinner

From: Dave Chinner <[email protected]>

For DAX, we are now doing block zeroing and
we are updating the file size during allocation. This means we no longer need an IO completion callback to do these things, so remove the completion callbacks from the __dax_fault and __dax_mkwrite calls.

Quoted 1 line

Where do we "update the file size during allocation?"

Stale comment. For page faults, we'll never update the file size (segv if fault is beyond EOF), and DIO still does IO completion based file size updates.

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

From: Dave Chinner <[email protected]>

->pfn_mkwrite support is needed so that when a page with allocated backing store takes a write fault we can check that the fault has not raced with a truncate and is pointing to a region beyond the current end of file.

This also allows us to update the timestamp on the inode, too, which fixes a generic/080 failure.

Signed-off-by Dave Chinner <[email protected]>

---

 fs/xfs/xfs_file.c  | 35 +++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trace.h |  1 +
 2 files changed, 36 insertions(+)

fs/xfs/xfs_file.c +35 -0

index 403151a..e7cf9ec 100644
@@ -1577,11 +1577,46 @@ xfs_filemap_pmd_fault(
 	return ret;
 }
 
+/*
+ * pfn_mkwrite was originally inteneded to ensure we capture time stamp
+ * updates on write faults. In reality, it's need to serialise against
+ * truncate similar to page_mkwrite. Hence we open-code dax_pfn_mkwrite()
+ * here and cycle the XFS_MMAPLOCK_SHARED to ensure we serialise the fault
+ * barrier in place.
+ */
+static int
+xfs_filemap_pfn_mkwrite(
+	struct vm_area_struct	*vma,
+	struct vm_fault		*vmf)
+{
+
+	struct inode		*inode = file_inode(vma->vm_file);
+	struct xfs_inode	*ip = XFS_I(inode);
+	int			ret = VM_FAULT_NOPAGE;
+	loff_t			size;
+
+	trace_xfs_filemap_pfn_mkwrite(ip);
+
+	sb_start_pagefault(inode->i_sb);
+	file_update_time(vma->vm_file);
+
+	/* check if the faulting page hasn't raced with truncate */
+	xfs_ilock(ip, XFS_MMAPLOCK_SHARED);
+	size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	if (vmf->pgoff >= size)
+		ret = VM_FAULT_SIGBUS;
+	xfs_iunlock(ip, XFS_MMAPLOCK_SHARED);
+	sb_end_pagefault(inode->i_sb);
+	return ret;
+
+}
+
 static const struct vm_operations_struct xfs_file_vm_ops = {
 	.fault		= xfs_filemap_fault,
 	.pmd_fault	= xfs_filemap_pmd_fault,
 	.map_pages	= filemap_map_pages,
 	.page_mkwrite	= xfs_filemap_page_mkwrite,
+	.pfn_mkwrite	= xfs_filemap_pfn_mkwrite,
 };
 
 STATIC int

fs/xfs/xfs_trace.h +1 -0

index 957f5cc..877079eb 100644
@@ -689,6 +689,7 @@ DEFINE_INODE_EVENT(xfs_inode_free_eofblocks_invalid);
 DEFINE_INODE_EVENT(xfs_filemap_fault);
 DEFINE_INODE_EVENT(xfs_filemap_pmd_fault);
 DEFINE_INODE_EVENT(xfs_filemap_page_mkwrite);
+DEFINE_INODE_EVENT(xfs_filemap_pfn_mkwrite);
 
 DECLARE_EVENT_CLASS(xfs_iref_class,
 	TP_PROTO(struct xfs_inode *ip, unsigned long caller_ip),

Signature · 6 lines

2.5.0

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

From: Dave Chinner <[email protected]>

The code initially committed didn't have the same checks for write faults as the dax_pmd_fault code and hence treats all faults as write faults. We can get read faults through this path because they is no pmd_mkwrite path for write faults similar to the normal page fault path. Hence we need to ensure that we only do c/mtime updates on write faults, and freeze protection is unnecessary for read faults.

Signed-off-by Dave Chinner <[email protected]>

---

 fs/xfs/xfs_file.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

fs/xfs/xfs_file.c +16 -4

index e7cf9ec..0045b0a 100644
@@ -1482,7 +1482,7 @@ xfs_file_llseek(
  *
  * mmap_sem (MM)
  *   sb_start_pagefault(vfs, freeze)
- *     i_mmap_lock (XFS - truncate serialisation)
+ *     i_mmaplock (XFS - truncate serialisation)
  *       page_lock (MM)
  *         i_lock (XFS - extent map serialisation)
  */

@@ -1550,6 +1550,13 @@ xfs_filemap_fault(
 	return ret;
 }
 
+/*
+ * Similar to xfs_filemap_fault(), the DAX fault path can call into here on
+ * both read and write faults. Hence we need to handle both cases. There is no
+ * ->pmd_mkwrite callout for huge pages, so we have a single function here to
+ * handle both cases here. @flags carries the information on the type of fault
+ * occuring.
+ */
 STATIC int
 xfs_filemap_pmd_fault(
 	struct vm_area_struct	*vma,

@@ -1566,13 +1573,18 @@ xfs_filemap_pmd_fault(
 
 	trace_xfs_filemap_pmd_fault(ip);
 
-	sb_start_pagefault(inode->i_sb);
-	file_update_time(vma->vm_file);
+	if (flags & FAULT_FLAG_WRITE) {
+		sb_start_pagefault(inode->i_sb);
+		file_update_time(vma->vm_file);
+	}
+
 	xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
 	ret = __dax_pmd_fault(vma, addr, pmd, flags, xfs_get_blocks_dax_fault,
 			      NULL);
 	xfs_iunlock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
-	sb_end_pagefault(inode->i_sb);
+
+	if (flags & FAULT_FLAG_WRITE)
+		sb_end_pagefault(inode->i_sb);
 
 	return ret;
 }

Signature · 6 lines

2.5.0

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Hi folks,

This is an updated patch set that was first posted here:

http://oss.sgi.com/archives/xfs/2015-10/msg00006.html

I've dropped the DAX locking revert patch from it; that's on it's way to Linus via other channels and is essentially independent to this set of XFS changes.

The only real change in the XFS code between the two versions is the addition of XFS_TRANS_RESERVE in the DAX path in xfs_iomap_write_direct() to allow it to dip into the reserve block pool for unwritten extent conversion rather than reporting ENOSPC.

Patches are against 4.3-rc5 + XFS for-next branch.

-Dave.

Quoted 10 lines · Dave Chinner

Hi folks,

This is an updated patch set that was first posted here:

http://oss.sgi.com/archives/xfs/2015-10/msg00006.html

I've dropped the DAX locking revert patch from it; that's on it's way to Linus via other channels and is essentially independent to this set of XFS changes.

Quoted 8 lines

The only real change in the XFS code between the two versions is the addition of XFS_TRANS_RESERVE in the DAX path in xfs_iomap_write_direct() to allow it to dip into the reserve block pool for unwritten extent conversion rather than reporting ENOSPC.

Patches are against 4.3-rc5 + XFS for-next branch.

-Dave.

Hey Dave,

I was going to start testing these, but I'm having trouble finding a baseline where they apply cleanly. It looks like xfs/for-next already contains v1 of the series, and they don't seem to apply cleanly to the current xfs/xfs-misc-fixes-for-4.4-2 nor to v4.3. The xfs repo I'm looking at is
git://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git