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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
