[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 at 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 at fromorbit.com