linux-kernel - Re: [PATCH] ext4: add error handling when discard cmd is fail in FITRIM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.00.1207301323550.23560@localhost>
Date:	Mon, 30 Jul 2012 13:31:39 +0200 (CEST)
From:	Lukáš Czerner <lczerner@...hat.com>
To:	Namjae Jeon <linkinjeon@...il.com>
cc:	tytso@....edu, sandeen@...hat.com, lczerner@...hat.com,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
	Amit Sahrawat <amit.sahrawat83@...il.com>
Subject: Re: [PATCH] ext4: add error handling when discard cmd is fail in
 FITRIM

On Sun, 29 Jul 2012, Namjae Jeon wrote:

> Date: Sun, 29 Jul 2012 07:31:54 -0400
> From: Namjae Jeon <linkinjeon@...il.com>
> To: tytso@....edu, sandeen@...hat.com, lczerner@...hat.com,
>     linux-ext4@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org, Namjae Jeon <linkinjeon@...il.com>,
>     Amit Sahrawat <amit.sahrawat83@...il.com>
> Subject: [PATCH] ext4: add error handling when discard cmd is fail in FITRIM
> 
> Although free extents is proper not trimmed(mmc driver return error code
> while sending trim command), currently FITRIM ioctl return success.
> Add exception routine to inform user error code.
> 
> #> ./fitrim_test
> end_request: I/O error, dev mmcblk0, sector 27232
> EXT4-fs warning (device mmcblk0): ext4_trim_all_free:4857:
> Discard command returned error -5
> #>

Well, by this change you're actually reverting commit

d9f34504e6952e909a6932c5b2d1857716606380
	 ext4: ignore errors when issuing discards

which effectively reverts a30eec2a8.

Now I think that the way it is now is actually better than your
proposal for the reasons mentioned in the commit
d9f34504e6952e909a6932c5b2d1857716606380. However I think that the
discard errors should be logged nevertheless but not at the file
system level, but rather on block layer level if it is not done
already.

Thanks!
-Lukas

> 
> Signed-off-by: Namjae Jeon <linkinjeon@...il.com>
> Signed-off-by: Amit Sahrawat <amit.sahrawat83@...il.com>
> Reviewed-by: Jan Kara <jack@...e.cz>
> ---
>  fs/ext4/mballoc.c |   25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 8eae947..07569b6 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4852,10 +4852,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 err;
>  
>  	trace_ext4_trim_extent(sb, group, start, count);
>  
> @@ -4871,9 +4872,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);
> +	err = ext4_issue_discard(sb, group, start, count);
>  	ext4_lock_group(sb, group);
>  	mb_free_blocks(NULL, e4b, start, ex.fe_len);
> +	return err;
>  }
>  
>  /**
> @@ -4902,7 +4904,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);
>  
> @@ -4929,15 +4931,22 @@ 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);
> -			count += next - start;
> +			ret = ext4_trim_extent(sb, start,
> +						next - start, group, &e4b);
> +			if (ret < 0) {
> +				if (ret != -EOPNOTSUPP)
> +					ext4_warning(sb,
> +						"Discard command returned error %d\n",
> +						ret);
> +					break;
> +			} else
> +				count += next - start;
>  		}
>  		free_count += next - start;
>  		start = next + 1;
>  
>  		if (fatal_signal_pending(current)) {
> -			count = -ERESTARTSYS;
> +			ret = -ERESTARTSYS;
>  			break;
>  		}
>  
> @@ -4960,7 +4969,7 @@ out:
>  	ext4_debug("trimmed %d blocks in the group %d\n",
>  		count, group);
>  
> -	return count;
> +	return (ret < 0) ? ret : count;
>  }
>  
>  /**
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/