Btrfs devel - Apr 2013 - [PATCH] btrfs: close any open devices if btrfs_mount fails

This:

   # mkfs.btrfs /dev/sdb{1,2} ; wipefs -a /dev/sdb1; mount /dev/sdb2 /mnt/test

would lead to a blkdev open/close mismatch when the mount fails, and
a permanently busy (opened O_EXCL) sdb2:

   # wipefs -a /dev/sdb2
   wipefs: error: /dev/sdb2: probing initialization failed: Device or resource
busy

It''s because btrfs_open_devices() may open some devices, and still
return failure.  So the error unwinding needs to close any open
devices in fs_devices before returning.

Reported-by: Jan Safranek <jsafrane@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Note, __btrfs_open_devices is weird; it seems to return success or
failure based on the outcome of the result of the last call
to btrfs_get_bdev_and_sb().  But that''s a different bug...

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f6b8859..60c67fa 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1125,7 +1125,7 @@ static struct dentry *btrfs_mount(struct file_system_type
*fs_type, int flags,
 
 	error = btrfs_open_devices(fs_devices, mode, fs_type);
 	if (error)
-		goto error_fs_info;
+		goto error_close_devices;
 
 	if (!(flags & MS_RDONLY) && fs_devices->rw_devices == 0) {
 		error = -EACCES;
@@ -1161,7 +1161,8 @@ static struct dentry *btrfs_mount(struct file_system_type
*fs_type, int flags,
 	return root;
 
 error_close_devices:
-	btrfs_close_devices(fs_devices);
+	if (fs_devices->opened)
+		btrfs_close_devices(fs_devices);
 error_fs_info:
 	free_fs_info(fs_info);
 	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

This:

   # mkfs.btrfs /dev/sdb{1,2} ; wipefs -a /dev/sdb1; mount /dev/sdb2 /mnt/test

would lead to a blkdev open/close mismatch when the mount fails, and
a permanently busy (opened O_EXCL) sdb2:

   # wipefs -a /dev/sdb2
   wipefs: error: /dev/sdb2: probing initialization failed: Device or resource
busy

It''s because btrfs_open_devices() may open some devices, and still
return failure.  So the error unwinding needs to close any open
devices in fs_devices before returning.

Reported-by: Jan Safranek <jsafrane@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: use open_devices not opened, that seems like the right test...
if a test is necessary I''m not certain, tbh.  Seems prudently
defensive.

Note, __btrfs_open_devices is weird; it seems to return success or
failure based on the outcome of the result of the last call
to btrfs_get_bdev_and_sb().  But that''s a different bug...

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f6b8859..60c67fa 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1125,7 +1125,7 @@ static struct dentry *btrfs_mount(struct file_system_type
*fs_type, int flags,
 
 	error = btrfs_open_devices(fs_devices, mode, fs_type);
 	if (error)
-		goto error_fs_info;
+		goto error_close_devices;
 
 	if (!(flags & MS_RDONLY) && fs_devices->rw_devices == 0) {
 		error = -EACCES;
@@ -1161,7 +1161,8 @@ static struct dentry *btrfs_mount(struct file_system_type
*fs_type, int flags,
 	return root;
 
 error_close_devices:
-	btrfs_close_devices(fs_devices);
+	if (fs_devices->open_devices)
+		btrfs_close_devices(fs_devices);
 error_fs_info:
 	free_fs_info(fs_info);
 	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


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

> It''s because btrfs_open_devices() may open some devices, and still
> return failure.  So the error unwinding needs to close any open
> devices in fs_devices before returning.
Yeah, looks like.
> Note, __btrfs_open_devices is weird; it seems to return success or
> failure based on the outcome of the result of the last call
> to btrfs_get_bdev_and_sb().  But that''s a different bug...
I disagree that this is a different bug, I think it''s the root cause of
this bug.
> @@ -1125,7 +1125,7 @@ static struct dentry *btrfs_mount(struct
file_system_type *fs_type, int flags,
>  
>  	error = btrfs_open_devices(fs_devices, mode, fs_type);
>  	if (error)
> -		goto error_fs_info;
> +		goto error_close_devices;
Wouldn''t open_seed_devices() also need a change like this?

I''d just rework __btrfs_open_devices to clean up after itself when it
returns an error.
>  error_close_devices:
> -	btrfs_close_devices(fs_devices);
> +	if (fs_devices->open_devices)
> +		btrfs_close_devices(fs_devices);
I guess that ->open_devices is supposed to be protected by the
uuid_mutex so it shouldn''t be tested out here.  In any case, it
wouldn''t
be needed if btrfs_open_devices() cleaned up as it failed.

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

On 4/4/13 1:46 PM, Zach Brown wrote:
>> It''s because btrfs_open_devices() may open some devices, and
still
>> return failure.  So the error unwinding needs to close any open
>> devices in fs_devices before returning.
> 
> Yeah, looks like.
> 
>> Note, __btrfs_open_devices is weird; it seems to return success or
>> failure based on the outcome of the result of the last call
>> to btrfs_get_bdev_and_sb().  But that''s a different bug...
> 
> I disagree that this is a different bug, I think it''s the root
cause of
> this bug.
> 
>> @@ -1125,7 +1125,7 @@ static struct dentry *btrfs_mount(struct
file_system_type *fs_type, int flags,
>>  
>>  	error = btrfs_open_devices(fs_devices, mode, fs_type);
>>  	if (error)
>> -		goto error_fs_info;
>> +		goto error_close_devices;
> 
> Wouldn''t open_seed_devices() also need a change like this?
> 
> I''d just rework __btrfs_open_devices to clean up after itself when
it
> returns an error.
> 
>>  error_close_devices:
>> -	btrfs_close_devices(fs_devices);
>> +	if (fs_devices->open_devices)
>> +		btrfs_close_devices(fs_devices);
> 
> I guess that ->open_devices is supposed to be protected by the
> uuid_mutex so it shouldn''t be tested out here.  In any case, it
wouldn''t
> be needed if btrfs_open_devices() cleaned up as it failed.
I guess I had a feeling that in something like a degraded mount scenario
you might live with failures.  But I guess that is initiated on the mount
commandline, i.e. "mount this subset; it''s degraded" not
"mount these devices,
and if some fail that''s cool."

Right?

Thanks,
-Eric
> - z
> 
--
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

> I guess I had a feeling that in something like a degraded mount scenario
> you might live with failures.  But I guess that is initiated on the mount
> commandline, i.e. "mount this subset; it''s degraded" not
"mount these devices,
> and if some fail that''s cool."
> 
> Right?
Maybe?  Who knows.

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

Quoting Eric Sandeen (2013-04-04 15:45:28)
> On 4/4/13 1:46 PM, Zach Brown wrote:
> >> It''s because btrfs_open_devices() may open some devices,
and still
> >> return failure.  So the error unwinding needs to close any open
> >> devices in fs_devices before returning.
> > 
> > Yeah, looks like.
> > 
> >> Note, __btrfs_open_devices is weird; it seems to return success or
> >> failure based on the outcome of the result of the last call
> >> to btrfs_get_bdev_and_sb().  But that''s a different
bug...
> > 
> > I disagree that this is a different bug, I think it''s the
root cause of
> > this bug.
> > 
> >> @@ -1125,7 +1125,7 @@ static struct dentry *btrfs_mount(struct
file_system_type *fs_type, int flags,
> >>  
> >>      error = btrfs_open_devices(fs_devices, mode, fs_type);
> >>      if (error)
> >> -            goto error_fs_info;
> >> +            goto error_close_devices;
> > 
> > Wouldn''t open_seed_devices() also need a change like this?
> > 
> > I''d just rework __btrfs_open_devices to clean up after itself
when it
> > returns an error.
> > 
> >>  error_close_devices:
> >> -    btrfs_close_devices(fs_devices);
> >> +    if (fs_devices->open_devices)
> >> +            btrfs_close_devices(fs_devices);
> > 
> > I guess that ->open_devices is supposed to be protected by the
> > uuid_mutex so it shouldn''t be tested out here.  In any case,
it wouldn''t
> > be needed if btrfs_open_devices() cleaned up as it failed.
> 
> I guess I had a feeling that in something like a degraded mount scenario
> you might live with failures.  But I guess that is initiated on the mount
> commandline, i.e. "mount this subset; it''s degraded" not
"mount these devices,
> and if some fail that''s cool."
btrfs_open_devices just means: go off and open every bdev you can from
this uuid.  It should return success if we opened any of them at all.

__btrfs_open_devices() already ignores failures, and this is the
only place it is explicitly setting ret.  It should only happen if there
are no devices to close.

        if (fs_devices->open_devices == 0) {
                ret = -EINVAL;
                goto out;
        }

Unless of course we happen to fail to open the last device in the list:

	ret = btrfs_get_bdev_and_sb(device->name->str, flags, holder, 1,
&bdev, &bh);
	if (ret)
		continue;

This is two curlies and a ret = 0 away from correct.

-chris

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

This:

   # mkfs.btrfs /dev/sdb{1,2} ; wipefs -a /dev/sdb1; mount /dev/sdb2 /mnt/test

would lead to a blkdev open/close mismatch when the mount fails, and
a permanently busy (opened O_EXCL) sdb2:

   # wipefs -a /dev/sdb2
   wipefs: error: /dev/sdb2: probing initialization failed: Device or resource
busy

It''s because btrfs_open_devices() may open some devices, fail on
the last one, and return that failure stored in "ret."   The mount
then fails, but the caller then does not clean up the open devices.

Chris assures me that:

"btrfs_open_devices just means: go off and open every bdev you can from
this uuid.  It should return success if we opened any of them at all."

So change the logic to ignore any open failures; just skip processing
of that device.  Later on it''s decided whether we have enough devices
to continue.

Reported-by: Jan Safranek <jsafrane@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V3: just ignore failures from btrfs_get_bdev_and_sb

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5989a92..8edcd33 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -717,9 +717,9 @@ static int __btrfs_open_devices(struct btrfs_fs_devices
*fs_devices,
 		if (!device->name)
 			continue;
 
-		ret = btrfs_get_bdev_and_sb(device->name->str, flags, holder, 1,
-					    &bdev, &bh);
-		if (ret)
+		/* Just open everything we can; ignore failures here */
+		if (btrfs_get_bdev_and_sb(device->name->str, flags, holder, 1,
+					    &bdev, &bh))
 			continue;
 
 		disk_super = (struct btrfs_super_block *)bh->b_data;


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