Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1361226 > unrolled thread

[PATCH 1/1] Btrfs: Code Cleanup

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/1] Btrfs: Code Cleanup Flex Liu <fliu@novell.com> - 2016-03-20 08:20 +0100
    Re: [PATCH 1/1] Btrfs: Code Cleanup Anand Jain <anand.jain@oracle.com> - 2016-03-21 09:40 +0100
    Re: [PATCH 1/1] Btrfs: Code Cleanup David Sterba <dsterba@suse.cz> - 2016-03-24 16:10 +0100
      Re: [PATCH 1/1] Btrfs: Code Cleanup Petr Tesarik <ptesarik@suse.com> - 2016-03-24 16:10 +0100
        Re: [PATCH 1/1] Btrfs: Code Cleanup David Sterba <dsterba@suse.cz> - 2016-03-24 17:10 +0100

#1361226 — [PATCH 1/1] Btrfs: Code Cleanup

From: Flex Liu <fliu@suse.com>

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 <fliu@suse.com>
---
 fs/btrfs/volumes.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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;
-- 
2.1.4

[toc] | [next] | [standalone]

#1361607

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;
>

[toc] | [prev] | [next] | [standalone]

#1364255

On Sun, Mar 20, 2016 at 03:11:11PM +0800, Flex Liu wrote:
> From: Flex Liu <fliu@suse.com>
> 
> 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.

[toc] | [prev] | [next] | [standalone]

#1364258

On Thu, 24 Mar 2016 16:03:07 +0100
David Sterba <dsterba@suse.cz> 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

[toc] | [prev] | [next] | [standalone]

#1364299

On Thu, Mar 24, 2016 at 04:08:18PM +0100, Petr Tesarik wrote:
> On Thu, 24 Mar 2016 16:03:07 +0100
> David Sterba <dsterba@suse.cz> 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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web