xfs
[Top] [All Lists]

Re: [PATCH] xfs: fix the wrong new_size/rnew_size at xfs_iext_realloc_di

To: Jeff Liu <jeff.liu@xxxxxxxxxx>
Subject: Re: [PATCH] xfs: fix the wrong new_size/rnew_size at xfs_iext_realloc_direct()
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 24 Sep 2013 09:56:42 +1000
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <523FC7DB.20204@xxxxxxxxxx>
References: <523EA96B.3040904@xxxxxxxxxx> <20130923005657.GN12541@dastard> <523FC7DB.20204@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>