| Message ID | dfe752bda3e3d57c352725a4951e332b016506a9.1709991269.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: return accurate error code on open failure | expand |
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;
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; >
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.
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.
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
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
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;
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(-)