thr3ads.net - Btrfs devel - [PATCH] Btrfs: setup blank root and fs

If this information is useful, please help other people find it:
Share via:

Josef Bacik

2010-Nov-19 19:59 UTC

[PATCH] Btrfs: setup blank root and fs_info for mount time

There is a problem with how we use sget, it searches through the list of supers
attached to the fs_type looking for a super with the same fs_devices as what
we''re trying to mount.  This depends on sb->s_fs_info being filled,
but we don''t
fill that in until we get to btrfs_fill_super, so we could hit supers on the
fs_type super list that have a null s_fs_info.  In order to fix that we need to
go ahead and setup a blank root with a blank fs_info to hold fs_devices, that
way our test will work out right and then we can set s_fs_info in
btrfs_set_super, and then open_ctree will simply use our pre-allocated root and
fs_info when setting everything up.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/btrfs/disk-io.c |    6 ++----
 fs/btrfs/super.c   |   35 ++++++++++++++++++++++++++++++++---
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index fb827d0..f0aa204 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1538,10 +1538,8 @@ struct btrfs_root *open_ctree(struct super_block *sb,
 						 GFP_NOFS);
 	struct btrfs_root *csum_root = kzalloc(sizeof(struct btrfs_root),
 						 GFP_NOFS);
-	struct btrfs_root *tree_root = kzalloc(sizeof(struct btrfs_root),
-					       GFP_NOFS);
-	struct btrfs_fs_info *fs_info = kzalloc(sizeof(*fs_info),
-						GFP_NOFS);
+	struct btrfs_root *tree_root = btrfs_sb(sb);
+	struct btrfs_fs_info *fs_info = tree_root->fs_info;
 	struct btrfs_root *chunk_root = kzalloc(sizeof(struct btrfs_root),
 						GFP_NOFS);
 	struct btrfs_root *dev_root = kzalloc(sizeof(struct btrfs_root),
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 8299a25..9145551 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -562,12 +562,20 @@ static int btrfs_show_options(struct seq_file *seq, struct
vfsmount *vfs)
 
 static int btrfs_test_super(struct super_block *s, void *data)
 {
-	struct btrfs_fs_devices *test_fs_devices = data;
+	struct btrfs_root *test_root = data;
 	struct btrfs_root *root = btrfs_sb(s);
 
-	return root->fs_info->fs_devices == test_fs_devices;
+	return root->fs_info->fs_devices ==
test_root->fs_info->fs_devices;
 }
 
+static int btrfs_set_super(struct super_block *s, void *data)
+{
+	s->s_fs_info = data;
+
+	return set_anon_super(s, data);
+}
+
+
 /*
  * Find a superblock for the given device / mount point.
  *
@@ -581,6 +589,8 @@ static struct dentry *btrfs_mount(struct file_system_type
*fs_type, int flags,
 	struct super_block *s;
 	struct dentry *root;
 	struct btrfs_fs_devices *fs_devices = NULL;
+	struct btrfs_root *tree_root = NULL;
+	struct btrfs_fs_info *fs_info = NULL;
 	fmode_t mode = FMODE_READ;
 	char *subvol_name = NULL;
 	u64 subvol_objectid = 0;
@@ -608,8 +618,25 @@ static struct dentry *btrfs_mount(struct file_system_type
*fs_type, int flags,
 		goto error_close_devices;
 	}
 
+	/*
+	 * Setup a dummy root and fs_info for test/set super.  This is because
+	 * we don''t actually fill this stuff out until open_ctree, but we
need
+	 * it for searching for existing supers, so this lets us do that and
+	 * then open_ctree will properly initialize everything later.
+	 */
+	fs_info = kzalloc(sizeof(struct btrfs_fs_info), GFP_NOFS);
+	tree_root = kzalloc(sizeof(struct btrfs_root), GFP_NOFS);
+	if (!fs_info || !tree_root) {
+		kfree(fs_info);
+		kfree(tree_root);
+		goto error_close_devices;
+	}
+	fs_info->tree_root = tree_root;
+	fs_info->fs_devices = fs_devices;
+	tree_root->fs_info = fs_info;
+
 	bdev = fs_devices->latest_bdev;
-	s = sget(fs_type, btrfs_test_super, set_anon_super, fs_devices);
+	s = sget(fs_type, btrfs_test_super, btrfs_set_super, tree_root);
 	if (IS_ERR(s))
 		goto error_s;
 
@@ -675,6 +702,8 @@ error_s:
 	error = PTR_ERR(s);
 error_close_devices:
 	btrfs_close_devices(fs_devices);
+	kfree(fs_info);
+	kfree(tree_root);
 error_free_subvol_name:
 	kfree(subvol_name);
 	return ERR_PTR(error);
-- 
1.6.6.1

--
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

Li Zefan

2010-Nov-22 01:25 UTC

head link

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

> +	/*
> +	 * Setup a dummy root and fs_info for test/set super.  This is because
> +	 * we don''t actually fill this stuff out until open_ctree, but
we need
> +	 * it for searching for existing supers, so this lets us do that and
> +	 * then open_ctree will properly initialize everything later.
> +	 */
> +	fs_info = kzalloc(sizeof(struct btrfs_fs_info), GFP_NOFS);
> +	tree_root = kzalloc(sizeof(struct btrfs_root), GFP_NOFS);
> +	if (!fs_info || !tree_root) {
> +		kfree(fs_info);
> +		kfree(tree_root);
The above 2 kfree() calls are redundant.

And error should be set to -ENOMEM.
> +		goto error_close_devices;
> +	}
> +	fs_info->tree_root = tree_root;
> +	fs_info->fs_devices = fs_devices;
> +	tree_root->fs_info = fs_info;
> +
>  	bdev = fs_devices->latest_bdev;
> -	s = sget(fs_type, btrfs_test_super, set_anon_super, fs_devices);
> +	s = sget(fs_type, btrfs_test_super, btrfs_set_super, tree_root);
>  	if (IS_ERR(s))
>  		goto error_s;
>  
> @@ -675,6 +702,8 @@ error_s:
>  	error = PTR_ERR(s);
>  error_close_devices:
>  	btrfs_close_devices(fs_devices);
> +	kfree(fs_info);
> +	kfree(tree_root);
>  error_free_subvol_name:
>  	kfree(subvol_name);
>  	return ERR_PTR(error);--
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

Ian Kent

2010-Nov-22 01:49 UTC

head link

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

On Mon, 2010-11-22 at 09:25 +0800, Li Zefan wrote:> > +	/*
> > +	 * Setup a dummy root and fs_info for test/set super.  This is
because
> > +	 * we don''t actually fill this stuff out until open_ctree,
but we need
> > +	 * it for searching for existing supers, so this lets us do that and
> > +	 * then open_ctree will properly initialize everything later.
> > +	 */
> > +	fs_info = kzalloc(sizeof(struct btrfs_fs_info), GFP_NOFS);
> > +	tree_root = kzalloc(sizeof(struct btrfs_root), GFP_NOFS);
> > +	if (!fs_info || !tree_root) {
> > +		kfree(fs_info);
> > +		kfree(tree_root);
> 
> The above 2 kfree() calls are redundant.
That''s what I thought when I first looked at it but what happens when
fs_info != NULL and tree_root == NULL.

Although I do wonder if doing two successive allocation requests and
then checking both is a good idea. If memory is low there may be a bunch
of page scanning to try and free memory and if it eventually fails that
same process probably would happen all over again on the second call.
I''m not sure though.
> 
> And error should be set to -ENOMEM.
Ah, yes .. I missed that.
> 
> > +		goto error_close_devices;
> > +	}
> > +	fs_info->tree_root = tree_root;
> > +	fs_info->fs_devices = fs_devices;
> > +	tree_root->fs_info = fs_info;
> > +
> >  	bdev = fs_devices->latest_bdev;
> > -	s = sget(fs_type, btrfs_test_super, set_anon_super, fs_devices);
> > +	s = sget(fs_type, btrfs_test_super, btrfs_set_super, tree_root);
> >  	if (IS_ERR(s))
> >  		goto error_s;
> >  
> > @@ -675,6 +702,8 @@ error_s:
> >  	error = PTR_ERR(s);
> >  error_close_devices:
> >  	btrfs_close_devices(fs_devices);
> > +	kfree(fs_info);
> > +	kfree(tree_root);
> >  error_free_subvol_name:
> >  	kfree(subvol_name);
> >  	return ERR_PTR(error);

--
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

Josef Bacik

2010-Nov-22 01:51 UTC

head link

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

On Mon, Nov 22, 2010 at 09:49:33AM +0800, Ian Kent
wrote:> On Mon, 2010-11-22 at 09:25 +0800, Li Zefan wrote:
> > > +	/*
> > > +	 * Setup a dummy root and fs_info for test/set super.  This is
because
> > > +	 * we don''t actually fill this stuff out until
open_ctree, but we need
> > > +	 * it for searching for existing supers, so this lets us do
that and
> > > +	 * then open_ctree will properly initialize everything later.
> > > +	 */
> > > +	fs_info = kzalloc(sizeof(struct btrfs_fs_info), GFP_NOFS);
> > > +	tree_root = kzalloc(sizeof(struct btrfs_root), GFP_NOFS);
> > > +	if (!fs_info || !tree_root) {
> > > +		kfree(fs_info);
> > > +		kfree(tree_root);
> > 
> > The above 2 kfree() calls are redundant.
> 
> That''s what I thought when I first looked at it but what happens
when
> fs_info != NULL and tree_root == NULL.
>
When i do goto error_close_devices; i do the kfree''s there too, my bad,
I''ll fix
it up.
 > Although I do wonder if doing two successive allocation requests and
> then checking both is a good idea. If memory is low there may be a bunch
> of page scanning to try and free memory and if it eventually fails that
> same process probably would happen all over again on the second call.
> I''m not sure though.
>
Sure but how often is that going to happen :)
 > > 
> > And error should be set to -ENOMEM.
> 
> Ah, yes .. I missed that.
>
Yup me too.  Thanks for the review guys,

Josef 
--
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

Li Zefan

2010-Nov-22 02:01 UTC

head link

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

于 2010年11月22日 09:49, Ian Kent 写道:> On Mon, 2010-11-22 at 09:25 +0800, Li Zefan wrote:
>>> +	/*
>>> +	 * Setup a dummy root and fs_info for test/set super.  This is
because
>>> +	 * we don''t actually fill this stuff out until
open_ctree, but we need
>>> +	 * it for searching for existing supers, so this lets us do that
and
>>> +	 * then open_ctree will properly initialize everything later.
>>> +	 */
>>> +	fs_info = kzalloc(sizeof(struct btrfs_fs_info), GFP_NOFS);
>>> +	tree_root = kzalloc(sizeof(struct btrfs_root), GFP_NOFS);
>>> +	if (!fs_info || !tree_root) {
>>> +		kfree(fs_info);
>>> +		kfree(tree_root);
>> The above 2 kfree() calls are redundant.
> 
> That''s what I thought when I first looked at it but what happens
when
> fs_info != NULL and tree_root == NULL.
> 
Nothing bad happens in that case. ;)

We''ll do the cleanup after "goto error_close_devices", and
kfree(NULL)
is Ok. Did I miss something?
> Although I do wonder if doing two successive allocation requests and
> then checking both is a good idea. If memory is low there may be a bunch
> of page scanning to try and free memory and if it eventually fails that
> same process probably would happen all over again on the second call.
> I''m not sure though.
> 
>> And error should be set to -ENOMEM.
> 
> Ah, yes .. I missed that.
> --
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

Ian Kent

2010-Nov-22 02:21 UTC

head link

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

On Fri, 2010-11-19 at 14:59 -0500, Josef Bacik wrote:> There is a problem with how we use sget, it searches through the list of
supers
> attached to the fs_type looking for a super with the same fs_devices as
what
> we''re trying to mount.  This depends on sb->s_fs_info being
filled, but we don''t
> fill that in until we get to btrfs_fill_super, so we could hit supers on
the
> fs_type super list that have a null s_fs_info.  In order to fix that we
need to
> go ahead and setup a blank root with a blank fs_info to hold fs_devices,
that
> way our test will work out right and then we can set s_fs_info in
> btrfs_set_super, and then open_ctree will simply use our pre-allocated root
and
> fs_info when setting everything up.  Thanks,
> 
> Signed-off-by: Josef Bacik <josef@redhat.com>
> ---
>  fs/btrfs/disk-io.c |    6 ++----
>  fs/btrfs/super.c   |   35 ++++++++++++++++++++++++++++++++---
>  2 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index fb827d0..f0aa204 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1538,10 +1538,8 @@ struct btrfs_root *open_ctree(struct super_block
*sb,
>  						 GFP_NOFS);
>  	struct btrfs_root *csum_root = kzalloc(sizeof(struct btrfs_root),
>  						 GFP_NOFS);
> -	struct btrfs_root *tree_root = kzalloc(sizeof(struct btrfs_root),
> -					       GFP_NOFS);
> -	struct btrfs_fs_info *fs_info = kzalloc(sizeof(*fs_info),
> -						GFP_NOFS);
> +	struct btrfs_root *tree_root = btrfs_sb(sb);
> +	struct btrfs_fs_info *fs_info = tree_root->fs_info;
>  	struct btrfs_root *chunk_root = kzalloc(sizeof(struct btrfs_root),
>  						GFP_NOFS);
>  	struct btrfs_root *dev_root = kzalloc(sizeof(struct btrfs_root),
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 8299a25..9145551 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -562,12 +562,20 @@ static int btrfs_show_options(struct seq_file *seq,
struct vfsmount *vfs)
>  
>  static int btrfs_test_super(struct super_block *s, void *data)
>  {
> -	struct btrfs_fs_devices *test_fs_devices = data;
> +	struct btrfs_root *test_root = data;
>  	struct btrfs_root *root = btrfs_sb(s);
>  
> -	return root->fs_info->fs_devices == test_fs_devices;
> +	return root->fs_info->fs_devices ==
test_root->fs_info->fs_devices;
>  }
>  
> +static int btrfs_set_super(struct super_block *s, void *data)
> +{
> +	s->s_fs_info = data;
> +
> +	return set_anon_super(s, data);
> +}
> +
> +
>  /*
>   * Find a superblock for the given device / mount point.
>   *
> @@ -581,6 +589,8 @@ static struct dentry *btrfs_mount(struct
file_system_type *fs_type, int flags,
>  	struct super_block *s;
>  	struct dentry *root;
>  	struct btrfs_fs_devices *fs_devices = NULL;
> +	struct btrfs_root *tree_root = NULL;
> +	struct btrfs_fs_info *fs_info = NULL;
>  	fmode_t mode = FMODE_READ;
>  	char *subvol_name = NULL;
>  	u64 subvol_objectid = 0;
> @@ -608,8 +618,25 @@ static struct dentry *btrfs_mount(struct
file_system_type *fs_type, int flags,
>  		goto error_close_devices;
>  	}
>  
> +	/*
> +	 * Setup a dummy root and fs_info for test/set super.  This is because
> +	 * we don''t actually fill this stuff out until open_ctree, but
we need
> +	 * it for searching for existing supers, so this lets us do that and
> +	 * then open_ctree will properly initialize everything later.
> +	 */
> +	fs_info = kzalloc(sizeof(struct btrfs_fs_info), GFP_NOFS);
> +	tree_root = kzalloc(sizeof(struct btrfs_root), GFP_NOFS);
> +	if (!fs_info || !tree_root) {
> +		kfree(fs_info);
> +		kfree(tree_root);
> +		goto error_close_devices;
> +	}
> +	fs_info->tree_root = tree_root;
> +	fs_info->fs_devices = fs_devices;
> +	tree_root->fs_info = fs_info;
> +
>  	bdev = fs_devices->latest_bdev;
> -	s = sget(fs_type, btrfs_test_super, set_anon_super, fs_devices);
> +	s = sget(fs_type, btrfs_test_super, btrfs_set_super, tree_root);
>  	if (IS_ERR(s))
>  		goto error_s;
There''s no context here to comment against but just below
there''s a
check on s->s_root which tells us that an existing super block was
found. If the flags test then fails these two allocations are freed in
the error exit otherwise the existing super is used and the new fs_info
and tree_root are leaked.

I thought you were going to merge my patch for that in to yours?
>  
> @@ -675,6 +702,8 @@ error_s:
>  	error = PTR_ERR(s);
>  error_close_devices:
>  	btrfs_close_devices(fs_devices);
> +	kfree(fs_info);
> +	kfree(tree_root);
>  error_free_subvol_name:
>  	kfree(subvol_name);
>  	return ERR_PTR(error);
This is a race against other processes that are mounting but there is
the very small race window for processes that are umounting as well. I
know you didn''t think that was a problem but I think that was mostly
because of how I described it. I was hoping you would have another look
at that and include my patch in your series since it is related to this
problem.

Have a look in fs/super.c:generic_shutdown_super(), called by
fs/super.c:kill_anon_super(), where the super method ->put_super() is
called, setting the super s_fs_info to NULL, before taking the sb_lock
and removing it from the list of supers.

Here''s my patch.

btrfs - fix race between btrfs_get_sb() and umount

From: Ian Kent <raven@themaw.net>

When mounting a btrfs file system btrfs_test_super() may attempt to
use sb->s_fs_info, the btrfs root, of a super block that is going away
and that has had the btrfs root set to NULL in its ->put_super(). But
if the super block is going away it cannot be an existing super block
so we can return false in this case.

Signed-off-by: Ian Kent <raven@themaw.net>
---

 fs/btrfs/super.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)


diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 6b57da3..960b320 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -565,6 +565,12 @@ static int btrfs_test_super(struct super_block *s, void
*data)
 	struct btrfs_fs_devices *test_fs_devices = data->fs_info->fs_devices;
 	struct btrfs_root *root = btrfs_sb(s);
 
+	/*
+	 * If this super block is going away, return false as it
+	 * can''t match as an existing super block.
+	 */
+	if (!atomic_read(&s->s_active))
+		return 0;
 	return root->fs_info->fs_devices == test_fs_devices;
 }
 


--
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

Ian Kent

2010-Nov-22 02:22 UTC

head link

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

On Mon, 2010-11-22 at 10:01 +0800, Li Zefan wrote:> 于 2010年11月22日 09:49, Ian Kent 写道:
> > On Mon, 2010-11-22 at 09:25 +0800, Li Zefan wrote:
> >>> +	/*
> >>> +	 * Setup a dummy root and fs_info for test/set super.  This
is because
> >>> +	 * we don''t actually fill this stuff out until
open_ctree, but we need
> >>> +	 * it for searching for existing supers, so this lets us do
that and
> >>> +	 * then open_ctree will properly initialize everything
later.
> >>> +	 */
> >>> +	fs_info = kzalloc(sizeof(struct btrfs_fs_info), GFP_NOFS);
> >>> +	tree_root = kzalloc(sizeof(struct btrfs_root), GFP_NOFS);
> >>> +	if (!fs_info || !tree_root) {
> >>> +		kfree(fs_info);
> >>> +		kfree(tree_root);
> >> The above 2 kfree() calls are redundant.
> > 
> > That''s what I thought when I first looked at it but what
happens when
> > fs_info != NULL and tree_root == NULL.
> > 
> 
> Nothing bad happens in that case. ;)
> 
> We''ll do the cleanup after "goto error_close_devices",
and kfree(NULL)
> is Ok. Did I miss something?
No, you were correct, Josef also pointed that out, oops!
> 
> > Although I do wonder if doing two successive allocation requests and
> > then checking both is a good idea. If memory is low there may be a
bunch
> > of page scanning to try and free memory and if it eventually fails
that
> > same process probably would happen all over again on the second call.
> > I''m not sure though.
> > 
> >> And error should be set to -ENOMEM.
> > 
> > Ah, yes .. I missed that.
> > 

--
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

Li Zefan

2010-Nov-22 09:21 UTC

head link

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

> Have a look in fs/super.c:generic_shutdown_super(), called by
> fs/super.c:kill_anon_super(), where the super method ->put_super() is
> called, setting the super s_fs_info to NULL, before taking the sb_lock
> and removing it from the list of supers.
> 
> Here''s my patch.
> 
> btrfs - fix race between btrfs_get_sb() and umount
> 
> From: Ian Kent <raven@themaw.net>
> 
> When mounting a btrfs file system btrfs_test_super() may attempt to
> use sb->s_fs_info, the btrfs root, of a super block that is going away
> and that has had the btrfs root set to NULL in its ->put_super(). But
> if the super block is going away it cannot be an existing super block
> so we can return false in this case.
I think your analysis is right.

Actually I ran the test script (posted in an earlier email), and it
still crashed without your supplementary patch.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
> 
>  fs/btrfs/super.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 6b57da3..960b320 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -565,6 +565,12 @@ static int btrfs_test_super(struct super_block *s,
void *data)
>  	struct btrfs_fs_devices *test_fs_devices =
data->fs_info->fs_devices;
>  	struct btrfs_root *root = btrfs_sb(s);
>  
> +	/*
> +	 * If this super block is going away, return false as it
> +	 * can''t match as an existing super block.
> +	 */
> +	if (!atomic_read(&s->s_active))
> +		return 0;
>  	return root->fs_info->fs_devices == test_fs_devices;
>  }
>  
> --
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

Ian Kent

2010-Nov-22 12:59 UTC

head link

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

On Mon, 2010-11-22 at 17:21 +0800, Li Zefan wrote:> > Have a look in fs/super.c:generic_shutdown_super(), called by
> > fs/super.c:kill_anon_super(), where the super method ->put_super()
is
> > called, setting the super s_fs_info to NULL, before taking the sb_lock
> > and removing it from the list of supers.
> > 
> > Here''s my patch.
> > 
> > btrfs - fix race between btrfs_get_sb() and umount
> > 
> > From: Ian Kent <raven@themaw.net>
> > 
> > When mounting a btrfs file system btrfs_test_super() may attempt to
> > use sb->s_fs_info, the btrfs root, of a super block that is going
away
> > and that has had the btrfs root set to NULL in its ->put_super().
But
> > if the super block is going away it cannot be an existing super block
> > so we can return false in this case.
> 
> I think your analysis is right.
> 
> Actually I ran the test script (posted in an earlier email), and it
> still crashed without your supplementary patch.
And vice versa, as we found out working the Fedora bug, when I thought
this was the actual bug at one point, both patches are needed.
> 
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> > 
> >  fs/btrfs/super.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > 
> > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > index 6b57da3..960b320 100644
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -565,6 +565,12 @@ static int btrfs_test_super(struct super_block
*s, void *data)
> >  	struct btrfs_fs_devices *test_fs_devices =
data->fs_info->fs_devices;
> >  	struct btrfs_root *root = btrfs_sb(s);
> >  
> > +	/*
> > +	 * If this super block is going away, return false as it
> > +	 * can''t match as an existing super block.
> > +	 */
> > +	if (!atomic_read(&s->s_active))
> > +		return 0;
> >  	return root->fs_info->fs_devices == test_fs_devices;
> >  }
> >  
> > 

--
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

Btrfs devel - Nov 2010 - [PATCH] Btrfs: setup blank root and fs_info for mount time

[PATCH] Btrfs: setup blank root and fs_info for mount time

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time

Re: [PATCH] Btrfs: setup blank root and fs_info for mount time