[PATCH] btrfs: return accurate error code on open failure

[PATCH] btrfs: return accurate error code on open failure

[Anand Jain]

When attempting to exclusive open a device which has no exclusive open
permission, such as a physical device associated with the flakey dm
device, the open operation will fail, resulting in a mount failure.

In this particular scenario, we erroneously return -EINVAL instead of the
correct error code provided by the bdev_open_by_path() function, which is
-EBUSY.

Fix this, by returning error code from the bdev_open_by_path() function.
With this correction, the mount error message will align with that of
ext4 and xfs.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bb0857cfbef2..8a35605822bf 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1191,6 +1191,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 	struct btrfs_device *device;
 	struct btrfs_device *latest_dev = NULL;
 	struct btrfs_device *tmp_device;
+	int ret_err = 0;
 
 	list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
 				 dev_list) {
@@ -1205,9 +1206,15 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
 			list_del(&device->dev_list);
 			btrfs_free_device(device);
 		}
+		if (ret_err == 0 && ret != 0)
+			ret_err = ret;
 	}
-	if (fs_devices->open_devices == 0)
+
+	if (fs_devices->open_devices == 0) {
+		if (ret_err)
+			return ret_err;
 		return -EINVAL;
+	}
 
 	fs_devices->opened = 1;
 	fs_devices->latest_dev = latest_dev;
-- 
2.38.1

Re: [PATCH] btrfs: return accurate error code on open failure

[Anand Jain]

And this one as well, for the review. Thx.


On 3/9/24 19:16, Anand Jain wrote:
> When attempting to exclusive open a device which has no exclusive open
> permission, such as a physical device associated with the flakey dm
> device, the open operation will fail, resulting in a mount failure.
> 
> In this particular scenario, we erroneously return -EINVAL instead of the
> correct error code provided by the bdev_open_by_path() function, which is
> -EBUSY.
> 
> Fix this, by returning error code from the bdev_open_by_path() function.
> With this correction, the mount error message will align with that of
> ext4 and xfs.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>   fs/btrfs/volumes.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bb0857cfbef2..8a35605822bf 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1191,6 +1191,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>   	struct btrfs_device *device;
>   	struct btrfs_device *latest_dev = NULL;
>   	struct btrfs_device *tmp_device;
> +	int ret_err = 0;
>   
>   	list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
>   				 dev_list) {
> @@ -1205,9 +1206,15 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>   			list_del(&device->dev_list);
>   			btrfs_free_device(device);
>   		}
> +		if (ret_err == 0 && ret != 0)
> +			ret_err = ret;
>   	}
> -	if (fs_devices->open_devices == 0)
> +
> +	if (fs_devices->open_devices == 0) {
> +		if (ret_err)
> +			return ret_err;
>   		return -EINVAL;
> +	}
>   
>   	fs_devices->opened = 1;
>   	fs_devices->latest_dev = latest_dev;

Re: [PATCH] btrfs: return accurate error code on open failure

[Boris Burkov]

On Thu, Mar 14, 2024 at 07:09:24PM +0530, Anand Jain wrote:
> 
> And this one as well, for the review. Thx.
> 
> 
> On 3/9/24 19:16, Anand Jain wrote:
> > When attempting to exclusive open a device which has no exclusive open
> > permission, such as a physical device associated with the flakey dm
> > device, the open operation will fail, resulting in a mount failure.
> > 
> > In this particular scenario, we erroneously return -EINVAL instead of the
> > correct error code provided by the bdev_open_by_path() function, which is
> > -EBUSY.
> > 
> > Fix this, by returning error code from the bdev_open_by_path() function.
> > With this correction, the mount error message will align with that of
> > ext4 and xfs.
> > 
> > Signed-off-by: Anand Jain <anand.jain@oracle.com>

One small "nit", but LGTM.

Reviewed-by: Boris Burkov <boris@bur.io>

> > ---
> >   fs/btrfs/volumes.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index bb0857cfbef2..8a35605822bf 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -1191,6 +1191,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
> >   	struct btrfs_device *device;
> >   	struct btrfs_device *latest_dev = NULL;
> >   	struct btrfs_device *tmp_device;
> > +	int ret_err = 0;

A quick grep shows that "err" is a much more common name for the
variable when using this pattern in btrfs.

> >   	list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
> >   				 dev_list) {
> > @@ -1205,9 +1206,15 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
> >   			list_del(&device->dev_list);
> >   			btrfs_free_device(device);
> >   		}
> > +		if (ret_err == 0 && ret != 0)
> > +			ret_err = ret;
> >   	}
> > -	if (fs_devices->open_devices == 0)
> > +
> > +	if (fs_devices->open_devices == 0) {
> > +		if (ret_err)
> > +			return ret_err;
> >   		return -EINVAL;
> > +	}
> >   	fs_devices->opened = 1;
> >   	fs_devices->latest_dev = latest_dev;
> 

Re: [PATCH] btrfs: return accurate error code on open failure

[David Sterba]

On Thu, Mar 14, 2024 at 09:50:31AM -0700, Boris Burkov wrote:
> On Thu, Mar 14, 2024 at 07:09:24PM +0530, Anand Jain wrote:
> > 
> > And this one as well, for the review. Thx.
> > 
> > 
> > On 3/9/24 19:16, Anand Jain wrote:
> > > When attempting to exclusive open a device which has no exclusive open
> > > permission, such as a physical device associated with the flakey dm
> > > device, the open operation will fail, resulting in a mount failure.
> > > 
> > > In this particular scenario, we erroneously return -EINVAL instead of the
> > > correct error code provided by the bdev_open_by_path() function, which is
> > > -EBUSY.
> > > 
> > > Fix this, by returning error code from the bdev_open_by_path() function.
> > > With this correction, the mount error message will align with that of
> > > ext4 and xfs.
> > > 
> > > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> One small "nit", but LGTM.
> 
> Reviewed-by: Boris Burkov <boris@bur.io>
> 
> > > ---
> > >   fs/btrfs/volumes.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > > index bb0857cfbef2..8a35605822bf 100644
> > > --- a/fs/btrfs/volumes.c
> > > +++ b/fs/btrfs/volumes.c
> > > @@ -1191,6 +1191,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
> > >   	struct btrfs_device *device;
> > >   	struct btrfs_device *latest_dev = NULL;
> > >   	struct btrfs_device *tmp_device;
> > > +	int ret_err = 0;
> 
> A quick grep shows that "err" is a much more common name for the
> variable when using this pattern in btrfs.

Well 'err' is there for historical reasons and the pattern we'd like to
use everywhere is to have 'ret' matching the function return type or a
common variable for any random function called. If there's need for more
than one 'ret' (so the function-wide is not overwritten) it's ret2 etc.
https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#code

Patches to convert err -> ret are also welcome as it can be confusing
what to use in new code when there are two ways.

Re: [PATCH] btrfs: return accurate error code on open failure

[David Sterba]

On Sat, Mar 09, 2024 at 07:16:35PM +0530, Anand Jain wrote:
> When attempting to exclusive open a device which has no exclusive open
> permission, such as a physical device associated with the flakey dm
> device, the open operation will fail, resulting in a mount failure.
> 
> In this particular scenario, we erroneously return -EINVAL instead of the
> correct error code provided by the bdev_open_by_path() function, which is
> -EBUSY.
> 
> Fix this, by returning error code from the bdev_open_by_path() function.
> With this correction, the mount error message will align with that of
> ext4 and xfs.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bb0857cfbef2..8a35605822bf 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1191,6 +1191,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>  	struct btrfs_device *device;
>  	struct btrfs_device *latest_dev = NULL;
>  	struct btrfs_device *tmp_device;
> +	int ret_err = 0;

Please use 'ret' here

>  
>  	list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
>  				 dev_list) {
> @@ -1205,9 +1206,15 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>  			list_del(&device->dev_list);
>  			btrfs_free_device(device);
>  		}
> +		if (ret_err == 0 && ret != 0)

and rename the original 'ret' used in the scope as 'ret2', this is the
preferred pattern. For simple changes within one function it's ok to do
it in one patch.

Re: [PATCH] btrfs: return accurate error code on open failure

[Anand Jain]

On 3/19/24 04:06, David Sterba wrote:
> On Sat, Mar 09, 2024 at 07:16:35PM +0530, Anand Jain wrote:
>> When attempting to exclusive open a device which has no exclusive open
>> permission, such as a physical device associated with the flakey dm
>> device, the open operation will fail, resulting in a mount failure.
>>
>> In this particular scenario, we erroneously return -EINVAL instead of the
>> correct error code provided by the bdev_open_by_path() function, which is
>> -EBUSY.
>>
>> Fix this, by returning error code from the bdev_open_by_path() function.
>> With this correction, the mount error message will align with that of
>> ext4 and xfs.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index bb0857cfbef2..8a35605822bf 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1191,6 +1191,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>>   	struct btrfs_device *device;
>>   	struct btrfs_device *latest_dev = NULL;
>>   	struct btrfs_device *tmp_device;
>> +	int ret_err = 0;
> 
> Please use 'ret' here
> 
>>   
>>   	list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
>>   				 dev_list) {
>> @@ -1205,9 +1206,15 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>>   			list_del(&device->dev_list);
>>   			btrfs_free_device(device);
>>   		}
>> +		if (ret_err == 0 && ret != 0)
> 
> and rename the original 'ret' used in the scope as 'ret2', this is the
> preferred pattern. For simple changes within one function it's ok to do
> it in one patch.

Yep. Done.

Thanks, Anand

Re: [PATCH] btrfs: return accurate error code on open failure

[Anand Jain]

On 3/19/24 04:04, David Sterba wrote:
> On Thu, Mar 14, 2024 at 09:50:31AM -0700, Boris Burkov wrote:
>> On Thu, Mar 14, 2024 at 07:09:24PM +0530, Anand Jain wrote:
>>>
>>> And this one as well, for the review. Thx.
>>>
>>>
>>> On 3/9/24 19:16, Anand Jain wrote:
>>>> When attempting to exclusive open a device which has no exclusive open
>>>> permission, such as a physical device associated with the flakey dm
>>>> device, the open operation will fail, resulting in a mount failure.
>>>>
>>>> In this particular scenario, we erroneously return -EINVAL instead of the
>>>> correct error code provided by the bdev_open_by_path() function, which is
>>>> -EBUSY.
>>>>
>>>> Fix this, by returning error code from the bdev_open_by_path() function.
>>>> With this correction, the mount error message will align with that of
>>>> ext4 and xfs.
>>>>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>
>> One small "nit", but LGTM.
>>
>> Reviewed-by: Boris Burkov <boris@bur.io>
>>
>>>> ---
>>>>    fs/btrfs/volumes.c | 9 ++++++++-
>>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index bb0857cfbef2..8a35605822bf 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -1191,6 +1191,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>>>>    	struct btrfs_device *device;
>>>>    	struct btrfs_device *latest_dev = NULL;
>>>>    	struct btrfs_device *tmp_device;
>>>> +	int ret_err = 0;
>>
>> A quick grep shows that "err" is a much more common name for the
>> variable when using this pattern in btrfs.
> 

> Well 'err' is there for historical reasons and the pattern we'd like to
> use everywhere is to have 'ret' matching the function return type or a
> common variable for any random function called. If there's need for more
> than one 'ret' (so the function-wide is not overwritten) it's ret2 etc.
> https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#code
> 

> Patches to convert err -> ret are also welcome as it can be confusing
> what to use in new code when there are two ways.

I have made these changes in all the functions. I will send them soon.
It looks nice with that consistency.

Thanks, Anand