On Tue, 2013-01-22 at 14:42 +0000, Richard W.M. Jones wrote:
On Tue, Jan 22, 2013 at 02:07:08PM +0000, Matthew Booth wrote:
> We have a problem with btrfs subvolumes, as they can hold a filesystem
> which can be mounted, but aren't describable using only the name of a
> block device. Specifically they need at least a block device and a
> subvolume name.
>
> There are several APIs which operate on filesystems which currently
> describe it using only a block device. I've listed them all below.
>
> In all cases, these APIs need to use a more fully specified 'mountable'
> object. There are currently 2 cases we need to support: bare device, and
> device + subvolume. As this is a significant API change, I believe we
> should also allow the mountable description to be safely extended
> without breaking the api again.
>
> I propose a string which contains a descriptor followed by data custom
> to the descriptor. For the 2 existing cases this would be:
>
> device:/dev/sda3
> subvol:/dev/sda1,root
>
> Any other solution must convey the same information. Could we achieve
> this with a union, for e.g.? How would this work with bindings?
Yikes, this is ugly.
s/ugly/correct/ ;)
> I would deprecate all of the apis below and replace them with
> alternative versions with a _ext suffix. The replacement apis would
> accept and return the enhanced descriptor.
_ext is also ugly, and deprecating really fundamental APIs like mount
is wrong.
We can discuss naming, but if a really fundamental API is broken, it
needs to be fixed.
Thanks for doing the analysis, but I don't think this is a good
thing
to do (not that I see any other good way right now either).
Also I think the analysis misses out:
(a) that we can extend existing APIs by adding optional arguments
I can see 2 possible things you might be getting at here:
1. Add subvolume information as an optional argument
This doesn't work, because an API which must return a subvolume can't
use an equivalent optional return value.
2. Add a 'work correctly' flag to existing APIS
e.g. $g->mount('subvol:/dev/sda1,root', '/', WORK_CORRECTLY => 1);
You could do that, but it's a whole lot of extra arguments everywhere.
This is even uglier than deprecating the existing apis and creating new
ones. If you wanted to go down this route, I would instead add an option
to launch which changed it universally for the entire session.
(b) the string returned from inspect_os can be treated as an opaque
string, provided we return a device name for != btrfs OSes.
Sure, but I don't think this makes any significant difference to the
solution.
I think we should sit on this problem for longer.
After all, btrfs isn't going to be the default filesystem for anyone
who cares about their data for quite a while (given we've had a data
corrupting bug open upstream for 4 months now, and precisely zero has
been done to fix it).
I disagree. btrfs is here now and the awkward nature of the problem
isn't going to change by waiting. I also expect this to be critical to
RHEL 7.
Matt