[RFC PATCH 0/4] xfs: add support for tracking inodes with post-EOF speculative preallocation

[RFC PATCH 0/4] xfs: add support for tracking inodes with post-EOF speculative preallocation

[Brian Foster]

Hi all,

The following patchset adds support for tracking and clearing inodes with
post-EOF space due to speculative preallocation. The purpose of this work is to
add some flexibility and management control over inodes in this state. The
specific impetus for this work is to support improved quota management in the
glusterfs cluster filesystem. For example, this provides the ability to
potentially free up a significant amount of unwritten space when at or near
EDQUOT (FWIW, I'm also working on a patch to support prealloc throttling near
EDQUOT, but I'm considering that separate). Despite the motivation, this work is
intended to be generic and hopefully useful for other XFS users.

We track inodes with post-EOF space via a new tag in the per-ag inode radix
tree. The tag management, introduced in patch 1, is generally lazy in an
attempt to keep things simple. Patch 2 implements scanning functionality to
locate and potentially trim inodes with post-EOF space. Patch 3 adds a new
ioctl() to extend support to userspace. Patch 4 adds background scanning based
on the existing syncd workqueue. 

This patchset is marked RFC because aside from general review, I have some
questions on the best approach for several bits (marked with 'TODO' comments),
most notably whether we should include new tunables for the background scanning.
Some further testing is warranted, but I suspect that using the syncd default of
30s might be a bit too aggressive for large files.

These patches have been lightly unit tested and run through xfstests. Thoughts
appreciated.

Brian

P.S., Credit to Dave Chinner for early review and guidance on design.

Brian Foster (4):
  xfs: add EOFBLOCKS inode tagging/untagging
  xfs: create function to scan and clear EOFBLOCKS inodes
  xfs: add FREE_EOFBLOCKS ioctl
  xfs: add background scanning to clear EOFBLOCKS inodes

 fs/xfs/xfs_ag.h       |    1 +
 fs/xfs/xfs_fs.h       |   10 ++
 fs/xfs/xfs_inode.h    |    8 ++-
 fs/xfs/xfs_ioctl.c    |   25 +++++
 fs/xfs/xfs_iomap.c    |    7 ++
 fs/xfs/xfs_iops.c     |    3 +
 fs/xfs/xfs_mount.h    |    2 +
 fs/xfs/xfs_quota.h    |    1 +
 fs/xfs/xfs_quotaops.c |    2 +-
 fs/xfs/xfs_sync.c     |  277 +++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_sync.h     |    9 ++
 fs/xfs/xfs_trace.h    |    5 +
 fs/xfs/xfs_vnodeops.c |   16 ++-
 fs/xfs/xfs_vnodeops.h |    2 +
 14 files changed, 362 insertions(+), 6 deletions(-)

-- 
1.7.7.6

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[RFC PATCH 1/4] xfs: add EOFBLOCKS inode tagging/untagging

[Brian Foster]

Add the XFS_ICI_EOFBLOCKS_TAG inode tag to identify inodes with
speculatively preallocated blocks beyond EOF. An inode is tagged
when speculative preallocation occurs and untagged either via
truncate down or when post-EOF blocks are freed via release or
reclaim.

The tag management is intentionally not aggressive to prefer
simplicity over the complexity of handling all the corner cases
under which post-EOF blocks could be freed (i.e., forward
truncation, fallocate, write error conditions, etc.). This means
that a tagged inode may or may not have post-EOF blocks after a
period of time. The tag is eventually cleared when the inode is
released or reclaimed.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_ag.h       |    1 +
 fs/xfs/xfs_inode.h    |    8 ++++-
 fs/xfs/xfs_iomap.c    |    7 ++++
 fs/xfs/xfs_iops.c     |    3 ++
 fs/xfs/xfs_sync.c     |   76 +++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_sync.h     |    3 ++
 fs/xfs/xfs_trace.h    |    5 +++
 fs/xfs/xfs_vnodeops.c |    3 ++
 8 files changed, 105 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_ag.h b/fs/xfs/xfs_ag.h
index 44d65c1..22bd4db 100644
--- a/fs/xfs/xfs_ag.h
+++ b/fs/xfs/xfs_ag.h
@@ -233,6 +233,7 @@ typedef struct xfs_perag {
 #define XFS_ICI_NO_TAG		(-1)	/* special flag for an untagged lookup
 					   in xfs_inode_ag_iterator */
 #define XFS_ICI_RECLAIM_TAG	0	/* inode is to be reclaimed */
+#define XFS_ICI_EOFBLOCKS_TAG	1	/* inode has blocks beyond EOF */
 
 #define	XFS_AG_MAXLEVELS(mp)		((mp)->m_ag_maxlevels)
 #define	XFS_MIN_FREELIST_RAW(bl,cl,mp)	\
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 94b32f9..2cd2883 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -305,10 +305,16 @@ xfs_iflags_set(xfs_inode_t *ip, unsigned short flags)
 }
 
 static inline void
+__xfs_iflags_clear(xfs_inode_t *ip, unsigned short flags)
+{
+	ip->i_flags &= ~flags;
+}
+
+static inline void
 xfs_iflags_clear(xfs_inode_t *ip, unsigned short flags)
 {
 	spin_lock(&ip->i_flags_lock);
-	ip->i_flags &= ~flags;
+	__xfs_iflags_clear(ip, flags);
 	spin_unlock(&ip->i_flags_lock);
 }
 
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 973dff6..2968ee8 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -459,6 +459,13 @@ retry:
 	if (!(imap[0].br_startblock || XFS_IS_REALTIME_INODE(ip)))
 		return xfs_alert_fsblock_zero(ip, &imap[0]);
 
+	/*
+	 * Tag the inode as speculatively preallocated so we can reclaim this
+	 * space on demand, if necessary.
+	 */
+	if (prealloc)
+		xfs_inode_set_eofblocks_tag(ip);
+
 	*ret_imap = imap[0];
 	return 0;
 }
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 4e00cf0..dcd1d5f 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -854,6 +854,9 @@ xfs_setattr_size(
 		 * and do not wait the usual (long) time for writeout.
 		 */
 		xfs_iflags_set(ip, XFS_ITRUNCATED);
+
+		/* A truncate down always removes post-EOF blocks. */
+		xfs_inode_clear_eofblocks_tag(ip);
 	}
 
 	if (mask & ATTR_CTIME) {
diff --git a/fs/xfs/xfs_sync.c b/fs/xfs/xfs_sync.c
index 9654817..5e14741 100644
--- a/fs/xfs/xfs_sync.c
+++ b/fs/xfs/xfs_sync.c
@@ -971,3 +971,79 @@ xfs_reclaim_inodes_count(
 	return reclaimable;
 }
 
+STATIC void
+__xfs_inode_set_eofblocks_tag(
+	struct xfs_perag	*pag,
+	struct xfs_inode	*ip)
+{
+	int tagged = radix_tree_tagged(&pag->pag_ici_root,
+				       XFS_ICI_EOFBLOCKS_TAG);
+
+	radix_tree_tag_set(&pag->pag_ici_root,
+			   XFS_INO_TO_AGINO(ip->i_mount, ip->i_ino),
+			   XFS_ICI_EOFBLOCKS_TAG);
+
+	if (!tagged) {
+		/* propagate the eofblocks tag up into the perag radix tree */
+		spin_lock(&ip->i_mount->m_perag_lock);
+		radix_tree_tag_set(&ip->i_mount->m_perag_tree,
+				   XFS_INO_TO_AGNO(ip->i_mount, ip->i_ino),
+				   XFS_ICI_EOFBLOCKS_TAG);
+		spin_unlock(&ip->i_mount->m_perag_lock);
+
+		trace_xfs_perag_set_eofblocks(ip->i_mount, pag->pag_agno,
+					      -1, _RET_IP_);
+	}
+}
+
+void
+xfs_inode_set_eofblocks_tag(
+	xfs_inode_t	*ip)
+{
+	struct xfs_mount *mp = ip->i_mount;
+	struct xfs_perag *pag;
+
+	pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino));
+	spin_lock(&pag->pag_ici_lock);
+	trace_xfs_set_eofblocks_tag(ip);
+	__xfs_inode_set_eofblocks_tag(pag, ip);
+	spin_unlock(&pag->pag_ici_lock);
+	xfs_perag_put(pag);
+}
+
+STATIC void
+__xfs_inode_clear_eofblocks(
+	xfs_perag_t	*pag,
+	xfs_inode_t	*ip)
+{
+	radix_tree_tag_clear(&pag->pag_ici_root,
+			     XFS_INO_TO_AGINO(ip->i_mount, ip->i_ino),
+			     XFS_ICI_EOFBLOCKS_TAG);
+
+	if (!radix_tree_tagged(&pag->pag_ici_root, XFS_ICI_EOFBLOCKS_TAG)) {
+		/* clear the eofblocks tag from the perag radix tree */
+		spin_lock(&ip->i_mount->m_perag_lock);
+		radix_tree_tag_clear(&ip->i_mount->m_perag_tree,
+				     XFS_INO_TO_AGNO(ip->i_mount, ip->i_ino),
+				     XFS_ICI_EOFBLOCKS_TAG);
+		spin_unlock(&ip->i_mount->m_perag_lock);
+		trace_xfs_perag_clear_eofblocks(ip->i_mount, pag->pag_agno,
+					       -1, _RET_IP_);
+	}
+}
+
+void
+xfs_inode_clear_eofblocks_tag(
+	xfs_inode_t	*ip)
+{
+	struct xfs_mount *mp = ip->i_mount;
+	struct xfs_perag *pag;
+
+	pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino));
+	spin_lock(&pag->pag_ici_lock);
+	trace_xfs_clear_eofblocks_tag(ip);
+	__xfs_inode_clear_eofblocks(pag, ip);
+	spin_unlock(&pag->pag_ici_lock);
+	xfs_perag_put(pag);
+}
+
diff --git a/fs/xfs/xfs_sync.h b/fs/xfs/xfs_sync.h
index 941202e..4486491 100644
--- a/fs/xfs/xfs_sync.h
+++ b/fs/xfs/xfs_sync.h
@@ -43,6 +43,9 @@ void __xfs_inode_set_reclaim_tag(struct xfs_perag *pag, struct xfs_inode *ip);
 void __xfs_inode_clear_reclaim_tag(struct xfs_mount *mp, struct xfs_perag *pag,
 				struct xfs_inode *ip);
 
+void xfs_inode_set_eofblocks_tag(struct xfs_inode *ip);
+void xfs_inode_clear_eofblocks_tag(struct xfs_inode *ip);
+
 int xfs_sync_inode_grab(struct xfs_inode *ip);
 int xfs_inode_ag_iterator(struct xfs_mount *mp,
 	int (*execute)(struct xfs_inode *ip, struct xfs_perag *pag, int flags),
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index e5795dd..47e37da 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -129,6 +129,8 @@ DEFINE_PERAG_REF_EVENT(xfs_perag_get_tag);
 DEFINE_PERAG_REF_EVENT(xfs_perag_put);
 DEFINE_PERAG_REF_EVENT(xfs_perag_set_reclaim);
 DEFINE_PERAG_REF_EVENT(xfs_perag_clear_reclaim);
+DEFINE_PERAG_REF_EVENT(xfs_perag_set_eofblocks);
+DEFINE_PERAG_REF_EVENT(xfs_perag_clear_eofblocks);
 
 TRACE_EVENT(xfs_attr_list_node_descend,
 	TP_PROTO(struct xfs_attr_list_context *ctx,
@@ -584,6 +586,9 @@ DEFINE_INODE_EVENT(xfs_update_time);
 DEFINE_INODE_EVENT(xfs_dquot_dqalloc);
 DEFINE_INODE_EVENT(xfs_dquot_dqdetach);
 
+DEFINE_INODE_EVENT(xfs_set_eofblocks_tag);
+DEFINE_INODE_EVENT(xfs_clear_eofblocks_tag);
+
 DECLARE_EVENT_CLASS(xfs_iref_class,
 	TP_PROTO(struct xfs_inode *ip, unsigned long caller_ip),
 	TP_ARGS(ip, caller_ip),
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 2a5c6373..658ee2e 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -467,6 +467,8 @@ xfs_release(
 		if (error)
 			return error;
 
+		xfs_inode_clear_eofblocks_tag(ip);
+
 		/* delalloc blocks after truncation means it really is dirty */
 		if (ip->i_delayed_blks)
 			xfs_iflags_set(ip, XFS_IDIRTY_RELEASE);
@@ -523,6 +525,7 @@ xfs_inactive(
 			error = xfs_free_eofblocks(mp, ip, false);
 			if (error)
 				return VN_INACTIVE_CACHE;
+			xfs_inode_clear_eofblocks_tag(ip);
 		}
 		goto out;
 	}
-- 
1.7.7.6

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[RFC PATCH 2/4] xfs: create function to scan and clear EOFBLOCKS inodes

[Brian Foster]

xfs_inodes_free_eofblocks() implements scanning functionality for
EOFBLOCKS inodes. It scans the radix tree and frees post-EOF blocks
for inodes that meet particular criteria. The scan can be filtered
by a particular quota type/id and minimum file size. The scan can
also be invoked in trylock mode or wait (force) mode.

The xfs_free_eofblocks() helper is invoked to clear post-EOF space.
It is slightly modified to support an output parameter that
indicates whether space was freed and helps decide whether the
EOFBLOCKS tag should be cleared in trylock scans.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_sync.c     |  168 +++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_sync.h     |    3 +
 fs/xfs/xfs_vnodeops.c |   17 +++--
 fs/xfs/xfs_vnodeops.h |    2 +
 4 files changed, 184 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_sync.c b/fs/xfs/xfs_sync.c
index 5e14741..27c3c46 100644
--- a/fs/xfs/xfs_sync.c
+++ b/fs/xfs/xfs_sync.c
@@ -971,6 +971,174 @@ xfs_reclaim_inodes_count(
 	return reclaimable;
 }
 
+/*
+ * Handle an EOFBLOCKS tagged inode. If this is a forced scan, we wait on the
+ * iolock ourselves rather than rely on the trylock in xfs_free_eofblocks(). 
+ *
+ * We rely on the output parameter from xfs_free_eofblocks() to determine
+ * whether we should clear the tag because in the trylock case, it could have
+ * skipped the inode due to lock contention.
+ */
+STATIC int
+xfs_inode_free_eofblocks(
+	struct xfs_inode	*ip,
+	int			flags)
+{
+	int ret = 0;
+	bool freed = false;
+	bool wait_iolock = (flags & EOFBLOCKS_WAIT) ? true : false;
+
+	if (wait_iolock)
+		xfs_ilock(ip, XFS_IOLOCK_EXCL);
+
+	if ((S_ISREG(ip->i_d.di_mode) &&
+	     (VFS_I(ip)->i_size > 0 ||
+	     (VN_CACHED(VFS_I(ip)) > 0 || ip->i_delayed_blks > 0)) &&
+	     (ip->i_df.if_flags & XFS_IFEXTENTS)) &&
+	    (!(ip->i_d.di_flags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)))) {
+		/* !wait_iolock == need_iolock in xfs_free_eofblocks() */
+		ret = xfs_free_eofblocks(ip->i_mount, ip, !wait_iolock, &freed);
+		if (freed)
+			xfs_inode_clear_eofblocks_tag(ip);
+	} else {
+		/* inode could be preallocated or append-only */
+		xfs_inode_clear_eofblocks_tag(ip);
+	}
+
+	if (wait_iolock)
+		xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+
+	return ret;
+}
+
+/*
+ * Determine whether an inode matches a particular qouta id.
+ */
+STATIC int
+xfs_inode_match_quota_id(
+	struct xfs_inode	*ip,
+	int			qtype,
+	uint32_t		id)
+{
+	switch (qtype) {
+	case XFS_DQ_USER:
+		return ip->i_d.di_uid == id;
+	case XFS_DQ_GROUP:
+		return ip->i_d.di_gid == id;
+	default:
+		return xfs_get_projid(ip) == id;
+	}
+
+	return 0;
+}
+
+/*
+ * This is mostly copied from xfs_reclaim_inodes_ag().
+ *
+ * TODO:
+ * - Could we enhance ag_iterator to support a tag and use it instead of this?
+ */
+int
+xfs_inodes_free_eofblocks(
+	struct xfs_mount	*mp,
+	int			qtype,
+	uint32_t		id,
+	uint64_t		min_file_size,
+	int			flags)
+{
+	struct xfs_perag	*pag;
+	int			error = 0;
+	int			last_error = 0;
+	xfs_agnumber_t		ag;
+
+	ag = 0;
+	while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_EOFBLOCKS_TAG))) {
+		unsigned long	first_index = 0;
+		int		nr_found = 0;
+		int		done = 0;
+
+		ag = pag->pag_agno + 1;
+
+		do {
+			struct xfs_inode *batch[XFS_LOOKUP_BATCH];
+			int	i;
+
+			rcu_read_lock();
+			nr_found = radix_tree_gang_lookup_tag(
+					&pag->pag_ici_root,
+					(void **)batch, first_index,
+					XFS_LOOKUP_BATCH,
+					XFS_ICI_EOFBLOCKS_TAG);
+			if (!nr_found) {
+				rcu_read_unlock();
+				break;
+			}
+
+			/*
+			 * Grab the inodes before we drop the lock. if we found
+			 * nothing, nr == 0 and the loop will be skipped.
+			 */
+			for (i = 0; i < nr_found; i++) {
+				struct xfs_inode *ip = batch[i];
+
+				if (done || xfs_inode_ag_walk_grab(ip))
+					batch[i] = NULL;
+
+				/*
+				 * Update the index for the next lookup. Catch
+				 * overflows into the next AG range which can occur if
+				 * we have inodes in the last block of the AG and we
+				 * are currently pointing to the last inode.
+				 *
+				 * Because we may see inodes that are from the wrong AG
+				 * due to RCU freeing and reallocation, only update the
+				 * index if it lies in this AG. It was a race that lead
+				 * us to see this inode, so another lookup from the
+				 * same index will not find it again.
+				 */
+				if (XFS_INO_TO_AGNO(mp, ip->i_ino) !=
+								pag->pag_agno)
+					continue;
+				first_index = XFS_INO_TO_AGINO(mp, ip->i_ino + 1);
+				if (first_index < XFS_INO_TO_AGINO(mp, ip->i_ino))
+					done = 1;
+			}
+
+			/* unlock now we've grabbed the inodes. */
+			rcu_read_unlock();
+
+			for (i = 0; i < nr_found; i++) {
+				if (!batch[i])
+					continue;
+
+				/* default projid represents a full scan */
+				if ((!(qtype == XFS_DQ_PROJ &&
+				       id == XFS_PROJID_DEFAULT) &&
+				     !xfs_inode_match_quota_id(batch[i], qtype,
+							       id)) ||
+				    (min_file_size && XFS_ISIZE(batch[i]) < 
+								min_file_size)
+				   ) {
+					IRELE(batch[i]);
+					continue;
+				}
+
+				error = xfs_inode_free_eofblocks(batch[i], flags);
+				IRELE(batch[i]);
+				if (error)
+					last_error = error;
+			}
+
+			cond_resched();
+
+		} while (nr_found && !done);
+
+		xfs_perag_put(pag);
+	}
+
+	return XFS_ERROR(last_error);
+}
+
 STATIC void
 __xfs_inode_set_eofblocks_tag(
 	struct xfs_perag	*pag,
diff --git a/fs/xfs/xfs_sync.h b/fs/xfs/xfs_sync.h
index 4486491..78aca41 100644
--- a/fs/xfs/xfs_sync.h
+++ b/fs/xfs/xfs_sync.h
@@ -43,8 +43,11 @@ void __xfs_inode_set_reclaim_tag(struct xfs_perag *pag, struct xfs_inode *ip);
 void __xfs_inode_clear_reclaim_tag(struct xfs_mount *mp, struct xfs_perag *pag,
 				struct xfs_inode *ip);
 
+#define EOFBLOCKS_WAIT		0x0001
+
 void xfs_inode_set_eofblocks_tag(struct xfs_inode *ip);
 void xfs_inode_clear_eofblocks_tag(struct xfs_inode *ip);
+int xfs_inodes_free_eofblocks(struct xfs_mount *, int, uint32_t, uint64_t, int);
 
 int xfs_sync_inode_grab(struct xfs_inode *ip);
 int xfs_inode_ag_iterator(struct xfs_mount *mp,
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 658ee2e..53460f3 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -150,11 +150,12 @@ xfs_readlink(
  * when the link count isn't zero and by xfs_dm_punch_hole() when
  * punching a hole to EOF.
  */
-STATIC int
+int
 xfs_free_eofblocks(
 	xfs_mount_t	*mp,
 	xfs_inode_t	*ip,
-	bool		need_iolock)
+	bool		need_iolock,
+	bool		*blocks_freed)
 {
 	xfs_trans_t	*tp;
 	int		error;
@@ -237,6 +238,9 @@ xfs_free_eofblocks(
 		} else {
 			error = xfs_trans_commit(tp,
 						XFS_TRANS_RELEASE_LOG_RES);
+			if (blocks_freed)
+				*blocks_freed = true;
+
 		}
 
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
@@ -391,6 +395,7 @@ xfs_release(
 {
 	xfs_mount_t	*mp = ip->i_mount;
 	int		error;
+	bool		freed = false;
 
 	if (!S_ISREG(ip->i_d.di_mode) || (ip->i_d.di_mode == 0))
 		return 0;
@@ -463,11 +468,11 @@ xfs_release(
 		if (xfs_iflags_test(ip, XFS_IDIRTY_RELEASE))
 			return 0;
 
-		error = xfs_free_eofblocks(mp, ip, true);
+		error = xfs_free_eofblocks(mp, ip, true, &freed);
 		if (error)
 			return error;
-
-		xfs_inode_clear_eofblocks_tag(ip);
+		if (freed)
+			xfs_inode_clear_eofblocks_tag(ip);
 
 		/* delalloc blocks after truncation means it really is dirty */
 		if (ip->i_delayed_blks)
@@ -522,7 +527,7 @@ xfs_inactive(
 		    (!(ip->i_d.di_flags &
 				(XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)) ||
 		     ip->i_delayed_blks != 0))) {
-			error = xfs_free_eofblocks(mp, ip, false);
+			error = xfs_free_eofblocks(mp, ip, false, NULL);
 			if (error)
 				return VN_INACTIVE_CACHE;
 			xfs_inode_clear_eofblocks_tag(ip);
diff --git a/fs/xfs/xfs_vnodeops.h b/fs/xfs/xfs_vnodeops.h
index 447e146..918d24d 100644
--- a/fs/xfs/xfs_vnodeops.h
+++ b/fs/xfs/xfs_vnodeops.h
@@ -57,5 +57,7 @@ int xfs_flush_pages(struct xfs_inode *ip, xfs_off_t first,
 int xfs_wait_on_pages(struct xfs_inode *ip, xfs_off_t first, xfs_off_t last);
 
 int xfs_zero_eof(struct xfs_inode *, xfs_off_t, xfs_fsize_t);
+int xfs_free_eofblocks(struct xfs_mount *, struct xfs_inode *, bool, bool *);
+
 
 #endif /* _XFS_VNODEOPS_H */
-- 
1.7.7.6

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[RFC PATCH 3/4] xfs: add FREE_EOFBLOCKS ioctl

[Brian Foster]

The XFS_IOC_FREE_EOFBLOCKS ioctl allows users to invoke an EOFBLOCKS
scan. The xfs_eofblocks structure is defined to support the command
parameters (quota type/id and minimum file size).

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_fs.h       |   10 ++++++++++
 fs/xfs/xfs_ioctl.c    |   25 +++++++++++++++++++++++++
 fs/xfs/xfs_quota.h    |    1 +
 fs/xfs/xfs_quotaops.c |    2 +-
 4 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
index c13fed8..6f93db9 100644
--- a/fs/xfs/xfs_fs.h
+++ b/fs/xfs/xfs_fs.h
@@ -339,6 +339,15 @@ typedef struct xfs_error_injection {
 
 
 /*
+ * Speculative preallocation trimming.
+ */
+typedef struct xfs_eofblocks {
+	__u32		id;		/* quota id */
+	__u32		qtype;		/* quota type */
+	__u64		min_file_size;	/* minimum file size */
+} xfs_eofblocks_t;
+
+/*
  * The user-level Handle Request interface structure.
  */
 typedef struct xfs_fsop_handlereq {
@@ -456,6 +465,7 @@ typedef struct xfs_handle {
 /*	XFS_IOC_GETBIOSIZE ---- deprecated 47	   */
 #define XFS_IOC_GETBMAPX	_IOWR('X', 56, struct getbmap)
 #define XFS_IOC_ZERO_RANGE	_IOW ('X', 57, struct xfs_flock64)
+#define XFS_IOC_FREE_EOFBLOCKS	_IOR ('X', 58, struct xfs_eofblocks)
 
 /*
  * ioctl commands that replace IRIX syssgi()'s
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 0e0232c..b91cbcd 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1602,6 +1602,31 @@ xfs_file_ioctl(
 		error = xfs_errortag_clearall(mp, 1);
 		return -error;
 
+	case XFS_IOC_FREE_EOFBLOCKS: {
+		struct xfs_eofblocks eofb;
+		int qtype;
+
+		if (copy_from_user(&eofb, arg, sizeof(eofb)))
+			return -XFS_ERROR(EFAULT);
+
+		qtype = xfs_quota_type(eofb.qtype);
+
+		/*
+		 * TODO: The filtering code currently uses the id in the inode.
+		 * Therefore, I don't think it really matters whether the
+		 * particular quota type is enabled (and the dquot is attached).
+		 *
+		 * Alternatively, we could filter by dquot type. This would
+		 * mean we might have to make sure dquot's are attached during
+		 * the scan and that the particular quota type is enabled. I'm
+		 * not sure that this buys us anything.
+		 */
+
+		/* TODO: might want to just use the eofb structure here */
+		error = xfs_inodes_free_eofblocks(mp, qtype, eofb.id, eofb.min_file_size, EOFBLOCKS_WAIT);
+		return -error;
+	}
+
 	default:
 		return -ENOTTY;
 	}
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index b50ec5b..3ec6224 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -379,6 +379,7 @@ static inline int xfs_trans_reserve_quota_bydquots(struct xfs_trans *tp,
 extern int xfs_qm_dqcheck(struct xfs_mount *, xfs_disk_dquot_t *,
 				xfs_dqid_t, uint, uint, char *);
 extern int xfs_mount_reset_sbqflags(struct xfs_mount *);
+int xfs_quota_type(int);
 
 #endif	/* __KERNEL__ */
 #endif	/* __XFS_QUOTA_H__ */
diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c
index fed504f..fe15aa7 100644
--- a/fs/xfs/xfs_quotaops.c
+++ b/fs/xfs/xfs_quotaops.c
@@ -28,7 +28,7 @@
 #include <linux/quota.h>
 
 
-STATIC int
+int
 xfs_quota_type(int type)
 {
 	switch (type) {
-- 
1.7.7.6

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[RFC PATCH 4/4] xfs: add background scanning to clear EOFBLOCKS inodes

[Brian Foster]

Create a delayed_work to enable background scanning and freeing
of EOFBLOCKS inodes. The scanner kicks in once speculative
preallocation occurs and stops requeueing itself when no EOFBLOCKS
inodes exist.

Scans are queued on the existing syncd workqueue and the interval
is tied to the syncd interval, which is a default of 30s. The
minimum file size for a background scan is hardcoded to 100MB.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_mount.h |    2 ++
 fs/xfs/xfs_sync.c  |   33 +++++++++++++++++++++++++++++++++
 fs/xfs/xfs_sync.h  |    3 +++
 3 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 05a05a7..a966bf4 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -204,6 +204,8 @@ typedef struct xfs_mount {
 	struct xfs_mru_cache	*m_filestream;  /* per-mount filestream data */
 	struct delayed_work	m_sync_work;	/* background sync work */
 	struct delayed_work	m_reclaim_work;	/* background inode reclaim */
+	struct delayed_work	m_eofblocks_work; /* background eof blocks
+						     trimming */
 	struct work_struct	m_flush_work;	/* background inode flush */
 	__int64_t		m_update_flags;	/* sb flags we need to update
 						   on the next remount,rw */
diff --git a/fs/xfs/xfs_sync.c b/fs/xfs/xfs_sync.c
index 27c3c46..bd1ca90 100644
--- a/fs/xfs/xfs_sync.c
+++ b/fs/xfs/xfs_sync.c
@@ -489,6 +489,34 @@ xfs_flush_worker(
 	xfs_sync_data(mp, SYNC_TRYLOCK | SYNC_WAIT);
 }
 
+/*
+ * Background scanning to trim post-EOF preallocated space. This is queued
+ * based on the syncd tunable (30s by default).
+ *
+ * TODO: Do we want an independent tunable (and default.. off by default)?
+ */
+STATIC void
+xfs_queue_eofblocks(
+	struct xfs_mount *mp)
+{
+	rcu_read_lock();
+	if (radix_tree_tagged(&mp->m_perag_tree, XFS_ICI_EOFBLOCKS_TAG))
+		queue_delayed_work(xfs_syncd_wq, &mp->m_eofblocks_work,
+			msecs_to_jiffies(xfs_syncd_centisecs * 10));
+	rcu_read_unlock();
+}
+
+STATIC void
+xfs_eofblocks_worker(
+	struct work_struct *work)
+{
+	struct xfs_mount *mp = container_of(to_delayed_work(work),
+				struct xfs_mount, m_eofblocks_work);
+	xfs_inodes_free_eofblocks(mp, XFS_DQ_PROJ, XFS_PROJID_DEFAULT,
+		XFS_EOFBLOCKS_DEFAULT_MINFILESIZE, 0);
+	xfs_queue_eofblocks(mp);
+}
+
 int
 xfs_syncd_init(
 	struct xfs_mount	*mp)
@@ -496,6 +524,7 @@ xfs_syncd_init(
 	INIT_WORK(&mp->m_flush_work, xfs_flush_worker);
 	INIT_DELAYED_WORK(&mp->m_sync_work, xfs_sync_worker);
 	INIT_DELAYED_WORK(&mp->m_reclaim_work, xfs_reclaim_worker);
+	INIT_DELAYED_WORK(&mp->m_eofblocks_work, xfs_eofblocks_worker);
 
 	xfs_syncd_queue_sync(mp);
 
@@ -508,6 +537,7 @@ xfs_syncd_stop(
 {
 	cancel_delayed_work_sync(&mp->m_sync_work);
 	cancel_delayed_work_sync(&mp->m_reclaim_work);
+	cancel_delayed_work_sync(&mp->m_eofblocks_work);
 	cancel_work_sync(&mp->m_flush_work);
 }
 
@@ -1159,6 +1189,9 @@ __xfs_inode_set_eofblocks_tag(
 				   XFS_ICI_EOFBLOCKS_TAG);
 		spin_unlock(&ip->i_mount->m_perag_lock);
 
+		/* kick off background trimming */
+		xfs_queue_eofblocks(ip->i_mount);
+
 		trace_xfs_perag_set_eofblocks(ip->i_mount, pag->pag_agno,
 					      -1, _RET_IP_);
 	}
diff --git a/fs/xfs/xfs_sync.h b/fs/xfs/xfs_sync.h
index 78aca41..3ce8c3f 100644
--- a/fs/xfs/xfs_sync.h
+++ b/fs/xfs/xfs_sync.h
@@ -45,6 +45,9 @@ void __xfs_inode_clear_reclaim_tag(struct xfs_mount *mp, struct xfs_perag *pag,
 
 #define EOFBLOCKS_WAIT		0x0001
 
+/* TODO: should we have a tunable? */
+#define XFS_EOFBLOCKS_DEFAULT_MINFILESIZE 1024 * 1024 * 100
+
 void xfs_inode_set_eofblocks_tag(struct xfs_inode *ip);
 void xfs_inode_clear_eofblocks_tag(struct xfs_inode *ip);
 int xfs_inodes_free_eofblocks(struct xfs_mount *, int, uint32_t, uint64_t, int);
-- 
1.7.7.6

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 1/4] xfs: add EOFBLOCKS inode tagging/untagging

[Dave Chinner]

On Mon, Aug 27, 2012 at 03:51:48PM -0400, Brian Foster wrote:
> Add the XFS_ICI_EOFBLOCKS_TAG inode tag to identify inodes with
> speculatively preallocated blocks beyond EOF. An inode is tagged
> when speculative preallocation occurs and untagged either via
> truncate down or when post-EOF blocks are freed via release or
> reclaim.
> 
> The tag management is intentionally not aggressive to prefer
> simplicity over the complexity of handling all the corner cases
> under which post-EOF blocks could be freed (i.e., forward
> truncation, fallocate, write error conditions, etc.). This means
> that a tagged inode may or may not have post-EOF blocks after a
> period of time. The tag is eventually cleared when the inode is
> released or reclaimed.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
.....
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 94b32f9..2cd2883 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -305,10 +305,16 @@ xfs_iflags_set(xfs_inode_t *ip, unsigned short flags)
>  }
>  
>  static inline void
> +__xfs_iflags_clear(xfs_inode_t *ip, unsigned short flags)
> +{
> +	ip->i_flags &= ~flags;
> +}
> +
> +static inline void
>  xfs_iflags_clear(xfs_inode_t *ip, unsigned short flags)
>  {
>  	spin_lock(&ip->i_flags_lock);
> -	ip->i_flags &= ~flags;
> +	__xfs_iflags_clear(ip, flags);
>  	spin_unlock(&ip->i_flags_lock);
>  }

Left overs from a previous version?

>  
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 973dff6..2968ee8 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -459,6 +459,13 @@ retry:
>  	if (!(imap[0].br_startblock || XFS_IS_REALTIME_INODE(ip)))
>  		return xfs_alert_fsblock_zero(ip, &imap[0]);
>  
> +	/*
> +	 * Tag the inode as speculatively preallocated so we can reclaim this
> +	 * space on demand, if necessary.
> +	 */
> +	if (prealloc)
> +		xfs_inode_set_eofblocks_tag(ip);
> +
>  	*ret_imap = imap[0];
>  	return 0;
>  }
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 4e00cf0..dcd1d5f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -854,6 +854,9 @@ xfs_setattr_size(
>  		 * and do not wait the usual (long) time for writeout.
>  		 */
>  		xfs_iflags_set(ip, XFS_ITRUNCATED);
> +
> +		/* A truncate down always removes post-EOF blocks. */
> +		xfs_inode_clear_eofblocks_tag(ip);
>  	}
>  
>  	if (mask & ATTR_CTIME) {
> diff --git a/fs/xfs/xfs_sync.c b/fs/xfs/xfs_sync.c
> index 9654817..5e14741 100644
> --- a/fs/xfs/xfs_sync.c
> +++ b/fs/xfs/xfs_sync.c
> @@ -971,3 +971,79 @@ xfs_reclaim_inodes_count(
>  	return reclaimable;
>  }
>  
> +STATIC void
> +__xfs_inode_set_eofblocks_tag(
> +	struct xfs_perag	*pag,
> +	struct xfs_inode	*ip)
> +{
> +	int tagged = radix_tree_tagged(&pag->pag_ici_root,
> +				       XFS_ICI_EOFBLOCKS_TAG);
> +
> +	radix_tree_tag_set(&pag->pag_ici_root,
> +			   XFS_INO_TO_AGINO(ip->i_mount, ip->i_ino),
> +			   XFS_ICI_EOFBLOCKS_TAG);
> +
> +	if (!tagged) {
> +		/* propagate the eofblocks tag up into the perag radix tree */
> +		spin_lock(&ip->i_mount->m_perag_lock);
> +		radix_tree_tag_set(&ip->i_mount->m_perag_tree,
> +				   XFS_INO_TO_AGNO(ip->i_mount, ip->i_ino),
> +				   XFS_ICI_EOFBLOCKS_TAG);
> +		spin_unlock(&ip->i_mount->m_perag_lock);
> +
> +		trace_xfs_perag_set_eofblocks(ip->i_mount, pag->pag_agno,
> +					      -1, _RET_IP_);
> +	}
> +}
> +
> +void
> +xfs_inode_set_eofblocks_tag(
> +	xfs_inode_t	*ip)
> +{
> +	struct xfs_mount *mp = ip->i_mount;
> +	struct xfs_perag *pag;
> +
> +	pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino));
> +	spin_lock(&pag->pag_ici_lock);
> +	trace_xfs_set_eofblocks_tag(ip);
> +	__xfs_inode_set_eofblocks_tag(pag, ip);
> +	spin_unlock(&pag->pag_ici_lock);
> +	xfs_perag_put(pag);
> +}

I know the code you copied had this two-function structure, but that
was because it has callers of the __ versions of the functions. I'd
just make these a single function.

> +
> +STATIC void
> +__xfs_inode_clear_eofblocks(
> +	xfs_perag_t	*pag,
> +	xfs_inode_t	*ip)
> +{
> +	radix_tree_tag_clear(&pag->pag_ici_root,
> +			     XFS_INO_TO_AGINO(ip->i_mount, ip->i_ino),
> +			     XFS_ICI_EOFBLOCKS_TAG);
> +
> +	if (!radix_tree_tagged(&pag->pag_ici_root, XFS_ICI_EOFBLOCKS_TAG)) {
> +		/* clear the eofblocks tag from the perag radix tree */
> +		spin_lock(&ip->i_mount->m_perag_lock);
> +		radix_tree_tag_clear(&ip->i_mount->m_perag_tree,
> +				     XFS_INO_TO_AGNO(ip->i_mount, ip->i_ino),
> +				     XFS_ICI_EOFBLOCKS_TAG);
> +		spin_unlock(&ip->i_mount->m_perag_lock);
> +		trace_xfs_perag_clear_eofblocks(ip->i_mount, pag->pag_agno,
> +					       -1, _RET_IP_);
> +	}
> +}
> +
> +void
> +xfs_inode_clear_eofblocks_tag(
> +	xfs_inode_t	*ip)
> +{
> +	struct xfs_mount *mp = ip->i_mount;
> +	struct xfs_perag *pag;
> +
> +	pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino));
> +	spin_lock(&pag->pag_ici_lock);
> +	trace_xfs_clear_eofblocks_tag(ip);
> +	__xfs_inode_clear_eofblocks(pag, ip);
> +	spin_unlock(&pag->pag_ici_lock);
> +	xfs_perag_put(pag);
> +}

Same here.

> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> index 2a5c6373..658ee2e 100644
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -467,6 +467,8 @@ xfs_release(
>  		if (error)
>  			return error;
>  
> +		xfs_inode_clear_eofblocks_tag(ip);
> +
>  		/* delalloc blocks after truncation means it really is dirty */
>  		if (ip->i_delayed_blks)
>  			xfs_iflags_set(ip, XFS_IDIRTY_RELEASE);
> @@ -523,6 +525,7 @@ xfs_inactive(
>  			error = xfs_free_eofblocks(mp, ip, false);
>  			if (error)
>  				return VN_INACTIVE_CACHE;
> +			xfs_inode_clear_eofblocks_tag(ip);
>  		}
>  		goto out;

I think it's better to call xfs_inode_clear_eofblocks_tag() inside
xfs_free_eofblocks() - that's where the blocks are being freed,
after all.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/4] xfs: create function to scan and clear EOFBLOCKS inodes

[Dave Chinner]

On Mon, Aug 27, 2012 at 03:51:49PM -0400, Brian Foster wrote:
> xfs_inodes_free_eofblocks() implements scanning functionality for
> EOFBLOCKS inodes. It scans the radix tree and frees post-EOF blocks
> for inodes that meet particular criteria. The scan can be filtered
> by a particular quota type/id and minimum file size. The scan can
> also be invoked in trylock mode or wait (force) mode.
> 
> The xfs_free_eofblocks() helper is invoked to clear post-EOF space.
> It is slightly modified to support an output parameter that
> indicates whether space was freed and helps decide whether the
> EOFBLOCKS tag should be cleared in trylock scans.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_sync.c     |  168 +++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_sync.h     |    3 +
>  fs/xfs/xfs_vnodeops.c |   17 +++--
>  fs/xfs/xfs_vnodeops.h |    2 +
>  4 files changed, 184 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_sync.c b/fs/xfs/xfs_sync.c
> index 5e14741..27c3c46 100644
> --- a/fs/xfs/xfs_sync.c
> +++ b/fs/xfs/xfs_sync.c
> @@ -971,6 +971,174 @@ xfs_reclaim_inodes_count(
>  	return reclaimable;
>  }
>  
> +/*
> + * Handle an EOFBLOCKS tagged inode. If this is a forced scan, we wait on the
> + * iolock ourselves rather than rely on the trylock in xfs_free_eofblocks(). 
> + *
> + * We rely on the output parameter from xfs_free_eofblocks() to determine
> + * whether we should clear the tag because in the trylock case, it could have
> + * skipped the inode due to lock contention.
> + */
> +STATIC int
> +xfs_inode_free_eofblocks(
> +	struct xfs_inode	*ip,
> +	int			flags)
> +{
> +	int ret = 0;
> +	bool freed = false;
> +	bool wait_iolock = (flags & EOFBLOCKS_WAIT) ? true : false;
> +
> +	if (wait_iolock)
> +		xfs_ilock(ip, XFS_IOLOCK_EXCL);

Why do we need the IO lock here? xfs_free_eofblocks() does all the
necessary locking....

> +
> +	if ((S_ISREG(ip->i_d.di_mode) &&
> +	     (VFS_I(ip)->i_size > 0 ||
> +	     (VN_CACHED(VFS_I(ip)) > 0 || ip->i_delayed_blks > 0)) &&
> +	     (ip->i_df.if_flags & XFS_IFEXTENTS)) &&
> +	    (!(ip->i_d.di_flags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)))) {

This check is now repeated in 3 places - xfs_inactive, xfs_release
and now here. I think it needs a helper.

> +		/* !wait_iolock == need_iolock in xfs_free_eofblocks() */
> +		ret = xfs_free_eofblocks(ip->i_mount, ip, !wait_iolock, &freed);
> +		if (freed)
> +			xfs_inode_clear_eofblocks_tag(ip);

If you move xfs_inode_clear_eofblocks_tag() inside
xfs_free_eofblocks(), there's no need for this extra return value.

> +	} else {
> +		/* inode could be preallocated or append-only */
> +		xfs_inode_clear_eofblocks_tag(ip);

This should be a rare event - it's probably worth adding a pair of
trace events here for the two cases so we can see if there is ever a
significant number of inodes being scanned for prealloc that can't
be cleared...

(e.g 'perf top -e xfs:xfs_i*' to count all the inode events)

> +	}
> +
> +	if (wait_iolock)
> +		xfs_iunlock(ip, XFS_IOLOCK_EXCL);
> +
> +	return ret;
> +}
> +
> +/*
> + * Determine whether an inode matches a particular qouta id.
> + */
> +STATIC int
> +xfs_inode_match_quota_id(
> +	struct xfs_inode	*ip,
> +	int			qtype,
> +	uint32_t		id)
> +{
> +	switch (qtype) {
> +	case XFS_DQ_USER:
> +		return ip->i_d.di_uid == id;
> +	case XFS_DQ_GROUP:
> +		return ip->i_d.di_gid == id;
> +	default:
> +		return xfs_get_projid(ip) == id;
> +	}
> +
> +	return 0;
> +}

There's nothing really quota specific about this scan. I'd leave
this functionality to a separate patch once all the core
infrastructure is in place.

> +
> +/*
> + * This is mostly copied from xfs_reclaim_inodes_ag().
> + *
> + * TODO:
> + * - Could we enhance ag_iterator to support a tag and use it instead of this?

Yes. This code is too tricky to duplicate for every use case, and
this doesn't have special case requirements like the reclaim code.

i.e. the xfs_inode_free_eofblocks() becomes the execute function
(and the quota checks move inside that eventually). Passing a tag of
"-1" would indicate a non-tag lookup, otherwise use a tag based
lookup. Given the extra fields that this version uses, passing a
void *args is probably necessary so that a structure can be passed
to the execute function along with the flags....

I'd suggest this conversion should be done in a patch prior to
introducing this scanner.

FWIW, this is going to conflict with my "get rid of xfs-sync.c patch
series, so we'll need to work out who rebases what at some point.

> + */
> +int
> +xfs_inodes_free_eofblocks(
> +	struct xfs_mount	*mp,
> +	int			qtype,
> +	uint32_t		id,
> +	uint64_t		min_file_size,
> +	int			flags)
> +{
.....
> +			for (i = 0; i < nr_found; i++) {
> +				if (!batch[i])
> +					continue;
> +
> +				/* default projid represents a full scan */

I don't think thats a good idea. From a normal users perspective,
the background trimming will occur irrespective of the quota groups
the inode is part of. Background trimming defines the default
behaviour, because that's what 99.99% of users will see active, not
quota/application specific events driven through ioctls.

IOWs, selecting inodes by quota type/id for pruning is a secondary
function of the execute implementation, not a primary concern of the
infrastructure.

> +				if ((!(qtype == XFS_DQ_PROJ &&
> +				       id == XFS_PROJID_DEFAULT) &&
> +				     !xfs_inode_match_quota_id(batch[i], qtype,
> +							       id)) ||
> +				    (min_file_size && XFS_ISIZE(batch[i]) < 
> +								min_file_size)

> +				   ) {
> +					IRELE(batch[i]);
> +					continue;
> +				}

Moving this check to the execute function will get rid of the indent
mess....

> +
> +				error = xfs_inode_free_eofblocks(batch[i], flags);
> +				IRELE(batch[i]);
> +				if (error)
> +					last_error = error;
> +			}
> +
> +			cond_resched();
> +
> +		} while (nr_found && !done);
> +
> +		xfs_perag_put(pag);
> +	}
> +
> +	return XFS_ERROR(last_error);
> +}
> +
>  STATIC void
>  __xfs_inode_set_eofblocks_tag(
>  	struct xfs_perag	*pag,
> diff --git a/fs/xfs/xfs_sync.h b/fs/xfs/xfs_sync.h
> index 4486491..78aca41 100644
> --- a/fs/xfs/xfs_sync.h
> +++ b/fs/xfs/xfs_sync.h
> @@ -43,8 +43,11 @@ void __xfs_inode_set_reclaim_tag(struct xfs_perag *pag, struct xfs_inode *ip);
>  void __xfs_inode_clear_reclaim_tag(struct xfs_mount *mp, struct xfs_perag *pag,
>  				struct xfs_inode *ip);
>  
> +#define EOFBLOCKS_WAIT		0x0001

I'd just reuse SYNC_WAIT and SYNC_TRYLOCK which are already defined
and used by the sync and reclaim iterators.

> +
>  void xfs_inode_set_eofblocks_tag(struct xfs_inode *ip);
>  void xfs_inode_clear_eofblocks_tag(struct xfs_inode *ip);
> +int xfs_inodes_free_eofblocks(struct xfs_mount *, int, uint32_t, uint64_t, int);
>  
>  int xfs_sync_inode_grab(struct xfs_inode *ip);
>  int xfs_inode_ag_iterator(struct xfs_mount *mp,
> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> index 658ee2e..53460f3 100644
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -150,11 +150,12 @@ xfs_readlink(
>   * when the link count isn't zero and by xfs_dm_punch_hole() when
>   * punching a hole to EOF.
>   */
> -STATIC int
> +int
>  xfs_free_eofblocks(
>  	xfs_mount_t	*mp,
>  	xfs_inode_t	*ip,
> -	bool		need_iolock)
> +	bool		need_iolock,
> +	bool		*blocks_freed)

I don't really see a point to adding this. Either we removed all the
EOF blocks or we didn't, and that means we should just clear the
tags directly in this function if it is appropriate.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 3/4] xfs: add FREE_EOFBLOCKS ioctl

[Dave Chinner]

On Mon, Aug 27, 2012 at 03:51:50PM -0400, Brian Foster wrote:
> The XFS_IOC_FREE_EOFBLOCKS ioctl allows users to invoke an EOFBLOCKS
> scan. The xfs_eofblocks structure is defined to support the command
> parameters (quota type/id and minimum file size).
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_fs.h       |   10 ++++++++++
>  fs/xfs/xfs_ioctl.c    |   25 +++++++++++++++++++++++++
>  fs/xfs/xfs_quota.h    |    1 +
>  fs/xfs/xfs_quotaops.c |    2 +-
>  4 files changed, 37 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
> index c13fed8..6f93db9 100644
> --- a/fs/xfs/xfs_fs.h
> +++ b/fs/xfs/xfs_fs.h
> @@ -339,6 +339,15 @@ typedef struct xfs_error_injection {
>  
>  
>  /*
> + * Speculative preallocation trimming.
> + */
> +typedef struct xfs_eofblocks {
> +	__u32		id;		/* quota id */
> +	__u32		qtype;		/* quota type */
> +	__u64		min_file_size;	/* minimum file size */
> +} xfs_eofblocks_t;

No typedefs.

Additionally: this is a user facing API for functionality that has
wider potential use than quota. For example, xfs_fsr runs out of
space in an AG, but it really wants to put a file there, so it calls
XFS_IOC_FREE_EOFBLOCKS to specify the AG it wants all the prealloc
removed from.

So, the structure needs a version number so the kernel knows what
fields the application is aware of, a flags field to say what
operation is being done (i.e. flush by projid) rather than a "qtype"
and a bunch of padding so that we can add new elements to it as need
arises without breaking existing user binaries.

>   * ioctl commands that replace IRIX syssgi()'s
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 0e0232c..b91cbcd 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1602,6 +1602,31 @@ xfs_file_ioctl(
>  		error = xfs_errortag_clearall(mp, 1);
>  		return -error;
>  
> +	case XFS_IOC_FREE_EOFBLOCKS: {
> +		struct xfs_eofblocks eofb;
> +		int qtype;
> +
> +		if (copy_from_user(&eofb, arg, sizeof(eofb)))
> +			return -XFS_ERROR(EFAULT);
> +
> +		qtype = xfs_quota_type(eofb.qtype);
> +
> +		/*
> +		 * TODO: The filtering code currently uses the id in the inode.
> +		 * Therefore, I don't think it really matters whether the
> +		 * particular quota type is enabled (and the dquot is attached).
> +		 *
> +		 * Alternatively, we could filter by dquot type. This would
> +		 * mean we might have to make sure dquot's are attached during
> +		 * the scan and that the particular quota type is enabled. I'm
> +		 * not sure that this buys us anything.
> +		 */

If quota is not enabled, then a request to free blocks determined by
a quota ID is invalid and should be rejected. It's a user API - what
is and isn't supported needs to be written down in black and white
(i.e. in the xfsctl man page).

> +		/* TODO: might want to just use the eofb structure here */
> +		error = xfs_inodes_free_eofblocks(mp, qtype, eofb.id, eofb.min_file_size, EOFBLOCKS_WAIT);

Yes, just pass the structure - it means it can be passed all the way
down to the execute function, and only that code needs to handle
different versions of the structure.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: add background scanning to clear EOFBLOCKS inodes

[Dave Chinner]

On Mon, Aug 27, 2012 at 03:51:51PM -0400, Brian Foster wrote:
> Create a delayed_work to enable background scanning and freeing
> of EOFBLOCKS inodes. The scanner kicks in once speculative
> preallocation occurs and stops requeueing itself when no EOFBLOCKS
> inodes exist.
> 
> Scans are queued on the existing syncd workqueue and the interval
> is tied to the syncd interval, which is a default of 30s. The
> minimum file size for a background scan is hardcoded to 100MB.

I'd set it to be much longer than the xfs_syncd_centisecs period.
The data may not be written back for a long while, and a file that
is open, written and closed repeatedly (NFS write patterns for large
and/or slowly written files) should not have it's preallocation
truncated every 30s.

I'd suggest that a 5 minute sweep is probably sufficiently frequent
to avoid most fragmentation issues and long term buildup of speculative
preallocation whilst not perturbing large/slow writers too much....

Apart from that, there isn't anything wrong with the code, except
I'll note again that it conflicts with my syncd removal patchset....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/4] xfs: create function to scan and clear EOFBLOCKS inodes

[Brian Foster]

On 09/03/2012 01:06 AM, Dave Chinner wrote:
> On Mon, Aug 27, 2012 at 03:51:49PM -0400, Brian Foster wrote:
...
>> +/*
>> + * Handle an EOFBLOCKS tagged inode. If this is a forced scan, we wait on the
>> + * iolock ourselves rather than rely on the trylock in xfs_free_eofblocks(). 
>> + *
>> + * We rely on the output parameter from xfs_free_eofblocks() to determine
>> + * whether we should clear the tag because in the trylock case, it could have
>> + * skipped the inode due to lock contention.
>> + */
>> +STATIC int
>> +xfs_inode_free_eofblocks(
>> +	struct xfs_inode	*ip,
>> +	int			flags)
>> +{
>> +	int ret = 0;
>> +	bool freed = false;
>> +	bool wait_iolock = (flags & EOFBLOCKS_WAIT) ? true : false;
>> +
>> +	if (wait_iolock)
>> +		xfs_ilock(ip, XFS_IOLOCK_EXCL);
> 
> Why do we need the IO lock here? xfs_free_eofblocks() does all the
> necessary locking....
> 

This was for the wait case (e.g., xfs_free_eofblocks() does a trylock
on the IO lock and we want to wait for the lock in this case).

Brian

>> +
>> +	if ((S_ISREG(ip->i_d.di_mode) &&
>> +	     (VFS_I(ip)->i_size > 0 ||
>> +	     (VN_CACHED(VFS_I(ip)) > 0 || ip->i_delayed_blks > 0)) &&
>> +	     (ip->i_df.if_flags & XFS_IFEXTENTS)) &&
>> +	    (!(ip->i_d.di_flags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)))) {
> 
> This check is now repeated in 3 places - xfs_inactive, xfs_release
> and now here. I think it needs a helper.
> 
>> +		/* !wait_iolock == need_iolock in xfs_free_eofblocks() */
>> +		ret = xfs_free_eofblocks(ip->i_mount, ip, !wait_iolock, &freed);
>> +		if (freed)
>> +			xfs_inode_clear_eofblocks_tag(ip);
> 
> If you move xfs_inode_clear_eofblocks_tag() inside
> xfs_free_eofblocks(), there's no need for this extra return value.
> 
>> +	} else {
>> +		/* inode could be preallocated or append-only */
>> +		xfs_inode_clear_eofblocks_tag(ip);
> 
> This should be a rare event - it's probably worth adding a pair of
> trace events here for the two cases so we can see if there is ever a
> significant number of inodes being scanned for prealloc that can't
> be cleared...
> 
> (e.g 'perf top -e xfs:xfs_i*' to count all the inode events)
> 
>> +	}
>> +
>> +	if (wait_iolock)
>> +		xfs_iunlock(ip, XFS_IOLOCK_EXCL);
>> +
>> +	return ret;
>> +}
>> +
>> +/*
>> + * Determine whether an inode matches a particular qouta id.
>> + */
>> +STATIC int
>> +xfs_inode_match_quota_id(
>> +	struct xfs_inode	*ip,
>> +	int			qtype,
>> +	uint32_t		id)
>> +{
>> +	switch (qtype) {
>> +	case XFS_DQ_USER:
>> +		return ip->i_d.di_uid == id;
>> +	case XFS_DQ_GROUP:
>> +		return ip->i_d.di_gid == id;
>> +	default:
>> +		return xfs_get_projid(ip) == id;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> There's nothing really quota specific about this scan. I'd leave
> this functionality to a separate patch once all the core
> infrastructure is in place.
> 
>> +
>> +/*
>> + * This is mostly copied from xfs_reclaim_inodes_ag().
>> + *
>> + * TODO:
>> + * - Could we enhance ag_iterator to support a tag and use it instead of this?
> 
> Yes. This code is too tricky to duplicate for every use case, and
> this doesn't have special case requirements like the reclaim code.
> 
> i.e. the xfs_inode_free_eofblocks() becomes the execute function
> (and the quota checks move inside that eventually). Passing a tag of
> "-1" would indicate a non-tag lookup, otherwise use a tag based
> lookup. Given the extra fields that this version uses, passing a
> void *args is probably necessary so that a structure can be passed
> to the execute function along with the flags....
> 
> I'd suggest this conversion should be done in a patch prior to
> introducing this scanner.
> 
> FWIW, this is going to conflict with my "get rid of xfs-sync.c patch
> series, so we'll need to work out who rebases what at some point.
> 
>> + */
>> +int
>> +xfs_inodes_free_eofblocks(
>> +	struct xfs_mount	*mp,
>> +	int			qtype,
>> +	uint32_t		id,
>> +	uint64_t		min_file_size,
>> +	int			flags)
>> +{
> .....
>> +			for (i = 0; i < nr_found; i++) {
>> +				if (!batch[i])
>> +					continue;
>> +
>> +				/* default projid represents a full scan */
> 
> I don't think thats a good idea. From a normal users perspective,
> the background trimming will occur irrespective of the quota groups
> the inode is part of. Background trimming defines the default
> behaviour, because that's what 99.99% of users will see active, not
> quota/application specific events driven through ioctls.
> 
> IOWs, selecting inodes by quota type/id for pruning is a secondary
> function of the execute implementation, not a primary concern of the
> infrastructure.
> 
>> +				if ((!(qtype == XFS_DQ_PROJ &&
>> +				       id == XFS_PROJID_DEFAULT) &&
>> +				     !xfs_inode_match_quota_id(batch[i], qtype,
>> +							       id)) ||
>> +				    (min_file_size && XFS_ISIZE(batch[i]) < 
>> +								min_file_size)
> 
>> +				   ) {
>> +					IRELE(batch[i]);
>> +					continue;
>> +				}
> 
> Moving this check to the execute function will get rid of the indent
> mess....
> 
>> +
>> +				error = xfs_inode_free_eofblocks(batch[i], flags);
>> +				IRELE(batch[i]);
>> +				if (error)
>> +					last_error = error;
>> +			}
>> +
>> +			cond_resched();
>> +
>> +		} while (nr_found && !done);
>> +
>> +		xfs_perag_put(pag);
>> +	}
>> +
>> +	return XFS_ERROR(last_error);
>> +}
>> +
>>  STATIC void
>>  __xfs_inode_set_eofblocks_tag(
>>  	struct xfs_perag	*pag,
>> diff --git a/fs/xfs/xfs_sync.h b/fs/xfs/xfs_sync.h
>> index 4486491..78aca41 100644
>> --- a/fs/xfs/xfs_sync.h
>> +++ b/fs/xfs/xfs_sync.h
>> @@ -43,8 +43,11 @@ void __xfs_inode_set_reclaim_tag(struct xfs_perag *pag, struct xfs_inode *ip);
>>  void __xfs_inode_clear_reclaim_tag(struct xfs_mount *mp, struct xfs_perag *pag,
>>  				struct xfs_inode *ip);
>>  
>> +#define EOFBLOCKS_WAIT		0x0001
> 
> I'd just reuse SYNC_WAIT and SYNC_TRYLOCK which are already defined
> and used by the sync and reclaim iterators.
> 
>> +
>>  void xfs_inode_set_eofblocks_tag(struct xfs_inode *ip);
>>  void xfs_inode_clear_eofblocks_tag(struct xfs_inode *ip);
>> +int xfs_inodes_free_eofblocks(struct xfs_mount *, int, uint32_t, uint64_t, int);
>>  
>>  int xfs_sync_inode_grab(struct xfs_inode *ip);
>>  int xfs_inode_ag_iterator(struct xfs_mount *mp,
>> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
>> index 658ee2e..53460f3 100644
>> --- a/fs/xfs/xfs_vnodeops.c
>> +++ b/fs/xfs/xfs_vnodeops.c
>> @@ -150,11 +150,12 @@ xfs_readlink(
>>   * when the link count isn't zero and by xfs_dm_punch_hole() when
>>   * punching a hole to EOF.
>>   */
>> -STATIC int
>> +int
>>  xfs_free_eofblocks(
>>  	xfs_mount_t	*mp,
>>  	xfs_inode_t	*ip,
>> -	bool		need_iolock)
>> +	bool		need_iolock,
>> +	bool		*blocks_freed)
> 
> I don't really see a point to adding this. Either we removed all the
> EOF blocks or we didn't, and that means we should just clear the
> tags directly in this function if it is appropriate.
> 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 3/4] xfs: add FREE_EOFBLOCKS ioctl

[Brian Foster]

On 09/03/2012 01:17 AM, Dave Chinner wrote:
> On Mon, Aug 27, 2012 at 03:51:50PM -0400, Brian Foster wrote:
>> The XFS_IOC_FREE_EOFBLOCKS ioctl allows users to invoke an EOFBLOCKS
>> scan. The xfs_eofblocks structure is defined to support the command
>> parameters (quota type/id and minimum file size).
>>
>> Signed-off-by: Brian Foster <bfoster@redhat.com>
>> ---
>>  fs/xfs/xfs_fs.h       |   10 ++++++++++
>>  fs/xfs/xfs_ioctl.c    |   25 +++++++++++++++++++++++++
>>  fs/xfs/xfs_quota.h    |    1 +
>>  fs/xfs/xfs_quotaops.c |    2 +-
>>  4 files changed, 37 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
>> index c13fed8..6f93db9 100644
>> --- a/fs/xfs/xfs_fs.h
>> +++ b/fs/xfs/xfs_fs.h
>> @@ -339,6 +339,15 @@ typedef struct xfs_error_injection {
>>  
>>  
>>  /*
>> + * Speculative preallocation trimming.
>> + */
>> +typedef struct xfs_eofblocks {
>> +	__u32		id;		/* quota id */
>> +	__u32		qtype;		/* quota type */
>> +	__u64		min_file_size;	/* minimum file size */
>> +} xfs_eofblocks_t;
> 
> No typedefs.
> 

This is something I see throughout the code that I haven't quite
followed (i.e., using the _t typedefs vs. not). Is the general consensus
to move away from typedefs when possible?

...
>> +	case XFS_IOC_FREE_EOFBLOCKS: {
>> +		struct xfs_eofblocks eofb;
>> +		int qtype;
>> +
>> +		if (copy_from_user(&eofb, arg, sizeof(eofb)))
>> +			return -XFS_ERROR(EFAULT);
>> +
>> +		qtype = xfs_quota_type(eofb.qtype);
>> +
>> +		/*
>> +		 * TODO: The filtering code currently uses the id in the inode.
>> +		 * Therefore, I don't think it really matters whether the
>> +		 * particular quota type is enabled (and the dquot is attached).
>> +		 *
>> +		 * Alternatively, we could filter by dquot type. This would
>> +		 * mean we might have to make sure dquot's are attached during
>> +		 * the scan and that the particular quota type is enabled. I'm
>> +		 * not sure that this buys us anything.
>> +		 */
> 
> If quota is not enabled, then a request to free blocks determined by
> a quota ID is invalid and should be rejected. It's a user API - what
> is and isn't supported needs to be written down in black and white
> (i.e. in the xfsctl man page).
> 

Ok. I was thinking that we could support the ability to scan by uid/gid
regardless of whether quota is enabled, but perhaps there's no purpose
to that if a quota isn't enabled.

Brian

>> +		/* TODO: might want to just use the eofb structure here */
>> +		error = xfs_inodes_free_eofblocks(mp, qtype, eofb.id, eofb.min_file_size, EOFBLOCKS_WAIT);
> 
> Yes, just pass the structure - it means it can be passed all the way
> down to the execute function, and only that code needs to handle
> different versions of the structure.
> 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: add background scanning to clear EOFBLOCKS inodes

[Brian Foster]

On 09/03/2012 01:28 AM, Dave Chinner wrote:
> On Mon, Aug 27, 2012 at 03:51:51PM -0400, Brian Foster wrote:
>> Create a delayed_work to enable background scanning and freeing
>> of EOFBLOCKS inodes. The scanner kicks in once speculative
>> preallocation occurs and stops requeueing itself when no EOFBLOCKS
>> inodes exist.
>>
>> Scans are queued on the existing syncd workqueue and the interval
>> is tied to the syncd interval, which is a default of 30s. The
>> minimum file size for a background scan is hardcoded to 100MB.
> 
> I'd set it to be much longer than the xfs_syncd_centisecs period.
> The data may not be written back for a long while, and a file that
> is open, written and closed repeatedly (NFS write patterns for large
> and/or slowly written files) should not have it's preallocation
> truncated every 30s.
> 
> I'd suggest that a 5 minute sweep is probably sufficiently frequent
> to avoid most fragmentation issues and long term buildup of speculative
> preallocation whilst not perturbing large/slow writers too much....
> 

Any thoughts on having tunables for both values (time and min size?) on
the background scanning?

> Apart from that, there isn't anything wrong with the code, except
> I'll note again that it conflicts with my syncd removal patchset....
> 

Yeah, I skimmed through those patches and noticed it would shuffle this
code around. Thanks for pointing it out. I'll keep an eye on that set.

Thanks for the review. I'll follow up with a patchset that incorporates
the feedback.

Brian

> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/4] xfs: create function to scan and clear EOFBLOCKS inodes

[Dave Chinner]

On Tue, Sep 04, 2012 at 10:10:00AM -0400, Brian Foster wrote:
> On 09/03/2012 01:06 AM, Dave Chinner wrote:
> > On Mon, Aug 27, 2012 at 03:51:49PM -0400, Brian Foster wrote:
> ...
> >> +/*
> >> + * Handle an EOFBLOCKS tagged inode. If this is a forced scan, we wait on the
> >> + * iolock ourselves rather than rely on the trylock in xfs_free_eofblocks(). 
> >> + *
> >> + * We rely on the output parameter from xfs_free_eofblocks() to determine
> >> + * whether we should clear the tag because in the trylock case, it could have
> >> + * skipped the inode due to lock contention.
> >> + */
> >> +STATIC int
> >> +xfs_inode_free_eofblocks(
> >> +	struct xfs_inode	*ip,
> >> +	int			flags)
> >> +{
> >> +	int ret = 0;
> >> +	bool freed = false;
> >> +	bool wait_iolock = (flags & EOFBLOCKS_WAIT) ? true : false;
> >> +
> >> +	if (wait_iolock)
> >> +		xfs_ilock(ip, XFS_IOLOCK_EXCL);
> > 
> > Why do we need the IO lock here? xfs_free_eofblocks() does all the
> > necessary locking....
> > 
> 
> This was for the wait case (e.g., xfs_free_eofblocks() does a trylock
> on the IO lock and we want to wait for the lock in this case).

Does it really matter if we can't get the iolock and skip the inode?

Note that xfs_inode_ag_walk() will keep a skipped count if you
return EAGAIN, and re-run the walk on the AG if any inodes are
skipped. This is done to prevent blocking on locks slowing down the
scan - instead of blocking we move on to the next inode, and revist
the blocked inodes again in a later pass.

IOWs, if you convert the code to use the generic walk and make
xfs_free_eofblocks() return EAGAIN when it can't get the io lock,
you get the retry for free...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 3/4] xfs: add FREE_EOFBLOCKS ioctl

[Dave Chinner]

On Tue, Sep 04, 2012 at 10:10:35AM -0400, Brian Foster wrote:
> On 09/03/2012 01:17 AM, Dave Chinner wrote:
> > On Mon, Aug 27, 2012 at 03:51:50PM -0400, Brian Foster wrote:
> >> The XFS_IOC_FREE_EOFBLOCKS ioctl allows users to invoke an EOFBLOCKS
> >> scan. The xfs_eofblocks structure is defined to support the command
> >> parameters (quota type/id and minimum file size).
> >>
> >> Signed-off-by: Brian Foster <bfoster@redhat.com>
> >> ---
> >>  fs/xfs/xfs_fs.h       |   10 ++++++++++
> >>  fs/xfs/xfs_ioctl.c    |   25 +++++++++++++++++++++++++
> >>  fs/xfs/xfs_quota.h    |    1 +
> >>  fs/xfs/xfs_quotaops.c |    2 +-
> >>  4 files changed, 37 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
> >> index c13fed8..6f93db9 100644
> >> --- a/fs/xfs/xfs_fs.h
> >> +++ b/fs/xfs/xfs_fs.h
> >> @@ -339,6 +339,15 @@ typedef struct xfs_error_injection {
> >>  
> >>  
> >>  /*
> >> + * Speculative preallocation trimming.
> >> + */
> >> +typedef struct xfs_eofblocks {
> >> +	__u32		id;		/* quota id */
> >> +	__u32		qtype;		/* quota type */
> >> +	__u64		min_file_size;	/* minimum file size */
> >> +} xfs_eofblocks_t;
> > 
> > No typedefs.
> > 
> 
> This is something I see throughout the code that I haven't quite
> followed (i.e., using the _t typedefs vs. not). Is the general consensus
> to move away from typedefs when possible?

Yes. The Irix code that XFS came from was full of typedefs - part
of it was to try to strictly type check things that were the same
storage size or on-disk vs in-memory. We've got other ways of doing
that better (e.g. the endian checking sparse does), and typedefs
are generally frowned upon in the main kernel code because they
often obfuscate the code rather than improve it, so we're
removing them as we modify code or write new code.

> >> +
> >> +		/*
> >> +		 * TODO: The filtering code currently uses the id in the inode.
> >> +		 * Therefore, I don't think it really matters whether the
> >> +		 * particular quota type is enabled (and the dquot is attached).
> >> +		 *
> >> +		 * Alternatively, we could filter by dquot type. This would
> >> +		 * mean we might have to make sure dquot's are attached during
> >> +		 * the scan and that the particular quota type is enabled. I'm
> >> +		 * not sure that this buys us anything.
> >> +		 */
> > 
> > If quota is not enabled, then a request to free blocks determined by
> > a quota ID is invalid and should be rejected. It's a user API - what
> > is and isn't supported needs to be written down in black and white
> > (i.e. in the xfsctl man page).
> > 
> 
> Ok. I was thinking that we could support the ability to scan by uid/gid
> regardless of whether quota is enabled, but perhaps there's no purpose
> to that if a quota isn't enabled.

I can't really think of a use case for doing this. Making the API
more expansive in future if someone needs this can be done - it's
removing stuff that is really hard to do. Hence, don't add it if it
is not going to be used immediately. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: add background scanning to clear EOFBLOCKS inodes

[Dave Chinner]

On Tue, Sep 04, 2012 at 10:10:57AM -0400, Brian Foster wrote:
> On 09/03/2012 01:28 AM, Dave Chinner wrote:
> > On Mon, Aug 27, 2012 at 03:51:51PM -0400, Brian Foster wrote:
> >> Create a delayed_work to enable background scanning and freeing
> >> of EOFBLOCKS inodes. The scanner kicks in once speculative
> >> preallocation occurs and stops requeueing itself when no EOFBLOCKS
> >> inodes exist.
> >>
> >> Scans are queued on the existing syncd workqueue and the interval
> >> is tied to the syncd interval, which is a default of 30s. The
> >> minimum file size for a background scan is hardcoded to 100MB.
> > 
> > I'd set it to be much longer than the xfs_syncd_centisecs period.
> > The data may not be written back for a long while, and a file that
> > is open, written and closed repeatedly (NFS write patterns for large
> > and/or slowly written files) should not have it's preallocation
> > truncated every 30s.
> > 
> > I'd suggest that a 5 minute sweep is probably sufficiently frequent
> > to avoid most fragmentation issues and long term buildup of speculative
> > preallocation whilst not perturbing large/slow writers too much....
> > 
> 
> Any thoughts on having tunables for both values (time and min size?) on
> the background scanning?

Well, my suggestion for timing is as per above (xfs_syncd_centisecs
* 100), but I don't really have any good rule of thumb for the
minimum size. What threshold do people start to notice this?

I'd SWAG that something like 32MB is a good size to start at because
most IO subsystems will still be able to reach full bandwidth with
extents of this size when reading files.

Alternatively, if you can determine if the inode is still in use at
the time of the scan (e.g. elevated reference count due to an open
fd) and skip the truncation for those inodes, then a minimum size is
not really needed, right?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 2/4] xfs: create function to scan and clear EOFBLOCKS inodes

[Brian Foster]

On 09/05/2012 02:42 AM, Dave Chinner wrote:
> On Tue, Sep 04, 2012 at 10:10:00AM -0400, Brian Foster wrote:
>> On 09/03/2012 01:06 AM, Dave Chinner wrote:
>>> On Mon, Aug 27, 2012 at 03:51:49PM -0400, Brian Foster wrote:
>> ...
>>>> +/*
>>>> + * Handle an EOFBLOCKS tagged inode. If this is a forced scan, we wait on the
>>>> + * iolock ourselves rather than rely on the trylock in xfs_free_eofblocks(). 
>>>> + *
>>>> + * We rely on the output parameter from xfs_free_eofblocks() to determine
>>>> + * whether we should clear the tag because in the trylock case, it could have
>>>> + * skipped the inode due to lock contention.
>>>> + */
>>>> +STATIC int
>>>> +xfs_inode_free_eofblocks(
>>>> +	struct xfs_inode	*ip,
>>>> +	int			flags)
>>>> +{
>>>> +	int ret = 0;
>>>> +	bool freed = false;
>>>> +	bool wait_iolock = (flags & EOFBLOCKS_WAIT) ? true : false;
>>>> +
>>>> +	if (wait_iolock)
>>>> +		xfs_ilock(ip, XFS_IOLOCK_EXCL);
>>>
>>> Why do we need the IO lock here? xfs_free_eofblocks() does all the
>>> necessary locking....
>>>
>>
>> This was for the wait case (e.g., xfs_free_eofblocks() does a trylock
>> on the IO lock and we want to wait for the lock in this case).
> 
> Does it really matter if we can't get the iolock and skip the inode?
> 

Well I thought that was the point of the force/wait scan as opposed to
the best effort scan. But...

> Note that xfs_inode_ag_walk() will keep a skipped count if you
> return EAGAIN, and re-run the walk on the AG if any inodes are
> skipped. This is done to prevent blocking on locks slowing down the
> scan - instead of blocking we move on to the next inode, and revist
> the blocked inodes again in a later pass.
> 
> IOWs, if you convert the code to use the generic walk and make
> xfs_free_eofblocks() return EAGAIN when it can't get the io lock,
> you get the retry for free...
> 

This sounds more efficient. ;) I'll move it to this approach. Thanks.

Brian

> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 3/4] xfs: add FREE_EOFBLOCKS ioctl

[Brian Foster]

On 09/05/2012 02:49 AM, Dave Chinner wrote:
> On Tue, Sep 04, 2012 at 10:10:35AM -0400, Brian Foster wrote:
>> On 09/03/2012 01:17 AM, Dave Chinner wrote:
>>> On Mon, Aug 27, 2012 at 03:51:50PM -0400, Brian Foster wrote:
...
>> This is something I see throughout the code that I haven't quite
>> followed (i.e., using the _t typedefs vs. not). Is the general consensus
>> to move away from typedefs when possible?
> 
> Yes. The Irix code that XFS came from was full of typedefs - part
> of it was to try to strictly type check things that were the same
> storage size or on-disk vs in-memory. We've got other ways of doing
> that better (e.g. the endian checking sparse does), and typedefs
> are generally frowned upon in the main kernel code because they
> often obfuscate the code rather than improve it, so we're
> removing them as we modify code or write new code.
> 

Good to know.

...
>> Ok. I was thinking that we could support the ability to scan by uid/gid
>> regardless of whether quota is enabled, but perhaps there's no purpose
>> to that if a quota isn't enabled.
> 
> I can't really think of a use case for doing this. Making the API
> more expansive in future if someone needs this can be done - it's
> removing stuff that is really hard to do. Hence, don't add it if it
> is not going to be used immediately. :)
> 

Ok.

Brian

> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: add background scanning to clear EOFBLOCKS inodes

[Brian Foster]

On 09/05/2012 03:00 AM, Dave Chinner wrote:
> On Tue, Sep 04, 2012 at 10:10:57AM -0400, Brian Foster wrote:
>> On 09/03/2012 01:28 AM, Dave Chinner wrote:
>>> On Mon, Aug 27, 2012 at 03:51:51PM -0400, Brian Foster wrote:
...
>>
>> Any thoughts on having tunables for both values (time and min size?) on
>> the background scanning?
> 
> Well, my suggestion for timing is as per above (xfs_syncd_centisecs
> * 100), but I don't really have any good rule of thumb for the
> minimum size. What threshold do people start to notice this?
> 

For the testing I've done so far, I'm hitting EDQUOT with 20-30GB of
space left while sequentially writing to many large files. I'm really
just trying to get used space before failure more in the ball park of
the limit, so I'm not going to complain too much over leaving a few
hundred MB or so around on an otherwise full quota. ;) From where I sit,
the problem is more when we extend a file by 2, 4, 8GB and consume a
large amount of limited available space.

I suppose for the background scanning, it's more about just using a
value that doesn't get in the way of general behavior/performance. I'll
do some more testing in this area.

> I'd SWAG that something like 32MB is a good size to start at because
> most IO subsystems will still be able to reach full bandwidth with
> extents of this size when reading files.
> 
> Alternatively, if you can determine if the inode is still in use at
> the time of the scan (e.g. elevated reference count due to an open
> fd) and skip the truncation for those inodes, then a minimum size is
> not really needed, right?
> 

Hmm, good idea. Though perhaps I can use the min_size as a force
parameter (i.e., trim anything over this size), and the inode in use
check allows a more conservative default. I'll have to play around with
the right time/size values some more to get a better feel for it. I'll
probably include tunables at least for testing purposes, and they can
always be removed later. Thanks.

Brian

> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC PATCH 4/4] xfs: add background scanning to clear EOFBLOCKS inodes

[Dave Chinner]

On Wed, Sep 05, 2012 at 08:22:14AM -0400, Brian Foster wrote:
> On 09/05/2012 03:00 AM, Dave Chinner wrote:
> > On Tue, Sep 04, 2012 at 10:10:57AM -0400, Brian Foster wrote:
> >> On 09/03/2012 01:28 AM, Dave Chinner wrote:
> >>> On Mon, Aug 27, 2012 at 03:51:51PM -0400, Brian Foster wrote:
> ...
> >>
> >> Any thoughts on having tunables for both values (time and min size?) on
> >> the background scanning?
> > 
> > Well, my suggestion for timing is as per above (xfs_syncd_centisecs
> > * 100), but I don't really have any good rule of thumb for the
> > minimum size. What threshold do people start to notice this?
> > 
> 
> For the testing I've done so far, I'm hitting EDQUOT with 20-30GB of
> space left while sequentially writing to many large files.

Sure, background scanning won't prevent that, though. The background
scan is to catch preallocation that is no longer needed. i.e. the
files are no longer being written and have no dirty date but due
to the access nature, xfs_release() didn't free the unused
preallocation. The background scan will clean that up faster than
waiting for the inodes to cycle through the cache....

> I'm really
> just trying to get used space before failure more in the ball park of
> the limit,

That's what prealloc size throttling is for. ;)

> so I'm not going to complain too much over leaving a few
> hundred MB or so around on an otherwise full quota. ;) From where I sit,
> the problem is more when we extend a file by 2, 4, 8GB and consume a
> large amount of limited available space.
> 
> I suppose for the background scanning, it's more about just using a
> value that doesn't get in the way of general behavior/performance. I'll
> do some more testing in this area.

Right.

> > I'd SWAG that something like 32MB is a good size to start at because
> > most IO subsystems will still be able to reach full bandwidth with
> > extents of this size when reading files.
> > 
> > Alternatively, if you can determine if the inode is still in use at
> > the time of the scan (e.g. elevated reference count due to an open
> > fd) and skip the truncation for those inodes, then a minimum size is
> > not really needed, right?
> > 
> 
> Hmm, good idea. Though perhaps I can use the min_size as a force
> parameter (i.e., trim anything over this size),

If it's a background scan, we don't want to trim active
preallocations.

> and the inode in use
> check allows a more conservative default.

I just thought of a better check than an in-use check - if the inode
has a dirty page cache, don't trim it as the speculative prealloc is
still useful. If the inode is clean, it has not recently been written
to so we can remove the speculative prealloc and we are unlikely to
suffer any penalty from doing so....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs