On Fri, Jan 26, 2018 at 08:04:29AM -0500, Brian Foster wrote:

On Thu, Jan 25, 2018 at 11:21:42AM -0800, Darrick J. Wong wrote:

quoted

On Thu, Jan 25, 2018 at 08:06:45AM -0500, Brian Foster wrote:

quoted

On Tue, Jan 23, 2018 at 06:18:29PM -0800, Darrick J. Wong wrote:

quoted

From: Darrick J. Wong <redacted>

Track the number of blocks reserved in the CoW fork so that we can
move the quota reservations whenever we chown, and don't account for
CoW fork delalloc reservations in i_delayed_blks.  This should make
chown work properly for quota reservations, enables us to fully
account for real extents in the cow fork in the file stat info, and
improves the post-eof scanning decisions because we're no longer
confusing data fork delalloc extents with cow fork delalloc extents.

Signed-off-by: Darrick J. Wong <redacted>
---
 fs/xfs/libxfs/xfs_bmap.c      |   16 ++++++++++++----
 fs/xfs/libxfs/xfs_inode_buf.c |    1 +
 fs/xfs/xfs_bmap_util.c        |    5 +++++
 fs/xfs/xfs_icache.c           |    3 ++-
 fs/xfs/xfs_inode.c            |   11 +++++------
 fs/xfs/xfs_inode.h            |    1 +
 fs/xfs/xfs_iops.c             |    3 ++-
 fs/xfs/xfs_itable.c           |    3 ++-
 fs/xfs/xfs_qm.c               |    2 +-
 fs/xfs/xfs_reflink.c          |    4 ++--
 fs/xfs/xfs_super.c            |    1 +
 11 files changed, 34 insertions(+), 16 deletions(-)

...

quoted

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 4a38cfc..a208825 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c

...

quoted

@@ -1669,7 +1667,7 @@ xfs_release(
 		truncated = xfs_iflags_test_and_clear(ip, XFS_ITRUNCATED);
 		if (truncated) {
 			xfs_iflags_clear(ip, XFS_IDIRTY_RELEASE);
-			if (ip->i_delayed_blks > 0) {
+			if (ip->i_delayed_blks > 0 || ip->i_cow_blocks > 0) {
 				error = filemap_flush(VFS_I(ip)->i_mapping);
 				if (error)
 					return error;

Is having cowblocks really relevant to this hunk? I thought this was
purely a delalloc vs. file size thing, but I could be wrong. 

AFAICT, if we (1) use truncate to reduce a file's size, (2) write
somewhere past eof, (3) make some delalloc reservations for the post-eof
write, and (4) close the file, then this chunk flushes the dirty data to
disk so that if we crash after the close() call returns, the file will
still have all the data that was written out.  IOWs, this provides for
flush-on-close after a file size reduction.

I think it goes back to problems where those subsequent buffered writes
increase the file size again and the fs crashes before all data is
written out. E.g., the problem described by commit ba87ea699e ("[XFS]
Fix to prevent the notorious 'NULL files' problem after a crash."). It's
not totally clear to me whether that fixed the problem and this
particular hack is still needed.

Me neither.  It looks like deferring the size update until the write
end_io would have closed this bug... but on the other hand maybe its
function is more to avoid disappointing the people who expect flush on
close behavior...

FWIW, the flush code looks like it goes back to commit 7d4fb40ad7
("[XFS] Start writeout earlier (on last close) ...").

quoted

So I was thinking that if a write to a lower offset causes the creation
of a speculative cow extent of some kind that extends past eof, we'd
still want to flush the dirty data to disk on close even if there are no
delalloc reservations in the data fork.

This whole stanza still depends on a truncate in the first place
though..?

I guess I'm not necessarily against doing this, I just think we should
verify whether it's actually useful to prevent some kind of similar
crash-recovery problem it was intended to help mitigate. If not, then
we're subjecting ourselves to the tradeoff, which appears to be that
we'll initiate writeback of any file with cowblocks on close that has
been truncated.

Granted the truncate operation is probably infrequent with respect to
close() so it's probably not that big of a deal, but in the delalloc

It's probably infrequent wrt cow-and-close, but "echo foo > existingfile"
would trigger this for the regular da case.  I don't really mind
dropping it either, aside from my sense of paranoia. :P

case a flush is at least generally expected to clear the file of delayed
allocation. It's my understanding that the same is not necessarily true
for cowblocks.. cow prealloc means blocks can sit around in the cow fork
for a while in anticipation of future copy-on-writes, right?

Yes.

--D

Brian

quoted

Ofc now I see that xfs_file_iomap_begin_delay will create the data fork
da reservation for a non-shared block even if a cow fork extent already
exists (the write is promoted to cow), so perhaps this isn't strictly
necessary... but adding a data fork da extent when there's already a cow
fork extent seems like a (mostly harmless) bug to me.

--D

quoted

Brian

quoted

@@ -1909,7 +1907,8 @@ xfs_inactive(
 
 	if (S_ISREG(VFS_I(ip)->i_mode) &&
 	    (ip->i_d.di_size != 0 || XFS_ISIZE(ip) != 0 ||
-	     ip->i_d.di_nextents > 0 || ip->i_delayed_blks > 0))
+	     ip->i_d.di_nextents > 0 || ip->i_delayed_blks > 0 ||
+	     ip->i_cow_blocks > 0))
 		truncate = 1;
 
 	error = xfs_qm_dqattach(ip, 0);

diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index ff56486..6feee8a 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h

@@ -62,6 +62,7 @@ typedef struct xfs_inode {
 	/* Miscellaneous state. */
 	unsigned long		i_flags;	/* see defined flags below */
 	unsigned int		i_delayed_blks;	/* count of delay alloc blks */
+	unsigned int		i_cow_blocks;	/* count of cow fork blocks */
 
 	struct xfs_icdinode	i_d;		/* most of ondisk inode */
 

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 56475fc..6c3381c 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c

@@ -513,7 +513,8 @@ xfs_vn_getattr(
 	stat->mtime = inode->i_mtime;
 	stat->ctime = inode->i_ctime;
 	stat->blocks =
-		XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
+		XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks +
+				  ip->i_cow_blocks);
 
 	if (ip->i_d.di_version == 3) {
 		if (request_mask & STATX_BTIME) {

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index d583105..412d7eb 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c

@@ -122,7 +122,8 @@ xfs_bulkstat_one_int(
 	case XFS_DINODE_FMT_BTREE:
 		buf->bs_rdev = 0;
 		buf->bs_blksize = mp->m_sb.sb_blocksize;
-		buf->bs_blocks = dic->di_nblocks + ip->i_delayed_blks;
+		buf->bs_blocks = dic->di_nblocks + ip->i_delayed_blks +
+				 ip->i_cow_blocks;
 		break;
 	}
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 5b848f4..28f12f8 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c

@@ -1847,7 +1847,7 @@ xfs_qm_vop_chown_reserve(
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
 	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
 
-	delblks = ip->i_delayed_blks;
+	delblks = ip->i_delayed_blks + ip->i_cow_blocks;
 	blkflags = XFS_IS_REALTIME_INODE(ip) ?
 			XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
 

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index e367351..f875ea7 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c

@@ -619,7 +619,7 @@ xfs_reflink_cancel_cow_blocks(
 	}
 
 	/* clear tag if cow fork is emptied */
-	if (!ifp->if_bytes)
+	if (ip->i_cow_blocks == 0)
 		xfs_inode_clear_cowblocks_tag(ip);
 
 	return error;

@@ -704,7 +704,7 @@ xfs_reflink_end_cow(
 	trace_xfs_reflink_end_cow(ip, offset, count);
 
 	/* No COW extents?  That's easy! */
-	if (ifp->if_bytes == 0)
+	if (ip->i_cow_blocks == 0)
 		return 0;
 
 	offset_fsb = XFS_B_TO_FSBT(ip->i_mount, offset);

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index f3e0001..9d04cfb 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c

@@ -989,6 +989,7 @@ xfs_fs_destroy_inode(
 	xfs_inactive(ip);
 
 	ASSERT(XFS_FORCED_SHUTDOWN(ip->i_mount) || ip->i_delayed_blks == 0);
+	ASSERT(XFS_FORCED_SHUTDOWN(ip->i_mount) || ip->i_cow_blocks == 0);
 	XFS_STATS_INC(ip->i_mount, vn_reclaim);
 
 	/*

--

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

--
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

--
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

--
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