speculative preallocation inode tracking

This check is used in multiple places to determine whether we should check for (and potentially free) post EOF blocks on an inode. Add a helper to consolidate the check.

Note that when we remove an inode from the cache (xfs_inactive()), we are required to trim post-EOF blocks even if the inode is marked preallocated or append-only to maintain correct space accounting. The 'force' parameter to xfs_can_free_eofblocks() specifies whether we should ignore the prealloc/append-only status of the inode.

Signed-off-by Brian Foster <[email protected]>

xfs_inodes_free_eofblocks() implements scanning functionality for EOFBLOCKS inodes. It uses the AG iterator to walk the tagged inodes and free post-EOF blocks via the xfs_inode_free_eofblocks() execute function. The scan can be invoked in best-effort mode or wait (force) mode.

A best-effort scan (default) handles all inodes that do not have a dirty cache and we successfully acquire the io lock via trylock. In wait mode, we continue to cycle through an AG until all inodes are handled.

Signed-off-by Brian Foster <[email protected]>

The XFS_IOC_FREE_EOFBLOCKS ioctl allows users to invoke an EOFBLOCKS scan. The xfs_eofblocks structure is defined to support the command parameters (scan mode).

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_fs.h     |   15 +++++++++++++++
 fs/xfs/xfs_icache.c |    5 +++--
 fs/xfs/xfs_icache.h |    2 +-
 fs/xfs/xfs_ioctl.c  |   16 ++++++++++++++++
 4 files changed, 35 insertions(+), 3 deletions(-)

fs/xfs/xfs_fs.h +15 -0

index c13fed8..5f48b3e 100644
@@ -339,6 +339,20 @@ typedef struct xfs_error_injection {
 
 
 /*
+ * Speculative preallocation trimming.
+ */
+#define XFS_EOFBLOCKS_VERSION		1
+struct xfs_eofblocks {
+	__u32		eof_version;
+	__u32		eof_flags;
+	unsigned char	pad[12];
+};
+
+/* eof_flags values */
+#define XFS_EOF_FLAGS_SYNC		0x01	/* sync/wait mode scan */
+
+
+/*
  * The user-level Handle Request interface structure.
  */
 typedef struct xfs_fsop_handlereq {

@@ -456,6 +470,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

fs/xfs/xfs_icache.c +3 -2

index b02a3df..35efdda 100644
@@ -1206,11 +1206,12 @@ xfs_inode_free_eofblocks(
 int
 xfs_icache_free_eofblocks(
 	struct xfs_mount	*mp,
-	int			flags)
+	int			flags,
+	struct xfs_eofblocks	*eofb)
 {
 	ASSERT((flags & ~(SYNC_TRYLOCK|SYNC_WAIT)) == 0);
 	return xfs_inode_ag_iterator_tag(mp, xfs_inode_free_eofblocks, flags,
-					 NULL, XFS_ICI_EOFBLOCKS_TAG);
+					 eofb, XFS_ICI_EOFBLOCKS_TAG);
 }
 
 void

fs/xfs/xfs_icache.h +1 -1

index abca9fb..b46716c 100644
@@ -37,7 +37,7 @@ void xfs_inode_set_reclaim_tag(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_icache_free_eofblocks(struct xfs_mount *, int);
+int xfs_icache_free_eofblocks(struct xfs_mount *, int, struct xfs_eofblocks *);
 
 int xfs_sync_inode_grab(struct xfs_inode *ip);
 int xfs_inode_ag_iterator(struct xfs_mount *mp,

fs/xfs/xfs_ioctl.c +16 -0

index 0e0232c..ad4352f 100644
@@ -42,6 +42,7 @@
 #include "xfs_inode_item.h"
 #include "xfs_export.h"
 #include "xfs_trace.h"
+#include "xfs_icache.h"
 
 #include <linux/capability.h>
 #include <linux/dcache.h>

@@ -1602,6 +1603,21 @@ xfs_file_ioctl(
 		error = xfs_errortag_clearall(mp, 1);
 		return -error;
 
+	case XFS_IOC_FREE_EOFBLOCKS: {
+		struct xfs_eofblocks eofb;
+		int flags;
+
+		if (copy_from_user(&eofb, arg, sizeof(eofb)))
+			return -XFS_ERROR(EFAULT);
+
+		if (eofb.eof_version != XFS_EOFBLOCKS_VERSION)
+			return -XFS_ERROR(EINVAL);
+
+		flags = (eofb.eof_flags & XFS_EOF_FLAGS_SYNC) ? SYNC_WAIT : SYNC_TRYLOCK;
+		error = xfs_icache_free_eofblocks(mp, flags, &eofb);
+		return -error;
+	}
+
 	default:
 		return -ENOTTY;
 	}

Signature · 6 lines

1.7.7.6

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Hey Brian,

Quoted 3 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 (scan mode).

It would help to have an xfstest to exercise this ioctl to pull in with this series. Do you have any code that could be wrangled into a test case?

Regards,
Ben

Quoted 111 lines

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_fs.h     |   15 +++++++++++++++
 fs/xfs/xfs_icache.c |    5 +++--
 fs/xfs/xfs_icache.h |    2 +-
 fs/xfs/xfs_ioctl.c  |   16 ++++++++++++++++
 4 files changed, 35 insertions(+), 3 deletions(-)

fs/xfs/xfs_fs.h +15 -0

index c13fed8..5f48b3e 100644
@@ -339,6 +339,20 @@ typedef struct xfs_error_injection {
 
 
 /*
+ * Speculative preallocation trimming.
+ */
+#define XFS_EOFBLOCKS_VERSION		1
+struct xfs_eofblocks {
+	__u32		eof_version;
+	__u32		eof_flags;
+	unsigned char	pad[12];
+};
+
+/* eof_flags values */
+#define XFS_EOF_FLAGS_SYNC		0x01	/* sync/wait mode scan */
+
+
+/*
  * The user-level Handle Request interface structure.
  */
 typedef struct xfs_fsop_handlereq {

@@ -456,6 +470,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

fs/xfs/xfs_icache.c +3 -2

index b02a3df..35efdda 100644
@@ -1206,11 +1206,12 @@ xfs_inode_free_eofblocks(
 int
 xfs_icache_free_eofblocks(
 	struct xfs_mount	*mp,
-	int			flags)
+	int			flags,
+	struct xfs_eofblocks	*eofb)
 {
 	ASSERT((flags & ~(SYNC_TRYLOCK|SYNC_WAIT)) == 0);
 	return xfs_inode_ag_iterator_tag(mp, xfs_inode_free_eofblocks, flags,
-					 NULL, XFS_ICI_EOFBLOCKS_TAG);
+					 eofb, XFS_ICI_EOFBLOCKS_TAG);
 }
 
 void

fs/xfs/xfs_icache.h +1 -1

index abca9fb..b46716c 100644
@@ -37,7 +37,7 @@ void xfs_inode_set_reclaim_tag(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_icache_free_eofblocks(struct xfs_mount *, int);
+int xfs_icache_free_eofblocks(struct xfs_mount *, int, struct xfs_eofblocks *);
 
 int xfs_sync_inode_grab(struct xfs_inode *ip);
 int xfs_inode_ag_iterator(struct xfs_mount *mp,

fs/xfs/xfs_ioctl.c +16 -0

index 0e0232c..ad4352f 100644
@@ -42,6 +42,7 @@
 #include "xfs_inode_item.h"
 #include "xfs_export.h"
 #include "xfs_trace.h"
+#include "xfs_icache.h"
 
 #include <linux/capability.h>
 #include <linux/dcache.h>

@@ -1602,6 +1603,21 @@ xfs_file_ioctl(
 		error = xfs_errortag_clearall(mp, 1);
 		return -error;
 
+	case XFS_IOC_FREE_EOFBLOCKS: {
+		struct xfs_eofblocks eofb;
+		int flags;
+
+		if (copy_from_user(&eofb, arg, sizeof(eofb)))
+			return -XFS_ERROR(EFAULT);
+
+		if (eofb.eof_version != XFS_EOFBLOCKS_VERSION)
+			return -XFS_ERROR(EINVAL);
+
+		flags = (eofb.eof_flags & XFS_EOF_FLAGS_SYNC) ? SYNC_WAIT : SYNC_TRYLOCK;
+		error = xfs_icache_free_eofblocks(mp, flags, &eofb);
+		return -error;
+	}
+
 	default:
 		return -ENOTTY;
 	}

Signature · 1 line

1.7.7.6

Quoted 4 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 10 lines · Ben Myers

Hey Brian,

Quoted 3 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 (scan mode).

It would help to have an xfstest to exercise this ioctl to pull in with this series. Do you have any code that could be wrangled into a test case?

Yes, makes sense. I have some very basic test code I could put somewhere to invoke the ioctl(). One of the questions I've been meaning to ask is whether it would be relevant for that code to live in a common tool, such as adding a new command to xfs_io. Then perhaps create an xfstests test using that. Thoughts?

FYI, I have a few other things on my plate at the moment so unfortunately it will be a bit before I can get back to XFS work... But I'm fine with the set pending until I can come up with some test coverage if that is preferable, of course.

Brian

Quoted 114 lines

Regards,
Ben

Quoted 111 lines

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_fs.h     |   15 +++++++++++++++
 fs/xfs/xfs_icache.c |    5 +++--
 fs/xfs/xfs_icache.h |    2 +-
 fs/xfs/xfs_ioctl.c  |   16 ++++++++++++++++
 4 files changed, 35 insertions(+), 3 deletions(-)

fs/xfs/xfs_fs.h +15 -0

index c13fed8..5f48b3e 100644
@@ -339,6 +339,20 @@ typedef struct xfs_error_injection {
 
 
 /*
+ * Speculative preallocation trimming.
+ */
+#define XFS_EOFBLOCKS_VERSION		1
+struct xfs_eofblocks {
+	__u32		eof_version;
+	__u32		eof_flags;
+	unsigned char	pad[12];
+};
+
+/* eof_flags values */
+#define XFS_EOF_FLAGS_SYNC		0x01	/* sync/wait mode scan */
+
+
+/*
  * The user-level Handle Request interface structure.
  */
 typedef struct xfs_fsop_handlereq {

@@ -456,6 +470,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

fs/xfs/xfs_icache.c +3 -2

index b02a3df..35efdda 100644
@@ -1206,11 +1206,12 @@ xfs_inode_free_eofblocks(
 int
 xfs_icache_free_eofblocks(
 	struct xfs_mount	*mp,
-	int			flags)
+	int			flags,
+	struct xfs_eofblocks	*eofb)
 {
 	ASSERT((flags & ~(SYNC_TRYLOCK|SYNC_WAIT)) == 0);
 	return xfs_inode_ag_iterator_tag(mp, xfs_inode_free_eofblocks, flags,
-					 NULL, XFS_ICI_EOFBLOCKS_TAG);
+					 eofb, XFS_ICI_EOFBLOCKS_TAG);
 }
 
 void

fs/xfs/xfs_icache.h +1 -1

index abca9fb..b46716c 100644
@@ -37,7 +37,7 @@ void xfs_inode_set_reclaim_tag(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_icache_free_eofblocks(struct xfs_mount *, int);
+int xfs_icache_free_eofblocks(struct xfs_mount *, int, struct xfs_eofblocks *);
 
 int xfs_sync_inode_grab(struct xfs_inode *ip);
 int xfs_inode_ag_iterator(struct xfs_mount *mp,

fs/xfs/xfs_ioctl.c +16 -0

index 0e0232c..ad4352f 100644
@@ -42,6 +42,7 @@
 #include "xfs_inode_item.h"
 #include "xfs_export.h"
 #include "xfs_trace.h"
+#include "xfs_icache.h"
 
 #include <linux/capability.h>
 #include <linux/dcache.h>

@@ -1602,6 +1603,21 @@ xfs_file_ioctl(
 		error = xfs_errortag_clearall(mp, 1);
 		return -error;
 
+	case XFS_IOC_FREE_EOFBLOCKS: {
+		struct xfs_eofblocks eofb;
+		int flags;
+
+		if (copy_from_user(&eofb, arg, sizeof(eofb)))
+			return -XFS_ERROR(EFAULT);
+
+		if (eofb.eof_version != XFS_EOFBLOCKS_VERSION)
+			return -XFS_ERROR(EINVAL);
+
+		flags = (eofb.eof_flags & XFS_EOF_FLAGS_SYNC) ? SYNC_WAIT : SYNC_TRYLOCK;
+		error = xfs_icache_free_eofblocks(mp, flags, &eofb);
+		return -error;
+	}
+
 	default:
 		return -ENOTTY;
 	}

Signature · 1 line

1.7.7.6

Quoted 4 lines

Quoted 4 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Hey Brian,

Quoted 12 lines · Brian Foster

Quoted 10 lines · Ben Myers

Hey Brian,

Quoted 3 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 (scan mode).

It would help to have an xfstest to exercise this ioctl to pull in with this series. Do you have any code that could be wrangled into a test case?

Quoted 5 lines

Yes, makes sense. I have some very basic test code I could put somewhere to invoke the ioctl(). One of the questions I've been meaning to ask is whether it would be relevant for that code to live in a common tool, such as adding a new command to xfs_io. Then perhaps create an xfstests test using that. Thoughts?

IMO you are right on the mark. xfs_io is a great place for this.

Quoted 4 lines

FYI, I have a few other things on my plate at the moment so unfortunately it will be a bit before I can get back to XFS work... But I'm fine with the set pending until I can come up with some test coverage if that is preferable, of course.

I do think it is preferable to have a test case go in with the code where possible. Since you don't mind waiting a bit, that seems to be the way to go. The other option could be to look for a volunteer to work on the test. ;)

Quoted 28 lines · Ben Myers

Hey Brian,

Quoted 12 lines · Brian Foster

Quoted 10 lines · Ben Myers

Hey Brian,

Quoted 3 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 (scan mode).

It would help to have an xfstest to exercise this ioctl to pull in with this series. Do you have any code that could be wrangled into a test case?

Quoted 5 lines

Yes, makes sense. I have some very basic test code I could put somewhere to invoke the ioctl(). One of the questions I've been meaning to ask is whether it would be relevant for that code to live in a common tool, such as adding a new command to xfs_io. Then perhaps create an xfstests test using that. Thoughts?

IMO you are right on the mark. xfs_io is a great place for this.

Quoted 4 lines

FYI, I have a few other things on my plate at the moment so unfortunately it will be a bit before I can get back to XFS work... But I'm fine with the set pending until I can come up with some test coverage if that is preferable, of course.

Quoted 3 lines

I do think it is preferable to have a test case go in with the code where possible. Since you don't mind waiting a bit, that seems to be the way to go. The other option could be to look for a volunteer to work on the test. ;)

FWIW, given the background cleanup code can be trivially verified to work (open, apend, close, repeat, wait 5 minutes) and is the functionality that is needed in mainline, having something to test the ioctls should not stop the patchset from being merged.

xfs_io -f -c "pwrite $((i * 4096)) 4096" /mnt/scratch/foo done

Quoted 29 lines · Dave Chinner

Quoted 28 lines · Ben Myers

Hey Brian,

Quoted 12 lines · Brian Foster

Quoted 10 lines · Ben Myers

Hey Brian,

Quoted 3 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 (scan mode).

It would help to have an xfstest to exercise this ioctl to pull in with this series. Do you have any code that could be wrangled into a test case?

Quoted 5 lines

Yes, makes sense. I have some very basic test code I could put somewhere to invoke the ioctl(). One of the questions I've been meaning to ask is whether it would be relevant for that code to live in a common tool, such as adding a new command to xfs_io. Then perhaps create an xfstests test using that. Thoughts?

IMO you are right on the mark. xfs_io is a great place for this.

Quoted 4 lines

FYI, I have a few other things on my plate at the moment so unfortunately it will be a bit before I can get back to XFS work... But I'm fine with the set pending until I can come up with some test coverage if that is preferable, of course.

Quoted 8 lines

Quoted 3 lines

I do think it is preferable to have a test case go in with the code where possible. Since you don't mind waiting a bit, that seems to be the way to go. The other option could be to look for a volunteer to work on the test. ;)

FWIW, given the background cleanup code can be trivially verified to work (open, apend, close, repeat, wait 5 minutes) and is the functionality that is needed in mainline, having something to test the ioctls should not stop the patchset from being merged.

i.e.:

$ for i in `seq 0 512`; do

Quoted 2 lines

xfs_io -f -c "pwrite $((i * 4096)) 4096" /mnt/scratch/foo done

$ stat -c %b /mnt/scratch/foo
8192
$ sync; stat -c %b /mnt/scratch/foo
8192
$ sleep 30; stat -c %b /mnt/scratch/foo
8192
$ sleep 300; stat -c %b /mnt/scratch/foo
4104

It works. ;)

Hi Fellas,

Quoted 4 lines · Dave Chinner

Quoted 3 lines · Dave Chinner

Quoted 2 lines · Ben Myers

Hey Brian,

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

I do think it is preferable to have a test case go in with the code where possible. Since you don't mind waiting a bit, that seems to be the way to go. The other option could be to look for a volunteer to work on the test. ;)

FWIW, given the background cleanup code can be trivially verified to work (open, apend, close, repeat, wait 5 minutes) and is the functionality that is needed in mainline, having something to test the ioctls should not stop the patchset from being merged.

Can we be assured that we'll get an xfstest for it eventually?

I think we can pull this in if Brian is willing post his test code. Initially it needn't be posted as an xfstest, that's fine. As it stands today it appears that Brian is the only one to ever use the ioctl and there is no way for anyone else to test it. Not an ideal situation.

I think this is a reasonable request. Usually it's Christoph who asks for a test case. ;)

Quoted 14 lines

i.e.:

$ for i in `seq 0 512`; do

Quoted 2 lines

xfs_io -f -c "pwrite $((i * 4096)) 4096" /mnt/scratch/foo done

$ stat -c %b /mnt/scratch/foo
8192
$ sync; stat -c %b /mnt/scratch/foo
8192
$ sleep 30; stat -c %b /mnt/scratch/foo
8192
$ sleep 300; stat -c %b /mnt/scratch/foo
4104

Quoted 1 line

It works. ;)

Nice!

Regards,
Ben

Quoted 5 lines · Ben Myers

Quoted 4 lines · Dave Chinner

Quoted 3 lines · Dave Chinner

Quoted 2 lines · Ben Myers

Hey Brian,

...

Quoted 9 lines

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

I do think it is preferable to have a test case go in with the code where possible. Since you don't mind waiting a bit, that seems to be the way to go. The other option could be to look for a volunteer to work on the test. ;)

FWIW, given the background cleanup code can be trivially verified to work (open, apend, close, repeat, wait 5 minutes) and is the functionality that is needed in mainline, having something to test the ioctls should not stop the patchset from being merged.

Quoted 2 lines

Can we be assured that we'll get an xfstest for it eventually?

Absolutely. Getting a command into xfs_io to support such a test is now the top of my todo list with regard to XFS. :)

Quoted 5 lines

I think we can pull this in if Brian is willing post his test code. Initially it needn't be posted as an xfstest, that's fine. As it stands today it appears that Brian is the only one to ever use the ioctl and there is no way for anyone else to test it. Not an ideal situation.

The impetus for this work was some prototype quota work for the gluster distributed filesystem.

Beyond testing via that, I just had a very
simple/stupid program with the command and data structure bits copy/pasted in to open a file and invoke an xfsctl() with hardcoded parameters. I don't consider it post-worthy or very useful, but attached nonetheless since I have a couple things to deal with before I get a chance to play with xfs_io.

Hey Brian,

Quoted 16 lines · Brian Foster

Quoted 5 lines · Ben Myers

Quoted 4 lines · Dave Chinner

Quoted 3 lines · Dave Chinner

Quoted 2 lines · Ben Myers

Hey Brian,

...

Quoted 9 lines

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

I do think it is preferable to have a test case go in with the code where possible. Since you don't mind waiting a bit, that seems to be the way to go. The other option could be to look for a volunteer to work on the test. ;)

FWIW, given the background cleanup code can be trivially verified to work (open, apend, close, repeat, wait 5 minutes) and is the functionality that is needed in mainline, having something to test the ioctls should not stop the patchset from being merged.

Quoted 5 lines

Quoted 2 lines

Can we be assured that we'll get an xfstest for it eventually?

Absolutely. Getting a command into xfs_io to support such a test is now the top of my todo list with regard to XFS. :)

Excellent!

Quoted 6 lines

Quoted 5 lines

I think we can pull this in if Brian is willing post his test code. Initially it needn't be posted as an xfstest, that's fine. As it stands today it appears that Brian is the only one to ever use the ioctl and there is no way for anyone else to test it. Not an ideal situation.

Quoted 2 lines

The impetus for this work was some prototype quota work for the gluster distributed filesystem.

Now I'm curious where you're coming from. Is gluster your primary project?

Quoted 6 lines

Beyond testing via that, I just had a very
simple/stupid program with the command and data structure bits copy/pasted in to open a file and invoke an xfsctl() with hardcoded parameters. I don't consider it post-worthy or very useful, but attached nonetheless since I have a couple things to deal with before I get a chance to play with xfs_io.

Thanks for posting it anyway. That's enough for an interested party to do a little fooling. I believe Mark is finishing up a review of this series, so we'll get it in soon.

Regards,
Ben

Quoted 6 lines · Ben Myers

Quoted 4 lines

Quoted 4 lines

FWIW, given the background cleanup code can be trivially verified to work (open, apend, close, repeat, wait 5 minutes) and is the functionality that is needed in mainline, having something to test the ioctls should not stop the patchset from being merged.

Can we be assured that we'll get an xfstest for it eventually?

Absolutely. Getting a command into xfs_io to support such a test is now the top of my todo list with regard to XFS. :)

Quoted 6 lines · Brian Foster

Quoted 5 lines · Ben Myers

Quoted 4 lines

Quoted 4 lines

FWIW, given the background cleanup code can be trivially verified to work (open, apend, close, repeat, wait 5 minutes) and is the functionality that is needed in mainline, having something to test the ioctls should not stop the patchset from being merged.

Quoted 4 lines

Quoted 1 line

Can we be assured that we'll get an xfstest for it eventually?

Absolutely. Getting a command into xfs_io to support such a test is now the top of my todo list with regard to XFS. :)

Here's a patch to the new xfs_spaceman program I'm writing that adds control for these ioctls.

Cheers,

Dave.

Quoted 12 lines · Dave Chinner

Quoted 10 lines · Brian Foster

Quoted 6 lines · Ben Myers

Quoted 4 lines

Quoted 4 lines

FWIW, given the background cleanup code can be trivially verified to work (open, apend, close, repeat, wait 5 minutes) and is the functionality that is needed in mainline, having something to test the ioctls should not stop the patchset from being merged.

Can we be assured that we'll get an xfstest for it eventually?

Absolutely. Getting a command into xfs_io to support such a test is now the top of my todo list with regard to XFS. :)

Quoted 3 lines

Here's a patch to the new xfs_spaceman program I'm writing that adds control for these ioctls.

Very cool, thanks. Catchy name for the tool as well, btw ;).

For some reason my mailer is stripping out the patch,

but my only
comment is with regard to minlen. Shouldn't that variable be handled as an unsigned?

Now that I think of it, that makes me wonder if I should make that a 64-bit unsigned in xfs_eofblocks..?

The XFS_IOC_FREE_EOFBLOCKS ioctl allows users to invoke an EOFBLOCKS scan. The xfs_eofblocks structure is defined to support the command parameters (scan mode).

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_fs.h     |   15 +++++++++++++++
 fs/xfs/xfs_icache.c |    5 +++--
 fs/xfs/xfs_icache.h |    2 +-
 fs/xfs/xfs_ioctl.c  |   16 ++++++++++++++++
 4 files changed, 35 insertions(+), 3 deletions(-)

fs/xfs/xfs_fs.h +0 -0

index c13fed8..5f48b3e 100644
@@ -339,6 +339,20 @@ typedef struct xfs_error_injection {
 

fs/xfs/xfs_ioctl.c +1 -0

index 0e0232c..ad4352f 100644
@@ -42,6 +42,7 @@
 #include "xfs_inode_item.h"
 #include "xfs_export.h"
 #include "xfs_trace.h"
+#include "xfs_icache.h"
 
 #include <linux/capability.h>
 #include <linux/dcache.h>

@@ -1602,6 +1603,21 @@ xfs_file_ioctl(
 		error = xfs_errortag_clearall(mp, 1);
 		return -error;

On Fri, Oct 05, 2012 at 10:17:12AM -0400, Brian Foster wrote:

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

Quoted 9 lines

/*
+ * Speculative preallocation trimming.
+ */

fs/xfs/xfs_fs.h +6 -0

+#define XFS_EOFBLOCKS_VERSION		1
+struct xfs_eofblocks {
+	__u32		eof_version;
+	__u32		eof_flags;
+	unsigned char	pad[12];
+};

12 bytes of padding is a bit wierd at this point. It's problematic for 32bit userspace on 64 bit kernels, in that the size of the structure can end up different (i.e. 20 bytes on 32b, 24 bytes on 64b) depending on the architectures natural alignment.

I can also see that adding multiple extra variables to the structure are quite likely (e.g. per-ag control, start/end inode numbers, etc), so 12 bytes of padding really isn't sufficient, IMO. I'd tend to pad out to, say, 128 bytes rather than 32, just in case. i.e:

	__u64		pad[15];

And then take away from this padding space as you add functioanlity in fucture patches.

This extra padding means the version number won't need to increase any time soon, as it will be a while before we run out of either padding or flag space, instead of as soon as we add a new function to the ioctl....

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

Quoted 3 lines

fs/xfs/xfs_fs.h +3 -0

+
+/* eof_flags values */
+#define XFS_EOF_FLAGS_SYNC		0x01	/* sync/wait mode scan */

I kind of prefer flags being defined by (1 << 0) style to keep larger flag numbers concise, but that's not a big deal.

Quoted 16 lines

fs/xfs/xfs_ioctl.c +1 -0

index 0e0232c..ad4352f 100644
@@ -42,6 +42,7 @@
 #include "xfs_inode_item.h"
 #include "xfs_export.h"
 #include "xfs_trace.h"
+#include "xfs_icache.h"
 
 #include <linux/capability.h>
 #include <linux/dcache.h>

@@ -1602,6 +1603,21 @@ xfs_file_ioctl(
 		error = xfs_errortag_clearall(mp, 1);
 		return -error;
 

fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1614

Quoted 9 lines

fs/xfs/xfs_ioctl.c +9 -0

+	case XFS_IOC_FREE_EOFBLOCKS: {
+		struct xfs_eofblocks eofb;
+		int flags;
+
+		if (copy_from_user(&eofb, arg, sizeof(eofb)))
+			return -XFS_ERROR(EFAULT);
+
+		if (eofb.eof_version != XFS_EOFBLOCKS_VERSION)
+			return -XFS_ERROR(EINVAL);

Checking that no unsupported flags are set here is necessary. Also, checking the padding is zero is probably a good idea, as it will force applications to zero the structure properly before being able to use this interface properly....

fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1616

Quoted 1 line

fs/xfs/xfs_ioctl.c +1 -0

+		flags = (eofb.eof_flags & XFS_EOF_FLAGS_SYNC) ? SYNC_WAIT : SYNC_TRYLOCK;

Line if a bit too long. However, would it be better to place this inside xfs_icache_free_eofblocks()?

fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1617

Quoted 1 line

fs/xfs/xfs_ioctl.c +1 -0

+		error = xfs_icache_free_eofblocks(mp, flags, &eofb);

Cheers,

Dave.

Quoted 5 lines · Dave Chinner

fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1616

Quoted 1 line · Brian Foster

fs/xfs/xfs_ioctl.c +1 -0

+		flags = (eofb.eof_flags & XFS_EOF_FLAGS_SYNC) ? SYNC_WAIT : SYNC_TRYLOCK;

Line if a bit too long. However, would it be better to place this inside xfs_icache_free_eofblocks()?

Are you suggesting to eliminate the flags parameter to xfs_icache_free_eofblocks? The reason for the current interface is that the background scan caller doesn't require the eofb parameter, so I decided to generalize the sync/wait parameter in the caller.

If we want to push it into xfs_icache_free_eofblocks(), I think it would be better at that point to eliminate the flags param and either infer SYNC_TRYLOCK on a NULL eofb or to require an eofb and pass one with a cleared XFS_EOF_FLAGS_SYNC from the background scan. Thoughts?

Make xfs_quota_type() into a non-static function. This is to support quota ID filtering in the eofblocks ioctl().

Signed-off-by Brian Foster <[email protected]>

Support quota ID filtering in the eofblocks scan. The caller must set the XFS_EOF_FLAGS_QUOTA flags bit, quota ID and quota type. The associated quota type must be enabled on the mounted filesystem.

fs/xfs/xfs_icache.c +4 -0

index 35efdda..b39970b 100644
@@ -1171,6 +1171,22 @@ xfs_reclaim_inodes_count(
 }
 
 STATIC int
+xfs_inode_match_quota_id(
+	struct xfs_inode	*ip,
+	struct xfs_eofblocks	*eofb)
+{

fs/xfs/xfs_icache.c +7 -0

@@ -1194,6 +1211,13 @@ xfs_inode_free_eofblocks(
 	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY))
 		return 0;
 
+	if (eofb) {
+		/* filter by quota id */
+		if (eofb->eof_flags & XFS_EOF_FLAGS_QUOTA &&
+		    !xfs_inode_match_quota_id(ip, eofb))
+			return 0;
+	}
+
 	ret = xfs_free_eofblocks(ip->i_mount, ip, true);
 
 	/* don't revisit the inode if we're not waiting */

fs/xfs/xfs_ioctl.c +0 -0

index ad4352f..547363b 100644

Quoted 3 lines · Brian Foster

Support quota ID filtering in the eofblocks scan. The caller must set the XFS_EOF_FLAGS_QUOTA flags bit, quota ID and quota type. The associated quota type must be enabled on the mounted filesystem.

I'm wondering if this even needs quota enabled to filter based on a uid, gid, or prid?

The quota part of it seem irrelevant to the filtering that is being executed - we are only matching against the inode uid/gid/prid, not against dquots - and I can see situations where just being able to filter on a given uid/gid might be useful in a multi-user environment regardless of whether quotas are being used.

Indeed, even testing is made much easier if we don't have to juggle quota mount options to test the filtering....

Quoted 12 lines

fs/xfs/xfs_icache.c +4 -0

index 35efdda..b39970b 100644
@@ -1171,6 +1171,22 @@ xfs_reclaim_inodes_count(
 }
 
 STATIC int
+xfs_inode_match_quota_id(
+	struct xfs_inode	*ip,
+	struct xfs_eofblocks	*eofb)
+{

fs/xfs/xfs_icache.c xfs_reclaim_inodes_count( · line 1187

Quoted 10 lines

fs/xfs/xfs_icache.c +10 -0

+	unsigned int type = xfs_quota_type(eofb->eof_q_type);
+	if (type == XFS_DQ_USER)
+		return ip->i_d.di_uid == eofb->eof_q_id;
+	else if (type == XFS_DQ_GROUP)
+		return ip->i_d.di_gid == eofb->eof_q_id;
+	else if (type == XFS_DQ_PROJ)
+		return xfs_get_projid(ip) == eofb->eof_q_id;
+
+	return 0;
+}

Why do you need xfs_quota_type() here? why not just:

	switch (type) {
	case XQM_USRQUOTA:
		return ip->i_d.di_uid == eofb->eof_q_id;
	case XQM_GRPQUOTA:
		return ip->i_d.di_gid == eofb->eof_q_id;
	case XQM_PRJQUOTA:
		return xfs_get_projid(ip) == eofb->eof_q_id;
	default:
		break;
	}

	return 0;

Quoted 22 lines

fs/xfs/xfs_icache.c +7 -0

@@ -1194,6 +1211,13 @@ xfs_inode_free_eofblocks(
 	    mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY))
 		return 0;
 
+	if (eofb) {
+		/* filter by quota id */
+		if (eofb->eof_flags & XFS_EOF_FLAGS_QUOTA &&
+		    !xfs_inode_match_quota_id(ip, eofb))
+			return 0;
+	}
+
 	ret = xfs_free_eofblocks(ip->i_mount, ip, true);
 
 	/* don't revisit the inode if we're not waiting */

fs/xfs/xfs_ioctl.c +0 -0

index ad4352f..547363b 100644
@@ -1613,6 +1613,14 @@ xfs_file_ioctl(
 		if (eofb.eof_version != XFS_EOFBLOCKS_VERSION)
 			return -XFS_ERROR(EINVAL);
 

fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1622

Quoted 7 lines

fs/xfs/xfs_ioctl.c +7 -0

+		if (eofb.eof_flags & XFS_EOF_FLAGS_QUOTA) {
+			unsigned int type = xfs_quota_type(eofb.eof_q_type);
+			if ((type == XFS_DQ_USER && !XFS_IS_UQUOTA_ON(mp)) ||
+			    (type == XFS_DQ_GROUP && !XFS_IS_GQUOTA_ON(mp)) ||
+			    (type == XFS_DQ_PROJ && !XFS_IS_PQUOTA_ON(mp)))
+				return -XFS_ERROR(EINVAL);
+		}

Again, I don't really see the reason for needing xfs_quota_type() here, and whether the quota check is relevant at all...

Cheers,

Dave.

Quoted 8 lines · Dave Chinner

Quoted 3 lines · Brian Foster

Support quota ID filtering in the eofblocks scan. The caller must set the XFS_EOF_FLAGS_QUOTA flags bit, quota ID and quota type. The associated quota type must be enabled on the mounted filesystem.

I'm wondering if this even needs quota enabled to filter based on a uid, gid, or prid?

Not really...

Quoted 6 lines

The quota part of it seem irrelevant to the filtering that is being executed - we are only matching against the inode uid/gid/prid, not against dquots - and I can see situations where just being able to filter on a given uid/gid might be useful in a multi-user environment regardless of whether quotas are being used.

This is how my rfc version was implemented (with a comment to the same effect). I added the explicit checking in response to:

http://oss.sgi.com/archives/xfs/2012-09/msg00024.html

Quoted 3 lines

Indeed, even testing is made much easier if we don't have to juggle quota mount options to test the filtering....

I agree. I suppose the check can make a bit of sense if the current group/proj mode of the mount conflicts with the eofb request, but as you've outlined above, we're checking against the inode values so I don't think it introduces a functional problem.

Quoted 13 lines

Why do you need xfs_quota_type() here? why not just:

	switch (type) {
	case XQM_USRQUOTA:
		return ip->i_d.di_uid == eofb->eof_q_id;
	case XQM_GRPQUOTA:
		return ip->i_d.di_gid == eofb->eof_q_id;
	case XQM_PRJQUOTA:
		return xfs_get_projid(ip) == eofb->eof_q_id;
	default:
		break;
	}

Quoted 2 lines

	return 0;

Technically we don't really need xfs_quota_type(). I just thought it cleaner to use the same mapping we use throughout XFS (i.e., I don't see those definitions used elsewhere in XFS)...

Quoted 19 lines · Brian Foster

Quoted 8 lines · Dave Chinner

Quoted 3 lines · Brian Foster

Support quota ID filtering in the eofblocks scan. The caller must set the XFS_EOF_FLAGS_QUOTA flags bit, quota ID and quota type. The associated quota type must be enabled on the mounted filesystem.

I'm wondering if this even needs quota enabled to filter based on a uid, gid, or prid?

Not really...

Quoted 6 lines

The quota part of it seem irrelevant to the filtering that is being executed - we are only matching against the inode uid/gid/prid, not against dquots - and I can see situations where just being able to filter on a given uid/gid might be useful in a multi-user environment regardless of whether quotas are being used.

Quoted 4 lines

This is how my rfc version was implemented (with a comment to the same effect). I added the explicit checking in response to:

http://oss.sgi.com/archives/xfs/2012-09/msg00024.html

Ok, that was in the context of "we need to be specific about what the ID is". Now the question is "is quota ID the right ID to use?". Different question. ;)

Quoted 8 lines

Quoted 3 lines

Indeed, even testing is made much easier if we don't have to juggle quota mount options to test the filtering....

I agree. I suppose the check can make a bit of sense if the current group/proj mode of the mount conflicts with the eofb request, but as you've outlined above, we're checking against the inode values so I don't think it introduces a functional problem.

Right, and if we even get the separate project quota inode patches merged, then all three can be valid at once. Indeed, that makes me ask the question - should we be able to filter on multiple IDs in a single pass (e.g. walk Jim's files in the jasper project)?

Seeing as it is just a filter, enabling the above (even though it would be rarely used) might make sense. i.e. separate field for each id. It's not like we have to keep the size of the structure to a minimum...

Cheers,

Dave.

Quoted 13 lines

Quoted 13 lines

Why do you need xfs_quota_type() here? why not just:

	switch (type) {
	case XQM_USRQUOTA:
		return ip->i_d.di_uid == eofb->eof_q_id;
	case XQM_GRPQUOTA:
		return ip->i_d.di_gid == eofb->eof_q_id;
	case XQM_PRJQUOTA:
		return xfs_get_projid(ip) == eofb->eof_q_id;
	default:
		break;
	}

Quoted 6 lines

Quoted 2 lines

	return 0;

Technically we don't really need xfs_quota_type(). I just thought it cleaner to use the same mapping we use throughout XFS (i.e., I don't see those definitions used elsewhere in XFS)...

XQM_* are the userspace XFS quota interface definitions. They are what xfs_quota uses. They just so happen to be the same as the generic quota definitions except the generic quotas don't define project quota types (hence the need for mapping). See include/uapi/linux/dqblk_xfs.h.

I just figured that rather than mapping something unknown to something known, just using something known in the first place is much simpler...

Cheers,

Dave.

Quoted 20 lines · Dave Chinner

Quoted 19 lines · Brian Foster

Quoted 8 lines · Dave Chinner

Quoted 3 lines · Brian Foster

Support quota ID filtering in the eofblocks scan. The caller must set the XFS_EOF_FLAGS_QUOTA flags bit, quota ID and quota type. The associated quota type must be enabled on the mounted filesystem.

I'm wondering if this even needs quota enabled to filter based on a uid, gid, or prid?

Not really...

Quoted 6 lines

The quota part of it seem irrelevant to the filtering that is being executed - we are only matching against the inode uid/gid/prid, not against dquots - and I can see situations where just being able to filter on a given uid/gid might be useful in a multi-user environment regardless of whether quotas are being used.

Quoted 9 lines

Quoted 4 lines

This is how my rfc version was implemented (with a comment to the same effect). I added the explicit checking in response to:

http://oss.sgi.com/archives/xfs/2012-09/msg00024.html

Ok, that was in the context of "we need to be specific about what the ID is". Now the question is "is quota ID the right ID to use?". Different question. ;)

Fair enough. :) I just want to make sure we're on the same page. I'll take this as an Ok to remove the quota enabled checks. The resulting behavior will now be that quota ID filtering is supported regardless of whether quotas are enabled on the targeted mount.

Quoted 5 lines

Seeing as it is just a filter, enabling the above (even though it would be rarely used) might make sense. i.e. separate field for each id. It's not like we have to keep the size of the structure to a minimum...

... but that is also starting to further complicate the filtering and testing required. I'm also not quite sure how we would currently specify a union of id's in the flags given that we migrated towards the use of the real quota type field (eof_q_type) rather than my original approach of specifying the quota id type as a flag.

Would we be sufficiently prepared for the future if we broke the id into three fields, i.e.:

struct xfs_eofblocks {
	...
	__u32	eof_uquota_id;
	__u32	eof_gquota_id;
	__u32	eof_pquota_id;
	__u32	eof_q_type;
	...

... but the in kernel implementation remains as is by using the associated field based on the quota type? To be honest, even if we had a way to specify multiple id's, I think I would prefer to leave the implementation as is and do the enhanced filtering in future patches (e.g., get the data structure right, but EINVAL for now if multiple id's are flagged),

if for nothing else than the testing I've already done against this implementation. But I suspect if there isn't currently a standard way to specify multiple quota types, we're fated to have to update the structure with a new version anyways..?

Quoted 10 lines

XQM_* are the userspace XFS quota interface definitions. They are what xfs_quota uses. They just so happen to be the same as the generic quota definitions except the generic quotas don't define project quota types (hence the need for mapping). See include/uapi/linux/dqblk_xfs.h.

I just figured that rather than mapping something unknown to something known, just using something known in the first place is much simpler...

Right, at the time I think I dug through the various quota type definitions, saw that we provided this mapping function and got the impression we should be consistent throughout the filesystem. But I think your point here is that the expected input should be the definitions we already define for userspace whereas I was thinking the generic definitions were ubiquitous from userspace.

Quoted 3 lines · Brian Foster

Quoted 2 lines · Dave Chinner

Quoted 1 line · Brian Foster

On 10/22/2012 09:42 PM, Dave Chinner wrote:

Quoted 11 lines

Quoted 5 lines

Seeing as it is just a filter, enabling the above (even though it would be rarely used) might make sense. i.e. separate field for each id. It's not like we have to keep the size of the structure to a minimum...

... but that is also starting to further complicate the filtering and testing required. I'm also not quite sure how we would currently specify a union of id's in the flags given that we migrated towards the use of the real quota type field (eof_q_type) rather than my original approach of specifying the quota id type as a flag.

Well, quota type just goes away completely - it's irrelevant.

Quoted 10 lines

Would we be sufficiently prepared for the future if we broke the id into three fields, i.e.:

struct xfs_eofblocks {
	...
	__u32	eof_uquota_id;
	__u32	eof_gquota_id;
	__u32	eof_pquota_id;
	__u32	eof_q_type;
	...

I'd suggest uid, gid and prid as the types, dropping the quota type altogether. Then add specific flags to indicate each field is set rather than a flag to say "look at the q_type field for which id field to read"...

i.e. the API is indepedent of quota configurations and quota IDs, but is still capable of expressing such control when required.

Quoted 6 lines

... but the in kernel implementation remains as is by using the associated field based on the quota type? To be honest, even if we had a way to specify multiple id's, I think I would prefer to leave the implementation as is and do the enhanced filtering in future patches (e.g., get the data structure right, but EINVAL for now if multiple id's are flagged),

Sure, that's fine.

Quoted 4 lines

if for nothing else than the testing I've already done against this implementation. But I suspect if there isn't currently a standard way to specify multiple quota types, we're fated to have to update the structure with a new version anyways..?

There is a standard way of recording a *single* "quota ID" in kernel space now - see quota.h:

struct kqid { /* Type in which we store the quota identifier */

        union {
                kuid_t uid;
                kgid_t gid;
                kprojid_t projid;
        };
        enum quota_type type;  /* USRQUOTA (uid) or GRPQUOTA (gid) or PRJQUOTA (projid) */
}

and a bunch or wrappers for manipulating them. That's the sort of thing that we'd need to build internally for filtering so we end up with user namespace support. This is shiny and new, just merged in 3.7-rc1....

Quoted 11 lines

Quoted 10 lines

XQM_* are the userspace XFS quota interface definitions. They are what xfs_quota uses. They just so happen to be the same as the generic quota definitions except the generic quotas don't define project quota types (hence the need for mapping). See include/uapi/linux/dqblk_xfs.h.

I just figured that rather than mapping something unknown to something known, just using something known in the first place is much simpler...

Quoted 6 lines

Right, at the time I think I dug through the various quota type definitions, saw that we provided this mapping function and got the impression we should be consistent throughout the filesystem. But I think your point here is that the expected input should be the definitions we already define for userspace whereas I was thinking the generic definitions were ubiquitous from userspace.

Well, the generic definitions never defined project quota. If you look now in the 3.7 codebase you'll see that PRJQUOTA has been added to include/linux/quota.h, as has a kprojid_t type. Hence project quota IDs are now considered a first class quota citizen by the generic quota *kernel* code. However, include/uapi/linux/quota.h is completely unchanged, so the generic quota userspace interface still knows nothing about project IDs, which means the only actual user facing definition is still in include/uapi/linux/dqblks_xfs.h - XQM_PRJQUOTA. Hence we assume a quota ID that doesn't match anything the generic quota understands to be a project quota as that's the only way stuff works in a predictable, reliable manner...

The origin of this mess are historic as XFS brought along it's own quota implementation and API from Irix years ago because the linux quota subsystem was not robust or fully featured enough to even consider integration at the time (e.g. generic quotas didn't even support journalled quotas). So, yeah, a mess but one that is slowly getting cleaned up.

Cheers,

Dave.

Support minimum file size filtering in the eofblocks scan. The caller must set the XFS_EOF_FLAGS_MINFILESIZE flags bit and minimum file size value in bytes.

Signed-off-by Brian Foster <[email protected]>

Create a new mount workqueue and 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.

The scan interval is based on the new 'background_prealloc_discard_period' tunable (default to 5m). The

Hi all,

This is v3 of the speculative preallocation inode tracking patchset. This functionality tracks inodes with post-EOF speculative preallocation for the purpose of background and on-demand trimming.

Background scanning occurs on a longish interval (5 minutes by default) and in a best-effort mode (i.e., inodes are skipped due to lock contention or dirty cache). The intent is to clear up post-EOF blocks on inodes that might have allocations hanging around due to open-write-close sequences (NFS).

On demand scanning is provided via a new ioctl and supports various parameters such as scan mode, filtering by quota id and minimum file size. A pending use case for on demand scanning is for accurate quota accounting via the gluster scale out filesystem (i.e., to free up preallocated space when near a usage limit).

Brian

Quoted 25 lines · Brian Foster

Hi all,

This is v3 of the speculative preallocation inode tracking patchset. This functionality tracks inodes with post-EOF speculative preallocation for the
purpose of background and on-demand trimming.

Background scanning occurs on a longish interval (5 minutes by default) and in
a best-effort mode (i.e., inodes are skipped due to lock contention or dirty
cache). The intent is to clear up post-EOF blocks on inodes that might have
allocations hanging around due to open-write-close sequences (NFS).

On demand scanning is provided via a new ioctl and supports various parameters
such as scan mode, filtering by quota id and minimum file size. A pending use
case for on demand scanning is for accurate quota accounting via the gluster
scale out filesystem (i.e., to free up preallocated space when near a usage
limit).

Quoted 1 line

Brian

The series looks great.

I am just curious, what is the reason for the padding in the xfs_eofblocks structure?

Reviewed-by Mark Tinguely <[email protected]>

Quoted 26 lines · Mark Tinguely

Quoted 25 lines · Brian Foster

Hi all,

This is v3 of the speculative preallocation inode tracking patchset. This functionality tracks inodes with post-EOF speculative preallocation for the
purpose of background and on-demand trimming.

Background scanning occurs on a longish interval (5 minutes by default) and in
a best-effort mode (i.e., inodes are skipped due to lock contention or dirty
cache). The intent is to clear up post-EOF blocks on inodes that might have
allocations hanging around due to open-write-close sequences (NFS).

On demand scanning is provided via a new ioctl and supports various parameters
such as scan mode, filtering by quota id and minimum file size. A pending use
case for on demand scanning is for accurate quota accounting via the gluster
scale out filesystem (i.e., to free up preallocated space when near a usage
limit).

Quoted 4 lines

Quoted 1 line

Brian

The series looks great.

Hi Mark,

Thanks for the review.

Quoted 3 lines

I am just curious, what is the reason for the padding in the xfs_eofblocks structure?

I added the padding in response to review on an early revision of the set:

http://oss.sgi.com/archives/xfs/2012-09/msg00024.html

The purpose is to allow adding fields to the control structure down the road without breaking existing binaries.

Brian

Quoted 1 line

Reviewed-by Mark Tinguely<[email protected]>

Not at all a big deal, (16 used and 12 padding = ) 28 bytes was an odd number and it got my curiosity up. I suspected it was part a plan of yours and Pinky to take over the world. :)

Quoted 9 lines · Brian Foster

Quoted 3 lines · Mark Tinguely

I am just curious, what is the reason for the padding in the xfs_eofblocks structure?

I added the padding in response to review on an early revision of the set:

http://oss.sgi.com/archives/xfs/2012-09/msg00024.html

Quoted 2 lines

The purpose is to allow adding fields to the control structure down the road without breaking existing binaries.

Thank-you for the information.

I would think that changing the number of arguments would also involving changing the version number.

The kernel should know
that version 1 copies in 16 bytes, version 2 copies in 16+t bytes, version n copies in 16+n bytes...

This is v3 of the speculative preallocation inode tracking patchset.