Hi @ahrens, @grwilson,
Are there news about that patch ?
Best regards,
Ganael.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-393063362
-
Hi @grwilson,
Do you have any other concerns / comments about that patch ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-372652991
---
Hi @grwilson,
As a recall, the original problem that led to the patch is described here :
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=204622
> These knobs expose the internals of the product.
Yes, but I don't think exposing the internals of the product is a problem here
because it does
I'll chime in here.
There have been a number of cases in the past where I have wanted the moral
equivalent to dd if=/dev/zero of=/dev/da0 bs=1M
Typically, and this may be FreeBSD specific, it's a case of ZFS thinks the
device is unused, then geom attaches to the device, then ZFS magically sees
One of the guiding principles for zfs is simple administration and it seems
like we're exposing way too many knobs to the administrator. These knobs expose
the internals of the product. For example, if I want to clear the labels, why
wouldn't I just run `zpool labelclear` and have the command fi
martymac commented on this pull request.
> + if (pread64(fd, &label, sizeof (vdev_label_t),
+ label_offset(size, l)) != sizeof (vdev_label_t))
+ return (-1);
+
+ if (!force) {
+
martymac commented on this pull request.
> @@ -709,8 +751,12 @@ zpool_do_labelclear(int argc, char **argv)
}
if (zpool_read_label(fd, &config) != 0) {
- (void) fprintf(stderr,
- gettext("failed to read label from %s\n"), vdev);
+ if
grwilson commented on this pull request.
I have concerns about the ultimate goal for this command. It's unclear if we
really want to "wipe" or just "forget" about this device.
> @@ -709,8 +751,12 @@ zpool_do_labelclear(int argc, char **argv)
}
if (zpool_read_label(fd, &config)
ahrens approved this pull request.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#pullrequestreview-93366574
--
openzfs-developer
Archiv
Hi and happy new year :)
I've updated the patch with latest changes from master. If there are no more
comments, can it be merged upstream ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/
@martymac pushed 1 commit.
29d743b Merge branch 'master' of https://github.com/openzfs/openzfs into
zpool-labelclear-improvements
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/openzfs/openzfs/pull/424/files/7be4c748e5b99478657b945bb
@ahrens Have you had time to take a deeper look at latest changes ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-337288816
--
@martymac I'll take a look at your latest changes.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-328663255
--
openzfs-develope
Compilation warnings have been fixed and all checks have passed.
@ahrens @rmustacc @yuripv Do you have additional comments regarding that PR ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzf
@ahrens My last commit should fix compilation warnings.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-328064611
--
openzfs-dev
@martymac pushed 1 commit.
7be4c74 Move VDEV_LABELS definition to sys/fs/zfs.h
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/openzfs/openzfs/pull/424/files/7485b1b72227ba2c14464c9a8f37b96c338388ca..7be4c748e5b99478657b945bbe10dc54936
> You mean moving VDEV_LABELS definition to sys/fs/zfs.h ?
Yes.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-327854271
--
op
@martymac pushed 1 commit.
7485b1b Follow Matthew Ahrens' recommendations
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/openzfs/openzfs/pull/424/files/e0dbedafea472876343fc49402990c0930896712..7485b1b72227ba2c14464c9a8f37b96c338388ca
martymac commented on this pull request.
> @@ -47,6 +48,7 @@
#include
#include
#include
+#include
Yes, this is needed just for VDEV_LABELS. You mean moving VDEV_LABELS
definition to sys/fs/zfs.h ? I don't know what that would imply for other parts
of the code :/ Help would be welcome h
Hi Matthew,
I've pushed fixes following your recommendations, thanks :)
Only remains warnings about abd stuff. Any hint ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomm
ahrens commented on this pull request.
> @@ -47,6 +48,7 @@
#include
#include
#include
+#include
It's possible that this is causing the compilation (lint) error (see
http://jenkins.open-zfs.org/blue/organizations/jenkins/openzfs%2Fopenzfs/detail/PR-424/4/pipeline).
Is this needed just
Sorry for the latency, I was on vacation last week.
That design sounds OK to me. I (or someone else familiar with this code) still
need to review the code. @yuripv do you want to take another look?
--
You are receiving this because you are subscribed to this thread.
Reply to this email direct
@ahrens Any thoughts on that new patch ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-326551259
--
openzfs-developer
Archives
@ahrens Hi Matthew,
Well, I have modified the patch to try to keep simplicity as well as
flexibility :
- As you suggest, always check label validity by default (but one can use the
-f flag twice to override that behaviour)
- Use the minimal mode by default but provide an option (-w) to wipe (ze
@martymac pushed 4 commits.
dc767f1 Check label validity by default and remove option '-c'
81bb4c6 Invalidate labels the minimal way by default
8c9eeb3 Rewrite boolean test for more readability
e0dbeda Specify -f twice to treat invalid labels as valid
--
You are receiving this because you a
I'm not convinced that people are "used to the fact that this command blindly
zeroes 512KiB at the beginning and the end of the disk". I think that this
command is seldom-used, and when it's used it's for the documented purpose:
"Removes ZFS label information." That said, I'm open to evidence
@ahrens Hi Matthew, what do you think about my previous comments ? Can we leave
the default options as they are ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-32428
What I mean is that people are probably used to the fact that this command
blindly zeroes 512KiB at the beginning and the end of the disk and may (ab)use
that property in a way or another (e.g. in an install script to wipe other
kinds of metadata - shame on me, I remember having done that myself
@martymac What could break if we change the default behavior? It seems like it
would still have the same semantic of leaving the disk without any valid labels.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.
Hi Matthew,
Thanks for your feedback.
My original idea was to extend the command while keeping its default behaviour.
The reason is simple : that command is quite new in OpenZFS world but has
already existed on FreeBSD for about 6 years now, see :
https://svnweb.freebsd.org/base?view=revision&
This adds quite a lot of flexibility to an already obscure subcommand, making
it even harder to know how to use correctly. Is there any reason you wouldn't
always want the `-cm` behavior? What would you think about changing `zpool
labelclear` (without any flags) to always make the minimal modi
Hi,
Thanks for your feedback.
C99LMODE seems to be derived from C99MODE (see Makefile.master) so setting
C99MODE only should be enough ? I've modified the Makefile accordingly.
Regards,
Ganael.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly
@martymac pushed 1 commit.
22fe1ea Use C99_ENABLE macro
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/openzfs/openzfs/pull/424/files/c94b7fd8fef2a1d5ff20f540e5b0c7e338a53d3d..22fe1ea7d7b816181336970f6ba242d89bdda9ba
yuripv approved this pull request.
This looks good to me except for the small nit in the Makefile, thanks!
> INCS += -I../../common/zfs -I$(STATCOMMONDIR)
+C99MODE= -xc99=%all
Please use the $(C99_ENABLE) macro instead for both.
--
You are receiving this because you are subscribed to
Thanks for your feedback! I've just committed changes. Can you check them out ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-315520516
--
@martymac pushed 6 commits.
ccdede9 Use strtonum() instead of strtoll()
31452fc Add buflen check to nvlist_invalidate()
c876bde Move nvlist_invalidate symbol to SUNWprivate_1.1
1561da9 Simplify boolean check syntax
72b508c Avoid compilation warning related to ignored return value of memset()
rmustacc commented on this pull request.
> @@ -2355,6 +2355,18 @@ nvlist_common(nvlist_t *nvl, char *buf, size_t
> *buflen, int encoding,
}
int
+nvlist_invalidate(char *buf)
Shouldn't this function be taking the size of the buffer in question that we're
modifying? I realize that we're onl
yuripv commented on this pull request.
> @@ -198,6 +198,7 @@ SYMBOL_VERSION SUNW_1.1 {
nvlist_alloc;
nvlist_dup;
nvlist_free;
+ nvlist_invalidate;
You can't add the symbols to already existing public version, either move it to
SUNWprivate_1.1, or provide new publi
38 matches
Mail list logo