* [PATCH] xfs: fix the extent count when allocating an new indirection array entry @ 2013-09-21 14:38 Jeff Liu 2013-09-23 0:24 ` Dave Chinner 0 siblings, 1 reply; 3+ messages in thread From: Jeff Liu @ 2013-09-21 14:38 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. Signed-off-by: Jie Liu <jeff.liu@oracle.com> --- fs/xfs/xfs_inode_fork.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c index 02f1083..dfb4226 100644 --- a/fs/xfs/xfs_inode_fork.c +++ b/fs/xfs/xfs_inode_fork.c @@ -1035,11 +1035,11 @@ xfs_iext_add( 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, + (int)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] xfs: fix the extent count when allocating an new indirection array entry 2013-09-21 14:38 [PATCH] xfs: fix the extent count when allocating an new indirection array entry Jeff Liu @ 2013-09-23 0:24 ` Dave Chinner 2013-09-25 7:38 ` Jeff Liu 0 siblings, 1 reply; 3+ messages in thread From: Dave Chinner @ 2013-09-23 0:24 UTC (permalink / raw) To: Jeff Liu; +Cc: xfs@oss.sgi.com On Sat, Sep 21, 2013 at 10:38:31PM +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. Definitely looks like a bug, but what are the symptoms of it and how did you find the problem? Is there any test case that demonstrates a problem with the er_extcount being set incorrectly here? > Signed-off-by: Jie Liu <jeff.liu@oracle.com> > --- > fs/xfs/xfs_inode_fork.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c > index 02f1083..dfb4226 100644 > --- a/fs/xfs/xfs_inode_fork.c > +++ b/fs/xfs/xfs_inode_fork.c > @@ -1035,11 +1035,11 @@ xfs_iext_add( > > 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, > + (int)XFS_LINEAR_EXTS); > + count -= erp->er_extcount; count is declared as an int, whereas XFS_LINEAR_EXTS probably ends up with a type of uint because of a cast in the macro. because we are decrementing to zero, the count can be declared as a uint, too, and the cast in the MIN() can go away. Indeed, MIN() should be converted to min() seeing as we are touching the code here, and if you want to retain the current types, the min_t() is appropriate, not min(x, (some cast)y).... Cheers, Dave. -- Dave Chinner david@fromorbit.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] xfs: fix the extent count when allocating an new indirection array entry 2013-09-23 0:24 ` Dave Chinner @ 2013-09-25 7:38 ` Jeff Liu 0 siblings, 0 replies; 3+ messages in thread From: Jeff Liu @ 2013-09-25 7:38 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs@oss.sgi.com On 09/23/2013 08:24 AM, Dave Chinner wrote: > On Sat, Sep 21, 2013 at 10:38:31PM +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. > > Definitely looks like a bug, but what are the symptoms of it and how > did you find the problem? Is there any test case that demonstrates a > problem with the er_extcount being set incorrectly here? Sorry for the too late response, I found this problem while reading the code. However, I can not figure out a test case to break the kernel until now. :( > >> Signed-off-by: Jie Liu <jeff.liu@oracle.com> >> --- >> fs/xfs/xfs_inode_fork.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c >> index 02f1083..dfb4226 100644 >> --- a/fs/xfs/xfs_inode_fork.c >> +++ b/fs/xfs/xfs_inode_fork.c >> @@ -1035,11 +1035,11 @@ xfs_iext_add( >> >> 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, >> + (int)XFS_LINEAR_EXTS); >> + count -= erp->er_extcount; > > count is declared as an int, whereas XFS_LINEAR_EXTS probably ends > up with a type of uint because of a cast in the macro. because we > are decrementing to zero, the count can be declared as a uint, too, > and the cast in the MIN() can go away. Indeed, MIN() should be > converted to min() seeing as we are touching the code here, and if > you want to retain the current types, the min_t() is appropriate, > not min(x, (some cast)y).... Ok, will fix it. Thanks, -Jeff _______________________________________________ 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-09-25 7:37 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-21 14:38 [PATCH] xfs: fix the extent count when allocating an new indirection array entry Jeff Liu 2013-09-23 0:24 ` Dave Chinner 2013-09-25 7:38 ` Jeff Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox