* optimize the COW I/O path
@ 2016-10-10 13:37 Christoph Hellwig
2016-10-10 13:37 ` [PATCH 1/9] iomap: add IOMAP_REPORT Christoph Hellwig
` (8 more replies)
0 siblings, 9 replies; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-10 13:37 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
This series contains a couple optimizations for the COW I/O path:
- avoid any overhead for reads from COW files
- streamline the COW handling in write_begin
- reduce the number of extent lookups during COW I/O completion
It does not touch the direct I/O path (yet), I still have a few
unsolved problems there.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/9] iomap: add IOMAP_REPORT
2016-10-10 13:37 optimize the COW I/O path Christoph Hellwig
@ 2016-10-10 13:37 ` Christoph Hellwig
2016-10-11 1:45 ` Darrick J. Wong
2016-10-10 13:37 ` [PATCH 2/9] xfs: add xfs_trim_extent Christoph Hellwig
` (7 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-10 13:37 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
This allows the file system to tell a FIEMAP from a read operation, and thus
avoids the need to report flags that aren't actually used in the read path.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap.c | 2 +-
include/linux/iomap.h | 17 +++++++++++------
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/fs/iomap.c b/fs/iomap.c
index 013d1d3..a922040 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -561,7 +561,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
}
while (len > 0) {
- ret = iomap_apply(inode, start, len, 0, ops, &ctx,
+ ret = iomap_apply(inode, start, len, IOMAP_REPORT, ops, &ctx,
iomap_fiemap_actor);
/* inode with no (attribute) mapping will give ENOENT */
if (ret == -ENOENT)
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index e63e288..7892f55 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -19,11 +19,15 @@ struct vm_fault;
#define IOMAP_UNWRITTEN 0x04 /* blocks allocated @blkno in unwritten state */
/*
- * Flags for iomap mappings:
+ * Flags for all iomap mappings:
*/
-#define IOMAP_F_MERGED 0x01 /* contains multiple blocks/extents */
-#define IOMAP_F_SHARED 0x02 /* block shared with another file */
-#define IOMAP_F_NEW 0x04 /* blocks have been newly allocated */
+#define IOMAP_F_NEW 0x01 /* blocks have been newly allocated */
+
+/*
+ * Flags that only need to be reported for IOMAP_REPORT requests:
+ */
+#define IOMAP_F_MERGED 0x10 /* contains multiple blocks/extents */
+#define IOMAP_F_SHARED 0x20 /* block shared with another file */
/*
* Magic value for blkno:
@@ -42,8 +46,9 @@ struct iomap {
/*
* Flags for iomap_begin / iomap_end. No flag implies a read.
*/
-#define IOMAP_WRITE (1 << 0)
-#define IOMAP_ZERO (1 << 1)
+#define IOMAP_WRITE (1 << 0) /* writing, must allocate blocks */
+#define IOMAP_ZERO (1 << 1) /* zeroing operation, may skip holes */
+#define IOMAP_REPORT (1 << 2) /* report extent status, e.g. FIEMAP */
struct iomap_ops {
/*
--
2.10.1.382.ga23ca1b
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/9] xfs: add xfs_trim_extent
2016-10-10 13:37 optimize the COW I/O path Christoph Hellwig
2016-10-10 13:37 ` [PATCH 1/9] iomap: add IOMAP_REPORT Christoph Hellwig
@ 2016-10-10 13:37 ` Christoph Hellwig
2016-10-10 13:37 ` [PATCH 3/9] xfs: handle "raw" delayed extents xfs_reflink_trim_around_shared Christoph Hellwig
` (6 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-10 13:37 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
From: "Darrick J. Wong" <darrick.wong@oracle.com>
This helpers allows to trim an extent to a subset of it's original range
while making sure the block numbers in it remain valid,
In the future xfs_trim_extent and xfs_bmapi_trim_map should probably be
merged in some form.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
[hch: split from a previous patch from Darrick, moved around and added
support for "raw" delayed extents"]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_bmap.c | 33 +++++++++++++++++++++++++++++++++
fs/xfs/libxfs/xfs_bmap.h | 2 ++
2 files changed, 35 insertions(+)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index c27344c..016dacc 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3999,6 +3999,39 @@ xfs_bmap_alloc(
return xfs_bmap_btalloc(ap);
}
+/* Trim extent to fit a logical block range. */
+void
+xfs_trim_extent(
+ struct xfs_bmbt_irec *irec,
+ xfs_fileoff_t bno,
+ xfs_filblks_t len)
+{
+ xfs_fileoff_t distance;
+ xfs_fileoff_t end = bno + len;
+
+ if (irec->br_startoff + irec->br_blockcount <= bno ||
+ irec->br_startoff >= end) {
+ irec->br_blockcount = 0;
+ return;
+ }
+
+ if (irec->br_startoff < bno) {
+ distance = bno - irec->br_startoff;
+ if (isnullstartblock(irec->br_startblock))
+ irec->br_startblock = DELAYSTARTBLOCK;
+ if (irec->br_startblock != DELAYSTARTBLOCK &&
+ irec->br_startblock != HOLESTARTBLOCK)
+ irec->br_startblock += distance;
+ irec->br_startoff += distance;
+ irec->br_blockcount -= distance;
+ }
+
+ if (end < irec->br_startoff + irec->br_blockcount) {
+ distance = irec->br_startoff + irec->br_blockcount - end;
+ irec->br_blockcount -= distance;
+ }
+}
+
/*
* Trim the returned map to the required bounds
*/
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index f97db71..eb86af0 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -190,6 +190,8 @@ void xfs_bmap_trace_exlist(struct xfs_inode *ip, xfs_extnum_t cnt,
#define XFS_BMAP_TRACE_EXLIST(ip,c,w)
#endif
+void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
+ xfs_filblks_t len);
int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd);
void xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork);
void xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
--
2.10.1.382.ga23ca1b
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 3/9] xfs: handle "raw" delayed extents xfs_reflink_trim_around_shared
2016-10-10 13:37 optimize the COW I/O path Christoph Hellwig
2016-10-10 13:37 ` [PATCH 1/9] iomap: add IOMAP_REPORT Christoph Hellwig
2016-10-10 13:37 ` [PATCH 2/9] xfs: add xfs_trim_extent Christoph Hellwig
@ 2016-10-10 13:37 ` Christoph Hellwig
2016-10-10 13:38 ` [PATCH 4/9] xfs: don't bother looking at the refcount tree for reads Christoph Hellwig
` (5 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-10 13:37 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
Delalloc extents in the extent list contain the number of reserved
indirect blocks in their startblock value and don't use the magic
DELAYSTARTBLOCK constant. Ensure that xfs_reflink_trim_around_shared
handles them properly by checking for isnullstartblock().
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_reflink.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 5965e94..46c824c 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -182,7 +182,8 @@ xfs_reflink_trim_around_shared(
if (!xfs_is_reflink_inode(ip) ||
ISUNWRITTEN(irec) ||
irec->br_startblock == HOLESTARTBLOCK ||
- irec->br_startblock == DELAYSTARTBLOCK) {
+ irec->br_startblock == DELAYSTARTBLOCK ||
+ isnullstartblock(irec->br_startblock)) {
*shared = false;
return 0;
}
--
2.10.1.382.ga23ca1b
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 4/9] xfs: don't bother looking at the refcount tree for reads
2016-10-10 13:37 optimize the COW I/O path Christoph Hellwig
` (2 preceding siblings ...)
2016-10-10 13:37 ` [PATCH 3/9] xfs: handle "raw" delayed extents xfs_reflink_trim_around_shared Christoph Hellwig
@ 2016-10-10 13:38 ` Christoph Hellwig
2016-10-10 13:38 ` [PATCH 5/9] xfs: optimize writes to reflink files Christoph Hellwig
` (4 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-10 13:38 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
There is no need to trim an extent into a shared or non-shared one, or
report any flags for plain old reads.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_iomap.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index d907eb9..1dabf2e 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -996,11 +996,14 @@ xfs_file_iomap_begin(
return error;
}
- /* Trim the mapping to the nearest shared extent boundary. */
- error = xfs_reflink_trim_around_shared(ip, &imap, &shared, &trimmed);
- if (error) {
- xfs_iunlock(ip, lockmode);
- return error;
+ if (flags & (IOMAP_WRITE | IOMAP_ZERO | IOMAP_REPORT)) {
+ /* Trim the mapping to the nearest shared extent boundary. */
+ error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
+ &trimmed);
+ if (error) {
+ xfs_iunlock(ip, lockmode);
+ return error;
+ }
}
if ((flags & IOMAP_WRITE) && imap_needs_alloc(inode, &imap, nimaps)) {
--
2.10.1.382.ga23ca1b
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 5/9] xfs: optimize writes to reflink files
2016-10-10 13:37 optimize the COW I/O path Christoph Hellwig
` (3 preceding siblings ...)
2016-10-10 13:38 ` [PATCH 4/9] xfs: don't bother looking at the refcount tree for reads Christoph Hellwig
@ 2016-10-10 13:38 ` Christoph Hellwig
2016-10-12 14:12 ` Brian Foster
2016-10-10 13:38 ` [PATCH 6/9] xfs: refactor xfs_bunmapi_cow Christoph Hellwig
` (3 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-10 13:38 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
Instead of reserving space as the first thing in write_begin move it past
reading the extent in the data fork. That way we only have to read from
the data fork once and can reuse that information for trimming the extent
to the shared/unshared boundary. Additionally this allows to easily
limit the actual write size to said boundary, and avoid a roundtrip on the
ilock.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_iomap.c | 56 +++++++++++++-------
fs/xfs/xfs_reflink.c | 141 +++++++++++++++++++++------------------------------
fs/xfs/xfs_reflink.h | 4 +-
fs/xfs/xfs_trace.h | 3 +-
4 files changed, 100 insertions(+), 104 deletions(-)
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 1dabf2e..436e109 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -566,6 +566,17 @@ xfs_file_iomap_begin_delay(
xfs_bmap_search_extents(ip, offset_fsb, XFS_DATA_FORK, &eof, &idx,
&got, &prev);
if (!eof && got.br_startoff <= offset_fsb) {
+ if (xfs_is_reflink_inode(ip)) {
+ bool shared;
+
+ end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
+ maxbytes_fsb);
+ xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
+ error = xfs_reflink_reserve_cow(ip, &got, &shared);
+ if (error)
+ goto out_unlock;
+ }
+
trace_xfs_iomap_found(ip, offset, count, 0, &got);
goto done;
}
@@ -961,19 +972,13 @@ xfs_file_iomap_begin(
struct xfs_mount *mp = ip->i_mount;
struct xfs_bmbt_irec imap;
xfs_fileoff_t offset_fsb, end_fsb;
- bool shared, trimmed;
int nimaps = 1, error = 0;
+ bool shared = false, trimmed = false;
unsigned lockmode;
if (XFS_FORCED_SHUTDOWN(mp))
return -EIO;
- if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
- error = xfs_reflink_reserve_cow_range(ip, offset, length);
- if (error < 0)
- return error;
- }
-
if ((flags & IOMAP_WRITE) && !IS_DAX(inode) &&
!xfs_get_extsz_hint(ip)) {
/* Reserve delalloc blocks for regular writeback. */
@@ -981,7 +986,16 @@ xfs_file_iomap_begin(
iomap);
}
- lockmode = xfs_ilock_data_map_shared(ip);
+ /*
+ * COW writes will allocate delalloc space, so we need to make sure
+ * to take the lock exclusively here.
+ */
+ if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
+ lockmode = XFS_ILOCK_EXCL;
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
+ } else {
+ lockmode = xfs_ilock_data_map_shared(ip);
+ }
ASSERT(offset <= mp->m_super->s_maxbytes);
if ((xfs_fsize_t)offset + length > mp->m_super->s_maxbytes)
@@ -991,19 +1005,24 @@ xfs_file_iomap_begin(
error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
&nimaps, 0);
- if (error) {
- xfs_iunlock(ip, lockmode);
- return error;
- }
+ if (error)
+ goto out_unlock;
- if (flags & (IOMAP_WRITE | IOMAP_ZERO | IOMAP_REPORT)) {
+ if (flags & IOMAP_REPORT) {
/* Trim the mapping to the nearest shared extent boundary. */
error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
&trimmed);
- if (error) {
- xfs_iunlock(ip, lockmode);
- return error;
- }
+ if (error)
+ goto out_unlock;
+ }
+
+ if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
+ error = xfs_reflink_reserve_cow(ip, &imap, &shared);
+ if (error)
+ goto out_unlock;
+
+ end_fsb = imap.br_startoff + imap.br_blockcount;
+ length = XFS_FSB_TO_B(mp, end_fsb) - offset;
}
if ((flags & IOMAP_WRITE) && imap_needs_alloc(inode, &imap, nimaps)) {
@@ -1042,6 +1061,9 @@ xfs_file_iomap_begin(
if (shared)
iomap->flags |= IOMAP_F_SHARED;
return 0;
+out_unlock:
+ xfs_iunlock(ip, lockmode);
+ return error;
}
static int
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 46c824c..5d230ea 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -228,50 +228,54 @@ xfs_reflink_trim_around_shared(
}
}
-/* Create a CoW reservation for a range of blocks within a file. */
-static int
-__xfs_reflink_reserve_cow(
+/*
+ * Trim the passed in imap to the next shared/unshared extent boundary, and
+ * if imap->br_startoff points to a shared extent reserve space for it in the
+ * COW fork. In this case *shared is set to true, else to false.
+ *
+ * Note that imap will always contain the block numbers for the existing blocks
+ * in the data fork, as the upper layers need them for read-modify-write
+ * operations.
+ */
+int
+xfs_reflink_reserve_cow(
struct xfs_inode *ip,
- xfs_fileoff_t *offset_fsb,
- xfs_fileoff_t end_fsb,
- bool *skipped)
+ struct xfs_bmbt_irec *imap,
+ bool *shared)
{
- struct xfs_bmbt_irec got, prev, imap;
- xfs_fileoff_t orig_end_fsb;
- int nimaps, eof = 0, error = 0;
- bool shared = false, trimmed = false;
+ struct xfs_bmbt_irec got, prev;
+ xfs_fileoff_t end_fsb, orig_end_fsb;
+ int eof = 0, error = 0;
+ bool trimmed;
xfs_extnum_t idx;
xfs_extlen_t align;
- /* Already reserved? Skip the refcount btree access. */
- xfs_bmap_search_extents(ip, *offset_fsb, XFS_COW_FORK, &eof, &idx,
+ /*
+ * Search the COW fork extent list first. This serves two purposes:
+ * first this implement the speculative preallocation using cowextisze,
+ * so that we also unshared block adjacent to shared blocks instead
+ * of just the shared blocks themselves. Second the lookup in the
+ * extent list is generally faster than going out to the shared extent
+ * tree.
+ */
+ xfs_bmap_search_extents(ip, imap->br_startoff, XFS_COW_FORK, &eof, &idx,
&got, &prev);
- if (!eof && got.br_startoff <= *offset_fsb) {
- end_fsb = orig_end_fsb = got.br_startoff + got.br_blockcount;
- trace_xfs_reflink_cow_found(ip, &got);
- goto done;
- }
+ if (!eof && got.br_startoff <= imap->br_startoff) {
+ trace_xfs_reflink_cow_found(ip, imap);
+ xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
- /* Read extent from the source file. */
- nimaps = 1;
- error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
- &imap, &nimaps, 0);
- if (error)
- goto out_unlock;
- ASSERT(nimaps == 1);
+ *shared = true;
+ return 0;
+ }
/* Trim the mapping to the nearest shared extent boundary. */
- error = xfs_reflink_trim_around_shared(ip, &imap, &shared, &trimmed);
+ error = xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
if (error)
- goto out_unlock;
-
- end_fsb = orig_end_fsb = imap.br_startoff + imap.br_blockcount;
+ return error;
/* Not shared? Just report the (potentially capped) extent. */
- if (!shared) {
- *skipped = true;
- goto done;
- }
+ if (!*shared)
+ return 0;
/*
* Fork all the shared blocks from our write offset until the end of
@@ -279,73 +283,40 @@ __xfs_reflink_reserve_cow(
*/
error = xfs_qm_dqattach_locked(ip, 0);
if (error)
- goto out_unlock;
+ return error;
+
+ end_fsb = orig_end_fsb = imap->br_startoff + imap->br_blockcount;
align = xfs_eof_alignment(ip, xfs_get_cowextsz_hint(ip));
if (align)
end_fsb = roundup_64(end_fsb, align);
retry:
- error = xfs_bmapi_reserve_delalloc(ip, XFS_COW_FORK, *offset_fsb,
- end_fsb - *offset_fsb, &got,
- &prev, &idx, eof);
+ error = xfs_bmapi_reserve_delalloc(ip, XFS_COW_FORK, imap->br_startoff,
+ end_fsb - imap->br_startoff, &got, &prev, &idx, eof);
switch (error) {
case 0:
break;
case -ENOSPC:
case -EDQUOT:
/* retry without any preallocation */
- trace_xfs_reflink_cow_enospc(ip, &imap);
+ trace_xfs_reflink_cow_enospc(ip, imap);
if (end_fsb != orig_end_fsb) {
end_fsb = orig_end_fsb;
goto retry;
}
/*FALLTHRU*/
default:
- goto out_unlock;
+ return error;
}
if (end_fsb != orig_end_fsb)
xfs_inode_set_cowblocks_tag(ip);
trace_xfs_reflink_cow_alloc(ip, &got);
-done:
- *offset_fsb = end_fsb;
-out_unlock:
- return error;
+ return 0;
}
-/* Create a CoW reservation for part of a file. */
-int
-xfs_reflink_reserve_cow_range(
- struct xfs_inode *ip,
- xfs_off_t offset,
- xfs_off_t count)
-{
- struct xfs_mount *mp = ip->i_mount;
- xfs_fileoff_t offset_fsb, end_fsb;
- bool skipped = false;
- int error;
-
- trace_xfs_reflink_reserve_cow_range(ip, offset, count);
-
- offset_fsb = XFS_B_TO_FSBT(mp, offset);
- end_fsb = XFS_B_TO_FSB(mp, offset + count);
-
- xfs_ilock(ip, XFS_ILOCK_EXCL);
- while (offset_fsb < end_fsb) {
- error = __xfs_reflink_reserve_cow(ip, &offset_fsb, end_fsb,
- &skipped);
- if (error) {
- trace_xfs_reflink_reserve_cow_range_error(ip, error,
- _RET_IP_);
- break;
- }
- }
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
-
- return error;
-}
/* Allocate all CoW reservations covering a range of blocks in a file. */
static int
@@ -359,9 +330,8 @@ __xfs_reflink_allocate_cow(
struct xfs_defer_ops dfops;
struct xfs_trans *tp;
xfs_fsblock_t first_block;
- xfs_fileoff_t next_fsb;
int nimaps = 1, error;
- bool skipped = false;
+ bool shared;
xfs_defer_init(&dfops, &first_block);
@@ -372,33 +342,38 @@ __xfs_reflink_allocate_cow(
xfs_ilock(ip, XFS_ILOCK_EXCL);
- next_fsb = *offset_fsb;
- error = __xfs_reflink_reserve_cow(ip, &next_fsb, end_fsb, &skipped);
+ /* Read extent from the source file. */
+ nimaps = 1;
+ error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
+ &imap, &nimaps, 0);
+ if (error)
+ goto out_unlock;
+ ASSERT(nimaps == 1);
+
+ error = xfs_reflink_reserve_cow(ip, &imap, &shared);
if (error)
goto out_trans_cancel;
- if (skipped) {
- *offset_fsb = next_fsb;
+ if (!shared) {
+ *offset_fsb = imap.br_startoff + imap.br_blockcount;
goto out_trans_cancel;
}
xfs_trans_ijoin(tp, ip, 0);
- error = xfs_bmapi_write(tp, ip, *offset_fsb, next_fsb - *offset_fsb,
+ error = xfs_bmapi_write(tp, ip, imap.br_startoff, imap.br_blockcount,
XFS_BMAPI_COWFORK, &first_block,
XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK),
&imap, &nimaps, &dfops);
if (error)
goto out_trans_cancel;
- /* We might not have been able to map the whole delalloc extent */
- *offset_fsb = min(*offset_fsb + imap.br_blockcount, next_fsb);
-
error = xfs_defer_finish(&tp, &dfops, NULL);
if (error)
goto out_trans_cancel;
error = xfs_trans_commit(tp);
+ *offset_fsb = imap.br_startoff + imap.br_blockcount;
out_unlock:
xfs_iunlock(ip, XFS_ILOCK_EXCL);
return error;
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index 5dc3c8a..9f76924 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -26,8 +26,8 @@ extern int xfs_reflink_find_shared(struct xfs_mount *mp, xfs_agnumber_t agno,
extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
struct xfs_bmbt_irec *irec, bool *shared, bool *trimmed);
-extern int xfs_reflink_reserve_cow_range(struct xfs_inode *ip,
- xfs_off_t offset, xfs_off_t count);
+extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
+ struct xfs_bmbt_irec *imap, bool *shared);
extern int xfs_reflink_allocate_cow_range(struct xfs_inode *ip,
xfs_off_t offset, xfs_off_t count);
extern bool xfs_reflink_find_cow_mapping(struct xfs_inode *ip, xfs_off_t offset,
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 2586c9c..520660d 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -3347,7 +3347,7 @@ DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_alloc);
DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_found);
DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_enospc);
-DEFINE_RW_EVENT(xfs_reflink_reserve_cow_range);
+DEFINE_RW_EVENT(xfs_reflink_reserve_cow);
DEFINE_RW_EVENT(xfs_reflink_allocate_cow_range);
DEFINE_INODE_IREC_EVENT(xfs_reflink_bounce_dio_write);
@@ -3359,7 +3359,6 @@ DEFINE_SIMPLE_IO_EVENT(xfs_reflink_end_cow);
DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap);
DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap_piece);
-DEFINE_INODE_ERROR_EVENT(xfs_reflink_reserve_cow_range_error);
DEFINE_INODE_ERROR_EVENT(xfs_reflink_allocate_cow_range_error);
DEFINE_INODE_ERROR_EVENT(xfs_reflink_cancel_cow_range_error);
DEFINE_INODE_ERROR_EVENT(xfs_reflink_end_cow_error);
--
2.10.1.382.ga23ca1b
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 6/9] xfs: refactor xfs_bunmapi_cow
2016-10-10 13:37 optimize the COW I/O path Christoph Hellwig
` (4 preceding siblings ...)
2016-10-10 13:38 ` [PATCH 5/9] xfs: optimize writes to reflink files Christoph Hellwig
@ 2016-10-10 13:38 ` Christoph Hellwig
2016-10-12 14:13 ` Brian Foster
2016-10-10 13:38 ` [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks Christoph Hellwig
` (2 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-10 13:38 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
Split out two helpers for deleting delayed or real extents from the COW fork.
This allows to call them directly from xfs_reflink_cow_end_io once that
function is refactored to iterate the extent tree. It will also allow
to reuse the delalloc deletion from xfs_bunmapi in the future.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_bmap.c | 374 ++++++++++++++++++++++++++++-------------------
fs/xfs/libxfs/xfs_bmap.h | 5 +
2 files changed, 225 insertions(+), 154 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 016dacc..814980d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4862,6 +4862,219 @@ xfs_bmap_split_indlen(
return stolen;
}
+int
+xfs_bmap_del_extent_delay(
+ struct xfs_inode *ip,
+ int whichfork,
+ xfs_extnum_t *idx,
+ struct xfs_bmbt_irec *got,
+ struct xfs_bmbt_irec *del)
+{
+ struct xfs_mount *mp = ip->i_mount;
+ struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
+ struct xfs_bmbt_irec new;
+ int64_t da_old, da_new, da_diff = 0;
+ xfs_fileoff_t del_endoff, got_endoff;
+ xfs_filblks_t got_indlen, new_indlen, stolen;
+ int error = 0, state = 0;
+ bool isrt;
+
+ XFS_STATS_INC(mp, xs_del_exlist);
+
+ isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
+ del_endoff = del->br_startoff + del->br_blockcount;
+ got_endoff = got->br_startoff + got->br_blockcount;
+ da_old = startblockval(got->br_startblock);
+ da_new = 0;
+
+ ASSERT(*idx >= 0);
+ ASSERT(*idx < ifp->if_bytes / sizeof(struct xfs_bmbt_rec));
+ ASSERT(del->br_blockcount > 0);
+ ASSERT(got->br_startoff <= del->br_startoff);
+ ASSERT(got_endoff >= del_endoff);
+
+ if (isrt) {
+ int64_t rtexts = XFS_FSB_TO_B(mp, del->br_blockcount);
+
+ do_div(rtexts, mp->m_sb.sb_rextsize);
+ xfs_mod_frextents(mp, rtexts);
+ }
+
+ /*
+ * Update the inode delalloc counter now and wait to update the
+ * sb counters as we might have to borrow some blocks for the
+ * indirect block accounting.
+ */
+ xfs_trans_reserve_quota_nblks(NULL, ip, -((long)del->br_blockcount), 0,
+ isrt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
+ ip->i_delayed_blks -= del->br_blockcount;
+
+ if (whichfork == XFS_COW_FORK)
+ state |= BMAP_COWFORK;
+
+ if (got->br_startoff == del->br_startoff)
+ state |= BMAP_LEFT_CONTIG;
+ if (got_endoff == del_endoff)
+ state |= BMAP_RIGHT_CONTIG;
+
+ switch (state & (BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG)) {
+ case BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG:
+ /*
+ * Matches the whole extent. Delete the entry.
+ */
+ xfs_iext_remove(ip, *idx, 1, state);
+ --*idx;
+ break;
+ case BMAP_LEFT_CONTIG:
+ /*
+ * Deleting the first part of the extent.
+ */
+ trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+ got->br_startoff = del_endoff;
+ got->br_blockcount -= del->br_blockcount;
+ da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
+ got->br_blockcount), da_old);
+ got->br_startblock = nullstartblock((int)da_new);
+ xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+ trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
+ break;
+ case BMAP_RIGHT_CONTIG:
+ /*
+ * Deleting the last part of the extent.
+ */
+ trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+ got->br_blockcount = got->br_blockcount - del->br_blockcount;
+ da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
+ got->br_blockcount), da_old);
+ got->br_startblock = nullstartblock((int)da_new);
+ xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+ trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
+ break;
+ case 0:
+ /*
+ * Deleting the middle of the extent.
+ *
+ * Distribute the original indlen reservation across the two
+ * new extents. Steal blocks from the deleted extent if
+ * necessary. Stealing blocks simply fudges the fdblocks
+ * accounting in xfs_bunmapi().
+ */
+ trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+ got->br_blockcount = del->br_startoff - got->br_startoff;
+
+ got_indlen = xfs_bmap_worst_indlen(ip, got->br_blockcount);
+ new_indlen = xfs_bmap_worst_indlen(ip, new.br_blockcount);
+ stolen = xfs_bmap_split_indlen(da_old, &got_indlen, &new_indlen,
+ del->br_blockcount);
+ da_new = got_indlen + new_indlen - stolen;
+ del->br_blockcount -= stolen;
+
+ /*
+ * Set the reservation for each extent. Warn if either
+ * is zero as this can lead to delalloc problems.
+ */
+ WARN_ON_ONCE(!got_indlen || !new_indlen);
+ got->br_startblock = nullstartblock((int)got_indlen);
+ xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+ trace_xfs_bmap_post_update(ip, *idx, 0, _THIS_IP_);
+
+ new.br_startoff = del_endoff;
+ new.br_blockcount = got_endoff - del_endoff;
+ new.br_state = got->br_state;
+ new.br_startblock = nullstartblock((int)new_indlen);
+
+ ++*idx;
+ xfs_iext_insert(ip, *idx, 1, &new, state);
+ break;
+ }
+
+ ASSERT(da_old >= da_new);
+ da_diff = da_old - da_new;
+ if (!isrt)
+ da_diff += del->br_blockcount;
+ if (da_diff)
+ xfs_mod_fdblocks(mp, da_diff, false);
+ return error;
+}
+
+void
+xfs_bmap_del_extent_cow(
+ struct xfs_inode *ip,
+ xfs_extnum_t *idx,
+ struct xfs_bmbt_irec *got,
+ struct xfs_bmbt_irec *del)
+{
+ struct xfs_mount *mp = ip->i_mount;
+ struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+ struct xfs_bmbt_irec new;
+ xfs_fileoff_t del_endoff, got_endoff;
+ int state = BMAP_COWFORK;
+
+ XFS_STATS_INC(mp, xs_del_exlist);
+
+ del_endoff = del->br_startoff + del->br_blockcount;
+ got_endoff = got->br_startoff + got->br_blockcount;
+
+ ASSERT(*idx >= 0);
+ ASSERT(*idx < ifp->if_bytes / sizeof(struct xfs_bmbt_rec));
+ ASSERT(del->br_blockcount > 0);
+ ASSERT(got->br_startoff <= del->br_startoff);
+ ASSERT(got_endoff >= del_endoff);
+ ASSERT(!isnullstartblock(got->br_startblock));
+
+ if (got->br_startoff == del->br_startoff)
+ state |= BMAP_LEFT_CONTIG;
+ if (got_endoff == del_endoff)
+ state |= BMAP_RIGHT_CONTIG;
+
+ switch (state & (BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG)) {
+ case BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG:
+ /*
+ * Matches the whole extent. Delete the entry.
+ */
+ xfs_iext_remove(ip, *idx, 1, state);
+ --*idx;
+ break;
+ case BMAP_LEFT_CONTIG:
+ /*
+ * Deleting the first part of the extent.
+ */
+ trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+ got->br_startoff = del_endoff;
+ got->br_blockcount -= del->br_blockcount;
+ got->br_startblock = del->br_startblock + del->br_blockcount;
+ xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+ trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
+ break;
+ case BMAP_RIGHT_CONTIG:
+ /*
+ * Deleting the last part of the extent.
+ */
+ trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+ got->br_blockcount -= del->br_blockcount;
+ xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+ trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
+ break;
+ case 0:
+ /*
+ * Deleting the middle of the extent.
+ */
+ trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
+ got->br_blockcount = del->br_startoff - got->br_startoff;
+ xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
+ trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
+
+ new.br_startoff = del_endoff;
+ new.br_blockcount = got_endoff - del_endoff;
+ new.br_state = got->br_state;
+ new.br_startblock = del->br_startblock + del->br_blockcount;
+
+ ++*idx;
+ xfs_iext_insert(ip, *idx, 1, &new, state);
+ break;
+ }
+}
+
/*
* Called by xfs_bmapi to update file extent records and the btree
* after removing space (or undoing a delayed allocation).
@@ -5210,167 +5423,20 @@ xfs_bunmapi_cow(
struct xfs_inode *ip,
struct xfs_bmbt_irec *del)
{
- xfs_filblks_t da_new;
- xfs_filblks_t da_old;
- xfs_fsblock_t del_endblock = 0;
- xfs_fileoff_t del_endoff;
- int delay;
struct xfs_bmbt_rec_host *ep;
- int error;
struct xfs_bmbt_irec got;
- xfs_fileoff_t got_endoff;
- struct xfs_ifork *ifp;
- struct xfs_mount *mp;
- xfs_filblks_t nblks;
struct xfs_bmbt_irec new;
- /* REFERENCED */
- uint qfield;
- xfs_filblks_t temp;
- xfs_filblks_t temp2;
- int state = BMAP_COWFORK;
int eof;
xfs_extnum_t eidx;
- mp = ip->i_mount;
- XFS_STATS_INC(mp, xs_del_exlist);
-
ep = xfs_bmap_search_extents(ip, del->br_startoff, XFS_COW_FORK, &eof,
- &eidx, &got, &new);
-
- ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK); ifp = ifp;
- ASSERT((eidx >= 0) && (eidx < ifp->if_bytes /
- (uint)sizeof(xfs_bmbt_rec_t)));
- ASSERT(del->br_blockcount > 0);
- ASSERT(got.br_startoff <= del->br_startoff);
- del_endoff = del->br_startoff + del->br_blockcount;
- got_endoff = got.br_startoff + got.br_blockcount;
- ASSERT(got_endoff >= del_endoff);
- delay = isnullstartblock(got.br_startblock);
- ASSERT(isnullstartblock(del->br_startblock) == delay);
- qfield = 0;
- error = 0;
- /*
- * If deleting a real allocation, must free up the disk space.
- */
- if (!delay) {
- nblks = del->br_blockcount;
- qfield = XFS_TRANS_DQ_BCOUNT;
- /*
- * Set up del_endblock and cur for later.
- */
- del_endblock = del->br_startblock + del->br_blockcount;
- da_old = da_new = 0;
- } else {
- da_old = startblockval(got.br_startblock);
- da_new = 0;
- nblks = 0;
- }
- qfield = qfield;
- nblks = nblks;
-
- /*
- * Set flag value to use in switch statement.
- * Left-contig is 2, right-contig is 1.
- */
- switch (((got.br_startoff == del->br_startoff) << 1) |
- (got_endoff == del_endoff)) {
- case 3:
- /*
- * Matches the whole extent. Delete the entry.
- */
- xfs_iext_remove(ip, eidx, 1, BMAP_COWFORK);
- --eidx;
- break;
-
- case 2:
- /*
- * Deleting the first part of the extent.
- */
- trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
- xfs_bmbt_set_startoff(ep, del_endoff);
- temp = got.br_blockcount - del->br_blockcount;
- xfs_bmbt_set_blockcount(ep, temp);
- if (delay) {
- temp = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip, temp),
- da_old);
- xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
- trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
- da_new = temp;
- break;
- }
- xfs_bmbt_set_startblock(ep, del_endblock);
- trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
- break;
-
- case 1:
- /*
- * Deleting the last part of the extent.
- */
- temp = got.br_blockcount - del->br_blockcount;
- trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
- xfs_bmbt_set_blockcount(ep, temp);
- if (delay) {
- temp = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip, temp),
- da_old);
- xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
- trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
- da_new = temp;
- break;
- }
- trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
- break;
-
- case 0:
- /*
- * Deleting the middle of the extent.
- */
- temp = del->br_startoff - got.br_startoff;
- trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
- xfs_bmbt_set_blockcount(ep, temp);
- new.br_startoff = del_endoff;
- temp2 = got_endoff - del_endoff;
- new.br_blockcount = temp2;
- new.br_state = got.br_state;
- if (!delay) {
- new.br_startblock = del_endblock;
- } else {
- temp = xfs_bmap_worst_indlen(ip, temp);
- xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
- temp2 = xfs_bmap_worst_indlen(ip, temp2);
- new.br_startblock = nullstartblock((int)temp2);
- da_new = temp + temp2;
- while (da_new > da_old) {
- if (temp) {
- temp--;
- da_new--;
- xfs_bmbt_set_startblock(ep,
- nullstartblock((int)temp));
- }
- if (da_new == da_old)
- break;
- if (temp2) {
- temp2--;
- da_new--;
- new.br_startblock =
- nullstartblock((int)temp2);
- }
- }
- }
- trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
- xfs_iext_insert(ip, eidx + 1, 1, &new, state);
- ++eidx;
- break;
- }
-
- /*
- * Account for change in delayed indirect blocks.
- * Nothing to do for disk quota accounting here.
- */
- ASSERT(da_old >= da_new);
- if (da_old > da_new)
- xfs_mod_fdblocks(mp, (int64_t)(da_old - da_new), false);
-
- return error;
+ &eidx, &got, &new);
+ ASSERT(ep);
+ if (isnullstartblock(got.br_startblock))
+ xfs_bmap_del_extent_delay(ip, XFS_COW_FORK, &eidx, &got, del);
+ else
+ xfs_bmap_del_extent_cow(ip, &eidx, &got, del);
+ return 0;
}
/*
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index eb86af0..5f18248 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -224,6 +224,11 @@ int xfs_bunmapi(struct xfs_trans *tp, struct xfs_inode *ip,
xfs_extnum_t nexts, xfs_fsblock_t *firstblock,
struct xfs_defer_ops *dfops, int *done);
int xfs_bunmapi_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *del);
+int xfs_bmap_del_extent_delay(struct xfs_inode *ip, int whichfork,
+ xfs_extnum_t *idx, struct xfs_bmbt_irec *got,
+ struct xfs_bmbt_irec *del);
+void xfs_bmap_del_extent_cow(struct xfs_inode *ip, xfs_extnum_t *idx,
+ struct xfs_bmbt_irec *got, struct xfs_bmbt_irec *del);
int xfs_check_nostate_extents(struct xfs_ifork *ifp, xfs_extnum_t idx,
xfs_extnum_t num);
uint xfs_default_attroffset(struct xfs_inode *ip);
--
2.10.1.382.ga23ca1b
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks
2016-10-10 13:37 optimize the COW I/O path Christoph Hellwig
` (5 preceding siblings ...)
2016-10-10 13:38 ` [PATCH 6/9] xfs: refactor xfs_bunmapi_cow Christoph Hellwig
@ 2016-10-10 13:38 ` Christoph Hellwig
2016-10-12 14:13 ` Brian Foster
2016-10-10 13:38 ` [PATCH 8/9] xfs: optimize xfs_reflink_end_cow Christoph Hellwig
2016-10-10 13:38 ` [PATCH 9/9] xfs: remove xfs_bunmapi_cow Christoph Hellwig
8 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-10 13:38 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
Rewrite xfs_reflink_cancel_cow_blocks so that we only do a search for
the first extent in the extent list and then iterate over the remaining
extents using the extent index, passing the extent we operate on
directly to xfs_bmap_del_extent_delay or xfs_bmap_del_extent_cow instead
of going through xfs_bunmapi and doing yet another extent list lookup.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_reflink.c | 59 +++++++++++++++++++++++-----------------------------
1 file changed, 26 insertions(+), 33 deletions(-)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 5d230ea..08170bb 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -512,58 +512,52 @@ xfs_reflink_cancel_cow_blocks(
xfs_fileoff_t offset_fsb,
xfs_fileoff_t end_fsb)
{
- struct xfs_bmbt_irec irec;
- xfs_filblks_t count_fsb;
+ struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+ struct xfs_bmbt_irec got, prev, del;
+ xfs_extnum_t idx;
xfs_fsblock_t firstfsb;
struct xfs_defer_ops dfops;
- int error = 0;
- int nimaps;
+ int error = 0, eof = 0;
if (!xfs_is_reflink_inode(ip))
return 0;
- /* Go find the old extent in the CoW fork. */
- while (offset_fsb < end_fsb) {
- nimaps = 1;
- count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
- error = xfs_bmapi_read(ip, offset_fsb, count_fsb, &irec,
- &nimaps, XFS_BMAPI_COWFORK);
- if (error)
- break;
- ASSERT(nimaps == 1);
+ xfs_bmap_search_extents(ip, offset_fsb, XFS_COW_FORK, &eof, &idx,
+ &got, &prev);
+ if (eof)
+ return 0;
- trace_xfs_reflink_cancel_cow(ip, &irec);
+ do {
+ if (got.br_startoff >= end_fsb)
+ break;
- if (irec.br_startblock == DELAYSTARTBLOCK) {
- /* Free a delayed allocation. */
- xfs_mod_fdblocks(ip->i_mount, irec.br_blockcount,
- false);
- ip->i_delayed_blks -= irec.br_blockcount;
+ del = got;
+ xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
+ trace_xfs_reflink_cancel_cow(ip, &del);
- /* Remove the mapping from the CoW fork. */
- error = xfs_bunmapi_cow(ip, &irec);
+ if (isnullstartblock(del.br_startblock)) {
+ error = xfs_bmap_del_extent_delay(ip, XFS_COW_FORK,
+ &idx, &got, &del);
if (error)
break;
- } else if (irec.br_startblock == HOLESTARTBLOCK) {
- /* empty */
} else {
xfs_trans_ijoin(*tpp, ip, 0);
xfs_defer_init(&dfops, &firstfsb);
/* Free the CoW orphan record. */
error = xfs_refcount_free_cow_extent(ip->i_mount,
- &dfops, irec.br_startblock,
- irec.br_blockcount);
+ &dfops, del.br_startblock,
+ del.br_blockcount);
if (error)
break;
xfs_bmap_add_free(ip->i_mount, &dfops,
- irec.br_startblock, irec.br_blockcount,
+ del.br_startblock, del.br_blockcount,
NULL);
/* Update quota accounting */
xfs_trans_mod_dquot_byino(*tpp, ip, XFS_TRANS_DQ_BCOUNT,
- -(long)irec.br_blockcount);
+ -(long)del.br_blockcount);
/* Roll the transaction */
error = xfs_defer_finish(tpp, &dfops, ip);
@@ -573,14 +567,13 @@ xfs_reflink_cancel_cow_blocks(
}
/* Remove the mapping from the CoW fork. */
- error = xfs_bunmapi_cow(ip, &irec);
- if (error)
- break;
+ xfs_bmap_del_extent_cow(ip, &idx, &got, &del);
}
- /* Roll on... */
- offset_fsb = irec.br_startoff + irec.br_blockcount;
- }
+ if (++idx >= ifp->if_bytes / sizeof(struct xfs_bmbt_rec))
+ return 0;
+ xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
+ } while (1);
return error;
}
--
2.10.1.382.ga23ca1b
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 8/9] xfs: optimize xfs_reflink_end_cow
2016-10-10 13:37 optimize the COW I/O path Christoph Hellwig
` (6 preceding siblings ...)
2016-10-10 13:38 ` [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks Christoph Hellwig
@ 2016-10-10 13:38 ` Christoph Hellwig
2016-10-12 14:15 ` Brian Foster
2016-10-10 13:38 ` [PATCH 9/9] xfs: remove xfs_bunmapi_cow Christoph Hellwig
8 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-10 13:38 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
Instead of doing a full extent list search for each extent that is to be
deleted using xfs_bmapi_read and then doing another one inside of
xfs_bunmapi_cow use the same scheme that xfs_bumapi uses: look up the
last extent to be deleted and then use the extent index to walk downward
until we are outside the range to be deleted.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_reflink.c | 114 ++++++++++++++++++++++-----------------------------
fs/xfs/xfs_trace.h | 1 -
2 files changed, 49 insertions(+), 66 deletions(-)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 08170bb..c4e7521 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -637,25 +637,22 @@ xfs_reflink_end_cow(
xfs_off_t offset,
xfs_off_t count)
{
- struct xfs_bmbt_irec irec;
- struct xfs_bmbt_irec uirec;
+ struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+ struct xfs_bmbt_irec got, prev, del;
struct xfs_trans *tp;
xfs_fileoff_t offset_fsb;
xfs_fileoff_t end_fsb;
- xfs_filblks_t count_fsb;
xfs_fsblock_t firstfsb;
struct xfs_defer_ops dfops;
- int error;
+ int error, eof = 0;
unsigned int resblks;
- xfs_filblks_t ilen;
xfs_filblks_t rlen;
- int nimaps;
+ xfs_extnum_t idx;
trace_xfs_reflink_end_cow(ip, offset, count);
offset_fsb = XFS_B_TO_FSBT(ip->i_mount, offset);
end_fsb = XFS_B_TO_FSB(ip->i_mount, offset + count);
- count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
/* Start a rolling transaction to switch the mappings */
resblks = XFS_EXTENTADD_SPACE_RES(ip->i_mount, XFS_DATA_FORK);
@@ -667,73 +664,61 @@ xfs_reflink_end_cow(
xfs_ilock(ip, XFS_ILOCK_EXCL);
xfs_trans_ijoin(tp, ip, 0);
- /* Go find the old extent in the CoW fork. */
- while (offset_fsb < end_fsb) {
- /* Read extent from the source file */
- nimaps = 1;
- count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
- error = xfs_bmapi_read(ip, offset_fsb, count_fsb, &irec,
- &nimaps, XFS_BMAPI_COWFORK);
- if (error)
- goto out_cancel;
- ASSERT(nimaps == 1);
-
- ASSERT(irec.br_startblock != DELAYSTARTBLOCK);
- trace_xfs_reflink_cow_remap(ip, &irec);
+ xfs_bmap_search_extents(ip, end_fsb - 1, XFS_COW_FORK, &eof, &idx,
+ &got, &prev);
+ if (eof) {
+ ASSERT(idx > 0);
+ xfs_bmbt_get_all(xfs_iext_get_ext(ifp, --idx), &got);
+ }
- /*
- * We can have a hole in the CoW fork if part of a directio
- * write is CoW but part of it isn't.
- */
- rlen = ilen = irec.br_blockcount;
- if (irec.br_startblock == HOLESTARTBLOCK)
+ do {
+ ASSERT(!isnullstartblock(got.br_startblock));
+ del = got;
+ xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
+ if (!del.br_blockcount)
goto next_extent;
/* Unmap the old blocks in the data fork. */
- while (rlen) {
- xfs_defer_init(&dfops, &firstfsb);
- error = __xfs_bunmapi(tp, ip, irec.br_startoff,
- &rlen, 0, 1, &firstfsb, &dfops);
- if (error)
- goto out_defer;
-
- /*
- * Trim the extent to whatever got unmapped.
- * Remember, bunmapi works backwards.
- */
- uirec.br_startblock = irec.br_startblock + rlen;
- uirec.br_startoff = irec.br_startoff + rlen;
- uirec.br_blockcount = irec.br_blockcount - rlen;
- irec.br_blockcount = rlen;
- trace_xfs_reflink_cow_remap_piece(ip, &uirec);
+ xfs_defer_init(&dfops, &firstfsb);
+ rlen = del.br_blockcount;
+ error = __xfs_bunmapi(tp, ip, del.br_startoff, &rlen, 0, 1,
+ &firstfsb, &dfops);
+ if (error)
+ goto out_defer;
- /* Free the CoW orphan record. */
- error = xfs_refcount_free_cow_extent(tp->t_mountp,
- &dfops, uirec.br_startblock,
- uirec.br_blockcount);
- if (error)
- goto out_defer;
+ /* Trim the extent to whatever got unmapped. */
+ if (rlen) {
+ xfs_trim_extent(&del, del.br_startoff + rlen,
+ del.br_blockcount - rlen);
+ }
+ trace_xfs_reflink_cow_remap(ip, &del);
- /* Map the new blocks into the data fork. */
- error = xfs_bmap_map_extent(tp->t_mountp, &dfops,
- ip, &uirec);
- if (error)
- goto out_defer;
+ /* Free the CoW orphan record. */
+ error = xfs_refcount_free_cow_extent(tp->t_mountp, &dfops,
+ del.br_startblock, del.br_blockcount);
+ if (error)
+ goto out_defer;
- /* Remove the mapping from the CoW fork. */
- error = xfs_bunmapi_cow(ip, &uirec);
- if (error)
- goto out_defer;
+ /* Map the new blocks into the data fork. */
+ error = xfs_bmap_map_extent(tp->t_mountp, &dfops, ip, &del);
+ if (error)
+ goto out_defer;
- error = xfs_defer_finish(&tp, &dfops, ip);
- if (error)
- goto out_defer;
- }
+ /* Remove the mapping from the CoW fork. */
+ xfs_bmap_del_extent_cow(ip, &idx, &got, &del);
+
+ error = xfs_defer_finish(&tp, &dfops, ip);
+ if (error)
+ goto out_defer;
next_extent:
- /* Roll on... */
- offset_fsb = irec.br_startoff + ilen;
- }
+ if (del.br_startoff == offset_fsb || idx < 0)
+ break;
+
+ xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
+ if (got.br_startoff >= del.br_startoff && --idx >= 0)
+ xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
+ } while (got.br_startoff < del.br_startoff);
error = xfs_trans_commit(tp);
xfs_iunlock(ip, XFS_ILOCK_EXCL);
@@ -743,7 +728,6 @@ next_extent:
out_defer:
xfs_defer_cancel(&dfops);
-out_cancel:
xfs_trans_cancel(tp);
xfs_iunlock(ip, XFS_ILOCK_EXCL);
out:
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 520660d..f3b4418 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -3357,7 +3357,6 @@ DEFINE_INODE_IREC_EVENT(xfs_reflink_trim_irec);
DEFINE_SIMPLE_IO_EVENT(xfs_reflink_cancel_cow_range);
DEFINE_SIMPLE_IO_EVENT(xfs_reflink_end_cow);
DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap);
-DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap_piece);
DEFINE_INODE_ERROR_EVENT(xfs_reflink_allocate_cow_range_error);
DEFINE_INODE_ERROR_EVENT(xfs_reflink_cancel_cow_range_error);
--
2.10.1.382.ga23ca1b
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 9/9] xfs: remove xfs_bunmapi_cow
2016-10-10 13:37 optimize the COW I/O path Christoph Hellwig
` (7 preceding siblings ...)
2016-10-10 13:38 ` [PATCH 8/9] xfs: optimize xfs_reflink_end_cow Christoph Hellwig
@ 2016-10-10 13:38 ` Christoph Hellwig
8 siblings, 0 replies; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-10 13:38 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
Since no one uses it anymore.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_bmap.c | 22 ----------------------
fs/xfs/libxfs/xfs_bmap.h | 1 -
2 files changed, 23 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 814980d..680fa9d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5417,28 +5417,6 @@ done:
return error;
}
-/* Remove an extent from the CoW fork. Similar to xfs_bmap_del_extent. */
-int
-xfs_bunmapi_cow(
- struct xfs_inode *ip,
- struct xfs_bmbt_irec *del)
-{
- struct xfs_bmbt_rec_host *ep;
- struct xfs_bmbt_irec got;
- struct xfs_bmbt_irec new;
- int eof;
- xfs_extnum_t eidx;
-
- ep = xfs_bmap_search_extents(ip, del->br_startoff, XFS_COW_FORK, &eof,
- &eidx, &got, &new);
- ASSERT(ep);
- if (isnullstartblock(got.br_startblock))
- xfs_bmap_del_extent_delay(ip, XFS_COW_FORK, &eidx, &got, del);
- else
- xfs_bmap_del_extent_cow(ip, &eidx, &got, del);
- return 0;
-}
-
/*
* Unmap (remove) blocks from a file.
* If nexts is nonzero then the number of extents to remove is limited to
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 5f18248..7cae6ec 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -223,7 +223,6 @@ int xfs_bunmapi(struct xfs_trans *tp, struct xfs_inode *ip,
xfs_fileoff_t bno, xfs_filblks_t len, int flags,
xfs_extnum_t nexts, xfs_fsblock_t *firstblock,
struct xfs_defer_ops *dfops, int *done);
-int xfs_bunmapi_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *del);
int xfs_bmap_del_extent_delay(struct xfs_inode *ip, int whichfork,
xfs_extnum_t *idx, struct xfs_bmbt_irec *got,
struct xfs_bmbt_irec *del);
--
2.10.1.382.ga23ca1b
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/9] iomap: add IOMAP_REPORT
2016-10-10 13:37 ` [PATCH 1/9] iomap: add IOMAP_REPORT Christoph Hellwig
@ 2016-10-11 1:45 ` Darrick J. Wong
2016-10-11 4:58 ` Christoph Hellwig
[not found] ` <20161011144557.GA16368@lst.de>
0 siblings, 2 replies; 25+ messages in thread
From: Darrick J. Wong @ 2016-10-11 1:45 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Mon, Oct 10, 2016 at 03:37:57PM +0200, Christoph Hellwig wrote:
> This allows the file system to tell a FIEMAP from a read operation, and thus
> avoids the need to report flags that aren't actually used in the read path.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/iomap.c | 2 +-
> include/linux/iomap.h | 17 +++++++++++------
> 2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 013d1d3..a922040 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -561,7 +561,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> }
>
> while (len > 0) {
> - ret = iomap_apply(inode, start, len, 0, ops, &ctx,
> + ret = iomap_apply(inode, start, len, IOMAP_REPORT, ops, &ctx,
Hmm. /me notices that generic/352, generic/353, and generic/372 fail after
this patchset is applied. Will have a look at this tomorrow, unless this rings
a bell?
--D
> iomap_fiemap_actor);
> /* inode with no (attribute) mapping will give ENOENT */
> if (ret == -ENOENT)
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index e63e288..7892f55 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -19,11 +19,15 @@ struct vm_fault;
> #define IOMAP_UNWRITTEN 0x04 /* blocks allocated @blkno in unwritten state */
>
> /*
> - * Flags for iomap mappings:
> + * Flags for all iomap mappings:
> */
> -#define IOMAP_F_MERGED 0x01 /* contains multiple blocks/extents */
> -#define IOMAP_F_SHARED 0x02 /* block shared with another file */
> -#define IOMAP_F_NEW 0x04 /* blocks have been newly allocated */
> +#define IOMAP_F_NEW 0x01 /* blocks have been newly allocated */
> +
> +/*
> + * Flags that only need to be reported for IOMAP_REPORT requests:
> + */
> +#define IOMAP_F_MERGED 0x10 /* contains multiple blocks/extents */
> +#define IOMAP_F_SHARED 0x20 /* block shared with another file */
>
> /*
> * Magic value for blkno:
> @@ -42,8 +46,9 @@ struct iomap {
> /*
> * Flags for iomap_begin / iomap_end. No flag implies a read.
> */
> -#define IOMAP_WRITE (1 << 0)
> -#define IOMAP_ZERO (1 << 1)
> +#define IOMAP_WRITE (1 << 0) /* writing, must allocate blocks */
> +#define IOMAP_ZERO (1 << 1) /* zeroing operation, may skip holes */
> +#define IOMAP_REPORT (1 << 2) /* report extent status, e.g. FIEMAP */
>
> struct iomap_ops {
> /*
> --
> 2.10.1.382.ga23ca1b
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/9] iomap: add IOMAP_REPORT
2016-10-11 1:45 ` Darrick J. Wong
@ 2016-10-11 4:58 ` Christoph Hellwig
[not found] ` <20161011144557.GA16368@lst.de>
1 sibling, 0 replies; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-11 4:58 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs
On Mon, Oct 10, 2016 at 06:45:46PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 10, 2016 at 03:37:57PM +0200, Christoph Hellwig wrote:
> > This allows the file system to tell a FIEMAP from a read operation, and thus
> > avoids the need to report flags that aren't actually used in the read path.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> > fs/iomap.c | 2 +-
> > include/linux/iomap.h | 17 +++++++++++------
> > 2 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/iomap.c b/fs/iomap.c
> > index 013d1d3..a922040 100644
> > --- a/fs/iomap.c
> > +++ b/fs/iomap.c
> > @@ -561,7 +561,7 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
> > }
> >
> > while (len > 0) {
> > - ret = iomap_apply(inode, start, len, 0, ops, &ctx,
> > + ret = iomap_apply(inode, start, len, IOMAP_REPORT, ops, &ctx,
>
> Hmm. /me notices that generic/352, generic/353, and generic/372 fail after
> this patchset is applied. Will have a look at this tomorrow, unless this rings
> a bell?
I don't see any of those failures.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/9] iomap: add IOMAP_REPORT
[not found] ` <20161011144557.GA16368@lst.de>
@ 2016-10-11 16:22 ` Darrick J. Wong
0 siblings, 0 replies; 25+ messages in thread
From: Darrick J. Wong @ 2016-10-11 16:22 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Tue, Oct 11, 2016 at 04:45:57PM +0200, Christoph Hellwig wrote:
> Just curious: did you maybe manage to only rebuild the xfs module
> and not the core code and have a mismatch of the flags values?
D'oh! Yes, I forgot to reboot the VMs to pick up the new vmlinux. Everything
checks out now.
--D
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/9] xfs: optimize writes to reflink files
2016-10-10 13:38 ` [PATCH 5/9] xfs: optimize writes to reflink files Christoph Hellwig
@ 2016-10-12 14:12 ` Brian Foster
2016-10-13 6:49 ` Christoph Hellwig
0 siblings, 1 reply; 25+ messages in thread
From: Brian Foster @ 2016-10-12 14:12 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, darrick.wong
On Mon, Oct 10, 2016 at 03:38:01PM +0200, Christoph Hellwig wrote:
> Instead of reserving space as the first thing in write_begin move it past
> reading the extent in the data fork. That way we only have to read from
> the data fork once and can reuse that information for trimming the extent
> to the shared/unshared boundary. Additionally this allows to easily
> limit the actual write size to said boundary, and avoid a roundtrip on the
> ilock.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_iomap.c | 56 +++++++++++++-------
> fs/xfs/xfs_reflink.c | 141 +++++++++++++++++++++------------------------------
> fs/xfs/xfs_reflink.h | 4 +-
> fs/xfs/xfs_trace.h | 3 +-
> 4 files changed, 100 insertions(+), 104 deletions(-)
>
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 1dabf2e..436e109 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -566,6 +566,17 @@ xfs_file_iomap_begin_delay(
> xfs_bmap_search_extents(ip, offset_fsb, XFS_DATA_FORK, &eof, &idx,
> &got, &prev);
> if (!eof && got.br_startoff <= offset_fsb) {
> + if (xfs_is_reflink_inode(ip)) {
> + bool shared;
> +
> + end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
> + maxbytes_fsb);
> + xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
> + error = xfs_reflink_reserve_cow(ip, &got, &shared);
> + if (error)
> + goto out_unlock;
All in all this seems fine, but I don't see why we need to get all the
way down through xfs_reflink_reserve_cow() ->
xfs_reflink_trim_around_shared() to handle the basic delalloc overwrite
case on a reflink inode. Could we enhance the is_reflink_inode() helper
or create a new one that can consider whether the data fork extent is a
hole or delalloc?
Otherwise looks Ok to me:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> + }
> +
> trace_xfs_iomap_found(ip, offset, count, 0, &got);
> goto done;
> }
> @@ -961,19 +972,13 @@ xfs_file_iomap_begin(
> struct xfs_mount *mp = ip->i_mount;
> struct xfs_bmbt_irec imap;
> xfs_fileoff_t offset_fsb, end_fsb;
> - bool shared, trimmed;
> int nimaps = 1, error = 0;
> + bool shared = false, trimmed = false;
> unsigned lockmode;
>
> if (XFS_FORCED_SHUTDOWN(mp))
> return -EIO;
>
> - if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
> - error = xfs_reflink_reserve_cow_range(ip, offset, length);
> - if (error < 0)
> - return error;
> - }
> -
> if ((flags & IOMAP_WRITE) && !IS_DAX(inode) &&
> !xfs_get_extsz_hint(ip)) {
> /* Reserve delalloc blocks for regular writeback. */
> @@ -981,7 +986,16 @@ xfs_file_iomap_begin(
> iomap);
> }
>
> - lockmode = xfs_ilock_data_map_shared(ip);
> + /*
> + * COW writes will allocate delalloc space, so we need to make sure
> + * to take the lock exclusively here.
> + */
> + if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
> + lockmode = XFS_ILOCK_EXCL;
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> + } else {
> + lockmode = xfs_ilock_data_map_shared(ip);
> + }
>
> ASSERT(offset <= mp->m_super->s_maxbytes);
> if ((xfs_fsize_t)offset + length > mp->m_super->s_maxbytes)
> @@ -991,19 +1005,24 @@ xfs_file_iomap_begin(
>
> error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
> &nimaps, 0);
> - if (error) {
> - xfs_iunlock(ip, lockmode);
> - return error;
> - }
> + if (error)
> + goto out_unlock;
>
> - if (flags & (IOMAP_WRITE | IOMAP_ZERO | IOMAP_REPORT)) {
> + if (flags & IOMAP_REPORT) {
> /* Trim the mapping to the nearest shared extent boundary. */
> error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
> &trimmed);
> - if (error) {
> - xfs_iunlock(ip, lockmode);
> - return error;
> - }
> + if (error)
> + goto out_unlock;
> + }
> +
> + if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
> + error = xfs_reflink_reserve_cow(ip, &imap, &shared);
> + if (error)
> + goto out_unlock;
> +
> + end_fsb = imap.br_startoff + imap.br_blockcount;
> + length = XFS_FSB_TO_B(mp, end_fsb) - offset;
> }
>
> if ((flags & IOMAP_WRITE) && imap_needs_alloc(inode, &imap, nimaps)) {
> @@ -1042,6 +1061,9 @@ xfs_file_iomap_begin(
> if (shared)
> iomap->flags |= IOMAP_F_SHARED;
> return 0;
> +out_unlock:
> + xfs_iunlock(ip, lockmode);
> + return error;
> }
>
> static int
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 46c824c..5d230ea 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -228,50 +228,54 @@ xfs_reflink_trim_around_shared(
> }
> }
>
> -/* Create a CoW reservation for a range of blocks within a file. */
> -static int
> -__xfs_reflink_reserve_cow(
> +/*
> + * Trim the passed in imap to the next shared/unshared extent boundary, and
> + * if imap->br_startoff points to a shared extent reserve space for it in the
> + * COW fork. In this case *shared is set to true, else to false.
> + *
> + * Note that imap will always contain the block numbers for the existing blocks
> + * in the data fork, as the upper layers need them for read-modify-write
> + * operations.
> + */
> +int
> +xfs_reflink_reserve_cow(
> struct xfs_inode *ip,
> - xfs_fileoff_t *offset_fsb,
> - xfs_fileoff_t end_fsb,
> - bool *skipped)
> + struct xfs_bmbt_irec *imap,
> + bool *shared)
> {
> - struct xfs_bmbt_irec got, prev, imap;
> - xfs_fileoff_t orig_end_fsb;
> - int nimaps, eof = 0, error = 0;
> - bool shared = false, trimmed = false;
> + struct xfs_bmbt_irec got, prev;
> + xfs_fileoff_t end_fsb, orig_end_fsb;
> + int eof = 0, error = 0;
> + bool trimmed;
> xfs_extnum_t idx;
> xfs_extlen_t align;
>
> - /* Already reserved? Skip the refcount btree access. */
> - xfs_bmap_search_extents(ip, *offset_fsb, XFS_COW_FORK, &eof, &idx,
> + /*
> + * Search the COW fork extent list first. This serves two purposes:
> + * first this implement the speculative preallocation using cowextisze,
> + * so that we also unshared block adjacent to shared blocks instead
> + * of just the shared blocks themselves. Second the lookup in the
> + * extent list is generally faster than going out to the shared extent
> + * tree.
> + */
> + xfs_bmap_search_extents(ip, imap->br_startoff, XFS_COW_FORK, &eof, &idx,
> &got, &prev);
> - if (!eof && got.br_startoff <= *offset_fsb) {
> - end_fsb = orig_end_fsb = got.br_startoff + got.br_blockcount;
> - trace_xfs_reflink_cow_found(ip, &got);
> - goto done;
> - }
> + if (!eof && got.br_startoff <= imap->br_startoff) {
> + trace_xfs_reflink_cow_found(ip, imap);
> + xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
>
> - /* Read extent from the source file. */
> - nimaps = 1;
> - error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
> - &imap, &nimaps, 0);
> - if (error)
> - goto out_unlock;
> - ASSERT(nimaps == 1);
> + *shared = true;
> + return 0;
> + }
>
> /* Trim the mapping to the nearest shared extent boundary. */
> - error = xfs_reflink_trim_around_shared(ip, &imap, &shared, &trimmed);
> + error = xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
> if (error)
> - goto out_unlock;
> -
> - end_fsb = orig_end_fsb = imap.br_startoff + imap.br_blockcount;
> + return error;
>
> /* Not shared? Just report the (potentially capped) extent. */
> - if (!shared) {
> - *skipped = true;
> - goto done;
> - }
> + if (!*shared)
> + return 0;
>
> /*
> * Fork all the shared blocks from our write offset until the end of
> @@ -279,73 +283,40 @@ __xfs_reflink_reserve_cow(
> */
> error = xfs_qm_dqattach_locked(ip, 0);
> if (error)
> - goto out_unlock;
> + return error;
> +
> + end_fsb = orig_end_fsb = imap->br_startoff + imap->br_blockcount;
>
> align = xfs_eof_alignment(ip, xfs_get_cowextsz_hint(ip));
> if (align)
> end_fsb = roundup_64(end_fsb, align);
>
> retry:
> - error = xfs_bmapi_reserve_delalloc(ip, XFS_COW_FORK, *offset_fsb,
> - end_fsb - *offset_fsb, &got,
> - &prev, &idx, eof);
> + error = xfs_bmapi_reserve_delalloc(ip, XFS_COW_FORK, imap->br_startoff,
> + end_fsb - imap->br_startoff, &got, &prev, &idx, eof);
> switch (error) {
> case 0:
> break;
> case -ENOSPC:
> case -EDQUOT:
> /* retry without any preallocation */
> - trace_xfs_reflink_cow_enospc(ip, &imap);
> + trace_xfs_reflink_cow_enospc(ip, imap);
> if (end_fsb != orig_end_fsb) {
> end_fsb = orig_end_fsb;
> goto retry;
> }
> /*FALLTHRU*/
> default:
> - goto out_unlock;
> + return error;
> }
>
> if (end_fsb != orig_end_fsb)
> xfs_inode_set_cowblocks_tag(ip);
>
> trace_xfs_reflink_cow_alloc(ip, &got);
> -done:
> - *offset_fsb = end_fsb;
> -out_unlock:
> - return error;
> + return 0;
> }
>
> -/* Create a CoW reservation for part of a file. */
> -int
> -xfs_reflink_reserve_cow_range(
> - struct xfs_inode *ip,
> - xfs_off_t offset,
> - xfs_off_t count)
> -{
> - struct xfs_mount *mp = ip->i_mount;
> - xfs_fileoff_t offset_fsb, end_fsb;
> - bool skipped = false;
> - int error;
> -
> - trace_xfs_reflink_reserve_cow_range(ip, offset, count);
> -
> - offset_fsb = XFS_B_TO_FSBT(mp, offset);
> - end_fsb = XFS_B_TO_FSB(mp, offset + count);
> -
> - xfs_ilock(ip, XFS_ILOCK_EXCL);
> - while (offset_fsb < end_fsb) {
> - error = __xfs_reflink_reserve_cow(ip, &offset_fsb, end_fsb,
> - &skipped);
> - if (error) {
> - trace_xfs_reflink_reserve_cow_range_error(ip, error,
> - _RET_IP_);
> - break;
> - }
> - }
> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -
> - return error;
> -}
>
> /* Allocate all CoW reservations covering a range of blocks in a file. */
> static int
> @@ -359,9 +330,8 @@ __xfs_reflink_allocate_cow(
> struct xfs_defer_ops dfops;
> struct xfs_trans *tp;
> xfs_fsblock_t first_block;
> - xfs_fileoff_t next_fsb;
> int nimaps = 1, error;
> - bool skipped = false;
> + bool shared;
>
> xfs_defer_init(&dfops, &first_block);
>
> @@ -372,33 +342,38 @@ __xfs_reflink_allocate_cow(
>
> xfs_ilock(ip, XFS_ILOCK_EXCL);
>
> - next_fsb = *offset_fsb;
> - error = __xfs_reflink_reserve_cow(ip, &next_fsb, end_fsb, &skipped);
> + /* Read extent from the source file. */
> + nimaps = 1;
> + error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
> + &imap, &nimaps, 0);
> + if (error)
> + goto out_unlock;
> + ASSERT(nimaps == 1);
> +
> + error = xfs_reflink_reserve_cow(ip, &imap, &shared);
> if (error)
> goto out_trans_cancel;
>
> - if (skipped) {
> - *offset_fsb = next_fsb;
> + if (!shared) {
> + *offset_fsb = imap.br_startoff + imap.br_blockcount;
> goto out_trans_cancel;
> }
>
> xfs_trans_ijoin(tp, ip, 0);
> - error = xfs_bmapi_write(tp, ip, *offset_fsb, next_fsb - *offset_fsb,
> + error = xfs_bmapi_write(tp, ip, imap.br_startoff, imap.br_blockcount,
> XFS_BMAPI_COWFORK, &first_block,
> XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK),
> &imap, &nimaps, &dfops);
> if (error)
> goto out_trans_cancel;
>
> - /* We might not have been able to map the whole delalloc extent */
> - *offset_fsb = min(*offset_fsb + imap.br_blockcount, next_fsb);
> -
> error = xfs_defer_finish(&tp, &dfops, NULL);
> if (error)
> goto out_trans_cancel;
>
> error = xfs_trans_commit(tp);
>
> + *offset_fsb = imap.br_startoff + imap.br_blockcount;
> out_unlock:
> xfs_iunlock(ip, XFS_ILOCK_EXCL);
> return error;
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index 5dc3c8a..9f76924 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -26,8 +26,8 @@ extern int xfs_reflink_find_shared(struct xfs_mount *mp, xfs_agnumber_t agno,
> extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
> struct xfs_bmbt_irec *irec, bool *shared, bool *trimmed);
>
> -extern int xfs_reflink_reserve_cow_range(struct xfs_inode *ip,
> - xfs_off_t offset, xfs_off_t count);
> +extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
> + struct xfs_bmbt_irec *imap, bool *shared);
> extern int xfs_reflink_allocate_cow_range(struct xfs_inode *ip,
> xfs_off_t offset, xfs_off_t count);
> extern bool xfs_reflink_find_cow_mapping(struct xfs_inode *ip, xfs_off_t offset,
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 2586c9c..520660d 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -3347,7 +3347,7 @@ DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_alloc);
> DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_found);
> DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_enospc);
>
> -DEFINE_RW_EVENT(xfs_reflink_reserve_cow_range);
> +DEFINE_RW_EVENT(xfs_reflink_reserve_cow);
> DEFINE_RW_EVENT(xfs_reflink_allocate_cow_range);
>
> DEFINE_INODE_IREC_EVENT(xfs_reflink_bounce_dio_write);
> @@ -3359,7 +3359,6 @@ DEFINE_SIMPLE_IO_EVENT(xfs_reflink_end_cow);
> DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap);
> DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap_piece);
>
> -DEFINE_INODE_ERROR_EVENT(xfs_reflink_reserve_cow_range_error);
> DEFINE_INODE_ERROR_EVENT(xfs_reflink_allocate_cow_range_error);
> DEFINE_INODE_ERROR_EVENT(xfs_reflink_cancel_cow_range_error);
> DEFINE_INODE_ERROR_EVENT(xfs_reflink_end_cow_error);
> --
> 2.10.1.382.ga23ca1b
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/9] xfs: refactor xfs_bunmapi_cow
2016-10-10 13:38 ` [PATCH 6/9] xfs: refactor xfs_bunmapi_cow Christoph Hellwig
@ 2016-10-12 14:13 ` Brian Foster
2016-10-13 6:54 ` Christoph Hellwig
0 siblings, 1 reply; 25+ messages in thread
From: Brian Foster @ 2016-10-12 14:13 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, darrick.wong
On Mon, Oct 10, 2016 at 03:38:02PM +0200, Christoph Hellwig wrote:
> Split out two helpers for deleting delayed or real extents from the COW fork.
> This allows to call them directly from xfs_reflink_cow_end_io once that
> function is refactored to iterate the extent tree. It will also allow
> to reuse the delalloc deletion from xfs_bunmapi in the future.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 374 ++++++++++++++++++++++++++++-------------------
> fs/xfs/libxfs/xfs_bmap.h | 5 +
> 2 files changed, 225 insertions(+), 154 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 016dacc..814980d 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4862,6 +4862,219 @@ xfs_bmap_split_indlen(
> return stolen;
> }
>
> +int
> +xfs_bmap_del_extent_delay(
> + struct xfs_inode *ip,
> + int whichfork,
> + xfs_extnum_t *idx,
> + struct xfs_bmbt_irec *got,
> + struct xfs_bmbt_irec *del)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
> + struct xfs_bmbt_irec new;
> + int64_t da_old, da_new, da_diff = 0;
> + xfs_fileoff_t del_endoff, got_endoff;
> + xfs_filblks_t got_indlen, new_indlen, stolen;
> + int error = 0, state = 0;
> + bool isrt;
> +
> + XFS_STATS_INC(mp, xs_del_exlist);
> +
> + isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
> + del_endoff = del->br_startoff + del->br_blockcount;
> + got_endoff = got->br_startoff + got->br_blockcount;
> + da_old = startblockval(got->br_startblock);
> + da_new = 0;
> +
> + ASSERT(*idx >= 0);
> + ASSERT(*idx < ifp->if_bytes / sizeof(struct xfs_bmbt_rec));
> + ASSERT(del->br_blockcount > 0);
> + ASSERT(got->br_startoff <= del->br_startoff);
> + ASSERT(got_endoff >= del_endoff);
> +
> + if (isrt) {
> + int64_t rtexts = XFS_FSB_TO_B(mp, del->br_blockcount);
> +
> + do_div(rtexts, mp->m_sb.sb_rextsize);
> + xfs_mod_frextents(mp, rtexts);
> + }
> +
> + /*
> + * Update the inode delalloc counter now and wait to update the
> + * sb counters as we might have to borrow some blocks for the
> + * indirect block accounting.
> + */
> + xfs_trans_reserve_quota_nblks(NULL, ip, -((long)del->br_blockcount), 0,
> + isrt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
> + ip->i_delayed_blks -= del->br_blockcount;
> +
This appears to be fixed up later, but i_delayed_blks is accounted twice
as of this patch. It would be nice if we could avoid known breakage,
even if transient.
> + if (whichfork == XFS_COW_FORK)
> + state |= BMAP_COWFORK;
> +
> + if (got->br_startoff == del->br_startoff)
> + state |= BMAP_LEFT_CONTIG;
> + if (got_endoff == del_endoff)
> + state |= BMAP_RIGHT_CONTIG;
> +
> + switch (state & (BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG)) {
> + case BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG:
> + /*
> + * Matches the whole extent. Delete the entry.
> + */
> + xfs_iext_remove(ip, *idx, 1, state);
> + --*idx;
> + break;
> + case BMAP_LEFT_CONTIG:
> + /*
> + * Deleting the first part of the extent.
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_startoff = del_endoff;
> + got->br_blockcount -= del->br_blockcount;
> + da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
> + got->br_blockcount), da_old);
> + got->br_startblock = nullstartblock((int)da_new);
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> + break;
> + case BMAP_RIGHT_CONTIG:
> + /*
> + * Deleting the last part of the extent.
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_blockcount = got->br_blockcount - del->br_blockcount;
> + da_new = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip,
> + got->br_blockcount), da_old);
> + got->br_startblock = nullstartblock((int)da_new);
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> + break;
> + case 0:
> + /*
> + * Deleting the middle of the extent.
> + *
> + * Distribute the original indlen reservation across the two
> + * new extents. Steal blocks from the deleted extent if
> + * necessary. Stealing blocks simply fudges the fdblocks
> + * accounting in xfs_bunmapi().
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_blockcount = del->br_startoff - got->br_startoff;
> +
> + got_indlen = xfs_bmap_worst_indlen(ip, got->br_blockcount);
> + new_indlen = xfs_bmap_worst_indlen(ip, new.br_blockcount);
Doesn't look like new.br_blockcount is set until a few lines below.
Brian
> + stolen = xfs_bmap_split_indlen(da_old, &got_indlen, &new_indlen,
> + del->br_blockcount);
> + da_new = got_indlen + new_indlen - stolen;
> + del->br_blockcount -= stolen;
> +
> + /*
> + * Set the reservation for each extent. Warn if either
> + * is zero as this can lead to delalloc problems.
> + */
> + WARN_ON_ONCE(!got_indlen || !new_indlen);
> + got->br_startblock = nullstartblock((int)got_indlen);
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, 0, _THIS_IP_);
> +
> + new.br_startoff = del_endoff;
> + new.br_blockcount = got_endoff - del_endoff;
> + new.br_state = got->br_state;
> + new.br_startblock = nullstartblock((int)new_indlen);
> +
> + ++*idx;
> + xfs_iext_insert(ip, *idx, 1, &new, state);
> + break;
> + }
> +
> + ASSERT(da_old >= da_new);
> + da_diff = da_old - da_new;
> + if (!isrt)
> + da_diff += del->br_blockcount;
> + if (da_diff)
> + xfs_mod_fdblocks(mp, da_diff, false);
> + return error;
> +}
> +
> +void
> +xfs_bmap_del_extent_cow(
> + struct xfs_inode *ip,
> + xfs_extnum_t *idx,
> + struct xfs_bmbt_irec *got,
> + struct xfs_bmbt_irec *del)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> + struct xfs_bmbt_irec new;
> + xfs_fileoff_t del_endoff, got_endoff;
> + int state = BMAP_COWFORK;
> +
> + XFS_STATS_INC(mp, xs_del_exlist);
> +
> + del_endoff = del->br_startoff + del->br_blockcount;
> + got_endoff = got->br_startoff + got->br_blockcount;
> +
> + ASSERT(*idx >= 0);
> + ASSERT(*idx < ifp->if_bytes / sizeof(struct xfs_bmbt_rec));
> + ASSERT(del->br_blockcount > 0);
> + ASSERT(got->br_startoff <= del->br_startoff);
> + ASSERT(got_endoff >= del_endoff);
> + ASSERT(!isnullstartblock(got->br_startblock));
> +
> + if (got->br_startoff == del->br_startoff)
> + state |= BMAP_LEFT_CONTIG;
> + if (got_endoff == del_endoff)
> + state |= BMAP_RIGHT_CONTIG;
> +
> + switch (state & (BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG)) {
> + case BMAP_LEFT_CONTIG | BMAP_RIGHT_CONTIG:
> + /*
> + * Matches the whole extent. Delete the entry.
> + */
> + xfs_iext_remove(ip, *idx, 1, state);
> + --*idx;
> + break;
> + case BMAP_LEFT_CONTIG:
> + /*
> + * Deleting the first part of the extent.
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_startoff = del_endoff;
> + got->br_blockcount -= del->br_blockcount;
> + got->br_startblock = del->br_startblock + del->br_blockcount;
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> + break;
> + case BMAP_RIGHT_CONTIG:
> + /*
> + * Deleting the last part of the extent.
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_blockcount -= del->br_blockcount;
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> + break;
> + case 0:
> + /*
> + * Deleting the middle of the extent.
> + */
> + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> + got->br_blockcount = del->br_startoff - got->br_startoff;
> + xfs_bmbt_set_all(xfs_iext_get_ext(ifp, *idx), got);
> + trace_xfs_bmap_post_update(ip, *idx, state, _THIS_IP_);
> +
> + new.br_startoff = del_endoff;
> + new.br_blockcount = got_endoff - del_endoff;
> + new.br_state = got->br_state;
> + new.br_startblock = del->br_startblock + del->br_blockcount;
> +
> + ++*idx;
> + xfs_iext_insert(ip, *idx, 1, &new, state);
> + break;
> + }
> +}
> +
> /*
> * Called by xfs_bmapi to update file extent records and the btree
> * after removing space (or undoing a delayed allocation).
> @@ -5210,167 +5423,20 @@ xfs_bunmapi_cow(
> struct xfs_inode *ip,
> struct xfs_bmbt_irec *del)
> {
> - xfs_filblks_t da_new;
> - xfs_filblks_t da_old;
> - xfs_fsblock_t del_endblock = 0;
> - xfs_fileoff_t del_endoff;
> - int delay;
> struct xfs_bmbt_rec_host *ep;
> - int error;
> struct xfs_bmbt_irec got;
> - xfs_fileoff_t got_endoff;
> - struct xfs_ifork *ifp;
> - struct xfs_mount *mp;
> - xfs_filblks_t nblks;
> struct xfs_bmbt_irec new;
> - /* REFERENCED */
> - uint qfield;
> - xfs_filblks_t temp;
> - xfs_filblks_t temp2;
> - int state = BMAP_COWFORK;
> int eof;
> xfs_extnum_t eidx;
>
> - mp = ip->i_mount;
> - XFS_STATS_INC(mp, xs_del_exlist);
> -
> ep = xfs_bmap_search_extents(ip, del->br_startoff, XFS_COW_FORK, &eof,
> - &eidx, &got, &new);
> -
> - ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK); ifp = ifp;
> - ASSERT((eidx >= 0) && (eidx < ifp->if_bytes /
> - (uint)sizeof(xfs_bmbt_rec_t)));
> - ASSERT(del->br_blockcount > 0);
> - ASSERT(got.br_startoff <= del->br_startoff);
> - del_endoff = del->br_startoff + del->br_blockcount;
> - got_endoff = got.br_startoff + got.br_blockcount;
> - ASSERT(got_endoff >= del_endoff);
> - delay = isnullstartblock(got.br_startblock);
> - ASSERT(isnullstartblock(del->br_startblock) == delay);
> - qfield = 0;
> - error = 0;
> - /*
> - * If deleting a real allocation, must free up the disk space.
> - */
> - if (!delay) {
> - nblks = del->br_blockcount;
> - qfield = XFS_TRANS_DQ_BCOUNT;
> - /*
> - * Set up del_endblock and cur for later.
> - */
> - del_endblock = del->br_startblock + del->br_blockcount;
> - da_old = da_new = 0;
> - } else {
> - da_old = startblockval(got.br_startblock);
> - da_new = 0;
> - nblks = 0;
> - }
> - qfield = qfield;
> - nblks = nblks;
> -
> - /*
> - * Set flag value to use in switch statement.
> - * Left-contig is 2, right-contig is 1.
> - */
> - switch (((got.br_startoff == del->br_startoff) << 1) |
> - (got_endoff == del_endoff)) {
> - case 3:
> - /*
> - * Matches the whole extent. Delete the entry.
> - */
> - xfs_iext_remove(ip, eidx, 1, BMAP_COWFORK);
> - --eidx;
> - break;
> -
> - case 2:
> - /*
> - * Deleting the first part of the extent.
> - */
> - trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
> - xfs_bmbt_set_startoff(ep, del_endoff);
> - temp = got.br_blockcount - del->br_blockcount;
> - xfs_bmbt_set_blockcount(ep, temp);
> - if (delay) {
> - temp = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip, temp),
> - da_old);
> - xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
> - trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> - da_new = temp;
> - break;
> - }
> - xfs_bmbt_set_startblock(ep, del_endblock);
> - trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> - break;
> -
> - case 1:
> - /*
> - * Deleting the last part of the extent.
> - */
> - temp = got.br_blockcount - del->br_blockcount;
> - trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
> - xfs_bmbt_set_blockcount(ep, temp);
> - if (delay) {
> - temp = XFS_FILBLKS_MIN(xfs_bmap_worst_indlen(ip, temp),
> - da_old);
> - xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
> - trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> - da_new = temp;
> - break;
> - }
> - trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> - break;
> -
> - case 0:
> - /*
> - * Deleting the middle of the extent.
> - */
> - temp = del->br_startoff - got.br_startoff;
> - trace_xfs_bmap_pre_update(ip, eidx, state, _THIS_IP_);
> - xfs_bmbt_set_blockcount(ep, temp);
> - new.br_startoff = del_endoff;
> - temp2 = got_endoff - del_endoff;
> - new.br_blockcount = temp2;
> - new.br_state = got.br_state;
> - if (!delay) {
> - new.br_startblock = del_endblock;
> - } else {
> - temp = xfs_bmap_worst_indlen(ip, temp);
> - xfs_bmbt_set_startblock(ep, nullstartblock((int)temp));
> - temp2 = xfs_bmap_worst_indlen(ip, temp2);
> - new.br_startblock = nullstartblock((int)temp2);
> - da_new = temp + temp2;
> - while (da_new > da_old) {
> - if (temp) {
> - temp--;
> - da_new--;
> - xfs_bmbt_set_startblock(ep,
> - nullstartblock((int)temp));
> - }
> - if (da_new == da_old)
> - break;
> - if (temp2) {
> - temp2--;
> - da_new--;
> - new.br_startblock =
> - nullstartblock((int)temp2);
> - }
> - }
> - }
> - trace_xfs_bmap_post_update(ip, eidx, state, _THIS_IP_);
> - xfs_iext_insert(ip, eidx + 1, 1, &new, state);
> - ++eidx;
> - break;
> - }
> -
> - /*
> - * Account for change in delayed indirect blocks.
> - * Nothing to do for disk quota accounting here.
> - */
> - ASSERT(da_old >= da_new);
> - if (da_old > da_new)
> - xfs_mod_fdblocks(mp, (int64_t)(da_old - da_new), false);
> -
> - return error;
> + &eidx, &got, &new);
> + ASSERT(ep);
> + if (isnullstartblock(got.br_startblock))
> + xfs_bmap_del_extent_delay(ip, XFS_COW_FORK, &eidx, &got, del);
> + else
> + xfs_bmap_del_extent_cow(ip, &eidx, &got, del);
> + return 0;
> }
>
> /*
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index eb86af0..5f18248 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -224,6 +224,11 @@ int xfs_bunmapi(struct xfs_trans *tp, struct xfs_inode *ip,
> xfs_extnum_t nexts, xfs_fsblock_t *firstblock,
> struct xfs_defer_ops *dfops, int *done);
> int xfs_bunmapi_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *del);
> +int xfs_bmap_del_extent_delay(struct xfs_inode *ip, int whichfork,
> + xfs_extnum_t *idx, struct xfs_bmbt_irec *got,
> + struct xfs_bmbt_irec *del);
> +void xfs_bmap_del_extent_cow(struct xfs_inode *ip, xfs_extnum_t *idx,
> + struct xfs_bmbt_irec *got, struct xfs_bmbt_irec *del);
> int xfs_check_nostate_extents(struct xfs_ifork *ifp, xfs_extnum_t idx,
> xfs_extnum_t num);
> uint xfs_default_attroffset(struct xfs_inode *ip);
> --
> 2.10.1.382.ga23ca1b
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks
2016-10-10 13:38 ` [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks Christoph Hellwig
@ 2016-10-12 14:13 ` Brian Foster
2016-10-13 7:01 ` Christoph Hellwig
0 siblings, 1 reply; 25+ messages in thread
From: Brian Foster @ 2016-10-12 14:13 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, darrick.wong
On Mon, Oct 10, 2016 at 03:38:03PM +0200, Christoph Hellwig wrote:
> Rewrite xfs_reflink_cancel_cow_blocks so that we only do a search for
> the first extent in the extent list and then iterate over the remaining
> extents using the extent index, passing the extent we operate on
> directly to xfs_bmap_del_extent_delay or xfs_bmap_del_extent_cow instead
> of going through xfs_bunmapi and doing yet another extent list lookup.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_reflink.c | 59 +++++++++++++++++++++++-----------------------------
> 1 file changed, 26 insertions(+), 33 deletions(-)
>
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 5d230ea..08170bb 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -512,58 +512,52 @@ xfs_reflink_cancel_cow_blocks(
> xfs_fileoff_t offset_fsb,
> xfs_fileoff_t end_fsb)
> {
> - struct xfs_bmbt_irec irec;
> - xfs_filblks_t count_fsb;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> + struct xfs_bmbt_irec got, prev, del;
> + xfs_extnum_t idx;
> xfs_fsblock_t firstfsb;
> struct xfs_defer_ops dfops;
> - int error = 0;
> - int nimaps;
> + int error = 0, eof = 0;
>
> if (!xfs_is_reflink_inode(ip))
> return 0;
>
> - /* Go find the old extent in the CoW fork. */
> - while (offset_fsb < end_fsb) {
> - nimaps = 1;
> - count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
> - error = xfs_bmapi_read(ip, offset_fsb, count_fsb, &irec,
> - &nimaps, XFS_BMAPI_COWFORK);
> - if (error)
> - break;
> - ASSERT(nimaps == 1);
> + xfs_bmap_search_extents(ip, offset_fsb, XFS_COW_FORK, &eof, &idx,
> + &got, &prev);
> + if (eof)
> + return 0;
>
> - trace_xfs_reflink_cancel_cow(ip, &irec);
> + do {
> + if (got.br_startoff >= end_fsb)
> + break;
How about using a while loop..?
while (got.br_startoff < end_fsb) {
...
Otherwise, looks fine:
Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> - if (irec.br_startblock == DELAYSTARTBLOCK) {
> - /* Free a delayed allocation. */
> - xfs_mod_fdblocks(ip->i_mount, irec.br_blockcount,
> - false);
> - ip->i_delayed_blks -= irec.br_blockcount;
> + del = got;
> + xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
> + trace_xfs_reflink_cancel_cow(ip, &del);
>
> - /* Remove the mapping from the CoW fork. */
> - error = xfs_bunmapi_cow(ip, &irec);
> + if (isnullstartblock(del.br_startblock)) {
> + error = xfs_bmap_del_extent_delay(ip, XFS_COW_FORK,
> + &idx, &got, &del);
> if (error)
> break;
> - } else if (irec.br_startblock == HOLESTARTBLOCK) {
> - /* empty */
> } else {
> xfs_trans_ijoin(*tpp, ip, 0);
> xfs_defer_init(&dfops, &firstfsb);
>
> /* Free the CoW orphan record. */
> error = xfs_refcount_free_cow_extent(ip->i_mount,
> - &dfops, irec.br_startblock,
> - irec.br_blockcount);
> + &dfops, del.br_startblock,
> + del.br_blockcount);
> if (error)
> break;
>
> xfs_bmap_add_free(ip->i_mount, &dfops,
> - irec.br_startblock, irec.br_blockcount,
> + del.br_startblock, del.br_blockcount,
> NULL);
>
> /* Update quota accounting */
> xfs_trans_mod_dquot_byino(*tpp, ip, XFS_TRANS_DQ_BCOUNT,
> - -(long)irec.br_blockcount);
> + -(long)del.br_blockcount);
>
> /* Roll the transaction */
> error = xfs_defer_finish(tpp, &dfops, ip);
> @@ -573,14 +567,13 @@ xfs_reflink_cancel_cow_blocks(
> }
>
> /* Remove the mapping from the CoW fork. */
> - error = xfs_bunmapi_cow(ip, &irec);
> - if (error)
> - break;
> + xfs_bmap_del_extent_cow(ip, &idx, &got, &del);
> }
>
> - /* Roll on... */
> - offset_fsb = irec.br_startoff + irec.br_blockcount;
> - }
> + if (++idx >= ifp->if_bytes / sizeof(struct xfs_bmbt_rec))
> + return 0;
> + xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
> + } while (1);
>
> return error;
> }
> --
> 2.10.1.382.ga23ca1b
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 8/9] xfs: optimize xfs_reflink_end_cow
2016-10-10 13:38 ` [PATCH 8/9] xfs: optimize xfs_reflink_end_cow Christoph Hellwig
@ 2016-10-12 14:15 ` Brian Foster
2016-10-13 7:06 ` Christoph Hellwig
0 siblings, 1 reply; 25+ messages in thread
From: Brian Foster @ 2016-10-12 14:15 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, darrick.wong
On Mon, Oct 10, 2016 at 03:38:04PM +0200, Christoph Hellwig wrote:
> Instead of doing a full extent list search for each extent that is to be
> deleted using xfs_bmapi_read and then doing another one inside of
> xfs_bunmapi_cow use the same scheme that xfs_bumapi uses: look up the
> last extent to be deleted and then use the extent index to walk downward
> until we are outside the range to be deleted.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_reflink.c | 114 ++++++++++++++++++++++-----------------------------
> fs/xfs/xfs_trace.h | 1 -
> 2 files changed, 49 insertions(+), 66 deletions(-)
>
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 08170bb..c4e7521 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -637,25 +637,22 @@ xfs_reflink_end_cow(
> xfs_off_t offset,
> xfs_off_t count)
> {
> - struct xfs_bmbt_irec irec;
> - struct xfs_bmbt_irec uirec;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> + struct xfs_bmbt_irec got, prev, del;
> struct xfs_trans *tp;
> xfs_fileoff_t offset_fsb;
> xfs_fileoff_t end_fsb;
> - xfs_filblks_t count_fsb;
> xfs_fsblock_t firstfsb;
> struct xfs_defer_ops dfops;
> - int error;
> + int error, eof = 0;
> unsigned int resblks;
> - xfs_filblks_t ilen;
> xfs_filblks_t rlen;
> - int nimaps;
> + xfs_extnum_t idx;
>
> trace_xfs_reflink_end_cow(ip, offset, count);
>
> offset_fsb = XFS_B_TO_FSBT(ip->i_mount, offset);
> end_fsb = XFS_B_TO_FSB(ip->i_mount, offset + count);
> - count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
>
> /* Start a rolling transaction to switch the mappings */
> resblks = XFS_EXTENTADD_SPACE_RES(ip->i_mount, XFS_DATA_FORK);
> @@ -667,73 +664,61 @@ xfs_reflink_end_cow(
> xfs_ilock(ip, XFS_ILOCK_EXCL);
> xfs_trans_ijoin(tp, ip, 0);
>
> - /* Go find the old extent in the CoW fork. */
> - while (offset_fsb < end_fsb) {
> - /* Read extent from the source file */
> - nimaps = 1;
> - count_fsb = (xfs_filblks_t)(end_fsb - offset_fsb);
> - error = xfs_bmapi_read(ip, offset_fsb, count_fsb, &irec,
> - &nimaps, XFS_BMAPI_COWFORK);
> - if (error)
> - goto out_cancel;
> - ASSERT(nimaps == 1);
> -
> - ASSERT(irec.br_startblock != DELAYSTARTBLOCK);
> - trace_xfs_reflink_cow_remap(ip, &irec);
> + xfs_bmap_search_extents(ip, end_fsb - 1, XFS_COW_FORK, &eof, &idx,
> + &got, &prev);
> + if (eof) {
> + ASSERT(idx > 0);
> + xfs_bmbt_get_all(xfs_iext_get_ext(ifp, --idx), &got);
> + }
Hmm, should this happen? Do we really want to proceed with the previous
extent?
I suppose the extent trim below would catch if we've dropped outside the
range of the I/O, but note that if trim extent sets blockcount = 0, it
doesn't necessarily reset the start offset for the (del.br_startoff ==
offset_fsb) check at next_extent. Perhaps that should break once the del
end offset precedes offset_fsb..?
Either way, it would be nice to have a comment here if there's some
reason we shouldn't just bail out or flag a problem...
>
> - /*
> - * We can have a hole in the CoW fork if part of a directio
> - * write is CoW but part of it isn't.
> - */
> - rlen = ilen = irec.br_blockcount;
> - if (irec.br_startblock == HOLESTARTBLOCK)
> + do {
> + ASSERT(!isnullstartblock(got.br_startblock));
> + del = got;
> + xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
> + if (!del.br_blockcount)
> goto next_extent;
>
> /* Unmap the old blocks in the data fork. */
> - while (rlen) {
> - xfs_defer_init(&dfops, &firstfsb);
> - error = __xfs_bunmapi(tp, ip, irec.br_startoff,
> - &rlen, 0, 1, &firstfsb, &dfops);
> - if (error)
> - goto out_defer;
> -
> - /*
> - * Trim the extent to whatever got unmapped.
> - * Remember, bunmapi works backwards.
> - */
> - uirec.br_startblock = irec.br_startblock + rlen;
> - uirec.br_startoff = irec.br_startoff + rlen;
> - uirec.br_blockcount = irec.br_blockcount - rlen;
> - irec.br_blockcount = rlen;
> - trace_xfs_reflink_cow_remap_piece(ip, &uirec);
> + xfs_defer_init(&dfops, &firstfsb);
> + rlen = del.br_blockcount;
> + error = __xfs_bunmapi(tp, ip, del.br_startoff, &rlen, 0, 1,
> + &firstfsb, &dfops);
> + if (error)
> + goto out_defer;
>
> - /* Free the CoW orphan record. */
> - error = xfs_refcount_free_cow_extent(tp->t_mountp,
> - &dfops, uirec.br_startblock,
> - uirec.br_blockcount);
> - if (error)
> - goto out_defer;
> + /* Trim the extent to whatever got unmapped. */
> + if (rlen) {
> + xfs_trim_extent(&del, del.br_startoff + rlen,
> + del.br_blockcount - rlen);
> + }
> + trace_xfs_reflink_cow_remap(ip, &del);
>
> - /* Map the new blocks into the data fork. */
> - error = xfs_bmap_map_extent(tp->t_mountp, &dfops,
> - ip, &uirec);
> - if (error)
> - goto out_defer;
> + /* Free the CoW orphan record. */
> + error = xfs_refcount_free_cow_extent(tp->t_mountp, &dfops,
> + del.br_startblock, del.br_blockcount);
> + if (error)
> + goto out_defer;
>
> - /* Remove the mapping from the CoW fork. */
> - error = xfs_bunmapi_cow(ip, &uirec);
> - if (error)
> - goto out_defer;
> + /* Map the new blocks into the data fork. */
> + error = xfs_bmap_map_extent(tp->t_mountp, &dfops, ip, &del);
> + if (error)
> + goto out_defer;
>
> - error = xfs_defer_finish(&tp, &dfops, ip);
> - if (error)
> - goto out_defer;
> - }
> + /* Remove the mapping from the CoW fork. */
> + xfs_bmap_del_extent_cow(ip, &idx, &got, &del);
> +
> + error = xfs_defer_finish(&tp, &dfops, ip);
> + if (error)
> + goto out_defer;
>
> next_extent:
> - /* Roll on... */
> - offset_fsb = irec.br_startoff + ilen;
> - }
> + if (del.br_startoff == offset_fsb || idx < 0)
> + break;
> +
> + xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
> + if (got.br_startoff >= del.br_startoff && --idx >= 0)
> + xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
> + } while (got.br_startoff < del.br_startoff);
Maybe it's just me, but I find this whole loop kind of confusing. I
realize it is tricky because we have to handle several different cases
with regard to index (no more extents to the left, bmap_del_extent_cow()
pushing index forward or back).
In staring at it a bit, I'm wondering if something like the following
would work:
/* walk backwards until we're out of the I/O range... */
while (got.br_startoff + got.br_blockcount > offset_fsb) {
del = got;
xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
/* extent delete may have bumped idx forward */
if (!del.br_blockcount) {
idx--;
goto next_extent;
}
...
next_extent:
if (idx < 0)
break;
xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
}
Thoughts?
Brian
>
> error = xfs_trans_commit(tp);
> xfs_iunlock(ip, XFS_ILOCK_EXCL);
> @@ -743,7 +728,6 @@ next_extent:
>
> out_defer:
> xfs_defer_cancel(&dfops);
> -out_cancel:
> xfs_trans_cancel(tp);
> xfs_iunlock(ip, XFS_ILOCK_EXCL);
> out:
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 520660d..f3b4418 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -3357,7 +3357,6 @@ DEFINE_INODE_IREC_EVENT(xfs_reflink_trim_irec);
> DEFINE_SIMPLE_IO_EVENT(xfs_reflink_cancel_cow_range);
> DEFINE_SIMPLE_IO_EVENT(xfs_reflink_end_cow);
> DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap);
> -DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap_piece);
>
> DEFINE_INODE_ERROR_EVENT(xfs_reflink_allocate_cow_range_error);
> DEFINE_INODE_ERROR_EVENT(xfs_reflink_cancel_cow_range_error);
> --
> 2.10.1.382.ga23ca1b
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/9] xfs: optimize writes to reflink files
2016-10-12 14:12 ` Brian Foster
@ 2016-10-13 6:49 ` Christoph Hellwig
2016-10-13 13:26 ` Brian Foster
0 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-13 6:49 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs, darrick.wong
On Wed, Oct 12, 2016 at 10:12:34AM -0400, Brian Foster wrote:
> > + if (xfs_is_reflink_inode(ip)) {
> > + bool shared;
> > +
> > + end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
> > + maxbytes_fsb);
> > + xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
> > + error = xfs_reflink_reserve_cow(ip, &got, &shared);
> > + if (error)
> > + goto out_unlock;
>
> All in all this seems fine, but I don't see why we need to get all the
> way down through xfs_reflink_reserve_cow() ->
> xfs_reflink_trim_around_shared() to handle the basic delalloc overwrite
> case on a reflink inode. Could we enhance the is_reflink_inode() helper
> or create a new one that can consider whether the data fork extent is a
> hole or delalloc?
Do you mean delalloc non-overwrite? We could skip non-overwrite extents
by factoring out a helper that checks for extent types that don't need to
be overwritten. But this would defeat the COW fork speculative
preallocation logic, which causes additional COW operations even for
extents we would not nessecarily have to COW. So we'll always have to
look at the COW fork first if we already have an allocation to implement
that scheme (and we should probably document it better).
xfs_reflink_trim_around_shared does a check for the non-COWable extent
types as the very first thing, so that's where we are done with the COW
overhead for a non-overwrite that doesn't have a speculative
preallocation in the COW fork.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/9] xfs: refactor xfs_bunmapi_cow
2016-10-12 14:13 ` Brian Foster
@ 2016-10-13 6:54 ` Christoph Hellwig
2016-10-13 13:26 ` Brian Foster
0 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-13 6:54 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs, darrick.wong
On Wed, Oct 12, 2016 at 10:13:08AM -0400, Brian Foster wrote:
> > + /*
> > + * Update the inode delalloc counter now and wait to update the
> > + * sb counters as we might have to borrow some blocks for the
> > + * indirect block accounting.
> > + */
> > + xfs_trans_reserve_quota_nblks(NULL, ip, -((long)del->br_blockcount), 0,
> > + isrt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
> > + ip->i_delayed_blks -= del->br_blockcount;
> > +
>
> This appears to be fixed up later, but i_delayed_blks is accounted twice
> as of this patch. It would be nice if we could avoid known breakage,
> even if transient.
Where do we account for it the second time?
> > + case 0:
> > + /*
> > + * Deleting the middle of the extent.
> > + *
> > + * Distribute the original indlen reservation across the two
> > + * new extents. Steal blocks from the deleted extent if
> > + * necessary. Stealing blocks simply fudges the fdblocks
> > + * accounting in xfs_bunmapi().
> > + */
> > + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> > + got->br_blockcount = del->br_startoff - got->br_startoff;
> > +
> > + got_indlen = xfs_bmap_worst_indlen(ip, got->br_blockcount);
> > + new_indlen = xfs_bmap_worst_indlen(ip, new.br_blockcount);
>
> Doesn't look like new.br_blockcount is set until a few lines below.
Indeed. Fixed for the next resend.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks
2016-10-12 14:13 ` Brian Foster
@ 2016-10-13 7:01 ` Christoph Hellwig
0 siblings, 0 replies; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-13 7:01 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs, darrick.wong
On Wed, Oct 12, 2016 at 10:13:25AM -0400, Brian Foster wrote:
> How about using a while loop..?
>
> while (got.br_startoff < end_fsb) {
> ...
>
> Otherwise, looks fine:
Sure. My initial version had a real do {} while loop, but the helper
to make that possible was dropped from this series and will be sent
separately.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 8/9] xfs: optimize xfs_reflink_end_cow
2016-10-12 14:15 ` Brian Foster
@ 2016-10-13 7:06 ` Christoph Hellwig
2016-10-13 13:27 ` Brian Foster
0 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2016-10-13 7:06 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs, darrick.wong
On Wed, Oct 12, 2016 at 10:15:37AM -0400, Brian Foster wrote:
> > + xfs_bmap_search_extents(ip, end_fsb - 1, XFS_COW_FORK, &eof, &idx,
> > + &got, &prev);
> > + if (eof) {
> > + ASSERT(idx > 0);
> > + xfs_bmbt_get_all(xfs_iext_get_ext(ifp, --idx), &got);
> > + }
>
> Hmm, should this happen? Do we really want to proceed with the previous
> extent?
Yes. This is the same pattern as xfs_bunmapi and it is needed. For
an explanation we need to recall the semantics of xfs_bmap_search_extents,
which gives us the extent at bno IFF there is one, else if gives us the
extent beyond bno.
So we search for the last extent in the to be deleted range here,
but if there is no extent at the end of the range we'll get a return
outside the range, in which case we need to back one up (which
bunmapi does and we catch somewhat confusingly with the trim_extent
and goto next_extent, I should clean that up..), but as a special
case we can get the eof case where there is no extent beyong bno.
> I suppose the extent trim below would catch if we've dropped outside the
> range of the I/O, but note that if trim extent sets blockcount = 0, it
> doesn't necessarily reset the start offset for the (del.br_startoff ==
> offset_fsb) check at next_extent. Perhaps that should break once the del
> end offset precedes offset_fsb..?
>
> Either way, it would be nice to have a comment here if there's some
> reason we shouldn't just bail out or flag a problem...
Yes, both this code and bumapi could use some cleanup..
> Maybe it's just me, but I find this whole loop kind of confusing. I
> realize it is tricky because we have to handle several different cases
> with regard to index (no more extents to the left, bmap_del_extent_cow()
> pushing index forward or back).
I found it rather confusing as well.
>
> In staring at it a bit, I'm wondering if something like the following
> would work:
>
> /* walk backwards until we're out of the I/O range... */
> while (got.br_startoff + got.br_blockcount > offset_fsb) {
> del = got;
> xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
> /* extent delete may have bumped idx forward */
> if (!del.br_blockcount) {
> idx--;
> goto next_extent;
> }
>
> ...
>
> next_extent:
> if (idx < 0)
> break;
> xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
> }
>
> Thoughts?
That looks reasonable, I'll see if it passes xfstests.. :)
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/9] xfs: optimize writes to reflink files
2016-10-13 6:49 ` Christoph Hellwig
@ 2016-10-13 13:26 ` Brian Foster
2016-10-13 18:28 ` Darrick J. Wong
0 siblings, 1 reply; 25+ messages in thread
From: Brian Foster @ 2016-10-13 13:26 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, darrick.wong
On Thu, Oct 13, 2016 at 08:49:25AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 12, 2016 at 10:12:34AM -0400, Brian Foster wrote:
> > > + if (xfs_is_reflink_inode(ip)) {
> > > + bool shared;
> > > +
> > > + end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
> > > + maxbytes_fsb);
> > > + xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
> > > + error = xfs_reflink_reserve_cow(ip, &got, &shared);
> > > + if (error)
> > > + goto out_unlock;
> >
> > All in all this seems fine, but I don't see why we need to get all the
> > way down through xfs_reflink_reserve_cow() ->
> > xfs_reflink_trim_around_shared() to handle the basic delalloc overwrite
> > case on a reflink inode. Could we enhance the is_reflink_inode() helper
> > or create a new one that can consider whether the data fork extent is a
> > hole or delalloc?
>
> Do you mean delalloc non-overwrite? We could skip non-overwrite extents
> by factoring out a helper that checks for extent types that don't need to
> be overwritten. But this would defeat the COW fork speculative
> preallocation logic, which causes additional COW operations even for
> extents we would not nessecarily have to COW. So we'll always have to
> look at the COW fork first if we already have an allocation to implement
> that scheme (and we should probably document it better).
>
Either way... delalloc into a hole or overwrite of an existing (data
fork) delalloc, will fall out of xfs_reflink_reserve_cow() so long as
nothing is in the cow fork, right?
But regardless, I see your point now. For whatever reason the comment
update in xfs_reflink_reserve_cow() went right over my head. IIUC, the
idea is that cow delalloc writes can include preallocation and thus have
delalloc for blocks that might not actually be shared in the data fork.
Therefore, we have to query the cow fork first and cannot reliably use
the data fork shared state to determine whether cow fork blocks actually
exist. A clarification of the comment is probably fine.. thanks for the
explanation.
Brian
> xfs_reflink_trim_around_shared does a check for the non-COWable extent
> types as the very first thing, so that's where we are done with the COW
> overhead for a non-overwrite that doesn't have a speculative
> preallocation in the COW fork.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/9] xfs: refactor xfs_bunmapi_cow
2016-10-13 6:54 ` Christoph Hellwig
@ 2016-10-13 13:26 ` Brian Foster
0 siblings, 0 replies; 25+ messages in thread
From: Brian Foster @ 2016-10-13 13:26 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, darrick.wong
On Thu, Oct 13, 2016 at 08:54:32AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 12, 2016 at 10:13:08AM -0400, Brian Foster wrote:
> > > + /*
> > > + * Update the inode delalloc counter now and wait to update the
> > > + * sb counters as we might have to borrow some blocks for the
> > > + * indirect block accounting.
> > > + */
> > > + xfs_trans_reserve_quota_nblks(NULL, ip, -((long)del->br_blockcount), 0,
> > > + isrt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
> > > + ip->i_delayed_blks -= del->br_blockcount;
> > > +
> >
> > This appears to be fixed up later, but i_delayed_blks is accounted twice
> > as of this patch. It would be nice if we could avoid known breakage,
> > even if transient.
>
> Where do we account for it the second time?
>
Note again that this is transient and fixed up in a later patch (so you
won't see it looking at the code with the full series applied). But as
of this patch the following is possible:
...
xfs_reflink_cancel_cow_blocks()
i_delayed_blks -= blockcount
xfs_bunmapi_cow()
xfs_bmap_del_extent_delay()
i_delayed_blks -= blockcount
Brian
> > > + case 0:
> > > + /*
> > > + * Deleting the middle of the extent.
> > > + *
> > > + * Distribute the original indlen reservation across the two
> > > + * new extents. Steal blocks from the deleted extent if
> > > + * necessary. Stealing blocks simply fudges the fdblocks
> > > + * accounting in xfs_bunmapi().
> > > + */
> > > + trace_xfs_bmap_pre_update(ip, *idx, state, _THIS_IP_);
> > > + got->br_blockcount = del->br_startoff - got->br_startoff;
> > > +
> > > + got_indlen = xfs_bmap_worst_indlen(ip, got->br_blockcount);
> > > + new_indlen = xfs_bmap_worst_indlen(ip, new.br_blockcount);
> >
> > Doesn't look like new.br_blockcount is set until a few lines below.
>
> Indeed. Fixed for the next resend.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 8/9] xfs: optimize xfs_reflink_end_cow
2016-10-13 7:06 ` Christoph Hellwig
@ 2016-10-13 13:27 ` Brian Foster
0 siblings, 0 replies; 25+ messages in thread
From: Brian Foster @ 2016-10-13 13:27 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, darrick.wong
On Thu, Oct 13, 2016 at 09:06:39AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 12, 2016 at 10:15:37AM -0400, Brian Foster wrote:
> > > + xfs_bmap_search_extents(ip, end_fsb - 1, XFS_COW_FORK, &eof, &idx,
> > > + &got, &prev);
> > > + if (eof) {
> > > + ASSERT(idx > 0);
> > > + xfs_bmbt_get_all(xfs_iext_get_ext(ifp, --idx), &got);
> > > + }
> >
> > Hmm, should this happen? Do we really want to proceed with the previous
> > extent?
>
> Yes. This is the same pattern as xfs_bunmapi and it is needed. For
> an explanation we need to recall the semantics of xfs_bmap_search_extents,
> which gives us the extent at bno IFF there is one, else if gives us the
> extent beyond bno.
>
Ok, right..
> So we search for the last extent in the to be deleted range here,
> but if there is no extent at the end of the range we'll get a return
> outside the range, in which case we need to back one up (which
> bunmapi does and we catch somewhat confusingly with the trim_extent
> and goto next_extent, I should clean that up..), but as a special
> case we can get the eof case where there is no extent beyong bno.
>
Got it, thanks. I think it was just the bmap_search_extents() semantics
that threw me off. For some reason I was thinking eof meant there was
nothing in the range, but that's not valid here because we are walking
backwards.
And the ASSERT(idx > 0) exists because if we're at eof, we must have at
least one preceding extent because we're in the end I/O handler, after
all. Makes sense.
> > I suppose the extent trim below would catch if we've dropped outside the
> > range of the I/O, but note that if trim extent sets blockcount = 0, it
> > doesn't necessarily reset the start offset for the (del.br_startoff ==
> > offset_fsb) check at next_extent. Perhaps that should break once the del
> > end offset precedes offset_fsb..?
> >
> > Either way, it would be nice to have a comment here if there's some
> > reason we shouldn't just bail out or flag a problem...
>
> Yes, both this code and bumapi could use some cleanup..
>
> > Maybe it's just me, but I find this whole loop kind of confusing. I
> > realize it is tricky because we have to handle several different cases
> > with regard to index (no more extents to the left, bmap_del_extent_cow()
> > pushing index forward or back).
>
> I found it rather confusing as well.
> >
> > In staring at it a bit, I'm wondering if something like the following
> > would work:
> >
> > /* walk backwards until we're out of the I/O range... */
> > while (got.br_startoff + got.br_blockcount > offset_fsb) {
> > del = got;
> > xfs_trim_extent(&del, offset_fsb, end_fsb - offset_fsb);
> > /* extent delete may have bumped idx forward */
> > if (!del.br_blockcount) {
> > idx--;
> > goto next_extent;
> > }
> >
> > ...
> >
> > next_extent:
> > if (idx < 0)
> > break;
> > xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), &got);
> > }
> >
> > Thoughts?
>
> That looks reasonable, I'll see if it passes xfstests.. :)
Great, thanks!
Brian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/9] xfs: optimize writes to reflink files
2016-10-13 13:26 ` Brian Foster
@ 2016-10-13 18:28 ` Darrick J. Wong
0 siblings, 0 replies; 25+ messages in thread
From: Darrick J. Wong @ 2016-10-13 18:28 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs
On Thu, Oct 13, 2016 at 09:26:05AM -0400, Brian Foster wrote:
> On Thu, Oct 13, 2016 at 08:49:25AM +0200, Christoph Hellwig wrote:
> > On Wed, Oct 12, 2016 at 10:12:34AM -0400, Brian Foster wrote:
> > > > + if (xfs_is_reflink_inode(ip)) {
> > > > + bool shared;
> > > > +
> > > > + end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
> > > > + maxbytes_fsb);
> > > > + xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
> > > > + error = xfs_reflink_reserve_cow(ip, &got, &shared);
> > > > + if (error)
> > > > + goto out_unlock;
> > >
> > > All in all this seems fine, but I don't see why we need to get all the
> > > way down through xfs_reflink_reserve_cow() ->
> > > xfs_reflink_trim_around_shared() to handle the basic delalloc overwrite
> > > case on a reflink inode. Could we enhance the is_reflink_inode() helper
> > > or create a new one that can consider whether the data fork extent is a
> > > hole or delalloc?
> >
> > Do you mean delalloc non-overwrite? We could skip non-overwrite extents
> > by factoring out a helper that checks for extent types that don't need to
> > be overwritten. But this would defeat the COW fork speculative
> > preallocation logic, which causes additional COW operations even for
> > extents we would not nessecarily have to COW. So we'll always have to
> > look at the COW fork first if we already have an allocation to implement
> > that scheme (and we should probably document it better).
> >
>
> Either way... delalloc into a hole or overwrite of an existing (data
> fork) delalloc, will fall out of xfs_reflink_reserve_cow() so long as
> nothing is in the cow fork, right?
>
> But regardless, I see your point now. For whatever reason the comment
> update in xfs_reflink_reserve_cow() went right over my head. IIUC, the
> idea is that cow delalloc writes can include preallocation and thus have
> delalloc for blocks that might not actually be shared in the data fork.
> Therefore, we have to query the cow fork first and cannot reliably use
> the data fork shared state to determine whether cow fork blocks actually
> exist. A clarification of the comment is probably fine.. thanks for the
> explanation.
Yep, that's correct. We can promote non-CoW writes to CoW as a strategy
to try to reduce fragmentation. I'll clarify that aspect in the docs.
--D
>
> Brian
>
> > xfs_reflink_trim_around_shared does a check for the non-COWable extent
> > types as the very first thing, so that's where we are done with the COW
> > overhead for a non-overwrite that doesn't have a speculative
> > preallocation in the COW fork.
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2016-10-13 19:26 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-10 13:37 optimize the COW I/O path Christoph Hellwig
2016-10-10 13:37 ` [PATCH 1/9] iomap: add IOMAP_REPORT Christoph Hellwig
2016-10-11 1:45 ` Darrick J. Wong
2016-10-11 4:58 ` Christoph Hellwig
[not found] ` <20161011144557.GA16368@lst.de>
2016-10-11 16:22 ` Darrick J. Wong
2016-10-10 13:37 ` [PATCH 2/9] xfs: add xfs_trim_extent Christoph Hellwig
2016-10-10 13:37 ` [PATCH 3/9] xfs: handle "raw" delayed extents xfs_reflink_trim_around_shared Christoph Hellwig
2016-10-10 13:38 ` [PATCH 4/9] xfs: don't bother looking at the refcount tree for reads Christoph Hellwig
2016-10-10 13:38 ` [PATCH 5/9] xfs: optimize writes to reflink files Christoph Hellwig
2016-10-12 14:12 ` Brian Foster
2016-10-13 6:49 ` Christoph Hellwig
2016-10-13 13:26 ` Brian Foster
2016-10-13 18:28 ` Darrick J. Wong
2016-10-10 13:38 ` [PATCH 6/9] xfs: refactor xfs_bunmapi_cow Christoph Hellwig
2016-10-12 14:13 ` Brian Foster
2016-10-13 6:54 ` Christoph Hellwig
2016-10-13 13:26 ` Brian Foster
2016-10-10 13:38 ` [PATCH 7/9] xfs: optimize xfs_reflink_cancel_cow_blocks Christoph Hellwig
2016-10-12 14:13 ` Brian Foster
2016-10-13 7:01 ` Christoph Hellwig
2016-10-10 13:38 ` [PATCH 8/9] xfs: optimize xfs_reflink_end_cow Christoph Hellwig
2016-10-12 14:15 ` Brian Foster
2016-10-13 7:06 ` Christoph Hellwig
2016-10-13 13:27 ` Brian Foster
2016-10-10 13:38 ` [PATCH 9/9] xfs: remove xfs_bunmapi_cow Christoph Hellwig
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.