[PATCH] btrfs: cleanup btrfs_async_submit_limit to return the final limit value

[PATCH] btrfs: cleanup btrfs_async_submit_limit to return the final limit value

[Anand Jain]

btrfs_async_submit_limit() would return the q depth to be 256, however
when we are using it we are making it 2/3 times of it. So instead let
the function return the final computed value.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/disk-io.c | 6 ++++--
 fs/btrfs/volumes.c | 1 -
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index dfdab849037b..e58bbc2a68a5 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -861,7 +861,10 @@ unsigned long btrfs_async_submit_limit(struct btrfs_fs_info *info)
 	unsigned long limit = min_t(unsigned long,
 				    info->thread_pool_size,
 				    info->fs_devices->open_devices);
-	return 256 * limit;
+	/*
+	 * As of now limit is computed as 2/3 * 256.
+	 */
+	return 170 * limit;
 }
 
 static void run_one_async_start(struct btrfs_work *work)
@@ -887,7 +890,6 @@ static void run_one_async_done(struct btrfs_work *work)
 	fs_info = async->fs_info;
 
 	limit = btrfs_async_submit_limit(fs_info);
-	limit = limit * 2 / 3;
 
 	/*
 	 * atomic_dec_return implies a barrier for waitqueue_active
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d1d8aa226bff..8044790c5de6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -382,7 +382,6 @@ static noinline void run_scheduled_bios(struct btrfs_device *device)
 
 	bdi = device->bdev->bd_bdi;
 	limit = btrfs_async_submit_limit(fs_info);
-	limit = limit * 2 / 3;
 
 loop:
 	spin_lock(&device->io_lock);
-- 
2.13.1

Re: [PATCH] btrfs: cleanup btrfs_async_submit_limit to return the final limit value

[Nikolay Borisov]

On 31.10.2017 14:59, Anand Jain wrote:
> btrfs_async_submit_limit() would return the q depth to be 256, however
> when we are using it we are making it 2/3 times of it. So instead let
> the function return the final computed value.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Could you put a bit info about what exactly is this limit controlling in
the changelog.

> ---
>  fs/btrfs/disk-io.c | 6 ++++--
>  fs/btrfs/volumes.c | 1 -
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index dfdab849037b..e58bbc2a68a5 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -861,7 +861,10 @@ unsigned long btrfs_async_submit_limit(struct btrfs_fs_info *info)
>  	unsigned long limit = min_t(unsigned long,
>  				    info->thread_pool_size,
>  				    info->fs_devices->open_devices);
> -	return 256 * limit;
> +	/*
> +	 * As of now limit is computed as 2/3 * 256.
> +	 */

Drop the "As of now" part, since it doesn't bring any value.

> +	return 170 * limit;
>  }
>  
>  static void run_one_async_start(struct btrfs_work *work)
> @@ -887,7 +890,6 @@ static void run_one_async_done(struct btrfs_work *work)
>  	fs_info = async->fs_info;
>  
>  	limit = btrfs_async_submit_limit(fs_info);
> -	limit = limit * 2 / 3;
>  
>  	/*
>  	 * atomic_dec_return implies a barrier for waitqueue_active
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index d1d8aa226bff..8044790c5de6 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -382,7 +382,6 @@ static noinline void run_scheduled_bios(struct btrfs_device *device)
>  
>  	bdi = device->bdev->bd_bdi;
>  	limit = btrfs_async_submit_limit(fs_info);
> -	limit = limit * 2 / 3;
>  
>  loop:
>  	spin_lock(&device->io_lock);
> 

Re: [PATCH] btrfs: cleanup btrfs_async_submit_limit to return the final limit value

[Anand Jain]

On 10/31/2017 10:18 PM, Nikolay Borisov wrote:
> 
> 
> On 31.10.2017 14:59, Anand Jain wrote:
>> btrfs_async_submit_limit() would return the q depth to be 256, however
>> when we are using it we are making it 2/3 times of it. So instead let
>> the function return the final computed value.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> Could you put a bit info about what exactly is this limit controlling in
> the changelog.

  Ok will update.

>> ---
>>   fs/btrfs/disk-io.c | 6 ++++--
>>   fs/btrfs/volumes.c | 1 -
>>   2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index dfdab849037b..e58bbc2a68a5 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -861,7 +861,10 @@ unsigned long btrfs_async_submit_limit(struct btrfs_fs_info *info)
>>   	unsigned long limit = min_t(unsigned long,
>>   				    info->thread_pool_size,
>>   				    info->fs_devices->open_devices);
>> -	return 256 * limit;
>> +	/*
>> +	 * As of now limit is computed as 2/3 * 256.
>> +	 */
> 
> Drop the "As of now" part, since it doesn't bring any value.

Ok.

Thanks
Anand

>> +	return 170 * limit;
>>   }
>>   
>>   static void run_one_async_start(struct btrfs_work *work)
>> @@ -887,7 +890,6 @@ static void run_one_async_done(struct btrfs_work *work)
>>   	fs_info = async->fs_info;
>>   
>>   	limit = btrfs_async_submit_limit(fs_info);
>> -	limit = limit * 2 / 3;
>>   
>>   	/*
>>   	 * atomic_dec_return implies a barrier for waitqueue_active
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index d1d8aa226bff..8044790c5de6 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -382,7 +382,6 @@ static noinline void run_scheduled_bios(struct btrfs_device *device)
>>   
>>   	bdi = device->bdev->bd_bdi;
>>   	limit = btrfs_async_submit_limit(fs_info);
>> -	limit = limit * 2 / 3;
>>   
>>   loop:
>>   	spin_lock(&device->io_lock);
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v2] btrfs: cleanup btrfs_async_submit_limit to return the final limit value

[Anand Jain]

We feedback IO progress when it falls below 2/3 times of the limit
obtained from btrfs_async_submit_limit(), and creates a wait for the
write process and makes progress during the async submission.

In general device/transport q depth is 256 and, btrfs_async_submit_limit()
returns 256 times per device which originally was introduced by [1]. But
256 at the device level is for all types of IOs (R/W sync/async) and so
may be it was possible that entire of 256 could have occupied by async
writes and, so later patch [2] took only 2/3 times of 256 which seemed to
work well.

 [1]
 cb03c743c648
 Btrfs: Change the congestion functions to meter the number of async submits as well

 [2]
 4854ddd0ed0a
 Btrfs: Wait for kernel threads to make progress during async submission

This patch is a cleanup patch, no functional changes. And now as we are taking
only 2/3 of limit (256), so btrfs_async_submit_limit() will return 170 itself.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
IMO:
1. If the pdflush issue is fixed, we should go back to bdi congestion method,
as block layer is more appropriate and accurate to tell when the device is
congested. Device q depth 256 is very generic.
2. Consider RAID1 devices at different speed (SSD and iscsi LUN) not too sure
if this approach would lead to the FS layer IO performance  throttle at the
speed of the lowest ? wonder how to reliably test it.

 fs/btrfs/disk-io.c | 6 ++++--
 fs/btrfs/volumes.c | 1 -
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index dfdab849037b..12702e292007 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -861,7 +861,10 @@ unsigned long btrfs_async_submit_limit(struct btrfs_fs_info *info)
 	unsigned long limit = min_t(unsigned long,
 				    info->thread_pool_size,
 				    info->fs_devices->open_devices);
-	return 256 * limit;
+	/*
+	 * limit:170 is computed as 2/3 * 256.
+	 */
+	return 170 * limit;
 }
 
 static void run_one_async_start(struct btrfs_work *work)
@@ -887,7 +890,6 @@ static void run_one_async_done(struct btrfs_work *work)
 	fs_info = async->fs_info;
 
 	limit = btrfs_async_submit_limit(fs_info);
-	limit = limit * 2 / 3;
 
 	/*
 	 * atomic_dec_return implies a barrier for waitqueue_active
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d1d8aa226bff..8044790c5de6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -382,7 +382,6 @@ static noinline void run_scheduled_bios(struct btrfs_device *device)
 
 	bdi = device->bdev->bd_bdi;
 	limit = btrfs_async_submit_limit(fs_info);
-	limit = limit * 2 / 3;
 
 loop:
 	spin_lock(&device->io_lock);
-- 
2.13.1

Re: [PATCH v2] btrfs: cleanup btrfs_async_submit_limit to return the final limit value

[David Sterba]

On Thu, Nov 02, 2017 at 02:03:53PM +0800, Anand Jain wrote:
> We feedback IO progress when it falls below 2/3 times of the limit
> obtained from btrfs_async_submit_limit(), and creates a wait for the
> write process and makes progress during the async submission.
> 
> In general device/transport q depth is 256 and, btrfs_async_submit_limit()
> returns 256 times per device which originally was introduced by [1]. But
> 256 at the device level is for all types of IOs (R/W sync/async) and so
> may be it was possible that entire of 256 could have occupied by async
> writes and, so later patch [2] took only 2/3 times of 256 which seemed to
> work well.
> 
>  [1]
>  cb03c743c648
>  Btrfs: Change the congestion functions to meter the number of async submits as well
> 
>  [2]
>  4854ddd0ed0a
>  Btrfs: Wait for kernel threads to make progress during async submission
> 
> This patch is a cleanup patch, no functional changes. And now as we are taking
> only 2/3 of limit (256), so btrfs_async_submit_limit() will return 170 itself.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> IMO:
> 1. If the pdflush issue is fixed, we should go back to bdi congestion method,
> as block layer is more appropriate and accurate to tell when the device is
> congested. Device q depth 256 is very generic.
> 2. Consider RAID1 devices at different speed (SSD and iscsi LUN) not too sure
> if this approach would lead to the FS layer IO performance  throttle at the
> speed of the lowest ? wonder how to reliably test it.
> 
>  fs/btrfs/disk-io.c | 6 ++++--
>  fs/btrfs/volumes.c | 1 -
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index dfdab849037b..12702e292007 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -861,7 +861,10 @@ unsigned long btrfs_async_submit_limit(struct btrfs_fs_info *info)
>  	unsigned long limit = min_t(unsigned long,
>  				    info->thread_pool_size,
>  				    info->fs_devices->open_devices);
> -	return 256 * limit;
> +	/*
> +	 * limit:170 is computed as 2/3 * 256.
> +	 */
> +	return 170 * limit;

Please keep it opencoded, the constant will be calculated by compiler
but for code clarity it's better to be written as 2 / 3 and it's
self-documenting, so you can drop the comment.

Re: [PATCH v2] btrfs: cleanup btrfs_async_submit_limit to return the final limit value

[David Sterba]

On Thu, Nov 02, 2017 at 02:03:53PM +0800, Anand Jain wrote:
> We feedback IO progress when it falls below 2/3 times of the limit
> obtained from btrfs_async_submit_limit(), and creates a wait for the
> write process and makes progress during the async submission.
> 
> In general device/transport q depth is 256 and, btrfs_async_submit_limit()
> returns 256 times per device which originally was introduced by [1]. But
> 256 at the device level is for all types of IOs (R/W sync/async) and so
> may be it was possible that entire of 256 could have occupied by async
> writes and, so later patch [2] took only 2/3 times of 256 which seemed to
> work well.
> 
>  [1]
>  cb03c743c648
>  Btrfs: Change the congestion functions to meter the number of async submits as well
> 
>  [2]
>  4854ddd0ed0a
>  Btrfs: Wait for kernel threads to make progress during async submission
> 
> This patch is a cleanup patch, no functional changes. And now as we are taking
> only 2/3 of limit (256), so btrfs_async_submit_limit() will return 170 itself.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> IMO:
> 1. If the pdflush issue is fixed, we should go back to bdi congestion method,
> as block layer is more appropriate and accurate to tell when the device is
> congested. Device q depth 256 is very generic.
> 2. Consider RAID1 devices at different speed (SSD and iscsi LUN) not too sure
> if this approach would lead to the FS layer IO performance  throttle at the
> speed of the lowest ? wonder how to reliably test it.

The referenced commits are from 2008, there have been many changes in
the queue flushing etc, so we might need to revisit the current
behaviour completely. Using the congestion API is desired, but we also
need to keep the IO behaviour (or make it better of course). In such
case I'd suggest small steps so we can possibly catch the regressions.

[PATCH v3] btrfs: cleanup btrfs_async_submit_limit to return the final limit value

[Anand Jain]

We feedback IO progress when it falls below the 2/3 times of the limit
obtained from btrfs_async_submit_limit() so to creates a wait for the
write process and make uncontested progress during the async submission.

In general device/transport q depth is 256 and, btrfs_async_submit_limit()
returns 256 times per device which originally was introduced by [1]. But
256 at the device level is for all types of IOs (R/W sync/async) and so
may be it was possible that entire of 256 could have occupied by async
writes and, so later patch [2] took only 2/3 times of 256 which seemed to
work well.

 [1]
 cb03c743c648
 Btrfs: Change the congestion functions to meter the number of async submits as well

 [2]
 4854ddd0ed0a
 Btrfs: Wait for kernel threads to make progress during async submission

This patch is a cleanup patch, no functional changes. And now as we are taking
only 2/3 of limit (256), so btrfs_async_submit_limit() will return 256 * 2/3.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: add more change log.
v3: don't compute 256 * 2/3. I didn't know compiler will do it anyway,
    thats nice. So keeping that open coded. And comment removed.

 fs/btrfs/disk-io.c | 4 ++--
 fs/btrfs/volumes.c | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index dfdab849037b..6e27259e965b 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -861,7 +861,8 @@ unsigned long btrfs_async_submit_limit(struct btrfs_fs_info *info)
 	unsigned long limit = min_t(unsigned long,
 				    info->thread_pool_size,
 				    info->fs_devices->open_devices);
-	return 256 * limit;
+
+	return (256 * 2/3) * limit;
 }
 
 static void run_one_async_start(struct btrfs_work *work)
@@ -887,7 +888,6 @@ static void run_one_async_done(struct btrfs_work *work)
 	fs_info = async->fs_info;
 
 	limit = btrfs_async_submit_limit(fs_info);
-	limit = limit * 2 / 3;
 
 	/*
 	 * atomic_dec_return implies a barrier for waitqueue_active
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index b39737568c22..61cefa37b56a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -376,7 +376,6 @@ static noinline void run_scheduled_bios(struct btrfs_device *device)
 
 	bdi = device->bdev->bd_bdi;
 	limit = btrfs_async_submit_limit(fs_info);
-	limit = limit * 2 / 3;
 
 loop:
 	spin_lock(&device->io_lock);
-- 
2.13.1

Re: [PATCH v2] btrfs: cleanup btrfs_async_submit_limit to return the final limit value

[Anand Jain]

>> 1. If the pdflush issue is fixed, we should go back to bdi congestion method,
>> as block layer is more appropriate and accurate to tell when the device is
>> congested. Device q depth 256 is very generic.
>> 2. Consider RAID1 devices at different speed (SSD and iscsi LUN) not too sure
>> if this approach would lead to the FS layer IO performance  throttle at the
>> speed of the lowest ? wonder how to reliably test it.
> 
> The referenced commits are from 2008, there have been many changes in
> the queue flushing etc, so we might need to revisit the current
> behaviour completely. Using the congestion API is desired, but we also
> need to keep the IO behaviour (or make it better of course). In such
> case I'd suggest small steps so we can possibly catch the regressions.

  Ok. Will try. Its still confusing to me.

Thanks, Anand