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

Qu Wenruo Thu, 29 Jan 2015 20:01:45 -0800


-------- 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日 11:21

On Fri, 30 Jan 2015 10:51:52 +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日 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.

But we need fix a problem, not hide a problem.

Thanks
Miao

Yes, But I don't mean to hide it.

If it's a bug in space cache, it will still be reproducible on withspace_cache mount option.

The patch itself is focused on the space cache created withnospace_cache mount option,

at least it's doing it job.

The wrong space cahce problem is another story then.

I'll remove the wrong space cache description in the commit message innext version.


Thanks,
Qu

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

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

Reply via email to