[PATCH] xfs: fix the wrong new_size/rnew_size at xfs_iext_realloc_direct()

[PATCH] xfs: fix the wrong new_size/rnew_size at xfs_iext_realloc_direct()

[Jeff Liu]

From: Jie Liu <jeff.liu@oracle.com>

At xfs_iext_realloc_direct(), the new_size is changed by adding
if_bytes if originally the extent records are stored at the inline
extent buffer, and we have to switch from it to a direct extent
list for those new allocated extents, this is wrong. e.g,

Create a file with three extents which was showing as following,

xfs_io -f -c "truncate 100m" /xfs/testme

for i in $(seq 0 5 10); do
	offset=$(($i * $((1 << 20))))
	xfs_io -c "pwrite $offset 1m" /xfs/testme
done

Inline
------
irec:	if_bytes	bytes_diff	new_size
1st	0		16		16
2nd	16		16		32

Switching
---------						rnew_size
3rd	32		16		48 + 32 = 80	roundup=128

In this case, the desired value of new_size should be 48, and then
it will be roundup to 64 and be assigned to rnew_size.

However, this issue has been covered by resetting the if_bytes to
the new_size which is calculated at the begnning of xfs_iext_add()
before leaving out this function, and in turn make the rnew_size
correctly again. Hence, this can not be detected via xfstestes.

This patch fix above problem and revise the new_size comments at
xfs_iext_realloc_direct() to make it more readable.  Also, fix the
comments while switching from the inline extent buffer to a direct
extent list to reflect this change.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
 fs/xfs/xfs_inode_fork.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c
index dfb4226..7c6192a 100644
--- a/fs/xfs/xfs_inode_fork.c
+++ b/fs/xfs/xfs_inode_fork.c
@@ -1359,7 +1359,7 @@ xfs_iext_remove_indirect(
 void
 xfs_iext_realloc_direct(
 	xfs_ifork_t	*ifp,		/* inode fork pointer */
-	int		new_size)	/* new size of extents */
+	int		new_size)	/* new size of extents after adding */
 {
 	int		rnew_size;	/* real new size of extents */
 
@@ -1397,13 +1397,8 @@ xfs_iext_realloc_direct(
 				rnew_size - ifp->if_real_bytes);
 		}
 	}
-	/*
-	 * Switch from the inline extent buffer to a direct
-	 * extent list. Be sure to include the inline extent
-	 * bytes in new_size.
-	 */
+	/* Switch from the inline extent buffer to a direct extent list */
 	else {
-		new_size += ifp->if_bytes;
 		if (!is_power_of_2(new_size)) {
 			rnew_size = roundup_pow_of_two(new_size);
 		}
-- 
1.7.9.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix the wrong new_size/rnew_size at xfs_iext_realloc_direct()

[Dave Chinner]

On Sun, Sep 22, 2013 at 04:25:15PM +0800, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
> 
> At xfs_iext_realloc_direct(), the new_size is changed by adding
> if_bytes if originally the extent records are stored at the inline
> extent buffer, and we have to switch from it to a direct extent
> list for those new allocated extents, this is wrong. e.g,
> 
> Create a file with three extents which was showing as following,
> 
> xfs_io -f -c "truncate 100m" /xfs/testme
> 
> for i in $(seq 0 5 10); do
> 	offset=$(($i * $((1 << 20))))
> 	xfs_io -c "pwrite $offset 1m" /xfs/testme
> done
> 
> Inline
> ------
> irec:	if_bytes	bytes_diff	new_size
> 1st	0		16		16
> 2nd	16		16		32
> 
> Switching
> ---------						rnew_size
> 3rd	32		16		48 + 32 = 80	roundup=128
> 
> In this case, the desired value of new_size should be 48, and then
> it will be roundup to 64 and be assigned to rnew_size.

Ok, so it allocates 128 bytes instead of 64 bytes. It tracks that
allocation size correctly ifp->if_real_bytes, and all it means is
that there are 4 empty extra slots in the extent array. The code
already handles having empty slots in the direct extent array, so
what impact is there as a result of the oversized initial allocation
that is currently happening?

i.e. if fixing the oversized results in more memory allocations due
to resizing more regularly, then is there a benefit to changing this
code given that the rewrite of the ifp->if_bytes value in the case
where we do inline->direct conversion prevents this over-allocation
from being a problem...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix the wrong new_size/rnew_size at xfs_iext_realloc_direct()

[Jeff Liu]

Hi Dave,

On 09/23/2013 08:56 AM, Dave Chinner wrote:

> On Sun, Sep 22, 2013 at 04:25:15PM +0800, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@oracle.com>
>>
>> At xfs_iext_realloc_direct(), the new_size is changed by adding
>> if_bytes if originally the extent records are stored at the inline
>> extent buffer, and we have to switch from it to a direct extent
>> list for those new allocated extents, this is wrong. e.g,
>>
>> Create a file with three extents which was showing as following,
>>
>> xfs_io -f -c "truncate 100m" /xfs/testme
>>
>> for i in $(seq 0 5 10); do
>> 	offset=$(($i * $((1 << 20))))
>> 	xfs_io -c "pwrite $offset 1m" /xfs/testme
>> done
>>
>> Inline
>> ------
>> irec:	if_bytes	bytes_diff	new_size
>> 1st	0		16		16
>> 2nd	16		16		32
>>
>> Switching
>> ---------						rnew_size
>> 3rd	32		16		48 + 32 = 80	roundup=128
>>
>> In this case, the desired value of new_size should be 48, and then
>> it will be roundup to 64 and be assigned to rnew_size.
> 
> Ok, so it allocates 128 bytes instead of 64 bytes. It tracks that
> allocation size correctly ifp->if_real_bytes, and all it means is
> that there are 4 empty extra slots in the extent array. The code
> already handles having empty slots in the direct extent array, so
> what impact is there as a result of the oversized initial allocation
> that is currently happening?
> 
> i.e. if fixing the oversized results in more memory allocations due
> to resizing more regularly, then is there a benefit to changing this
> code given that the rewrite of the ifp->if_bytes value in the case
> where we do inline->direct conversion prevents this over-allocation
> from being a problem...

I guess my current patch subject/description mislead you.  The result
of the oversized can be ignored since this can be handled in the direct
extent array as empty slots.

Actually, what I want to say is that we don't need to perform "new_size += ifp->if_bytes;"
again at xfs_iext_realloc_direct() because the new_size at xfs_iext_add()
already be the size of extents after adding, just as the variable comments
is mentioned.

Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix the wrong new_size/rnew_size at xfs_iext_realloc_direct()

[Dave Chinner]

On Mon, Sep 23, 2013 at 12:47:23PM +0800, Jeff Liu wrote:
> Hi Dave,
> 
> On 09/23/2013 08:56 AM, Dave Chinner wrote:
> 
> > On Sun, Sep 22, 2013 at 04:25:15PM +0800, Jeff Liu wrote:
> >> From: Jie Liu <jeff.liu@oracle.com>
> >>
> >> At xfs_iext_realloc_direct(), the new_size is changed by adding
> >> if_bytes if originally the extent records are stored at the inline
> >> extent buffer, and we have to switch from it to a direct extent
> >> list for those new allocated extents, this is wrong. e.g,
> >>
> >> Create a file with three extents which was showing as following,
> >>
> >> xfs_io -f -c "truncate 100m" /xfs/testme
> >>
> >> for i in $(seq 0 5 10); do
> >> 	offset=$(($i * $((1 << 20))))
> >> 	xfs_io -c "pwrite $offset 1m" /xfs/testme
> >> done
> >>
> >> Inline
> >> ------
> >> irec:	if_bytes	bytes_diff	new_size
> >> 1st	0		16		16
> >> 2nd	16		16		32
> >>
> >> Switching
> >> ---------						rnew_size
> >> 3rd	32		16		48 + 32 = 80	roundup=128
> >>
> >> In this case, the desired value of new_size should be 48, and then
> >> it will be roundup to 64 and be assigned to rnew_size.
> > 
> > Ok, so it allocates 128 bytes instead of 64 bytes. It tracks that
> > allocation size correctly ifp->if_real_bytes, and all it means is
> > that there are 4 empty extra slots in the extent array. The code
> > already handles having empty slots in the direct extent array, so
> > what impact is there as a result of the oversized initial allocation
> > that is currently happening?
> > 
> > i.e. if fixing the oversized results in more memory allocations due
> > to resizing more regularly, then is there a benefit to changing this
> > code given that the rewrite of the ifp->if_bytes value in the case
> > where we do inline->direct conversion prevents this over-allocation
> > from being a problem...
> 
> I guess my current patch subject/description mislead you.  The result
> of the oversized can be ignored since this can be handled in the direct
> extent array as empty slots.

That's what I thought ;)

> Actually, what I want to say is that we don't need to perform
> "new_size += ifp->if_bytes;" again at xfs_iext_realloc_direct()
> because the new_size at xfs_iext_add() already be the size of
> extents after adding, just as the variable comments is mentioned.

Yes, I understand.

What I'm really asking is that whether there is any specific impact
you can measure as a result of changing the initial allocation size?
i.e. are there workloads where there is a measurable difference in
memory footprint or noticable performance impact of having to
reallocate the direct array more frequently as files grow and/or
shrink?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix the wrong new_size/rnew_size at xfs_iext_realloc_direct()

[Jeff Liu]

On 09/24/2013 07:56 AM, Dave Chinner wrote:

> On Mon, Sep 23, 2013 at 12:47:23PM +0800, Jeff Liu wrote:
>> Hi Dave,
>>
>> On 09/23/2013 08:56 AM, Dave Chinner wrote:
>>
>>> On Sun, Sep 22, 2013 at 04:25:15PM +0800, Jeff Liu wrote:
>>>> From: Jie Liu <jeff.liu@oracle.com>
>>>>
>>>> At xfs_iext_realloc_direct(), the new_size is changed by adding
>>>> if_bytes if originally the extent records are stored at the inline
>>>> extent buffer, and we have to switch from it to a direct extent
>>>> list for those new allocated extents, this is wrong. e.g,
>>>>
>>>> Create a file with three extents which was showing as following,
>>>>
>>>> xfs_io -f -c "truncate 100m" /xfs/testme
>>>>
>>>> for i in $(seq 0 5 10); do
>>>> 	offset=$(($i * $((1 << 20))))
>>>> 	xfs_io -c "pwrite $offset 1m" /xfs/testme
>>>> done
>>>>
>>>> Inline
>>>> ------
>>>> irec:	if_bytes	bytes_diff	new_size
>>>> 1st	0		16		16
>>>> 2nd	16		16		32
>>>>
>>>> Switching
>>>> ---------						rnew_size
>>>> 3rd	32		16		48 + 32 = 80	roundup=128
>>>>
>>>> In this case, the desired value of new_size should be 48, and then
>>>> it will be roundup to 64 and be assigned to rnew_size.
>>>
>>> Ok, so it allocates 128 bytes instead of 64 bytes. It tracks that
>>> allocation size correctly ifp->if_real_bytes, and all it means is
>>> that there are 4 empty extra slots in the extent array. The code
>>> already handles having empty slots in the direct extent array, so
>>> what impact is there as a result of the oversized initial allocation
>>> that is currently happening?
>>>
>>> i.e. if fixing the oversized results in more memory allocations due
>>> to resizing more regularly, then is there a benefit to changing this
>>> code given that the rewrite of the ifp->if_bytes value in the case
>>> where we do inline->direct conversion prevents this over-allocation
>>> from being a problem...
>>
>> I guess my current patch subject/description mislead you.  The result
>> of the oversized can be ignored since this can be handled in the direct
>> extent array as empty slots.
> 
> That's what I thought ;)
> 
>> Actually, what I want to say is that we don't need to perform
>> "new_size += ifp->if_bytes;" again at xfs_iext_realloc_direct()
>> because the new_size at xfs_iext_add() already be the size of
>> extents after adding, just as the variable comments is mentioned.
> 
> Yes, I understand.
> 
> What I'm really asking is that whether there is any specific impact
> you can measure as a result of changing the initial allocation size?
> i.e. are there workloads where there is a measurable difference in
> memory footprint or noticable performance impact of having to
> reallocate the direct array more frequently as files grow and/or
> shrink?

Not yet observed any performance matter, but IMO this problem can cause
difference in dynamic memory footprint for creating a large number of
files with 3 extents and with additional kmalloc/kfree overhead for 4
extents file.

For the first case, the current code will allocate buffers from
kmalloc-128 slab cache rather than kmalloc-64, hence it would occupy
more memory until being dropped from the cache, e.g,

# Create 10240 files with 3 extents
for ((i=0; i<10240; i++))
do
	xfs_io -f -c 'truncate 10m' /xfs/test_$i
	xfs_io -c 'pwrite 0 1' /xfs/test_$i 2>&1 >>/dev/null
	xfs_io -c 'pwrite 1m 1' /xfs/test_$i 2>&1 >>/dev/null
	xfs_io -c 'pwrite 5m 1' /xfs/test_$i 2>&1 >>/dev/null
done

# cat /proc/slab_info
# name        <active_objs>  <num_objs> <objsize> <objperslab> <pagesperslab>...

# Non-patched -- before creating files
kmalloc-128         5391   	6176       128		32    		1
kmalloc-64         21852  	25152      64 		64		1

# After that -- the number of objects in 128 slab increased significantly, while
there basically no change in 64 slab
kmalloc-128        15381  	15488      128		32    		1
kmalloc-64         21958	25088      64		64    		1


# patched -- before creating files
kmalloc-128         5751   	7072	   128		32   		1
kmalloc-64         21420	24896	   64		64		1	

After after
kmalloc-128         6155	6688	   128		32		1
kmalloc-64         30464	30464	   64		64		1

With this patch, we can reduce the memory footprint for this particular scenario.

For the 2nd case, i.e, 4 extents file.  It need to resize the direct extent list
to add the fourth extent because rnew_bytes is be re-initialized to 64 at the
beginning of xfs_iext_realloc_direct(), however the ifp->if_real_bytes is 128...

I can not think out a convenient approach(perf kmem not works on working laptop
for now) to demonstrate the consequence, but using ftrace to figure out the
different number of kmalloc. e.g,

# Creating 4096 files with 4 extents and fetch the # of kmalloc.
echo 0 > /sys/kernel/debug/tracing/events/kmem/kmalloc/enable
echo > /sys/kernel/debug/tracing/trace

for ((i=0; i<4096; i++))
do
	xfs_io -f -c 'truncate 10m' /xfs/test_$i
	xfs_io -c 'pwrite 0 1' /xfs/test_$i 2>&1 >>/dev/null
	xfs_io -c 'pwrite 1m 1' /xfs/test_$i 2>&1 >>/dev/null
	xfs_io -c 'pwrite 5m 1' /xfs/test_$i 2>&1 >>/dev/null
done

echo 1 > /sys/kernel/debug/tracing/events/kmem/kmalloc/enable
for ((i=0; i<4096; i++))
do
	xfs_io -c 'pwrite 8m 1' /xfs/test_$i 2>&1 >>/dev/null
done
cat /sys/kernel/debug/tracing/trace|grep kmalloc|wc -l

# The number of kmalloc calls
Default     	Patched
110364		103471


Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix the wrong new_size/rnew_size at xfs_iext_realloc_direct()

[Dave Chinner]

On Tue, Sep 24, 2013 at 08:57:30PM +0800, Jeff Liu wrote:
> On 09/24/2013 07:56 AM, Dave Chinner wrote:
> 
> > On Mon, Sep 23, 2013 at 12:47:23PM +0800, Jeff Liu wrote:
> >> Hi Dave,
> >>
> >> On 09/23/2013 08:56 AM, Dave Chinner wrote:
> >>
> >>> On Sun, Sep 22, 2013 at 04:25:15PM +0800, Jeff Liu wrote:
> >>>> From: Jie Liu <jeff.liu@oracle.com>
> >>>>
> >>>> At xfs_iext_realloc_direct(), the new_size is changed by adding
> >>>> if_bytes if originally the extent records are stored at the inline
> >>>> extent buffer, and we have to switch from it to a direct extent
> >>>> list for those new allocated extents, this is wrong. e.g,
....
> >> Actually, what I want to say is that we don't need to perform
> >> "new_size += ifp->if_bytes;" again at xfs_iext_realloc_direct()
> >> because the new_size at xfs_iext_add() already be the size of
> >> extents after adding, just as the variable comments is mentioned.
> > 
> > Yes, I understand.
> > 
> > What I'm really asking is that whether there is any specific impact
> > you can measure as a result of changing the initial allocation size?
> > i.e. are there workloads where there is a measurable difference in
> > memory footprint or noticable performance impact of having to
> > reallocate the direct array more frequently as files grow and/or
> > shrink?
> 
> Not yet observed any performance matter, but IMO this problem can cause
> difference in dynamic memory footprint for creating a large number of
> files with 3 extents and with additional kmalloc/kfree overhead for 4
> extents file.
> 
> For the first case, the current code will allocate buffers from
> kmalloc-128 slab cache rather than kmalloc-64, hence it would occupy
> more memory until being dropped from the cache, e.g,
> 
> # Create 10240 files with 3 extents
> for ((i=0; i<10240; i++))
> do
> 	xfs_io -f -c 'truncate 10m' /xfs/test_$i
> 	xfs_io -c 'pwrite 0 1' /xfs/test_$i 2>&1 >>/dev/null
> 	xfs_io -c 'pwrite 1m 1' /xfs/test_$i 2>&1 >>/dev/null
> 	xfs_io -c 'pwrite 5m 1' /xfs/test_$i 2>&1 >>/dev/null
> done
> 
> # cat /proc/slab_info
> # name        <active_objs>  <num_objs> <objsize> <objperslab> <pagesperslab>...
> 
> # Non-patched -- before creating files
> kmalloc-128         5391   	6176       128		32    		1
> kmalloc-64         21852  	25152      64 		64		1
> 
> # After that -- the number of objects in 128 slab increased significantly, while
> there basically no change in 64 slab
> kmalloc-128        15381  	15488      128		32    		1
> kmalloc-64         21958	25088      64		64    		1
> 
> 
> # patched -- before creating files
> kmalloc-128         5751   	7072	   128		32   		1
> kmalloc-64         21420	24896	   64		64		1	
> 
> After after
> kmalloc-128         6155	6688	   128		32		1
> kmalloc-64         30464	30464	   64		64		1
> 
> With this patch, we can reduce the memory footprint for this particular scenario.

Ok, so it's used the kmalloc-64 slab much more effectively and not
touched the kmalloc-128 slab. Ok, so thats a  measurable difference ;)
> 
> For the 2nd case, i.e, 4 extents file.  It need to resize the direct extent list
> to add the fourth extent because rnew_bytes is be re-initialized to 64 at the
> beginning of xfs_iext_realloc_direct(), however the ifp->if_real_bytes is 128...
...
> # The number of kmalloc calls
> Default     	Patched
> 110364		103471

And that demonstrates the impact in that the array is downsized as
the array grows. Ok, I'm convinced there is a net win here :)

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix the wrong new_size/rnew_size at xfs_iext_realloc_direct()

[Ben Myers]

On Sun, Sep 22, 2013 at 04:25:15PM +0800, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
> 
> At xfs_iext_realloc_direct(), the new_size is changed by adding
> if_bytes if originally the extent records are stored at the inline
> extent buffer, and we have to switch from it to a direct extent
> list for those new allocated extents, this is wrong. e.g,
> 
> Create a file with three extents which was showing as following,
> 
> xfs_io -f -c "truncate 100m" /xfs/testme
> 
> for i in $(seq 0 5 10); do
> 	offset=$(($i * $((1 << 20))))
> 	xfs_io -c "pwrite $offset 1m" /xfs/testme
> done
> 
> Inline
> ------
> irec:	if_bytes	bytes_diff	new_size
> 1st	0		16		16
> 2nd	16		16		32
> 
> Switching
> ---------						rnew_size
> 3rd	32		16		48 + 32 = 80	roundup=128
> 
> In this case, the desired value of new_size should be 48, and then
> it will be roundup to 64 and be assigned to rnew_size.
> 
> However, this issue has been covered by resetting the if_bytes to
> the new_size which is calculated at the begnning of xfs_iext_add()
> before leaving out this function, and in turn make the rnew_size
> correctly again. Hence, this can not be detected via xfstestes.
> 
> This patch fix above problem and revise the new_size comments at
> xfs_iext_realloc_direct() to make it more readable.  Also, fix the
> comments while switching from the inline extent buffer to a direct
> extent list to reflect this change.
> 
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>

Applied.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs