Re: [PATCH V2] btrfs: close any open devices if btrfs_mount fails

[Chris Mason <chris.mason@fusionio.com>]

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