[Libguestfs] APIs affected by btrfs subvolumes

[Matthew Booth]

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