[PATCH] fat: make discard a mount option

[PATCH] fat: make discard a mount option

[Christoph Hellwig]

Currently shipping discard capable SSDs and arrays have rather sub-optimal
implementations of the command and can the use of it can cause massive
slowdowns.  Make issueing these commands option as it's already in btrfs
and gfs2.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/fat/fat.h
===================================================================
--- linux-2.6.orig/fs/fat/fat.h	2009-11-18 13:47:49.380003970 +0100
+++ linux-2.6/fs/fat/fat.h	2009-11-18 13:48:13.169014622 +0100
@@ -44,7 +44,8 @@ struct fat_mount_options {
 		 nocase:1,	  /* Does this need case conversion? 0=need case conversion*/
 		 usefree:1,	  /* Use free_clusters for FAT32 */
 		 tz_utc:1,	  /* Filesystem timestamps are in UTC */
-		 rodir:1;	  /* allow ATTR_RO for directory */
+		 rodir:1,	  /* allow ATTR_RO for directory */
+		 discard:1;	  /* Issue discard requests on deletions */
 };
 
 #define FAT_HASH_BITS	8
Index: linux-2.6/fs/fat/fatent.c
===================================================================
--- linux-2.6.orig/fs/fat/fatent.c	2009-11-18 13:48:24.102254108 +0100
+++ linux-2.6/fs/fat/fatent.c	2009-11-18 13:49:14.338033708 +0100
@@ -573,8 +573,11 @@ int fat_free_clusters(struct inode *inod
 		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);
+			if (sbi->options.discard) {
+				sb_issue_discard(sb,
+					fat_clus_to_blknr(sbi, first_cl),
+					nr_clus * sbi->sec_per_clus);
+			}
 			first_cl = cluster;
 		}
 
Index: linux-2.6/fs/fat/inode.c
===================================================================
--- linux-2.6.orig/fs/fat/inode.c	2009-11-18 13:46:40.484256340 +0100
+++ linux-2.6/fs/fat/inode.c	2009-11-18 13:47:39.457256306 +0100
@@ -871,7 +871,7 @@ enum {
 	Opt_shortname_winnt, Opt_shortname_mixed, Opt_utf8_no, Opt_utf8_yes,
 	Opt_uni_xl_no, Opt_uni_xl_yes, Opt_nonumtail_no, Opt_nonumtail_yes,
 	Opt_obsolate, Opt_flush, Opt_tz_utc, Opt_rodir, Opt_err_cont,
-	Opt_err_panic, Opt_err_ro, Opt_err,
+	Opt_err_panic, Opt_err_ro, Opt_discard, Opt_err,
 };
 
 static const match_table_t fat_tokens = {
@@ -910,6 +910,7 @@ static const match_table_t fat_tokens = 
 	{Opt_obsolate, "cvf_format=%20s"},
 	{Opt_obsolate, "cvf_options=%100s"},
 	{Opt_obsolate, "posix"},
+	{Opt_discard, "discard"},
 	{Opt_err, NULL},
 };
 static const match_table_t msdos_tokens = {
@@ -1136,6 +1137,9 @@ static int parse_options(char *options, 
 		case Opt_rodir:
 			opts->rodir = 1;
 			break;
+		case Opt_discard:
+			opts->discard = 1;
+			break;
 
 		/* obsolete mount options */
 		case Opt_obsolate:

Re: [PATCH] fat: make discard a mount option

[OGAWA Hirofumi]

Christoph Hellwig <hch@lst.de> writes:

> Currently shipping discard capable SSDs and arrays have rather sub-optimal
> implementations of the command and can the use of it can cause massive
> slowdowns.  Make issueing these commands option as it's already in btrfs
> and gfs2.

Thanks. Looks good to me, I'll apply as is.

BTW, what value is default for those users? And from users of some
filesystems, it might be better to do in lower layer or VFS?

> Index: linux-2.6/fs/fat/fat.h
> ===================================================================
> --- linux-2.6.orig/fs/fat/fat.h	2009-11-18 13:47:49.380003970 +0100
> +++ linux-2.6/fs/fat/fat.h	2009-11-18 13:48:13.169014622 +0100
> @@ -44,7 +44,8 @@ struct fat_mount_options {
>  		 nocase:1,	  /* Does this need case conversion? 0=need case conversion*/
>  		 usefree:1,	  /* Use free_clusters for FAT32 */
>  		 tz_utc:1,	  /* Filesystem timestamps are in UTC */
> -		 rodir:1;	  /* allow ATTR_RO for directory */
> +		 rodir:1,	  /* allow ATTR_RO for directory */
> +		 discard:1;	  /* Issue discard requests on deletions */
>  };
>  
>  #define FAT_HASH_BITS	8
> Index: linux-2.6/fs/fat/fatent.c
> ===================================================================
> --- linux-2.6.orig/fs/fat/fatent.c	2009-11-18 13:48:24.102254108 +0100
> +++ linux-2.6/fs/fat/fatent.c	2009-11-18 13:49:14.338033708 +0100
> @@ -573,8 +573,11 @@ int fat_free_clusters(struct inode *inod
>  		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);
> +			if (sbi->options.discard) {
> +				sb_issue_discard(sb,
> +					fat_clus_to_blknr(sbi, first_cl),
> +					nr_clus * sbi->sec_per_clus);
> +			}
>  			first_cl = cluster;
>  		}
>  
> Index: linux-2.6/fs/fat/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/fat/inode.c	2009-11-18 13:46:40.484256340 +0100
> +++ linux-2.6/fs/fat/inode.c	2009-11-18 13:47:39.457256306 +0100
> @@ -871,7 +871,7 @@ enum {
>  	Opt_shortname_winnt, Opt_shortname_mixed, Opt_utf8_no, Opt_utf8_yes,
>  	Opt_uni_xl_no, Opt_uni_xl_yes, Opt_nonumtail_no, Opt_nonumtail_yes,
>  	Opt_obsolate, Opt_flush, Opt_tz_utc, Opt_rodir, Opt_err_cont,
> -	Opt_err_panic, Opt_err_ro, Opt_err,
> +	Opt_err_panic, Opt_err_ro, Opt_discard, Opt_err,
>  };
>  
>  static const match_table_t fat_tokens = {
> @@ -910,6 +910,7 @@ static const match_table_t fat_tokens = 
>  	{Opt_obsolate, "cvf_format=%20s"},
>  	{Opt_obsolate, "cvf_options=%100s"},
>  	{Opt_obsolate, "posix"},
> +	{Opt_discard, "discard"},
>  	{Opt_err, NULL},
>  };
>  static const match_table_t msdos_tokens = {
> @@ -1136,6 +1137,9 @@ static int parse_options(char *options, 
>  		case Opt_rodir:
>  			opts->rodir = 1;
>  			break;
> +		case Opt_discard:
> +			opts->discard = 1;
> +			break;
>  
>  		/* obsolete mount options */
>  		case Opt_obsolate:
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: make discard a mount option

[Christoph Hellwig]

On Thu, Nov 19, 2009 at 03:38:04AM +0900, OGAWA Hirofumi wrote:
> Christoph Hellwig <hch@lst.de> writes:
> 
> > Currently shipping discard capable SSDs and arrays have rather sub-optimal
> > implementations of the command and can the use of it can cause massive
> > slowdowns.  Make issueing these commands option as it's already in btrfs
> > and gfs2.
> 
> Thanks. Looks good to me, I'll apply as is.
> 
> BTW, what value is default for those users? And from users of some
> filesystems, it might be better to do in lower layer or VFS?

The situation is the following:

As of 2.6.32-rc7:

 - btrfs needs the discard option to enable trim, it's off by default
 - gfs2 needs the discard option to enable trim, it's off by default
 - ext4 does discard by default, no option to disable
 - fat does discard by default, no option to disable
 - the swap code does discard by default, no option to disable

Eric has a patch for ext4 to implement the btrfs/gfs2 semantics, and
this is the one for fat.  A discussion is ongoing about swap.

I don't think moving block specific options is a good idea to do in the
VFS.  The other option would be to disable it in the block layer, but
that's not alwas intuitive.

Re: [PATCH] fat: make discard a mount option

[OGAWA Hirofumi]

Christoph Hellwig <hch@lst.de> writes:

> On Thu, Nov 19, 2009 at 03:38:04AM +0900, OGAWA Hirofumi wrote:
>> Christoph Hellwig <hch@lst.de> writes:
>> 
>> > Currently shipping discard capable SSDs and arrays have rather sub-optimal
>> > implementations of the command and can the use of it can cause massive
>> > slowdowns.  Make issueing these commands option as it's already in btrfs
>> > and gfs2.
>> 
>> Thanks. Looks good to me, I'll apply as is.
>> 
>> BTW, what value is default for those users? And from users of some
>> filesystems, it might be better to do in lower layer or VFS?
>
> The situation is the following:
>
> As of 2.6.32-rc7:
>
>  - btrfs needs the discard option to enable trim, it's off by default
>  - gfs2 needs the discard option to enable trim, it's off by default
>  - ext4 does discard by default, no option to disable
>  - fat does discard by default, no option to disable
>  - the swap code does discard by default, no option to disable

Thanks. From it, I'll use the disable by default, if you don't have
problem.

> Eric has a patch for ext4 to implement the btrfs/gfs2 semantics, and
> this is the one for fat.  A discussion is ongoing about swap.
>
> I don't think moving block specific options is a good idea to do in the
> VFS.  The other option would be to disable it in the block layer, but
> that's not alwas intuitive.

Yes. But, on the other view, it is the capability of block device (trim
like stuff is useful on it, or not. Also it would be able to used for
blacklist from driver).

Well, actually, I'm caring the difference of default for those options
can confuse a bit for users.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: make discard a mount option

[Christoph Hellwig]

On Thu, Nov 19, 2009 at 04:09:42AM +0900, OGAWA Hirofumi wrote:
> Yes. But, on the other view, it is the capability of block device (trim
> like stuff is useful on it, or not. Also it would be able to used for
> blacklist from driver).

Yes, and discard works perfectly fine on e.g. the OCZ Vertex SSDs or
storage arrays.  It's just not fast enough for the trim on transaction
commit style support that was rushed into various filesystems.  It works
perfectly fine with Mark Lord's wiper.sh script, or with an updated
version of my XFS btree walking periodic discard.

> Well, actually, I'm caring the difference of default for those options
> can confuse a bit for users.

I think it should be off by default for everyone, and I'm trying to make
sure that's the case everywhere.