xfs: add support for tracking inodes with post-EOF speculative preallocation

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 <[email protected]>

fs/xfs/xfs_inode.h +3 -0

index 94b32f9..2cd2883 100644
@@ -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;

fs/xfs/xfs_iomap.c +7 -0

index 973dff6..2968ee8 100644
@@ -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;
 }

fs/xfs/xfs_iops.c +3 -0

index 4e00cf0..dcd1d5f 100644
@@ -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) {

fs/xfs/xfs_sync.c +29 -0

index 9654817..5e14741 100644
@@ -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)
+{

fs/xfs/xfs_sync.c +26 -0

+
+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)
+{

fs/xfs/xfs_vnodeops.c +2 -0

index 2a5c6373..658ee2e 100644
@@ -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);

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 <[email protected]>

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

fs/xfs/xfs_sync.c +9 -0

index 5e14741..27c3c46 100644
@@ -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

fs/xfs/xfs_sync.c +18 -0

+	}
+
+	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) {

fs/xfs/xfs_sync.c +9 -0

+ */
+int
+xfs_inodes_free_eofblocks(
+	struct xfs_mount	*mp,
+	int			qtype,
+	uint32_t		id,
+	uint64_t		min_file_size,
+	int			flags)
+{

fs/xfs/xfs_sync.c +17 -0

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

fs/xfs/xfs_sync.h +0 -0

index 4486491..78aca41 100644

fs/xfs/xfs_sync.h +2 -0

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

fs/xfs/xfs_vnodeops.c +0 -0

index 658ee2e..53460f3 100644
@@ -150,11 +150,12 @@ xfs_readlink(
  * when the link count isn't zero and by xfs_dm_punch_hole() when

On Mon, Aug 27, 2012 at 03:51:49PM -0400, Brian Foster wrote:

Quoted 14 lines

fs/xfs/xfs_sync.c +14 -0

+/*
+ * 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;

fs/xfs/xfs_sync.c xfs_reclaim_inodes_count( · line 992

Quoted 5 lines

fs/xfs/xfs_sync.c +5 -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....

fs/xfs/xfs_sync.c xfs_reclaim_inodes_count( · line 998

Quoted 6 lines

fs/xfs/xfs_sync.c +6 -0

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

fs/xfs/xfs_sync.c xfs_reclaim_inodes_count( · line 1002

Quoted 4 lines

fs/xfs/xfs_sync.c +4 -0

+		/* !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.

fs/xfs/xfs_sync.c xfs_reclaim_inodes_count( · line 1005

Quoted 3 lines

fs/xfs/xfs_sync.c +3 -0

+	} 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)

Quoted 18 lines

fs/xfs/xfs_sync.c +18 -0

+	}
+
+	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) {

fs/xfs/xfs_sync.c xfs_reclaim_inodes_count( · line 1033

Quoted 10 lines

fs/xfs/xfs_sync.c +10 -0

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

fs/xfs/xfs_sync.c xfs_reclaim_inodes_count( · line 1039

Quoted 6 lines

fs/xfs/xfs_sync.c +6 -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?

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.

Quoted 9 lines

+ */

fs/xfs/xfs_sync.c +8 -0

+int
+xfs_inodes_free_eofblocks(
+	struct xfs_mount	*mp,
+	int			qtype,
+	uint32_t		id,
+	uint64_t		min_file_size,
+	int			flags)
+{

.....

fs/xfs/xfs_sync.c xfs_reclaim_inodes_count( · line 1114

Quoted 5 lines

fs/xfs/xfs_sync.c +5 -0

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

fs/xfs/xfs_sync.c xfs_reclaim_inodes_count( · line 1124

Quoted 11 lines

fs/xfs/xfs_sync.c +10 -0

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

Quoted 25 lines

fs/xfs/xfs_sync.c +17 -0

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

fs/xfs/xfs_sync.h +0 -0

index 4486491..78aca41 100644

fs/xfs/xfs_sync.h void __xfs_inode_set_reclaim_tag(struct xfs_perag *pag, struct xfs_inode *ip); · line 46

Quoted 4 lines

fs/xfs/xfs_sync.h +1 -0

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

Quoted 13 lines

fs/xfs/xfs_sync.h +2 -0

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

fs/xfs/xfs_vnodeops.c +0 -0

index 658ee2e..53460f3 100644
@@ -150,11 +150,12 @@ xfs_readlink(
  * when the link count isn't zero and by xfs_dm_punch_hole() when

fs/xfs/xfs_vnodeops.c xfs_readlink( · line 158

Quoted 10 lines

fs/xfs/xfs_vnodeops.c +3 -2

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

Quoted 1 line · Dave Chinner

On Mon, Aug 27, 2012 at 03:51:49PM -0400, Brian Foster wrote:

...

Quoted 14 lines

Quoted 14 lines

fs/xfs/xfs_sync.c +14 -0

+/*
+ * 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;

Quoted 9 lines

fs/xfs/xfs_sync.c xfs_reclaim_inodes_count( · line 992

Quoted 5 lines

fs/xfs/xfs_sync.c +5 -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).

Quoted 27 lines · Brian Foster

Quoted 1 line · Dave Chinner

On Mon, Aug 27, 2012 at 03:51:49PM -0400, Brian Foster wrote:

...

Quoted 23 lines

Quoted 9 lines

fs/xfs/xfs_sync.c +9 -0

+/*
+ * 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

fs/xfs/xfs_sync.c xfs_reclaim_inodes_count( · line 992

Quoted 10 lines

fs/xfs/xfs_sync.c +10 -0

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

Quoted 2 lines

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.

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 <[email protected]>

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

fs/xfs/xfs_fs.h +0 -0

index c13fed8..6f93db9 100644

fs/xfs/xfs_fs.h +0 -0

  * ioctl commands that replace IRIX syssgi()'s

fs/xfs/xfs_ioctl.c +9 -0

index 0e0232c..b91cbcd 100644
@@ -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);
+

Quoted 19 lines · 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 <[email protected]>

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

fs/xfs/xfs_fs.h +0 -0

index c13fed8..6f93db9 100644
@@ -339,6 +339,15 @@ typedef struct xfs_error_injection {
 
 

fs/xfs/xfs_fs.h typedef struct xfs_error_injection { · line 348

Quoted 8 lines

fs/xfs/xfs_fs.h +7 -0

 /*
+ * 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.

Quoted 16 lines

fs/xfs/xfs_fs.h +16 -0

+	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

fs/xfs/xfs_fs.h

Quoted 3 lines

fs/xfs/xfs_fs.h +3 -0

+		 * 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).

fs/xfs/xfs_fs.h

Quoted 2 lines

fs/xfs/xfs_fs.h +2 -0

+		/* 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.

Quoted 20 lines · Dave Chinner

Quoted 19 lines · 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 <[email protected]>

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

fs/xfs/xfs_fs.h +0 -0

index c13fed8..6f93db9 100644
@@ -339,6 +339,15 @@ typedef struct xfs_error_injection {
 
 

Quoted 11 lines

fs/xfs/xfs_fs.h typedef struct xfs_error_injection { · line 348

Quoted 8 lines

fs/xfs/xfs_fs.h +7 -0

 /*
+ * 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?

Quoted 8 lines

Quoted 8 lines

fs/xfs/xfs_fs.h +8 -0

+
+		/*
+		 * 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

Quoted 9 lines

fs/xfs/xfs_fs.h

Quoted 3 lines

fs/xfs/xfs_fs.h +3 -0

+		 * 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.

Quoted 2 lines · Brian Foster

Quoted 1 line · Dave Chinner

On Mon, Aug 27, 2012 at 03:51:50PM -0400, Brian Foster wrote:

Quoted 3 lines

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.

Quoted 3 lines

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.

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.

Quoted 8 lines · 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.

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.

Quoted 16 lines · Dave Chinner

Quoted 8 lines · 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.

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.

Quoted 4 lines

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?

Quoted 2 lines · Brian Foster

Quoted 1 line · Dave Chinner

On Mon, Aug 27, 2012 at 03:51:51PM -0400, Brian Foster wrote:

Quoted 3 lines

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.

Quoted 3 lines · Dave Chinner

Quoted 2 lines · Brian Foster

Quoted 1 line · Dave Chinner

On Mon, Aug 27, 2012 at 03:51:51PM -0400, Brian Foster wrote:

...

Quoted 4 lines

Quoted 3 lines

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

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.