Re: [PATCH RESEND v4 2/8] btrfs: Make btrfs_parse_options() parse mount option in a atomic way

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH RESEND v4 2/8] btrfs: Make btrfs_parse_options() 
parse mount option in a atomic way
From: Miao Xie <miao...@huawei.com>
To: Qu Wenruo <quwen...@cn.fujitsu.com>, <linux-btrfs@vger.kernel.org>
Date: 2015年01月30日 10:06
On Fri, 30 Jan 2015 09:33:17 +0800, Qu Wenruo wrote:
-------- Original Message --------
Subject: Re: [PATCH RESEND v4 2/8] btrfs: Make btrfs_parse_options() parse mount
option in a atomic way
From: Miao Xie <miao...@huawei.com>
To: Qu Wenruo <quwen...@cn.fujitsu.com>, <linux-btrfs@vger.kernel.org>
Date: 2015年01月30日 09:29
On Fri, 30 Jan 2015 09:20:46 +0800, Qu Wenruo wrote:
Here need ACCESS_ONCE to wrap info->mount_opt, or the complier might use
info->mount_opt instead of new_opt.
Thanks for pointing out this one.
But I worried that this is not key reason of the wrong space cache. Could
you explain the race condition which caused the wrong space cache?
Thanks
Miao
CPU0:
remount()
|- sync_fs() <- after sync_fs() we can start new trans
|- btrfs_parse_options()     CPU1:
                      |- start_transaction()
                      |- Do some bg allocation, not recorded in space_cache.
I think it is a bug if a free space is not recorded in space cache. Could you
explain why it is not recorded?

Thanks
Miao
IIRC, in that window, the fs_info->mount_opt's SPACE_CACHE bit is cleared.
So space cache is not recorded.
SPACE_CACHE is used to control cache write out, not in-memory cache. All the
free space should be recorded in in-memory cache.And when we write out
the in-memory space cache, we need protect the space cache from changing.

Thanks
Miao
You're right, the wrong space cache problem is not caused by the 
non-atomic mount option problem.
But the atomic mount option change with per-transaction mount option 
will at least make it disappear
when using nospace_cache mount option.

Thanks,
Qu

Thanks,
Qu
       |- set SPACE_CACHE bit due to cache_gen

                      |- commit_transaction()
                          |- write space cache and update cache_gen.
                              but since some of it is not recorded in space
cache,
                              the space cache missing some records.
       |- clear SPACE_CACHE bit dut to nospace_cache

So the space cache is wrong.

Thanks,
Qu
+    }
        kfree(orig);
        return ret;
    }

.



--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html