xfs
[Top] [All Lists]

Re: [PATCH v2] xfs: fix the extent count when allocating an new indirect

To: Jeff Liu <jeff.liu@xxxxxxxxxx>
Subject: Re: [PATCH v2] xfs: fix the extent count when allocating an new indirection array entry
From: Ben Myers <bpm@xxxxxxx>
Date: Mon, 4 Nov 2013 17:10:16 -0600
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131031213624.GQ1935@xxxxxxx>
References: <526A153C.2090408@xxxxxxxxxx> <20131031213624.GQ1935@xxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Thu, Oct 31, 2013 at 04:36:24PM -0500, Ben Myers wrote:
> On Fri, Oct 25, 2013 at 02:52:44PM +0800, Jeff Liu wrote:
> > From: Jie Liu <jeff.liu@xxxxxxxxxx>
> > 
> > At xfs_iext_add(), if extent(s) are being appended to the last page in
> > the indirection array and the new extent(s) don't fit in the page, the
> > number of extents(erp->er_extcount) in a new allocated entry should be
> > the minimum value between count and XFS_LINEAR_EXTS, instead of count.
> > 
> > For now, there is no existing test case can demonstrates a problem with
> > the er_extcount being set incorrectly here, but it obviously like a bug.
> > 
> > Signed-off-by: Jie Liu <jeff.liu@xxxxxxxxxx>
> > ---
> > v2: * Declare count to uint as it will be decreased to 0 and XFS_LINEAR_EXTS
> >       can be uint because of a case in the macro.
> >     * Convert MIN() to min().
> >     * Revise the commits log to indicate there is no existing test case can
> >       reflect this issue for future tracking up.
> > 
> >  fs/xfs/xfs_inode_fork.c |    9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c
> > index 22c9837..cfee14a 100644
> > --- a/fs/xfs/xfs_inode_fork.c
> > +++ b/fs/xfs/xfs_inode_fork.c
> > @@ -1021,15 +1021,14 @@ xfs_iext_add(
> >              * the next index needed in the indirection array.
> >              */
> >             else {
> > -                   int     count = ext_diff;
> > +                   uint    count = ext_diff;
> >  
> >                     while (count) {
> >                             erp = xfs_iext_irec_new(ifp, erp_idx);
> > -                           erp->er_extcount = count;
> > -                           count -= MIN(count, (int)XFS_LINEAR_EXTS);
> > -                           if (count) {
> > +                           erp->er_extcount = min(count, XFS_LINEAR_EXTS);
> > +                           count -= erp->er_extcount;
> > +                           if (count)
> >                                     erp_idx++;
> > -                           }
> >                     }
> >             }
> >     }
> 
> Really nice find.  So there is potential for incorrect er_extcount and
> er_extoff when adding > 256 extents to the end of the indirection array.  
> You'd
> think we'd be seeing some side effects since xfs_iext_idx_to_irec uses them in
> it's binary search.
> 
> Reviewed-by: Ben Myers <bpm@xxxxxxx>

Applied this.  Thanks Jeff.

-Ben

<Prev in Thread] Current Thread [Next in Thread>
  • Re: [PATCH v2] xfs: fix the extent count when allocating an new indirection array entry, Ben Myers <=