Add ioctl FITRIM.

Hi,

The patch looks good. Just one small thing:

fs/ext3/balloc.c unsigned long ext3_bg_num_gdb(struct super_block *sb, int group) · line 1937

Quoted 5 lines · Lukas Czerner

fs/ext3/balloc.c +5 -0

+	gdp = ext3_get_group_desc(sb, group, &gdp_bh);
+	if (!gdp)
+		goto err_out;
+
+	BUFFER_TRACE(gd_bh, "get_write_access");

                     ^^^^^ gdp_bh

So you can add: Reviewed-by: Jan Kara <[email protected]>

Honza

fs/ext3/balloc.c +11 -0

+		/*
+		 * Clear bits in the bitmap
+		 */
+		for (bit = start; bit < next; bit++) {
+			BUFFER_TRACE(bitmap_bh, "clear bit");
+			if (!ext3_clear_bit_atomic(sb_bgl_lock(sbi, group),
+						bit, bitmap_bh->b_data)) {
+				ext3_error(sb, __func__,
+					"bit already cleared for block "E3FSBLK,
+					 (unsigned long)bit);
+				BUFFER_TRACE(bitmap_bh, "bit already cleared");

Quoted 11 lines · Lukas Czerner

fs/ext3/balloc.c +11 -0

+		/*
+		 * Clear bits in the bitmap
+		 */
+		for (bit = start; bit < next; bit++) {
+			BUFFER_TRACE(bitmap_bh, "clear bit");
+			if (!ext3_clear_bit_atomic(sb_bgl_lock(sbi, group),
+						bit, bitmap_bh->b_data)) {
+				ext3_error(sb, __func__,
+					"bit already cleared for block "E3FSBLK,
+					 (unsigned long)bit);
+				BUFFER_TRACE(bitmap_bh, "bit already cleared");

fs/ext3/balloc.c unsigned long ext3_bg_num_gdb(struct super_block *sb, int group) · line 2008

Quoted 10 lines

fs/ext3/balloc.c +10 -0

+			} else {
+				freed++;
+			}
+		}
+
+		/* Update couters */
+		spin_lock(sb_bgl_lock(sbi, group));
+		le16_add_cpu(&gdp->bg_free_blocks_count, freed);
+		spin_unlock(sb_bgl_lock(sbi, group));
+		percpu_counter_add(&sbi->s_freeblocks_counter, next - start);

It wouldn't be a terrible idea to put this code into a helper function that is shared with ext3_free_blocks_sb() so that it is kept in sync. There are a lot of places that need to be kept coordinated (disk bitmap, memory bitmap, group descriptor, percpu counter) so I'd like that to be localized to one part of the code.

Cheers, Andreas

Walk through each allocation group and trim all free extents. It can be invoked through TRIM ioctl on the file system. The main idea is to provide a way to trim the whole file system if needed, since some SSD's may suffer from performance loss after the whole device was filled (it does not mean that fs is full!).

It search fro free extents in each allocation group. When the free extent is found, blocks are marked as used in the buddy bitmap and then trimmed. Afterwards these blocks are marked as free in per-group buddy bitmap.

Since fstrim is a long operation it is good to have an ability to interrupt it by a signal. This was added by Dmitry Monakhov. Thanks Dimitry.

Signed-off-by Lukas Czerner <[email protected]>

Signed-off-by Dmitry Monakhov <[email protected]>

Adds an filesystem independent ioctl to allow implementation of file system batched discard support.

Signed-off-by Lukas Czerner <[email protected]>

---
 fs/ioctl.c         |   31 +++++++++++++++++++++++++++++++
 include/linux/fs.h |    2 ++
 2 files changed, 33 insertions(+), 0 deletions(-)

fs/ioctl.c +0 -0

index 2d140a7..6c01c3c 100644

fs/ioctl.c +30 -0

+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+	unsigned int minlen;
+	int err;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* If filesystem doesn't support trim feature, return. */
+	if (sb->s_op->trim_fs == NULL)
+		return -EOPNOTSUPP;
+
+	/* If a blockdevice-backed filesystem isn't specified, return EINVAL. */
+	if (sb->s_bdev == NULL)
+		return -EINVAL;
+
+	err = get_user(minlen, (unsigned int __user *) arg);
+	if (err)
+		return err;
+
+	err = sb->s_op->trim_fs(minlen, sb);
+	if (err)
+		return err;
+	return 0;
+}
+
 /*
  * When you add any new common ioctls to the switches above and below
  * please update compat_sys_ioctl() too.

@@ -590,6 +617,10 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 		error = ioctl_fsthaw(filp);
 		break;
 
+	case FITRIM:
+		error = ioctl_fstrim(filp, arg);
+		break;
+
 	case FS_IOC_FIEMAP:
 		return ioctl_fiemap(filp, arg);
 

include/linux/fs.h +2 -0

index 68ca1b0..01632e4 100644
@@ -315,6 +315,7 @@ struct inodes_stat_t {
 #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
 #define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
 #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
+#define FITRIM		_IOWR('X', 121, int)	/* Trim */
 
 #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
 #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)

@@ -1580,6 +1581,7 @@ struct super_operations {
 	ssize_t (*quota_write)(struct super_block *, int, const char *, size_t, loff_t);
 #endif
 	int (*bdev_try_to_free_page)(struct super_block*, struct page*, gfp_t);
+	int (*trim_fs) (unsigned int, struct super_block *);
 };
 
 /*

Quoted 17 lines · Lukas Czerner

Adds an filesystem independent ioctl to allow implementation of file system batched discard support.

Signed-off-by Lukas Czerner <[email protected]>

---
 fs/ioctl.c         |   31 +++++++++++++++++++++++++++++++
 include/linux/fs.h |    2 ++
 2 files changed, 33 insertions(+), 0 deletions(-)

fs/ioctl.c +0 -0

index 2d140a7..6c01c3c 100644
@@ -540,6 +540,33 @@ static int ioctl_fsthaw(struct file *filp)
 	return thaw_super(sb);
 }
 

fs/ioctl.c static int ioctl_fsthaw(struct file *filp) · line 543

Quoted 1 line

fs/ioctl.c +1 -0

+static int ioctl_fstrim(struct file *filp, unsigned long arg)

BTW why do we have to trim fs in one shot ?
IMHO it is much suitable to provide start,len parameters as we do in most functions(truncate, bdevdiscard, getdents).
It allow userspace caller to implement a fancy looking progress bars.

Quoted 53 lines

fs/ioctl.c +30 -0

+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+	unsigned int minlen;
+	int err;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* If filesystem doesn't support trim feature, return. */
+	if (sb->s_op->trim_fs == NULL)
+		return -EOPNOTSUPP;
+
+	/* If a blockdevice-backed filesystem isn't specified, return EINVAL. */
+	if (sb->s_bdev == NULL)
+		return -EINVAL;
+
+	err = get_user(minlen, (unsigned int __user *) arg);
+	if (err)
+		return err;
+
+	err = sb->s_op->trim_fs(minlen, sb);
+	if (err)
+		return err;
+	return 0;
+}
+
 /*
  * When you add any new common ioctls to the switches above and below
  * please update compat_sys_ioctl() too.

@@ -590,6 +617,10 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 		error = ioctl_fsthaw(filp);
 		break;
 
+	case FITRIM:
+		error = ioctl_fstrim(filp, arg);
+		break;
+
 	case FS_IOC_FIEMAP:
 		return ioctl_fiemap(filp, arg);
 

include/linux/fs.h +1 -0

index 68ca1b0..01632e4 100644
@@ -315,6 +315,7 @@ struct inodes_stat_t {
 #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
 #define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
 #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
+#define FITRIM		_IOWR('X', 121, int)	/* Trim */
 
 #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
 #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)

include/linux/fs.h struct super_operations { · line 1587

Quoted 7 lines

include/linux/fs.h +1 -0

@@ -1580,6 +1581,7 @@ struct super_operations {
 	ssize_t (*quota_write)(struct super_block *, int, const char *, size_t, loff_t);
 #endif
 	int (*bdev_try_to_free_page)(struct super_block*, struct page*, gfp_t);
+	int (*trim_fs) (unsigned int, struct super_block *);
 };
 
 /*

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 19 lines · Dmitry Monakhov

Quoted 17 lines · Lukas Czerner

Adds an filesystem independent ioctl to allow implementation of file system batched discard support.

Signed-off-by Lukas Czerner <[email protected]>

---
 fs/ioctl.c         |   31 +++++++++++++++++++++++++++++++ include/linux/fs.h |    2 ++

 2 files changed, 33 insertions(+), 0 deletions(-)

fs/ioctl.c +0 -0

index 2d140a7..6c01c3c 100644
@@ -540,6 +540,33 @@ static int ioctl_fsthaw(struct file *filp)
     return thaw_super(sb);
  }
 

Quoted 5 lines

fs/ioctl.c static int ioctl_fsthaw(struct file *filp) · line 543

Quoted 1 line

fs/ioctl.c +1 -0

+static int ioctl_fstrim(struct file *filp, unsigned long arg)

BTW why do we have to trim fs in one shot ?
IMHO it is much suitable to provide start,len parameters as we do in most functions(truncate, bdevdiscard, getdents).
It allow userspace caller to implement a fancy looking progress bars.

Hi,

do you think it is really needed when even with todays SSD's it takes just a couple of seconds ? And I suppose it will improve in future. But generally I think we can do that..I would like to hear some more opinions before I start looking at this.

Thanks.

-Lukas.

Since the proposed patch is not aggregating discards into multiple ranges per ATA command, I thought some of the non-optimized devices would take minutes / hours?

If true, a way to control the progress from userspace is important.

If in general it is only going to take a few seconds for a full FITRIM to run, it is much less important, but I suppose the the RT project might find even that problematic.

Quoted 10 lines · Greg Freemyer

Since the proposed patch is not aggregating discards into multiple ranges per ATA command, I thought some of the non-optimized devices would take minutes / hours?

If true, a way to control the progress from userspace is important.

If in general it is only going to take a few seconds for a full FITRIM to run, it is much less important, but I suppose the the RT project might find even that problematic.

Even if it without the RT project, if disk activity is slowed or completely stopped for a few seconds, I can think of plenty of workloads where this would be totally unacceptable. Suppose you are running a web site; it doesn't really matter whether it is at Google, Facebook, Twitter, etc. If this means that one or more web pages get stalled by "a few seconds" while the FITRIM is going on, this is generally not considered acceptable. Even if it slows down the server by 30-50%, for some sites this would also be quite unacceptable.

This is a hard problem to solve, though, especially if there is an insistence to solve it in a fs-independent fashion. I could imagine doing this at work, by doing things one block group at a time, and then I could measure, for our specific hardware, how badly disk performance would get hit, and for how long, and then the userspace daemon could control how many block groups to do per unit time. But this would be of necessity ext2/3/4 specific....

So I'm not sure what to suggest here. Maybe the answer is we can have a fs-independent ioctl for desktop workloads, and one which gives more fine-grained control for those who need it? That seems ugly, but it might be the best compromise.

- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Greg Freemyer

If true, a way to control the progress from userspace is important.

If in general it is only going to take a few seconds for a full FITRIM to run, it is much less important, but I suppose the the RT project might find even that problematic.

Even if it without the RT project, if disk activity is slowed or completely stopped for a few seconds, I can think of plenty of workloads where this would be totally unacceptable. Suppose you are running a web site; it doesn't really matter whether it is at Google, Facebook, Twitter, etc. If this means that one or more web pages get stalled by "a few seconds" while the FITRIM is going on, this is generally not considered acceptable. Even if it slows down the server by 30-50%, for some sites this would also be quite unacceptable.

This is a hard problem to solve, though, especially if there is an insistence to solve it in a fs-independent fashion. I could imagine doing this at work, by doing things one block group at a time, and then I could measure, for our specific hardware, how badly disk performance would get hit, and for how long, and then the userspace daemon could control how many block groups to do per unit time. But this would be of necessity ext2/3/4 specific....

So I'm not sure what to suggest here. Maybe the answer is we can have a fs-independent ioctl for desktop workloads, and one which gives more fine-grained control for those who need it? That seems ugly, but it might be the best compromise.

Quoted 19 lines · Dmitry Monakhov

Quoted 17 lines · Lukas Czerner

Adds an filesystem independent ioctl to allow implementation of file system batched discard support.

Signed-off-by Lukas Czerner <[email protected]>

---
 fs/ioctl.c         |   31 +++++++++++++++++++++++++++++++
 include/linux/fs.h |    2 ++
 2 files changed, 33 insertions(+), 0 deletions(-)

fs/ioctl.c +0 -0

index 2d140a7..6c01c3c 100644
@@ -540,6 +540,33 @@ static int ioctl_fsthaw(struct file *filp)
 	return thaw_super(sb);
 }
 

Quoted 5 lines

fs/ioctl.c static int ioctl_fsthaw(struct file *filp) · line 543

Quoted 1 line

fs/ioctl.c +1 -0

+static int ioctl_fstrim(struct file *filp, unsigned long arg)

BTW why do we have to trim fs in one shot ?
IMHO it is much suitable to provide start,len parameters as we do in most functions(truncate, bdevdiscard, getdents).
It allow userspace caller to implement a fancy looking progress bars.

Hi,

do you think it is really needed when even with todays SSD's it takes just a couple of seconds ? And I suppose it will improve in future. But generally I think we can do that..I would like to hear some more opinions before I start looking at this.

Thanks.

-Lukas.

Quoted 59 lines

Quoted 59 lines

fs/ioctl.c +30 -0

+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+	unsigned int minlen;
+	int err;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* If filesystem doesn't support trim feature, return. */
+	if (sb->s_op->trim_fs == NULL)
+		return -EOPNOTSUPP;
+
+	/* If a blockdevice-backed filesystem isn't specified, return EINVAL. */
+	if (sb->s_bdev == NULL)
+		return -EINVAL;
+
+	err = get_user(minlen, (unsigned int __user *) arg);
+	if (err)
+		return err;
+
+	err = sb->s_op->trim_fs(minlen, sb);
+	if (err)
+		return err;
+	return 0;
+}
+
 /*
  * When you add any new common ioctls to the switches above and below
  * please update compat_sys_ioctl() too.

@@ -590,6 +617,10 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 		error = ioctl_fsthaw(filp);
 		break;
 
+	case FITRIM:
+		error = ioctl_fstrim(filp, arg);
+		break;
+
 	case FS_IOC_FIEMAP:
 		return ioctl_fiemap(filp, arg);
 

include/linux/fs.h +2 -0

index 68ca1b0..01632e4 100644
@@ -315,6 +315,7 @@ struct inodes_stat_t {
 #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
 #define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
 #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
+#define FITRIM		_IOWR('X', 121, int)	/* Trim */
 
 #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
 #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)

@@ -1580,6 +1581,7 @@ struct super_operations {
 	ssize_t (*quota_write)(struct super_block *, int, const char *, size_t, loff_t);
 #endif
 	int (*bdev_try_to_free_page)(struct super_block*, struct page*, gfp_t);
+	int (*trim_fs) (unsigned int, struct super_block *);
 };
 

Quoted 6 lines

include/linux/fs.h struct super_operations { · line 1587

Quoted 1 line

include/linux/fs.h +0 -0

 /*

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 27 lines · Lukas Czerner

Quoted 19 lines · Dmitry Monakhov

Quoted 17 lines · Lukas Czerner

Adds an filesystem independent ioctl to allow implementation of file system batched discard support.

Signed-off-by Lukas Czerner <[email protected]>

---
 fs/ioctl.c         |   31 +++++++++++++++++++++++++++++++
 include/linux/fs.h |    2 ++
 2 files changed, 33 insertions(+), 0 deletions(-)

fs/ioctl.c +0 -0

index 2d140a7..6c01c3c 100644
@@ -540,6 +540,33 @@ static int ioctl_fsthaw(struct file *filp)
 	return thaw_super(sb);
 }
 

Quoted 5 lines

fs/ioctl.c static int ioctl_fsthaw(struct file *filp) · line 543

Quoted 1 line

fs/ioctl.c +1 -0

+static int ioctl_fstrim(struct file *filp, unsigned long arg)

BTW why do we have to trim fs in one shot ?
IMHO it is much suitable to provide start,len parameters as we do in most functions(truncate, bdevdiscard, getdents).
It allow userspace caller to implement a fancy looking progress bars.

Quoted 6 lines

Hi,

do you think it is really needed when even with todays SSD's it takes just a couple of seconds ? And I suppose it will improve in future. But generally I think we can do that..I would like to hear some more opinions before I start looking at this.

Hi, Lukas
we may face a really long delays due to bad layouts and slow devices Please read my response to Ted
I'm agree with you what this interface is important, BTW i already enabled FITRIM support on my notebook, my speed difference is about 2-3%. But let's provide right user interface from very beginning.