[PATCH] btrfs: allow changing 'thread_pool' size at remount time

[PATCH] btrfs: allow changing 'thread_pool' size at remount time

[Sergei Trofimovich]

From: Sergei Trofimovich <slyfox@gentoo.org>

Changing 'mount -oremount,thread_pool=2 /' didn't make any effect:

maximum amount of worker threads is specified in 2 places:
- in 'strict btrfs_fs_info::thread_pool_size'
- in each worker struct: 'struct btrfs_workers::max_workers'

'mount -oremount' updated only 'btrfs_fs_info::thread_pool_size'.

Fix it by pushing new maximum value to all created worker structures
as well.

Cc: Josef Bacik <josef@redhat.com>
Cc: Chris Mason <chris.mason@oracle.com>
Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
---
WARNING: This patch makes sense only with
WARNING:    "btrfs: fix early abort in 'remount'"
WARNING: sitting in Josef's -next branch.
 fs/btrfs/super.c |   38 +++++++++++++++++++++++++++++++++-----
 1 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 2f28fc0..ed2dab9 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -435,11 +435,8 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 		case Opt_thread_pool:
 			intarg = 0;
 			match_int(&args[0], &intarg);
-			if (intarg) {
+			if (intarg)
 				info->thread_pool_size = intarg;
-				printk(KERN_INFO "btrfs: thread pool %d\n",
-				       info->thread_pool_size);
-			}
 			break;
 		case Opt_max_inline:
 			num = match_strdup(&args[0]);
@@ -1119,6 +1116,35 @@ error_fs_info:
 	return ERR_PTR(error);
 }
 
+static void btrfs_set_max_workers(struct btrfs_workers *workers, int new_limit)
+{
+	workers->max_workers = new_limit;
+}
+
+static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info, int new_pool_size, int old_pool_size)
+{
+	if (new_pool_size != old_pool_size) {
+		fs_info->thread_pool_size = new_pool_size;
+
+		printk(KERN_INFO "btrfs: resize thread pool %d -> %d\n", old_pool_size, new_pool_size);
+
+		btrfs_set_max_workers(&fs_info->generic_worker, new_pool_size);
+		btrfs_set_max_workers(&fs_info->workers, new_pool_size);
+		btrfs_set_max_workers(&fs_info->delalloc_workers, new_pool_size);
+		btrfs_set_max_workers(&fs_info->submit_workers, new_pool_size);
+		btrfs_set_max_workers(&fs_info->caching_workers, new_pool_size);
+		btrfs_set_max_workers(&fs_info->fixup_workers, new_pool_size);
+		btrfs_set_max_workers(&fs_info->endio_workers, new_pool_size);
+		btrfs_set_max_workers(&fs_info->endio_meta_workers, new_pool_size);
+		btrfs_set_max_workers(&fs_info->endio_meta_write_workers, new_pool_size);
+		btrfs_set_max_workers(&fs_info->endio_write_workers, new_pool_size);
+		btrfs_set_max_workers(&fs_info->endio_freespace_worker, new_pool_size);
+		btrfs_set_max_workers(&fs_info->delayed_workers, new_pool_size);
+		btrfs_set_max_workers(&fs_info->readahead_workers, new_pool_size);
+		btrfs_set_max_workers(&fs_info->scrub_workers, new_pool_size);
+	}
+}
+
 static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
@@ -1138,6 +1164,8 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 		goto restore;
 	}
 
+	btrfs_resize_thread_pool(fs_info, fs_info->thread_pool_size, old_thread_pool_size);
+
 	if ((*flags & MS_RDONLY) == (sb->s_flags & MS_RDONLY))
 		return 0;
 
@@ -1181,7 +1209,7 @@ restore:
 	fs_info->compress_type = old_compress_type;
 	fs_info->max_inline = old_max_inline;
 	fs_info->alloc_start = old_alloc_start;
-	fs_info->thread_pool_size = old_thread_pool_size;
+	btrfs_resize_thread_pool(fs_info, old_thread_pool_size, fs_info->thread_pool_size);
 	fs_info->metadata_ratio = old_metadata_ratio;
 	return ret;
 }
-- 
1.7.8.5

Re: [PATCH] btrfs: allow changing 'thread_pool' size at remount time

[Josef Bacik]

On Tue, Apr 24, 2012 at 08:26:44PM +0300, Sergei Trofimovich wrote:
> From: Sergei Trofimovich <slyfox@gentoo.org>
> 
> Changing 'mount -oremount,thread_pool=2 /' didn't make any effect:
> 
> maximum amount of worker threads is specified in 2 places:
> - in 'strict btrfs_fs_info::thread_pool_size'
> - in each worker struct: 'struct btrfs_workers::max_workers'
> 
> 'mount -oremount' updated only 'btrfs_fs_info::thread_pool_size'.
> 
> Fix it by pushing new maximum value to all created worker structures
> as well.
> 
> Cc: Josef Bacik <josef@redhat.com>
> Cc: Chris Mason <chris.mason@oracle.com>
> Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
> ---
> WARNING: This patch makes sense only with
> WARNING:    "btrfs: fix early abort in 'remount'"
> WARNING: sitting in Josef's -next branch.
>  fs/btrfs/super.c |   38 +++++++++++++++++++++++++++++++++-----
>  1 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 2f28fc0..ed2dab9 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -435,11 +435,8 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
>  		case Opt_thread_pool:
>  			intarg = 0;
>  			match_int(&args[0], &intarg);
> -			if (intarg) {
> +			if (intarg)
>  				info->thread_pool_size = intarg;
> -				printk(KERN_INFO "btrfs: thread pool %d\n",
> -				       info->thread_pool_size);
> -			}
>  			break;
>  		case Opt_max_inline:
>  			num = match_strdup(&args[0]);
> @@ -1119,6 +1116,35 @@ error_fs_info:
>  	return ERR_PTR(error);
>  }
>  
> +static void btrfs_set_max_workers(struct btrfs_workers *workers, int new_limit)
> +{
> +	workers->max_workers = new_limit;
> +}

This needs to be protected by it's spin lock, so do

spin_lock_irq(&workers->lock);
workers->max_workers = new_limit;
spin_unlock_irq(&workers->lock);

Also this doesn't do anything for thread pools that have already gone above the
new maximum.  In practice this shouldn't happen really since most of these
workers are related to writing anyway, but it may be prudent in the future to
make btrfs_set_max_workers to go through and stop threads until the workers are
below the new limit.  This doesn't have to be done in this patch, just good idea
for future work.

> +
> +static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info, int new_pool_size, int old_pool_size)
> +{
> +	if (new_pool_size != old_pool_size) {
> +		fs_info->thread_pool_size = new_pool_size;
> +
> +		printk(KERN_INFO "btrfs: resize thread pool %d -> %d\n", old_pool_size, new_pool_size);
> +
> +		btrfs_set_max_workers(&fs_info->generic_worker, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->workers, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->delalloc_workers, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->submit_workers, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->caching_workers, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->fixup_workers, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->endio_workers, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->endio_meta_workers, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->endio_meta_write_workers, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->endio_write_workers, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->endio_freespace_worker, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->delayed_workers, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->readahead_workers, new_pool_size);
> +		btrfs_set_max_workers(&fs_info->scrub_workers, new_pool_size);
> +	}
> +}
> +

These lines are past the 80 column mark, if you are using vim do

:set textwidth=80

and that should help.  Thanks,

Josef

Re: [PATCH] btrfs: allow changing 'thread_pool' size at remount time

[Josef Bacik]

On Tue, Apr 24, 2012 at 09:11:15PM +0300, Sergei Trofimovich wrote:
> On Tue, 24 Apr 2012 13:32:37 -0400
> Josef Bacik <josef@redhat.com> wrote:
> 
> > On Tue, Apr 24, 2012 at 08:26:44PM +0300, Sergei Trofimovich wrote:
> > > From: Sergei Trofimovich <slyfox@gentoo.org>
> > > 
> > > Changing 'mount -oremount,thread_pool=2 /' didn't make any effect:
> > > 
> > > maximum amount of worker threads is specified in 2 places:
> > > - in 'strict btrfs_fs_info::thread_pool_size'
> > > - in each worker struct: 'struct btrfs_workers::max_workers'
> > > 
> > > 'mount -oremount' updated only 'btrfs_fs_info::thread_pool_size'.
> > > 
> > > Fix it by pushing new maximum value to all created worker structures
> > > as well.
> > > 
> > > Cc: Josef Bacik <josef@redhat.com>
> > > Cc: Chris Mason <chris.mason@oracle.com>
> > > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
> > > ---
> > > WARNING: This patch makes sense only with
> > > WARNING:    "btrfs: fix early abort in 'remount'"
> > > WARNING: sitting in Josef's -next branch.
> > >  fs/btrfs/super.c |   38 +++++++++++++++++++++++++++++++++-----
> > >  1 files changed, 33 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > > index 2f28fc0..ed2dab9 100644
> > > --- a/fs/btrfs/super.c
> > > +++ b/fs/btrfs/super.c
> > > @@ -435,11 +435,8 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
> > >  		case Opt_thread_pool:
> > >  			intarg = 0;
> > >  			match_int(&args[0], &intarg);
> > > -			if (intarg) {
> > > +			if (intarg)
> > >  				info->thread_pool_size = intarg;
> > > -				printk(KERN_INFO "btrfs: thread pool %d\n",
> > > -				       info->thread_pool_size);
> > > -			}
> > >  			break;
> > >  		case Opt_max_inline:
> > >  			num = match_strdup(&args[0]);
> > > @@ -1119,6 +1116,35 @@ error_fs_info:
> > >  	return ERR_PTR(error);
> > >  }
> > >  
> > > +static void btrfs_set_max_workers(struct btrfs_workers *workers, int new_limit)
> > > +{
> > > +	workers->max_workers = new_limit;
> > > +}
> > 
> > This needs to be protected by it's spin lock, so do
> > 
> > spin_lock_irq(&workers->lock);
> > workers->max_workers = new_limit;
> > spin_unlock_irq(&workers->lock);
> > 
> > Also this doesn't do anything for thread pools that have already gone above the
> > new maximum.  In practice this shouldn't happen really since most of these
> > workers are related to writing anyway, but it may be prudent in the future to
> > make btrfs_set_max_workers to go through and stop threads until the workers are
> > below the new limit.  This doesn't have to be done in this patch, just good idea
> > for future work.
> 
> I've found a place where value is seemingly read w/o lock:
> /* fs/btrfs/disk-io.c */
> unsigned long btrfs_async_submit_limit(struct btrfs_fs_info *info)
> {
>         unsigned long limit = min_t(unsigned long,
>                                     info->workers.max_workers,
>                                     info->fs_devices->open_devices);
>         return 256 * limit;
> }
> 
> How hot that path is? Is it fine to guard with similar lock
> or it's better to consider something more SMP friendly?

It's pretty hot, we're ok reading from the value in this case without a lock
since we just want to try and throttle ourselves, it's not important to be super
accurate.  But in your case you are actually setting a new value which is going
to affect how we start new threads so you definitely want to lock it.  Thanks,

Josef

Re: [PATCH] btrfs: allow changing 'thread_pool' size at remount time

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 4581 bytes --]

On Tue, 24 Apr 2012 13:32:37 -0400
Josef Bacik <josef@redhat.com> wrote:

> On Tue, Apr 24, 2012 at 08:26:44PM +0300, Sergei Trofimovich wrote:
> > From: Sergei Trofimovich <slyfox@gentoo.org>
> > 
> > Changing 'mount -oremount,thread_pool=2 /' didn't make any effect:
> > 
> > maximum amount of worker threads is specified in 2 places:
> > - in 'strict btrfs_fs_info::thread_pool_size'
> > - in each worker struct: 'struct btrfs_workers::max_workers'
> > 
> > 'mount -oremount' updated only 'btrfs_fs_info::thread_pool_size'.
> > 
> > Fix it by pushing new maximum value to all created worker structures
> > as well.
> > 
> > Cc: Josef Bacik <josef@redhat.com>
> > Cc: Chris Mason <chris.mason@oracle.com>
> > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
> > ---
> > WARNING: This patch makes sense only with
> > WARNING:    "btrfs: fix early abort in 'remount'"
> > WARNING: sitting in Josef's -next branch.
> >  fs/btrfs/super.c |   38 +++++++++++++++++++++++++++++++++-----
> >  1 files changed, 33 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > index 2f28fc0..ed2dab9 100644
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -435,11 +435,8 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
> >  		case Opt_thread_pool:
> >  			intarg = 0;
> >  			match_int(&args[0], &intarg);
> > -			if (intarg) {
> > +			if (intarg)
> >  				info->thread_pool_size = intarg;
> > -				printk(KERN_INFO "btrfs: thread pool %d\n",
> > -				       info->thread_pool_size);
> > -			}
> >  			break;
> >  		case Opt_max_inline:
> >  			num = match_strdup(&args[0]);
> > @@ -1119,6 +1116,35 @@ error_fs_info:
> >  	return ERR_PTR(error);
> >  }
> >  
> > +static void btrfs_set_max_workers(struct btrfs_workers *workers, int new_limit)
> > +{
> > +	workers->max_workers = new_limit;
> > +}
> 
> This needs to be protected by it's spin lock, so do
> 
> spin_lock_irq(&workers->lock);
> workers->max_workers = new_limit;
> spin_unlock_irq(&workers->lock);
> 
> Also this doesn't do anything for thread pools that have already gone above the
> new maximum.  In practice this shouldn't happen really since most of these
> workers are related to writing anyway, but it may be prudent in the future to
> make btrfs_set_max_workers to go through and stop threads until the workers are
> below the new limit.  This doesn't have to be done in this patch, just good idea
> for future work.

I've found a place where value is seemingly read w/o lock:
/* fs/btrfs/disk-io.c */
unsigned long btrfs_async_submit_limit(struct btrfs_fs_info *info)
{
        unsigned long limit = min_t(unsigned long,
                                    info->workers.max_workers,
                                    info->fs_devices->open_devices);
        return 256 * limit;
}

How hot that path is? Is it fine to guard with similar lock
or it's better to consider something more SMP friendly?

> > +static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info, int new_pool_size, int old_pool_size)
> > +{
> > +	if (new_pool_size != old_pool_size) {
> > +		fs_info->thread_pool_size = new_pool_size;
> > +
> > +		printk(KERN_INFO "btrfs: resize thread pool %d -> %d\n", old_pool_size, new_pool_size);
> > +
> > +		btrfs_set_max_workers(&fs_info->generic_worker, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->workers, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->delalloc_workers, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->submit_workers, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->caching_workers, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->fixup_workers, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->endio_workers, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->endio_meta_workers, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->endio_meta_write_workers, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->endio_write_workers, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->endio_freespace_worker, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->delayed_workers, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->readahead_workers, new_pool_size);
> > +		btrfs_set_max_workers(&fs_info->scrub_workers, new_pool_size);
> > +	}
> > +}
> > +
> 
> These lines are past the 80 column mark, if you are using vim do
> 
> :set textwidth=80
> 
> and that should help.  Thanks,
>
> Josef

Will do.

Thanks!

-- 

  Sergei

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v2] btrfs: allow changing 'thread_pool' size at remount time

[Sergei Trofimovich]

From: Sergei Trofimovich <slyfox@gentoo.org>

Changing 'mount -oremount,thread_pool=2 /' didn't make any effect:

maximum amount of worker threads is specified in 2 places:
- in 'strict btrfs_fs_info::thread_pool_size'
- in each worker struct: 'struct btrfs_workers::max_workers'

'mount -oremount' updated only 'btrfs_fs_info::thread_pool_size'.

Fix it by pushing new maximum value to all created worker structures
as well.

Cc: Josef Bacik <josef@redhat.com>
Cc: Chris Mason <chris.mason@oracle.com>
Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
---
v1->v2: - added locking around 'workers' struct
        - almost succeeded to fit 80-chars limit (one-off)
WARNING: This patch makes sense only with
WARNING:    "btrfs: fix early abort in 'remount'"
WARNING: sitting in Josef's -next branch.
 fs/btrfs/super.c |   45 ++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 2f28fc0..289dbfc 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -435,11 +435,8 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 		case Opt_thread_pool:
 			intarg = 0;
 			match_int(&args[0], &intarg);
-			if (intarg) {
+			if (intarg)
 				info->thread_pool_size = intarg;
-				printk(KERN_INFO "btrfs: thread pool %d\n",
-				       info->thread_pool_size);
-			}
 			break;
 		case Opt_max_inline:
 			num = match_strdup(&args[0]);
@@ -1119,6 +1116,40 @@ error_fs_info:
 	return ERR_PTR(error);
 }
 
+static void btrfs_set_max_workers(struct btrfs_workers *workers, int new_limit)
+{
+	spin_lock_irq(&workers->lock);
+	workers->max_workers = new_limit;
+	spin_unlock_irq(&workers->lock);
+}
+
+static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info,
+				     int new_pool_size, int old_pool_size)
+{
+	if (new_pool_size == old_pool_size)
+		return;
+
+	fs_info->thread_pool_size = new_pool_size;
+
+	printk(KERN_INFO "btrfs: resize thread pool %d -> %d\n",
+	       old_pool_size, new_pool_size);
+
+	btrfs_set_max_workers(&fs_info->generic_worker, new_pool_size);
+	btrfs_set_max_workers(&fs_info->workers, new_pool_size);
+	btrfs_set_max_workers(&fs_info->delalloc_workers, new_pool_size);
+	btrfs_set_max_workers(&fs_info->submit_workers, new_pool_size);
+	btrfs_set_max_workers(&fs_info->caching_workers, new_pool_size);
+	btrfs_set_max_workers(&fs_info->fixup_workers, new_pool_size);
+	btrfs_set_max_workers(&fs_info->endio_workers, new_pool_size);
+	btrfs_set_max_workers(&fs_info->endio_meta_workers, new_pool_size);
+	btrfs_set_max_workers(&fs_info->endio_meta_write_workers, new_pool_size);
+	btrfs_set_max_workers(&fs_info->endio_write_workers, new_pool_size);
+	btrfs_set_max_workers(&fs_info->endio_freespace_worker, new_pool_size);
+	btrfs_set_max_workers(&fs_info->delayed_workers, new_pool_size);
+	btrfs_set_max_workers(&fs_info->readahead_workers, new_pool_size);
+	btrfs_set_max_workers(&fs_info->scrub_workers, new_pool_size);
+}
+
 static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
@@ -1138,6 +1169,9 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 		goto restore;
 	}
 
+	btrfs_resize_thread_pool(fs_info,
+		fs_info->thread_pool_size, old_thread_pool_size);
+
 	if ((*flags & MS_RDONLY) == (sb->s_flags & MS_RDONLY))
 		return 0;
 
@@ -1181,7 +1215,8 @@ restore:
 	fs_info->compress_type = old_compress_type;
 	fs_info->max_inline = old_max_inline;
 	fs_info->alloc_start = old_alloc_start;
-	fs_info->thread_pool_size = old_thread_pool_size;
+	btrfs_resize_thread_pool(fs_info,
+		old_thread_pool_size, fs_info->thread_pool_size);
 	fs_info->metadata_ratio = old_metadata_ratio;
 	return ret;
 }
-- 
1.7.8.5

Re: [PATCH v2] btrfs: allow changing 'thread_pool' size at remount time

[Josef Bacik]

On Tue, Apr 24, 2012 at 10:59:16PM +0300, Sergei Trofimovich wrote:
> From: Sergei Trofimovich <slyfox@gentoo.org>
> 
> Changing 'mount -oremount,thread_pool=2 /' didn't make any effect:
> 
> maximum amount of worker threads is specified in 2 places:
> - in 'strict btrfs_fs_info::thread_pool_size'
> - in each worker struct: 'struct btrfs_workers::max_workers'
> 
> 'mount -oremount' updated only 'btrfs_fs_info::thread_pool_size'.
> 
> Fix it by pushing new maximum value to all created worker structures
> as well.
> 
> Cc: Josef Bacik <josef@redhat.com>
> Cc: Chris Mason <chris.mason@oracle.com>
> Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>

Reviewed-by: Josef Bacik <josef@redhat.com>

Thanks,

Josef