[v2,1/5] ext4: improve ext4_es_can_be_merged() to avoid a potential overflow

From: Zheng Liu <wenqing.lz@taobao.com>

This commit tries to improve ext4_es_can_be_merged.  We check the length
of extent to avoid a potential overflow.

Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/extents_status.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

On Wed, Mar 06, 2013 at 10:17:11PM +0800, Zheng Liu wrote:
> +	if (ext4_es_status(es1) ^ ext4_es_status(es2))
>  		return 0;
>  
> -	if (ext4_es_status(es1) != ext4_es_status(es2))

Did you have a reason why changed != to ^?  

It's identical from a functional perspective, but it's less obvious to
future readers of the code what's going on.  I tried checking to see
if GCC did any better optimizing the code, but it doesn't seem to make
any difference.  I'm going to switch it back to !=....

> +	/* we need to check delayed extent is without unwritten status */
> +	if (ext4_es_is_delayed(es1) && !ext4_es_is_unwritten(es1))
> +		return 1;

I'm not sure why we need to check the unwritten status?  Under what
circumstances would we have an extent marked as under delayed
allocation but also unwritten?

							- Ted

This is how I've restructured this function for now mainly to make it
easier to understand;

static int ext4_es_can_be_merged(struct extent_status *es1,
				 struct extent_status *es2)
{
	if (ext4_es_status(es1) != ext4_es_status(es2))
		return 0;

	if (((__u64) es1->es_len) + es2->es_len > 0xFFFFFFFFULL)
		return 0;

	if (((__u64) es1->es_lblk) + es1->es_len != es2->es_lblk)
		return 0;

	if ((ext4_es_is_written(es1) || ext4_es_is_unwritten(es1)) &&
	    (ext4_es_pblock(es1) + es1->es_len == ext4_es_pblock(es2)))
		return 1;

	if (ext4_es_is_hole(es1))
		return 1;

	/* we need to check delayed extent is without unwritten status */
	if (ext4_es_is_delayed(es1) && !ext4_es_is_unwritten(es1))
		return 1;

	return 0;
}
--
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

On Sun, Mar 10, 2013 at 08:43:58PM -0400, Theodore Ts'o wrote:
> On Wed, Mar 06, 2013 at 10:17:11PM +0800, Zheng Liu wrote:
> > +	if (ext4_es_status(es1) ^ ext4_es_status(es2))
> >  		return 0;
> >  
> > -	if (ext4_es_status(es1) != ext4_es_status(es2))
> 
> Did you have a reason why changed != to ^?  

Honestly, no.  Just because subconsciously I think bit operation is
faster than other operations, and in ext4_can_extents_be_merged() it
also use '^'.  So I guess there is an optimization.

> 
> It's identical from a functional perspective, but it's less obvious to
> future readers of the code what's going on.  I tried checking to see
> if GCC did any better optimizing the code, but it doesn't seem to make
> any difference.  I'm going to switch it back to !=....

Obviously I'm wrong.  Thanks for checking it.

> 
> > +	/* we need to check delayed extent is without unwritten status */
> > +	if (ext4_es_is_delayed(es1) && !ext4_es_is_unwritten(es1))
> > +		return 1;
> 
> I'm not sure why we need to check the unwritten status?  Under what
> circumstances would we have an extent marked as under delayed
> allocation but also unwritten?

We could do some buffered writes into a hole.  So the extent will be
with delayed status.  Before these extents are written out, user might
uses fallocate(2) to preallocate some blocks at the same offset.  Then
these extents are marked as unwritten status.  But we still need to keep
delayed status because later these extents will be written out and we
will update reserved space according to these extents, especially in a
bigalloc file system.

> 
> 							- Ted
> 
> This is how I've restructured this function for now mainly to make it
> easier to understand;
> 
> static int ext4_es_can_be_merged(struct extent_status *es1,
> 				 struct extent_status *es2)
> {
> 	if (ext4_es_status(es1) != ext4_es_status(es2))
> 		return 0;
> 
> 	if (((__u64) es1->es_len) + es2->es_len > 0xFFFFFFFFULL)
> 		return 0;
> 
> 	if (((__u64) es1->es_lblk) + es1->es_len != es2->es_lblk)
> 		return 0;
> 
> 	if ((ext4_es_is_written(es1) || ext4_es_is_unwritten(es1)) &&
> 	    (ext4_es_pblock(es1) + es1->es_len == ext4_es_pblock(es2)))
> 		return 1;
> 
> 	if (ext4_es_is_hole(es1))
> 		return 1;
> 
> 	/* we need to check delayed extent is without unwritten status */
> 	if (ext4_es_is_delayed(es1) && !ext4_es_is_unwritten(es1))
> 		return 1;
> 
> 	return 0;
> }

It looks good to me.

Thanks,
                                                - Zheng
--
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