diff mbox series

[RFC] Revert "btrfs: qgroup: return ENOTCONN instead of EINVAL when quotas are not enabled"

Message ID 33ce2f6df841772666ca1cf3a4876b0ff6612983.1603921124.git.osandov@fb.com (mailing list archive)
State New, archived
Headers show
Series [RFC] Revert "btrfs: qgroup: return ENOTCONN instead of EINVAL when quotas are not enabled" | expand

Commit Message

Omar Sandoval Oct. 28, 2020, 9:42 p.m. UTC 
From: Omar Sandoval <osandov@fb.com>

This reverts commit 8a36e408d40606e21cd4e2dd9601004a67b14868.

At Facebook, we have some userspace code that calls
BTRFS_IOC_QGROUP_CREATE when qgroups may be disabled and specifically
handles EINVAL. When we updated to 5.6, this started failing with the
new error code and broke the application. ENOTCONN is indeed better, but
this is effectively an ABI breakage.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
The userspace code in question is actually unit testing code for our
container system, so it's trivial for us to update that to handle the
new error. However, this may be considered an ABI breakage, so I wanted
to throw this out there and see if anyone thinks this is important
enough to revert.

 fs/btrfs/qgroup.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Qu Wenruo Oct. 29, 2020, 3:12 a.m. UTC | #1 
On 2020/10/29 上午5:42, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This reverts commit 8a36e408d40606e21cd4e2dd9601004a67b14868.
> 
> At Facebook, we have some userspace code that calls
> BTRFS_IOC_QGROUP_CREATE when qgroups may be disabled and specifically
> handles EINVAL. When we updated to 5.6, this started failing with the
> new error code and broke the application. ENOTCONN is indeed better, but
> this is effectively an ABI breakage.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> The userspace code in question is actually unit testing code for our
> container system, so it's trivial for us to update that to handle the
> new error. However, this may be considered an ABI breakage, so I wanted
> to throw this out there and see if anyone thinks this is important
> enough to revert.

Well, I'm afraid that reverting back to -EINVAL is too ambitious and the
new -ENOTCONN is indeed better to indicate the special case of quota not
enabled.

Thus reverting back to the old EINVAL is not really a good idea, it's
more like a dirty fix specific to the use cases at FB.

Sorry, I'm not a fan of reverting this patch.

Thanks,
Qu

> 
>  fs/btrfs/qgroup.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 580899bdb991..50396e85dd92 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1318,7 +1318,7 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
>  
>  	mutex_lock(&fs_info->qgroup_ioctl_lock);
>  	if (!fs_info->quota_root) {
> -		ret = -ENOTCONN;
> +		ret = -EINVAL;
>  		goto out;
>  	}
>  	member = find_qgroup_rb(fs_info, src);
> @@ -1377,7 +1377,7 @@ static int __del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
>  		return -ENOMEM;
>  
>  	if (!fs_info->quota_root) {
> -		ret = -ENOTCONN;
> +		ret = -EINVAL;
>  		goto out;
>  	}
>  
> @@ -1443,7 +1443,7 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
>  
>  	mutex_lock(&fs_info->qgroup_ioctl_lock);
>  	if (!fs_info->quota_root) {
> -		ret = -ENOTCONN;
> +		ret = -EINVAL;
>  		goto out;
>  	}
>  	quota_root = fs_info->quota_root;
> @@ -1480,7 +1480,7 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
>  
>  	mutex_lock(&fs_info->qgroup_ioctl_lock);
>  	if (!fs_info->quota_root) {
> -		ret = -ENOTCONN;
> +		ret = -EINVAL;
>  		goto out;
>  	}
>  
> @@ -1531,7 +1531,7 @@ int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid,
>  
>  	mutex_lock(&fs_info->qgroup_ioctl_lock);
>  	if (!fs_info->quota_root) {
> -		ret = -ENOTCONN;
> +		ret = -EINVAL;
>  		goto out;
>  	}
>  
>
David Sterba Nov. 3, 2020, 6:57 p.m. UTC | #2 
On Wed, Oct 28, 2020 at 02:42:38PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This reverts commit 8a36e408d40606e21cd4e2dd9601004a67b14868.
> 
> At Facebook, we have some userspace code that calls
> BTRFS_IOC_QGROUP_CREATE when qgroups may be disabled and specifically
> handles EINVAL. When we updated to 5.6, this started failing with the
> new error code and broke the application. ENOTCONN is indeed better, but
> this is effectively an ABI breakage.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> The userspace code in question is actually unit testing code for our
> container system, so it's trivial for us to update that to handle the
> new error. However, this may be considered an ABI breakage, so I wanted
> to throw this out there and see if anyone thinks this is important
> enough to revert.

Making the errors fine grained is a change but I don't consider it an
ABI breakage, it's not changing behaviour (like suddenly to succeed). It
fails and gives a better reason so the application should make use of
that.
Amy Parker Nov. 3, 2020, 7:08 p.m. UTC | #3 
On Tue, Nov 3, 2020 at 11:02 AM David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, Oct 28, 2020 at 02:42:38PM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> >
> > This reverts commit 8a36e408d40606e21cd4e2dd9601004a67b14868.
> >
> > At Facebook, we have some userspace code that calls
> > BTRFS_IOC_QGROUP_CREATE when qgroups may be disabled and specifically
> > handles EINVAL. When we updated to 5.6, this started failing with the
> > new error code and broke the application. ENOTCONN is indeed better, but
> > this is effectively an ABI breakage.
> >
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> > The userspace code in question is actually unit testing code for our
> > container system, so it's trivial for us to update that to handle the
> > new error. However, this may be considered an ABI breakage, so I wanted
> > to throw this out there and see if anyone thinks this is important
> > enough to revert.
>
> Making the errors fine grained is a change but I don't consider it an
> ABI breakage, it's not changing behaviour (like suddenly to succeed). It
> fails and gives a better reason so the application should make use of
> that.

Yep, this could've been avoided by either sticking with 5.4 (it's LTS
after all, bug fixes
get backported) or what *should* have been done is reading the 5.6 changelog
and updating code accordingly. This isn't an ABI breakage, rather an improvement
between kernel versions which should've been accounted for before upgrading.

Best regards,
Amy Parker
(they/them)
diff mbox series

Patch

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 580899bdb991..50396e85dd92 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1318,7 +1318,7 @@  int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_root) {
-		ret = -ENOTCONN;
+		ret = -EINVAL;
 		goto out;
 	}
 	member = find_qgroup_rb(fs_info, src);
@@ -1377,7 +1377,7 @@  static int __del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
 		return -ENOMEM;
 
 	if (!fs_info->quota_root) {
-		ret = -ENOTCONN;
+		ret = -EINVAL;
 		goto out;
 	}
 
@@ -1443,7 +1443,7 @@  int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_root) {
-		ret = -ENOTCONN;
+		ret = -EINVAL;
 		goto out;
 	}
 	quota_root = fs_info->quota_root;
@@ -1480,7 +1480,7 @@  int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_root) {
-		ret = -ENOTCONN;
+		ret = -EINVAL;
 		goto out;
 	}
 
@@ -1531,7 +1531,7 @@  int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid,
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_root) {
-		ret = -ENOTCONN;
+		ret = -EINVAL;
 		goto out;
 	}