Discussion:
Review please #4012: Upper limit of zfs set bounds check for refreservation on volumes is too low
Saso Kiselkov
2013-08-08 11:32:56 UTC
Permalink
Fixes incorrect refreservation bounds check in libzfs, which doesn't
take metadata and copies overhead into account, making it impossible to
properly set a volume's refreservation to "thick provisioning" from the
command line.

Issue: https://www.illumos.org/issues/4012
Fix: http://cr.illumos.org/~webrev/skiselkov/4012/

Cheers,
--
Saso
Matthew Ahrens
2013-08-10 17:47:59 UTC
Permalink
Why is zvol_volsize_to_reservation_impl() in the header file? Seems like
it should be static.

Looks good aside from that.

--matt


On Thu, Aug 8, 2013 at 4:32 AM, Saso Kiselkov <***@gmail.com>wrote:

> Fixes incorrect refreservation bounds check in libzfs, which doesn't
> take metadata and copies overhead into account, making it impossible to
> properly set a volume's refreservation to "thick provisioning" from the
> command line.
>
> Issue: https://www.illumos.org/issues/4012
> Fix: http://cr.illumos.org/~webrev/skiselkov/4012/
>
> Cheers,
> --
> Saso
>
>
> -------------------------------------------
> illumos-zfs
> Archives: https://www.listbox.com/member/archive/182191/=now
> RSS Feed:
> https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460
> Modify Your Subscription:
> https://www.listbox.com/member/?&
> Powered by Listbox: http://www.listbox.com
>



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Saso Kiselkov
2013-08-10 18:17:18 UTC
Permalink
On 8/10/13 6:47 PM, Matthew Ahrens wrote:
> Why is zvol_volsize_to_reservation_impl() in the header file? Seems
> like it should be static.
>
> Looks good aside from that.

Because some external libzfs users might want to use the algorithm
itself. This way external storage management tools can know how to set a
refreservation to completely contain a volume without actually having to
contain a separate copy of the algorithm.

Cheers,
--
Saso
Matthew Ahrens
2013-08-10 20:02:27 UTC
Permalink
It just seems strange to me that we need 2, nearly identical functions that
do basically the same thing. zvol_volsize_to_reservation() and
zvol_volsize_to_reservation_impl().
Can't external consumers use zvol_volsize_to_reservation()?

--matt


On Sat, Aug 10, 2013 at 11:17 AM, Saso Kiselkov <***@gmail.com>wrote:

> On 8/10/13 6:47 PM, Matthew Ahrens wrote:
> > Why is zvol_volsize_to_reservation_impl() in the header file? Seems
> > like it should be static.
> >
> > Looks good aside from that.
>
> Because some external libzfs users might want to use the algorithm
> itself. This way external storage management tools can know how to set a
> refreservation to completely contain a volume without actually having to
> contain a separate copy of the algorithm.
>
> Cheers,
> --
> Saso
>



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Saso Kiselkov
2013-08-10 22:12:15 UTC
Permalink
On 8/10/13 9:02 PM, Matthew Ahrens wrote:
> It just seems strange to me that we need 2, nearly identical functions
> that do basically the same thing. zvol_volsize_to_reservation()
> and zvol_volsize_to_reservation_impl(). Can't external consumers use
> zvol_volsize_to_reservation()?

The existing zvol_volsize_to_reservation interface takes an nvlist with
hidden properties inside it that the caller needs to pre-fill, rather
than directly pass the requisite values as part of the function call. Is
it usable? Sure. But it's ugly. It forces consumers of the API to jump
through hoops just to run a simple calculation and it sidesteps compiler
checks on argument correctness.

Cheers,
--
Saso
Saso Kiselkov
2013-10-14 21:41:34 UTC
Permalink
On 8/10/13 11:12 PM, Saso Kiselkov wrote:
> On 8/10/13 9:02 PM, Matthew Ahrens wrote:
>> It just seems strange to me that we need 2, nearly identical functions
>> that do basically the same thing. zvol_volsize_to_reservation()
>> and zvol_volsize_to_reservation_impl(). Can't external consumers use
>> zvol_volsize_to_reservation()?
>
> The existing zvol_volsize_to_reservation interface takes an nvlist with
> hidden properties inside it that the caller needs to pre-fill, rather
> than directly pass the requisite values as part of the function call. Is
> it usable? Sure. But it's ugly. It forces consumers of the API to jump
> through hoops just to run a simple calculation and it sidesteps compiler
> checks on argument correctness.

Can I please also get some reviews on this? Should I rename the function
to zvol_volsize_to_reservation2 or something? Should I restructure it
differently? As it stands, this is a bug and it will trip up people
playing around with refreserv on their zvols.

Cheers,
--
Saso
Matthew Ahrens
2013-10-14 21:57:55 UTC
Permalink
On Mon, Oct 14, 2013 at 2:41 PM, Saso Kiselkov <***@gmail.com>wrote:

> On 8/10/13 11:12 PM, Saso Kiselkov wrote:
> > On 8/10/13 9:02 PM, Matthew Ahrens wrote:
> >> It just seems strange to me that we need 2, nearly identical functions
> >> that do basically the same thing. zvol_volsize_to_reservation()
> >> and zvol_volsize_to_reservation_impl(). Can't external consumers use
> >> zvol_volsize_to_reservation()?
> >
> > The existing zvol_volsize_to_reservation interface takes an nvlist with
> > hidden properties inside it that the caller needs to pre-fill, rather
> > than directly pass the requisite values as part of the function call. Is
> > it usable? Sure. But it's ugly. It forces consumers of the API to jump
> > through hoops just to run a simple calculation and it sidesteps compiler
> > checks on argument correctness.
>
> Can I please also get some reviews on this? Should I rename the function
> to zvol_volsize_to_reservation2 or something? Should I restructure it
> differently? As it stands, this is a bug and it will trip up people
> playing around with refreserv on their zvols.
>
>
You are creating an API without a consumer. I don't think that's a great
idea. If your new API is better than the old one (it does seem nicer to
me), how about changing the existing consumer to use it?

--matt



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Saso Kiselkov
2013-10-14 22:04:24 UTC
Permalink
On 10/14/13 10:57 PM, Matthew Ahrens wrote:
> You are creating an API without a consumer. I don't think that's a
> great idea. If your new API is better than the old one (it does seem
> nicer to me), how about changing the existing consumer to use it?

There is a number of libzfs consumers (most of which are closed source,
we had one at DEY) who would get tripped up if we changed the committed
external API, even though it's not marked as stable. Thus I'd prefer
creating a new API call. As for marking it as static/internal, I'd
rather we expose it, since it's useful in storage management software
(hence why we used it at DEY).

Cheers,
--
Saso
Matthew Ahrens
2013-10-14 22:11:55 UTC
Permalink
On Mon, Oct 14, 2013 at 3:04 PM, Saso Kiselkov <***@gmail.com>wrote:

> On 10/14/13 10:57 PM, Matthew Ahrens wrote:
> > You are creating an API without a consumer. I don't think that's a
> > great idea. If your new API is better than the old one (it does seem
> > nicer to me), how about changing the existing consumer to use it?
>
> There is a number of libzfs consumers (most of which are closed source,
> we had one at DEY) who would get tripped up if we changed the committed
> external API, even though it's not marked as stable.


libzfs is not a committed API! Though I'll accept the argument that we
should not break consumers if we don't have to.


> Thus I'd prefer
> creating a new API call. As for marking it as static/internal, I'd
> rather we expose it, since it's useful in storage management software
> (hence why we used it at DEY).


All good arguments. Can you also answer my question:

If your new API is better than the old one (it does seem nicer to me), how
about changing the existing consumer to use it?

--matt



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Saso Kiselkov
2013-10-14 22:17:16 UTC
Permalink
On 10/14/13 11:11 PM, Matthew Ahrens wrote:
> On Mon, Oct 14, 2013 at 3:04 PM, Saso Kiselkov <***@gmail.com
> <mailto:***@gmail.com>> wrote:
>
> On 10/14/13 10:57 PM, Matthew Ahrens wrote:
> > You are creating an API without a consumer. I don't think that's a
> > great idea. If your new API is better than the old one (it does seem
> > nicer to me), how about changing the existing consumer to use it?
>
> There is a number of libzfs consumers (most of which are closed source,
> we had one at DEY) who would get tripped up if we changed the committed
> external API, even though it's not marked as stable.
>
>
> libzfs is not a committed API! Though I'll accept the argument that we
> should not break consumers if we don't have to.
>
>
> Thus I'd prefer
> creating a new API call. As for marking it as static/internal, I'd
> rather we expose it, since it's useful in storage management software
> (hence why we used it at DEY).
>
>
> All good arguments. Can you also answer my question:
>
> If your new API is better than the old one (it does seem nicer to me),
> how about changing the existing consumer to use it?

I thought I addressed that by saying that there *are* external
consumers, only that they're not public. I understand that the API is
not committed and so it's a "your own fault" kind of situation if their
applications break, but why antagonize our users if we don't have to?

I don't feel very strongly either way, I'd just like to get this
resolved. I just happen to think that creating a new function and
perhaps marking the old one as deprecated is the cleanest solution with
the fewest troubles down the road.

Cheers,
--
Saso
Richard Laager
2013-10-14 22:30:51 UTC
Permalink
On Mon, 2013-10-14 at 23:17 +0100, Saso Kiselkov wrote:
> On 10/14/13 11:11 PM, Matthew Ahrens wrote:
> > If your new API is better than the old one (it does seem nicer to me),
> > how about changing the existing consumer to use it?
>
> I thought I addressed that by saying that there *are* external
> consumers, only that they're not public.

Are there any internal consumers? If so, have you updated none, some, or
all of them?

--
Saso Kiselkov
2013-10-14 22:35:03 UTC
Permalink
On 10/14/13 11:30 PM, Richard Laager wrote:
> On Mon, 2013-10-14 at 23:17 +0100, Saso Kiselkov wrote:
>> On 10/14/13 11:11 PM, Matthew Ahrens wrote:
>>> If your new API is better than the old one (it does seem nicer to me),
>>> how about changing the existing consumer to use it?
>>
>> I thought I addressed that by saying that there *are* external
>> consumers, only that they're not public.
>
> Are there any internal consumers? If so, have you updated none, some, or
> all of them?
>

I changed libzfs so that zvol_volsize_to_reservation internally calls
zvol_volsize_to_reservation_impl to do the actual computation.

--
Saso
Matthew Ahrens
2013-10-14 22:31:44 UTC
Permalink
On Mon, Oct 14, 2013 at 3:17 PM, Saso Kiselkov <***@gmail.com>wrote:

> On 10/14/13 11:11 PM, Matthew Ahrens wrote:
> > On Mon, Oct 14, 2013 at 3:04 PM, Saso Kiselkov <***@gmail.com
> > <mailto:***@gmail.com>> wrote:
> >
> > On 10/14/13 10:57 PM, Matthew Ahrens wrote:
> > > You are creating an API without a consumer. I don't think that's a
> > > great idea. If your new API is better than the old one (it does
> seem
> > > nicer to me), how about changing the existing consumer to use it?
> >
> > There is a number of libzfs consumers (most of which are closed
> source,
> > we had one at DEY) who would get tripped up if we changed the
> committed
> > external API, even though it's not marked as stable.
> >
> >
> > libzfs is not a committed API! Though I'll accept the argument that we
> > should not break consumers if we don't have to.
> >
> >
> > Thus I'd prefer
> > creating a new API call. As for marking it as static/internal, I'd
> > rather we expose it, since it's useful in storage management software
> > (hence why we used it at DEY).
> >
> >
> > All good arguments. Can you also answer my question:
> >
> > If your new API is better than the old one (it does seem nicer to me),
> > how about changing the existing consumer to use it?
>
> I thought I addressed that by saying that there *are* external
> consumers, only that they're not public. I understand that the API is
> not committed and so it's a "your own fault" kind of situation if their
> applications break, but why antagonize our users if we don't have to?
>
>
To me, that sounds like an argument to not remove the existing API, which
I'm (begrudgingly) OK with.

My question was about changing the existing consumer to use the new API.
Specifically, I am asking you to consider changing zfs_do_create() to call
the new API.

--matt



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Saso Kiselkov
2013-10-14 22:37:53 UTC
Permalink
On 10/14/13 11:31 PM, Matthew Ahrens wrote:
> My question was about changing the existing consumer to use the new API.
> Specifically, I am asking you to consider changing zfs_do_create() to
> call the new API.

I see your point. I'll take a look at changing zfs(1M) to use it.

Cheers,
--
Saso
Richard Elling
2013-10-15 03:12:55 UTC
Permalink
On Oct 14, 2013, at 3:31 PM, Matthew Ahrens <***@delphix.com> wrote:

> On Mon, Oct 14, 2013 at 3:17 PM, Saso Kiselkov <***@gmail.com> wrote:
> On 10/14/13 11:11 PM, Matthew Ahrens wrote:
> > On Mon, Oct 14, 2013 at 3:04 PM, Saso Kiselkov <***@gmail.com
> > <mailto:***@gmail.com>> wrote:
> >
> > On 10/14/13 10:57 PM, Matthew Ahrens wrote:
> > > You are creating an API without a consumer. I don't think that's a
> > > great idea. If your new API is better than the old one (it does seem
> > > nicer to me), how about changing the existing consumer to use it?
> >
> > There is a number of libzfs consumers (most of which are closed source,
> > we had one at DEY) who would get tripped up if we changed the committed
> > external API, even though it's not marked as stable.
> >
> >
> > libzfs is not a committed API! Though I'll accept the argument that we
> > should not break consumers if we don't have to.
> >
> >
> > Thus I'd prefer
> > creating a new API call. As for marking it as static/internal, I'd
> > rather we expose it, since it's useful in storage management software
> > (hence why we used it at DEY).
> >
> >
> > All good arguments. Can you also answer my question:
> >
> > If your new API is better than the old one (it does seem nicer to me),
> > how about changing the existing consumer to use it?
>
> I thought I addressed that by saying that there *are* external
> consumers, only that they're not public. I understand that the API is
> not committed and so it's a "your own fault" kind of situation if their
> applications break, but why antagonize our users if we don't have to?
>
>
> To me, that sounds like an argument to not remove the existing API, which I'm (begrudgingly) OK with.
>
> My question was about changing the existing consumer to use the new API. Specifically, I am asking you to consider changing zfs_do_create() to call the new API.

I like this approach. +1
-- richard

--

***@RichardElling.com
+1-760-896-4422












-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Saso Kiselkov
2013-10-15 13:02:29 UTC
Permalink
On 10/15/13 4:12 AM, Richard Elling wrote:
>
> On Oct 14, 2013, at 3:31 PM, Matthew Ahrens <***@delphix.com
> <mailto:***@delphix.com>> wrote:
>
>> On Mon, Oct 14, 2013 at 3:17 PM, Saso Kiselkov <***@gmail.com
>> <mailto:***@gmail.com>> wrote:
>>
>> On 10/14/13 11:11 PM, Matthew Ahrens wrote:
>> > On Mon, Oct 14, 2013 at 3:04 PM, Saso Kiselkov
>> <***@gmail.com <mailto:***@gmail.com>
>> > <mailto:***@gmail.com <mailto:***@gmail.com>>>
>> wrote:
>> >
>> > On 10/14/13 10:57 PM, Matthew Ahrens wrote:
>> > > You are creating an API without a consumer. I don't think
>> that's a
>> > > great idea. If your new API is better than the old one
>> (it does seem
>> > > nicer to me), how about changing the existing consumer to
>> use it?
>> >
>> > There is a number of libzfs consumers (most of which are
>> closed source,
>> > we had one at DEY) who would get tripped up if we changed
>> the committed
>> > external API, even though it's not marked as stable.
>> >
>> >
>> > libzfs is not a committed API! Though I'll accept the argument
>> that we
>> > should not break consumers if we don't have to.
>> >
>> >
>> > Thus I'd prefer
>> > creating a new API call. As for marking it as
>> static/internal, I'd
>> > rather we expose it, since it's useful in storage management
>> software
>> > (hence why we used it at DEY).
>> >
>> >
>> > All good arguments. Can you also answer my question:
>> >
>> > If your new API is better than the old one (it does seem nicer
>> to me),
>> > how about changing the existing consumer to use it?
>>
>> I thought I addressed that by saying that there *are* external
>> consumers, only that they're not public. I understand that the API is
>> not committed and so it's a "your own fault" kind of situation if
>> their
>> applications break, but why antagonize our users if we don't have to?
>>
>>
>> To me, that sounds like an argument to not remove the existing API,
>> which I'm (begrudgingly) OK with.
>>
>> My question was about changing the existing consumer to use the new
>> API. Specifically, I am asking you to consider changing
>> zfs_do_create() to call the new API.
>
> I like this approach. +1

Done: http://cr.illumos.org/~webrev/skiselkov/4012/
Please review.

--
Saso
George Wilson
2013-10-15 13:49:50 UTC
Permalink
On 10/15/13 9:02 AM, Saso Kiselkov wrote:
> On 10/15/13 4:12 AM, Richard Elling wrote:
>> On Oct 14, 2013, at 3:31 PM, Matthew Ahrens <***@delphix.com
>> <mailto:***@delphix.com>> wrote:
>>
>>> On Mon, Oct 14, 2013 at 3:17 PM, Saso Kiselkov <***@gmail.com
>>> <mailto:***@gmail.com>> wrote:
>>>
>>> On 10/14/13 11:11 PM, Matthew Ahrens wrote:
>>> > On Mon, Oct 14, 2013 at 3:04 PM, Saso Kiselkov
>>> <***@gmail.com <mailto:***@gmail.com>
>>> > <mailto:***@gmail.com <mailto:***@gmail.com>>>
>>> wrote:
>>> >
>>> > On 10/14/13 10:57 PM, Matthew Ahrens wrote:
>>> > > You are creating an API without a consumer. I don't think
>>> that's a
>>> > > great idea. If your new API is better than the old one
>>> (it does seem
>>> > > nicer to me), how about changing the existing consumer to
>>> use it?
>>> >
>>> > There is a number of libzfs consumers (most of which are
>>> closed source,
>>> > we had one at DEY) who would get tripped up if we changed
>>> the committed
>>> > external API, even though it's not marked as stable.
>>> >
>>> >
>>> > libzfs is not a committed API! Though I'll accept the argument
>>> that we
>>> > should not break consumers if we don't have to.
>>> >
>>> >
>>> > Thus I'd prefer
>>> > creating a new API call. As for marking it as
>>> static/internal, I'd
>>> > rather we expose it, since it's useful in storage management
>>> software
>>> > (hence why we used it at DEY).
>>> >
>>> >
>>> > All good arguments. Can you also answer my question:
>>> >
>>> > If your new API is better than the old one (it does seem nicer
>>> to me),
>>> > how about changing the existing consumer to use it?
>>>
>>> I thought I addressed that by saying that there *are* external
>>> consumers, only that they're not public. I understand that the API is
>>> not committed and so it's a "your own fault" kind of situation if
>>> their
>>> applications break, but why antagonize our users if we don't have to?
>>>
>>>
>>> To me, that sounds like an argument to not remove the existing API,
>>> which I'm (begrudgingly) OK with.
>>>
>>> My question was about changing the existing consumer to use the new
>>> API. Specifically, I am asking you to consider changing
>>> zfs_do_create() to call the new API.
>> I like this approach. +1
> Done: http://cr.illumos.org/~webrev/skiselkov/4012/
> Please review.
>
This still seems odd to me. If the best interface is to have all
consumers call zvol_volsize_to_reservation() with volsize, blocksize,
and copies then why not just make that the interface that everyone uses
(including zfs_do_create)?

I think we should avoid exposing any impl functions to external consumers.

- George
Saso Kiselkov
2013-10-15 13:52:38 UTC
Permalink
On 10/15/13 2:49 PM, George Wilson wrote:
> On 10/15/13 9:02 AM, Saso Kiselkov wrote:
>> On 10/15/13 4:12 AM, Richard Elling wrote:
>>> On Oct 14, 2013, at 3:31 PM, Matthew Ahrens <***@delphix.com
>>> <mailto:***@delphix.com>> wrote:
>>>
>>>> On Mon, Oct 14, 2013 at 3:17 PM, Saso Kiselkov <***@gmail.com
>>>> <mailto:***@gmail.com>> wrote:
>>>>
>>>> On 10/14/13 11:11 PM, Matthew Ahrens wrote:
>>>> > On Mon, Oct 14, 2013 at 3:04 PM, Saso Kiselkov
>>>> <***@gmail.com <mailto:***@gmail.com>
>>>> > <mailto:***@gmail.com <mailto:***@gmail.com>>>
>>>> wrote:
>>>> >
>>>> > On 10/14/13 10:57 PM, Matthew Ahrens wrote:
>>>> > > You are creating an API without a consumer. I don't think
>>>> that's a
>>>> > > great idea. If your new API is better than the old one
>>>> (it does seem
>>>> > > nicer to me), how about changing the existing consumer to
>>>> use it?
>>>> >
>>>> > There is a number of libzfs consumers (most of which are
>>>> closed source,
>>>> > we had one at DEY) who would get tripped up if we changed
>>>> the committed
>>>> > external API, even though it's not marked as stable.
>>>> >
>>>> >
>>>> > libzfs is not a committed API! Though I'll accept the argument
>>>> that we
>>>> > should not break consumers if we don't have to.
>>>> >
>>>> >
>>>> > Thus I'd prefer
>>>> > creating a new API call. As for marking it as
>>>> static/internal, I'd
>>>> > rather we expose it, since it's useful in storage management
>>>> software
>>>> > (hence why we used it at DEY).
>>>> >
>>>> >
>>>> > All good arguments. Can you also answer my question:
>>>> >
>>>> > If your new API is better than the old one (it does seem nicer
>>>> to me),
>>>> > how about changing the existing consumer to use it?
>>>>
>>>> I thought I addressed that by saying that there *are* external
>>>> consumers, only that they're not public. I understand that the
>>>> API is
>>>> not committed and so it's a "your own fault" kind of situation if
>>>> their
>>>> applications break, but why antagonize our users if we don't
>>>> have to?
>>>>
>>>>
>>>> To me, that sounds like an argument to not remove the existing API,
>>>> which I'm (begrudgingly) OK with.
>>>>
>>>> My question was about changing the existing consumer to use the new
>>>> API. Specifically, I am asking you to consider changing
>>>> zfs_do_create() to call the new API.
>>> I like this approach. +1
>> Done: http://cr.illumos.org/~webrev/skiselkov/4012/
>> Please review.
>>
> This still seems odd to me. If the best interface is to have all
> consumers call zvol_volsize_to_reservation() with volsize, blocksize,
> and copies then why not just make that the interface that everyone uses
> (including zfs_do_create)?
>
> I think we should avoid exposing any impl functions to external consumers.

The _impl part is just a name, you could equally call it
"zvol_volsize_to_reservation2" or something. The reason why I don't want
to change the original function is that external users could be
referencing it and changing the call signature would break them.

--
Saso
George Wilson
2013-10-15 14:08:46 UTC
Permalink
On 10/15/13 9:52 AM, Saso Kiselkov wrote:
> On 10/15/13 2:49 PM, George Wilson wrote:
>> On 10/15/13 9:02 AM, Saso Kiselkov wrote:
>>> On 10/15/13 4:12 AM, Richard Elling wrote:
>>>> On Oct 14, 2013, at 3:31 PM, Matthew Ahrens <***@delphix.com
>>>> <mailto:***@delphix.com>> wrote:
>>>>
>>>>> On Mon, Oct 14, 2013 at 3:17 PM, Saso Kiselkov <***@gmail.com
>>>>> <mailto:***@gmail.com>> wrote:
>>>>>
>>>>> On 10/14/13 11:11 PM, Matthew Ahrens wrote:
>>>>> > On Mon, Oct 14, 2013 at 3:04 PM, Saso Kiselkov
>>>>> <***@gmail.com <mailto:***@gmail.com>
>>>>> > <mailto:***@gmail.com <mailto:***@gmail.com>>>
>>>>> wrote:
>>>>> >
>>>>> > On 10/14/13 10:57 PM, Matthew Ahrens wrote:
>>>>> > > You are creating an API without a consumer. I don't think
>>>>> that's a
>>>>> > > great idea. If your new API is better than the old one
>>>>> (it does seem
>>>>> > > nicer to me), how about changing the existing consumer to
>>>>> use it?
>>>>> >
>>>>> > There is a number of libzfs consumers (most of which are
>>>>> closed source,
>>>>> > we had one at DEY) who would get tripped up if we changed
>>>>> the committed
>>>>> > external API, even though it's not marked as stable.
>>>>> >
>>>>> >
>>>>> > libzfs is not a committed API! Though I'll accept the argument
>>>>> that we
>>>>> > should not break consumers if we don't have to.
>>>>> >
>>>>> >
>>>>> > Thus I'd prefer
>>>>> > creating a new API call. As for marking it as
>>>>> static/internal, I'd
>>>>> > rather we expose it, since it's useful in storage management
>>>>> software
>>>>> > (hence why we used it at DEY).
>>>>> >
>>>>> >
>>>>> > All good arguments. Can you also answer my question:
>>>>> >
>>>>> > If your new API is better than the old one (it does seem nicer
>>>>> to me),
>>>>> > how about changing the existing consumer to use it?
>>>>>
>>>>> I thought I addressed that by saying that there *are* external
>>>>> consumers, only that they're not public. I understand that the
>>>>> API is
>>>>> not committed and so it's a "your own fault" kind of situation if
>>>>> their
>>>>> applications break, but why antagonize our users if we don't
>>>>> have to?
>>>>>
>>>>>
>>>>> To me, that sounds like an argument to not remove the existing API,
>>>>> which I'm (begrudgingly) OK with.
>>>>>
>>>>> My question was about changing the existing consumer to use the new
>>>>> API. Specifically, I am asking you to consider changing
>>>>> zfs_do_create() to call the new API.
>>>> I like this approach. +1
>>> Done: http://cr.illumos.org/~webrev/skiselkov/4012/
>>> Please review.
>>>
>> This still seems odd to me. If the best interface is to have all
>> consumers call zvol_volsize_to_reservation() with volsize, blocksize,
>> and copies then why not just make that the interface that everyone uses
>> (including zfs_do_create)?
>>
>> I think we should avoid exposing any impl functions to external consumers.
> The _impl part is just a name, you could equally call it
> "zvol_volsize_to_reservation2" or something. The reason why I don't want
> to change the original function is that external users could be
> referencing it and changing the call signature would break them.
>
And this external consumers are closed source? I'm confused. I thought
the external consumers wanted the new interface (i.e. your new _impl
interface). If that's the case then make that the default. Or are you
saying that there are external consumers that use both? If so, then I
say pick one. I personally like the new interface. It doesn't make sense
to try to maintain two separate interfaces for external consumers that
do the exact same thing. There is no guarantee that someone won't come
in after you and clean this up and break the world.

- George
Saso Kiselkov
2013-10-15 14:14:09 UTC
Permalink
On 10/15/13 3:08 PM, George Wilson wrote:
> And this external consumers are closed source? I'm confused. I thought
> the external consumers wanted the new interface (i.e. your new _impl
> interface). If that's the case then make that the default. Or are you
> saying that there are external consumers that use both? If so, then I
> say pick one. I personally like the new interface. It doesn't make sense
> to try to maintain two separate interfaces for external consumers that
> do the exact same thing. There is no guarantee that someone won't come
> in after you and clean this up and break the world.

No, the point is that external consumers could be using *only* the old
interface and removing it would break them (I don't know of all external
users, but I'm fairly confident that they exist, and probably even where
to look for them). There is a reasonable case to be made, of course,
that since the libzfs API is not committed as stable, breakage can occur
at any time. I don't feel particularly either way. If people insist we
change the function signature, we'll change it (and then I'd propose
bumping the library version as well to avoid unpredictable runtime
behavior).

Cheers,
--
Saso
George Wilson
2013-10-15 14:24:14 UTC
Permalink
On 10/15/13 10:14 AM, Saso Kiselkov wrote:
> On 10/15/13 3:08 PM, George Wilson wrote:
>> And this external consumers are closed source? I'm confused. I thought
>> the external consumers wanted the new interface (i.e. your new _impl
>> interface). If that's the case then make that the default. Or are you
>> saying that there are external consumers that use both? If so, then I
>> say pick one. I personally like the new interface. It doesn't make sense
>> to try to maintain two separate interfaces for external consumers that
>> do the exact same thing. There is no guarantee that someone won't come
>> in after you and clean this up and break the world.
> No, the point is that external consumers could be using *only* the old
> interface and removing it would break them (I don't know of all external
> users, but I'm fairly confident that they exist, and probably even where
> to look for them). There is a reasonable case to be made, of course,
> that since the libzfs API is not committed as stable, breakage can occur
> at any time. I don't feel particularly either way. If people insist we
> change the function signature, we'll change it (and then I'd propose
> bumping the library version as well to avoid unpredictable runtime
> behavior).
>
> Cheers,
Thanks for clarifying this. My opinion is that we just pick one
interface which we think is best suited for consumers. If we want to
force everyone to create an nvlist and pass in the properties then fine.
Otherwise we change it to pass in the explicit values (which I think
makes it more clear) and we bump the library version and force external
consumers to modify their code. I just want to avoid making this problem
worse by allowing external consumers to choose between two interfaces
that do the same thing.

My vote is for changing to the new interface.

- George
g***@gmail.com
2013-10-16 13:41:18 UTC
Permalink
On Oct 15, 2013, at 7:24 AM, George Wilson <***@delphix.com> wrote:

> On 10/15/13 10:14 AM, Saso Kiselkov wrote:
>> On 10/15/13 3:08 PM, George Wilson wrote:
>>> And this external consumers are closed source? I'm confused. I thought
>>> the external consumers wanted the new interface (i.e. your new _impl
>>> interface). If that's the case then make that the default. Or are you
>>> saying that there are external consumers that use both? If so, then I
>>> say pick one. I personally like the new interface. It doesn't make sense
>>> to try to maintain two separate interfaces for external consumers that
>>> do the exact same thing. There is no guarantee that someone won't come
>>> in after you and clean this up and break the world.
>> No, the point is that external consumers could be using *only* the old
>> interface and removing it would break them (I don't know of all external
>> users, but I'm fairly confident that they exist, and probably even where
>> to look for them). There is a reasonable case to be made, of course,
>> that since the libzfs API is not committed as stable, breakage can occur
>> at any time. I don't feel particularly either way. If people insist we
>> change the function signature, we'll change it (and then I'd propose
>> bumping the library version as well to avoid unpredictable runtime
>> behavior).
>>
>> Cheers,
> Thanks for clarifying this. My opinion is that we just pick one interface which we think is best suited for consumers. If we want to force everyone to create an nvlist and pass in the properties then fine. Otherwise we change it to pass in the explicit values (which I think makes it more clear) and we bump the library version and force external consumers to modify their code. I just want to avoid making this problem worse by allowing external consumers to choose between two interfaces that do the same thing.

Given that this is a private interface that seems reasonable. I don't think there is that much harm in leaving the old behind for a while, but given we don't have "releases" as such, its unclear how we we would ever decide that it was time to finally nuke the old one.

I'm ambivalent either way. Ditching the old interface entirely is more consistent with Linux practices than Solaris practices though. :-)

DEY's private use of this no longer really matters, and I'm pretty sure other down streams have their own forks and would be able to insulate themselves against this if they really needed to. (I'm guess Nexenta and Delphix are the most likely cases here, but just guessing.) I'd be more worried about folks with in-house tools, but I suspect that few of those are calling libzfs directly. Much more likely to be wrapping around the ZFS command line tools.

- Garrett
Saso Kiselkov
2013-10-16 14:02:07 UTC
Permalink
On 10/16/13 2:41 PM, ***@gmail.com wrote:
> Given that this is a private interface that seems reasonable. I don't think there is that much harm in leaving the old behind for a while, but given we don't have "releases" as such, its unclear how we we would ever decide that it was time to finally nuke the old one.
>
> I'm ambivalent either way. Ditching the old interface entirely
> is more consistent with Linux practices than Solaris practices
> though. :-)

Agree that it is un-Solarisy to break APIs, but since both you and me
are ambivalent here and George and Matt are opposed, I've gone ahead and
created a new webrev with a modified call signature and bumped library
version:
http://cr.illumos.org/~webrev/skiselkov/4012_new_api/

Let's get a clear-cut decision on this. Frankly, if we can't make any
headway on a change this small I worry about our ability to keep up with
larger changes that are in the queue.

Cheers,
--
Saso
George Wilson
2013-10-16 16:43:26 UTC
Permalink
On 10/16/13 10:02 AM, Saso Kiselkov wrote:
> On 10/16/13 2:41 PM, ***@gmail.com wrote:
>> Given that this is a private interface that seems reasonable. I don't think there is that much harm in leaving the old behind for a while, but given we don't have "releases" as such, its unclear how we we would ever decide that it was time to finally nuke the old one.
>>
>> I'm ambivalent either way. Ditching the old interface entirely
>> is more consistent with Linux practices than Solaris practices
>> though. :-)
> Agree that it is un-Solarisy to break APIs, but since both you and me
> are ambivalent here and George and Matt are opposed, I've gone ahead and
> created a new webrev with a modified call signature and bumped library
> version:
> http://cr.illumos.org/~webrev/skiselkov/4012_new_api/
>
> Let's get a clear-cut decision on this. Frankly, if we can't make any
> headway on a change this small I worry about our ability to keep up with
> larger changes that are in the queue.
>
> Cheers,
First the main code change look good to me (minus the library revision):

On the question of breaking APIs, we have to remember that illumos is a
source repo and not a release. Should we be required to rev the library
in the source repo if it's a private interface? In Nevada we made
changes to private APIs all the time without a library rev. So what is
the model here?

- George
Schlacta, Christ
2013-10-16 16:51:49 UTC
Permalink
The problem is that libzfs needs a stable public interface that's easy to
use and effective. Zfs command line utility should be updated to use the
stable interface, and changes like this should happen primarily in the
unstable internal library layer. I'd like to work on a small number of
useful apps that use libzfs, but of course I'm smart enough to wait until
the interface is stable; apparently unlike some.
On Oct 16, 2013 9:43 AM, "George Wilson" <***@delphix.com> wrote:

> On 10/16/13 10:02 AM, Saso Kiselkov wrote:
>
>> On 10/16/13 2:41 PM, ***@gmail.com wrote:
>>
>>> Given that this is a private interface that seems reasonable. I don't
>>> think there is that much harm in leaving the old behind for a while, but
>>> given we don't have "releases" as such, its unclear how we we would ever
>>> decide that it was time to finally nuke the old one.
>>>
>>> I'm ambivalent either way. Ditching the old interface entirely
>>> is more consistent with Linux practices than Solaris practices
>>> though. :-)
>>>
>> Agree that it is un-Solarisy to break APIs, but since both you and me
>> are ambivalent here and George and Matt are opposed, I've gone ahead and
>> created a new webrev with a modified call signature and bumped library
>> version:
>> http://cr.illumos.org/~webrev/**skiselkov/4012_new_api/<http://cr.illumos.org/~webrev/skiselkov/4012_new_api/>
>>
>> Let's get a clear-cut decision on this. Frankly, if we can't make any
>> headway on a change this small I worry about our ability to keep up with
>> larger changes that are in the queue.
>>
>> Cheers,
>>
> First the main code change look good to me (minus the library revision):
>
> On the question of breaking APIs, we have to remember that illumos is a
> source repo and not a release. Should we be required to rev the library in
> the source repo if it's a private interface? In Nevada we made changes to
> private APIs all the time without a library rev. So what is the model here?
>
> - George
>
>
> ------------------------------**-------------
> illumos-zfs
> Archives: https://www.listbox.com/**member/archive/182191/=now<https://www.listbox.com/member/archive/182191/=now>
> RSS Feed: https://www.listbox.com/**member/archive/rss/182191/**
> 23054485-60ad043a<https://www.listbox.com/member/archive/rss/182191/23054485-60ad043a>
> Modify Your Subscription: https://www.listbox.com/**
> member/?&id_**secret=23054485-335460f5<https://www.listbox.com/member/?&>
> Powered by Listbox: http://www.listbox.com
>



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Saso Kiselkov
2013-10-16 16:59:43 UTC
Permalink
On 10/16/13 5:43 PM, George Wilson wrote:
> On 10/16/13 10:02 AM, Saso Kiselkov wrote:
>> On 10/16/13 2:41 PM, ***@gmail.com wrote:
>>> Given that this is a private interface that seems reasonable. I
>>> don't think there is that much harm in leaving the old behind for a
>>> while, but given we don't have "releases" as such, its unclear how we
>>> we would ever decide that it was time to finally nuke the old one.
>>>
>>> I'm ambivalent either way. Ditching the old interface entirely
>>> is more consistent with Linux practices than Solaris practices
>>> though. :-)
>> Agree that it is un-Solarisy to break APIs, but since both you and me
>> are ambivalent here and George and Matt are opposed, I've gone ahead and
>> created a new webrev with a modified call signature and bumped library
>> version:
>> http://cr.illumos.org/~webrev/skiselkov/4012_new_api/
>>
>> Let's get a clear-cut decision on this. Frankly, if we can't make any
>> headway on a change this small I worry about our ability to keep up with
>> larger changes that are in the queue.
>>
>> Cheers,
> First the main code change look good to me (minus the library revision):
>
> On the question of breaking APIs, we have to remember that illumos is a
> source repo and not a release. Should we be required to rev the library
> in the source repo if it's a private interface? In Nevada we made
> changes to private APIs all the time without a library rev. So what is
> the model here?

Good question. I'm not sure. I think simply stating that "we're a source
repo!" doesn't really absolve us of playing nice with downstream users.
We *do* include a build system that is used to prepare packages and
control versions, so we are at least partly responsible for making sure
things don't explode haphazardly.

Cheers,
--
Saso
Robert Mustacchi
2013-10-16 17:24:17 UTC
Permalink
On 10/16/13 9:59 , Saso Kiselkov wrote:
> On 10/16/13 5:43 PM, George Wilson wrote:
>> On 10/16/13 10:02 AM, Saso Kiselkov wrote:
>>> On 10/16/13 2:41 PM, ***@gmail.com wrote:
>>>> Given that this is a private interface that seems reasonable. I
>>>> don't think there is that much harm in leaving the old behind for a
>>>> while, but given we don't have "releases" as such, its unclear how we
>>>> we would ever decide that it was time to finally nuke the old one.
>>>>
>>>> I'm ambivalent either way. Ditching the old interface entirely
>>>> is more consistent with Linux practices than Solaris practices
>>>> though. :-)
>>> Agree that it is un-Solarisy to break APIs, but since both you and me
>>> are ambivalent here and George and Matt are opposed, I've gone ahead and
>>> created a new webrev with a modified call signature and bumped library
>>> version:
>>> http://cr.illumos.org/~webrev/skiselkov/4012_new_api/
>>>
>>> Let's get a clear-cut decision on this. Frankly, if we can't make any
>>> headway on a change this small I worry about our ability to keep up with
>>> larger changes that are in the queue.
>>>
>>> Cheers,
>> First the main code change look good to me (minus the library revision):
>>
>> On the question of breaking APIs, we have to remember that illumos is a
>> source repo and not a release. Should we be required to rev the library
>> in the source repo if it's a private interface? In Nevada we made
>> changes to private APIs all the time without a library rev. So what is
>> the model here?
>
> Good question. I'm not sure. I think simply stating that "we're a source
> repo!" doesn't really absolve us of playing nice with downstream users.
> We *do* include a build system that is used to prepare packages and
> control versions, so we are at least partly responsible for making sure
> things don't explode haphazardly.

Part of playing nice involves telling people what we consider stable and
what we don't and folks who use the things that we say aren't stable
being willing to accept the fact that they might get broken. It also
underscores the importance of providing useful stable interfaces. Yes
the illumos-gate packages deliver header files and compilation symlinks
to unstable libraries. I think it's worthwhile that we do so folks can
opt to consume an unstable interface with the risk of breakage.

In illumos today, any library that is marked stable, eg. uses versioned
symbols, has manual pages the functionality and stability, etc. will
cause at least folks in illumos to bend over backwards not to break
API/ABI even when it really sucks (see 32-bit libc and FILE *). It
doesn't matter that there isn't a 'release' of illumos for that.
Conversely when something isn't marked stable we do have the right to
break it. Obviously, the costs have to be weighed appropriately, you
don't want to break users arbitrarily. libzfs has no manual pages, only
uses SUNWprivate symbols, and hey, at least it doesn't call exit anymore
for you (I think).

Really this just further underscores the importance of getting to a
stable zfs library interface, which libzfs was never intended to be.
I've certainly been at the receiving end of libzfs breakage and it's
annoying, but really it sounds like we should really be working to get
libzfs_core.so into a state where we can declare it to be stable.

Robert
Saso Kiselkov
2013-10-16 17:30:25 UTC
Permalink
On 10/16/13 6:24 PM, Robert Mustacchi wrote:
> On 10/16/13 9:59 , Saso Kiselkov wrote:
>> On 10/16/13 5:43 PM, George Wilson wrote:
>>> On 10/16/13 10:02 AM, Saso Kiselkov wrote:
>>>> On 10/16/13 2:41 PM, ***@gmail.com wrote:
>>>>> Given that this is a private interface that seems reasonable. I
>>>>> don't think there is that much harm in leaving the old behind for a
>>>>> while, but given we don't have "releases" as such, its unclear how we
>>>>> we would ever decide that it was time to finally nuke the old one.
>>>>>
>>>>> I'm ambivalent either way. Ditching the old interface entirely
>>>>> is more consistent with Linux practices than Solaris practices
>>>>> though. :-)
>>>> Agree that it is un-Solarisy to break APIs, but since both you and me
>>>> are ambivalent here and George and Matt are opposed, I've gone ahead and
>>>> created a new webrev with a modified call signature and bumped library
>>>> version:
>>>> http://cr.illumos.org/~webrev/skiselkov/4012_new_api/
>>>>
>>>> Let's get a clear-cut decision on this. Frankly, if we can't make any
>>>> headway on a change this small I worry about our ability to keep up with
>>>> larger changes that are in the queue.
>>>>
>>>> Cheers,
>>> First the main code change look good to me (minus the library revision):
>>>
>>> On the question of breaking APIs, we have to remember that illumos is a
>>> source repo and not a release. Should we be required to rev the library
>>> in the source repo if it's a private interface? In Nevada we made
>>> changes to private APIs all the time without a library rev. So what is
>>> the model here?
>>
>> Good question. I'm not sure. I think simply stating that "we're a source
>> repo!" doesn't really absolve us of playing nice with downstream users.
>> We *do* include a build system that is used to prepare packages and
>> control versions, so we are at least partly responsible for making sure
>> things don't explode haphazardly.
>
> Part of playing nice involves telling people what we consider stable and
> what we don't and folks who use the things that we say aren't stable
> being willing to accept the fact that they might get broken. It also
> underscores the importance of providing useful stable interfaces. Yes
> the illumos-gate packages deliver header files and compilation symlinks
> to unstable libraries. I think it's worthwhile that we do so folks can
> opt to consume an unstable interface with the risk of breakage.
>
> In illumos today, any library that is marked stable, eg. uses versioned
> symbols, has manual pages the functionality and stability, etc. will
> cause at least folks in illumos to bend over backwards not to break
> API/ABI even when it really sucks (see 32-bit libc and FILE *). It
> doesn't matter that there isn't a 'release' of illumos for that.
> Conversely when something isn't marked stable we do have the right to
> break it. Obviously, the costs have to be weighed appropriately, you
> don't want to break users arbitrarily. libzfs has no manual pages, only
> uses SUNWprivate symbols, and hey, at least it doesn't call exit anymore
> for you (I think).
>
> Really this just further underscores the importance of getting to a
> stable zfs library interface, which libzfs was never intended to be.
> I've certainly been at the receiving end of libzfs breakage and it's
> annoying, but really it sounds like we should really be working to get
> libzfs_core.so into a state where we can declare it to be stable.

Okay, so I've reuploaded the webrev so that it no longer includes a
version bump: http://cr.illumos.org/~webrev/skiselkov/4012_new_api/

--
Saso
Matthew Ahrens
2013-10-16 18:29:57 UTC
Permalink
On Wed, Oct 16, 2013 at 10:30 AM, Saso Kiselkov <***@gmail.com>wrote:

> >> Good question. I'm not sure. I think simply stating that "we're a source
> >> repo!" doesn't really absolve us of playing nice with downstream users.
> >> We *do* include a build system that is used to prepare packages and
> >> control versions, so we are at least partly responsible for making sure
> >> things don't explode haphazardly.
> >
> > Part of playing nice involves telling people what we consider stable and
> > what we don't and folks who use the things that we say aren't stable
> > being willing to accept the fact that they might get broken. It also
> > underscores the importance of providing useful stable interfaces. Yes
> > the illumos-gate packages deliver header files and compilation symlinks
> > to unstable libraries. I think it's worthwhile that we do so folks can
> > opt to consume an unstable interface with the risk of breakage.
> >
> > In illumos today, any library that is marked stable, eg. uses versioned
> > symbols, has manual pages the functionality and stability, etc. will
> > cause at least folks in illumos to bend over backwards not to break
> > API/ABI even when it really sucks (see 32-bit libc and FILE *). It
> > doesn't matter that there isn't a 'release' of illumos for that.
> > Conversely when something isn't marked stable we do have the right to
> > break it. Obviously, the costs have to be weighed appropriately, you
> > don't want to break users arbitrarily. libzfs has no manual pages, only
> > uses SUNWprivate symbols, and hey, at least it doesn't call exit anymore
> > for you (I think).
> >
> > Really this just further underscores the importance of getting to a
> > stable zfs library interface, which libzfs was never intended to be.
> > I've certainly been at the receiving end of libzfs breakage and it's
> > annoying, but really it sounds like we should really be working to get
> > libzfs_core.so into a state where we can declare it to be stable.
>
> Okay, so I've reuploaded the webrev so that it no longer includes a
> version bump: http://cr.illumos.org/~webrev/skiselkov/4012_new_api/
>

Changes look great. Thanks for working out the interface issues, Saso.

--matt



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Matthew Ahrens
2013-11-22 20:31:07 UTC
Permalink
Saso, these changes have been reviewed and are ready to push to illumos.
Please file an RTI for them.

--matt


On Wed, Oct 16, 2013 at 11:29 AM, Matthew Ahrens <***@delphix.com>wrote:

> On Wed, Oct 16, 2013 at 10:30 AM, Saso Kiselkov <***@gmail.com>wrote:
>
>> >> Good question. I'm not sure. I think simply stating that "we're a
>> source
>> >> repo!" doesn't really absolve us of playing nice with downstream users.
>> >> We *do* include a build system that is used to prepare packages and
>> >> control versions, so we are at least partly responsible for making sure
>> >> things don't explode haphazardly.
>> >
>> > Part of playing nice involves telling people what we consider stable and
>> > what we don't and folks who use the things that we say aren't stable
>> > being willing to accept the fact that they might get broken. It also
>> > underscores the importance of providing useful stable interfaces. Yes
>> > the illumos-gate packages deliver header files and compilation symlinks
>> > to unstable libraries. I think it's worthwhile that we do so folks can
>> > opt to consume an unstable interface with the risk of breakage.
>> >
>> > In illumos today, any library that is marked stable, eg. uses versioned
>> > symbols, has manual pages the functionality and stability, etc. will
>> > cause at least folks in illumos to bend over backwards not to break
>> > API/ABI even when it really sucks (see 32-bit libc and FILE *). It
>> > doesn't matter that there isn't a 'release' of illumos for that.
>> > Conversely when something isn't marked stable we do have the right to
>> > break it. Obviously, the costs have to be weighed appropriately, you
>> > don't want to break users arbitrarily. libzfs has no manual pages, only
>> > uses SUNWprivate symbols, and hey, at least it doesn't call exit anymore
>> > for you (I think).
>> >
>> > Really this just further underscores the importance of getting to a
>> > stable zfs library interface, which libzfs was never intended to be.
>> > I've certainly been at the receiving end of libzfs breakage and it's
>> > annoying, but really it sounds like we should really be working to get
>> > libzfs_core.so into a state where we can declare it to be stable.
>>
>> Okay, so I've reuploaded the webrev so that it no longer includes a
>> version bump: http://cr.illumos.org/~webrev/skiselkov/4012_new_api/
>>
>
> Changes look great. Thanks for working out the interface issues, Saso.
>
> --matt
>



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Saso Kiselkov
2013-11-23 03:07:06 UTC
Permalink
On 11/22/13, 8:31 PM, Matthew Ahrens wrote:
> Saso, these changes have been reviewed and are ready to push to illumos.
> Please file an RTI for them.

Ok, will do.

--
Saso
George Wilson
2013-10-21 15:07:45 UTC
Permalink
On 10/16/13 1:30 PM, Saso Kiselkov wrote:
> On 10/16/13 6:24 PM, Robert Mustacchi wrote:
>> On 10/16/13 9:59 , Saso Kiselkov wrote:
>>> On 10/16/13 5:43 PM, George Wilson wrote:
>>>> On 10/16/13 10:02 AM, Saso Kiselkov wrote:
>>>>> On 10/16/13 2:41 PM, ***@gmail.com wrote:
>>>>>> Given that this is a private interface that seems reasonable. I
>>>>>> don't think there is that much harm in leaving the old behind for a
>>>>>> while, but given we don't have "releases" as such, its unclear how we
>>>>>> we would ever decide that it was time to finally nuke the old one.
>>>>>>
>>>>>> I'm ambivalent either way. Ditching the old interface entirely
>>>>>> is more consistent with Linux practices than Solaris practices
>>>>>> though. :-)
>>>>> Agree that it is un-Solarisy to break APIs, but since both you and me
>>>>> are ambivalent here and George and Matt are opposed, I've gone ahead and
>>>>> created a new webrev with a modified call signature and bumped library
>>>>> version:
>>>>> http://cr.illumos.org/~webrev/skiselkov/4012_new_api/
>>>>>
>>>>> Let's get a clear-cut decision on this. Frankly, if we can't make any
>>>>> headway on a change this small I worry about our ability to keep up with
>>>>> larger changes that are in the queue.
>>>>>
>>>>> Cheers,
>>>> First the main code change look good to me (minus the library revision):
>>>>
>>>> On the question of breaking APIs, we have to remember that illumos is a
>>>> source repo and not a release. Should we be required to rev the library
>>>> in the source repo if it's a private interface? In Nevada we made
>>>> changes to private APIs all the time without a library rev. So what is
>>>> the model here?
>>> Good question. I'm not sure. I think simply stating that "we're a source
>>> repo!" doesn't really absolve us of playing nice with downstream users.
>>> We *do* include a build system that is used to prepare packages and
>>> control versions, so we are at least partly responsible for making sure
>>> things don't explode haphazardly.
>> Part of playing nice involves telling people what we consider stable and
>> what we don't and folks who use the things that we say aren't stable
>> being willing to accept the fact that they might get broken. It also
>> underscores the importance of providing useful stable interfaces. Yes
>> the illumos-gate packages deliver header files and compilation symlinks
>> to unstable libraries. I think it's worthwhile that we do so folks can
>> opt to consume an unstable interface with the risk of breakage.
>>
>> In illumos today, any library that is marked stable, eg. uses versioned
>> symbols, has manual pages the functionality and stability, etc. will
>> cause at least folks in illumos to bend over backwards not to break
>> API/ABI even when it really sucks (see 32-bit libc and FILE *). It
>> doesn't matter that there isn't a 'release' of illumos for that.
>> Conversely when something isn't marked stable we do have the right to
>> break it. Obviously, the costs have to be weighed appropriately, you
>> don't want to break users arbitrarily. libzfs has no manual pages, only
>> uses SUNWprivate symbols, and hey, at least it doesn't call exit anymore
>> for you (I think).
>>
>> Really this just further underscores the importance of getting to a
>> stable zfs library interface, which libzfs was never intended to be.
>> I've certainly been at the receiving end of libzfs breakage and it's
>> annoying, but really it sounds like we should really be working to get
>> libzfs_core.so into a state where we can declare it to be stable.
> Okay, so I've reuploaded the webrev so that it no longer includes a
> version bump: http://cr.illumos.org/~webrev/skiselkov/4012_new_api/
>
Looks good to me.

- George
Matthew Ahrens
2013-10-16 18:27:52 UTC
Permalink
On Wed, Oct 16, 2013 at 10:24 AM, Robert Mustacchi <***@joyent.com> wrote:

>
> Part of playing nice involves telling people what we consider stable and
> what we don't and folks who use the things that we say aren't stable
> being willing to accept the fact that they might get broken. It also
> underscores the importance of providing useful stable interfaces. Yes
> the illumos-gate packages deliver header files and compilation symlinks
> to unstable libraries. I think it's worthwhile that we do so folks can
> opt to consume an unstable interface with the risk of breakage.
>
> In illumos today, any library that is marked stable, eg. uses versioned
> symbols, has manual pages the functionality and stability, etc. will
> cause at least folks in illumos to bend over backwards not to break
> API/ABI even when it really sucks (see 32-bit libc and FILE *). It
> doesn't matter that there isn't a 'release' of illumos for that.
> Conversely when something isn't marked stable we do have the right to
> break it. Obviously, the costs have to be weighed appropriately, you
> don't want to break users arbitrarily. libzfs has no manual pages, only
> uses SUNWprivate symbols, and hey, at least it doesn't call exit anymore
> for you (I think).
>
> Really this just further underscores the importance of getting to a
> stable zfs library interface, which libzfs was never intended to be.
> I've certainly been at the receiving end of libzfs breakage and it's
> annoying, but really it sounds like we should really be working to get
> libzfs_core.so into a state where we can declare it to be stable.
>
>
I completely agree!

--matt



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
g***@gmail.com
2013-10-16 18:40:58 UTC
Permalink
On Oct 16, 2013, at 10:24 AM, Robert Mustacchi <***@joyent.com> wrote:

> On 10/16/13 9:59 , Saso Kiselkov wrote:
>> On 10/16/13 5:43 PM, George Wilson wrote:
>>> On 10/16/13 10:02 AM, Saso Kiselkov wrote:
>>>> On 10/16/13 2:41 PM, ***@gmail.com wrote:
>>>>> Given that this is a private interface that seems reasonable. I
>>>>> don't think there is that much harm in leaving the old behind for a
>>>>> while, but given we don't have "releases" as such, its unclear how we
>>>>> we would ever decide that it was time to finally nuke the old one.
>>>>>
>>>>> I'm ambivalent either way. Ditching the old interface entirely
>>>>> is more consistent with Linux practices than Solaris practices
>>>>> though. :-)
>>>> Agree that it is un-Solarisy to break APIs, but since both you and me
>>>> are ambivalent here and George and Matt are opposed, I've gone ahead and
>>>> created a new webrev with a modified call signature and bumped library
>>>> version:
>>>> http://cr.illumos.org/~webrev/skiselkov/4012_new_api/
>>>>
>>>> Let's get a clear-cut decision on this. Frankly, if we can't make any
>>>> headway on a change this small I worry about our ability to keep up with
>>>> larger changes that are in the queue.
>>>>
>>>> Cheers,
>>> First the main code change look good to me (minus the library revision):
>>>
>>> On the question of breaking APIs, we have to remember that illumos is a
>>> source repo and not a release. Should we be required to rev the library
>>> in the source repo if it's a private interface? In Nevada we made
>>> changes to private APIs all the time without a library rev. So what is
>>> the model here?
>>
>> Good question. I'm not sure. I think simply stating that "we're a source
>> repo!" doesn't really absolve us of playing nice with downstream users.
>> We *do* include a build system that is used to prepare packages and
>> control versions, so we are at least partly responsible for making sure
>> things don't explode haphazardly.
>
> Part of playing nice involves telling people what we consider stable and
> what we don't and folks who use the things that we say aren't stable
> being willing to accept the fact that they might get broken. It also
> underscores the importance of providing useful stable interfaces. Yes
> the illumos-gate packages deliver header files and compilation symlinks
> to unstable libraries. I think it's worthwhile that we do so folks can
> opt to consume an unstable interface with the risk of breakage.
>
> In illumos today, any library that is marked stable, eg. uses versioned
> symbols, has manual pages the functionality and stability, etc. will
> cause at least folks in illumos to bend over backwards not to break
> API/ABI even when it really sucks (see 32-bit libc and FILE *). It
> doesn't matter that there isn't a 'release' of illumos for that.
> Conversely when something isn't marked stable we do have the right to
> break it. Obviously, the costs have to be weighed appropriately, you
> don't want to break users arbitrarily. libzfs has no manual pages, only
> uses SUNWprivate symbols, and hey, at least it doesn't call exit anymore
> for you (I think).
>
> Really this just further underscores the importance of getting to a
> stable zfs library interface, which libzfs was never intended to be.
> I've certainly been at the receiving end of libzfs breakage and it's
> annoying, but really it sounds like we should really be working to get
> libzfs_core.so into a state where we can declare it to be stable.

Agree on all counts. The problem is how badly do we burn people who are having to live with the lack of a public API? (And its not just APIs but ABIs that count too, btw.) I'm ok with the idea that this was a private API and we're going to break some folks -- but I also do think that if it was easy to avoid breaking them, there might be merit in doing so. Along with a great big warning "this legacy PRIVATE interface WILL be removed SOON." What I don't know is how many people will be busted by this change… I suspect its not very many, and that those who will be know how to cope.

I do want to be careful about setting precedents here. We need to be careful about breaking public interfaces, and we need to also move various critical interfaces from being private-only to being public. There are lots of candidates here -- not just libzfs. At DEY we wound up needing to access a bunch of private interfaces in order to get any real work done, unless we just wanted to fork/exec various programs.

Oracle/Sun was always very cautious about raising commitment for interfaces. Too cautious IMO. The situation with libzfs is a good example of that. People needed these interfaces, and since they couldn't get the committed interface, they just pushed on ahead.

We need to be more proactive in providing reasonable public/documented interfaces for those important parts of the system -- data links, networking, zone management, and of course ZFS.

We also need a release management system to tie interface change to, but that's another topic… :-)

- Garrett

>
> Robert
>
>
> -------------------------------------------
> illumos-zfs
> Archives: https://www.listbox.com/member/archive/182191/=now
> RSS Feed: https://www.listbox.com/member/archive/rss/182191/22035932-85c5d227
> Modify Your Subscription: https://www.listbox.com/member/?&
> Powered by Listbox: http://www.listbox.com
Matthew Ahrens
2013-11-23 03:27:42 UTC
Permalink
On Wed, Oct 16, 2013 at 10:24 AM, Robert Mustacchi <***@joyent.com> wrote:

> Really this just further underscores the importance of getting to a
> stable zfs library interface, which libzfs was never intended to be.
> I've certainly been at the receiving end of libzfs breakage and it's
> annoying, but really it sounds like we should really be working to get
> libzfs_core.so into a state where we can declare it to be stable.
>

I think that the Channel Program stuff that Chris Siden & Max Grossman
showed off at the OpenZFS Developer Summit will be a good way to create a
stable interface. It will allow us to more easily accommodate the few
interface changes we've made to libzfs_core so far.

See http://www.open-zfs.org/wiki/Projects/ZFS_Channel_Programs for more
details (note that the slides & video are more up to date than the info in
the page itself).

--matt



-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
Loading...