linux-ext4 - Re: [PATCH 1/2] ext4: remove while loop in 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: <20130621163236.GN13932@quack.suse.cz>
Date:	Fri, 21 Jun 2013 18:32:36 +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 1/2] ext4: remove while loop in
 mpage_map_and_submit_extent()

On Mon 17-06-13 09:58:07, Ted Tso wrote:
> The while loop in mpage_map_and_submit_extent() is pointless; this
> function is only called if mpd.map.m_len is non-zero, and at the end
> of the while loop, mpage_map_and_submit_buffers sets mpd.map.m_len to
> be zero on success.  (And on a failure, we return out of the while
> loop anyway.)
  This is not true. In case blocksize < pagesize and we were not able to
map a full page, mpage_map_and_submit_buffers() changes mpd->map to point
to the next extent inside the page that needs mapping. In that case we can
take several iterations of the loop. Maybe this fact deserves a comment
before the loop but there's a comment before the callsite of
mpage_map_and_submit_buffers() which mentions that it will get us next
extent to map and also the documentation of mpage_map_and_submit_buffers()
explicitely tells that it can update mpd->map.

								Honza

> Hence, the body of the while loop is guaranteed to execute once and
> exactly once.  This also fixes a gcc warning complaining that err
> might be left uninitialized.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@....edu>
> Cc: Jan Kara <jack@...e.cz>
> ---
>  fs/ext4/inode.c | 66 ++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 2644679..0e61543 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2157,44 +2157,42 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>  
>  	mpd->io_submit.io_end->offset =
>  				((loff_t)map->m_lblk) << inode->i_blkbits;
> -	while (map->m_len) {
> -		err = mpage_map_one_extent(handle, mpd);
> -		if (err < 0) {
> -			struct super_block *sb = inode->i_sb;
> +	err = mpage_map_one_extent(handle, mpd);
> +	if (err < 0) {
> +		struct super_block *sb = inode->i_sb;
>  
> -			/*
> -			 * 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);
> -			return err;
> -		}
>  		/*
> -		 * Update buffer state, submit mapped pages, and get us new
> -		 * extent to map
> +		 * Need to commit transaction to free blocks. Let upper
> +		 * layers sort it out.
>  		 */
> -		err = mpage_map_and_submit_buffers(mpd);
> -		if (err < 0)
> -			return err;
> +		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);
> +		return err;
>  	}
> +	/*
> +	 * Update buffer state, submit mapped pages, and get us new
> +	 * extent to map
> +	 */
> +	err = mpage_map_and_submit_buffers(mpd);
> +	if (err < 0)
> +		return err;
>  
>  	/* Update on-disk size after IO is submitted */
>  	disksize = ((loff_t)mpd->first_page) << PAGE_CACHE_SHIFT;
> -- 
> 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