[PATCH] Btrfs: don't do async reclaim during log replay V2

[PATCH] Btrfs: don't do async reclaim during log replay V2

[Josef Bacik]

Trying to reproduce a log enospc bug I hit a panic in the async reclaim code
during log replay.  This is because we use fs_info->fs_root as our root for
shrinking and such.  Technically we can use whatever root we want, but let's
just not allow async reclaim while we're doing log replay.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
V1->V2: use fs_info->log_root_recovering instead, didn't notice this existed
before.

 fs/btrfs/extent-tree.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 28a27d5..44d0497 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4513,7 +4513,13 @@ again:
 		space_info->flush = 1;
 	} else if (!ret && space_info->flags & BTRFS_BLOCK_GROUP_METADATA) {
 		used += orig_bytes;
-		if (need_do_async_reclaim(space_info, root->fs_info, used) &&
+		/*
+		 * We will do the space reservation dance during log replay,
+		 * which means we won't have fs_info->fs_root set, so don't do
+		 * the async reclaim as we will panic.
+		 */
+		if (!root->fs_info->log_root_recovering &&
+		    need_do_async_reclaim(space_info, root->fs_info, used) &&
 		    !work_busy(&root->fs_info->async_reclaim_work))
 			queue_work(system_unbound_wq,
 				   &root->fs_info->async_reclaim_work);
-- 
1.8.3.1

Re: [PATCH] Btrfs: don't do async reclaim during log replay V2

[Miao Xie]

On Thu, 18 Sep 2014 11:27:17 -0400, Josef Bacik wrote:
> Trying to reproduce a log enospc bug I hit a panic in the async reclaim code
> during log replay.  This is because we use fs_info->fs_root as our root for
> shrinking and such.  Technically we can use whatever root we want, but let's
> just not allow async reclaim while we're doing log replay.  Thanks,

Why not move the code of fs_root initialization to the front of log replay?
I think it is better than the fix way in this patch because the async reclaimer
can help us do some work.

Thanks
Miao

> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
> V1->V2: use fs_info->log_root_recovering instead, didn't notice this existed
> before.
> 
>  fs/btrfs/extent-tree.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 28a27d5..44d0497 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4513,7 +4513,13 @@ again:
>  		space_info->flush = 1;
>  	} else if (!ret && space_info->flags & BTRFS_BLOCK_GROUP_METADATA) {
>  		used += orig_bytes;
> -		if (need_do_async_reclaim(space_info, root->fs_info, used) &&
> +		/*
> +		 * We will do the space reservation dance during log replay,
> +		 * which means we won't have fs_info->fs_root set, so don't do
> +		 * the async reclaim as we will panic.
> +		 */
> +		if (!root->fs_info->log_root_recovering &&
> +		    need_do_async_reclaim(space_info, root->fs_info, used) &&
>  		    !work_busy(&root->fs_info->async_reclaim_work))
>  			queue_work(system_unbound_wq,
>  				   &root->fs_info->async_reclaim_work);
> 

Re: [PATCH] Btrfs: don't do async reclaim during log replay V2

[Miao Xie]

Ping..

On Thu, 23 Oct 2014 16:44:54 +0800, Miao Xie wrote:
> On Thu, 18 Sep 2014 11:27:17 -0400, Josef Bacik wrote:
>> Trying to reproduce a log enospc bug I hit a panic in the async reclaim code
>> during log replay.  This is because we use fs_info->fs_root as our root for
>> shrinking and such.  Technically we can use whatever root we want, but let's
>> just not allow async reclaim while we're doing log replay.  Thanks,
> 
> Why not move the code of fs_root initialization to the front of log replay?
> I think it is better than the fix way in this patch because the async reclaimer
> can help us do some work.
> 
> Thanks
> Miao
> 
>>
>> Signed-off-by: Josef Bacik <jbacik@fb.com>
>> ---
>> V1->V2: use fs_info->log_root_recovering instead, didn't notice this existed
>> before.
>>
>>  fs/btrfs/extent-tree.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 28a27d5..44d0497 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -4513,7 +4513,13 @@ again:
>>  		space_info->flush = 1;
>>  	} else if (!ret && space_info->flags & BTRFS_BLOCK_GROUP_METADATA) {
>>  		used += orig_bytes;
>> -		if (need_do_async_reclaim(space_info, root->fs_info, used) &&
>> +		/*
>> +		 * We will do the space reservation dance during log replay,
>> +		 * which means we won't have fs_info->fs_root set, so don't do
>> +		 * the async reclaim as we will panic.
>> +		 */
>> +		if (!root->fs_info->log_root_recovering &&
>> +		    need_do_async_reclaim(space_info, root->fs_info, used) &&
>>  		    !work_busy(&root->fs_info->async_reclaim_work))
>>  			queue_work(system_unbound_wq,
>>  				   &root->fs_info->async_reclaim_work);
>>
> 
> --
> 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
> 

Re: [PATCH] Btrfs: don't do async reclaim during log replay V2

[Josef Bacik]

On 10/23/2014 04:44 AM, Miao Xie wrote:
> On Thu, 18 Sep 2014 11:27:17 -0400, Josef Bacik wrote:
>> Trying to reproduce a log enospc bug I hit a panic in the async reclaim code
>> during log replay.  This is because we use fs_info->fs_root as our root for
>> shrinking and such.  Technically we can use whatever root we want, but let's
>> just not allow async reclaim while we're doing log replay.  Thanks,
>
> Why not move the code of fs_root initialization to the front of log replay?
> I think it is better than the fix way in this patch because the async reclaimer
> can help us do some work.
>

Because this is simpler.  We could move the initialization forward, but 
then say somebody comes and adds some other dependency to the async 
reclaim stuff in the future and doesn't think about log replay and 
suddenly some poor sap's box panics on mount.  Log replay is a known 
quantity, we don't have to worry about enospc, so lets make it as simple 
as possible.  Thanks,

Josef

Re: [PATCH] Btrfs: don't do async reclaim during log replay V2

[Miao Xie]

On Thu, 6 Nov 2014 09:39:19 -0500, Josef Bacik wrote:
> On 10/23/2014 04:44 AM, Miao Xie wrote:
>> On Thu, 18 Sep 2014 11:27:17 -0400, Josef Bacik wrote:
>>> Trying to reproduce a log enospc bug I hit a panic in the async reclaim code
>>> during log replay.  This is because we use fs_info->fs_root as our root for
>>> shrinking and such.  Technically we can use whatever root we want, but let's
>>> just not allow async reclaim while we're doing log replay.  Thanks,
>>
>> Why not move the code of fs_root initialization to the front of log replay?
>> I think it is better than the fix way in this patch because the async reclaimer
>> can help us do some work.
>>
> 
> Because this is simpler.  We could move the initialization forward, but then say somebody comes and adds some other dependency to the async reclaim stuff in the future and doesn't think about log replay and suddenly some poor sap's box panics on mount.  Log replay is a known quantity, we don't have to worry about enospc, so lets make it as simple as possible.  Thanks,

Yes, you are right.

So this patch looks good.

Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>