Skip to contentSkip to conversation

fat: Batched discard support for fat

PATCH Superseded Kyungmin Park 2 participants 8 messages Feb 25, 2011 – Mar 5, 2011

Subsystems (2)
VFS VFAT/FAT/msdos filesystem

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Kyungmin Park

author
#0 lore raw

PATCH v3 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/

Clearify the meaning of "len" (Cited form Lukas mail)

Let the "O" be free (bytes, blocks, whatever), and "=" be used. Now, we have a filesystem like this.

OOOO==O===OO===OOOOO==O===O===OOOOOOO===
^ ^
0 40

This is how it supposed to wotk if you have called FITIRM with parameters:

start = 0
minlen = 2
len = 20

So you will go through (blocks, bytes...) 0 -> 20

OOOO==O===OO===OOOOO==O===O===OOOOOOO===
^ ^
0 20

So, you will call discard on extents:

0-3
You'll skip 6 because is smaller than minlen
10-11
15-19

instead of

0-3
10-11
15-19
30-36

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

---
Changelog v3:
Adjust the minlen from queue discard_granularity Use the corrent len usage
Changelog v2:
Use the given start and len as Lukas comments Check the queue supports discard feature
---

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 +98 -5

124 lines not on this page · raw message

fs/fat/file.c +30 -0
index 7257752..9910aba 100644
@@ -125,6 +125,36 @@ 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 request_queue *q = bdev_get_queue(sb->s_bdev);
+		struct fstrim_range range;
+		int ret = 0;
+
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+
+		if (!blk_queue_discard(q))
+			return -EOPNOTSUPP;
+
+		if (copy_from_user(&range, (struct fstrim_range *)arg,
+					sizeof(range)))
+			return -EFAULT;
+
+		range.minlen = max((unsigned int)range.minlen,
+					q->limits.discard_granularity);
+		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 42 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."

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

Clearify the meaning of "len" (Cited form Lukas mail)

Let the "O" be free (bytes, blocks, whatever), and "=" be used. Now, we have a filesystem like this.

OOOO==O===OO===OOOOO==O===O===OOOOOOO===
^ ^
0 40

This is how it supposed to wotk if you have called FITIRM with parameters:

start = 0
minlen = 2
len = 20

So you will go through (blocks, bytes...) 0 -> 20

OOOO==O===OO===OOOOO==O===O===OOOOOOO===
^ ^
0 20

So, you will call discard on extents:

0-3
You'll skip 6 because is smaller than minlen
10-11
15-19

Quoted 6 lines

instead of

0-3
10-11
15-19
30-36

Hi thanks for the next version. And again I have to ask: Did you test it ? and how ? Did you tried xfstest No. 251 ? Couple of comments bellow.

Thanks!
-Lukas

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

---
Changelog v3:
Adjust the minlen from queue discard_granularity Use the corrent len usage
Changelog v2:
Use the given start and len as Lukas comments Check the queue supports discard feature
---

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 +46 -5
index b47d2c9..a8e3837 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;
 }
 
+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);
+}
+
 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,88 @@ out:
 	unlock_fat(sbi);
 	return err;
 }
+
+int fat_trim_fs(struct super_block *sb, struct fstrim_range *range)
+{
+	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, count, entry;
+	int start, len, minlen, trimmed;
+
+	start = range->start >> sb->s_blocksize_bits;
+	start = start / sbi->sec_per_clus;
+	len = range->len >> sb->s_blocksize_bits;
+	len = len / sbi->sec_per_clus;
+	minlen = range->minlen >> sb->s_blocksize_bits;
+	minlen = minlen / sbi->sec_per_clus;
+	trimmed = 0;
+	count = 0;
+
+	lock_fat(sbi);
+	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;
+
+	entry = 0;
+	free = 0;
+	fatent_init(&fatent);
+
+	if (start < FAT_START_ENT)
+		start = FAT_START_ENT;

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

Quoted 10 lines
fs/fat/fatent.c +10 -0
+
+	fatent_set_entry(&fatent, start);
+
+	while (count < sbi->max_cluster) {
+		if (fatent.entry >= sbi->max_cluster)
+			fatent.entry = FAT_START_ENT;
+		/* 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));

You really do not need new variable "rest" just for passing it into one function. Get rid of it.

Quoted 5 lines
fs/fat/fatent.c +5 -0
+		}
+		cur_block++;
+
+		err = fat_ent_read_block(sb, &fatent);
+		if (err)

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

Quoted 10 lines
fs/fat/fatent.c +10 -0
+			goto out;
+
+		do {
+			if (ops->ent_get(&fatent) == FAT_ENT_FREE) {
+				free++;
+				if (!entry)
+					entry = fatent.entry;
+				if (count >= len && free >= minlen) {
+					fat_issue_discard(sb, entry, free);
+					trimmed += free;

I really do not understand FAT code very much, but is this right ? Should not you be setting free = 0 ? What will happen if you'll end up in the same branch in next iteration ? -- free will be still set to previous value+1, bu you'll be discarding next entry. I am sorry but this whole thing is not very readable.

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

Quoted 13 lines
fs/fat/fatent.c +13 -0
+				}
+				if (count >= len)
+					goto done;
+			} else if (entry) {
+				if (free >= minlen) {
+					fat_issue_discard(sb, entry, free);
+					trimmed += free;
+				}
+				if (count >= len)
+					goto done;
+				free = 0;
+				entry = 0;
+			}

I don't not see why you are testing count all the time since it has not been changed since the "if" condition started. how about doing one test before the "if" condition ?

Quoted 56 lines
fs/fat/fatent.c +14 -0
+			count++;
+		} while (fat_ent_next(sbi, &fatent));
+	}
+	if (free >= minlen) {
+		fat_issue_discard(sb, entry, free);
+		trimmed += free;
+	}
+done:
+	range->len = (trimmed * sbi->sec_per_clus) << sb->s_blocksize_bits;
+	fatent_brelse(&fatent);
+out:
+	unlock_fat(sbi);
+	return err;
+}
fs/fat/file.c +30 -0
index 7257752..9910aba 100644
@@ -125,6 +125,36 @@ 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 request_queue *q = bdev_get_queue(sb->s_bdev);
+		struct fstrim_range range;
+		int ret = 0;
+
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+
+		if (!blk_queue_discard(q))
+			return -EOPNOTSUPP;
+
+		if (copy_from_user(&range, (struct fstrim_range *)arg,
+					sizeof(range)))
+			return -EFAULT;
+
+		range.minlen = max((unsigned int)range.minlen,
+					q->limits.discard_granularity);
+		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 46 lines · Lukas Czerner
Quoted 44 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."

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

Clearify the meaning of "len" (Cited form Lukas mail)

Let the "O" be free (bytes, blocks, whatever), and "=" be used. Now, we have a filesystem like this.

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                                      ^
  0                                      40

This is how it supposed to wotk if you have called FITIRM with parameters:

start = 0
minlen = 2
len = 20

So you will go through (blocks, bytes...) 0 -> 20

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                   ^
  0                   20

So, you will call discard on extents:

0-3
You'll skip 6 because is smaller than minlen
10-11
15-19

instead of

Quoted 7 lines
Quoted 4 lines

0-3
10-11
15-19
30-36

Hi thanks for the next version. And again I have to ask: Did you test it ? and how ? Did you tried xfstest No. 251 ? Couple of comments bellow.

I tested it with your test program. Of course I modified for our environment (eMMC).

#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdint.h>
#include <sys/ioctl.h>

struct fstrim_range {
        uint64_t start;
        uint64_t len;
        uint64_t minlen;
};

#define FITRIM _IOWR('X', 121, struct fstrim_range)

int main(int argc, char **argv)

{
        struct fstrim_range range;
        uint64_t len;
        int fd;

        if (argc < 2) {
                fprintf(stderr, "usage: %s mountpoint [size]\n", argv[0]);
                return 1;
        }

        if (argc == 3)
                len = atoll(argv[1]);
        else
                len = ((1UL<<31) - 1);

        range.start = 0;
        range.len = len;
        range.minlen = 256 * 1024;      /* Minimum is 256KiB */

        fd = open(argv[1], O_RDONLY);
        if (fd < 0) {
                perror("open");
                return 1;
        }

        if (ioctl(fd, FITRIM, &range)) {
                if (errno == EOPNOTSUPP)
                        fprintf(stderr, "TRIM not supported\n");
                else
                        perror("FITRIM");
                return 1;
        }

        fprintf(stderr, "Trimmed size %llu\n", range.len);

        return 0;
}
Quoted 111 lines

Thanks!
-Lukas

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

---
Changelog v3:
      Adjust the minlen from queue discard_granularity Use the corrent len usage
Changelog v2:
      Use the given start and len as Lukas comments Check the queue supports discard feature
---

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 +51 -5
index b47d2c9..a8e3837 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;
  }
 
+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);
+}
+
  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,88 @@ out:
       unlock_fat(sbi);
       return err;
  }
+
+int fat_trim_fs(struct super_block *sb, struct fstrim_range *range)
+{
+     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, count, entry;
+     int start, len, minlen, trimmed;
+
+     start = range->start >> sb->s_blocksize_bits;
+     start = start / sbi->sec_per_clus;
+     len = range->len >> sb->s_blocksize_bits;
+     len = len / sbi->sec_per_clus;
+     minlen = range->minlen >> sb->s_blocksize_bits;
+     minlen = minlen / sbi->sec_per_clus;
+     trimmed = 0;
+     count = 0;
+
+     lock_fat(sbi);
+     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;
+
+     entry = 0;
+     free = 0;
+     fatent_init(&fatent);
+
+     if (start < FAT_START_ENT)
+             start = FAT_START_ENT;
+
+     fatent_set_entry(&fatent, start);
+
+     while (count < sbi->max_cluster) {
+             if (fatent.entry >= sbi->max_cluster)
Quoted 8 lines

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

Quoted 5 lines
fs/fat/fatent.c +5 -0
+                     fatent.entry = FAT_START_ENT;
+             /* 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));

You really do not need new variable "rest" just for passing it into one function. Get rid of it.

Umm. I don't want to modify it since it's routine is same as free_count codes. I just borrowed it from FAT codes.

Quoted 10 lines
Quoted 10 lines
fs/fat/fatent.c +9 -0
+             }
+             cur_block++;
+
+             err = fat_ent_read_block(sb, &fatent);
+             if (err)
+                     goto out;
+
+             do {
+                     if (ops->ent_get(&fatent) == FAT_ENT_FREE) {
Quoted 8 lines

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

Quoted 6 lines
fs/fat/fatent.c +6 -0
+                             free++;
+                             if (!entry)
+                                     entry = fatent.entry;
+                             if (count >= len && free >= minlen) {
+                                     fat_issue_discard(sb, entry, free);
+                                     trimmed += free;

I really do not understand FAT code very much, but is this right ? Should not you be setting free = 0 ?

Right but count is larger then "len". it will exit at next if (count

Quoted 22 lines

= len) goto done statement.

What will happen if you'll end up
in the same branch in next iteration ? -- free will be still set to previous value+1, bu you'll be discarding next entry. I am sorry but this whole thing is not very readable.

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

Quoted 13 lines
fs/fat/fatent.c +13 -0
+                             }
+                             if (count >= len)
+                                     goto done;
+                     } else if (entry) {
+                             if (free >= minlen) {
+                                     fat_issue_discard(sb, entry, free);
+                                     trimmed += free;
+                             }
+                             if (count >= len)
+                                     goto done;
+                             free = 0;
+                             entry = 0;
+                     }
Quoted 3 lines

I don't not see why you are testing count all the time since it has not been changed since the "if" condition started. how about doing one test before the "if" condition ?

For looping the whole fat max_cluster size. now start can be any address and if start is a middle point and len is max, then it will search from middle -> end -> start -> middle. The "count" is used for this purpose.

Thank you,
Kyungmin Park

Quoted 60 lines
Quoted 53 lines
fs/fat/fatent.c +14 -0
+                     count++;
+             } while (fat_ent_next(sbi, &fatent));
+     }
+     if (free >= minlen) {
+             fat_issue_discard(sb, entry, free);
+             trimmed += free;
+     }
+done:
+     range->len = (trimmed * sbi->sec_per_clus) << sb->s_blocksize_bits;
+     fatent_brelse(&fatent);
+out:
+     unlock_fat(sbi);
+     return err;
+}
fs/fat/file.c +0 -0
index 7257752..9910aba 100644
@@ -125,6 +125,36 @@ 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 request_queue *q = bdev_get_queue(sb->s_bdev);
+             struct fstrim_range range;
+             int ret = 0;

fs/fat/file.c +24 -0
+
+             if (!capable(CAP_SYS_ADMIN))
+                     return -EPERM;
+
+             if (!blk_queue_discard(q))
+                     return -EOPNOTSUPP;
+
+             if (copy_from_user(&range, (struct fstrim_range *)arg,
+                                     sizeof(range)))
+                     return -EFAULT;
+
+             range.minlen = max((unsigned int)range.minlen,
+                                     q->limits.discard_granularity);
+             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 */ }

--

#3 lore raw
Quoted 52 lines · Kyungmin Park
Quoted 51 lines · Lukas Czerner
Quoted 48 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."

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

Clearify the meaning of "len" (Cited form Lukas mail)

Let the "O" be free (bytes, blocks, whatever), and "=" be used. Now, we have a filesystem like this.

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                                      ^
  0                                      40

This is how it supposed to wotk if you have called FITIRM with parameters:

start = 0
minlen = 2
len = 20

So you will go through (blocks, bytes...) 0 -> 20

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                   ^
  0                   20

So, you will call discard on extents:

0-3
You'll skip 6 because is smaller than minlen
10-11
15-19

instead of

0-3
10-11
15-19
30-36

Quoted 5 lines
Quoted 2 lines

Hi thanks for the next version. And again I have to ask: Did you test it ? and how ? Did you tried xfstest No. 251 ? Couple of comments bellow.

I tested it with your test program. Of course I modified for our environment (eMMC).

Ok, good.

Quoted 26 lines

#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdint.h>
#include <sys/ioctl.h>

struct fstrim_range {
        uint64_t start;
        uint64_t len;
        uint64_t minlen;
};

#define FITRIM _IOWR('X', 121, struct fstrim_range)

int main(int argc, char **argv)

{
        struct fstrim_range range;
        uint64_t len;
        int fd;

        if (argc < 2) {
                fprintf(stderr, "usage: %s mountpoint [size]\n", argv[0]);
                return 1;
        }
Quoted 8 lines
        if (argc == 3)
                len = atoll(argv[1]);
        else
                len = ((1UL<<31) - 1);

        range.start = 0;
        range.len = len;
        range.minlen = 256 * 1024;      /* Minimum is 256KiB */

Why exactly you need to set this ? What will happen if the minlen is 0 ?

Quoted 143 lines
        fd = open(argv[1], O_RDONLY);
        if (fd < 0) {
                perror("open");
                return 1;
        }

        if (ioctl(fd, FITRIM, &range)) {
                if (errno == EOPNOTSUPP)
                        fprintf(stderr, "TRIM not supported\n");
                else
                        perror("FITRIM");
                return 1;
        }

        fprintf(stderr, "Trimmed size %llu\n", range.len);

        return 0;
}
Quoted 111 lines

Thanks!
-Lukas

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

---
Changelog v3:
      Adjust the minlen from queue discard_granularity Use the corrent len usage
Changelog v2:
      Use the given start and len as Lukas comments Check the queue supports discard feature
---

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 +51 -5
index b47d2c9..a8e3837 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;
  }
 
+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);
+}
+
  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,88 @@ out:
       unlock_fat(sbi);
       return err;
  }
+
+int fat_trim_fs(struct super_block *sb, struct fstrim_range *range)
+{
+     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, count, entry;
+     int start, len, minlen, trimmed;
+
+     start = range->start >> sb->s_blocksize_bits;
+     start = start / sbi->sec_per_clus;
+     len = range->len >> sb->s_blocksize_bits;
+     len = len / sbi->sec_per_clus;
+     minlen = range->minlen >> sb->s_blocksize_bits;
+     minlen = minlen / sbi->sec_per_clus;
+     trimmed = 0;
+     count = 0;
+
+     lock_fat(sbi);
+     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;
+
+     entry = 0;
+     free = 0;
+     fatent_init(&fatent);
+
+     if (start < FAT_START_ENT)
+             start = FAT_START_ENT;
+
+     fatent_set_entry(&fatent, start);
+
+     while (count < sbi->max_cluster) {
+             if (fatent.entry >= sbi->max_cluster)
Quoted 8 lines

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

Quoted 5 lines
fs/fat/fatent.c +5 -0
+                     fatent.entry = FAT_START_ENT;
+             /* 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));

You really do not need new variable "rest" just for passing it into one function. Get rid of it.

Umm. I don't want to modify it since it's routine is same as free_count codes. I just borrowed it from FAT codes.

Quoted 1 line
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
fs/fat/fatent.c +7 -0
+             }
+             cur_block++;
+
+             err = fat_ent_read_block(sb, &fatent);
+             if (err)
+                     goto out;
+
Quoted 8 lines

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

Quoted 8 lines
fs/fat/fatent.c +8 -0
+             do {
+                     if (ops->ent_get(&fatent) == FAT_ENT_FREE) {
+                             free++;
+                             if (!entry)
+                                     entry = fatent.entry;
+                             if (count >= len && free >= minlen) {
+                                     fat_issue_discard(sb, entry, free);
+                                     trimmed += free;

I think you can remove this condition completely is you move "done" label befor the if (free >= minlen) condition.

Quoted 4 lines
Quoted 2 lines

I really do not understand FAT code very much, but is this right ? Should not you be setting free = 0 ?

Right but count is larger then "len". it will exit at next if (count

Quoted 1 line

= len) goto done statement.

Oh, ok I see. But it proves my point that this code is not very well readable.

Quoted 10 lines
Quoted 10 lines

What will happen if you'll end up
in the same branch in next iteration ? -- free will be still set to previous value+1, bu you'll be discarding next entry. I am sorry but this whole thing is not very readable.

Quoted 3 lines
fs/fat/fatent.c +3 -0
+                             }
+                             if (count >= len)
+                                     goto done;

				   ^^^^^^^^^^^^^^^^^^

this

Quoted 7 lines
Quoted 7 lines

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

Quoted 7 lines
fs/fat/fatent.c +7 -0
+                     } else if (entry) {
+                             if (free >= minlen) {
+                                     fat_issue_discard(sb, entry, free);
+                                     trimmed += free;
+                             }
+                             if (count >= len)
+                                     goto done;

				   ^^^^^^^^^^^^^^^^^^

and this

Quoted 8 lines
Quoted 7 lines

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

Quoted 3 lines
fs/fat/fatent.c +3 -0
+                             free = 0;
+                             entry = 0;
+                     }

I don't not see why you are testing count all the time since it has not been changed since the "if" condition started. how about doing one test before the "if" condition ?

Quoted 8 lines

For looping the whole fat max_cluster size. now start can be any address and if start is a middle point and len is max, then it will search from middle -> end -> start -> middle. The "count" is used for this purpose.

Thank you,
Kyungmin Park

Quoted 1 line

can go here -->>
                          if (count >= len)
                                  goto done;

Quoted 60 lines
Quoted 59 lines
Quoted 52 lines
fs/fat/fatent.c +14 -0
+                     count++;
+             } while (fat_ent_next(sbi, &fatent));
+     }
+     if (free >= minlen) {
+             fat_issue_discard(sb, entry, free);
+             trimmed += free;
+     }
+done:
+     range->len = (trimmed * sbi->sec_per_clus) << sb->s_blocksize_bits;
+     fatent_brelse(&fatent);
+out:
+     unlock_fat(sbi);
+     return err;
+}
fs/fat/file.c +0 -0
index 7257752..9910aba 100644
@@ -125,6 +125,36 @@ 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 request_queue *q = bdev_get_queue(sb->s_bdev);
+             struct fstrim_range range;
+             int ret = 0;

fs/fat/file.c +24 -0
+
+             if (!capable(CAP_SYS_ADMIN))
+                     return -EPERM;
+
+             if (!blk_queue_discard(q))
+                     return -EOPNOTSUPP;
+
+             if (copy_from_user(&range, (struct fstrim_range *)arg,
+                                     sizeof(range)))
+                     return -EFAULT;
+
+             range.minlen = max((unsigned int)range.minlen,
+                                     q->limits.discard_granularity);
+             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 */ }

--

author
#4 lore raw
Quoted 93 lines · Lukas Czerner
Quoted 52 lines · Kyungmin Park
Quoted 51 lines · Lukas Czerner
Quoted 48 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."

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

Clearify the meaning of "len" (Cited form Lukas mail)

Let the "O" be free (bytes, blocks, whatever), and "=" be used. Now, we have a filesystem like this.

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                                      ^
  0                                      40

This is how it supposed to wotk if you have called FITIRM with parameters:

start = 0
minlen = 2
len = 20

So you will go through (blocks, bytes...) 0 -> 20

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                   ^
  0                   20

So, you will call discard on extents:

0-3
You'll skip 6 because is smaller than minlen
10-11
15-19

instead of

0-3
10-11
15-19
30-36

Quoted 5 lines
Quoted 2 lines

Hi thanks for the next version. And again I have to ask: Did you test it ? and how ? Did you tried xfstest No. 251 ? Couple of comments bellow.

I tested it with your test program. Of course I modified for our environment (eMMC).

Ok, good.

Quoted 31 lines

#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdint.h>
#include <sys/ioctl.h>

struct fstrim_range {
        uint64_t start;
        uint64_t len;
        uint64_t minlen;
};

#define FITRIM          _IOWR('X', 121, struct fstrim_range)

int main(int argc, char **argv)
{
        struct fstrim_range range;
        uint64_t len;
        int fd;

        if (argc < 2) {
                fprintf(stderr, "usage: %s mountpoint [size]\n", argv[0]); return 1;
        }

        if (argc == 3)
                len = atoll(argv[1]);
        else
                len = ((1UL<<31) - 1);

Quoted 5 lines
Quoted 3 lines

        range.start = 0;
        range.len = len;
        range.minlen = 256 * 1024;      /* Minimum is 256KiB */

Why exactly you need to set this ? What will happen if the minlen is 0 ?

It's dependent on eMMC chip. it's for our environment. If it passed with 0, the code is working but the less than 256KiB trim command is meaningless.

Quoted 178 lines
Quoted 144 lines

        fd = open(argv[1], O_RDONLY);
        if (fd < 0) {
                perror("open");
                return 1;
        }

        if (ioctl(fd, FITRIM, &range)) {
                if (errno == EOPNOTSUPP) fprintf(stderr, "TRIM not supported\n"); else
                        perror("FITRIM");
                return 1;
        }

        fprintf(stderr, "Trimmed size %llu\n", range.len);

        return 0;
}

Quoted 111 lines

Thanks!
-Lukas

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

---
Changelog v3:
      Adjust the minlen from queue discard_granularity Use the corrent len usage
Changelog v2:
      Use the given start and len as Lukas comments Check the queue supports discard feature
---

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 +51 -5
index b47d2c9..a8e3837 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;
  }
 
+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);
+}
+
  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,88 @@ out:
       unlock_fat(sbi);
       return err;
  }
+
+int fat_trim_fs(struct super_block *sb, struct fstrim_range *range)
+{
+     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, count, entry;
+     int start, len, minlen, trimmed;
+
+     start = range->start >> sb->s_blocksize_bits;
+     start = start / sbi->sec_per_clus;
+     len = range->len >> sb->s_blocksize_bits;
+     len = len / sbi->sec_per_clus;
+     minlen = range->minlen >> sb->s_blocksize_bits;
+     minlen = minlen / sbi->sec_per_clus;
+     trimmed = 0;
+     count = 0;
+
+     lock_fat(sbi);
+     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;
+
+     entry = 0;
+     free = 0;
+     fatent_init(&fatent);
+
+     if (start < FAT_START_ENT)
+             start = FAT_START_ENT;
+
+     fatent_set_entry(&fatent, start);
+
+     while (count < sbi->max_cluster) {
+             if (fatent.entry >= sbi->max_cluster)
Quoted 8 lines

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

Quoted 5 lines
fs/fat/fatent.c +5 -0
+                     fatent.entry = FAT_START_ENT;
+             /* 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));

You really do not need new variable "rest" just for passing it into one function. Get rid of it.

Umm. I don't want to modify it since it's routine is same as free_count codes. I just borrowed it from FAT codes.

Quoted 1 line
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
fs/fat/fatent.c +7 -0
+             }
+             cur_block++;
+
+             err = fat_ent_read_block(sb, &fatent);
+             if (err)
+                     goto out;
+
Quoted 8 lines

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

Quoted 8 lines
fs/fat/fatent.c +8 -0
+             do {
+                     if (ops->ent_get(&fatent) == FAT_ENT_FREE) {
+                             free++;
+                             if (!entry)
+                                     entry = fatent.entry;
+                             if (count >= len && free >= minlen) {
+                                     fat_issue_discard(sb, entry, free);
+                                     trimmed += free;

I think you can remove this condition completely is you move "done" label befor the if (free >= minlen) condition.

Quoted 4 lines
Quoted 2 lines

I really do not understand FAT code very much, but is this right ? Should not you be setting free = 0 ?

Right but count is larger then "len". it will exit at next if (count

Quoted 1 line

= len) goto done statement.

Oh, ok I see. But it proves my point that this code is not very well readable.

Quoted 7 lines
Quoted 7 lines

What will happen if you'll end up
in the same branch in next iteration ? -- free will be still set to previous value+1, bu you'll be discarding next entry. I am sorry but this whole thing is not very readable.

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

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

Quoted 3 lines
fs/fat/fatent.c +3 -0
+                             }
+                             if (count >= len)
+                                     goto done;

                                  ^^^^^^^^^^^^^^^^^^ this

Okay I will fix it.

Quoted 92 lines
Quoted 8 lines
Quoted 8 lines

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

Quoted 8 lines
fs/fat/fatent.c +7 -0
+                     } else if (entry) {
+                             if (free >= minlen) {
+                                     fat_issue_discard(sb, entry, free);
+                                     trimmed += free;
+                             }
+                             if (count >= len)
+                                     goto done;

                                  ^^^^^^^^^^^^^^^^^^ and this

Quoted 8 lines
Quoted 7 lines

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

Quoted 3 lines
fs/fat/fatent.c +3 -0
+                             free = 0;
+                             entry = 0;
+                     }

I don't not see why you are testing count all the time since it has not been changed since the "if" condition started. how about doing one test before the "if" condition ?

Quoted 8 lines

For looping the whole fat max_cluster size. now start can be any address and if start is a middle point and len is max, then it will search from middle -> end -> start -> middle. The "count" is used for this purpose.

Thank you,
Kyungmin Park

Quoted 1 line

can go here -->>
                           if (count >= len)
                                   goto done;

Quoted 51 lines
Quoted 51 lines
Quoted 51 lines
fs/fat/fatent.c +14 -0
+                     count++;
+             } while (fat_ent_next(sbi, &fatent));
+     }
+     if (free >= minlen) {
+             fat_issue_discard(sb, entry, free);
+             trimmed += free;
+     }
+done:
+     range->len = (trimmed * sbi->sec_per_clus) << sb->s_blocksize_bits;
+     fatent_brelse(&fatent);
+out:
+     unlock_fat(sbi);
+     return err;
+}
fs/fat/file.c +0 -0
index 7257752..9910aba 100644
@@ -125,6 +125,36 @@ 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 request_queue *q = bdev_get_queue(sb->s_bdev);
+             struct fstrim_range range;
+             int ret = 0;

fs/fat/file.c +23 -0
+
+             if (!capable(CAP_SYS_ADMIN))
+                     return -EPERM;
+
+             if (!blk_queue_discard(q))
+                     return -EOPNOTSUPP;
+
+             if (copy_from_user(&range, (struct fstrim_range *)arg,
+                                     sizeof(range)))
+                     return -EFAULT;
+
+             range.minlen = max((unsigned int)range.minlen,
+                                     q->limits.discard_granularity);
+             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 8 lines
Quoted 5 lines

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

--

Quoted 1 line

--

--
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
#5 lore raw
Quoted 282 lines · Kyungmin Park
Quoted 93 lines · Lukas Czerner
Quoted 52 lines · Kyungmin Park
Quoted 51 lines · Lukas Czerner
Quoted 48 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."

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

Clearify the meaning of "len" (Cited form Lukas mail)

Let the "O" be free (bytes, blocks, whatever), and "=" be used. Now, we have a filesystem like this.

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                                      ^
  0                                      40

This is how it supposed to wotk if you have called FITIRM with parameters:

start = 0
minlen = 2
len = 20

So you will go through (blocks, bytes...) 0 -> 20

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                   ^
  0                   20

So, you will call discard on extents:

0-3
You'll skip 6 because is smaller than minlen
10-11
15-19

instead of

0-3
10-11
15-19
30-36

Quoted 5 lines
Quoted 2 lines

Hi thanks for the next version. And again I have to ask: Did you test it ? and how ? Did you tried xfstest No. 251 ? Couple of comments bellow.

I tested it with your test program. Of course I modified for our environment (eMMC).

Ok, good.

Quoted 31 lines

#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdint.h>
#include <sys/ioctl.h>

struct fstrim_range {
        uint64_t start;
        uint64_t len;
        uint64_t minlen;
};

#define FITRIM          _IOWR('X', 121, struct fstrim_range)

int main(int argc, char **argv)
{
        struct fstrim_range range;
        uint64_t len;
        int fd;

        if (argc < 2) {
                fprintf(stderr, "usage: %s mountpoint [size]\n", argv[0]); return 1;
        }

        if (argc == 3)
                len = atoll(argv[1]);
        else
                len = ((1UL<<31) - 1);

Quoted 5 lines
Quoted 3 lines

        range.start = 0;
        range.len = len;
        range.minlen = 256 * 1024;      /* Minimum is 256KiB */

Why exactly you need to set this ? What will happen if the minlen is 0 ?

It's dependent on eMMC chip. it's for our environment. If it passed with 0, the code is working but the less than 256KiB trim command is meaningless.

Quoted 178 lines
Quoted 144 lines

        fd = open(argv[1], O_RDONLY);
        if (fd < 0) {
                perror("open");
                return 1;
        }

        if (ioctl(fd, FITRIM, &range)) {
                if (errno == EOPNOTSUPP) fprintf(stderr, "TRIM not supported\n"); else
                        perror("FITRIM");
                return 1;
        }

        fprintf(stderr, "Trimmed size %llu\n", range.len);

        return 0;
}

Quoted 111 lines

Thanks!
-Lukas

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

---
Changelog v3:
      Adjust the minlen from queue discard_granularity Use the corrent len usage
Changelog v2:
      Use the given start and len as Lukas comments Check the queue supports discard feature
---

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 +51 -5
index b47d2c9..a8e3837 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;
  }
 
+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);
+}
+
  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,88 @@ out:
       unlock_fat(sbi);
       return err;
  }
+
+int fat_trim_fs(struct super_block *sb, struct fstrim_range *range)
+{
+     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, count, entry;
+     int start, len, minlen, trimmed;
+
+     start = range->start >> sb->s_blocksize_bits;
+     start = start / sbi->sec_per_clus;
+     len = range->len >> sb->s_blocksize_bits;
+     len = len / sbi->sec_per_clus;
+     minlen = range->minlen >> sb->s_blocksize_bits;
+     minlen = minlen / sbi->sec_per_clus;
+     trimmed = 0;
+     count = 0;
+
+     lock_fat(sbi);
+     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;
+
+     entry = 0;
+     free = 0;
+     fatent_init(&fatent);
+
+     if (start < FAT_START_ENT)
+             start = FAT_START_ENT;
+
+     fatent_set_entry(&fatent, start);
+
+     while (count < sbi->max_cluster) {
+             if (fatent.entry >= sbi->max_cluster)
Quoted 8 lines

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

Quoted 5 lines
fs/fat/fatent.c +5 -0
+                     fatent.entry = FAT_START_ENT;
+             /* 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));

You really do not need new variable "rest" just for passing it into one function. Get rid of it.

Umm. I don't want to modify it since it's routine is same as free_count codes. I just borrowed it from FAT codes.

Quoted 1 line
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
fs/fat/fatent.c +7 -0
+             }
+             cur_block++;
+
+             err = fat_ent_read_block(sb, &fatent);
+             if (err)
+                     goto out;
+
Quoted 8 lines

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

Quoted 8 lines
fs/fat/fatent.c +8 -0
+             do {
+                     if (ops->ent_get(&fatent) == FAT_ENT_FREE) {
+                             free++;
+                             if (!entry)
+                                     entry = fatent.entry;
+                             if (count >= len && free >= minlen) {
+                                     fat_issue_discard(sb, entry, free);
+                                     trimmed += free;

I think you can remove this condition completely is you move "done" label befor the if (free >= minlen) condition.

Quoted 4 lines
Quoted 2 lines

I really do not understand FAT code very much, but is this right ? Should not you be setting free = 0 ?

Right but count is larger then "len". it will exit at next if (count

Quoted 1 line

= len) goto done statement.

Oh, ok I see. But it proves my point that this code is not very well readable.

Quoted 7 lines
Quoted 7 lines

What will happen if you'll end up
in the same branch in next iteration ? -- free will be still set to previous value+1, bu you'll be discarding next entry. I am sorry but this whole thing is not very readable.

Quoted 41 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

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

Quoted 3 lines
fs/fat/fatent.c +3 -0
+                             }
+                             if (count >= len)
+                                     goto done;

                                  ^^^^^^^^^^^^^^^^^^ this

Okay I will fix it.

Quoted 34 lines
Quoted 8 lines
Quoted 8 lines

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

Quoted 8 lines
fs/fat/fatent.c +7 -0
+                     } else if (entry) {
+                             if (free >= minlen) {
+                                     fat_issue_discard(sb, entry, free);
+                                     trimmed += free;
+                             }
+                             if (count >= len)
+                                     goto done;

                                  ^^^^^^^^^^^^^^^^^^ and this

Quoted 8 lines
Quoted 7 lines

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

Quoted 3 lines
fs/fat/fatent.c +3 -0
+                             free = 0;
+                             entry = 0;
+                     }

I don't not see why you are testing count all the time since it has not been changed since the "if" condition started. how about doing one test before the "if" condition ?

Quoted 8 lines

For looping the whole fat max_cluster size. now start can be any address and if start is a middle point and len is max, then it will search from middle -> end -> start -> middle. The "count" is used for this purpose.

Thank you,
Kyungmin Park

Quoted 1 line

can go here -->>
                           if (count >= len)
                                   goto done;

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
fs/fat/fatent.c +3 -0
+                     count++;
+             } while (fat_ent_next(sbi, &fatent));
+     }

How about this code?

        while (count < sbi->max_cluster) {
                if (fatent.entry >= sbi->max_cluster)
                        fatent.entry = FAT_START_ENT;
                /* 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) {
                                /*
                                 * Discard if free entry is equal or greater
                                 * than minimum length
                                 */
                                if (free >= minlen) {
                                        fat_issue_discard(sb, entry, free);
                                        trimmed += free;
                                }
                                free = 0;
                                entry = 0;
                        }
                        count++;
                        /* Check the loop count */
                        if (count >= len)
                                goto done;
                } while (fat_ent_next(sbi, &fatent));
        }
done:
        if (free >= minlen) {
                fat_issue_discard(sb, entry, free);
                trimmed += free;
        }
        range->len = (trimmed * sbi->sec_per_clus) << sb->s_blocksize_bits;
        fatent_brelse(&fatent);
out:
        unlock_fat(sbi);
        return err;
Quoted 58 lines
Quoted 58 lines
Quoted 57 lines
Quoted 56 lines
Quoted 49 lines
fs/fat/fatent.c +11 -0
+     if (free >= minlen) {
+             fat_issue_discard(sb, entry, free);
+             trimmed += free;
+     }
+done:
+     range->len = (trimmed * sbi->sec_per_clus) << sb->s_blocksize_bits;
+     fatent_brelse(&fatent);
+out:
+     unlock_fat(sbi);
+     return err;
+}
fs/fat/file.c +0 -0
index 7257752..9910aba 100644
@@ -125,6 +125,36 @@ 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 request_queue *q = bdev_get_queue(sb->s_bdev);
+             struct fstrim_range range;
+             int ret = 0;

fs/fat/file.c +24 -0
+
+             if (!capable(CAP_SYS_ADMIN))
+                     return -EPERM;
+
+             if (!blk_queue_discard(q))
+                     return -EOPNOTSUPP;
+
+             if (copy_from_user(&range, (struct fstrim_range *)arg,
+                                     sizeof(range)))
+                     return -EFAULT;
+
+             range.minlen = max((unsigned int)range.minlen,
+                                     q->limits.discard_granularity);
+             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 2 lines
Quoted 1 line

--

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

#6 lore raw
Quoted 99 lines · Kyungmin Park
Quoted 98 lines · Kyungmin Park
Quoted 97 lines · Lukas Czerner
Quoted 52 lines · Kyungmin Park
Quoted 51 lines · Lukas Czerner
Quoted 48 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."

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

Clearify the meaning of "len" (Cited form Lukas mail)

Let the "O" be free (bytes, blocks, whatever), and "=" be used. Now, we have a filesystem like this.

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                                      ^
  0                                      40

This is how it supposed to wotk if you have called FITIRM with parameters:

start = 0
minlen = 2
len = 20

So you will go through (blocks, bytes...) 0 -> 20

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                   ^
  0                   20

So, you will call discard on extents:

0-3
You'll skip 6 because is smaller than minlen
10-11
15-19

instead of

0-3
10-11
15-19
30-36

Quoted 5 lines
Quoted 2 lines

Hi thanks for the next version. And again I have to ask: Did you test it ? and how ? Did you tried xfstest No. 251 ? Couple of comments bellow.

I tested it with your test program. Of course I modified for our environment (eMMC).

Ok, good.

Quoted 34 lines

#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdint.h>
#include <sys/ioctl.h>

struct fstrim_range {
        uint64_t start;
        uint64_t len;
        uint64_t minlen;
};

#define FITRIM          _IOWR('X', 121, struct fstrim_range)

int main(int argc, char **argv)
{
        struct fstrim_range range;
        uint64_t len;
        int fd;

        if (argc < 2) {
                fprintf(stderr, "usage: %s mountpoint [size]\n", argv[0]); return 1;
        }

        if (argc == 3)
                len = atoll(argv[1]);
        else
                len = ((1UL<<31) - 1);

        range.start = 0;
        range.len = len;
        range.minlen = 256 * 1024;      /* Minimum is 256KiB */

Quoted 5 lines
Quoted 5 lines
Quoted 1 line

Why exactly you need to set this ? What will happen if the minlen is 0 ?

It's dependent on eMMC chip. it's for our environment. If it passed with 0, the code is working but the less than 256KiB trim command is meaningless.

Ok but user would not care, actually he would not even know so it is good that this will work, but if kernel knows what minlen value is meaningless maybe you should adjust that properly the same way as you are adjusting according to the discard_granularity ?

..snip..

Quoted 19 lines

How about this code?

        while (count < sbi->max_cluster) {
                if (fatent.entry >= sbi->max_cluster)
                        fatent.entry = FAT_START_ENT;
                /* 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) {
Quoted 8 lines
                                free++;
                                if (!entry)
                                        entry = fatent.entry;
                        } else if (entry) {
                                /*
                                 * Discard if free entry is equal or greater
                                 * than minimum length
                                 */

This comment states what we can already see in the simple condition, so it is not needed, but rather comment while() and do..while loops. But as I said I am not familiar with FAT code so that might be just my fault.

Quoted 9 lines
                                if (free >= minlen) {
                                        fat_issue_discard(sb, entry, free);
                                        trimmed += free;
                                }
                                free = 0;
                                entry = 0;
                        }
                        count++;
                        /* Check the loop count */

same thing with this comment, we can see that in the simple condition.

Quoted 6 lines
                        if (count >= len)
                                goto done;
                } while (fat_ent_next(sbi, &fatent));
        }
done:
        if (free >= minlen) {
Quoted 9 lines
                fat_issue_discard(sb, entry, free);
                trimmed += free;
        }
        range->len = (trimmed * sbi->sec_per_clus) << sb->s_blocksize_bits;
        fatent_brelse(&fatent);
out:
        unlock_fat(sbi);
        return err;

It looks better, thanks for your work!

-Lukas

..snip..

author
#7 lore raw
Quoted 103 lines · Lukas Czerner
Quoted 101 lines · Kyungmin Park
Quoted 100 lines · Kyungmin Park
Quoted 98 lines · Lukas Czerner
Quoted 52 lines · Kyungmin Park
Quoted 51 lines · Lukas Czerner
Quoted 48 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."

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

Clearify the meaning of "len" (Cited form Lukas mail)

Let the "O" be free (bytes, blocks, whatever), and "=" be used. Now, we have a filesystem like this.

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                                      ^
  0                                      40

This is how it supposed to wotk if you have called FITIRM with parameters:

start = 0
minlen = 2
len = 20

So you will go through (blocks, bytes...) 0 -> 20

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                   ^
  0                   20

So, you will call discard on extents:

0-3
You'll skip 6 because is smaller than minlen
10-11
15-19

instead of

0-3
10-11
15-19
30-36

Quoted 5 lines
Quoted 2 lines

Hi thanks for the next version. And again I have to ask: Did you test it ? and how ? Did you tried xfstest No. 251 ? Couple of comments bellow.

I tested it with your test program. Of course I modified for our environment (eMMC).

Ok, good.

Quoted 26 lines

#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdint.h>
#include <sys/ioctl.h>

struct fstrim_range {
        uint64_t start;
        uint64_t len;
        uint64_t minlen;
};

#define FITRIM          _IOWR('X', 121, struct fstrim_range)

int main(int argc, char **argv)
{
        struct fstrim_range range;
        uint64_t len;
        int fd;

        if (argc < 2) {
                fprintf(stderr, "usage: %s mountpoint [size]\n", argv[0]); return 1;
        }

Quoted 8 lines

        if (argc == 3)
                len = atoll(argv[1]);
        else
                len = ((1UL<<31) - 1);

        range.start = 0;
        range.len = len;
        range.minlen = 256 * 1024;      /* Minimum is 256KiB */

Why exactly you need to set this ? What will happen if the minlen is 0 ?

Quoted 8 lines
Quoted 3 lines
Quoted 3 lines

It's dependent on eMMC chip. it's for our environment. If it passed with 0, the code is working but the less than 256KiB trim command is meaningless.

Ok but user would not care, actually he would not even know so it is good that this will work, but if kernel knows what minlen value is meaningless maybe you should adjust that properly the same way as you are adjusting according to the discard_granularity ?

It's out of this patch, it will be handled each underlying devices, e.g., MMC and ATA.

Quoted 31 lines

..snip..

Quoted 17 lines

How about this code?

        while (count < sbi->max_cluster) {
                if (fatent.entry >= sbi->max_cluster) fatent.entry = FAT_START_ENT;
                /* 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;

Quoted 10 lines

                do {
                        if (ops->ent_get(&fatent) == FAT_ENT_FREE) { free++;
                                if (!entry)
                                        entry = fatent.entry;
                        } else if (entry) {
                                /*
                                 * Discard if free entry is equal or greater
                                 * than minimum length
                                 */

Quoted 3 lines

This comment states what we can already see in the simple condition, so it is not needed, but rather comment while() and do..while loops. But as I said I am not familiar with FAT code so that might be just my fault.

No problem I will delete it.

Quoted 11 lines
Quoted 10 lines

                                if (free >= minlen) { fat_issue_discard(sb, entry, free); trimmed += free;
                                }
                                free = 0;
                                entry = 0;
                        }
                        count++;
                        /* Check the loop count */

same thing with this comment, we can see that in the simple condition.

I'll delete it.

Quoted 15 lines
Quoted 6 lines

                        if (count >= len) goto done;
                } while (fat_ent_next(sbi, &fatent)); }
done:
        if (free >= minlen) {

Quoted 9 lines

                fat_issue_discard(sb, entry, free);
                trimmed += free;
        }
        range->len = (trimmed * sbi->sec_per_clus) << sb->s_blocksize_bits; fatent_brelse(&fatent);
out:
        unlock_fat(sbi);
        return err;

Quoted 1 line

It looks better, thanks for your work!

I'll resend it.

Thank you,
Kyungmin Park

Quoted 5 lines

-Lukas

..snip..