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

[Qu Wenruo <quwenruo@xxxxxxxxxxxxxx>]

-------- Original Message --------
Subject: Re: [PATCH RESEND v4 2/8] btrfs: Make btrfs_parse_options() 
parse mount option in a atomic way
From: Miao Xie <miaoxie@xxxxxxxxxx>
To: Qu Wenruo <quwenruo@xxxxxxxxxxxxxx>, <linux-btrfs@xxxxxxxxxxxxxxx>
Date: 2015年01月30日 08:31
> On Thu, 29 Jan 2015 10:24:35 +0800, Qu Wenruo wrote:
> > Current btrfs_parse_options() is not atomic, which can set and clear a
> > bit, especially for nospace_cache case.
> >
> > For example, if a fs is mounted with nospace_cache,
> > btrfs_parse_options() will set SPACE_CACHE bit first(since
> > cache_generation is non-zeo) and clear the SPACE_CACHE bit due to
> > nospace_cache mount option.
> > So under heavy operations and remount a nospace_cache btrfs, there is a
> > windows for commit to create space cache.
> >
> > This bug can be reproduced by fstest/btrfs/071 073 074 with
> > nospace_cache mount option. It has about 50% chance to create space
> > cache, and about 10% chance to create wrong space cache, which can't
> > pass btrfsck.
> >
> > This patch will do the mount option parse in a copy-and-update method.
> > First copy the mount_opt from fs_info to new_opt, and only update
> > options in new_opt. At last, copy the new_opt back to
> > fs_info->mount_opt.
> >
> > This patch is already good enough to fix the above nospace_cache +
> > remount bug, but need later patch to make sure mount options does not
> > change during transaction.
> >
> > Signed-off-by: Qu Wenruo <quwenruo@xxxxxxxxxxxxxx>
> > ---
> >   fs/btrfs/ctree.h |  16 ++++----
> >   fs/btrfs/super.c | 115 +++++++++++++++++++++++++++++--------------------------
> >   2 files changed, 69 insertions(+), 62 deletions(-)
> >
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 5f99743..26bb47b 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -2119,18 +2119,18 @@ struct btrfs_ioctl_defrag_range_args {
> >   #define btrfs_test_opt(root, opt)	((root)->fs_info->mount_opt & \
> >   					 BTRFS_MOUNT_##opt)
> >   
> > -#define btrfs_set_and_info(root, opt, fmt, args...)			\
> > +#define btrfs_set_and_info(fs_info, val, opt, fmt, args...)		\
> >   {									\
> > -	if (!btrfs_test_opt(root, opt))					\
> > -		btrfs_info(root->fs_info, fmt, ##args);			\
> > -	btrfs_set_opt(root->fs_info->mount_opt, opt);			\
> > +	if (!btrfs_raw_test_opt(val, opt))				\
> > +		btrfs_info(fs_info, fmt, ##args);			\
> > +	btrfs_set_opt(val, opt);					\
> >   }
> >   
> > -#define btrfs_clear_and_info(root, opt, fmt, args...)			\
> > +#define btrfs_clear_and_info(fs_info, val, opt, fmt, args...)		\
> >   {									\
> > -	if (btrfs_test_opt(root, opt))					\
> > -		btrfs_info(root->fs_info, fmt, ##args);			\
> > -	btrfs_clear_opt(root->fs_info->mount_opt, opt);			\
> > +	if (btrfs_raw_test_opt(val, opt))				\
> > +		btrfs_info(fs_info, fmt, ##args);			\
> > +	btrfs_clear_opt(val, opt);					\
> >   }
> >   
> >   /*
> > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > index b0c45b2..490fe1f 100644
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -395,10 +395,13 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
> >   	int ret = 0;
> >   	char *compress_type;
> >   	bool compress_force = false;
> > +	unsigned long new_opt;
> > +
> > +	new_opt = info->mount_opt;
> Here and
>
> >   
> >   	cache_gen = btrfs_super_cache_generation(root->fs_info->super_copy);
> >   	if (cache_gen)
> > -		btrfs_set_opt(info->mount_opt, SPACE_CACHE);
> [SNIP]
> >   out:
> > -	if (!ret && btrfs_test_opt(root, SPACE_CACHE))
> > -		btrfs_info(root->fs_info, "disk space caching is enabled");
> > +	if (!ret) {
> > +		if (btrfs_raw_test_opt(new_opt, SPACE_CACHE))
> > +			btrfs_info(info, "disk space caching is enabled");
> > +		info->mount_opt = new_opt;
> 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.
     |- 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html