* [PATCH v2] xfs: fix the extent count when allocating an new indirection array entry
@ 2013-10-25 6:52 Jeff Liu
2013-10-31 21:36 ` Ben Myers
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Liu @ 2013-10-25 6:52 UTC (permalink / raw)
To: xfs@oss.sgi.com
From: Jie Liu <jeff.liu@oracle.com>
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@oracle.com>
---
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++;
- }
}
}
}
--
1.7.9.5
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] xfs: fix the extent count when allocating an new indirection array entry
2013-10-25 6:52 [PATCH v2] xfs: fix the extent count when allocating an new indirection array entry Jeff Liu
@ 2013-10-31 21:36 ` Ben Myers
2013-11-04 23:10 ` Ben Myers
0 siblings, 1 reply; 3+ messages in thread
From: Ben Myers @ 2013-10-31 21:36 UTC (permalink / raw)
To: Jeff Liu; +Cc: xfs@oss.sgi.com
Hey Jeff,
On Fri, Oct 25, 2013 at 02:52:44PM +0800, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
>
> 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@oracle.com>
> ---
> 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@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] xfs: fix the extent count when allocating an new indirection array entry
2013-10-31 21:36 ` Ben Myers
@ 2013-11-04 23:10 ` Ben Myers
0 siblings, 0 replies; 3+ messages in thread
From: Ben Myers @ 2013-11-04 23:10 UTC (permalink / raw)
To: Jeff Liu; +Cc: xfs@oss.sgi.com
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@oracle.com>
> >
> > 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@oracle.com>
> > ---
> > 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@sgi.com>
Applied this. Thanks Jeff.
-Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-11-04 23:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-25 6:52 [PATCH v2] xfs: fix the extent count when allocating an new indirection array entry Jeff Liu
2013-10-31 21:36 ` Ben Myers
2013-11-04 23:10 ` Ben Myers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox