[PATCH] xfs: Remove duplicated check

[PATCH] xfs: Remove duplicated check

[Nikolay Borisov]

The check performed before the memcpy responsible for copying the rest
of the inode is already performed before we call xfs_log_dinode_to_disk.
So let's remove the 2nd instance of the check. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reported-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/xfs/xfs_log_recover.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 00240c9ee72e..88dccfb1de96 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3174,11 +3174,8 @@ xlog_recover_inode_pass2(
 	xfs_log_dinode_to_disk(ldip, dip);
 
 	/* the rest is in on-disk format */
-	if (item->ri_buf[1].i_len > isize) {
-		memcpy((char *)dip + isize,
-			item->ri_buf[1].i_addr + isize,
+	memcpy((char *)dip + isize, item->ri_buf[1].i_addr + isize,
 			item->ri_buf[1].i_len - isize);
-	}
 
 	fields = in_f->ilf_fields;
 	if (fields & XFS_ILOG_DEV)
-- 
2.7.4

Re: [PATCH] xfs: Remove duplicated check

[Darrick J. Wong]

On Tue, Feb 20, 2018 at 03:53:02PM +0200, Nikolay Borisov wrote:
> The check performed before the memcpy responsible for copying the rest
> of the inode is already performed before we call xfs_log_dinode_to_disk.
> So let's remove the 2nd instance of the check. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Reported-by: Jeff Mahoney <jeffm@suse.com>

Looks ok, will test...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_log_recover.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 00240c9ee72e..88dccfb1de96 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3174,11 +3174,8 @@ xlog_recover_inode_pass2(
>  	xfs_log_dinode_to_disk(ldip, dip);
>  
>  	/* the rest is in on-disk format */
> -	if (item->ri_buf[1].i_len > isize) {
> -		memcpy((char *)dip + isize,
> -			item->ri_buf[1].i_addr + isize,
> +	memcpy((char *)dip + isize, item->ri_buf[1].i_addr + isize,
>  			item->ri_buf[1].i_len - isize);
> -	}
>  
>  	fields = in_f->ilf_fields;
>  	if (fields & XFS_ILOG_DEV)
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: Remove duplicated check

[Dave Chinner]

On Tue, Feb 20, 2018 at 03:53:02PM +0200, Nikolay Borisov wrote:
> The check performed before the memcpy responsible for copying the rest
> of the inode is already performed before we call xfs_log_dinode_to_disk.
> So let's remove the 2nd instance of the check. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Reported-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  fs/xfs/xfs_log_recover.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 00240c9ee72e..88dccfb1de96 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3174,11 +3174,8 @@ xlog_recover_inode_pass2(
>  	xfs_log_dinode_to_disk(ldip, dip);
>  
>  	/* the rest is in on-disk format */
> -	if (item->ri_buf[1].i_len > isize) {
> -		memcpy((char *)dip + isize,
> -			item->ri_buf[1].i_addr + isize,
> +	memcpy((char *)dip + isize, item->ri_buf[1].i_addr + isize,
>  			item->ri_buf[1].i_len - isize);
> -	}
>  

This looks wrong.

The previous check is:

	if (unlikely(item->ri_buf[1].i_len > isize)) {
		CORRUPTION_ERROR
		....
		error = -EFSCORRUPTED;
		goto out_release;
	}

So after this item->ri_buf[1].i_len is always <= isize. IOWs, the
memcpy() is currently dead code that is never executed, not code we
want to execute in every inode recovery.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: Remove duplicated check

[Nikolay Borisov]

On 20.02.2018 23:11, Dave Chinner wrote:
> On Tue, Feb 20, 2018 at 03:53:02PM +0200, Nikolay Borisov wrote:
>> The check performed before the memcpy responsible for copying the rest
>> of the inode is already performed before we call xfs_log_dinode_to_disk.
>> So let's remove the 2nd instance of the check. No functional changes.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> Reported-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>  fs/xfs/xfs_log_recover.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
>> index 00240c9ee72e..88dccfb1de96 100644
>> --- a/fs/xfs/xfs_log_recover.c
>> +++ b/fs/xfs/xfs_log_recover.c
>> @@ -3174,11 +3174,8 @@ xlog_recover_inode_pass2(
>>  	xfs_log_dinode_to_disk(ldip, dip);
>>  
>>  	/* the rest is in on-disk format */
>> -	if (item->ri_buf[1].i_len > isize) {
>> -		memcpy((char *)dip + isize,
>> -			item->ri_buf[1].i_addr + isize,
>> +	memcpy((char *)dip + isize, item->ri_buf[1].i_addr + isize,
>>  			item->ri_buf[1].i_len - isize);
>> -	}
>>  
> 
> This looks wrong.
> 
> The previous check is:
> 
> 	if (unlikely(item->ri_buf[1].i_len > isize)) {
> 		CORRUPTION_ERROR
> 		....
> 		error = -EFSCORRUPTED;
> 		goto out_release;
> 	}
> 
> So after this item->ri_buf[1].i_len is always <= isize. IOWs, the
> memcpy() is currently dead code that is never executed, not code we
> want to execute in every inode recovery.

Doh, you are right, will resend.

> 
> Cheers,
> 
> Dave.
>