Possible miss-merge of 4369 implement zfs bookmarks

[Steven Hartland via illumos-zfs]

I've just spotted what looks like a possible miss-merge of:
4369 implement zfs bookmarks

It seems we've lost the snap must be in the specified pool
check as well as the check for error on unmount which was
specifically added by:
3744 zfs shouldn't ignore errors unmounting snapshots

Both seem a little odd.

https://github.com/illumos/illumos-gate/commit/78f171005391b928aaf1642b3206c534ed644332#diff-6296d5738a0663ee611fea974d51e0ca

Prior to this commit this we had:-

static int
zfs_ioc_destroy_snaps(const char *poolname, nvlist_t *innvl, nvlist_t *outnvl)
{
int error, poollen;
nvlist_t *snaps;
nvpair_t *pair;
boolean_t defer;

if (nvlist_lookup_nvlist(innvl, "snaps", &snaps) != 0)
return (SET_ERROR(EINVAL));
defer = nvlist_exists(innvl, "defer");

poollen = strlen(poolname);
for (pair = nvlist_next_nvpair(snaps, NULL); pair != NULL;
pair = nvlist_next_nvpair(snaps, pair)) {
const char *name = nvpair_name(pair);

/*
* The snap must be in the specified pool.
*/
if (strncmp(name, poolname, poollen) != 0 ||
(name[poollen] != '/' && name[poollen] != '@'))
return (SET_ERROR(EXDEV));

error = zfs_unmount_snap(name);
if (error != 0)
return (error);
}

return (dsl_destroy_snapshots_nvl(snaps, defer, outnvl));
}

After it we have:
zfs_ioc_destroy_snaps(const char *poolname, nvlist_t *innvl, nvlist_t *outnvl)
{
nvlist_t *snaps;
nvpair_t *pair;
boolean_t defer;

if (nvlist_lookup_nvlist(innvl, "snaps", &snaps) != 0)
return (SET_ERROR(EINVAL));
defer = nvlist_exists(innvl, "defer");

for (pair = nvlist_next_nvpair(snaps, NULL); pair != NULL;
pair = nvlist_next_nvpair(snaps, pair)) {
(void) zfs_unmount_snap(nvpair_name(pair));
}

return (dsl_destroy_snapshots_nvl(snaps, defer, outnvl));
}

[Matthew Ahrens via illumos-zfs]

On Mon, Sep 29, 2014 at 12:49 PM, Steven Hartland via illumos-zfs <

Post by Steven Hartland via illumos-zfs
4369 implement zfs bookmarks

This is not a mis-merge, at least not from DelphixOS, where the patch is
the same.

Post by Steven Hartland via illumos-zfs
It seems we've lost the snap must be in the specified pool
check as well as the check for error on unmount which was
3744 zfs shouldn't ignore errors unmounting snapshots

I believe that the semantics should be the same as if that code had been
left in place. dsl_destroy_snapshot_check() will fail if a snapshot is in
a different pool. It will also fail if a snapshot is mounted.

Do you have a test case which shows that the current behavior is
undesirable?

--matt

Post by Steven Hartland via illumos-zfs
Both seem a little odd.
https://github.com/illumos/illumos-gate/commit/
78f171005391b928aaf1642b3206c534ed644332#diff-
6296d5738a0663ee611fea974d51e0ca
Prior to this commit this we had:-
static int
zfs_ioc_destroy_snaps(const char *poolname, nvlist_t *innvl, nvlist_t *outnvl)
{
int error, poollen;
nvlist_t *snaps;
nvpair_t *pair;
boolean_t defer;
if (nvlist_lookup_nvlist(innvl, "snaps", &snaps) != 0)
return (SET_ERROR(EINVAL));
defer = nvlist_exists(innvl, "defer");
poollen = strlen(poolname);
for (pair = nvlist_next_nvpair(snaps, NULL); pair != NULL;
pair = nvlist_next_nvpair(snaps, pair)) {
const char *name = nvpair_name(pair);
/*
* The snap must be in the specified pool.
*/
if (strncmp(name, poolname, poollen) != 0 ||
return (SET_ERROR(EXDEV));
error = zfs_unmount_snap(name);
if (error != 0)
return (error);
}
return (dsl_destroy_snapshots_nvl(snaps, defer, outnvl));
}
zfs_ioc_destroy_snaps(const char *poolname, nvlist_t *innvl, nvlist_t *outnvl)
{
nvlist_t *snaps;
nvpair_t *pair;
boolean_t defer;
if (nvlist_lookup_nvlist(innvl, "snaps", &snaps) != 0)
return (SET_ERROR(EINVAL));
defer = nvlist_exists(innvl, "defer");
for (pair = nvlist_next_nvpair(snaps, NULL); pair != NULL;
pair = nvlist_next_nvpair(snaps, pair)) {
(void) zfs_unmount_snap(nvpair_name(pair));
}
return (dsl_destroy_snapshots_nvl(snaps, defer, outnvl));
}
-------------------------------------------
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

[Steven Hartland via illumos-zfs]

----- Original Message -----
From: "Matthew Ahrens" <***@delphix.com>
To: "illumos-zfs" <***@lists.illumos.org>; "Steven Hartland" <***@multiplay.co.uk>
Cc: "developer" <***@open-zfs.org>
Sent: Monday, September 29, 2014 9:52 PM
Subject: Re: [zfs] Possible miss-merge of 4369 implement zfs bookmarks

Post by Matthew Ahrens via illumos-zfs
On Mon, Sep 29, 2014 at 12:49 PM, Steven Hartland via illumos-zfs <

Post by Steven Hartland via illumos-zfs
4369 implement zfs bookmarks

This is not a mis-merge, at least not from DelphixOS, where the patch is
the same.

Post by Steven Hartland via illumos-zfs
It seems we've lost the snap must be in the specified pool
check as well as the check for error on unmount which was
3744 zfs shouldn't ignore errors unmounting snapshots

I believe that the semantics should be the same as if that code had been
left in place. dsl_destroy_snapshot_check() will fail if a snapshot is in
a different pool. It will also fail if a snapshot is mounted.
Do you have a test case which shows that the current behavior is
undesirable?

What actually drew my attention was the loss of the FreeBSD:-
(void) zvol_remove_minors(name);

Which used to run after zfs_unmount_snap(name); in zfs_ioc_destroy_snaps
but ended up in zfs_ioc_destroy_bookmarks.

Tracing back through the history lead to the merge of this change, which
looked a little odd given the previous history explicity added these check, I
assumed for a good reason.

This may be reason enough to keep the checks in FreeBSD as without
it we could destroy the minors even though the snapshot unmount failed
which would be indesireable.

Regards
Steve

[Steven Hartland via illumos-zfs]

----- Original Message -----
From: "Steven Hartland"

Post by Matthew Ahrens via illumos-zfs
I believe that the semantics should be the same as if that code had been
left in place. dsl_destroy_snapshot_check() will fail if a snapshot is in
a different pool. It will also fail if a snapshot is mounted.

Thinking about it, while the command will still fail surely the original
error (why the unmount failed) will be lost and hence its still better to
have this check and return early so the user gets the correct error?

[Matthew Ahrens via illumos-zfs]

----- Original Message ----- From: "Steven Hartland"
I believe that the semantics should be the same as if that code had been

Post by Matthew Ahrens via illumos-zfs
left in place. dsl_destroy_snapshot_check() will fail if a snapshot is in
a different pool. It will also fail if a snapshot is mounted.

Thinking about it, while the command will still fail surely the original
error (why the unmount failed) will be lost and hence its still better to
have this check and return early so the user gets the correct error?

If unmount gives a more specific error, which is properly printed by
userland, then I 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

[Pawel Jakub Dawidek via illumos-zfs]

Post by Steven Hartland via illumos-zfs
----- Original Message -----
Sent: Monday, September 29, 2014 9:52 PM
Subject: Re: [zfs] Possible miss-merge of 4369 implement zfs bookmarks

Post by Matthew Ahrens via illumos-zfs
On Mon, Sep 29, 2014 at 12:49 PM, Steven Hartland via illumos-zfs <

Post by Steven Hartland via illumos-zfs
4369 implement zfs bookmarks

This is not a mis-merge, at least not from DelphixOS, where the patch is
the same.

Post by Steven Hartland via illumos-zfs
It seems we've lost the snap must be in the specified pool
check as well as the check for error on unmount which was
3744 zfs shouldn't ignore errors unmounting snapshots

I believe that the semantics should be the same as if that code had been
left in place. dsl_destroy_snapshot_check() will fail if a snapshot is in
a different pool. It will also fail if a snapshot is mounted.
Do you have a test case which shows that the current behavior is
undesirable?

What actually drew my attention was the loss of the FreeBSD:-
(void) zvol_remove_minors(name);

Jumping in to explain what zvol_remove_minors() really does. In ZFS on
Solaris in the past ZVOLs were created immediately. At some point it
changed so that now (also in illumos) ZVOLs are created on-demand when a
lookup in /dev/ happens. We cannot implement it this way in FreeBSD, so
that in various places we create/destroy/rename GEOM providers so they
are always available.