[PATCH] xfs: fix the extent count when allocating an new indirection array entry

[PATCH] xfs: fix the extent count when allocating an new indirection array entry

[Jeff Liu]

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

Re: [PATCH] xfs: fix the extent count when allocating an new indirection array entry

[Dave Chinner]

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

Re: [PATCH] xfs: fix the extent count when allocating an new indirection array entry

[Jeff Liu]

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