delalloc and reflink fixes & tweaks V2
Hi all,
this series contains the basic reflink and delalloc fixups intended for 4.20. All the bits that need more work are left out for now.
Changes since v1:
- drop various patches
- print the inode number for dangling delalloc extents
PATCH 1/7 xfs: remove XFS_IO_INVALID
The invalid state isn't any different from a hole, so merge the two states. Use the more descriptive hole name, but keep it as the first value of the enum to catch uninitialized fields.
Signed-off-by Christoph Hellwig <[email protected]>
--- fs/xfs/xfs_aops.c | 4 ++-- fs/xfs/xfs_aops.h | 14 ++++++-------- 2 files changed, 8 insertions(+), 10 deletions(-)
fs/xfs/xfs_aops.c +2 -2
@@ -917,7 +917,7 @@ xfs_vm_writepage( struct writeback_control *wbc) {struct xfs_writepage_ctx wpc = { - .io_type =XFS_IO_INVALID , + .io_type =XFS_IO_HOLE , };int ret;
@@ -933,7 +933,7 @@ xfs_vm_writepages( struct writeback_control *wbc) {struct xfs_writepage_ctx wpc = { - .io_type =XFS_IO_INVALID , + .io_type =XFS_IO_HOLE , };int ret;
fs/xfs/xfs_aops.h +6 -8
@@ -12,21 +12,19 @@ extern struct bio_set xfs_ioend_bioset;* Types of I/O for bmap clustering and I/O completion tracking. */ enum { -XFS_IO_INVALID ,/* initial state */ +XFS_IO_HOLE ,/* covers region without any block allocation */ XFS_IO_DELALLOC ,/* covers delalloc region */ XFS_IO_UNWRITTEN ,/* covers allocated but uninitialized data */ XFS_IO_OVERWRITE ,/* covers already allocated extent */ XFS_IO_COW ,/* covers copy-on-write extent */ -XFS_IO_HOLE ,/* covers region without any block allocation */ };#define XFS_IO_TYPES \ - { XFS_IO_INVALID, "invalid" }, \ - { XFS_IO_DELALLOC, "delalloc" }, \ - { XFS_IO_UNWRITTEN, "unwritten" }, \ - { XFS_IO_OVERWRITE, "overwrite" }, \ - { XFS_IO_COW, "CoW" }, \ - { XFS_IO_HOLE, "hole" } + { XFS_IO_HOLE, "hole" }, \ + { XFS_IO_DELALLOC, "delalloc" }, \ + { XFS_IO_UNWRITTEN, "unwritten" }, \ + { XFS_IO_OVERWRITE, "overwrite" }, \ + { XFS_IO_COW, "CoW" }/* * Structure for buffered I/O completions.
Signature · 1 line
2.19.0
Re: [PATCH 1/7] xfs: remove XFS_IO_INVALID
Quoted 6 lines · Christoph Hellwig
The invalid state isn't any different from a hole, so merge the two states. Use the more descriptive hole name, but keep it as the first value of the enum to catch uninitialized fields.
Signed-off-by Christoph Hellwig <[email protected]>---
Reviewed-by Brian Foster <[email protected]>
Quoted 61 lines
fs/xfs/xfs_aops.c | 4 ++-- fs/xfs/xfs_aops.h | 14 ++++++-------- 2 files changed, 8 insertions(+), 10 deletions(-)fs/xfs/xfs_aops.c +2
-2@@ -917,7 +917,7 @@ xfs_vm_writepage( struct writeback_control *wbc) {struct xfs_writepage_ctx wpc = { - .io_type =XFS_IO_INVALID , + .io_type =XFS_IO_HOLE , };int ret;@@ -933,7 +933,7 @@ xfs_vm_writepages( struct writeback_control *wbc) {struct xfs_writepage_ctx wpc = { - .io_type =XFS_IO_INVALID , + .io_type =XFS_IO_HOLE , };int ret;fs/xfs/xfs_aops.h +6
-8@@ -12,21 +12,19 @@ extern struct bio_set xfs_ioend_bioset;* Types of I/O for bmap clustering and I/O completion tracking. */ enum { -XFS_IO_INVALID ,/* initial state */ +XFS_IO_HOLE ,/* covers region without any block allocation */ XFS_IO_DELALLOC ,/* covers delalloc region */ XFS_IO_UNWRITTEN ,/* covers allocated but uninitialized data */ XFS_IO_OVERWRITE ,/* covers already allocated extent */ XFS_IO_COW ,/* covers copy-on-write extent */ -XFS_IO_HOLE ,/* covers region without any block allocation */ };#define XFS_IO_TYPES \ - { XFS_IO_INVALID, "invalid" }, \ - { XFS_IO_DELALLOC, "delalloc" }, \ - { XFS_IO_UNWRITTEN, "unwritten" }, \ - { XFS_IO_OVERWRITE, "overwrite" }, \ - { XFS_IO_COW, "CoW" }, \ - { XFS_IO_HOLE, "hole" } + { XFS_IO_HOLE, "hole" }, \ + { XFS_IO_DELALLOC, "delalloc" }, \ + { XFS_IO_UNWRITTEN, "unwritten" }, \ + { XFS_IO_OVERWRITE, "overwrite" }, \ + { XFS_IO_COW, "CoW" }/* * Structure for buffered I/O completions. Signature · 1 line
2.19.0
PATCH 2/7 xfs: always allocate blocks as unwritten for file data
In reply to Christoph HellwigXFS historically had a small race that could lead to exposing uninitialized data in case of a crash. If we are filling holes using buffered I/O we convert the delayed allocation to a real allocation before writing out the data. If we crash after the blocks were allocated, but before the data was written this could lead to reading uninitialized blocks (or leaked data from a previous allocation that was reused). Now that we have the CIL logging extent format changes is cheap, so we can switch to always allocating blocks as unwritten. Note that this is not be strictly necessary for writes that append beyond i_size, but given that we have to log a transaction in that case anyway we might as well give all block allocations a uniform treatment.
Signed-off-by Christoph Hellwig <[email protected]>
--- fs/xfs/xfs_aops.c | 3 +-- fs/xfs/xfs_aops.h | 2 -- fs/xfs/xfs_iomap.c | 4 ++-- 3 files changed, 3 insertions(+), 6 deletions(-)
fs/xfs/xfs_aops.c +1 -2
@@ -437,8 +437,7 @@ xfs_map_blocks( imap.br_blockcount = cow_fsb - imap.br_startoff;if (isnullstartblock (imap.br_startblock)) { -/* got a delalloc extent */ - wpc->io_type =XFS_IO_DELALLOC ; + wpc->io_type =XFS_IO_UNWRITTEN ; goto allocate_blocks; }
fs/xfs/xfs_aops.h +0 -2
@@ -13,7 +13,6 @@ extern struct bio_set xfs_ioend_bioset;*/ enum {XFS_IO_HOLE ,/* covers region without any block allocation */ -XFS_IO_DELALLOC ,/* covers delalloc region */ XFS_IO_UNWRITTEN ,/* covers allocated but uninitialized data */ XFS_IO_OVERWRITE ,/* covers already allocated extent */ XFS_IO_COW ,/* covers copy-on-write extent */
@@ -21,7 +20,6 @@ enum {#define XFS_IO_TYPES \ { XFS_IO_HOLE, "hole" }, \ - { XFS_IO_DELALLOC, "delalloc" }, \ { XFS_IO_UNWRITTEN, "unwritten" }, \ { XFS_IO_OVERWRITE, "overwrite" }, \ { XFS_IO_COW, "CoW" }
fs/xfs/xfs_iomap.c +2 -2
@@ -662,11 +662,11 @@ xfs_iomap_write_allocate(xfs_trans_t *tp;int nimaps;int error =0 ; -int flags =XFS_BMAPI_DELALLOC ; +int flags =XFS_BMAPI_DELALLOC |XFS_BMAPI_PREALLOC ;int nres;if (whichfork ==XFS_COW_FORK ) - flags |=XFS_BMAPI_COWFORK |XFS_BMAPI_PREALLOC ; + flags |=XFS_BMAPI_COWFORK ;/* * Make sure that the dquots are there.
Signature · 1 line
2.19.0
Re: [PATCH 2/7] xfs: always allocate blocks as unwritten for file data
Quoted 13 lines · Christoph Hellwig
XFS historically had a small race that could lead to exposing uninitialized data in case of a crash. If we are filling holes using buffered I/O we convert the delayed allocation to a real allocation before writing out the data. If we crash after the blocks were allocated, but before the data was written this could lead to reading uninitialized blocks (or leaked data from a previous allocation that was reused). Now that we have the CIL logging extent format changes is cheap, so we can switch to always allocating blocks as unwritten. Note that this is not be strictly necessary for writes that append beyond
i_size, but given that we have to log a transaction in that case anyway we might as well give all block allocations a uniform treatment.
Quoted 2 lines
Signed-off-by Christoph Hellwig <[email protected]>---
It's great that we can finally fix this, particularly with such a simple change. IIRC, the only real thing standing in the way was the buffer head delalloc state management mess.
Quoted 5 lines
fs/xfs/xfs_aops.c | 3 +-- fs/xfs/xfs_aops.h | 2 -- fs/xfs/xfs_iomap.c | 4 ++-- 3 files changed, 3 insertions(+), 6 deletions(-)
...
fs/xfs/xfs_iomap.c xfs_iomap_write_allocate( · line 665
Quoted 10 lines
fs/xfs/xfs_iomap.c +1
-1@@ -662,11 +662,11 @@ xfs_iomap_write_allocate(xfs_trans_t *tp;int nimaps;int error =0 ; -int flags =XFS_BMAPI_DELALLOC ; +int flags =XFS_BMAPI_DELALLOC |XFS_BMAPI_PREALLOC ;
... though I don't quite think this is sufficient. xfs_bmapi_allocate() has this snippet of code:
if ((!bma->wasdel || (bma->flags & XFS_BMAPI_COWFORK)) &&
(bma->flags & XFS_BMAPI_PREALLOC) &&
xfs_sb_version_hasextflgbit(&mp->m_sb))
bma->got.br_state = XFS_EXT_UNWRITTEN;... which looks like it explicitly bypasses the PREALLOC flag for delalloc extents. I figured this would just be an inefficiency since prealloc conversion comes later, but if you look at xfs_bmapi_convert_unwritten():
/* check if we need to do real->unwritten conversion */
if (mval->br_state == XFS_EXT_NORM &&
(flags & (XFS_BMAPI_PREALLOC | XFS_BMAPI_CONVERT)) !=
(XFS_BMAPI_PREALLOC | XFS_BMAPI_CONVERT))
return 0;... it sees this as an existing extent and so doesn't change the state unless the CONVERT flag is also passed. A quick test to shut down immediately after the xfs_iomap_write_allocate() transaction commits seems to confirm this behavior:
# xfs_io -c "fiemap -v" /mnt/file /mnt/file: EXT: FILE-OFFSET BLOCK-RANGE TOTAL FLAGS 0: [0..7]: 72..79 8 0x1
I think the right fix here is to remove the referenced logic from xfs_bmapi_allocate(). I also think this demonstrates the need for an xfstest. ;) Expected behavior should be easy to confirm with a new error tag, for example.
Brian
Quoted 11 lines
fs/xfs/xfs_iomap.c +1
-1int nres;if (whichfork ==XFS_COW_FORK ) - flags |=XFS_BMAPI_COWFORK |XFS_BMAPI_PREALLOC ; + flags |=XFS_BMAPI_COWFORK ;/* * Make sure that the dquots are there. Signature · 1 line
2.19.0
PATCH 3/7 xfs: handle zeroing in xfs_file_iomap_begin_delay
In reply to Christoph HellwigWe only need to allocate blocks for zeroing for reflink inodes, and for we currently have a special case for reflink files in the otherwise direct I/O path that I'd like to get rid of.
Signed-off-by Christoph Hellwig <[email protected]>
--- fs/xfs/xfs_iomap.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)
fs/xfs/xfs_iomap.c +30 -4
@@ -62,6 +62,21 @@ xfs_bmbt_to_iomap( iomap->dax_dev =xfs_find_daxdev_for_inode (VFS_I (ip)); } +static void +xfs_hole_to_iomap ( +struct xfs_inode *ip, +struct iomap *iomap, +xfs_fileoff_t offset_fsb, +xfs_fileoff_t end_fsb) +{ + iomap->addr =IOMAP_NULL_ADDR ; + iomap->type =IOMAP_HOLE ; + iomap->offset =XFS_FSB_TO_B (ip->i_mount, offset_fsb); + iomap->length =XFS_FSB_TO_B (ip->i_mount, end_fsb - offset_fsb); + iomap->bdev =xfs_find_bdev_for_inode (VFS_I (ip)); + iomap->dax_dev =xfs_find_daxdev_for_inode (VFS_I (ip)); +} + xfs_extlen_t xfs_eof_alignment( struct xfs_inode *ip,
@@ -502,6 +517,7 @@ xfs_file_iomap_begin_delay(struct inode *inode, loff_t offset, loff_t count, + unsigned flags, struct iomap *iomap) { struct xfs_inode *ip = XFS_I(inode);
@@ -539,8 +555,12 @@ xfs_file_iomap_begin_delay( } eof = !xfs_iext_lookup_extent (ip, ifp, offset_fsb, &icur, &got); -if (!eof && got.br_startoff <= offset_fsb) { -if (xfs_is_reflink_inode (ip)) { +if (eof) + got.br_startoff = maxbytes_fsb; +if (got.br_startoff <= offset_fsb) { +if (xfs_is_reflink_inode (ip) && + ((flags &IOMAP_WRITE ) || + got.br_state !=XFS_EXT_UNWRITTEN )) {bool shared; end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
@@ -555,6 +575,11 @@ xfs_file_iomap_begin_delay( goto done; } +if (flags &IOMAP_ZERO ) { +xfs_hole_to_iomap (ip, iomap, offset_fsb, got.br_startoff); + goto out_unlock; + } + error =xfs_qm_dqattach_locked (ip, false);if (error) goto out_unlock;
@@ -1009,10 +1034,11 @@ xfs_file_iomap_begin(if (XFS_FORCED_SHUTDOWN (mp))return -EIO ; -if (((flags & (IOMAP_WRITE |IOMAP_DIRECT )) ==IOMAP_WRITE ) && +if ((flags & (IOMAP_WRITE |IOMAP_ZERO )) && !(flags &IOMAP_DIRECT ) && !IS_DAX (inode) && !xfs_get_extsz_hint (ip)) {/* Reserve delalloc blocks for regular writeback. */ -return xfs_file_iomap_begin_delay (inode, offset, length, iomap); +return xfs_file_iomap_begin_delay (inode, offset, length, flags, + iomap); }/*
Signature · 1 line
2.19.0
Re: [PATCH 3/7] xfs: handle zeroing in xfs_file_iomap_begin_delay
Quoted 44 lines · Christoph Hellwig
We only need to allocate blocks for zeroing for reflink inodes, and for we currently have a special case for reflink files in the otherwise direct I/O path that I'd like to get rid of.
Signed-off-by Christoph Hellwig <[email protected]>--- fs/xfs/xfs_iomap.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)fs/xfs/xfs_iomap.c +16
-0@@ -62,6 +62,21 @@ xfs_bmbt_to_iomap( iomap->dax_dev =xfs_find_daxdev_for_inode (VFS_I (ip)); } +static void +xfs_hole_to_iomap ( +struct xfs_inode *ip, +struct iomap *iomap, +xfs_fileoff_t offset_fsb, +xfs_fileoff_t end_fsb) +{ + iomap->addr =IOMAP_NULL_ADDR ; + iomap->type =IOMAP_HOLE ; + iomap->offset =XFS_FSB_TO_B (ip->i_mount, offset_fsb); + iomap->length =XFS_FSB_TO_B (ip->i_mount, end_fsb - offset_fsb); + iomap->bdev =xfs_find_bdev_for_inode (VFS_I (ip)); + iomap->dax_dev =xfs_find_daxdev_for_inode (VFS_I (ip)); +} + xfs_extlen_t xfs_eof_alignment( struct xfs_inode *ip,@@ -502,6 +517,7 @@ xfs_file_iomap_begin_delay(struct inode *inode, loff_t offset, loff_t count, + unsigned flags, struct iomap *iomap) { struct xfs_inode *ip = XFS_I(inode);
fs/xfs/xfs_iomap.c xfs_file_iomap_begin_delay( · line 559
Quoted 7 lines
fs/xfs/xfs_iomap.c +2
-2@@ -539,8 +555,12 @@ xfs_file_iomap_begin_delay( } eof = !xfs_iext_lookup_extent (ip, ifp, offset_fsb, &icur, &got); -if (!eof && got.br_startoff <= offset_fsb) { -if (xfs_is_reflink_inode (ip)) { +if (eof) + got.br_startoff = maxbytes_fsb;
What's the purpose of this? Can't we just continue to use eof in the logic below and report holes up through the requested range (offset + length) just like the other branch does (via xfs_bmapi_read())?
fs/xfs/xfs_iomap.c xfs_file_iomap_begin_delay( · line 563
Quoted 4 lines
fs/xfs/xfs_iomap.c +4
-0+if (got.br_startoff <= offset_fsb) { +if (xfs_is_reflink_inode (ip) && + ((flags &IOMAP_WRITE ) || + got.br_state !=XFS_EXT_UNWRITTEN )) {
I think a small comment is useful here due to the implicit logic. For example:
/* reservation is required for writes and zeroing over normal extents */
Otherwise this seems fine.
Brian
Quoted 32 lines
fs/xfs/xfs_iomap.c +8
-2bool shared; end_fsb = min(XFS_B_TO_FSB(mp, offset + count),@@ -555,6 +575,11 @@ xfs_file_iomap_begin_delay( goto done; } +if (flags &IOMAP_ZERO ) { +xfs_hole_to_iomap (ip, iomap, offset_fsb, got.br_startoff); + goto out_unlock; + } + error =xfs_qm_dqattach_locked (ip, false);if (error) goto out_unlock;@@ -1009,10 +1034,11 @@ xfs_file_iomap_begin(if (XFS_FORCED_SHUTDOWN (mp))return -EIO ; -if (((flags & (IOMAP_WRITE |IOMAP_DIRECT )) ==IOMAP_WRITE ) && +if ((flags & (IOMAP_WRITE |IOMAP_ZERO )) && !(flags &IOMAP_DIRECT ) && !IS_DAX (inode) && !xfs_get_extsz_hint (ip)) {/* Reserve delalloc blocks for regular writeback. */ -return xfs_file_iomap_begin_delay (inode, offset, length, iomap); +return xfs_file_iomap_begin_delay (inode, offset, length, flags, + iomap); }/* Signature · 1 line
2.19.0
Re: [PATCH 3/7] xfs: handle zeroing in xfs_file_iomap_begin_delay
Quoted 9 lines · Brian Foster
fs/xfs/xfs_iomap.c xfs_file_iomap_begin_delay( · line 559
Quoted 5 lines
fs/xfs/xfs_iomap.c +2
-2eof = !xfs_iext_lookup_extent (ip, ifp, offset_fsb, &icur, &got); -if (!eof && got.br_startoff <= offset_fsb) { -if (xfs_is_reflink_inode (ip)) { +if (eof) + got.br_startoff = maxbytes_fsb;What's the purpose of this? Can't we just continue to use eof in the logic below and report holes up through the requested range (offset + length) just like the other branch does (via
xfs_bmapi_read())?
Setting the startblock makes the code a lot simpler. But yes, we could limit the hole to the reporting range.
Quoted 10 lines
fs/xfs/xfs_iomap.c xfs_file_iomap_begin_delay( · line 563
Quoted 5 lines
fs/xfs/xfs_iomap.c +4
-0+if (got.br_startoff <= offset_fsb) { +if (xfs_is_reflink_inode (ip) && + ((flags &IOMAP_WRITE ) || + got.br_state !=XFS_EXT_UNWRITTEN )) {I think a small comment is useful here due to the implicit logic. For example:
/* reservation is required for writes and zeroing over normal extents */
Ok.
PATCH 4/7 xfs: remove the unused shared argument to xfs_reflink_reserve_cow
In reply to Christoph HellwigSigned-off-by Christoph Hellwig <[email protected]>
--- fs/xfs/xfs_iomap.c | 6 ++---- fs/xfs/xfs_reflink.c | 12 +++++------- fs/xfs/xfs_reflink.h | 2 +- 3 files changed, 8 insertions(+), 12 deletions(-)
fs/xfs/xfs_iomap.c +2 -4
@@ -561,12 +561,10 @@ xfs_file_iomap_begin_delay(if (xfs_is_reflink_inode (ip) && ((flags &IOMAP_WRITE ) || got.br_state !=XFS_EXT_UNWRITTEN )) { -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); + error =xfs_reflink_reserve_cow (ip, &got);if (error) goto out_unlock; }
@@ -1091,7 +1089,7 @@ xfs_file_iomap_begin(if (error) goto out_unlock; }else { - error =xfs_reflink_reserve_cow (ip, &imap, &shared); + error =xfs_reflink_reserve_cow (ip, &imap);if (error) goto out_unlock; }
fs/xfs/xfs_reflink.c +5 -7
@@ -241,7 +241,7 @@ xfs_reflink_trim_around_shared(/* * 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. +* COW fork. * * 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
@@ -250,14 +250,14 @@ xfs_reflink_trim_around_shared(int xfs_reflink_reserve_cow (struct xfs_inode *ip, -struct xfs_bmbt_irec *imap, -bool *shared) +struct xfs_bmbt_irec *imap) {struct xfs_ifork *ifp =XFS_IFORK_PTR (ip,XFS_COW_FORK );struct xfs_bmbt_irec got;int error =0 ;bool eof = false, trimmed;struct xfs_iext_cursor icur; +bool shared;/* * Search the COW fork extent list first. This serves two purposes:
@@ -273,18 +273,16 @@ xfs_reflink_reserve_cow(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); - - *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)return error;/* Not shared? Just report the (potentially capped) extent. */ -if (!*shared) +if (!shared)return 0 ;/*
fs/xfs/xfs_reflink.h +1 -1
@@ -13,7 +13,7 @@ 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 (struct xfs_inode *ip, -struct xfs_bmbt_irec *imap,bool *shared); +struct xfs_bmbt_irec *imap);extern int xfs_reflink_allocate_cow (struct xfs_inode *ip,struct xfs_bmbt_irec *imap,bool *shared,uint *lockmode); extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
Signature · 1 line
2.19.0
Re: [PATCH 4/7] xfs: remove the unused shared argument to xfs_reflink_reserve_cow
Quoted 2 lines · Christoph Hellwig
Signed-off-by Christoph Hellwig <[email protected]>---
Reviewed-by Brian Foster <[email protected]>
Quoted 99 lines
fs/xfs/xfs_iomap.c | 6 ++---- fs/xfs/xfs_reflink.c | 12 +++++------- fs/xfs/xfs_reflink.h | 2 +- 3 files changed, 8 insertions(+), 12 deletions(-)fs/xfs/xfs_iomap.c +2
-4@@ -561,12 +561,10 @@ xfs_file_iomap_begin_delay(if (xfs_is_reflink_inode (ip) && ((flags &IOMAP_WRITE ) || got.br_state !=XFS_EXT_UNWRITTEN )) { -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); + error =xfs_reflink_reserve_cow (ip, &got);if (error) goto out_unlock; }@@ -1091,7 +1089,7 @@ xfs_file_iomap_begin(if (error) goto out_unlock; }else { - error =xfs_reflink_reserve_cow (ip, &imap, &shared); + error =xfs_reflink_reserve_cow (ip, &imap);if (error) goto out_unlock; }fs/xfs/xfs_reflink.c +5
-7@@ -241,7 +241,7 @@ xfs_reflink_trim_around_shared(/* * 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. +* COW fork. * * 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 @@ -250,14 +250,14 @@ xfs_reflink_trim_around_shared(int xfs_reflink_reserve_cow (struct xfs_inode *ip, -struct xfs_bmbt_irec *imap, -bool *shared) +struct xfs_bmbt_irec *imap) {struct xfs_ifork *ifp =XFS_IFORK_PTR (ip,XFS_COW_FORK );struct xfs_bmbt_irec got;int error =0 ;bool eof = false, trimmed;struct xfs_iext_cursor icur; +bool shared;/* * Search the COW fork extent list first. This serves two purposes: @@ -273,18 +273,16 @@ xfs_reflink_reserve_cow(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); - - *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)return error;/* Not shared? Just report the (potentially capped) extent. */ -if (!*shared) +if (!shared)return 0 ;/* fs/xfs/xfs_reflink.h +1
-1@@ -13,7 +13,7 @@ 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 (struct xfs_inode *ip, -struct xfs_bmbt_irec *imap,bool *shared); +struct xfs_bmbt_irec *imap);extern int xfs_reflink_allocate_cow (struct xfs_inode *ip,struct xfs_bmbt_irec *imap,bool *shared,uint *lockmode); extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,Signature · 1 line
2.19.0
PATCH 5/7 xfs: remove the unused trimmed argument from xfs_reflink_trim_around_shared
In reply to Christoph HellwigSigned-off-by Christoph Hellwig <[email protected]>
--- fs/xfs/xfs_bmap_util.c | 4 ++-- fs/xfs/xfs_iomap.c | 5 ++--- fs/xfs/xfs_reflink.c | 15 +++++---------- fs/xfs/xfs_reflink.h | 2 +- 4 files changed, 10 insertions(+), 16 deletions(-)
fs/xfs/xfs_bmap_util.c +2 -2
@@ -406,10 +406,10 @@ xfs_getbmap_report_one( struct xfs_bmbt_irec *got) {struct kgetbmap *p = out + bmv->bmv_entries; -bool shared = false, trimmed = false; +bool shared = false;int error; - error =xfs_reflink_trim_around_shared (ip, got, &shared, &trimmed); + error =xfs_reflink_trim_around_shared (ip, got, &shared);if (error)return error;
fs/xfs/xfs_iomap.c +2 -3
@@ -1026,7 +1026,7 @@ xfs_file_iomap_begin(struct xfs_bmbt_irec imap;xfs_fileoff_t offset_fsb, end_fsb;int nimaps =1 , error =0 ; -bool shared = false, trimmed = false; +bool shared = false;unsigned lockmode;if (XFS_FORCED_SHUTDOWN (mp))
@@ -1062,8 +1062,7 @@ xfs_file_iomap_begin(if (flags &IOMAP_REPORT ) {/* 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);if (error) goto out_unlock; }
fs/xfs/xfs_reflink.c +5 -10
@@ -182,8 +182,7 @@ intxfs_reflink_trim_around_shared ( struct xfs_inode *ip, struct xfs_bmbt_irec *irec, - bool *shared, - bool *trimmed) + bool *shared) { xfs_agnumber_t agno;xfs_agblock_t agbno;
@@ -209,7 +208,7 @@ xfs_reflink_trim_around_shared(if (error)return error; - *shared = *trimmed = false; + *shared = false;if (fbno ==NULLAGBLOCK ) {/* No shared blocks at all. */ return 0 ;
@@ -222,8 +221,6 @@ xfs_reflink_trim_around_shared(*/ irec->br_blockcount = flen; *shared = true; -if (flen != aglen) - *trimmed = true;return 0 ; }else {/*
@@ -233,7 +230,6 @@ xfs_reflink_trim_around_shared(* start of the shared region. */ irec->br_blockcount = fbno - agbno; - *trimmed = true;return 0 ; } }
@@ -255,7 +251,7 @@ xfs_reflink_reserve_cow(struct xfs_ifork *ifp =XFS_IFORK_PTR (ip,XFS_COW_FORK );struct xfs_bmbt_irec got;int error =0 ; -bool eof = false, trimmed; +bool eof = false;struct xfs_iext_cursor icur;bool shared;
@@ -277,7 +273,7 @@ xfs_reflink_reserve_cow( }/* 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);if (error)return error;
@@ -366,7 +362,6 @@ xfs_find_trim_cow_extent(xfs_filblks_t count_fsb = imap->br_blockcount;struct xfs_iext_cursor icur;struct xfs_bmbt_irec got; -bool trimmed; *found = false;
@@ -376,7 +371,7 @@ xfs_find_trim_cow_extent(*/ if (!xfs_iext_lookup_extent (ip, ip->i_cowfp, offset_fsb, &icur, &got) || got.br_startoff > offset_fsb) -return xfs_reflink_trim_around_shared (ip, imap, shared, &trimmed); +return xfs_reflink_trim_around_shared (ip, imap, shared); *shared = true;if (isnullstartblock (got.br_startblock)) {
fs/xfs/xfs_reflink.h +1 -1
@@ -10,7 +10,7 @@ extern int xfs_reflink_find_shared(struct xfs_mount *mp, struct xfs_trans *tp, xfs_agnumber_t agno, xfs_agblock_t agbno, xfs_extlen_t aglen, xfs_agblock_t *fbno, xfs_extlen_t *flen, bool find_maximal);extern int xfs_reflink_trim_around_shared (struct xfs_inode *ip, -struct xfs_bmbt_irec *irec,bool *shared,bool *trimmed); +struct xfs_bmbt_irec *irec,bool *shared);extern int xfs_reflink_reserve_cow (struct xfs_inode *ip,struct xfs_bmbt_irec *imap);
Signature · 1 line
2.19.0
Re: [PATCH 5/7] xfs: remove the unused trimmed argument from xfs_reflink_trim_around_shared
Quoted 2 lines · Christoph Hellwig
Signed-off-by Christoph Hellwig <[email protected]>---
Reviewed-by Brian Foster <[email protected]>
Quoted 137 lines
fs/xfs/xfs_bmap_util.c | 4 ++-- fs/xfs/xfs_iomap.c | 5 ++--- fs/xfs/xfs_reflink.c | 15 +++++---------- fs/xfs/xfs_reflink.h | 2 +- 4 files changed, 10 insertions(+), 16 deletions(-)fs/xfs/xfs_bmap_util.c +2
-2@@ -406,10 +406,10 @@ xfs_getbmap_report_one( struct xfs_bmbt_irec *got) {struct kgetbmap *p = out + bmv->bmv_entries; -bool shared = false, trimmed = false; +bool shared = false;int error; - error =xfs_reflink_trim_around_shared (ip, got, &shared, &trimmed); + error =xfs_reflink_trim_around_shared (ip, got, &shared);if (error)return error;fs/xfs/xfs_iomap.c +2
-3@@ -1026,7 +1026,7 @@ xfs_file_iomap_begin(struct xfs_bmbt_irec imap;xfs_fileoff_t offset_fsb, end_fsb;int nimaps =1 , error =0 ; -bool shared = false, trimmed = false; +bool shared = false;unsigned lockmode;if (XFS_FORCED_SHUTDOWN (mp))@@ -1062,8 +1062,7 @@ xfs_file_iomap_begin(if (flags &IOMAP_REPORT ) {/* 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);if (error) goto out_unlock; }fs/xfs/xfs_reflink.c +5
-10@@ -182,8 +182,7 @@ intxfs_reflink_trim_around_shared ( struct xfs_inode *ip, struct xfs_bmbt_irec *irec, - bool *shared, - bool *trimmed) + bool *shared) { xfs_agnumber_t agno;xfs_agblock_t agbno;@@ -209,7 +208,7 @@ xfs_reflink_trim_around_shared(if (error)return error; - *shared = *trimmed = false; + *shared = false;if (fbno ==NULLAGBLOCK ) {/* No shared blocks at all. */ return 0 ;@@ -222,8 +221,6 @@ xfs_reflink_trim_around_shared(*/ irec->br_blockcount = flen; *shared = true; -if (flen != aglen) - *trimmed = true;return 0 ; }else {/* @@ -233,7 +230,6 @@ xfs_reflink_trim_around_shared(* start of the shared region. */ irec->br_blockcount = fbno - agbno; - *trimmed = true;return 0 ; } }@@ -255,7 +251,7 @@ xfs_reflink_reserve_cow(struct xfs_ifork *ifp =XFS_IFORK_PTR (ip,XFS_COW_FORK );struct xfs_bmbt_irec got;int error =0 ; -bool eof = false, trimmed; +bool eof = false;struct xfs_iext_cursor icur;bool shared;@@ -277,7 +273,7 @@ xfs_reflink_reserve_cow( }/* 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);if (error)return error;@@ -366,7 +362,6 @@ xfs_find_trim_cow_extent(xfs_filblks_t count_fsb = imap->br_blockcount;struct xfs_iext_cursor icur;struct xfs_bmbt_irec got; -bool trimmed; *found = false;@@ -376,7 +371,7 @@ xfs_find_trim_cow_extent(*/ if (!xfs_iext_lookup_extent (ip, ip->i_cowfp, offset_fsb, &icur, &got) || got.br_startoff > offset_fsb) -return xfs_reflink_trim_around_shared (ip, imap, shared, &trimmed); +return xfs_reflink_trim_around_shared (ip, imap, shared); *shared = true;if (isnullstartblock (got.br_startblock)) {fs/xfs/xfs_reflink.h +1
-1@@ -10,7 +10,7 @@ extern int xfs_reflink_find_shared(struct xfs_mount *mp, struct xfs_trans *tp, xfs_agnumber_t agno, xfs_agblock_t agbno, xfs_extlen_t aglen, xfs_agblock_t *fbno, xfs_extlen_t *flen, bool find_maximal);extern int xfs_reflink_trim_around_shared (struct xfs_inode *ip, -struct xfs_bmbt_irec *irec,bool *shared,bool *trimmed); +struct xfs_bmbt_irec *irec,bool *shared);extern int xfs_reflink_reserve_cow (struct xfs_inode *ip,struct xfs_bmbt_irec *imap);Signature · 1 line
2.19.0
PATCH 6/7 xfs: fix fork selection in xfs_find_trim_cow_extent
In reply to Christoph HellwigWe should want to write directly into the data fork for blocks that don't have an extent in the COW fork covering them yet.
Signed-off-by Christoph Hellwig <[email protected]>
--- fs/xfs/xfs_reflink.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
fs/xfs/xfs_reflink.c +6 -2
@@ -369,9 +369,13 @@ xfs_find_trim_cow_extent(* If we don't find an overlapping extent, trim the range we need to * allocate to fit the hole we found. */ -if (!xfs_iext_lookup_extent (ip, ip->i_cowfp, offset_fsb, &icur, &got) || - got.br_startoff > offset_fsb) +if (!xfs_iext_lookup_extent (ip, ip->i_cowfp, offset_fsb, &icur, &got)) + got.br_startoff = offset_fsb + count_fsb; +if (got.br_startoff > offset_fsb) { +xfs_trim_extent (imap, imap->br_startoff, + got.br_startoff - imap->br_startoff);return xfs_reflink_trim_around_shared (ip, imap, shared); + } *shared = true;if (isnullstartblock (got.br_startblock)) {
Signature · 1 line
2.19.0
Re: [PATCH 6/7] xfs: fix fork selection in xfs_find_trim_cow_extent
Quoted 5 lines · Christoph Hellwig
We should want to write directly into the data fork for blocks that don't have an extent in the COW fork covering them yet.
Signed-off-by Christoph Hellwig <[email protected]>---
Reviewed-by Brian Foster <[email protected]>
Quoted 26 lines
fs/xfs/xfs_reflink.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)fs/xfs/xfs_reflink.c +6
-2@@ -369,9 +369,13 @@ xfs_find_trim_cow_extent(* If we don't find an overlapping extent, trim the range we need to * allocate to fit the hole we found. */ -if (!xfs_iext_lookup_extent (ip, ip->i_cowfp, offset_fsb, &icur, &got) || - got.br_startoff > offset_fsb) +if (!xfs_iext_lookup_extent (ip, ip->i_cowfp, offset_fsb, &icur, &got)) + got.br_startoff = offset_fsb + count_fsb; +if (got.br_startoff > offset_fsb) { +xfs_trim_extent (imap, imap->br_startoff, + got.br_startoff - imap->br_startoff);return xfs_reflink_trim_around_shared (ip, imap, shared); + } *shared = true;if (isnullstartblock (got.br_startblock)) {Signature · 1 line
2.19.0
PATCH 7/7 xfs: print dangling delalloc extents
In reply to Christoph HellwigInstead of just asserting that we have no delalloc space dangling in an inode that gets freed print the actual offenders for debug mode.
Signed-off-by Christoph Hellwig <[email protected]>
Reviewed-by Darrick J. Wong <[email protected]>
--- fs/xfs/xfs_super.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)
fs/xfs/xfs_super.c +32 -1
@@ -933,6 +933,32 @@ xfs_fs_alloc_inode(return NULL ; } +#ifdef DEBUG +static void +xfs_check_delalloc ( +struct xfs_inode *ip, +int whichfork) +{ +struct xfs_ifork *ifp =XFS_IFORK_PTR (ip, whichfork); +struct xfs_bmbt_irec got; +struct xfs_iext_cursor icur; + +if (!ifp || !xfs_iext_lookup_extent (ip, ifp,0 , &icur, &got)) +return ; +do { +if (isnullstartblock (got.br_startblock)) { +xfs_warn (ip->i_mount, +"ino %llx %s fork has delalloc extent at [0x%llx:0x%llx]" , + ip->i_ino, + whichfork ==XFS_DATA_FORK ?"data" :"cow" , + got.br_startoff, got.br_blockcount); + } + }while (xfs_iext_next_extent (ifp, &icur, &got)); +} +#else +#define xfs_check_delalloc (ip, whichfork) do { } while (0) +#endif +/* * Now that the generic code is guaranteed not to be accessing * the linux inode, we can inactivate and reclaim the inode.
@@ -951,7 +977,12 @@ xfs_fs_destroy_inode(xfs_inactive (ip); -ASSERT (XFS_FORCED_SHUTDOWN (ip->i_mount) || ip->i_delayed_blks ==0 ); +if (!XFS_FORCED_SHUTDOWN (ip->i_mount) && ip->i_delayed_blks) { +xfs_check_delalloc (ip,XFS_DATA_FORK ); +xfs_check_delalloc (ip,XFS_COW_FORK ); +ASSERT (0 ); + } +XFS_STATS_INC (ip->i_mount, vn_reclaim);/*
Signature · 1 line
2.19.0
Re: [PATCH 7/7] xfs: print dangling delalloc extents
Quoted 7 lines · Christoph Hellwig
Instead of just asserting that we have no delalloc space dangling in an inode that gets freed print the actual offenders for debug mode.
Signed-off-by Christoph Hellwig <[email protected]>Reviewed-by Darrick J. Wong <[email protected]>---
Thanks for the tweak:
Reviewed-by Brian Foster <[email protected]>
Quoted 57 lines
fs/xfs/xfs_super.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)fs/xfs/xfs_super.c +32
-1@@ -933,6 +933,32 @@ xfs_fs_alloc_inode(return NULL ; } +#ifdef DEBUG +static void +xfs_check_delalloc ( +struct xfs_inode *ip, +int whichfork) +{ +struct xfs_ifork *ifp =XFS_IFORK_PTR (ip, whichfork); +struct xfs_bmbt_irec got; +struct xfs_iext_cursor icur; + +if (!ifp || !xfs_iext_lookup_extent (ip, ifp,0 , &icur, &got)) +return ; +do { +if (isnullstartblock (got.br_startblock)) { +xfs_warn (ip->i_mount, +"ino %llx %s fork has delalloc extent at [0x%llx:0x%llx]" , + ip->i_ino, + whichfork ==XFS_DATA_FORK ?"data" :"cow" , + got.br_startoff, got.br_blockcount); + } + }while (xfs_iext_next_extent (ifp, &icur, &got)); +} +#else +#define xfs_check_delalloc (ip, whichfork) do { } while (0) +#endif +/* * Now that the generic code is guaranteed not to be accessing * the linux inode, we can inactivate and reclaim the inode. @@ -951,7 +977,12 @@ xfs_fs_destroy_inode(xfs_inactive (ip); -ASSERT (XFS_FORCED_SHUTDOWN (ip->i_mount) || ip->i_delayed_blks ==0 ); +if (!XFS_FORCED_SHUTDOWN (ip->i_mount) && ip->i_delayed_blks) { +xfs_check_delalloc (ip,XFS_DATA_FORK ); +xfs_check_delalloc (ip,XFS_COW_FORK ); +ASSERT (0 ); + } +XFS_STATS_INC (ip->i_mount, vn_reclaim);/* Signature · 1 line
2.19.0