* [PATCH 1/7] xfs: fix inode size update overflow in xfs_map_direct()
2015-11-02 3:42 [PATCH 0/7] xfs: patches remaining for 4.4 merge window Dave Chinner
@ 2015-11-02 3:42 ` Dave Chinner
2015-11-02 3:42 ` [PATCH 2/7] xfs: introduce BMAPI_ZERO for allocating zeroed extents Dave Chinner
` (5 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2015-11-02 3:42 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Dave Chinner <david@fromorbit.com>
---
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(-)
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index e4fff58..366e41eb 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -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
diff --git a/fs/xfs/xfs_aops.h b/fs/xfs/xfs_aops.h
index 86afd1a..d39ba25 100644
--- a/fs/xfs/xfs_aops.h
+++ b/fs/xfs/xfs_aops.h
@@ -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 *);
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 2f7b6bd..7f873bc 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -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);
--
2.5.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/7] xfs: introduce BMAPI_ZERO for allocating zeroed extents
2015-11-02 3:42 [PATCH 0/7] xfs: patches remaining for 4.4 merge window Dave Chinner
2015-11-02 3:42 ` [PATCH 1/7] xfs: fix inode size update overflow in xfs_map_direct() Dave Chinner
@ 2015-11-02 3:42 ` Dave Chinner
2015-11-02 14:15 ` Brian Foster
2015-11-02 3:42 ` [PATCH 3/7] xfs: Don't use unwritten extents for DAX Dave Chinner
` (4 subsequent siblings)
6 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2015-11-02 3:42 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
fs/xfs/libxfs/xfs_alloc.c | 10 +++++++++-
fs/xfs/libxfs/xfs_alloc.h | 8 +++++---
fs/xfs/libxfs/xfs_bmap.c | 35 +++++++++++++++++++++++++++++++++--
fs/xfs/libxfs/xfs_bmap.h | 13 +++++++++++--
fs/xfs/xfs_bmap_util.c | 36 ++++++++++++++++++++++++++++++++++++
fs/xfs/xfs_mount.h | 3 +++
6 files changed, 97 insertions(+), 8 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index e926197..3479294 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -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),
args->len);
#endif
+
+ /* 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;
+ }
+
}
xfs_perag_put(args->pag);
return 0;
diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index ca1c816..0ecde4d 100644
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -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);
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index ab92d10..119c242 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -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;
+ /*
+ * 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;
+ }
+
error = xfs_bmap_add_extent_unwritten_real(bma->tp, bma->ip, &bma->idx,
&bma->cur, mval, bma->firstblock, bma->flist,
&tmp_logflags);
@@ -4514,6 +4533,18 @@ xfs_bmapi_write(
ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ /* zeroing is for currently only for data extents, not metadata */
+ ASSERT((flags & (XFS_BMAPI_METADATA | XFS_BMAPI_ZERO)) !=
+ (XFS_BMAPI_METADATA | XFS_BMAPI_ZERO));
+ /*
+ * we can allocate unwritten extents or pre-zero allocated blocks,
+ * but it makes no sense to do both at once. This would result in
+ * zeroing the unwritten extent twice, but it still being an
+ * unwritten extent....
+ */
+ ASSERT((flags & (XFS_BMAPI_PREALLOC | XFS_BMAPI_ZERO)) !=
+ (XFS_BMAPI_PREALLOC | XFS_BMAPI_ZERO));
+
if (unlikely(XFS_TEST_ERROR(
(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 6aaa0c1..a160f8a 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -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)
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index eca325e..dbae649 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -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);
+ 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);
+
+ /*
+ * 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);
+
+}
+
+/*
* 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;
}
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 8795272..f20e5de 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -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__ */
--
2.5.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/7] xfs: introduce BMAPI_ZERO for allocating zeroed extents
2015-11-02 3:42 ` [PATCH 2/7] xfs: introduce BMAPI_ZERO for allocating zeroed extents Dave Chinner
@ 2015-11-02 14:15 ` Brian Foster
0 siblings, 0 replies; 11+ messages in thread
From: Brian Foster @ 2015-11-02 14:15 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Mon, Nov 02, 2015 at 02:42:10PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
> ---
Looks good with the assert fix:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_alloc.c | 10 +++++++++-
> fs/xfs/libxfs/xfs_alloc.h | 8 +++++---
> fs/xfs/libxfs/xfs_bmap.c | 35 +++++++++++++++++++++++++++++++++--
> fs/xfs/libxfs/xfs_bmap.h | 13 +++++++++++--
> fs/xfs/xfs_bmap_util.c | 36 ++++++++++++++++++++++++++++++++++++
> fs/xfs/xfs_mount.h | 3 +++
> 6 files changed, 97 insertions(+), 8 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index e926197..3479294 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -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),
> args->len);
> #endif
> +
> + /* 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;
> + }
> +
> }
> xfs_perag_put(args->pag);
> return 0;
> diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> index ca1c816..0ecde4d 100644
> --- a/fs/xfs/libxfs/xfs_alloc.h
> +++ b/fs/xfs/libxfs/xfs_alloc.h
> @@ -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);
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index ab92d10..119c242 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -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;
>
> + /*
> + * 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;
> + }
> +
> error = xfs_bmap_add_extent_unwritten_real(bma->tp, bma->ip, &bma->idx,
> &bma->cur, mval, bma->firstblock, bma->flist,
> &tmp_logflags);
> @@ -4514,6 +4533,18 @@ xfs_bmapi_write(
> ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> + /* zeroing is for currently only for data extents, not metadata */
> + ASSERT((flags & (XFS_BMAPI_METADATA | XFS_BMAPI_ZERO)) !=
> + (XFS_BMAPI_METADATA | XFS_BMAPI_ZERO));
> + /*
> + * we can allocate unwritten extents or pre-zero allocated blocks,
> + * but it makes no sense to do both at once. This would result in
> + * zeroing the unwritten extent twice, but it still being an
> + * unwritten extent....
> + */
> + ASSERT((flags & (XFS_BMAPI_PREALLOC | XFS_BMAPI_ZERO)) !=
> + (XFS_BMAPI_PREALLOC | XFS_BMAPI_ZERO));
> +
> if (unlikely(XFS_TEST_ERROR(
> (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 6aaa0c1..a160f8a 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -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)
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index eca325e..dbae649 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -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);
> + 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);
> +
> + /*
> + * 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);
> +
> +}
> +
> +/*
> * 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;
> }
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 8795272..f20e5de 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -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__ */
> --
> 2.5.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/7] xfs: Don't use unwritten extents for DAX
2015-11-02 3:42 [PATCH 0/7] xfs: patches remaining for 4.4 merge window Dave Chinner
2015-11-02 3:42 ` [PATCH 1/7] xfs: fix inode size update overflow in xfs_map_direct() Dave Chinner
2015-11-02 3:42 ` [PATCH 2/7] xfs: introduce BMAPI_ZERO for allocating zeroed extents Dave Chinner
@ 2015-11-02 3:42 ` Dave Chinner
2015-11-02 14:15 ` Brian Foster
2015-11-02 3:42 ` [PATCH 4/7] xfs: DAX does not use IO completion callbacks Dave Chinner
` (3 subsequent siblings)
6 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2015-11-02 3:42 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
fs/dax.c | 5 +++++
fs/xfs/xfs_aops.c | 13 +++++++++----
fs/xfs/xfs_iomap.c | 21 ++++++++++++++++++++-
3 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index a86d3cc..131fd35a 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -29,6 +29,11 @@
#include <linux/uio.h>
#include <linux/vmstat.h>
+/*
+ * dax_clear_blocks() is called from within transaction context from XFS,
+ * and hence this means the stack from this point must follow GFP_NOFS
+ * semantics for all operations.
+ */
int dax_clear_blocks(struct inode *inode, sector_t block, long size)
{
struct block_device *bdev = inode->i_sb->s_bdev;
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 366e41eb..7b4f849 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -1293,15 +1293,12 @@ 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;
@@ -1429,10 +1426,12 @@ __xfs_get_blocks(
if (error)
goto out_unlock;
+ /* for DAX, we convert unwritten extents directly */
if (create &&
(!nimaps ||
(imap.br_startblock == HOLESTARTBLOCK ||
- imap.br_startblock == DELAYSTARTBLOCK))) {
+ imap.br_startblock == DELAYSTARTBLOCK) ||
+ (IS_DAX(inode) && ISUNWRITTEN(&imap)))) {
if (direct || xfs_get_extsz_hint(ip)) {
/*
* xfs_iomap_write_direct() expects the shared lock. It
@@ -1477,6 +1476,12 @@ __xfs_get_blocks(
goto out_unlock;
}
+ if (IS_DAX(inode) && create) {
+ ASSERT(!ISUNWRITTEN(&imap));
+ /* zeroing is not needed at a higher layer */
+ new = 0;
+ }
+
/* trim mapping down to size requested */
if (direct || size > (1 << inode->i_blkbits))
xfs_map_trim_size(inode, iblock, bh_result,
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index c3cb5a5..f4f5b43 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -132,6 +132,7 @@ xfs_iomap_write_direct(
int committed;
int error;
int lockmode;
+ int bmapi_flags = XFS_BMAPI_PREALLOC;
rt = XFS_IS_REALTIME_INODE(ip);
extsz = xfs_get_extsz_hint(ip);
@@ -195,6 +196,23 @@ xfs_iomap_write_direct(
* Allocate and setup the transaction
*/
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))) {
+ bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
+ tp->t_flags |= XFS_TRANS_RESERVE;
+ }
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;
--
2.5.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 3/7] xfs: Don't use unwritten extents for DAX
2015-11-02 3:42 ` [PATCH 3/7] xfs: Don't use unwritten extents for DAX Dave Chinner
@ 2015-11-02 14:15 ` Brian Foster
0 siblings, 0 replies; 11+ messages in thread
From: Brian Foster @ 2015-11-02 14:15 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Mon, Nov 02, 2015 at 02:42:11PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
> ---
I'm still not convinced that block zeroing is the right thing to do for
DAX/dio (re: the discussion on the previous version), but the code looks
fine to me and we should be able to easily optimize this in a separate
patch if warranted:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/dax.c | 5 +++++
> fs/xfs/xfs_aops.c | 13 +++++++++----
> fs/xfs/xfs_iomap.c | 21 ++++++++++++++++++++-
> 3 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/fs/dax.c b/fs/dax.c
> index a86d3cc..131fd35a 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -29,6 +29,11 @@
> #include <linux/uio.h>
> #include <linux/vmstat.h>
>
> +/*
> + * dax_clear_blocks() is called from within transaction context from XFS,
> + * and hence this means the stack from this point must follow GFP_NOFS
> + * semantics for all operations.
> + */
> int dax_clear_blocks(struct inode *inode, sector_t block, long size)
> {
> struct block_device *bdev = inode->i_sb->s_bdev;
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 366e41eb..7b4f849 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -1293,15 +1293,12 @@ 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;
> @@ -1429,10 +1426,12 @@ __xfs_get_blocks(
> if (error)
> goto out_unlock;
>
> + /* for DAX, we convert unwritten extents directly */
> if (create &&
> (!nimaps ||
> (imap.br_startblock == HOLESTARTBLOCK ||
> - imap.br_startblock == DELAYSTARTBLOCK))) {
> + imap.br_startblock == DELAYSTARTBLOCK) ||
> + (IS_DAX(inode) && ISUNWRITTEN(&imap)))) {
> if (direct || xfs_get_extsz_hint(ip)) {
> /*
> * xfs_iomap_write_direct() expects the shared lock. It
> @@ -1477,6 +1476,12 @@ __xfs_get_blocks(
> goto out_unlock;
> }
>
> + if (IS_DAX(inode) && create) {
> + ASSERT(!ISUNWRITTEN(&imap));
> + /* zeroing is not needed at a higher layer */
> + new = 0;
> + }
> +
> /* trim mapping down to size requested */
> if (direct || size > (1 << inode->i_blkbits))
> xfs_map_trim_size(inode, iblock, bh_result,
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index c3cb5a5..f4f5b43 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -132,6 +132,7 @@ xfs_iomap_write_direct(
> int committed;
> int error;
> int lockmode;
> + int bmapi_flags = XFS_BMAPI_PREALLOC;
>
> rt = XFS_IS_REALTIME_INODE(ip);
> extsz = xfs_get_extsz_hint(ip);
> @@ -195,6 +196,23 @@ xfs_iomap_write_direct(
> * Allocate and setup the transaction
> */
> 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))) {
> + bmapi_flags = XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO;
> + tp->t_flags |= XFS_TRANS_RESERVE;
> + }
> 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;
> --
> 2.5.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/7] xfs: DAX does not use IO completion callbacks
2015-11-02 3:42 [PATCH 0/7] xfs: patches remaining for 4.4 merge window Dave Chinner
` (2 preceding siblings ...)
2015-11-02 3:42 ` [PATCH 3/7] xfs: Don't use unwritten extents for DAX Dave Chinner
@ 2015-11-02 3:42 ` Dave Chinner
2015-11-02 3:42 ` [PATCH 5/7] xfs: add ->pfn_mkwrite support for DAX Dave Chinner
` (2 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2015-11-02 3:42 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
For DAX, we are now doing block zeroing during allocation. This
means we no longer need a special DAX fault IO completion callback
to do unwritten extent conversion. Because mmap never extends the
file size (it SEGVs the process) we don't need a callback to update
the file size, either. Hence we can remove the completion callbacks
from the __dax_fault and __dax_mkwrite calls.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Dave Chinner <david@fromorbit.com>
---
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(-)
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 7b4f849..29e7e5d 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -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,
diff --git a/fs/xfs/xfs_aops.h b/fs/xfs/xfs_aops.h
index d39ba25..f6ffc9a 100644
--- a/fs/xfs/xfs_aops.h
+++ b/fs/xfs/xfs_aops.h
@@ -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 *);
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 7f873bc..403151a 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -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);
--
2.5.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 5/7] xfs: add ->pfn_mkwrite support for DAX
2015-11-02 3:42 [PATCH 0/7] xfs: patches remaining for 4.4 merge window Dave Chinner
` (3 preceding siblings ...)
2015-11-02 3:42 ` [PATCH 4/7] xfs: DAX does not use IO completion callbacks Dave Chinner
@ 2015-11-02 3:42 ` Dave Chinner
2015-11-02 3:42 ` [PATCH 6/7] xfs: xfs_filemap_pmd_fault treats read faults as write faults Dave Chinner
2015-11-02 3:42 ` [PATCH 7/7] xfs: optimise away log forces on timestamp updates for fdatasync Dave Chinner
6 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2015-11-02 3:42 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
->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 <dchinner@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Dave Chinner <david@fromorbit.com>
---
fs/xfs/xfs_file.c | 35 +++++++++++++++++++++++++++++++++++
fs/xfs/xfs_trace.h | 1 +
2 files changed, 36 insertions(+)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 403151a..e7cf9ec 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -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
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 957f5cc..877079eb 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -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),
--
2.5.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 6/7] xfs: xfs_filemap_pmd_fault treats read faults as write faults
2015-11-02 3:42 [PATCH 0/7] xfs: patches remaining for 4.4 merge window Dave Chinner
` (4 preceding siblings ...)
2015-11-02 3:42 ` [PATCH 5/7] xfs: add ->pfn_mkwrite support for DAX Dave Chinner
@ 2015-11-02 3:42 ` Dave Chinner
2015-11-02 3:42 ` [PATCH 7/7] xfs: optimise away log forces on timestamp updates for fdatasync Dave Chinner
6 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2015-11-02 3:42 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Dave Chinner <david@fromorbit.com>
---
fs/xfs/xfs_file.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index e7cf9ec..0045b0a 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -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;
}
--
2.5.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 7/7] xfs: optimise away log forces on timestamp updates for fdatasync
2015-11-02 3:42 [PATCH 0/7] xfs: patches remaining for 4.4 merge window Dave Chinner
` (5 preceding siblings ...)
2015-11-02 3:42 ` [PATCH 6/7] xfs: xfs_filemap_pmd_fault treats read faults as write faults Dave Chinner
@ 2015-11-02 3:42 ` Dave Chinner
2015-11-02 14:15 ` Brian Foster
6 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2015-11-02 3:42 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
xfs: timestamp updates cause excessive fdatasync log traffic
Sage Weil reported that a ceph test workload was writing to the
log on every fdatasync during an overwrite workload. Event tracing
showed that the only metadata modification being made was the
timestamp updates during the write(2) syscall, but fdatasync(2)
is supposed to ignore them. The key observation was that the
transactions in the log all looked like this:
INODE: #regs: 4 ino: 0x8b flags: 0x45 dsize: 32
And contained a flags field of 0x45 or 0x85, and had data and
attribute forks following the inode core. This means that the
timestamp updates were triggering dirty relogging of previously
logged parts of the inode that hadn't yet been flushed back to
disk.
There are two parts to this problem. The first is that XFS relogs
dirty regions in subsequent transactions, so it carries around the
fields that have been dirtied since the last time the inode was
written back to disk, not since the last time the inode was forced
into the log.
The second part is that on v5 filesystems, the inode change count
update during inode dirtying also sets the XFS_ILOG_CORE flag, so
on v5 filesystems this makes a timestamp update dirty the entire
inode.
As a result when fdatasync is run, it looks at the dirty fields in
the inode, and sees more than just the timestamp flag, even though
the only metadata change since the last fdatasync was just the
timestamps. Hence we force the log on every subsequent fdatasync
even though it is not needed.
To fix this, add a new field to the inode log item that tracks
changes since the last time fsync/fdatasync forced the log to flush
the changes to the journal. This flag is updated when we dirty the
inode, but we do it before updating the change count so it does not
carry the "core dirty" flag from timestamp updates. The fields are
zeroed when the inode is marked clean (due to writeback/freeing) or
when an fsync/datasync forces the log. Hence if we only dirty the
timestamps on the inode between fsync/fdatasync calls, the fdatasync
will not trigger another log force.
Over 100 runs of the test program:
Ext4 baseline:
runtime: 1.63s +/- 0.24s
avg lat: 1.59ms +/- 0.24ms
iops: ~2000
XFS, vanilla kernel:
runtime: 2.45s +/- 0.18s
avg lat: 2.39ms +/- 0.18ms
log forces: ~400/s
iops: ~1000
XFS, patched kernel:
runtime: 1.49s +/- 0.26s
avg lat: 1.46ms +/- 0.25ms
log forces: ~30/s
iops: ~1500
Reported-by: Sage Weil <sage@redhat.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_file.c | 21 ++++++++++++++++-----
fs/xfs/xfs_inode.c | 2 ++
fs/xfs/xfs_inode_item.c | 1 +
fs/xfs/xfs_inode_item.h | 1 +
fs/xfs/xfs_trans_inode.c | 9 +++++++++
5 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 0045b0a..39743ef 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -242,19 +242,30 @@ xfs_file_fsync(
}
/*
- * All metadata updates are logged, which means that we just have
- * to flush the log up to the latest LSN that touched the inode.
+ * All metadata updates are logged, which means that we just have to
+ * flush the log up to the latest LSN that touched the inode. If we have
+ * concurrent fsync/fdatasync() calls, we need them to all block on the
+ * log force before we clear the ili_fsync_fields field. This ensures
+ * that we don't get a racing sync operation that does not wait for the
+ * metadata to hit the journal before returning. If we race with
+ * clearing the ili_fsync_fields, then all that will happen is the log
+ * force will do nothing as the lsn will already be on disk. We can't
+ * race with setting ili_fsync_fields because that is done under
+ * XFS_ILOCK_EXCL, and that can't happen because we hold the lock shared
+ * until after the ili_fsync_fields is cleared.
*/
xfs_ilock(ip, XFS_ILOCK_SHARED);
if (xfs_ipincount(ip)) {
if (!datasync ||
- (ip->i_itemp->ili_fields & ~XFS_ILOG_TIMESTAMP))
+ (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))
lsn = ip->i_itemp->ili_last_lsn;
}
- xfs_iunlock(ip, XFS_ILOCK_SHARED);
- if (lsn)
+ if (lsn) {
error = _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, &log_flushed);
+ ip->i_itemp->ili_fsync_fields = 0;
+ }
+ xfs_iunlock(ip, XFS_ILOCK_SHARED);
/*
* If we only have a single device, and the log force about was
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index a0f2bae..8ee3939 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2365,6 +2365,7 @@ retry:
iip->ili_last_fields = iip->ili_fields;
iip->ili_fields = 0;
+ iip->ili_fsync_fields = 0;
iip->ili_logged = 1;
xfs_trans_ail_copy_lsn(mp->m_ail, &iip->ili_flush_lsn,
&iip->ili_item.li_lsn);
@@ -3560,6 +3561,7 @@ xfs_iflush_int(
*/
iip->ili_last_fields = iip->ili_fields;
iip->ili_fields = 0;
+ iip->ili_fsync_fields = 0;
iip->ili_logged = 1;
xfs_trans_ail_copy_lsn(mp->m_ail, &iip->ili_flush_lsn,
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 62bd80f..d14b12b 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -719,6 +719,7 @@ xfs_iflush_abort(
* attempted.
*/
iip->ili_fields = 0;
+ iip->ili_fsync_fields = 0;
}
/*
* Release the inode's flush lock since we're done with it.
diff --git a/fs/xfs/xfs_inode_item.h b/fs/xfs/xfs_inode_item.h
index 488d812..4c7722e 100644
--- a/fs/xfs/xfs_inode_item.h
+++ b/fs/xfs/xfs_inode_item.h
@@ -34,6 +34,7 @@ typedef struct xfs_inode_log_item {
unsigned short ili_logged; /* flushed logged data */
unsigned int ili_last_fields; /* fields when flushed */
unsigned int ili_fields; /* fields to be logged */
+ unsigned int ili_fsync_fields; /* logged since last fsync */
} xfs_inode_log_item_t;
static inline int xfs_inode_clean(xfs_inode_t *ip)
diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
index 17280cd..b97f1df 100644
--- a/fs/xfs/xfs_trans_inode.c
+++ b/fs/xfs/xfs_trans_inode.c
@@ -108,6 +108,15 @@ xfs_trans_log_inode(
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
/*
+ * Record the specific change for fdatasync optimisation. This
+ * allows fdatasync to skip log forces for inodes that are only
+ * timestamp dirty. We do this before the change count so that
+ * the core being logged in this case does not impact on fdatasync
+ * behaviour.
+ */
+ ip->i_itemp->ili_fsync_fields |= flags;
+
+ /*
* First time we log the inode in a transaction, bump the inode change
* counter if it is configured for this to occur. We don't use
* inode_inc_version() because there is no need for extra locking around
--
2.5.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 7/7] xfs: optimise away log forces on timestamp updates for fdatasync
2015-11-02 3:42 ` [PATCH 7/7] xfs: optimise away log forces on timestamp updates for fdatasync Dave Chinner
@ 2015-11-02 14:15 ` Brian Foster
0 siblings, 0 replies; 11+ messages in thread
From: Brian Foster @ 2015-11-02 14:15 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Mon, Nov 02, 2015 at 02:42:15PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> xfs: timestamp updates cause excessive fdatasync log traffic
>
> Sage Weil reported that a ceph test workload was writing to the
> log on every fdatasync during an overwrite workload. Event tracing
> showed that the only metadata modification being made was the
> timestamp updates during the write(2) syscall, but fdatasync(2)
> is supposed to ignore them. The key observation was that the
> transactions in the log all looked like this:
>
> INODE: #regs: 4 ino: 0x8b flags: 0x45 dsize: 32
>
> And contained a flags field of 0x45 or 0x85, and had data and
> attribute forks following the inode core. This means that the
> timestamp updates were triggering dirty relogging of previously
> logged parts of the inode that hadn't yet been flushed back to
> disk.
>
> There are two parts to this problem. The first is that XFS relogs
> dirty regions in subsequent transactions, so it carries around the
> fields that have been dirtied since the last time the inode was
> written back to disk, not since the last time the inode was forced
> into the log.
>
> The second part is that on v5 filesystems, the inode change count
> update during inode dirtying also sets the XFS_ILOG_CORE flag, so
> on v5 filesystems this makes a timestamp update dirty the entire
> inode.
>
> As a result when fdatasync is run, it looks at the dirty fields in
> the inode, and sees more than just the timestamp flag, even though
> the only metadata change since the last fdatasync was just the
> timestamps. Hence we force the log on every subsequent fdatasync
> even though it is not needed.
>
> To fix this, add a new field to the inode log item that tracks
> changes since the last time fsync/fdatasync forced the log to flush
> the changes to the journal. This flag is updated when we dirty the
> inode, but we do it before updating the change count so it does not
> carry the "core dirty" flag from timestamp updates. The fields are
> zeroed when the inode is marked clean (due to writeback/freeing) or
> when an fsync/datasync forces the log. Hence if we only dirty the
> timestamps on the inode between fsync/fdatasync calls, the fdatasync
> will not trigger another log force.
>
> Over 100 runs of the test program:
>
> Ext4 baseline:
> runtime: 1.63s +/- 0.24s
> avg lat: 1.59ms +/- 0.24ms
> iops: ~2000
>
> XFS, vanilla kernel:
> runtime: 2.45s +/- 0.18s
> avg lat: 2.39ms +/- 0.18ms
> log forces: ~400/s
> iops: ~1000
>
> XFS, patched kernel:
> runtime: 1.49s +/- 0.26s
> avg lat: 1.46ms +/- 0.25ms
> log forces: ~30/s
> iops: ~1500
>
> Reported-by: Sage Weil <sage@redhat.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
Looks good:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_file.c | 21 ++++++++++++++++-----
> fs/xfs/xfs_inode.c | 2 ++
> fs/xfs/xfs_inode_item.c | 1 +
> fs/xfs/xfs_inode_item.h | 1 +
> fs/xfs/xfs_trans_inode.c | 9 +++++++++
> 5 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 0045b0a..39743ef 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -242,19 +242,30 @@ xfs_file_fsync(
> }
>
> /*
> - * All metadata updates are logged, which means that we just have
> - * to flush the log up to the latest LSN that touched the inode.
> + * All metadata updates are logged, which means that we just have to
> + * flush the log up to the latest LSN that touched the inode. If we have
> + * concurrent fsync/fdatasync() calls, we need them to all block on the
> + * log force before we clear the ili_fsync_fields field. This ensures
> + * that we don't get a racing sync operation that does not wait for the
> + * metadata to hit the journal before returning. If we race with
> + * clearing the ili_fsync_fields, then all that will happen is the log
> + * force will do nothing as the lsn will already be on disk. We can't
> + * race with setting ili_fsync_fields because that is done under
> + * XFS_ILOCK_EXCL, and that can't happen because we hold the lock shared
> + * until after the ili_fsync_fields is cleared.
> */
> xfs_ilock(ip, XFS_ILOCK_SHARED);
> if (xfs_ipincount(ip)) {
> if (!datasync ||
> - (ip->i_itemp->ili_fields & ~XFS_ILOG_TIMESTAMP))
> + (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))
> lsn = ip->i_itemp->ili_last_lsn;
> }
> - xfs_iunlock(ip, XFS_ILOCK_SHARED);
>
> - if (lsn)
> + if (lsn) {
> error = _xfs_log_force_lsn(mp, lsn, XFS_LOG_SYNC, &log_flushed);
> + ip->i_itemp->ili_fsync_fields = 0;
> + }
> + xfs_iunlock(ip, XFS_ILOCK_SHARED);
>
> /*
> * If we only have a single device, and the log force about was
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index a0f2bae..8ee3939 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2365,6 +2365,7 @@ retry:
>
> iip->ili_last_fields = iip->ili_fields;
> iip->ili_fields = 0;
> + iip->ili_fsync_fields = 0;
> iip->ili_logged = 1;
> xfs_trans_ail_copy_lsn(mp->m_ail, &iip->ili_flush_lsn,
> &iip->ili_item.li_lsn);
> @@ -3560,6 +3561,7 @@ xfs_iflush_int(
> */
> iip->ili_last_fields = iip->ili_fields;
> iip->ili_fields = 0;
> + iip->ili_fsync_fields = 0;
> iip->ili_logged = 1;
>
> xfs_trans_ail_copy_lsn(mp->m_ail, &iip->ili_flush_lsn,
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 62bd80f..d14b12b 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -719,6 +719,7 @@ xfs_iflush_abort(
> * attempted.
> */
> iip->ili_fields = 0;
> + iip->ili_fsync_fields = 0;
> }
> /*
> * Release the inode's flush lock since we're done with it.
> diff --git a/fs/xfs/xfs_inode_item.h b/fs/xfs/xfs_inode_item.h
> index 488d812..4c7722e 100644
> --- a/fs/xfs/xfs_inode_item.h
> +++ b/fs/xfs/xfs_inode_item.h
> @@ -34,6 +34,7 @@ typedef struct xfs_inode_log_item {
> unsigned short ili_logged; /* flushed logged data */
> unsigned int ili_last_fields; /* fields when flushed */
> unsigned int ili_fields; /* fields to be logged */
> + unsigned int ili_fsync_fields; /* logged since last fsync */
> } xfs_inode_log_item_t;
>
> static inline int xfs_inode_clean(xfs_inode_t *ip)
> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> index 17280cd..b97f1df 100644
> --- a/fs/xfs/xfs_trans_inode.c
> +++ b/fs/xfs/xfs_trans_inode.c
> @@ -108,6 +108,15 @@ xfs_trans_log_inode(
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> /*
> + * Record the specific change for fdatasync optimisation. This
> + * allows fdatasync to skip log forces for inodes that are only
> + * timestamp dirty. We do this before the change count so that
> + * the core being logged in this case does not impact on fdatasync
> + * behaviour.
> + */
> + ip->i_itemp->ili_fsync_fields |= flags;
> +
> + /*
> * First time we log the inode in a transaction, bump the inode change
> * counter if it is configured for this to occur. We don't use
> * inode_inc_version() because there is no need for extra locking around
> --
> 2.5.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 11+ messages in thread