xfs: don't unconditionally clear the reflink flag on zero-block files
PATCH 2/2 xfs: don't unconditionally clear the reflink flag on zero-block files
If we have speculative cow preallocations hanging around in the cow fork, don't let a truncate operation clear the reflink flag because if we do then there's a chance we'll forget to free those extents when we destroy the incore inode.
--- fs/xfs/xfs_inode.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
fs/xfs/xfs_inode.c +5 -3
@@ -1624,10 +1624,12 @@ xfs_itruncate_extents( goto out;/* -* Clear the reflink flag if we truncated everything. +* Clear the reflink flag if there are no data fork blocks and +* there are no extents staged in the cow fork. */ -if (ip->i_d.di_nblocks ==0 &&xfs_is_reflink_inode (ip)) { - ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ; +if (xfs_is_reflink_inode (ip) && ip->i_cnextents ==0 ) { +if (ip->i_d.di_nblocks ==0 ) + ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ;xfs_inode_clear_cowblocks_tag (ip); }
Quoted 7 lines · Darrick J. Wong
If we have speculative cow preallocations hanging around in the cow fork, don't let a truncate operation clear the reflink flag because if we do then there's a chance we'll forget to free those extents when we destroy the incore inode.
Reported-by Amir Goldstein <[email protected]>Signed-off-by Darrick J. Wong <[email protected]>
Looks good,
Quoted 28 lines
--- fs/xfs/xfs_inode.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)fs/xfs/xfs_inode.c +5
-4@@ -1624,10 +1624,12 @@ xfs_itruncate_extents( goto out;/* -* Clear the reflink flag if we truncated everything. +* Clear the reflink flag if there are no data fork blocks and +* there are no extents staged in the cow fork. */ -if (ip->i_d.di_nblocks ==0 &&xfs_is_reflink_inode (ip)) { - ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ; +if (xfs_is_reflink_inode (ip) && ip->i_cnextents ==0 ) { +if (ip->i_d.di_nblocks ==0 ) + ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ;xfs_inode_clear_cowblocks_tag (ip); } --To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Carlos
Quoted 19 lines · Darrick J. Wong
If we have speculative cow preallocations hanging around in the cow fork, don't let a truncate operation clear the reflink flag because if we do then there's a chance we'll forget to free those extents when we destroy the incore inode.
Reported-by Amir Goldstein <[email protected]>Signed-off-by Darrick J. Wong <[email protected]>--- fs/xfs/xfs_inode.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)fs/xfs/xfs_inode.c +0
-0@@ -1624,10 +1624,12 @@ xfs_itruncate_extents( goto out;/*
fs/xfs/xfs_inode.c xfs_itruncate_extents( · line 1633
Quoted 10 lines
fs/xfs/xfs_inode.c +5
-3-* Clear the reflink flag if we truncated everything. +* Clear the reflink flag if there are no data fork blocks and +* there are no extents staged in the cow fork. */ -if (ip->i_d.di_nblocks ==0 &&xfs_is_reflink_inode (ip)) { - ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ; +if (xfs_is_reflink_inode (ip) && ip->i_cnextents ==0 ) { +if (ip->i_d.di_nblocks ==0 ) + ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ;xfs_inode_clear_cowblocks_tag (ip);
This part looks ok:
But while looking at that I decided I'd want to take a look at other places where we potentially clear XFS_DIFLAG2_REFLINK.
In xfs_ioctl_setattr_xflags it seems like it's not currently safe for setting the RT flag vs having COW extents? I also think we'd need the iolock for every change to the rt flag, so this probably needs a bigger rework..
swapext also doesn't seem to flush out existing cow extents.
Quoted 33 lines · Christoph Hellwig
Quoted 19 lines · Darrick J. Wong
If we have speculative cow preallocations hanging around in the cow fork, don't let a truncate operation clear the reflink flag because if we do then there's a chance we'll forget to free those extents when we destroy the incore inode.
Reported-by Amir Goldstein <[email protected]>Signed-off-by Darrick J. Wong <[email protected]>--- fs/xfs/xfs_inode.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)fs/xfs/xfs_inode.c +0
-0@@ -1624,10 +1624,12 @@ xfs_itruncate_extents( goto out;/* fs/xfs/xfs_inode.c xfs_itruncate_extents( · line 1633
Quoted 10 lines
fs/xfs/xfs_inode.c +5
-3-* Clear the reflink flag if we truncated everything. +* Clear the reflink flag if there are no data fork blocks and +* there are no extents staged in the cow fork. */ -if (ip->i_d.di_nblocks ==0 &&xfs_is_reflink_inode (ip)) { - ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ; +if (xfs_is_reflink_inode (ip) && ip->i_cnextents ==0 ) { +if (ip->i_d.di_nblocks ==0 ) + ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK ;xfs_inode_clear_cowblocks_tag (ip);This part looks ok:
Quoted 7 lines
Reviewed-by Christoph Hellwig <[email protected]>But while looking at that I decided I'd want to take a look at other places where we potentially clear
XFS_DIFLAG2_REFLINK.In
xfs_ioctl_setattr_xflagsit seems like it's not currently safe for setting the RT flag vs having COW extents?
Yeah... if we set the rt flag (which for now implies clearing reflink) we also have to kill everything in the cow fork.
Quoted 2 lines
I also think we'd need the iolock for every change to the rt flag, so this probably needs a bigger rework..
<eep>
Quoted 1 line
swapext also doesn't seem to flush out existing cow extents.
Yes, it swaps the cow forks too... except that there's a bug where we only swap them if the reflink flags mismatch on ip/tip, and don't swap i_cnextents at all, ever. Ugggh.
--D
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html