Skip to contentSkip to conversation

fat: Batched discard support for fat

PATCH Superseded Kyungmin Park 2 participants 5 messages Feb 17, 2011 – Feb 18, 2011

Subsystems (2)
VFS VFAT/FAT/msdos filesystem

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Kyungmin Park

author
#0 lore raw

PATCH 1/1 fat: Batched discard support for fat

From: Kyungmin Park <[email protected]>

FAT supports batched discard as ext4.

Cited from Lukas words.
"The current solution is not ideal because of its bad performance impact. So basic idea to improve things is to avoid discarding every time some blocks are freed. and instead batching is together into bigger trims, which tends to be more effective."

You can find an information in detail at following URLs.
http://lwn.net/Articles/397538/
http://lwn.net/Articles/383933/

Signed-off-by Kyungmin Park <[email protected]>

---

fs/fat/fat.h +1 -0
index f504089..08b53e1 100644
@@ -299,6 +299,7 @@ extern int fat_alloc_clusters(struct inode *inode, int *cluster,
 			      int nr_cluster);
 extern int fat_free_clusters(struct inode *inode, int cluster);
 extern int fat_count_free_clusters(struct super_block *sb);
+extern int fat_trim_fs(struct super_block *sb, struct fstrim_range *range);
 
 /* fat/file.c */
 extern long fat_generic_ioctl(struct file *filp, unsigned int cmd,
fs/fat/fatent.c +83 -5

109 lines not on this page · raw message

fs/fat/file.c +24 -0
index 7257752..bfdd558 100644
@@ -125,6 +125,30 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return fat_ioctl_get_attributes(inode, user_attr);
 	case FAT_IOCTL_SET_ATTRIBUTES:
 		return fat_ioctl_set_attributes(filp, user_attr);
+	case FITRIM:
+	{
+		struct super_block *sb = inode->i_sb;
+		struct fstrim_range range;
+		int ret = 0;
+
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+
+		if (copy_from_user(&range, (struct fstrim_range *)arg,
+					sizeof(range)))
+			return -EFAULT;
+
+		ret = fat_trim_fs(sb, &range);
+		if (ret < 0)
+			return ret;
+
+		if (copy_to_user((struct fstrim_range *)arg, &range,
+					sizeof(range)))
+			return -EFAULT;
+
+		return 0;
+	}
+
 	default:
 		return -ENOTTY;	/* Inappropriate ioctl for device */
 	}
#1 lore raw
Quoted 10 lines · Kyungmin Park

From: Kyungmin Park <[email protected]>

FAT supports batched discard as ext4.

Cited from Lukas words.
"The current solution is not ideal because of its bad performance impact. So basic idea to improve things is to avoid discarding every time some blocks are freed. and instead batching is together into bigger trims, which tends to be more effective."

Quoted 3 lines

You can find an information in detail at following URLs.
http://lwn.net/Articles/397538/
http://lwn.net/Articles/383933/

Hi Kyungmin,

this is really great to see more and more filesystemS ADDING this. I can not really comment on fat specific code, but anyway I have couple of comments bellow.

Thanks!
-Lukas

Quoted 30 lines
Signed-off-by Kyungmin Park <[email protected]>

---

fs/fat/fat.h +1 -0
index f504089..08b53e1 100644
@@ -299,6 +299,7 @@ extern int fat_alloc_clusters(struct inode *inode, int *cluster,
 			      int nr_cluster);
 extern int fat_free_clusters(struct inode *inode, int cluster);
 extern int fat_count_free_clusters(struct super_block *sb);
+extern int fat_trim_fs(struct super_block *sb, struct fstrim_range *range);
 
 /* fat/file.c */
 extern long fat_generic_ioctl(struct file *filp, unsigned int cmd,
fs/fat/fatent.c +2 -0
index b47d2c9..777094b 100644
@@ -1,6 +1,8 @@
 /*
  * Copyright (C) 2004, OGAWA Hirofumi
  * Released under GPL v2.
+ *
+ * Batched discard support by Kyungmin Park <[email protected]>
  */
 
 #include <linux/module.h>

@@ -541,6 +543,16 @@ out:
 	return err;

fs/fat/fatent.c out: · line 553

Quoted 10 lines
fs/fat/fatent.c +8 -0
 }
 
+static int fat_issue_discard(struct super_block *sb, int cluster, int nr_clus)
+{
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	sector_t block, nr_blocks;
+
+        block = fat_clus_to_blknr(sbi, cluster);
+        nr_blocks = nr_clus * sbi->sec_per_clus;
+        return sb_issue_discard(sb, block, nr_blocks, GFP_NOFS, 0);

Use tabs for code indent.

Quoted 24 lines
fs/fat/fatent.c +5 -5
+}
+
 int fat_free_clusters(struct inode *inode, int cluster)
 {
 	struct super_block *sb = inode->i_sb;

@@ -575,11 +587,7 @@ int fat_free_clusters(struct inode *inode, int cluster)
 			if (cluster != fatent.entry + 1) {
 				int nr_clus = fatent.entry - first_cl + 1;
 
-				sb_issue_discard(sb,
-					fat_clus_to_blknr(sbi, first_cl),
-					nr_clus * sbi->sec_per_clus,
-					GFP_NOFS, 0);
-
+				fat_issue_discard(sb, first_cl, nr_clus);
 				first_cl = cluster;
 			}
 		}

@@ -683,3 +691,73 @@ out:
 	unlock_fat(sbi);
 	return err;
 }
+
+int fat_trim_fs(struct super_block *sb, struct fstrim_range *range)

fs/fat/fatent.c out: · line 705

Quoted 10 lines
fs/fat/fatent.c +10 -0
+{
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	struct fatent_operations *ops = sbi->fatent_ops;
+	struct fat_entry fatent;
+	unsigned long reada_blocks, reada_mask, cur_block;
+	int err = 0, free;
+	unsigned int start, len, minlen, trimmed;
+	int entry = 0;
+
+	start = range->start >> sb->s_blocksize_bits;

Start is not used anywhere either. Let me just explain the reason for having start, and len.

start - address the first Byte of the filesystem (or better, the device,
or part of the device underneath the filesystem itself). You are supposed to use this as a starting point for doing discard.

len - defines the length in Bytes of the filesystem (or better, the
device, or part of the device underneath the filesystem itself), user want to discard from the "start".

This two values gives us more flexibility in the way we are discarding the filesystem's free blocks. For example, if you have huge filesystem, or your device has bad discard performance, you probably do not want to do the FITRIM all-at-once, but rather per-partes, to not disturb other ongoing IO too much. Basically it allows you to spread the load through longer period of time.

Also, for bigger filesystems we might want to inform user that something is really happening in the form of progress bar (or whatever), which you can not do otherwise.

fs/fat/fatent.c out: · line 706

Quoted 1 line
fs/fat/fatent.c +1 -0
+	len = range->len >> sb->s_blocksize_bits;

"len" does not seem to be used anywhere, does it mean that you are discarding free extents on the whole filesystem with one run ? This operation can take pretty long time on devices with slow discard capability. Can you consider doing it per-partes as we are doing in ext4 for example ? I really can't say how much work does it mean in fat, but it might be worth it. Especially because of ... ->

fs/fat/fatent.c out: · line 712

Quoted 6 lines
fs/fat/fatent.c +6 -0
+	minlen = range->minlen >> sb->s_blocksize_bits;
+	trimmed = 0;
+
+	minlen = minlen / sbi->sec_per_clus;
+
+	lock_fat(sbi);

	^^^^^^^^^^^^^^
	-> this

You are holding this mutex the whole time you are discarding free extents and I think it would be devastating, because the users will see long stalls. The bigger the filesystem is and the worse discard performance of the device is, the longer stall will be. Please correct me I am mistaken. Have you done any testing ?

Quoted 57 lines
fs/fat/fatent.c +51 -0
+	if (sbi->free_clusters != -1 && sbi->free_clus_valid)
+		goto out;
+
+	reada_blocks = FAT_READA_SIZE >> sb->s_blocksize_bits;
+	reada_mask = reada_blocks - 1;
+	cur_block = 0;
+
+	free = 0;
+	fatent_init(&fatent);
+
+	/*
+	 * REVISIT: scan from the last free block.
+	 */
+	fatent_set_entry(&fatent, FAT_START_ENT);
+	while (fatent.entry < sbi->max_cluster) {
+		/* readahead of fat blocks */
+		if ((cur_block & reada_mask) == 0) {
+			unsigned long rest = sbi->fat_length - cur_block;
+			fat_ent_reada(sb, &fatent, min(reada_blocks, rest));
+		}
+		cur_block++;
+
+		err = fat_ent_read_block(sb, &fatent);
+		if (err)
+			goto out;
+
+		do {
+			if (ops->ent_get(&fatent) == FAT_ENT_FREE) {
+				free++;
+				if (!entry)
+					entry = fatent.entry;
+			} else if (entry) {
+				if (free >= minlen) {
+					fat_issue_discard(sb, entry, free);
+					trimmed += free;
+				}
+				free = 0;
+				entry = 0;
+			}
+		} while (fat_ent_next(sbi, &fatent));
+	}
+	if (free >= minlen) {
+		fat_issue_discard(sb, entry, free);
+		trimmed += free;
+	}
+	range->len = (trimmed * sbi->sec_per_clus) * sb->s_blocksize;
+	fatent_brelse(&fatent);
+out:
+	unlock_fat(sbi);
+	return err;
+}
fs/fat/file.c +0 -0
index 7257752..bfdd558 100644
@@ -125,6 +125,30 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return fat_ioctl_get_attributes(inode, user_attr);

fs/fat/file.c long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 135

Quoted 10 lines
fs/fat/file.c +8 -0
 	case FAT_IOCTL_SET_ATTRIBUTES:
 		return fat_ioctl_set_attributes(filp, user_attr);
+	case FITRIM:
+	{
+		struct super_block *sb = inode->i_sb;
+		struct fstrim_range range;
+		int ret = 0;
+
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;

You might want to add check whether the device actually support discard.
See: http://www.spinics.net/lists/linux-ext4/msg23144.html

Quoted 20 lines
fs/fat/file.c +16 -0
+
+		if (copy_from_user(&range, (struct fstrim_range *)arg,
+					sizeof(range)))
+			return -EFAULT;
+
+		ret = fat_trim_fs(sb, &range);
+		if (ret < 0)
+			return ret;
+
+		if (copy_to_user((struct fstrim_range *)arg, &range,
+					sizeof(range)))
+			return -EFAULT;
+
+		return 0;
+	}
+
 	default:
 		return -ENOTTY;	/* Inappropriate ioctl for device */
 	}
author
#2 lore raw
Quoted 106 lines · Lukas Czerner
Quoted 10 lines · Kyungmin Park

From: Kyungmin Park <[email protected]>

FAT supports batched discard as ext4.

Cited from Lukas words.
"The current solution is not ideal because of its bad performance impact. So basic idea to improve things is to avoid discarding every time some blocks are freed. and instead batching is together into bigger trims, which tends to be more effective."

Quoted 3 lines

You can find an information in detail at following URLs.
http://lwn.net/Articles/397538/
http://lwn.net/Articles/383933/

Hi Kyungmin,

this is really great to see more and more filesystemS ADDING this. I can not really comment on fat specific code, but anyway I have couple of comments bellow.

Thanks!
-Lukas

Quoted 30 lines
Signed-off-by Kyungmin Park <[email protected]>

---

fs/fat/fat.h +1 -0
index f504089..08b53e1 100644
@@ -299,6 +299,7 @@ extern int fat_alloc_clusters(struct inode *inode, int *cluster,
                             int nr_cluster);
  extern int fat_free_clusters(struct inode *inode, int cluster);
  extern int fat_count_free_clusters(struct super_block *sb);
+extern int fat_trim_fs(struct super_block *sb, struct fstrim_range *range);
 
  /* fat/file.c */
  extern long fat_generic_ioctl(struct file *filp, unsigned int cmd,
fs/fat/fatent.c +2 -0
index b47d2c9..777094b 100644
@@ -1,6 +1,8 @@
  /*
   * Copyright (C) 2004, OGAWA Hirofumi
   * Released under GPL v2.
+ *
+ * Batched discard support by Kyungmin Park <[email protected]>
   */
 
  #include <linux/module.h>

@@ -541,6 +543,16 @@ out:
       return err;

fs/fat/fatent.c out: · line 553

Quoted 10 lines
fs/fat/fatent.c +8 -0
  }
 
+static int fat_issue_discard(struct super_block *sb, int cluster, int nr_clus)
+{
+     struct msdos_sb_info *sbi = MSDOS_SB(sb);
+     sector_t block, nr_blocks;
+
+        block = fat_clus_to_blknr(sbi, cluster);
+        nr_blocks = nr_clus * sbi->sec_per_clus;
+        return sb_issue_discard(sb, block, nr_blocks, GFP_NOFS, 0);

Use tabs for code indent.

Quoted 24 lines
fs/fat/fatent.c +5 -5
+}
+
  int fat_free_clusters(struct inode *inode, int cluster)
  {
       struct super_block *sb = inode->i_sb;

@@ -575,11 +587,7 @@ int fat_free_clusters(struct inode *inode, int cluster)
                       if (cluster != fatent.entry + 1) {
                               int nr_clus = fatent.entry - first_cl + 1;
 
-                             sb_issue_discard(sb,
-                                     fat_clus_to_blknr(sbi, first_cl),
-                                     nr_clus * sbi->sec_per_clus,
-                                     GFP_NOFS, 0);
-
+                             fat_issue_discard(sb, first_cl, nr_clus);
                               first_cl = cluster;
                       }
               }

@@ -683,3 +691,73 @@ out:
       unlock_fat(sbi);
       return err;
  }
+
+int fat_trim_fs(struct super_block *sb, struct fstrim_range *range)

fs/fat/fatent.c out: · line 705

Quoted 10 lines
fs/fat/fatent.c +10 -0
+{
+     struct msdos_sb_info *sbi = MSDOS_SB(sb);
+     struct fatent_operations *ops = sbi->fatent_ops;
+     struct fat_entry fatent;
+     unsigned long reada_blocks, reada_mask, cur_block;
+     int err = 0, free;
+     unsigned int start, len, minlen, trimmed;
+     int entry = 0;
+
+     start = range->start >> sb->s_blocksize_bits;

Start is not used anywhere either. Let me just explain the reason for having start, and len.

Quoted 7 lines

start - address the first Byte of the filesystem (or better, the device, or part of the device underneath the filesystem itself). You are supposed to use this as a starting point for doing discard.

len - defines the length in Bytes of the filesystem (or better, the device, or part of the device underneath the filesystem itself), user want to discard from the "start".

Good explanation. yes I know it's not used at current patch but it's added for ext4 does.
If anyone complaint it, I can remove it for this time.

Quoted 21 lines

This two values gives us more flexibility in the way we are discarding the filesystem's free blocks. For example, if you have huge filesystem, or your device has bad discard performance, you probably do not want to do the FITRIM all-at-once, but rather per-partes, to not disturb other ongoing IO too much. Basically it allows you to spread the load through longer period of time.

Also, for bigger filesystems we might want to inform user that something is really happening in the form of progress bar (or whatever), which you can not do otherwise.

fs/fat/fatent.c out: · line 706

Quoted 1 line
fs/fat/fatent.c +1 -0
+     len = range->len >> sb->s_blocksize_bits;

"len" does not seem to be used anywhere, does it mean that you are discarding free extents on the whole filesystem with one run ? This operation can take pretty long time on devices with slow discard capability. Can you consider doing it per-partes as we are doing in ext4 for example ? I really can't say how much work does it mean in fat, but it might be worth it. Especially because of ... ->

Quoted 13 lines

fs/fat/fatent.c out: · line 712

Quoted 6 lines
fs/fat/fatent.c +6 -0
+     minlen = range->minlen >> sb->s_blocksize_bits;
+     trimmed = 0;
+
+     minlen = minlen / sbi->sec_per_clus;
+
+     lock_fat(sbi);

       ^^^^^^^^^^^^^^
       -> this
You are holding this mutex the whole time you are discarding free extents and I think it would be devastating, because the users will see long stalls. The bigger the filesystem is and the worse discard performance of the device is, the longer stall will be. Please correct me I am mistaken. Have you done any testing ?

Now I assume it's not perform during fat operation at normal case. It's performed at specific time instead of parallel IO as ext4 does. e.g., it's performed after disconnected from windows and so on. another reason is can't guarantee the fat table entry consistency during trim, I just lock the fat at start time as count the free space at fat codes.

Quoted 63 lines
Quoted 63 lines
fs/fat/fatent.c +51 -0
+     if (sbi->free_clusters != -1 && sbi->free_clus_valid)
+             goto out;
+
+     reada_blocks = FAT_READA_SIZE >> sb->s_blocksize_bits;
+     reada_mask = reada_blocks - 1;
+     cur_block = 0;
+
+     free = 0;
+     fatent_init(&fatent);
+
+     /*
+      * REVISIT: scan from the last free block.
+      */
+     fatent_set_entry(&fatent, FAT_START_ENT);
+     while (fatent.entry < sbi->max_cluster) {
+             /* readahead of fat blocks */
+             if ((cur_block & reada_mask) == 0) {
+                     unsigned long rest = sbi->fat_length - cur_block;
+                     fat_ent_reada(sb, &fatent, min(reada_blocks, rest));
+             }
+             cur_block++;
+
+             err = fat_ent_read_block(sb, &fatent);
+             if (err)
+                     goto out;
+
+             do {
+                     if (ops->ent_get(&fatent) == FAT_ENT_FREE) {
+                             free++;
+                             if (!entry)
+                                     entry = fatent.entry;
+                     } else if (entry) {
+                             if (free >= minlen) {
+                                     fat_issue_discard(sb, entry, free);
+                                     trimmed += free;
+                             }
+                             free = 0;
+                             entry = 0;
+                     }
+             } while (fat_ent_next(sbi, &fatent));
+     }
+     if (free >= minlen) {
+             fat_issue_discard(sb, entry, free);
+             trimmed += free;
+     }
+     range->len = (trimmed * sbi->sec_per_clus) * sb->s_blocksize;
+     fatent_brelse(&fatent);
+out:
+     unlock_fat(sbi);
+     return err;
+}
fs/fat/file.c +0 -0
index 7257752..bfdd558 100644
@@ -125,6 +125,30 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
               return fat_ioctl_get_attributes(inode, user_attr);

      case FAT_IOCTL_SET_ATTRIBUTES:
              return fat_ioctl_set_attributes(filp, user_attr);
+     case FITRIM:
+     {
+             struct super_block *sb = inode->i_sb;

Quoted 8 lines
Quoted 5 lines

+             struct fstrim_range range;
+             int ret = 0;
+
+             if (!capable(CAP_SYS_ADMIN))
+                     return -EPERM;

You might want to add check whether the device actually support discard.
See: http://www.spinics.net/lists/linux-ext4/msg23144.html

Good. I didn't see the patch, next version I'll add it.

Thank you,
Kyungmin Park

Quoted 15 lines
Quoted 15 lines
fs/fat/file.c +14 -0
+
+             if (copy_from_user(&range, (struct fstrim_range *)arg,
+                                     sizeof(range)))
+                     return -EFAULT;
+
+             ret = fat_trim_fs(sb, &range);
+             if (ret < 0)
+                     return ret;
+
+             if (copy_to_user((struct fstrim_range *)arg, &range,
+                                     sizeof(range)))
+                     return -EFAULT;
+
+             return 0;
Quoted 9 lines
Quoted 6 lines

+     }
+
      default:
              return -ENOTTY; /* Inappropriate ioctl for device */ }

--

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

#3 lore raw
Quoted 111 lines · Kyungmin Park
Quoted 110 lines · Lukas Czerner
Quoted 10 lines · Kyungmin Park

From: Kyungmin Park <[email protected]>

FAT supports batched discard as ext4.

Cited from Lukas words.
"The current solution is not ideal because of its bad performance impact. So basic idea to improve things is to avoid discarding every time some blocks are freed. and instead batching is together into bigger trims, which tends to be more effective."

Quoted 3 lines

You can find an information in detail at following URLs.
http://lwn.net/Articles/397538/
http://lwn.net/Articles/383933/

Hi Kyungmin,

this is really great to see more and more filesystemS ADDING this. I can not really comment on fat specific code, but anyway I have couple of comments bellow.

Thanks!
-Lukas

Quoted 30 lines
Signed-off-by Kyungmin Park <[email protected]>

---

fs/fat/fat.h +1 -0
index f504089..08b53e1 100644
@@ -299,6 +299,7 @@ extern int fat_alloc_clusters(struct inode *inode, int *cluster,
                             int nr_cluster);
  extern int fat_free_clusters(struct inode *inode, int cluster);
  extern int fat_count_free_clusters(struct super_block *sb);
+extern int fat_trim_fs(struct super_block *sb, struct fstrim_range *range);
 
  /* fat/file.c */
  extern long fat_generic_ioctl(struct file *filp, unsigned int cmd,
fs/fat/fatent.c +2 -0
index b47d2c9..777094b 100644
@@ -1,6 +1,8 @@
  /*
   * Copyright (C) 2004, OGAWA Hirofumi
   * Released under GPL v2.
+ *
+ * Batched discard support by Kyungmin Park <[email protected]>
   */
 
  #include <linux/module.h>

@@ -541,6 +543,16 @@ out:
       return err;

fs/fat/fatent.c out: · line 553

Quoted 10 lines
fs/fat/fatent.c +8 -0
  }
 
+static int fat_issue_discard(struct super_block *sb, int cluster, int nr_clus)
+{
+     struct msdos_sb_info *sbi = MSDOS_SB(sb);
+     sector_t block, nr_blocks;
+
+        block = fat_clus_to_blknr(sbi, cluster);
+        nr_blocks = nr_clus * sbi->sec_per_clus;
+        return sb_issue_discard(sb, block, nr_blocks, GFP_NOFS, 0);

Use tabs for code indent.

Quoted 24 lines
fs/fat/fatent.c +5 -5
+}
+
  int fat_free_clusters(struct inode *inode, int cluster)
  {
       struct super_block *sb = inode->i_sb;

@@ -575,11 +587,7 @@ int fat_free_clusters(struct inode *inode, int cluster)
                       if (cluster != fatent.entry + 1) {
                               int nr_clus = fatent.entry - first_cl + 1;
 
-                             sb_issue_discard(sb,
-                                     fat_clus_to_blknr(sbi, first_cl),
-                                     nr_clus * sbi->sec_per_clus,
-                                     GFP_NOFS, 0);
-
+                             fat_issue_discard(sb, first_cl, nr_clus);
                               first_cl = cluster;
                       }
               }

@@ -683,3 +691,73 @@ out:
       unlock_fat(sbi);
       return err;
  }
+
+int fat_trim_fs(struct super_block *sb, struct fstrim_range *range)

fs/fat/fatent.c out: · line 705

Quoted 10 lines
fs/fat/fatent.c +10 -0
+{
+     struct msdos_sb_info *sbi = MSDOS_SB(sb);
+     struct fatent_operations *ops = sbi->fatent_ops;
+     struct fat_entry fatent;
+     unsigned long reada_blocks, reada_mask, cur_block;
+     int err = 0, free;
+     unsigned int start, len, minlen, trimmed;
+     int entry = 0;
+
+     start = range->start >> sb->s_blocksize_bits;

Start is not used anywhere either. Let me just explain the reason for having start, and len.

start - address the first Byte of the filesystem (or better, the device, or part of the device underneath the filesystem itself). You are supposed to use this as a starting point for doing discard.

Quoted 7 lines
Quoted 3 lines

len - defines the length in Bytes of the filesystem (or better, the device, or part of the device underneath the filesystem itself), user want to discard from the "start".

Good explanation. yes I know it's not used at current patch but it's added for ext4 does.
If anyone complaint it, I can remove it for this time.

Well, I think you misunderstood the reason behind having one ioctl for all the filesystems. The reason is, that the behavior is the same! for all the filesystems which implements it, so the user can use one interface an can rely on the fact that it will do the same thing! So simply removing start and len is not really a solution!

It just has to be done the way it accepts "start" and "len" and use it as it is suppose to.

Quoted 35 lines
Quoted 22 lines

This two values gives us more flexibility in the way we are discarding the filesystem's free blocks. For example, if you have huge filesystem, or your device has bad discard performance, you probably do not want to do the FITRIM all-at-once, but rather per-partes, to not disturb other ongoing IO too much. Basically it allows you to spread the load through longer period of time.

Also, for bigger filesystems we might want to inform user that something is really happening in the form of progress bar (or whatever), which you can not do otherwise.

fs/fat/fatent.c out: · line 706

Quoted 1 line
fs/fat/fatent.c +1 -0
+     len = range->len >> sb->s_blocksize_bits;

"len" does not seem to be used anywhere, does it mean that you are discarding free extents on the whole filesystem with one run ? This operation can take pretty long time on devices with slow discard capability. Can you consider doing it per-partes as we are doing in ext4 for example ? I really can't say how much work does it mean in fat, but it might be worth it. Especially because of ... ->

Quoted 13 lines

fs/fat/fatent.c out: · line 712

Quoted 6 lines
fs/fat/fatent.c +6 -0
+     minlen = range->minlen >> sb->s_blocksize_bits;
+     trimmed = 0;
+
+     minlen = minlen / sbi->sec_per_clus;
+
+     lock_fat(sbi);

       ^^^^^^^^^^^^^^
       -> this
You are holding this mutex the whole time you are discarding free extents and I think it would be devastating, because the users will see long stalls. The bigger the filesystem is and the worse discard performance of the device is, the longer stall will be. Please correct me I am mistaken. Have you done any testing ?

Quoted 6 lines

Now I assume it's not perform during fat operation at normal case. It's performed at specific time instead of parallel IO as ext4 does. e.g., it's performed after disconnected from windows and so on. another reason is can't guarantee the fat table entry consistency during trim, I just lock the fat at start time as count the free space at fat codes.

Ok, if there is no way around the mutex I am ok with that. However, you really need to implement the per-partes approach FITRIM interface forces you to.

By the way, you can not just rely on users not using the filesystem while FITRIM is going on, because they just will not care, especially if FITRIM will be invoked periodically form cron (for example).

Quoted 105 lines
Quoted 63 lines
Quoted 63 lines
fs/fat/fatent.c +51 -0
+     if (sbi->free_clusters != -1 && sbi->free_clus_valid)
+             goto out;
+
+     reada_blocks = FAT_READA_SIZE >> sb->s_blocksize_bits;
+     reada_mask = reada_blocks - 1;
+     cur_block = 0;
+
+     free = 0;
+     fatent_init(&fatent);
+
+     /*
+      * REVISIT: scan from the last free block.
+      */
+     fatent_set_entry(&fatent, FAT_START_ENT);
+     while (fatent.entry < sbi->max_cluster) {
+             /* readahead of fat blocks */
+             if ((cur_block & reada_mask) == 0) {
+                     unsigned long rest = sbi->fat_length - cur_block;
+                     fat_ent_reada(sb, &fatent, min(reada_blocks, rest));
+             }
+             cur_block++;
+
+             err = fat_ent_read_block(sb, &fatent);
+             if (err)
+                     goto out;
+
+             do {
+                     if (ops->ent_get(&fatent) == FAT_ENT_FREE) {
+                             free++;
+                             if (!entry)
+                                     entry = fatent.entry;
+                     } else if (entry) {
+                             if (free >= minlen) {
+                                     fat_issue_discard(sb, entry, free);
+                                     trimmed += free;
+                             }
+                             free = 0;
+                             entry = 0;
+                     }
+             } while (fat_ent_next(sbi, &fatent));
+     }
+     if (free >= minlen) {
+             fat_issue_discard(sb, entry, free);
+             trimmed += free;
+     }
+     range->len = (trimmed * sbi->sec_per_clus) * sb->s_blocksize;
+     fatent_brelse(&fatent);
+out:
+     unlock_fat(sbi);
+     return err;
+}
fs/fat/file.c +0 -0
index 7257752..bfdd558 100644
@@ -125,6 +125,30 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
               return fat_ioctl_get_attributes(inode, user_attr);

      case FAT_IOCTL_SET_ATTRIBUTES:
              return fat_ioctl_set_attributes(filp, user_attr);
+     case FITRIM:
+     {
+             struct super_block *sb = inode->i_sb;

Quoted 8 lines
Quoted 5 lines

+             struct fstrim_range range;
+             int ret = 0;
+
+             if (!capable(CAP_SYS_ADMIN))
+                     return -EPERM;

You might want to add check whether the device actually support discard.
See: http://www.spinics.net/lists/linux-ext4/msg23144.html

Good. I didn't see the patch, next version I'll add it.

Thank you,
Kyungmin Park

Quoted 15 lines
Quoted 15 lines
fs/fat/file.c +14 -0
+
+             if (copy_from_user(&range, (struct fstrim_range *)arg,
+                                     sizeof(range)))
+                     return -EFAULT;
+
+             ret = fat_trim_fs(sb, &range);
+             if (ret < 0)
+                     return ret;
+
+             if (copy_to_user((struct fstrim_range *)arg, &range,
+                                     sizeof(range)))
+                     return -EFAULT;
+
+             return 0;
Quoted 9 lines
Quoted 6 lines

+     }
+
      default:
              return -ENOTTY; /* Inappropriate ioctl for device */ }

--

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

author
#4 lore raw
Quoted 121 lines · Lukas Czerner
Quoted 111 lines · Kyungmin Park
Quoted 110 lines · Lukas Czerner
Quoted 10 lines · Kyungmin Park

From: Kyungmin Park <[email protected]>

FAT supports batched discard as ext4.

Cited from Lukas words.
"The current solution is not ideal because of its bad performance impact. So basic idea to improve things is to avoid discarding every time some blocks are freed. and instead batching is together into bigger trims, which tends to be more effective."

Quoted 3 lines

You can find an information in detail at following URLs.
http://lwn.net/Articles/397538/
http://lwn.net/Articles/383933/

Hi Kyungmin,

this is really great to see more and more filesystemS ADDING this. I can not really comment on fat specific code, but anyway I have couple of comments bellow.

Thanks!
-Lukas

Quoted 30 lines
Signed-off-by Kyungmin Park <[email protected]>

---

fs/fat/fat.h +1 -0
index f504089..08b53e1 100644
@@ -299,6 +299,7 @@ extern int fat_alloc_clusters(struct inode *inode, int *cluster,
 á á á á á á á á á á á á á á int nr_cluster);
 áextern int fat_free_clusters(struct inode *inode, int cluster);
 áextern int fat_count_free_clusters(struct super_block *sb);
+extern int fat_trim_fs(struct super_block *sb, struct fstrim_range *range);

á/* fat/file.c */
áextern long fat_generic_ioctl(struct file *filp, unsigned int cmd,

fs/fat/fatent.c +0 -0
index b47d2c9..777094b 100644
@@ -1,6 +1,8 @@

á/*
á * Copyright (C) 2004, OGAWA Hirofumi
á * Released under GPL v2.
+ *
+ * Batched discard support by Kyungmin Park <[email protected]> á */

á#include <linux/module.h>

fs/fat/fatent.c +0 -0
@@ -541,6 +543,16 @@ out:
 á á á return err;

fs/fat/fatent.c out: · line 553

Quoted 10 lines
fs/fat/fatent.c +8 -0
 á}
 
+static int fat_issue_discard(struct super_block *sb, int cluster, int nr_clus)
+{
+ á á struct msdos_sb_info *sbi = MSDOS_SB(sb);
+ á á sector_t block, nr_blocks;
+
+ á á á áblock = fat_clus_to_blknr(sbi, cluster);
+ á á á ánr_blocks = nr_clus * sbi->sec_per_clus;
+ á á á áreturn sb_issue_discard(sb, block, nr_blocks, GFP_NOFS, 0);

Use tabs for code indent.

Quoted 24 lines
fs/fat/fatent.c +5 -5
+}
+
 áint fat_free_clusters(struct inode *inode, int cluster)
 á{
 á á á struct super_block *sb = inode->i_sb;

@@ -575,11 +587,7 @@ int fat_free_clusters(struct inode *inode, int cluster)
 á á á á á á á á á á á if (cluster != fatent.entry + 1) {
 á á á á á á á á á á á á á á á int nr_clus = fatent.entry - first_cl + 1;
 
- á á á á á á á á á á á á á á sb_issue_discard(sb,
- á á á á á á á á á á á á á á á á á á fat_clus_to_blknr(sbi, first_cl),
- á á á á á á á á á á á á á á á á á á nr_clus * sbi->sec_per_clus,
- á á á á á á á á á á á á á á á á á á GFP_NOFS, 0);
-
+ á á á á á á á á á á á á á á fat_issue_discard(sb, first_cl, nr_clus);
 á á á á á á á á á á á á á á á first_cl = cluster;
 á á á á á á á á á á á }
 á á á á á á á }

@@ -683,3 +691,73 @@ out:
 á á á unlock_fat(sbi);
 á á á return err;
 á}
+
+int fat_trim_fs(struct super_block *sb, struct fstrim_range *range)

fs/fat/fatent.c out: · line 705

Quoted 10 lines
fs/fat/fatent.c +10 -0
+{
+ á á struct msdos_sb_info *sbi = MSDOS_SB(sb);
+ á á struct fatent_operations *ops = sbi->fatent_ops;
+ á á struct fat_entry fatent;
+ á á unsigned long reada_blocks, reada_mask, cur_block;
+ á á int err = 0, free;
+ á á unsigned int start, len, minlen, trimmed;
+ á á int entry = 0;
+
+ á á start = range->start >> sb->s_blocksize_bits;

Start is not used anywhere either. Let me just explain the reason for having start, and len.

start - address the first Byte of the filesystem (or better, the device, áor part of the device underneath the filesystem itself). You are ásupposed to use this as a starting point for doing discard.

Quoted 7 lines
Quoted 3 lines

len - defines the length in Bytes of the filesystem (or better, the ádevice, or part of the device underneath the filesystem itself), user áwant to discard from the "start".

Good explanation. yes I know it's not used at current patch but it's added for ext4 does.
If anyone complaint it, I can remove it for this time.

Quoted 8 lines

Well, I think you misunderstood the reason behind having one ioctl for all the filesystems. The reason is, that the behavior is the same! for all the filesystems which implements it, so the user can use one interface an can rely on the fact that it will do the same thing! So simply removing start and len is not really a solution!

It just has to be done the way it accepts "start" and "len" and use it as it is suppose to.

Updated. next version handles the "start" and "len" properly.

Are there any fat specific comments?

Thank you,
Kyungmin Park

Quoted 151 lines
Quoted 36 lines
Quoted 23 lines

This two values gives us more flexibility in the way we are discarding the filesystem's free blocks. For example, if you have huge filesystem, or your device has bad discard performance, you probably do not want to do the FITRIM all-at-once, but rather per-partes, to not disturb other ongoing IO too much. Basically it allows you to spread the load through longer period of time.

Also, for bigger filesystems we might want to inform user that something is really happening in the form of progress bar (or whatever), which you can not do otherwise.

fs/fat/fatent.c out: · line 706

Quoted 1 line
fs/fat/fatent.c +1 -0
+ á á len = range->len >> sb->s_blocksize_bits;

"len" does not seem to be used anywhere, does it mean that you are discarding free extents on the whole filesystem with one run ? This operation can take pretty long time on devices with slow discard capability. Can you consider doing it per-partes as we are doing in ext4 for example ? I really can't say how much work does it mean in fat, but it might be worth it. Especially because of ... ->

Quoted 13 lines

fs/fat/fatent.c out: · line 712

Quoted 6 lines
fs/fat/fatent.c +6 -0
+ á á minlen = range->minlen >> sb->s_blocksize_bits;
+ á á trimmed = 0;
+
+ á á minlen = minlen / sbi->sec_per_clus;
+
+ á á lock_fat(sbi);

á á á á^^^^^^^^^^^^^^
á á á á-> this
You are holding this mutex the whole time you are discarding free extents and I think it would be devastating, because the users will see long stalls. The bigger the filesystem is and the worse discard performance of the device is, the longer stall will be. Please correct me I am mistaken. Have you done any testing ?

Quoted 6 lines

Now I assume it's not perform during fat operation at normal case. It's performed at specific time instead of parallel IO as ext4 does. e.g., it's performed after disconnected from windows and so on. another reason is can't guarantee the fat table entry consistency during trim, I just lock the fat at start time as count the free space at fat codes.

Ok, if there is no way around the mutex I am ok with that. However, you really need to implement the per-partes approach FITRIM interface forces you to.

By the way, you can not just rely on users not using the filesystem while FITRIM is going on, because they just will not care, especially if FITRIM will be invoked periodically form cron (for example).

Quoted 100 lines
Quoted 63 lines
Quoted 63 lines
fs/fat/fatent.c +51 -0
+ á á if (sbi->free_clusters != -1 && sbi->free_clus_valid)
+ á á á á á á goto out;
+
+ á á reada_blocks = FAT_READA_SIZE >> sb->s_blocksize_bits;
+ á á reada_mask = reada_blocks - 1;
+ á á cur_block = 0;
+
+ á á free = 0;
+ á á fatent_init(&fatent);
+
+ á á /*
+ á á á* REVISIT: scan from the last free block.
+ á á á*/
+ á á fatent_set_entry(&fatent, FAT_START_ENT);
+ á á while (fatent.entry < sbi->max_cluster) {
+ á á á á á á /* readahead of fat blocks */
+ á á á á á á if ((cur_block & reada_mask) == 0) {
+ á á á á á á á á á á unsigned long rest = sbi->fat_length - cur_block;
+ á á á á á á á á á á fat_ent_reada(sb, &fatent, min(reada_blocks, rest));
+ á á á á á á }
+ á á á á á á cur_block++;
+
+ á á á á á á err = fat_ent_read_block(sb, &fatent);
+ á á á á á á if (err)
+ á á á á á á á á á á goto out;
+
+ á á á á á á do {
+ á á á á á á á á á á if (ops->ent_get(&fatent) == FAT_ENT_FREE) {
+ á á á á á á á á á á á á á á free++;
+ á á á á á á á á á á á á á á if (!entry)
+ á á á á á á á á á á á á á á á á á á entry = fatent.entry;
+ á á á á á á á á á á } else if (entry) {
+ á á á á á á á á á á á á á á if (free >= minlen) {
+ á á á á á á á á á á á á á á á á á á fat_issue_discard(sb, entry, free);
+ á á á á á á á á á á á á á á á á á á trimmed += free;
+ á á á á á á á á á á á á á á }
+ á á á á á á á á á á á á á á free = 0;
+ á á á á á á á á á á á á á á entry = 0;
+ á á á á á á á á á á }
+ á á á á á á } while (fat_ent_next(sbi, &fatent));
+ á á }
+ á á if (free >= minlen) {
+ á á á á á á fat_issue_discard(sb, entry, free);
+ á á á á á á trimmed += free;
+ á á }
+ á á range->len = (trimmed * sbi->sec_per_clus) * sb->s_blocksize;
+ á á fatent_brelse(&fatent);
+out:
+ á á unlock_fat(sbi);
+ á á return err;
+}
fs/fat/file.c +0 -0
index 7257752..bfdd558 100644
@@ -125,6 +125,30 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 á á á á á á á return fat_ioctl_get_attributes(inode, user_attr);

á á á case FAT_IOCTL_SET_ATTRIBUTES:
á á á á á á á return fat_ioctl_set_attributes(filp, user_attr);
+ á á case FITRIM:
+ á á {
+ á á á á á á struct super_block *sb = inode->i_sb;

Quoted 8 lines
Quoted 5 lines

+ á á á á á á struct fstrim_range range;
+ á á á á á á int ret = 0;
+
+ á á á á á á if (!capable(CAP_SYS_ADMIN))
+ á á á á á á á á á á return -EPERM;

You might want to add check whether the device actually support discard.
See: http://www.spinics.net/lists/linux-ext4/msg23144.html

Good. I didn't see the patch, next version I'll add it.

Thank you,
Kyungmin Park

Quoted 24 lines
Quoted 17 lines
fs/fat/file.c +16 -0
+
+ á á á á á á if (copy_from_user(&range, (struct fstrim_range *)arg,
+ á á á á á á á á á á á á á á á á á á sizeof(range)))
+ á á á á á á á á á á return -EFAULT;
+
+ á á á á á á ret = fat_trim_fs(sb, &range);
+ á á á á á á if (ret < 0)
+ á á á á á á á á á á return ret;
+
+ á á á á á á if (copy_to_user((struct fstrim_range *)arg, &range,
+ á á á á á á á á á á á á á á á á á á sizeof(range)))
+ á á á á á á á á á á return -EFAULT;
+
+ á á á á á á return 0;
+ á á }
+
Quoted 4 lines

á á á default:
á á á á á á á return -ENOTTY; /* Inappropriate ioctl for device */ á á á }

--

Quoted 7 lines
Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" 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-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html