Skip to contentSkip to conversation

ext4: Add EXT4_IOC_DEFRAG ioctl and basic defrag functions

RFC PATCH In review Akira Fujita 2 participants 3 messages Jan 30, 2009 – Feb 4, 2009

Subsystems (2)
VFS ext4 file system
lore
Share & tools
lore

Akira Fujita

author
#0 lore raw

RFC PATCH 1/3 ext4: Add EXT4_IOC_DEFRAG ioctl and basic defrag functions

ext4: online defrag -- Add EXT4_IOC_DEFRAG ioctl and basic defrag functions.

From: Akira Fujita <[email protected]>

The EXT4_IOC_DEFRAG exchanges the blocks between org_fd and dest_fd, then write the file data of org_fd to dest_fd. ext4_defrag() is the main fucntion of ext4 online defrag, this patch includes it and defrag basic functions.

Signed-off-by Akira Fujita <[email protected]>
Signed-off-by Takashi Sato <[email protected]>
Signed-off-by Kazuya Mio <[email protected]>
---
 fs/ext4/Makefile       |   13 ++
 fs/ext4/defrag.c       |  477 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/ext4.h         |   18 ++-
 fs/ext4/ext4_extents.h |    4 +
 fs/ext4/extents.c      |    4 +-
 fs/ext4/ioctl.c        |   44 ++++-
 6 files changed, 544 insertions(+), 16 deletions(-)
fs/ext4/Makefile +1 -1
index a8ff003..e7fd8ba 100644
@@ -6,7 +6,7 @@ obj-$(CONFIG_EXT4_FS) += ext4.o
 
 ext4-y	:= balloc.o bitmap.o dir.o file.o fsync.o ialloc.o inode.o \
 		   ioctl.o namei.o super.o symlink.o hash.o resize.o extents.o \
-		   ext4_jbd2.o migrate.o mballoc.o
+		   ext4_jbd2.o migrate.o mballoc.o defrag.o
 
 ext4-$(CONFIG_EXT4_FS_XATTR)		+= xattr.o xattr_user.o xattr_trusted.o
 ext4-$(CONFIG_EXT4_FS_POSIX_ACL)	+= acl.o
fs/ext4/defrag.c new file +477 -0

477 lines not on this page · raw message

fs/ext4/ext4.h +12 -0
index a1c5caa..35071f4 100644
@@ -293,6 +293,7 @@ struct ext4_new_group_data {
 #define EXT4_IOC_GROUP_ADD		_IOW('f', 8, struct ext4_new_group_input)
 #define EXT4_IOC_MIGRATE		_IO('f', 9)
  /* note ioctl 11 reserved for filesystem-independent FIEMAP ioctl */
+#define EXT4_IOC_DEFRAG                 _IOW('f', 15, struct move_extent)
 
 /*
  * ioctl commands in 32 bit emulation

@@ -313,6 +314,13 @@ struct ext4_new_group_data {
 
 #define EXT4_IOC_DEBUG_DELALLOC		_IO('f', 42)
 
+struct move_extent {
+	int org_fd;		/* original file descriptor */
+	int dest_fd;		/* destination file descriptor */
+	ext4_lblk_t start;	/* logical offset of org_fd and dest_fd*/
+	ext4_lblk_t len;	/* exchange block length */
+};
+
 /*
  *  Mount options
  */

@@ -1016,6 +1024,10 @@ extern struct ext4_group_desc * ext4_get_group_desc(struct super_block * sb,
 						    struct buffer_head ** bh);
 extern int ext4_should_retry_alloc(struct super_block *sb, int *retries);
 
+/* defrag.c */
+extern int ext4_defrag(struct file *o_filp, struct file *d_filp,
+			ext4_lblk_t start, ext4_lblk_t len);
+
 /* dir.c */
 extern int ext4_check_dir_entry(const char *, struct inode *,
 				struct ext4_dir_entry_2 *,
fs/ext4/ext4_extents.h +4 -0
index 18cb67b..49a4614 100644
@@ -221,12 +221,16 @@ static inline int ext4_ext_get_actual_len(struct ext4_extent *ext)
 }
 
 extern int ext4_ext_calc_metadata_amount(struct inode *inode, int blocks);
+extern ext4_fsblk_t ext_pblock(struct ext4_extent *ex);
 extern ext4_fsblk_t idx_pblock(struct ext4_extent_idx *);
 extern void ext4_ext_store_pblock(struct ext4_extent *, ext4_fsblk_t);
 extern int ext4_extent_tree_init(handle_t *, struct inode *);
 extern int ext4_ext_calc_credits_for_single_extent(struct inode *inode,
 						   int num,
 						   struct ext4_ext_path *path);
+extern int ext4_can_extents_be_merged(struct inode *inode,
+				      struct ext4_extent *ex1,
+				      struct ext4_extent *ex2);
 extern int ext4_ext_try_to_merge(struct inode *inode,
 				 struct ext4_ext_path *path,
 				 struct ext4_extent *);
fs/ext4/extents.c +2 -2
index 54bf062..a64c706 100644
@@ -49,7 +49,7 @@
  * ext_pblock:
  * combine low and high parts of physical block number into ext4_fsblk_t
  */
-static ext4_fsblk_t ext_pblock(struct ext4_extent *ex)
+ext4_fsblk_t ext_pblock(struct ext4_extent *ex)
 {
 	ext4_fsblk_t block;
 

@@ -1328,7 +1328,7 @@ static int ext4_ext_correct_indexes(handle_t *handle, struct inode *inode,
 	return err;
 }
 
-static int
+int
 ext4_can_extents_be_merged(struct inode *inode, struct ext4_extent *ex1,
 				struct ext4_extent *ex2)
 {
fs/ext4/ioctl.c +33 -0
index 353d949..e1dde89 100644
@@ -14,6 +14,7 @@
 #include <linux/compat.h>
 #include <linux/smp_lock.h>
 #include <linux/mount.h>
+#include <linux/file.h>
 #include <asm/uaccess.h>
 #include "ext4_jbd2.h"
 #include "ext4.h"

@@ -225,6 +226,38 @@ setversion_out:
 
 		return err;
 	}
+
+	case EXT4_IOC_DEFRAG: {
+		struct move_extent me;
+		struct file *d_filp;
+		int fput_needed;
+		int err;
+
+		if (copy_from_user(&me,
+			(struct move_extent __user *)arg, sizeof(me)))
+			return -EFAULT;
+
+
+		d_filp = fget_light(me.dest_fd, &fput_needed);
+		if (!d_filp)
+			return -EBADF;
+
+		if (!capable(CAP_DAC_OVERRIDE)) {
+			if ((current->real_cred->fsuid != inode->i_uid) ||
+			    !(inode->i_mode & S_IRUSR) ||
+			    !(d_filp->f_dentry->d_inode->i_mode & S_IRUSR)) {
+				fput_light(d_filp, fput_needed);
+				return -EACCES;
+			}
+		}
+
+		err = ext4_defrag(filp, d_filp, me.start, me.len);
+
+		fput_light(d_filp, fput_needed);
+
+		return err;
+	}
+
 	case EXT4_IOC_GROUP_ADD: {
 		struct ext4_new_group_data input;
 		struct super_block *sb = inode->i_sb;
maintainer
#1 lore raw
Quoted 3 lines · Akira Fujita

ext4: online defrag -- Add EXT4_IOC_DEFRAG ioctl and basic defrag functions.

From: Akira Fujita <[email protected]>

Thanks, this patch is much better. Eventually I suspect you'll want to add an ioctl to control the allocation of blocks to the donor inode, but that can certainly come later.

I would combine the two kernel patches into one. There's no point separating them into separate patches; the maximum message size through the vger mailing lists is something like 100k, and the two patches combined are under 43k. The first patch won't function very well with two key functions stubbed out with "return 0;", anyway....

Also, I note that you are moving pages from the donor inode to the target inode one page at a time. It would be faster and involve less journal activity to move an extent at a time; after all, the donor fd should be no more fragmented than the source fd, or the userspace program shoudl have noticed and not attempted to call the defrag ioctl. Therefore, nearly all of the time, it should be possible to move a contiguous range of blocks at a time from the donor to the source without increasing the number of extents in the target inode's leaf node.

You may also want to consider whether it is truly worth it to swap the block assignments between the donor and the target inode, and whether it might be easier simply to release the blocks that had previously belonged to the target inode, and simply decrement the length of the extent and/or delete the extent from the donor inode altogether. That removes the need to split any leaf blocks in the donor inode's extent tree, since it is guaranteed to always stay the same size or become smaller.

fs/ext4/defrag.c line 124

Quoted 6 lines
fs/ext4/defrag.c new file +6 -0
+{
+	if (!S_ISREG(org_inode->i_mode) || !S_ISREG(dest_inode->i_mode)) {
+		printk(KERN_ERR "ext4 defrag: "
+			"The argument files should be regular file\n");
+		return -EINVAL;
+	}

Random printks is probably a bad idea; this should only be done if debugging is enabled, and if the goal is debugging, it would be more useful if the code printed more information that might be useful to someone trying to debug the userspaceprogram (such as the inode numbers passed to the ioctl, in the above example).

fs/ext4/defrag.c line 176

Quoted 6 lines
fs/ext4/defrag.c new file +6 -0
+		if (start + len > i_size_read(org_inode)) {
+			printk(KERN_ERR "ext4 defrag: Adjust defrag length "
+				" because it[%u] is larger than original "
+				"file size\n", len);
+			len = i_size_read(org_inode) - start;
+		}

The function ext4_defrag_check_arguments() is called before any locking is done. So you could potentially have a race where target inode gets truncated by another process between when the userspace program determined the parameters to the defrag ioctl and when the defrag ioctl starts executing.

fs/ext4/defrag.c line 192

Quoted 11 lines
fs/ext4/defrag.c new file +11 -0
+/**
+ * ext4_defrag_lock_two - acquire two inodes' lock
+ *
+ * @first:		inode structure to be locked first
+ * @second:		inode structure to be locked second
+ *
+ * Lock mutex and semaphore of the two inodes (@first and @second). First
+ * @first is locked and then @second is locked. Note that we *must* set
+ * arguments in order of small address.
+ */
+static void ext4_defrag_lock_two(struct inode *first, struct inode *second)

This function only gets called once, so it doesn't matter that much, but you can reduce the stack space in the caller (ext4_defrag()) by doing the comparison in the function, and not saving the result. i.e., change this:

+     /* Lock the lower address inode first */
+     if (ip > tip) {
+     	 ip = dest_inode;

+ tip = org_inode;
+ }
+ ext4_defrag_lock_two(ip, tip);

to this:

      ext4_defrag_lock_two(org_inode, dest_inode);

Note that you don't need to save ip and tip on the stack, because it is the _locking_ order which matters in order to prevent deadlocks. The order in which the two inodes are unlocked doesn't matter. So you can drop the ip and tip variables from ext4_defrag(), and just arrange to use a stable locking order in ext4_defrag_lock_two(). Personally, I'd prefer to use the inode number (target_inode->i_ino and donor_inode->i_ino) to provide the stable locking order, but using the address does work. The C language doesn't guarantee you can compare pointers that don't come from the same array, but it's not like the Linux kernel is likely to be ported to a architecture such as the Honeywell DPS8 (which ran Multics :-).

- Ted

maintainer
#2 lore raw
In reply to Akira Fujita
Quoted 6 lines · Akira Fujita

+ *
+ * Note that the extents of target file(@o_filp) are always converted
+ * from "uninitialized" to "initialized" after swapping the blocks
+ * between the original file and the destination one(@d_filp).
+ * In the next version, this behavior will be fixed not to change extents'
+ * initializing status.

Note also that if the extent in the target file is "uninitialized", it's not necessary to force the page into memory and then force it to be written out again. This is important if the file involved is a large database file which still has large areas preallocated, but not yet initialized.

- Ted