[PATCH 1/2] ext4: Notify when discard is not supported

[PATCH 1/2] ext4: Notify when discard is not supported

[Lukas Czerner]

Notify user when mounting the file system with -o discard option, but
the device does not support discard. Obviously we do not want to fail
the mount or disable the options, because the underlying device might
change in future even without file system remount.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/super.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7265a03..fd3ff41 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4017,6 +4017,14 @@ no_journal:
 	}
 #endif  /* CONFIG_QUOTA */
 
+	if (test_opt(sb, DISCARD)) {
+		struct request_queue *q = bdev_get_queue(sb->s_bdev);
+		if (!blk_queue_discard(q))
+			ext4_msg(sb, KERN_WARNING,
+				 "mounting with \"discard\" option, but "
+				 "the device does not support discard");
+	}
+
 	ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
 		 "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
 		 *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
-- 
1.7.7.6

[PATCH 2/2] ext4: Warn when discard request fails other than EOPNOTSUPP

[Lukas Czerner]

We should warn user then the discard request fails. However we need to
exclude -EOPNOTSUPP case since parts of the device might not support it
while other parts can. So print the kernel warning when the error !=
-EOPNOTSUPP is returned from ext4_issue_discard().

We should also handle error cases in batched discard, again excluding
EOPNOTSUPP.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/mballoc.c |   47 +++++++++++++++++++++++++++++++++++------------
 1 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index f8b27bf..cbab0c8 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2607,9 +2607,17 @@ static void ext4_free_data_callback(struct super_block *sb,
 	mb_debug(1, "gonna free %u blocks in group %u (0x%p):",
 		 entry->efd_count, entry->efd_group, entry);
 
-	if (test_opt(sb, DISCARD))
-		ext4_issue_discard(sb, entry->efd_group,
-				   entry->efd_start_cluster, entry->efd_count);
+	if (test_opt(sb, DISCARD)) {
+		err = ext4_issue_discard(sb, entry->efd_group,
+					 entry->efd_start_cluster,
+					 entry->efd_count);
+		if (err && err != -EOPNOTSUPP)
+			ext4_msg(sb, KERN_WARNING, "discard request in"
+				 " group:%d block:%d count:%d failed"
+				 " with %d", entry->efd_group,
+				 entry->efd_start_cluster,
+				 entry->efd_count, err);
+	}
 
 	err = ext4_mb_load_buddy(sb, entry->efd_group, &e4b);
 	/* we expect to find existing buddy because it's pinned */
@@ -4657,8 +4665,16 @@ do_more:
 		 * with group lock held. generate_buddy look at
 		 * them with group lock_held
 		 */
-		if (test_opt(sb, DISCARD))
-			ext4_issue_discard(sb, block_group, bit, count);
+		if (test_opt(sb, DISCARD)) {
+			err = ext4_issue_discard(sb, block_group, bit, count);
+			if (err && err != -EOPNOTSUPP)
+				ext4_msg(sb, KERN_WARNING, "discard request in"
+					 " group:%d block:%d count:%lu failed"
+					 " with %d", block_group, bit, count,
+					 err);
+		}
+
+
 		ext4_lock_group(sb, block_group);
 		mb_clear_bits(bitmap_bh->b_data, bit, count_clusters);
 		mb_free_blocks(inode, &e4b, bit, count_clusters);
@@ -4854,10 +4870,11 @@ error_return:
  * one will allocate those blocks, mark it as used in buddy bitmap. This must
  * be called with under the group lock.
  */
-static void ext4_trim_extent(struct super_block *sb, int start, int count,
+static int ext4_trim_extent(struct super_block *sb, int start, int count,
 			     ext4_group_t group, struct ext4_buddy *e4b)
 {
 	struct ext4_free_extent ex;
+	int ret = 0;
 
 	trace_ext4_trim_extent(sb, group, start, count);
 
@@ -4873,9 +4890,10 @@ static void ext4_trim_extent(struct super_block *sb, int start, int count,
 	 */
 	mb_mark_used(e4b, &ex);
 	ext4_unlock_group(sb, group);
-	ext4_issue_discard(sb, group, start, count);
+	ret = ext4_issue_discard(sb, group, start, count);
 	ext4_lock_group(sb, group);
 	mb_free_blocks(NULL, e4b, start, ex.fe_len);
+	return ret;
 }
 
 /**
@@ -4904,7 +4922,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
 	void *bitmap;
 	ext4_grpblk_t next, count = 0, free_count = 0;
 	struct ext4_buddy e4b;
-	int ret;
+	int ret = 0;
 
 	trace_ext4_trim_all_free(sb, group, start, max);
 
@@ -4931,8 +4949,11 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
 		next = mb_find_next_bit(bitmap, max + 1, start);
 
 		if ((next - start) >= minblocks) {
-			ext4_trim_extent(sb, start,
-					 next - start, group, &e4b);
+			ret = ext4_trim_extent(sb, start,
+					       next - start, group, &e4b);
+			if (ret && ret != -EOPNOTSUPP)
+				break;
+			ret = 0;
 			count += next - start;
 		}
 		free_count += next - start;
@@ -4953,8 +4974,10 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
 			break;
 	}
 
-	if (!ret)
+	if (!ret) {
+		ret = count;
 		EXT4_MB_GRP_SET_TRIMMED(e4b.bd_info);
+	}
 out:
 	ext4_unlock_group(sb, group);
 	ext4_mb_unload_buddy(&e4b);
@@ -4962,7 +4985,7 @@ out:
 	ext4_debug("trimmed %d blocks in the group %d\n",
 		count, group);
 
-	return count;
+	return ret;
 }
 
 /**
-- 
1.7.7.6

Re: [PATCH 2/2] ext4: Warn when discard request fails other than EOPNOTSUPP

[Carlos Maiolino]

On Fri, Oct 19, 2012 at 02:12:38PM +0200, Lukas Czerner wrote:
> We should warn user then the discard request fails. However we need to
> exclude -EOPNOTSUPP case since parts of the device might not support it
> while other parts can. So print the kernel warning when the error !=
> -EOPNOTSUPP is returned from ext4_issue_discard().
> 
> We should also handle error cases in batched discard, again excluding
> EOPNOTSUPP.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  fs/ext4/mballoc.c |   47 +++++++++++++++++++++++++++++++++++------------
>  1 files changed, 35 insertions(+), 12 deletions(-)

Looks good

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
-- 
--Carlos

Re: [PATCH 2/2] ext4: Warn when discard request fails other than EOPNOTSUPP

[Lukáš Czerner]

On Fri, 19 Oct 2012, Lukas Czerner wrote:

> Date: Fri, 19 Oct 2012 14:12:38 +0200
> From: Lukas Czerner <lczerner@redhat.com>
> To: linux-ext4@vger.kernel.org
> Cc: tytso@mit.edu, Lukas Czerner <lczerner@redhat.com>
> Subject: [PATCH 2/2] ext4: Warn when discard request fails other than
>     EOPNOTSUPP
> 
> We should warn user then the discard request fails. However we need to
> exclude -EOPNOTSUPP case since parts of the device might not support it
> while other parts can. So print the kernel warning when the error !=
> -EOPNOTSUPP is returned from ext4_issue_discard().
> 
> We should also handle error cases in batched discard, again excluding
> EOPNOTSUPP.

ping to the series. Any comments here ?

Thanks!
-Lukas

> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  fs/ext4/mballoc.c |   47 +++++++++++++++++++++++++++++++++++------------
>  1 files changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index f8b27bf..cbab0c8 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2607,9 +2607,17 @@ static void ext4_free_data_callback(struct super_block *sb,
>  	mb_debug(1, "gonna free %u blocks in group %u (0x%p):",
>  		 entry->efd_count, entry->efd_group, entry);
>  
> -	if (test_opt(sb, DISCARD))
> -		ext4_issue_discard(sb, entry->efd_group,
> -				   entry->efd_start_cluster, entry->efd_count);
> +	if (test_opt(sb, DISCARD)) {
> +		err = ext4_issue_discard(sb, entry->efd_group,
> +					 entry->efd_start_cluster,
> +					 entry->efd_count);
> +		if (err && err != -EOPNOTSUPP)
> +			ext4_msg(sb, KERN_WARNING, "discard request in"
> +				 " group:%d block:%d count:%d failed"
> +				 " with %d", entry->efd_group,
> +				 entry->efd_start_cluster,
> +				 entry->efd_count, err);
> +	}
>  
>  	err = ext4_mb_load_buddy(sb, entry->efd_group, &e4b);
>  	/* we expect to find existing buddy because it's pinned */
> @@ -4657,8 +4665,16 @@ do_more:
>  		 * with group lock held. generate_buddy look at
>  		 * them with group lock_held
>  		 */
> -		if (test_opt(sb, DISCARD))
> -			ext4_issue_discard(sb, block_group, bit, count);
> +		if (test_opt(sb, DISCARD)) {
> +			err = ext4_issue_discard(sb, block_group, bit, count);
> +			if (err && err != -EOPNOTSUPP)
> +				ext4_msg(sb, KERN_WARNING, "discard request in"
> +					 " group:%d block:%d count:%lu failed"
> +					 " with %d", block_group, bit, count,
> +					 err);
> +		}
> +
> +
>  		ext4_lock_group(sb, block_group);
>  		mb_clear_bits(bitmap_bh->b_data, bit, count_clusters);
>  		mb_free_blocks(inode, &e4b, bit, count_clusters);
> @@ -4854,10 +4870,11 @@ error_return:
>   * one will allocate those blocks, mark it as used in buddy bitmap. This must
>   * be called with under the group lock.
>   */
> -static void ext4_trim_extent(struct super_block *sb, int start, int count,
> +static int ext4_trim_extent(struct super_block *sb, int start, int count,
>  			     ext4_group_t group, struct ext4_buddy *e4b)
>  {
>  	struct ext4_free_extent ex;
> +	int ret = 0;
>  
>  	trace_ext4_trim_extent(sb, group, start, count);
>  
> @@ -4873,9 +4890,10 @@ static void ext4_trim_extent(struct super_block *sb, int start, int count,
>  	 */
>  	mb_mark_used(e4b, &ex);
>  	ext4_unlock_group(sb, group);
> -	ext4_issue_discard(sb, group, start, count);
> +	ret = ext4_issue_discard(sb, group, start, count);
>  	ext4_lock_group(sb, group);
>  	mb_free_blocks(NULL, e4b, start, ex.fe_len);
> +	return ret;
>  }
>  
>  /**
> @@ -4904,7 +4922,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
>  	void *bitmap;
>  	ext4_grpblk_t next, count = 0, free_count = 0;
>  	struct ext4_buddy e4b;
> -	int ret;
> +	int ret = 0;
>  
>  	trace_ext4_trim_all_free(sb, group, start, max);
>  
> @@ -4931,8 +4949,11 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
>  		next = mb_find_next_bit(bitmap, max + 1, start);
>  
>  		if ((next - start) >= minblocks) {
> -			ext4_trim_extent(sb, start,
> -					 next - start, group, &e4b);
> +			ret = ext4_trim_extent(sb, start,
> +					       next - start, group, &e4b);
> +			if (ret && ret != -EOPNOTSUPP)
> +				break;
> +			ret = 0;
>  			count += next - start;
>  		}
>  		free_count += next - start;
> @@ -4953,8 +4974,10 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
>  			break;
>  	}
>  
> -	if (!ret)
> +	if (!ret) {
> +		ret = count;
>  		EXT4_MB_GRP_SET_TRIMMED(e4b.bd_info);
> +	}
>  out:
>  	ext4_unlock_group(sb, group);
>  	ext4_mb_unload_buddy(&e4b);
> @@ -4962,7 +4985,7 @@ out:
>  	ext4_debug("trimmed %d blocks in the group %d\n",
>  		count, group);
>  
> -	return count;
> +	return ret;
>  }
>  
>  /**
> 

Re: [PATCH 2/2] ext4: Warn when discard request fails other than EOPNOTSUPP

[Theodore Ts'o]

On Fri, Oct 19, 2012 at 02:12:38PM +0200, Lukas Czerner wrote:
> We should warn user then the discard request fails. However we need to
> exclude -EOPNOTSUPP case since parts of the device might not support it
> while other parts can. So print the kernel warning when the error !=
> -EOPNOTSUPP is returned from ext4_issue_discard().
> 
> We should also handle error cases in batched discard, again excluding
> EOPNOTSUPP.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Thanks, applied.

						- Ted

Re: [PATCH 1/2] ext4: Notify when discard is not supported

[Carlos Maiolino]

On Fri, Oct 19, 2012 at 02:12:37PM +0200, Lukas Czerner wrote:
> Notify user when mounting the file system with -o discard option, but
> the device does not support discard. Obviously we do not want to fail
> the mount or disable the options, because the underlying device might
> change in future even without file system remount.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  fs/ext4/super.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 7265a03..fd3ff41 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4017,6 +4017,14 @@ no_journal:
>  	}
>  #endif  /* CONFIG_QUOTA */
>  
> +	if (test_opt(sb, DISCARD)) {
> +		struct request_queue *q = bdev_get_queue(sb->s_bdev);
> +		if (!blk_queue_discard(q))
> +			ext4_msg(sb, KERN_WARNING,
> +				 "mounting with \"discard\" option, but "
> +				 "the device does not support discard");
> +	}
> +
>  	ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
>  		 "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
>  		 *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Looks good to me

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
-- 
--Carlos

Re: [PATCH 1/2] ext4: Notify when discard is not supported

[Theodore Ts'o]

On Fri, Oct 19, 2012 at 02:12:37PM +0200, Lukas Czerner wrote:
> Notify user when mounting the file system with -o discard option, but
> the device does not support discard. Obviously we do not want to fail
> the mount or disable the options, because the underlying device might
> change in future even without file system remount.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Thanks, applied.

					- Ted