[1/1] Btrfs: Code Cleanup

Commit Message

Flex Liu March 20, 2016, 7:11 a.m. UTC

From: Flex Liu <[e-mail]>

In fs/btrfs/volumes.c:2328

        if (seeding_dev) {
                sb->s_flags &= ~MS_RDONLY;
                ret = btrfs_prepare_sprout(root);
                BUG_ON(ret); /* -ENOMEM */
        }

the error code would be return from:

        fs_devs = kzalloc(sizeof(*fs_devs), GFP_NOFS);
        if (!fs_devs)
                return ERR_PTR(-ENOMEM);

Insufficient memory in btrfs would let the kernel panic, it suboptimal.
instead, we should return the error code in btrfs_init_new_device to
btrfs_ioctl.

Hello kernel list.
This is my first patch for kernel, so if I missed some of the guidelines,
please be patient :) I hope everything is explained in the commit message.

Signed-off-by: Flex Liu <[e-mail]>
---
 fs/btrfs/volumes.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Anand Jain March 21, 2016, 8:29 a.m. UTC | #1

Hi Flex,


> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 366b335..5c16f04 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2325,7 +2325,10 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>   	if (seeding_dev) {
>   		sb->s_flags &= ~MS_RDONLY;

  This is not undone in the failure code. Theoretically
  it should report error during unmount, did you notice ?

  (in general, $subject can be more specific).

Thanks, Anand

>   		ret = btrfs_prepare_sprout(root);
> -		BUG_ON(ret); /* -ENOMEM */
> +		if (ret) {
> +			btrfs_abort_transaction(trans, root, ret);
> +			goto error_trans;
> +		}
>   	}
>
>   	device->fs_devices = root->fs_info->fs_devices;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Sterba March 24, 2016, 3:03 p.m. UTC | #2

On Sun, Mar 20, 2016 at 03:11:11PM +0800, Flex Liu wrote:
> From: Flex Liu <[e-mail]>
> 
> In fs/btrfs/volumes.c:2328
> 
>         if (seeding_dev) {
>                 sb->s_flags &= ~MS_RDONLY;
>                 ret = btrfs_prepare_sprout(root);
>                 BUG_ON(ret); /* -ENOMEM */
>         }
> 
> the error code would be return from:
> 
>         fs_devs = kzalloc(sizeof(*fs_devs), GFP_NOFS);
>         if (!fs_devs)
>                 return ERR_PTR(-ENOMEM);

> Insufficient memory in btrfs would let the kernel panic, it suboptimal.
> instead, we should return the error code in btrfs_init_new_device to
> btrfs_ioctl.
> 
> Hello kernel list.
> This is my first patch for kernel, so if I missed some of the guidelines,
> please be patient :) I hope everything is explained in the commit message.

So a few comments:

- the subject line should say something like

  "handle errors in btrfs_init_new_device"
  or "replace BUG_ON with proper error handling",

  because it's not a code cleanup in the sense we commonly use

- you don't need to quote the code in the changelog, we can see it in
  the sources (unless you want to point out something very specific that
  might not be obvious)

> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2325,7 +2325,10 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>  	if (seeding_dev) {
>  		sb->s_flags &= ~MS_RDONLY;
>  		ret = btrfs_prepare_sprout(root);
> -		BUG_ON(ret); /* -ENOMEM */
> +		if (ret) {
> +			btrfs_abort_transaction(trans, root, ret);

The transaction abort seems a bit heavy as it will take down the whole
filesystem. It's called from the device add ioctl, this is a restartable
operation.

Unfortunatelly btrfs_prepare_sprout is called after the transaction
start so btrfs_abort_transaction must be called. To avoid it, the code
would need to be reorganized, so the memory allocations happen in
advance.

Fixing the error handling might need more patches. btrfs_prepare_sprout
can be split into parts that do just allocations and initialization and
do not touch the filesystem state (like dropping the seeding flag).

The first part will hopefully cover all failure possibilities, before
the transaction stargs, the second shall not fail at all and the error
checking can be avoided completely.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Petr Tesarik March 24, 2016, 3:08 p.m. UTC | #3

On Thu, 24 Mar 2016 16:03:07 +0100
David Sterba <[e-mail]> wrote:

> On Sun, Mar 20, 2016 at 03:11:11PM +0800, Flex Liu wrote:
>[...]
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -2325,7 +2325,10 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
> >  	if (seeding_dev) {
> >  		sb->s_flags &= ~MS_RDONLY;
> >  		ret = btrfs_prepare_sprout(root);
> > -		BUG_ON(ret); /* -ENOMEM */
> > +		if (ret) {
> > +			btrfs_abort_transaction(trans, root, ret);
> 
> The transaction abort seems a bit heavy as it will take down the whole
> filesystem. It's called from the device add ioctl, this is a restartable
> operation.
> 
> Unfortunatelly btrfs_prepare_sprout is called after the transaction
> start so btrfs_abort_transaction must be called. To avoid it, the code
> would need to be reorganized, so the memory allocations happen in
> advance.

On the other hand, an abort is still better than a BUG_ON(), and it may
be easier to make incremental improvements.

Just my 2 cents (I haven't tried it),
Petr T
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Sterba March 24, 2016, 4:09 p.m. UTC | #4

On Thu, Mar 24, 2016 at 04:08:18PM +0100, Petr Tesarik wrote:
> On Thu, 24 Mar 2016 16:03:07 +0100
> David Sterba <[e-mail]> wrote:
> 
> > On Sun, Mar 20, 2016 at 03:11:11PM +0800, Flex Liu wrote:
> >[...]
> > > --- a/fs/btrfs/volumes.c
> > > +++ b/fs/btrfs/volumes.c
> > > @@ -2325,7 +2325,10 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
> > >  	if (seeding_dev) {
> > >  		sb->s_flags &= ~MS_RDONLY;
> > >  		ret = btrfs_prepare_sprout(root);
> > > -		BUG_ON(ret); /* -ENOMEM */
> > > +		if (ret) {
> > > +			btrfs_abort_transaction(trans, root, ret);
> > 
> > The transaction abort seems a bit heavy as it will take down the whole
> > filesystem. It's called from the device add ioctl, this is a restartable
> > operation.
> > 
> > Unfortunatelly btrfs_prepare_sprout is called after the transaction
> > start so btrfs_abort_transaction must be called. To avoid it, the code
> > would need to be reorganized, so the memory allocations happen in
> > advance.
> 
> On the other hand, an abort is still better than a BUG_ON(), and it may
> be easier to make incremental improvements.

That's acceptable, if there are going to be incremental improvements.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 366b335..5c16f04 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2325,7 +2325,10 @@  int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
 	if (seeding_dev) {
 		sb->s_flags &= ~MS_RDONLY;
 		ret = btrfs_prepare_sprout(root);
-		BUG_ON(ret); /* -ENOMEM */
+		if (ret) {
+			btrfs_abort_transaction(trans, root, ret);
+			goto error_trans;
+		}
 	}
 
 	device->fs_devices = root->fs_info->fs_devices;