linux-ext4 - Re: [PATCH 2/2] ext4: fix up error handling for mpage_map_and_submit

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: <20130621163809.GO13932@quack.suse.cz>
Date:	Fri, 21 Jun 2013 18:38:09 +0200
From:	Jan Kara <jack@...e.cz>
To:	Theodore Ts'o <tytso@....edu>
Cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>,
	Jan Kara <jack@...e.cz>
Subject: Re: [PATCH 2/2] ext4: fix up error handling for
 mpage_map_and_submit_extent()

On Mon 17-06-13 09:58:08, Ted Tso wrote:
> The function mpage_released_unused_page() must only be called once;
> otherwise the kernel will BUG() when the second call to
> mpage_released_unused_page() tries to unlock the pages which had been
> unlocked by the first call.
> 
> Also restructure the error handling so that we only give up on writing
> the dirty pages in the case of ENOSPC where retrying the allocation
> won't help.  Otherwise, a transient failure, such as a kmalloc()
> failure in calling ext4_map_blocks() might cause us to give up on
> those pages, leading to a scary message in /var/log/messages plus data
> loss.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@....edu>
> Cc: Jan Kara <jack@...e.cz>
  Yeah, this looks nicer. You can add:
Reviewed-by: Jan Kara <jack@...e.cz>

								Honza

> ---
>  fs/ext4/inode.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 0e61543..a183437 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2148,7 +2148,8 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
>   * guaranteed). After mapping we submit all mapped pages for IO.
>   */
>  static int mpage_map_and_submit_extent(handle_t *handle,
> -				       struct mpage_da_data *mpd)
> +				       struct mpage_da_data *mpd,
> +				       bool *give_up_on_write)
>  {
>  	struct inode *inode = mpd->inode;
>  	struct ext4_map_blocks *map = &mpd->map;
> @@ -2161,29 +2162,26 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>  	if (err < 0) {
>  		struct super_block *sb = inode->i_sb;
>  
> +		if (EXT4_SB(sb)->s_mount_flags & EXT4_MF_FS_ABORTED) {
> +			/* invalidate all the pages */
> +			*give_up_on_write = true;
> +			return err;
> +		}
> +		if ((err != -ENOSPC) || ext4_count_free_clusters(sb))
> +			return err;
>  		/*
>  		 * Need to commit transaction to free blocks. Let upper
>  		 * layers sort it out.
>  		 */
> -		if (err == -ENOSPC && ext4_count_free_clusters(sb))
> -			return -ENOSPC;
> -
> -		if (!(EXT4_SB(sb)->s_mount_flags & EXT4_MF_FS_ABORTED)) {
> -			ext4_msg(sb, KERN_CRIT,
> -				 "Delayed block allocation failed for "
> -				 "inode %lu at logical offset %llu with"
> -				 " max blocks %u with error %d",
> -				 inode->i_ino,
> -				 (unsigned long long)map->m_lblk,
> -				 (unsigned)map->m_len, err);
> -			ext4_msg(sb, KERN_CRIT,
> -				 "This should not happen!! Data will "
> -				 "be lost\n");
> -			if (err == -ENOSPC)
> -				ext4_print_free_blocks(inode);
> -		}
> -		/* invalidate all the pages */
> -		mpage_release_unused_pages(mpd, true);
> +		ext4_msg(sb, KERN_CRIT,
> +			 "Delayed block allocation failed for inode %lu at "
> +			 "logical offset %llu with max blocks %u",
> +			 inode->i_ino, (unsigned long long)map->m_lblk,
> +			 (unsigned)map->m_len);
> +		ext4_msg(sb, KERN_CRIT,
> +			 "This should not happen!! Data will be lost\n");
> +		ext4_print_free_blocks(inode);
> +		*give_up_on_write = true;  /* invalidate all the pages */
>  		return err;
>  	}
>  	/*
> @@ -2370,6 +2368,7 @@ static int ext4_writepages(struct address_space *mapping,
>  	struct ext4_sb_info *sbi = EXT4_SB(mapping->host->i_sb);
>  	bool done;
>  	struct blk_plug plug;
> +	bool give_up_on_write = false;
>  
>  	trace_ext4_writepages(inode, wbc);
>  
> @@ -2487,7 +2486,8 @@ retry:
>  		ret = mpage_prepare_extent_to_map(&mpd);
>  		if (!ret) {
>  			if (mpd.map.m_len)
> -				ret = mpage_map_and_submit_extent(handle, &mpd);
> +				ret = mpage_map_and_submit_extent(handle, &mpd,
> +					&give_up_on_write);
>  			else {
>  				/*
>  				 * We scanned the whole range (or exhausted
> @@ -2502,7 +2502,7 @@ retry:
>  		/* Submit prepared bio */
>  		ext4_io_submit(&mpd.io_submit);
>  		/* Unlock pages we didn't use */
> -		mpage_release_unused_pages(&mpd, false);
> +		mpage_release_unused_pages(&mpd, give_up_on_write);
>  		/* Drop our io_end reference we got from init */
>  		ext4_put_io_end(mpd.io_submit.io_end);
>  
> -- 
> 1.7.12.rc0.22.gcdd159b
> 
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html