Re: [PATCH] btrfs: delete unneeded assignments in btrfs_defrag_file

["tiantao (H)" <tiantao6@huawei.com>]

在 2021/4/29 2:02, David Sterba 写道:
> On Tue, Apr 27, 2021 at 09:50:21AM +0800, Tian Tao wrote:
>> ret is assigned -EAGAIN at line 1455 and then reassigned defrag_count
>> at line 1547 after exiting the while loop, but the btrfs_defrag_file
>> function returns a negative number indicating that the execution failed
>> because it does not make sense to reassign defrag_count to ret, so
>> delete it.
> The line references are fragile, so the 1455 is after defrag is
> cancelled.
>
>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>> ---
>>   fs/btrfs/ioctl.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index ee1dbab..2b3b228 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1544,8 +1544,6 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
>>   		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
>>   	}
>>   
>> -	ret = defrag_count;
> But this would change semantics of the whole function, after deleting
> this line any stale value of 'ret' would be returned, it's used for some
> intermediate return values in the whole while loop.
>
> 1597                 if (btrfs_defrag_cancelled(fs_info)) {
> 1598                         btrfs_debug(fs_info, "defrag_file cancelled");
> 1599                         ret = -EAGAIN;
> 1600                         break;
> 1601                 }
>
> Jumping to the 'out_ra' label looks like a candidate fix but that also
> jumps around all the incompat bit setting, so that could in some cases
> miss to set them properly. And actually this is a problem with all the
> other error cases.
>
> I'm not yet sure what's the proper fix, but the errors from within the
> while loop should be returned and incompat bits set.

How does the following change look?

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ee1dbab..b8e496e 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1453,7 +1453,7 @@ int btrfs_defrag_file(struct inode *inode, struct 
file *file,
                 if (btrfs_defrag_cancelled(fs_info)) {
                         btrfs_debug(fs_info, "defrag_file cancelled");
                         ret = -EAGAIN;
-                       break;
+                      goto error;
                 }

                 if (!should_defrag_range(inode, (u64)i << PAGE_SHIFT,
@@ -1531,6 +1531,9 @@ int btrfs_defrag_file(struct inode *inode, struct 
file *file,
                 }
         }

+       ret = defrag_count;
+
+error:
         if ((range->flags & BTRFS_DEFRAG_RANGE_START_IO)) {
                 filemap_flush(inode->i_mapping);
                 if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
@@ -1544,8 +1547,6 @@ int btrfs_defrag_file(struct inode *inode, struct 
file *file,
                 btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
         }

-       ret = defrag_count;
-
  out_ra:
         if (do_compress) {
                 btrfs_inode_lock(inode, 0);

> .
>