libzfs_core API compatibility & making 'zfs destroy' honor the absence of -f for snapshots

[Will Andrews]

Hi,

What's the plan for how/when changes to libzfs_core's API can be done?

I'm looking at zfs_destroy_snaps_nvl()/lzc_destroy_snaps()
specifically. The 'zfs destroy' command has an '-f' option which is
supposed to mean "do a force unmount". In the case of snapshots,
however, the absence of this flag is ignored and the snapshots are
always force unmounted, as you can see from:

zfs_do_destroy()
zfs_destroy_snaps_nvl()
lzc_destroy_snaps()
lzc_ioctl() -> ioctl()
--syscall interface--
zfs_ioc_destroy_snaps()
zfs_unmount_snap()
dounmount(..., MS_FORCE, ...)

It seems to me that zfs_destroy_snaps_nvl()/lzc_destroy_snaps() should
accept an args nvlist that already has the "snaps" and "defer" keys
filled in, so the call API doesn't need to be changed to support
additional flags, such as "force".

"force" in this case would be passed down to zfs_unmount_snap() in the
kernel so it has the option of returning an error.

Thoughts?

--Will.

[Matthew Ahrens]

Post by Will Andrews
Hi,
What's the plan for how/when changes to libzfs_core's API can be done?

The API is currently not committed, so we can make changes as necessary.
However we will want to commit to the interface at some point, and not
change it thereafter.

Post by Will Andrews
I'm looking at zfs_destroy_snaps_nvl()/lzc_destroy_snaps()
specifically. The 'zfs destroy' command has an '-f' option which is
supposed to mean "do a force unmount". In the case of snapshots,
however, the absence of this flag is ignored and the snapshots are
zfs_do_destroy()
zfs_destroy_snaps_nvl()
lzc_destroy_snaps()
lzc_ioctl() -> ioctl()
--syscall interface--
zfs_ioc_destroy_snaps()
zfs_unmount_snap()
dounmount(..., MS_FORCE, ...)
It seems to me that zfs_destroy_snaps_nvl()/lzc_destroy_snaps() should
accept an args nvlist that already has the "snaps" and "defer" keys
filled in, so the call API doesn't need to be changed to support
additional flags, such as "force".

That's an interesting idea. The tension is between a more flexible
interface (all nvlist all the time) and a more clearly structured one. It
sounds like you are arguing for an interface which is close to lzc_ioctl(),
which as very little structure.

I'm open to trying this out.

--matt

Post by Will Andrews
"force" in this case would be passed down to zfs_unmount_snap() in the
kernel so it has the option of returning an error.
Thoughts?
--Will.
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460
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

[Will Andrews]

Post by Matthew Ahrens
That's an interesting idea. The tension is between a more flexible
interface (all nvlist all the time) and a more clearly structured one. It
sounds like you are arguing for an interface which is close to lzc_ioctl(),
which as very little structure.
From my perspective, what the interface allows is ultimately going to have

to be checked and handled by the kernel ioctl handler. I'm not sure there
is a good rationale for having complexity on both sides of the syscall
interface.

I can see a future in which an updated version of the ioctl handler might
want to reject certain ioctl requests because the requested behavior is no
longer supported. But even then, at least you'll only have to add that
complexity in one place.

--Will.



-------------------------------------------
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]

Post by Matthew Ahrens

Post by Matthew Ahrens
That's an interesting idea. The tension is between a more flexible
interface (all nvlist all the time) and a more clearly structured one. It
sounds like you are arguing for an interface which is close to lzc_ioctl(),
which as very little structure.

From my perspective, what the interface allows is ultimately going to have
to be checked and handled by the kernel ioctl handler. I'm not sure there
is a good rationale for having complexity on both sides of the syscall
interface.
I can see a future in which an updated version of the ioctl handler might
want to reject certain ioctl requests because the requested behavior is no
longer supported. But even then, at least you'll only have to add that
complexity in one place.

Definitely the kernel needs to check the nvlist.

I think the benefit of a more structured interface is that it is easier to
program to, and it allows the kernel implementation to be more flexible
(because we can change the user-kernel interface, although I guess not in
FreeBSD). If we decide that lzc_ioctl() is the supported interface, then
it could still make sense to have "helper functions" that create the proper
nvlist for you and hand it off to lzc_ioctl(). It is nice to not have to
mess with nvlists for e.g. lzc_snaprange_space(). But I definitely see
your point that we can not change the function signature once the interface
is committed. If we wanted to add an additional parameter to
lzc_snaprange_space(), we'd have to add a new function.

--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

[Will Andrews]

Post by Matthew Ahrens
I think the benefit of a more structured interface is that it is easier to
program to, and it allows the kernel implementation to be more flexible
(because we can change the user-kernel interface, although I guess not in
FreeBSD). If we decide that lzc_ioctl() is the supported interface, then
it could still make sense to have "helper functions" that create the proper
nvlist for you and hand it off to lzc_ioctl(). It is nice to not have to
mess with nvlists for e.g. lzc_snaprange_space(). But I definitely see
your point that we can not change the function signature once the interface
is committed. If we wanted to add an additional parameter to
lzc_snaprange_space(), we'd have to add a new function.

I think helper functions are a fine idea in this context. And having a
structured interface is definitely great too. That said, I think it would
be helpful to draw a line between things we add helpers for and things we
don't. For example, lzc_destroy_snaps() has a 'defer' flag. Flags like
this, in my opinion, don't really justify having a helper for.


On the other subject I know you've asked about before:

On FreeBSD, "boot environments" are not supported for non-ZFS roots.
FreeBSD's interface stability guarantees are generally held only within a
given major branch. FreeBSD doesn't go out of its way to break API/ABI
compatibility, but sometimes it is seen as the appropriate approach to
implement changes. So, this has traditionally meant that upgrading across
major branches involves a two-stage process by which the new kernel is
booted prior to installing the userland. So, that's why FreeBSD cares
about supporting older userland binaries talking to a newer kernel.

If FreeBSD ever implements (and requires) boot environment support for UFS
(and whatever other filesystems are seen as needing it), then yes, FreeBSD
could require that userland and kernel updates to always be installed in
tandem, and throw out a bunch of compatibility code in the process.

That being said, I use BEs on FreeBSD ZFS and wouldn't go back.

--Will.



-------------------------------------------
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