diff mbox series

[v2] btrfs: return EAGAIN when receiving a pending signal in the defrag loops

Message ID 1605672156-29051-1-git-send-email-kaixuxia@tencent.com (mailing list archive)
State New, archived
Headers show
Series [v2] btrfs: return EAGAIN when receiving a pending signal in the defrag loops | expand

Commit Message

kaixuxia Nov. 18, 2020, 4:02 a.m. UTC
From: Kaixu Xia <kaixuxia@tencent.com>

The variable ret is overwritten by the following variable defrag_count.
Actually the code should return EAGAIN when receiving a pending signal
in the defrag loops.

Reported-by: Tosk Robot <tencent_os_robot@tencent.com>
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
v2
 -return EAGAIN instead of remove the EAGAIN error.

 fs/btrfs/ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Anand Jain Nov. 18, 2020, 9:53 a.m. UTC | #1
On 18/11/20 12:02 pm, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> The variable ret is overwritten by the following variable defrag_count.
> Actually the code should return EAGAIN when receiving a pending signal
> in the defrag loops.
> 
> Reported-by: Tosk Robot <tencent_os_robot@tencent.com>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
> v2
>   -return EAGAIN instead of remove the EAGAIN error.

  Sorry I might have missed in v1. Why was EAGAIN needed here?
  Return of defrag_count rather makes sense to me as of now.

Thanks, Anand

> 
>   fs/btrfs/ioctl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 69a384145dc6..6f13db6d30bd 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1519,7 +1519,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 out_ra;
>   		}
>   
>   		if (!should_defrag_range(inode, (u64)i << PAGE_SHIFT,
>
David Sterba Nov. 23, 2020, 7:17 p.m. UTC | #2
On Wed, Nov 18, 2020 at 12:02:36PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> The variable ret is overwritten by the following variable defrag_count.
> Actually the code should return EAGAIN when receiving a pending signal
> in the defrag loops.

This lacks explanation why is EAGAIN supposed to be the right return
value. This is about semantics of the FITRIM ioctl, changing that would
affect userspace applications.
diff mbox series

Patch

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 69a384145dc6..6f13db6d30bd 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1519,7 +1519,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 out_ra;
 		}
 
 		if (!should_defrag_range(inode, (u64)i << PAGE_SHIFT,