Re: [PATCH 05/11] xfs: track CoW blocks separately in the inode

["Darrick J. Wong" <darrick.wong@oracle.com>]

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:
> > On Thu, Jan 25, 2018 at 08:06:45AM -0500, Brian Foster wrote:
> > > On Tue, Jan 23, 2018 at 06:18:29PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > 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 <darrick.wong@oracle.com>
> > > > ---
> > > >  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(-)
> > > > 
> > > > 
> > > ...
> > > > 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
> > > ...
> > > > @@ -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) ...").
> 
> > 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
> 
> > 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
> > 
> > > 
> > > Brian
> > > 
> > > > @@ -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