Re: [developer] ZFS atomics, 64-bit on 32-bit platforms
First, a note that to my shame I discovered Linux interface for atomic
operations only yesterday. And I must say that the proposed zatomic interface
looks very similar to what Linux has. I guess that this is not surprising.
Linux defines these types:
typedef struct {
int counter;
} atomic_t;
#ifdef CONFIG_64BIT
typedef struct {
long counter;
} atomic64_t;
#endif
And atomic functions operate on these types.
I got an impression that Linux does not provide atomic64_t operations in 32-bit
kernels. But I got a bit lost in the maze of indirections, so maybe someone
more familiar with Linux could clarify on this.
It is a little bit tempting to propose to switch OpenZFS to Linux atomics and
have other platforms provide compatibility shims for them.
The rest of the reply is inline below:
On 03/10/2019 20:38, Matthew Ahrens wrote:
> On Wed, Oct 2, 2019 at 7:51 AM Andriy Gapon <mailto:a...@freebsd.org>> wrote:
>
>
> This is a work in progress report for my work on safer use of atomics in
> ZFS
> that was discussed in the August developer meeting.
>
> I took the approach suggested at the meeting of creating a new type that
> is to
> be used for all objects that should be modified atomically.
> The type is trivial:
> typedef struct zatomic64 {
> volatile uint64_t val;
> } zatomic64_t;
>
> I also made a decision upfront to use a wrapper type only for 64-bit
> objects as
> 32-bit objects should be already safe on all supported platforms where a
> native
> integer size is either 32 or 64 bit.
>
> Then, as suggested, I wrote trivial wrappers for all 64-bit atomic
> operations
> that are used in ZFS code. For example:
> static inline void
> zatomic_add_64(zatomic64_t *a, uint64_t val)
> {
> atomic_add_64(&a->val, val);
> }
>
> A side note on naming. I simply prepended original function names with
> 'z'.
> Although names like zatomic64_add() could be prettier and more aligned
> with the
> type name.
>
> After that I added three new functions:
> - zatomic_load_64 -- safe (with respect to torn values) read of a 64-bit
> atomic
> - zatomic_store_64 -- safe write of a 64-bit atomic
> - zatomic_init_64 -- unsafe write of a 64-bit atomic
>
> The last function just sets zatomic64_t.val and it is supposed to be used
> when
> it is known that there are no concurrent accesses. It's pretty redundant
> the
> field can be set directly or the whole object can be initialized with x =
> { 0 },
> but I decided to add it for symmetry.
>
> The first two functions are implemented like this:
> static inline uint64_t
> zatomic_load_64(zatomic64_t *a)
> {
> #ifdef __LP64__
> return (a->val);
> #else
> return (zatomic_add_64_nv(a, 0));
> #endif
> }
>
> static inline void
> zatomic_store_64(zatomic64_t *a, uint64_t val)
> {
> #ifdef __LP64__
> a->val = val;
> #else
> (void)zatomic_swap_64(a, 0);
> #endif
> }
>
> I am not sure if this was a right way to do it.
> Maybe there should be a standard implementation only for 64-bit platforms
> and
> each 32-bit platform should do its own thing?
> For example, there is more than one way to get atomic 64-bit loads on x86,
> according to this:
>
> https://stackoverflow.com/questions/48046591/how-do-i-atomically-move-a-64bit-value-in-x86-asm
>
>
> What you did above seems fine to me.
After getting my feet wet in FreeBSD multi-platform support for atomics, I am
not sure about this myself.
Some 32-bit platforms provide adequate 64-bit atomics including load and store
operations. So, checking for just __LP64__ is too naive.
I guess that the check will have to be OS-specific
In fact, on FreeBSD if you have other 64-bit atomic operations, then you will
have load and store for sure.
> Anyway, I started converting ZFS (/FreeBSD) code to use the new type.
> Immediately, I got some problems and some questions.
>
> First, I am quite hesitant about what to do with kstat-s.
> I wanted to confine the change to ZFS, but kstat is an external thing (at
> least
> on illumos). For now I decided to leave the kstat-s alone. Especially
> as I was
> not going to change any code that reads the kstat-s.
> But this also means that some things like arc_c and arc_p, which are
> aliases to
> kstat value, remain potentially unsafe with respect to torn reads.
>
>
> Yeah, we would have to convert these kstats to
Re: [developer] ZFS atomics, 64-bit on 32-bit platforms
On 04/10/2019 02:49, Richard Elling wrote:
> isn't this addressed by the compiler builtins for atomics?
'This' -- what specifically?
I do not think that plain reads of 64-bit objects on 32-bit platforms can be
addressed by anything until they cease being plain.
>> On Oct 3, 2019, at 10:38 AM, Matthew Ahrens > <mailto:mahr...@delphix.com>> wrote:
>>
>> On Wed, Oct 2, 2019 at 7:51 AM Andriy Gapon > <mailto:a...@freebsd.org>> wrote:
>>
>>
>> This is a work in progress report for my work on safer use of atomics in
>> ZFS
>> that was discussed in the August developer meeting.
>>
>> I took the approach suggested at the meeting of creating a new type that
>> is to
>> be used for all objects that should be modified atomically.
>> The type is trivial:
>> typedef struct zatomic64 {
>> volatile uint64_t val;
>> } zatomic64_t;
>>
>> I also made a decision upfront to use a wrapper type only for 64-bit
>> objects as
>> 32-bit objects should be already safe on all supported platforms where a
>> native
>> integer size is either 32 or 64 bit.
>>
>> Then, as suggested, I wrote trivial wrappers for all 64-bit atomic
>> operations
>> that are used in ZFS code. For example:
>> static inline void
>> zatomic_add_64(zatomic64_t *a, uint64_t val)
>> {
>> atomic_add_64(&a->val, val);
>> }
>>
>> A side note on naming. I simply prepended original function names with
>> 'z'.
>> Although names like zatomic64_add() could be prettier and more aligned
>> with the
>> type name.
>>
>> After that I added three new functions:
>> - zatomic_load_64 -- safe (with respect to torn values) read of a 64-bit
>> atomic
>> - zatomic_store_64 -- safe write of a 64-bit atomic
>> - zatomic_init_64 -- unsafe write of a 64-bit atomic
>>
>> The last function just sets zatomic64_t.val and it is supposed to be
>> used when
>> it is known that there are no concurrent accesses. It's pretty
>> redundant the
>> field can be set directly or the whole object can be initialized with x =
>> { 0 },
>> but I decided to add it for symmetry.
>>
>> The first two functions are implemented like this:
>> static inline uint64_t
>> zatomic_load_64(zatomic64_t *a)
>> {
>> #ifdef __LP64__
>> return (a->val);
>> #else
>> return (zatomic_add_64_nv(a, 0));
>> #endif
>> }
>>
>> static inline void
>> zatomic_store_64(zatomic64_t *a, uint64_t val)
>> {
>> #ifdef __LP64__
>> a->val = val;
>> #else
>> (void)zatomic_swap_64(a, 0);
>> #endif
>> }
>>
>> I am not sure if this was a right way to do it.
>> Maybe there should be a standard implementation only for 64-bit
>> platforms and
>> each 32-bit platform should do its own thing?
>> For example, there is more than one way to get atomic 64-bit loads on
>> x86,
>> according to this:
>>
>> https://stackoverflow.com/questions/48046591/how-do-i-atomically-move-a-64bit-value-in-x86-asm
>>
>>
>> What you did above seems fine to me.
>>
>>
>> Anyway, I started converting ZFS (/FreeBSD) code to use the new type.
>> Immediately, I got some problems and some questions.
>>
>> First, I am quite hesitant about what to do with kstat-s.
>> I wanted to confine the change to ZFS, but kstat is an external thing (at
>> least
>> on illumos). For now I decided to leave the kstat-s alone. Especially
>> as
>> I was
>> not going to change any code that reads the kstat-s.
>> But this also means that some things like arc_c and arc_p, which are
>> aliases to
>> kstat value, remain potentially unsafe with respect to torn reads.
>>
>>
>> Yeah, we would have to convert these kstats to have callbacks that do the
>> atomic reads.
>>
>>
>>
>> I have not converted atomics in the aggsum code yet.
>> I am actually a little bit confused about why as_lower_bound and
>> as_upper_bound
>> are manipulated with atomics. All manipulations seems to be done under
>> as_lock.
>> Maybe I overlooked something...
>>
>>
>> Th
[developer] ZFS atomics, 64-bit on 32-bit platforms
This is a work in progress report for my work on safer use of atomics in ZFS
that was discussed in the August developer meeting.
I took the approach suggested at the meeting of creating a new type that is to
be used for all objects that should be modified atomically.
The type is trivial:
typedef struct zatomic64 {
volatile uint64_t val;
} zatomic64_t;
I also made a decision upfront to use a wrapper type only for 64-bit objects as
32-bit objects should be already safe on all supported platforms where a native
integer size is either 32 or 64 bit.
Then, as suggested, I wrote trivial wrappers for all 64-bit atomic operations
that are used in ZFS code. For example:
static inline void
zatomic_add_64(zatomic64_t *a, uint64_t val)
{
atomic_add_64(&a->val, val);
}
A side note on naming. I simply prepended original function names with 'z'.
Although names like zatomic64_add() could be prettier and more aligned with the
type name.
After that I added three new functions:
- zatomic_load_64 -- safe (with respect to torn values) read of a 64-bit atomic
- zatomic_store_64 -- safe write of a 64-bit atomic
- zatomic_init_64 -- unsafe write of a 64-bit atomic
The last function just sets zatomic64_t.val and it is supposed to be used when
it is known that there are no concurrent accesses. It's pretty redundant the
field can be set directly or the whole object can be initialized with x = { 0 },
but I decided to add it for symmetry.
The first two functions are implemented like this:
static inline uint64_t
zatomic_load_64(zatomic64_t *a)
{
#ifdef __LP64__
return (a->val);
#else
return (zatomic_add_64_nv(a, 0));
#endif
}
static inline void
zatomic_store_64(zatomic64_t *a, uint64_t val)
{
#ifdef __LP64__
a->val = val;
#else
(void)zatomic_swap_64(a, 0);
#endif
}
I am not sure if this was a right way to do it.
Maybe there should be a standard implementation only for 64-bit platforms and
each 32-bit platform should do its own thing?
For example, there is more than one way to get atomic 64-bit loads on x86,
according to this:
https://stackoverflow.com/questions/48046591/how-do-i-atomically-move-a-64bit-value-in-x86-asm
Anyway, I started converting ZFS (/FreeBSD) code to use the new type.
Immediately, I got some problems and some questions.
First, I am quite hesitant about what to do with kstat-s.
I wanted to confine the change to ZFS, but kstat is an external thing (at least
on illumos). For now I decided to leave the kstat-s alone. Especially as I was
not going to change any code that reads the kstat-s.
But this also means that some things like arc_c and arc_p, which are aliases to
kstat value, remain potentially unsafe with respect to torn reads.
I have not converted atomics in the aggsum code yet.
I am actually a little bit confused about why as_lower_bound and as_upper_bound
are manipulated with atomics. All manipulations seems to be done under as_lock.
Maybe I overlooked something...
I converted refcount.h.
That had a bit of unexpected effect on rrwlock_t.
zfs_refcount_t is used for rr_anon_rcount and rr_linked_rcount fields.
The fields are always accessed rr_lock, so their atomicity is not actually
required. I guess that zfs_refcount_t is used for the support of references
with ownership. So, some bits of the rrwlock code access those fields through
the refcount interface, but other (conditional) pieces directly access rc_count
as a simple integer.
So, there are a couple of options here. The direct accesses can be replaced
with refcount accesses, but then we get unnecessary atomic operations in the
fast paths. Or the code can keep directly accessing internals of both
zfs_refcount_t and zatomic64_t. Another option is to create a copy of
zfs_refcount_t that relies on external synchronization (so that it does not use
atomics or any internal locks). But that would be a bit of code duplication.
Yet another hard case is znode's z_size.
It's changed with atomic_cas_64() in a single place, in all other places it's
used as a normal integer. On the one hand, there can be a torn read because of
that atomic operation. On the other hand, it's quite messy to update all uses
of z_size.
So, the work is progressing, but I feel like limiting the scope of the change.
Originally I planned to convert everything to zatomic in one go.
But now I feel that just a few things really need that.
Probably os_obj_next_percpu.
Likely zfs_refcount_t.
Anything else?
I am thinking that maybe a few things could be converted initially and the rest
could be converted on as needed basis.
I don't like having a mix of regular atomics and zatomics, but alas.
Any suggestions, comments, observations, ideas, etc are very welcome!
P.S.
Another thought is creeping in: maybe it's not worth bothering with ZFS on
32-bit platforms.
--
Andriy Gapon
-
[developer] zpool replace log device
zpool replace of a top-level log device seems to trigger a resilver that scans the whole pool. That's very different from what happens when the device is removed and re-added. I think that a full scan should not be needed given that the log device contains ZIL blocks only from the current unsync-ed TXGs. I think that the reason for the full scan is this code spa_vdev_attach(): /* * Set newvd's DTL to [TXG_INITIAL, dtl_max_txg) so that we account * for any dmu_sync-ed blocks. It will propagate upward when * spa_vdev_exit() calls vdev_dtl_reassess(). */ dtl_max_txg = txg + TXG_CONCURRENT_STATES; vdev_dtl_dirty(newvd, DTL_MISSING, TXG_INITIAL, dtl_max_txg - TXG_INITIAL); I wonder if fixing the problem could be as easy as using 'txg' instead of TXG_INITIAL in the case vdev_islog is true. As far as I can tell txg is set to spa_last_synced_txg(spa) + 1. What do you think? Thank you! -- Andriy Gapon -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Te6fad9972652421c-M770900762d3b490e7cd6814a Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
[developer] weird resumable receive state
I have got a report, for FreeBSD, about a filesystem getting into a weird state with respect to the resumable receive. The filesystem has non-empty 'receive_resume_token' property. But a send stream generated with that token cannot be received. Also, the receive state cannot be cleared with zfs recv -A. The filesystem's 'inconsistent' property is zero. >From the above I conclude that there is no %recv child. I was able to reproduce that condition -- again, on FreeBSD -- using the following steps. - create a full send stream and make a truncated copy of it (e.g. 50%) - zfs recv -F -s the truncated copy into a new destination dataset - this creates the destination dataset - marks the dataset inconsistent - receives as much data as available in the truncated stream - sets up a partial receive state for the dataset (in its ZAP) - repeat the same command with the same truncated stream again - because of -F, the command is allowed to proceed despite the existing destination - because the destination exists, the stream is received into %recv - a partial receive state is set up on %recv - create a resume stream with zfs send -t using the token from the previous step - zfs recv -F -s the resume stream - this finds %recv and the resume state recorded in it - this completes the receive into %recv - the clone swap is done to move the received objset into the parent dataset - %recv is destroyed - the parent dataset is marked consistent So, in the end we have a fully received dataset in a consistent state, but it still has the partial receive state from the first receive. I think that maybe dmu_recv_end_sync() should clear the resume state on origin_head in the !drc->drc_newfs case like it does for ds in the other case. Or maybe, if we start a new full receive and the destination already exists and it is inconsistent and it has the resume state, then we should assert that the destination has no snapshots and then re-create it, so that we can receive into it directly (without creating %recv). I see two ways of re-creating the destination. Either, we can destroy and create it. Or we can somehow discard all its data, e.g., roll back to @$ORIGIN. Maybe there are other options. What do you think? Thank you! P.S. I have not tried illumos or ZoL yet. Could anyone please try to reproduce? Thanks again. -- Andriy Gapon -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Tbb598d5972318c77-M2aaa54b879588765828c633d Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
Re: [developer] Who wants to implement compression on by default?
On 20/05/2019 01:20, Allan Jude wrote: > On 5/18/19 2:02 PM, Sean Fagan wrote: >> On May 18, 2019, at 1:47 AM, Allan Jude wrote: >>> If you write something else compressible, you should be able to use zdb to >>> see if it ended up using lzjb or lz4. >> >> How? :) >> > > cat /usr/src/sys/kern/*.c /usr/src/sys/kern/*.c > DS1/srcfile > > or even write a file of all 1s instead of all 0s. Just in case the how was about zdb, you can get the file's inode number using either 'stat -s' or 'ls -i' and then do 'zdb -d '. That command will report the file's blockpointers. -- Andriy Gapon -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Tcfdc8da2da71e1c0-Mcdc59228fef6f1ce2ddb8eba Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
[developer] Re: [zfs] 10230 zfs mishandles partial writes
On 14/01/2019 16:46, Jerry Jelinek wrote: > This is a fix for a problem we found with a syscall fuzzer. > > https://us-east.manta.joyent.com/jjelinek/public/webrevs/10230/index.html > Jerry, FWIW, in FreeBSD the code is slightly different: if (error == 0) error = sa_bulk_update(zp->z_sa_hdl, bulk, count, tx); else (void) sa_bulk_update(zp->z_sa_hdl, bulk, count, tx); -- Andriy Gapon -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Tdb8ec7781d11ac91-Mc5b070eca3d557dee135f0cb Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
[developer] Re: [openzfs/openzfs] 9630 add lzc_rename and lzc_destroy to libzfs_core (#660)
@avg-I pushed 1 commit. 6d3ea4b fix up copy-paste errors -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/660/files/9d3e5ae329d085a4c54623af8b30899ce88458d0..6d3ea4b966e57446bd6aad2fff2d3807f5937027 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/T47f1cc3e459efd4d-Md84c27fcc251694d10e68d4e Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9630 add lzc_rename and lzc_destroy to libzfs_core (#660)
@avg-I pushed 4 commits. faddcee make zfs_ioc_destroy and zfs_ioc_rename automatically determine objset type 29f6d88 lzc_rename: assert that libzfs_core is initialized 8b2c4bb move lzc_rename and lzc_destroy to a new version, ILLUMOS_0.3 9d3e5ae ZFS_IOC_DESTROY -> lzc_destroy, ZFS_IOC_RENAME -> lzc_rename in libzfs -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/660/files/95e02b454b015a162ab67bc6ab300c16d45e9b55..9d3e5ae329d085a4c54623af8b30899ce88458d0 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/T47f1cc3e459efd4d-M9672e21850dab1eb19b7c5aa Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9630 add lzc_rename and lzc_destroy to libzfs_core (#660)
avg-I commented on this pull request.
> @@ -249,6 +249,31 @@ lzc_remap(const char *fsname)
return (error);
}
+int
+lzc_rename(const char *source, const char *target)
+{
+ zfs_cmd_t zc = { 0 };
+ int error;
+
Thank you for the advice! I will do this.
--
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/660#discussion_r200630037
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/T47f1cc3e459efd4d-M34207572f0770da858f95039
Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9630 add lzc_rename and lzc_destroy to libzfs_core (#660)
avg-I commented on this pull request. > lzc_create; + lzc_destroy; @ahrens, no problem with doing that but I am curious if this is a recent requirement. I recall that in the past new functions were just added in ILLUMOS_0.1. But I see that there are 0.2 and 0.3 now. -- 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/660#discussion_r200628973 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/T47f1cc3e459efd4d-M26f63ce80a121331dcba4059 Delivery options: https://openzfs.topicbox.com/groups
[developer] Fwd: ZFS destroy caused panic ? 11.2 (r335771)
Below is a report from a ZFS/FreeBSD user.
I am not sure what exactly happened but something concerns me in
zcp_convert_return_values() code.
---
void
zcp_convert_return_values(lua_State *state, nvlist_t *nvl,
const char *key, zcp_eval_arg_t *evalargs)
{
int err;
lua_pushcfunction(state, zcp_lua_to_nvlist_helper);
lua_pushlightuserdata(state, (char *)key);
lua_pushlightuserdata(state, nvl);
lua_pushvalue(state, 1);
lua_remove(state, 1);
err = lua_pcall(state, 3, 0, 0); /* zcp_lua_to_nvlist_helper */
if (err != 0) {
zcp_lua_to_nvlist(state, 1, nvl, ZCP_RET_ERROR); < here
evalargs->ea_result = SET_ERROR(ECHRNG);
}
}
---
I am not sure if we can assume anything about state of 'state' after a call to
lua_pcall. I mean that zcp_lua_to_nvlist() assumes that 'state' would have
something expected at index one.
Forwarded Message
Subject: ZFS destroy caused panic ? 11.2 (r335771)
Date: Tue, 3 Jul 2018 15:19:43 -0400
From: Mike Tancsa
To: FreeBSD-STABLE Mailing List
I got the following panic while deleting a test snapshot
panic: unprotected error in call to Lua API (Invalid value type
'function' for key 'error')
cpuid = 4
KDB: stack backtrace:
#0 0x806fb437 at kdb_backtrace+0x67
#1 0x806b4a57 at vpanic+0x177
#2 0x806b48d3 at panic+0x43
#3 0x814500e4 at zcp_panic_cb+0x24
#4 0x8149b591 at luaD_throw+0x31
#5 0x814a4042 at luaG_errormsg+0x72
#6 0x814ae109 at lua_error+0x9
#7 0x8144f376 at zcp_convert_return_values+0x96
#8 0x8144ff4f at zcp_eval+0x71f
#9 0x813fe9f1 at dsl_destroy_snapshots_nvl+0x1b1
#10 0x81478644 at zfs_ioc_destroy_snaps+0x164
#11 0x8147f269 at zfsdev_ioctl+0x6f9
#12 0x80582418 at devfs_ioctl_f+0x128
#13 0x807174bd at kern_ioctl+0x26d
#14 0x807171dc at sys_ioctl+0x15c
#15 0x80945078 at amd64_syscall+0xa38
#16 0x80923cfd at fast_syscall_common+0x101
I was not able to get a backtrace saved. Any ideas what this might be
about ?
--
---
Mike Tancsa, tel +1 519 651 3400 x203
Sentex Communications, m...@sentex.net
Providing Internet services since 1994 www.sentex.net
Cambridge, Ontario Canada
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/Tb8ec0397edc615b0-M291fda76e6af1ea2edb88dbd
Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 5882 Temporary Pool Names (#600)
@avg-I pushed 1 commit. d1cd233 ashift seems to be a valid pool property in ZoL but not here -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/600/files/dc11cd3b27dcd485b3b4d2ea7d531f0344980723..d1cd233636baebab0580e567b7e6fd168d5a -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/T15f12678e376235c-M8402ff99e81f284e5700bfea Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 0000 a number of light-weight extensions to libzfs_core (#508)
My apologies. I completely forgot about about that plan until your reminder. I have opened #660 for that change. -- 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/508#issuecomment-400839158 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Taa68836d53cca8f3-M8a457eeadae4c7e0b4e48e49 Delivery options: https://openzfs.topicbox.com/groups
[developer] [openzfs/openzfs] 9630 add lzc_rename and lzc_destroy to libzfs_core (#660)
https://www.illumos.org/issues/9630 You can view, comment on, or merge this pull request online at: https://github.com/openzfs/openzfs/pull/660 -- Commit Summary -- * 9630 add lzc_rename and lzc_destroy to libzfs_core -- File Changes -- M usr/src/lib/libzfs_core/common/libzfs_core.c (25) M usr/src/lib/libzfs_core/common/libzfs_core.h (3) M usr/src/lib/libzfs_core/common/mapfile-vers (4) -- Patch Links -- https://github.com/openzfs/openzfs/pull/660.patch https://github.com/openzfs/openzfs/pull/660.diff -- 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/660 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/T47f1cc3e459efd4d-Md60215df4ef7f65fb1389e99 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 5882 Temporary Pool Names (#600)
@avg-I pushed 1 commit. 2d827de perhaps ZoL allows to specify pool name before options but we do not -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/600/files/fd1817c5b4c6b558592ad6f39331984387104404..2d827de153892fda4ca5c768584253ee04f32a00 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T900a082ade0bdbd3-M4599d9a37ba3ed4a4cd2dba9 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 5882 Temporary Pool Names (#600)
zpool_create_tempname failure is obvious and I am pushing a fix. It seems that ZoL allows placing a pool name on zpool create command line before options. E.g., zpool create newpool -o xxx ... import_rewind_device_replaced is not obvious and I am not sure if it's related to this merge request. Some details: ``` 19:08:59.94 SUCCESS: wait_for_pool_config testpool1 /dev_import-test/disk0 /dev_import-test/disk2 19:09:00.32 SUCCESS: zpool export testpool1 19:09:00.32 SUCCESS: mv /dev_import-test/disk2 /dev_import-test/backup/ 19:09:01.20 ERROR: zpool import -d /dev_import-test -T 52 testpool1 exited 1 19:09:01.20 cannot import 'testpool1': one or more devices is currently unavailable ``` >From zfs_dbgmsg log: ``` 19:09:01.94 spa_tryimport: importing testpool1, max_txg=52 19:09:01.94 spa_load($import, config trusted): LOADING 19:09:01.94 file vdev '/dev_import-test/disk0': best uberblock found for spa $import. txg 50 19:09:01.94 file vdev '/dev_import-test/disk0': label discarded as txg is too large (101 > 50) 19:09:01.94 file vdev '/dev_import-test/disk0': failed to read label config. Trying again without txg restrictions. 19:09:01.94 spa_load($import, config untrusted): using uberblock with txg=50 19:09:01.94 spa_load($import, config untrusted): FAILED: unable to open rootbp in dsl_pool_init [error=5] 19:09:01.94 spa_load($import, config untrusted): UNLOADING 19:09:01.94 spa_import: importing testpool1, max_txg=52 (RECOVERY MODE) 19:09:01.94 spa_load(testpool1, config trusted): LOADING 19:09:01.94 file vdev '/dev_import-test/disk0': best uberblock found for spa testpool1. txg 50 19:09:01.94 file vdev '/dev_import-test/disk0': label discarded as txg is too large (101 > 50) 19:09:01.94 file vdev '/dev_import-test/disk0': failed to read label config. Trying again without txg restrictions. 19:09:01.94 spa_load(testpool1, config untrusted): using uberblock with txg=50 19:09:01.94 spa_load(testpool1, config untrusted): FAILED: unable to open rootbp in dsl_pool_init [error=5] 19:09:01.94 spa_load(testpool1, config untrusted): UNLOADING 19:09:01.94 spa_load(testpool1, config untrusted): spa_load_retry: rewind, max txg: 49 ... 19:09:01.94 spa_load(testpool1, config untrusted): spa_load_retry: rewind, max txg: 46 ... 19:09:01.94 spa_load(testpool1, config untrusted): spa_load_retry: rewind, max txg: 43 ... ... 19:09:01.94 spa_load(testpool1, config untrusted): spa_load_retry: rewind, max txg: 4 19:09:01.94 spa_load(testpool1, config untrusted): LOADING 19:09:01.94 file vdev '/dev_import-test/disk0': best uberblock found for spa testpool1. txg 4 19:09:01.94 file vdev '/dev_import-test/disk0': label discarded as txg is too large (101 > 4) 19:09:01.94 file vdev '/dev_import-test/disk0': failed to read label config. Trying again without txg restrictions. 19:09:01.94 spa_load(testpool1, config untrusted): using uberblock with txg=4 19:09:01.94 spa_load(testpool1, config untrusted): FAILED: unable to open rootbp in dsl_pool_init [error=5] 19:09:01.94 spa_load(testpool1, config untrusted): UNLOADING 19:09:01.94 spa_load(testpool1, config untrusted): spa_load_retry: rewind, max txg: 3 19:09:01.94 spa_load(testpool1, config untrusted): LOADING 19:09:01.94 spa_load(testpool1, config untrusted): FAILED: no valid uberblock found 19:09:01.94 spa_load(testpool1, config untrusted): UNLOADING ``` Direct link to the log file [40MB]: http://jenkins.open-zfs.org/job/openzfs/job/openzfs/job/PR-600/9/artifact/run-zfs-tests.log -- 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/600#issuecomment-389138480 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T900a082ade0bdbd3-Mf1333b7f35b38ff6f4d09957 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 5882 Temporary Pool Names (#600)
Two test failures: ``` Tests with results other than PASS that are unexpected: FAIL cli_root/zpool_create/zpool_create_tempname (expected PASS) FAIL cli_root/zpool_import/import_rewind_device_replaced (expected PASS) ``` -- 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/600#issuecomment-389132635 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T900a082ade0bdbd3-M82f9f01215d359147df2485a Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9464 txg_kick() fails to see that we are quiescing, forcing transacti… (#616)
avg-I 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/616#pullrequestreview-119876549 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T482138142f6adde1-M241fc5168c801326f2be Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9464 txg_kick() fails to see that we are quiescing, forcing transacti… (#616)
I didn't inspect the change originally, but I have looked at it now and all parts of it look good to me. I also agree with the analysis. Thanks! -- 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/616#issuecomment-388850680 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T482138142f6adde1-M029796facef39e9a3c001a20 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 5882 Temporary Pool Names (#600)
I've updated with the fix for ZoL 7509 and my attempt to add the new testcases from ZoL. -- 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/600#issuecomment-388840981 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T900a082ade0bdbd3-M5515cfd027cff42d8461cd21 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 5882 Temporary Pool Names (#600)
Most likely yes. I will test. -- 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/600#issuecomment-387482483 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T900a082ade0bdbd3-Mafd5e2fe84153da8bc515740 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] Large alloc in zdb can cause trouble (#635)
avg-I commented on this pull request.
> @@ -4924,12 +4924,18 @@ zdb_embedded_block(char *thing)
exit(1);
}
ASSERT3U(BPE_GET_LSIZE(&bp), <=, SPA_MAXBLOCKSIZE);
+ buf = malloc(SPA_MAXBLOCKSIZE);
+ if (buf == NULL) {
+ (void) printf("out of memory\n");
Then that is another bug to fix.
fprintf(stderr) is what is used predominantly in the file and it's obviously a
better option for error reporting.
--
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/635#discussion_r186390797
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/discussions/T878c8282c1bf80d5-Mc6feb05edf1ea6ccbffa9f0a
Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] Large alloc in zdb can cause trouble (#635)
avg-I commented on this pull request.
> @@ -4924,12 +4924,18 @@ zdb_embedded_block(char *thing)
exit(1);
}
ASSERT3U(BPE_GET_LSIZE(&bp), <=, SPA_MAXBLOCKSIZE);
+ buf = malloc(SPA_MAXBLOCKSIZE);
+ if (buf == NULL) {
+ (void) printf("out of memory\n");
Please use `fprintf(stderr, ...)`
--
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/635#pullrequestreview-117953942
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/discussions/T878c8282c1bf80d5-M28a175683d042aeae72278e8
Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] Large alloc in zdb can cause trouble (#635)
This change is in FreeBSD already: https://svnweb.freebsd.org/base?view=revision&revision=326150 https://svnweb.freebsd.org/base?view=revision&revision=326187 -- 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/635#issuecomment-387027285 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T878c8282c1bf80d5-Mc564c56cb663cf2ffab3639a Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 5882 Temporary Pool Names (#600)
`../port/regex/regcomp.c:1783: fatal error: error writing to /tmp/nightly.tmpdir.2594/ccLaaq0L.s: No space left on device` -- 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/600#issuecomment-381610321 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T900a082ade0bdbd3-M42c479fa4176b0a6ab14ab31 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9464 txg_kick() fails to see that we are quiescing, forcing transacti… (#616)
Thank you! -- 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/616#issuecomment-381527949 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T482138142f6adde1-M77d83cfa252842e086e3c530 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9479 fix wrong format specifier for vdev_id (#623)
avg-I commented on this pull request.
> @@ -159,7 +159,7 @@ vdev_dbgmsg_print_tree(vdev_t *vd, int indent)
}
zfs_dbgmsg("%*svdev %u: %s%s, guid: %llu, path: %s, %s", indent,
- "", vd->vdev_id, vd->vdev_ops->vdev_op_type,
+ "", (int)vd->vdev_id, vd->vdev_ops->vdev_op_type,
I think that either can work, but just adding the int cast seemed like a
simpler change.
And I cannot imagine a pool with billions of vdevs.
--
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/623#discussion_r181656256
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/discussions/T2cb600ac138fbef6-Maaa8edcd2d14b8f7472706f2
Delivery options: https://openzfs.topicbox.com/groups
[developer] [openzfs/openzfs] 9479 fix wrong format specifier for vdev_id (#623)
https://www.illumos.org/issues/9479 You can view, comment on, or merge this pull request online at: https://github.com/openzfs/openzfs/pull/623 -- Commit Summary -- * 9479 fix wrong format specifier for vdev_id -- File Changes -- M usr/src/uts/common/fs/zfs/vdev.c (2) -- Patch Links -- https://github.com/openzfs/openzfs/pull/623.patch https://github.com/openzfs/openzfs/pull/623.diff -- 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/623 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T2cb600ac138fbef6-Mbd45d94d18e6b5e4e1959009 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9237 "zpool add" fails for very large pools (#582)
@asomers , could you please push (and rebase before that) to trigger another build? -- 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/582#issuecomment-381497292 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T5c88e9cffec03ab4-M4811d76c34de8ba84048aa4e Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9464 txg_kick() fails to see that we are quiescing, forcing transacti… (#616)
@brad-lewis could you please provide a link to `txg.d` or a legend for the numbers in the table? Thanks! -- 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/616#issuecomment-381027707 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T482138142f6adde1-M30376a5279269651b8f95805 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9443 panic when scrub a v10 pool (#610)
avg-I approved this pull request. LGTM -- 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/610#pullrequestreview-110247763 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T476cbf523d481025-M3d58479709b12fc6f64f174b Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 5882 Temporary Pool Names (#600)
@avg-I pushed 1 commit. 7806e35 address feedback from Steven Hartland -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/600/files/639a1ca0aedcdcb1931d6c837b3028539b7f9d26..7806e35fe7d016c27e03ca5693a0f7fcef067905 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T900a082ade0bdbd3-Mda2fdda84a094bff67e6fd70 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 5882 Temporary Pool Names (#600)
@avg-I pushed 1 commit. 639a1ca fix add_prop_list_default to not ignore poolprop -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/600/files/b4cd8198df7ec91fb1d45f06514689543ff20a70..639a1ca0aedcdcb1931d6c837b3028539b7f9d26 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T900a082ade0bdbd3-M5cb7b4119a089d15e12dcd06 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 5882 Temporary Pool Names (#600)
@prakashsurya thank you for the report. Hmm, the code is this way in ZoL too. I see two ways of fixing it: 1. remove the parameter 1. pass through the parameter to `add_prop_list` instead of the currently hardcoded `B_TRUE` Both approaches should work as all callers of `add_prop_list_default` pass `B_TRUE` in `poolprop`. I guess that the second approach is more flexible, so I am leaning to it. -- 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/600#issuecomment-379197785 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T900a082ade0bdbd3-Mf4d32e0a1f41eae1277bc735 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 5882 Temporary Pool Names (#600)
@prakashsurya Prakash, no problem and thank you. -- 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/600#issuecomment-378881501 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T900a082ade0bdbd3-M4bd17cf9b23417fea7964ff6 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 0000 Temporary Pool Names (#600)
I am planning to commit this to FreeBSD. -- 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/600#issuecomment-378489518 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/Tcbfe4cdacc89e066-Mee33a7f731469fdb53ebb54c Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 0000 a number of light-weight extensions to libzfs_core (#508)
I can open a separate PR for lzc_rename and lzc_destroy while the rest are being discussed. Regarding lzc_list_*, I need to think about that. The channel program iteration is great if everything is done within a program. But what if userland needs the list? Then the atomicity argument is immediately not applicable. Also, if the list of children is huge, can there be any problems with returning it userland? But I must admit that I haven't yet examined the channel program code well, so I think that I will have to do that before I can discuss further the list issue or the passing of props issue. Thank you for the review and suggestions! -- 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/508#issuecomment-376862781 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T3507ad6a4df1e9bb-M305b025e982d3f145d69bda2 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 0000 a number of light-weight extensions to libzfs_core (#508)
avg-I commented on this pull request.
> +}
+
+int
+lzc_destroy(const char *fsname)
+{
+ int error;
+
+ nvlist_t *args = fnvlist_alloc();
+ error = lzc_ioctl(ZFS_IOC_DESTROY, fsname, args, NULL);
+ nvlist_free(args);
+ return (error);
+}
+
+/* XXX "inherit" received values (zfs inherit -S) ? */
+int
+lzc_inherit_prop(const char *fsname, const char *name)
Will do.
--
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/508#discussion_r177724360
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/discussions/T3507ad6a4df1e9bb-M271145ae6f4bfbf65656d0b8
Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 0000 a number of light-weight extensions to libzfs_core (#508)
avg-I commented on this pull request.
> + (void) strlcpy(zc.zc_value, name, sizeof (zc.zc_value));
+ error = ioctl(g_fd, ZFS_IOC_INHERIT_PROP, &zc);
+ if (error != 0)
+ error = errno;
+ return (error);
+}
+
+int
+lzc_set_prop(const char *fsname, nvlist_t *props)
+{
+ nvpair_t *elem;
+ int error;
+
+ /* the list must have exactly one element */
+ if ((elem = nvlist_next_nvpair(props, NULL)) == NULL ||
+ nvlist_next_nvpair(props, elem) != NULL)
I don't recall the details now, but I think that there were some concerns about
`ZFS_IOC_SET_PROP` being non-atomic when given multiple properties.
As the comment near `zfs_set_prop_nvlist` says:
```
* This function is best effort. If it fails to set any of the given properties,
* it continues to set as many as it can and returns the last error
* encountered. If the caller provides a non-NULL errlist, it will be filled in
* with the list of names of all the properties that failed along with the
* corresponding error numbers.
```
But for some reason we wanted something that does all or nothing, so we opted
to have a single-property interface.
--
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/508#discussion_r177724317
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/discussions/T3507ad6a4df1e9bb-Ma4f4748b4698e2a933f4dbba
Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 0000 Temporary Pool Names (#600)
Looks like the build failed because of this? ``` pkg install: No matching version of metapackages/build-essential can be installed: Reject: pkg://openindiana.org/metapackages/build-essential@1.0-2018.0.0.12 Reason: This version is excluded by installed incorporation consolidation/userland/userland-incorporation@0.5.11-2018.0.0.10515 ``` -- 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/600#issuecomment-376856169 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/Tcbfe4cdacc89e066-Madb40ea62b920bd8898a809d Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 0000 Temporary Pool Names (#600)
I think that the manual page change needs some help with word-smithing and maybe markup. Perhaps it would make sense to create a separate synopsis for `zpool import -t` instead of tackling `-t` into the synopsis of the single pool import. -- 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/600#issuecomment-376849145 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/Tcbfe4cdacc89e066-M0dd2fd17115c3223fb17d117 Delivery options: https://openzfs.topicbox.com/groups
[developer] [openzfs/openzfs] 0000 Temporary Pool Names (#600)
This is a logical resurrection of PR #4. The actual changes are picked up from ZoL again and applied to the latest OpenZFS (via FreeBSD). zfsonlinux/zfs@3500a14595 is included (as suggested in PR #4). Author: Julian Elischer You can view, comment on, or merge this pull request online at: https://github.com/openzfs/openzfs/pull/600 -- Commit Summary -- * Temporary Pool Names -- File Changes -- M usr/src/cmd/zpool/zpool_main.c (63) M usr/src/common/zfs/zpool_prop.c (2) M usr/src/lib/libzfs/common/libzfs_pool.c (9) M usr/src/man/man1m/zpool.1m (30) M usr/src/uts/common/fs/zfs/spa.c (18) M usr/src/uts/common/fs/zfs/spa_config.c (30) M usr/src/uts/common/sys/fs/zfs.h (2) -- Patch Links -- https://github.com/openzfs/openzfs/pull/600.patch https://github.com/openzfs/openzfs/pull/600.diff -- 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/600 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/Tcbfe4cdacc89e066-M5a0d953bc7e0eb74e783fd4b Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 0000 a number of light-weight extensions to libzfs_core (#508)
@ahrens sorry for my not following up. I got busy elsewhere and dropped the ball on this one. I hope to get back to this some time next week. -- 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/508#issuecomment-373294588 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T94e916bc5cb9e1ed-M7e7c38a5e2605a24f824fafc Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9237 "zpool add" fails for very large pools (#582)
avg-I approved this pull request. Looked good for FreeBSD, still looks good. Thank you! -- 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/582#pullrequestreview-101320066 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T5c88e9cffec03ab4-M2c5e980ef6b2df6fa8cdd854 Delivery options: https://openzfs.topicbox.com/groups
[developer] [openzfs/openzfs] 9164 assert: newds == os->os_dsl_dataset (#559)
https://www.illumos.org/issues/9164 You can view, comment on, or merge this pull request online at: https://github.com/openzfs/openzfs/pull/559 -- Commit Summary -- * 9164 assert: newds == os->os_dsl_dataset -- File Changes -- M usr/src/uts/common/fs/zfs/dmu_objset.c (12) M usr/src/uts/common/fs/zfs/sys/dmu_objset.h (3) M usr/src/uts/common/fs/zfs/zfs_ioctl.c (6) -- Patch Links -- https://github.com/openzfs/openzfs/pull/559.patch https://github.com/openzfs/openzfs/pull/559.diff -- 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/559 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td65b945b80fcd432-M47a84dba8b95df2008b78c94 Powered by Topicbox: https://topicbox.com
Re: [developer] newds == os->os_dsl_dataset in dmu_objset_refresh_ownership
Matt, thank you for looking into this. I will open an issue and a PR. On 20/02/2018 19:25, Matthew Ahrens wrote: > I'm pretty sure I wrote this code, but I can't remember why I thought the > dataset (and objset) couldn't be evicted. I don't think > the dsl_pool_config_enter() prevents that. Your change looks good to me. > > --matt > > On Mon, Feb 19, 2018 at 1:30 AM, Andriy Gapon <mailto:a...@freebsd.org>> wrote: > > > dmu_objset_refresh_ownership() first disowns a dataset (and releases it) > and > then owns it again. There is an assert that the new dataset object is > the same > as the old dataset object: > > dsl_pool_config_enter(dp, FTAG); > dmu_objset_disown(os, tag); > VERIFY0(dsl_dataset_own(dp, name, tag, &newds)); > ==> VERIFY3P(newds, ==, os->os_dsl_dataset); > dsl_pool_config_exit(dp, FTAG); > > I cannot see what can really guarantee that assert to be true. > I think that the dataset object is allowed to be evicted, so it's possible > that > dsl_dataset_own() -> dsl_dataset_hold() may need to create a new object. > Maybe I am missing something here. Perhaps what's being asserted used to be > true before some changes in dbuf / dataset eviction.. > > When running ZFS Test Suite on FreeBSD we see this panic from > zpool_upgrade_007_pos test: > > panic: solaris assert: newds == os->os_dsl_dataset (0xf80045f4c000 == > 0xf80021ab4800) > > assfail3() at assfail3+0x2c/frame 0xfe002a621480 > dmu_objset_refresh_ownership() at dmu_objset_refresh_ownership+0x161/frame > 0xfe002a6215c0 > zfs_ioc_userspace_upgrade() at zfs_ioc_userspace_upgrade+0x97/frame > 0xfe002a621600 > zfs_prop_set_special() at zfs_prop_set_special+0x465/frame 0xfe002a621670 > zfs_set_prop_nvlist() at zfs_set_prop_nvlist+0x23f/frame 0xfe002a6216f0 > zfs_ioc_set_prop() at zfs_ioc_set_prop+0x129/frame 0xfe002a621740 > zfsdev_ioctl() at zfsdev_ioctl+0x76b/frame 0xfe002a6217e0 > > I see that the old dataset has dsl_dataset_evict_async() pending in > ds_dbu.dbu_tqent and its ds_dbuf is NULL. > > I've got this patch to fix the problem: > https://paste.debian.net/1010940/ <https://paste.debian.net/1010940/> > > What do you think? > Thank you! > -- > Andriy Gapon > > *openzfs-developer* | Archives > <https://openzfs.topicbox.com/groups/developer/discussions/T9d8097b8fffd8bfa-Mbf114e3ce50bbb0fe2db7ca6> > | Powered by Topicbox <https://topicbox.com> -- Andriy Gapon -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T9d8097b8fffd8bfa-Mf731f816f21b610595e49651 Powered by Topicbox: https://topicbox.com
[developer] newds == os->os_dsl_dataset in dmu_objset_refresh_ownership
dmu_objset_refresh_ownership() first disowns a dataset (and releases it) and then owns it again. There is an assert that the new dataset object is the same as the old dataset object: dsl_pool_config_enter(dp, FTAG); dmu_objset_disown(os, tag); VERIFY0(dsl_dataset_own(dp, name, tag, &newds)); ==> VERIFY3P(newds, ==, os->os_dsl_dataset); dsl_pool_config_exit(dp, FTAG); I cannot see what can really guarantee that assert to be true. I think that the dataset object is allowed to be evicted, so it's possible that dsl_dataset_own() -> dsl_dataset_hold() may need to create a new object. Maybe I am missing something here. Perhaps what's being asserted used to be true before some changes in dbuf / dataset eviction.. When running ZFS Test Suite on FreeBSD we see this panic from zpool_upgrade_007_pos test: panic: solaris assert: newds == os->os_dsl_dataset (0xf80045f4c000 == 0xf80021ab4800) assfail3() at assfail3+0x2c/frame 0xfe002a621480 dmu_objset_refresh_ownership() at dmu_objset_refresh_ownership+0x161/frame 0xfe002a6215c0 zfs_ioc_userspace_upgrade() at zfs_ioc_userspace_upgrade+0x97/frame 0xfe002a621600 zfs_prop_set_special() at zfs_prop_set_special+0x465/frame 0xfe002a621670 zfs_set_prop_nvlist() at zfs_set_prop_nvlist+0x23f/frame 0xfe002a6216f0 zfs_ioc_set_prop() at zfs_ioc_set_prop+0x129/frame 0xfe002a621740 zfsdev_ioctl() at zfsdev_ioctl+0x76b/frame 0xfe002a6217e0 I see that the old dataset has dsl_dataset_evict_async() pending in ds_dbu.dbu_tqent and its ds_dbuf is NULL. I've got this patch to fix the problem: https://paste.debian.net/1010940/ What do you think? Thank you! -- Andriy Gapon -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T9d8097b8fffd8bfa-M0c734e59f49480e8bbe36652 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
Perfect! Please RTI as soon as you can. P.S. I do not understand the latest build failure, seems like an infra problem: http://jenkins.open-zfs.org/blue/organizations/jenkins/openzfs%2Fopenzfs/detail/PR-505/6/pipeline/179 -- 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/505#issuecomment-365344318 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M6791b75296dd19f8791ee297 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
avg-I commented on this pull request.
> @@ -3534,11 +3545,11 @@ zio_done(zio_t *zio)
* If our children haven't all completed,
* wait for them and then repeat this pipeline stage.
*/
- if (zio_wait_for_children(zio, ZIO_CHILD_VDEV, ZIO_WAIT_DONE) ||
- zio_wait_for_children(zio, ZIO_CHILD_GANG, ZIO_WAIT_DONE) ||
- zio_wait_for_children(zio, ZIO_CHILD_DDT, ZIO_WAIT_DONE) ||
- zio_wait_for_children(zio, ZIO_CHILD_LOGICAL, ZIO_WAIT_DONE))
+ if (zio_wait_for_children(zio, ZIO_CHILD_VDEV_BIT |
+ ZIO_CHILD_GANG_BIT | ZIO_CHILD_DDT_BIT | ZIO_CHILD_LOGICAL_BIT,
+ ZIO_WAIT_DONE)) {
One small and probably last nit-pick, would we benefit from defining
`ZIO_CHILD_ALL_BITS` and using it here?
--
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/505#pullrequestreview-95624819
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M769c82a642435d083fd6bfda
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
avg-I approved this pull request. LGTM -- 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/505#pullrequestreview-95624832 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-Md31dbd08b553202ae00674f9 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
Maybe even `ZIO_GANG_BIT`, etc. -- 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/505#issuecomment-364269552 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M83a47cac272575e655a9a6fe Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
@grwilson this looks good to me! But... But! I've wondered only __just now__ if we could have best of both approaches by pre-defining the child type bits. E.g. ``` #define ZIO_CHILD_BIT_GANGZIO_CHILD_BIT(ZIO_CHILD_GANG) ``` ... or something like that. Maybe that could help to make the code where the bits are used to be just a little bit more compact. This is a purely stylistic suggestion, of course. -- 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/505#issuecomment-364269244 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M0d91c623e9549814c80465ae Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
[ping] -- 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/505#issuecomment-361855336 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M726c1a5115dac27c932bf470 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8966 Source file zfs_acl.c, function zfs_aclset_common contains a use after end of the lifetime of a local variable (#524)
Seems like the build error is because of a transient network issue. Could this PR please be moved further? -- 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/524#issuecomment-361855180 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Te13f69890aa6ddb1-M775b5aba77cc07072d51f3ea Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8969 Cannot boot from RAIDZ with parity > 1 (#526)
avg-I 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/526#pullrequestreview-89569476 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T6b96ee3544e7e44d-M708baec889dcc5b87da58d46 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8969 Cannot boot from RAIDZ with parity > 1 (#526)
This issue looked familiar... and here is why: https://svnweb.freebsd.org/base?view=revision&revision=243501 Sorry that I didn't recall it when I saw the root on raid-z support being introduced in illumos. -- 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/526#issuecomment-358422562 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T6b96ee3544e7e44d-M4107cf92f09e086d0d626dd7 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8966 Source file zfs_acl.c, function zfs_aclset_common contains a use after end of the lifetime of a local variable (#524)
avg-I 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/524#pullrequestreview-88695352 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Te13f69890aa6ddb1-M4d88fffc92f55ca5c20bb636 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8966 Source file zfs_acl.c, function zfs_aclset_common contains a use after end of the lifetime of a local variable (#524)
LGTM. -- 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/524#issuecomment-357544709 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Te13f69890aa6ddb1-M60362c741bea8375e1a2935e Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 ztest assertion failure in zil_lwb_write_issue (v3) (#514)
I like the idea in general. It's a rather big change, but it allows to express the intent more accurately. -- 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/514#issuecomment-356937339 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T9a006f4f91609eb5-M35c0dadd3ef914c63f3e9a64 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 ztest assertion failure in zil_lwb_write_issue (v3) (#514)
avg-I commented on this pull request. > ASSERT(!dsl_pool_sync_context(tx->tx_pool)); /* If we might wait, we must not hold the config lock. */ - ASSERT(txg_how != TXG_WAIT || !dsl_pool_config_held(tx->tx_pool)); + ASSERT((txg_how & TXG_WAIT) || !dsl_pool_config_held(tx->tx_pool)); I think that the first part of the condition is inverted here. It probably would make sense to use `IMPLY` here. -- 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/514#pullrequestreview-88167949 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T9a006f4f91609eb5-M320227e0ed5a7418cda40c95 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8372 sync and async zio-s share the same task queues (#438)
@prakashsurya I am all in doubts. On the one hand, I believe that the change is correct (or, at least, is a move in the correct direction). On the other hand, I have got some contradictory feedback. It would be great to get more performance testing. Might Delphix be interested in that? :-) -- 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/438#issuecomment-356934803 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T9cd08accc955a442-Mfbfd6929eba92cb620b67896 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 ztest assertion failure in zil_lwb_write_issue (v2) (#507)
avg-I commented on this pull request.
>*/
- error = dmu_tx_assign(tx, TXG_WAITED);
- if (error != 0) {
- ASSERT3S(error, ==, EIO);
- dmu_tx_abort(tx);
- return (NULL);
+ while ((error = dmu_tx_assign(tx, TXG_WAITED)) != 0) {
+ VERIFY3S(error, ==, ERESTART);
I understand that a `EIO` here would cause some problems.
But it is a very valid code that can be returned when `failmode` is `continue`
and the pool gets suspended.
Simply panicing with such configuration seems like a too drastic response.
Perhaps this code should treat that kind of `EIO` same as it treats `ERESTART`.
--
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/507#pullrequestreview-85134053
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T05cff66aac5a33a7-M52f083f99f002da0f3f13023
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 ztest assertion failure in zil_lwb_write_issue (v2) (#507)
avg-I commented on this pull request.
>*/
- error = dmu_tx_assign(tx, TXG_WAITED);
- if (error != 0) {
- ASSERT3S(error, ==, EIO);
- dmu_tx_abort(tx);
- return (NULL);
+ while ((error = dmu_tx_assign(tx, TXG_WAITED)) != 0) {
+ VERIFY3S(error, ==, ERESTART);
+ dmu_tx_unassign(tx);
And it seems that if `dmu_tx_assign` returns an error, then `dmu_tx_unassign`
has already been called by it.
--
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/507#discussion_r158336442
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T05cff66aac5a33a7-Md4e6d5daebf0745ce1238407
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 ztest assertion failure in zil_lwb_write_issue (v2) (#507)
avg-I commented on this pull request.
>*/
- error = dmu_tx_assign(tx, TXG_WAITED);
- if (error != 0) {
- ASSERT3S(error, ==, EIO);
- dmu_tx_abort(tx);
- return (NULL);
+ while ((error = dmu_tx_assign(tx, TXG_WAITED)) != 0) {
+ VERIFY3S(error, ==, ERESTART);
+ dmu_tx_unassign(tx);
It seems that `dmu_tx_unassign` is currently internal to dmu_tx.c and the build
complains about that.
--
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/507#pullrequestreview-85132426
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T05cff66aac5a33a7-Mf43a9f7d2ee0d0ea92d3adf4
Powered by Topicbox: https://topicbox.com
[developer] [openzfs/openzfs] 0000 a number of light-weight extensions to libzfs_core (#508)
This change adds a number of simple (simplistic, even!) interfaces to libzfs_core. It's quite possible that I have not thought through all required details or possible use-cases. But not having any functionality to, for example, rename or destroy a dataset is a big limitation of the current interface. The new functions are: - `lzc_rename` - `lzc_destroy` - `lzc_set_prop` (works only for a single property) - `lzc_get_props` (all properties, no filter / selector in the current implementation) - `lzc_inherit_prop` (only a plain inherit or revert to default, no way to revert to a received value) - `lzc_list_children` (actually makes a single iteration step over children) - `lzc_list_snaps` (same as above) All functions are very thin wrappers around the corresponding ioctls. Maybe, that makes the functions less convenient to use than they could be. But the change is really small and is really easy to understand. You can view, comment on, or merge this pull request online at: https://github.com/openzfs/openzfs/pull/508 -- Commit Summary -- * a number of light-weight extensions to libzfs_core -- File Changes -- M usr/src/lib/libzfs_core/common/libzfs_core.c (100) M usr/src/lib/libzfs_core/common/libzfs_core.h (10) M usr/src/lib/libzfs_core/common/mapfile-vers (9) -- Patch Links -- https://github.com/openzfs/openzfs/pull/508.patch https://github.com/openzfs/openzfs/pull/508.diff -- 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/508 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T94e916bc5cb9e1ed-M051acf5d57d0eaff88106095 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8930 zfs_zinactive: do not remove the node if the filesystem is readonly (#464)
I think that this PR is ready. -- 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/464#issuecomment-353100419 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T2d6e19e906add2a5-Mbc9dc0080dc4d9b70a4cffcb Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
Also, looks like the build error is actually a transient problem in the infrastructure... -- 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/505#issuecomment-353045436 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-Md8dc819615dd805016518df1 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
Oh! I guess my comment completely mirrors that of @youzhongyang Realized that a bit too late. -- 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/505#issuecomment-353045058 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-M2d2c8045cad44645f6486c24 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)
avg-I commented on this pull request.
> @@ -442,18 +444,26 @@ zio_remove_child(zio_t *pio, zio_t *cio, zio_link_t *zl)
}
static boolean_t
-zio_wait_for_children(zio_t *zio, enum zio_child child, enum zio_wait_type
wait)
+zio_wait_for_children(zio_t *zio, enum zio_child children,
Some compilers will complain about `children` parameter, because the values
passed through it are not of `enum zio_child` type. For example,
`ZIO_CHILD_VDEV | ZIO_CHILD_GANG` equals three which is not a valid value for
that enumeration.
> @@ -209,12 +209,13 @@ enum zio_flag {
ZIO_FLAG_CANFAIL)
enum zio_child {
- ZIO_CHILD_VDEV = 0,
- ZIO_CHILD_GANG,
- ZIO_CHILD_DDT,
- ZIO_CHILD_LOGICAL,
- ZIO_CHILD_TYPES
+ ZIO_CHILD_VDEV = 1 << 0,
+ ZIO_CHILD_GANG = 1 << 1,
+ ZIO_CHILD_DDT = 1 << 2,
+ ZIO_CHILD_LOGICAL = 1 << 3
I do not have any argument against this change, but have you considered keeping
this enumeration as is, but instead changing only `zio_wait_for_children` to
expect the bit-mask?
E.g. something like
```
#define ZIO_CHILD_BIT(c) (1 << (c))
```
and then
```
if (zio_wait_for_children(zio,
ZIO_CHILD_BIT(ZIO_CHILD_GANG) | ZIO_CHILD_BIT(ZIO_CHILD_LOGICAL),
ZIO_WAIT_READY)) {
...
}
```
Looks a bit too verbose, but has a benefit of affecting only
`zio_wait_for_children` calls which need to be modified in either case.
--
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/505#pullrequestreview-84737942
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/Td0adf1bfacf39ae0-Mb1599e3a1263637729e89df2
Powered by Topicbox: https://topicbox.com
[developer] Re: non-linear abd_alloc_for_io vs zio_checksum_compute
On 23/11/2017 18:05, Andriy Gapon wrote: > > I've done a little experimentation with a version of abd_alloc_for_io() that > returns a chunked abd. zio_checksum_compute() couldn't cope with that in the > ZCHECKSUM_FLAG_EMBEDDED case. > At present the function uses abd_to_buf() and that requires a linear abd. > The use of abd_to_buf() can be replaced with some calls to > abd_copy_to_buf_off() > and abd_copy_from_buf_off(). But the code would become a bit "heavier" and > uglier. > > Are there any other ways to get a small buffer view into a chunked abd? > Size of zio_eck_t is just 40 bytes. It's located either near the begnning of > a > block or at the end of it. I think that it is not likely to ever cross chunk > boundaries unless an insane chunk size is used. Another detail that's not directly related to the above is that even now we do not always send linear abd-s down to the leaf vdev layer. For example, raid-z creates chunked sub-ordinate abd-s. -- Andriy Gapon -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tdc55c3947a3b78d0-Mb00150324637b931da2383a1 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 ztest assertion failure in zil_lwb_write_issue (#492)
Seems like this PR has stalled a little bit. Are you planning to pick it up? -- 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/492#issuecomment-346800700 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T9576260544d0d727-Maff1fdda9eb8e5d38fcae13c Powered by Topicbox: https://topicbox.com
[developer] non-linear abd_alloc_for_io vs zio_checksum_compute
I've done a little experimentation with a version of abd_alloc_for_io() that returns a chunked abd. zio_checksum_compute() couldn't cope with that in the ZCHECKSUM_FLAG_EMBEDDED case. At present the function uses abd_to_buf() and that requires a linear abd. The use of abd_to_buf() can be replaced with some calls to abd_copy_to_buf_off() and abd_copy_from_buf_off(). But the code would become a bit "heavier" and uglier. Are there any other ways to get a small buffer view into a chunked abd? Size of zio_eck_t is just 40 bytes. It's located either near the begnning of a block or at the end of it. I think that it is not likely to ever cross chunk boundaries unless an insane chunk size is used. Thanks! -- Andriy Gapon -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tdc55c3947a3b78d0-M4da61a75715273c2e861b82e Powered by Topicbox: https://topicbox.com
[developer] assert: !claimed || !(zh->zh_flags & ZIL_CLAIM_LR_SEQ_VALID) || (max_blk_seq == claim_blk_seq && max_lr_seq == claim_lr_seq)
I have just had an accident where there was a random memory corruption (via DMA)
all over the place. Fortunately, the system crashed before too much damage has
been done. But there still was some damage to the ZFS pool. Initially the pool
even was in the faulted state, but zpool clear -F helped to restore it to a more
or less usable state.
I am trying to assess the state. zpool scrub has found some errors. Some files
were irreparable, but I was able to restore them. I realize that not all errors
can be found this way, so I am also verifying checksums for all files for which
I have them recorded (it's looking good so far).
There is one interesting trouble. A non-debug kernel is able to work with the
pool without problems. But a debug kernel hits this assert:
ASSERT(!claimed || !(zh->zh_flags & ZIL_CLAIM_LR_SEQ_VALID) ||
(max_blk_seq == claim_blk_seq && max_lr_seq == claim_lr_seq));
(kgdb) p *zilog->zl_header
$2 = {
zh_claim_txg = 11425925,
zh_replay_seq = 0,
zh_log = {
blk_dva = {{
dva_word = {0, 0}
}, {
dva_word = {0, 0}
}, {
dva_word = {0, 0}
}},
blk_prop = 0,
blk_pad = {0, 0},
blk_phys_birth = 0,
blk_birth = 0,
blk_fill = 0,
blk_cksum = {
zc_word = {0, 0, 0, 0}
}
},
zh_claim_blk_seq = 1,
zh_flags = 2,
zh_claim_lr_seq = 0,
zh_pad = {0, 0, 0}
}
So, the DVA is a hole, but zh_flags has ZIL_CLAIM_LR_SEQ_VALID and zh_claim_txg
and zh_claim_blk_seq are non-zero. The txg number is reasonable.
This does not look like a random corruption. Maybe the values are incorrect,
but they look sane.
This ASSERT is triggered for several different datasets.
What all of them have in common is that they were "dormant". They were not set
to readonly, but there should not have been any writes to them for a long time.
So, I am curious if there is anything interesting that can be deduced from this
information. Perhaps it could be a consequence of the rewind...
--
Andriy Gapon
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/Te134c02e590bc7f4-M70c719cf5262cce6c681124c
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 ztest assertion failure in zil_lwb_write_issue (#492)
@prakashsurya thank you. But I am still concerned about what impact the changes to `dmu_tx_assign` and `dmu_tx_try_assign` would have on the consumers that use the traditional scheme of `TXG_NOWAIT` followed by `TXG_WAITED`. It seems that now those consumers can get indefinitely blocked in `dmu_tx_assign(TXG_WAITED)` when a pool get suspended even if `failmode` is `continue`. Not sure if that would be correct. -- 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/492#issuecomment-341639648 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T9576260544d0d727-M4e7f5ca98bafd0bc5494d952 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 ztest assertion failure in zil_lwb_write_issue (#492)
avg-I commented on this pull request.
>*/
- error = dmu_tx_assign(tx, TXG_WAITED);
- if (error != 0) {
- ASSERT3S(error, ==, EIO);
- dmu_tx_abort(tx);
- return (NULL);
- }
+ VERIFY0(dmu_tx_assign(tx, TXG_WAITED));
Can't `dmu_tx_assign` -> `dmu_tx_try_assign` return `EIO` ?
I think that that still can happen if `spa_suspended` is true and
`spa_get_failmode(spa) == ZIO_FAILURE_MODE_CONTINUE`, because `TXG_WAITED` !=
`TXG_WAIT`.
Perhaps, we also need to change the condition from `txg_how != TXG_WAIT` to
`txg_how == TXG_NOWAIT` in the relevant clause in `dmu_tx_try_assign` like you
did in `dmu_tx_assign` ?
--
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/492#pullrequestreview-73957784
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T9576260544d0d727-M39293d969f5864bc3e527995
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8372 sync and async zio-s share the same task queues (#438)
I am getting some reports that this change actually hurts performance rather than improves it. Perhaps this is hardware dependent, perhaps not. So, please do not commit this just yet. I wonder if anyone else tested this change and can share any results. -- 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/438#issuecomment-341496739 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T2b9fdebe0c16de6b-Mf1f87ca2a70287497a7f4642 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8731 ASSERT3U(nui64s, <=, UINT16_MAX) fails for large blocks (#483)
Thank you! -- 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/483#issuecomment-340795284 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T1dada2f5266ff7f9-M6174f1140ce1bf1675c854cc Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8731 ASSERT3U(nui64s, <=, UINT16_MAX) fails for large blocks (#483)
I looked at the build report, but I couldn't figure out what the real error is. ``` running: aws ec2 request-spot-instances --type one-time --instance-count 1 --spot-price 0.100 --launch-specification file://request.json running: aws ec2 describe-spot-instance-requests --spot-instance-request-ids sir-47ti5r4k running: aws ec2 describe-spot-instance-requests --spot-instance-request-ids sir-47ti5r4k An error occurred (InvalidSpotInstanceRequestID.NotFound) when calling the DescribeSpotInstanceRequests operation: The spot instance request ID 'sir-47ti5r4k' does not exist failed: aws ec2 describe-spot-instance-requests --spot-instance-request-ids sir-47ti5r4k script returned exit code 1 ``` Is this some ec2 flakiness or was this error caused by another? -- 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/483#issuecomment-340711872 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T1dada2f5266ff7f9-M59c93796d1b49bc469384806 Powered by Topicbox: https://topicbox.com
[developer] [openzfs/openzfs] 8731 ASSERT3U(nui64s, <=, UINT16_MAX) fails for large blocks (#483)
https://www.illumos.org/issues/8731 You can view, comment on, or merge this pull request online at: https://github.com/openzfs/openzfs/pull/483 -- Commit Summary -- * 8731 ASSERT3U(nui64s, <=, UINT16_MAX) fails for large blocks -- File Changes -- M usr/src/uts/common/fs/zfs/zfs_fm.c (12) -- Patch Links -- https://github.com/openzfs/openzfs/pull/483.patch https://github.com/openzfs/openzfs/pull/483.diff -- 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/483 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T1dada2f5266ff7f9-M8c5e7b142d06fceb9fdc7b24 Powered by Topicbox: https://topicbox.com
Re: [developer] Re: [zfs] annotate_ecksum: nui64s <= UINT16_MAX
On 17/10/2017 23:21, Matthew Ahrens wrote: > I think this is because zei_histogram_set/cleared are arrays of uint16_t. > These > are counts of (64-bit) words that have the bit set/cleared when it shouldn't > be > (the index into the histogram is the bit number in the word). See > update_histogram() for details. > > So I think this assertion is valid since it ensures the histogram bucket does > not overflow (exceed UINT16_MAX). Thank you for the detailed explanation. > I see 2 options for fixing this: > > 1. Allow the bucket to overflow "gracefully", e.g. capping it at UINT16_MAX > rather than having it wrap around. This is a super safe option, of course, as it completely isolates the change. > 2. Change the zei_histogram_set/cleared to uint32_t's. I'm not sure the > impact > this would have to the fm_payload or consumers of it. I tried to analyze the potential impact. Both arrays currently have size of 128 bytes each (64 bit positions * 2 bytes). They are posted as nvlist elements of type DATA_TYPE_UINT16_ARRAY. So, changing the type to uint32_t would double array sizes to 256 bytes. That value does not seem like it's crossing any threshold. I think that the nvlist and fm code should be able to deal with the larger size without any issues. As to the consumers, I could not find any in-tree consumer that specifically looks for those two fields, bad_set_histogram and bad_cleared_histogram. Any code code that processes nvlists in a generic way should be able to deal with the type change. And any code, if any at all, that expects those elements to be DATA_TYPE_UINT16_ARRAY should simply fail to extract the arrays. So, at the very least, the change should not cause any silent misinterpretation of the data. > On Tue, Oct 17, 2017 at 6:11 AM, Andriy Gapon <mailto:a...@freebsd.org>> wrote: > > > Does anyone know a reason for this assertion in annotate_ecksum() ? > ASSERT3U(nui64s, <=, UINT16_MAX); > where nui64s = size / sizeof (uint64_t) with size being the size of the > corrupted block. I looked at the code but could not find any obvious > reason for > the limit. > > I think that with large block sizes (512KB and greater) that we support > now the > assertion can be violated. > > In fact, I have actually run into this on a raid-z pool where some > datasets have > recordsize=1m: > > panic: solaris assert: nui64s <= 0x (0x1 <= 0x), file: > /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_fm.c, line: > 555 > > (kgdb) bt > #0 doadump (textdump=1) at /usr/src/sys/kern/kern_shutdown.c:318 > #1 0x80660c75 in kern_reboot (howto=260) at > /usr/src/sys/kern/kern_shutdown.c:386 > #2 0x80661473 in vpanic (fmt=, > ap=0xfe051a4907a0) at > /usr/src/sys/kern/kern_shutdown.c:782 > #3 0x806614d3 in panic (fmt=) at > /usr/src/sys/kern/kern_shutdown.c:715 > #4 0x802e4bec in assfail3 (a=, lv=, > op=, rv=, f=, l=) at > /usr/src/sys/cddl/compat/opensolaris/kern/opensolaris_cmn_err.c:91 > #5 0x80396282 in annotate_ecksum (ereport=0xf800192692e0, > info=0xfe051a4908e0, > goodbuf=0xfe000b5ad000 ..., > badbuf=0xfe000d12d000 ..., size=524288, drop_if_identical=0) > at > /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_fm.c:555 > #6 0x80396733 in zfs_ereport_post_checksum (spa=, > vd=, zio=, offset=, > length=524288, > good_data=0xfe000b5ad000, bad_data=, zbc=) > at > /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_fm.c:795 > #7 0x80382f4b in raidz_checksum_error (zio=0xf80019265000, > rc=0xf8001b180660, bad_data=0xfe000d12d000) at > /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_raidz.c:2045 > #8 0x803826aa in raidz_parity_verify (zio=, > rm=0xf8001b180600) at > /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_raidz.c:2104 > #9 0x803812b1 in vdev_raidz_io_done (zio=0xf80019265000) at > /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_raidz.c:2402 > #10 0x803c3828 in zio_vdev_io_done (zio=0xf80019265000) at > /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c:3286 > #11 0x803bf7c8 in zio_execute (zio=0xf80019265000) at > /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c:1710 > #12 0x802e588e in taskq_run_ent (arg=, > pending=) at > /usr/src/sys/cddl/compat/opensolaris/kern/opensolaris_taskq.c:155 > #13 0x806ace21 in taskqueue_run_locked (q
Re: [developer] read-only pool, remove vdev
On 17/10/2017 19:05, George Wilson wrote: > Andriy, > > I'm not familiar with spa_async_thread_vd() (I don't believe that is > upstreamed > yet) so I can't comment on what is the correct thing to do there. Oh, I didn't realize that that part of the code was FreeBSD only. Alexander made the change in this commit: https://github.com/freebsd/freebsd/commit/07b58a38cf6664708546a9f4cd18fb8cc3a3bb1f So, this is a FreeBSD specific issue then. Thank you! > As for > spa_async_thread(), I think we can ignore all tasks when the pool is > read-only. I think that it would be nice to handle SPA_ASYNC_REMOVE even for read-only pools. But if that complicates things too much, then we can survive without that. > You'll probably need to update the call to vdev_probe() in zio_vdev_io_done() > so > that we only call it if spa_writeable(). vdev_probe_done() already checks spa_writeable() before issuing a write request. Not sure if that's enough. > On Tue, Oct 17, 2017 at 1:40 PM Andriy Gapon <mailto:a...@freebsd.org>> wrote: > > Both spa_async_thread() and spa_async_thread_vd() assert that spa_sync_on is > true. I think that the logic to ensure that is sound except for one case. > Specifically, if a pool is imported read-only, then spa_sync_on is never set > to > true (which is correct), but spa_async_suspended is not incremented either. > So, spa_async_request() ==> spa_async_dispatch_vd() does not bail out and > spa_async_thread_vd gets executed. > > I am actually not sure what's the best thing to do here for a read-only pool. > On the one hand, we should not touch its on-disk configuration, on the other > hand it seems that we should update the in-memory state of vdevs. > > Maybe the assertion simply can be dropped? > I am not sure if spa_async_thread_vd() really depends on spa_sync being > active. > What do you think? > > -- > Andriy Gapon > > *openzfs-developer* | Archives > <https://openzfs.topicbox.com/groups/developer/discussions/Tbc5170d4f71ec486-M4666ec33d56c533e901ac0d4> > | Powered by Topicbox <https://topicbox.com> -- Andriy Gapon -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tbc5170d4f71ec486-Mcdd2b81e7a5ea5d565a5a74e Powered by Topicbox: https://topicbox.com
[developer] read-only pool, remove vdev
Both spa_async_thread() and spa_async_thread_vd() assert that spa_sync_on is true. I think that the logic to ensure that is sound except for one case. Specifically, if a pool is imported read-only, then spa_sync_on is never set to true (which is correct), but spa_async_suspended is not incremented either. So, spa_async_request() ==> spa_async_dispatch_vd() does not bail out and spa_async_thread_vd gets executed. I am actually not sure what's the best thing to do here for a read-only pool. On the one hand, we should not touch its on-disk configuration, on the other hand it seems that we should update the in-memory state of vdevs. Maybe the assertion simply can be dropped? I am not sure if spa_async_thread_vd() really depends on spa_sync being active. What do you think? -- Andriy Gapon -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tbc5170d4f71ec486-M2da2a935fc77cf9438b27731 Powered by Topicbox: https://topicbox.com
[developer] annotate_ecksum: nui64s <= UINT16_MAX
Does anyone know a reason for this assertion in annotate_ecksum() ? ASSERT3U(nui64s, <=, UINT16_MAX); where nui64s = size / sizeof (uint64_t) with size being the size of the corrupted block. I looked at the code but could not find any obvious reason for the limit. I think that with large block sizes (512KB and greater) that we support now the assertion can be violated. In fact, I have actually run into this on a raid-z pool where some datasets have recordsize=1m: panic: solaris assert: nui64s <= 0x (0x1 <= 0x), file: /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_fm.c, line: 555 (kgdb) bt #0 doadump (textdump=1) at /usr/src/sys/kern/kern_shutdown.c:318 #1 0x80660c75 in kern_reboot (howto=260) at /usr/src/sys/kern/kern_shutdown.c:386 #2 0x80661473 in vpanic (fmt=, ap=0xfe051a4907a0) at /usr/src/sys/kern/kern_shutdown.c:782 #3 0x806614d3 in panic (fmt=) at /usr/src/sys/kern/kern_shutdown.c:715 #4 0x802e4bec in assfail3 (a=, lv=, op=, rv=, f=, l=) at /usr/src/sys/cddl/compat/opensolaris/kern/opensolaris_cmn_err.c:91 #5 0x80396282 in annotate_ecksum (ereport=0xf800192692e0, info=0xfe051a4908e0, goodbuf=0xfe000b5ad000 ..., badbuf=0xfe000d12d000 ..., size=524288, drop_if_identical=0) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_fm.c:555 #6 0x80396733 in zfs_ereport_post_checksum (spa=, vd=, zio=, offset=, length=524288, good_data=0xfe000b5ad000, bad_data=, zbc=) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_fm.c:795 #7 0x80382f4b in raidz_checksum_error (zio=0xf80019265000, rc=0xf8001b180660, bad_data=0xfe000d12d000) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_raidz.c:2045 #8 0x803826aa in raidz_parity_verify (zio=, rm=0xf8001b180600) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_raidz.c:2104 #9 0x803812b1 in vdev_raidz_io_done (zio=0xf80019265000) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_raidz.c:2402 #10 0x803c3828 in zio_vdev_io_done (zio=0xf80019265000) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c:3286 #11 0x803bf7c8 in zio_execute (zio=0xf80019265000) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c:1710 #12 0x802e588e in taskq_run_ent (arg=, pending=) at /usr/src/sys/cddl/compat/opensolaris/kern/opensolaris_taskq.c:155 #13 0x806ace21 in taskqueue_run_locked (queue=0xf80019e75e00) at /usr/src/sys/kern/subr_taskqueue.c:478 #14 0x806ada48 in taskqueue_thread_loop (arg=) at /usr/src/sys/kern/subr_taskqueue.c:787 -- Andriy Gapon -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T33b163a0291b77e1-Ma242ba4122727cf3ba907857 Powered by Topicbox: https://topicbox.com
[developer] spa_namespace_lock vs spa_config locks
It seems that are a couple of code paths where spa_config_enter is called while spa_namespace_lock is held [*]. I am curious if this is considered normal or it it's something that should be and, more importantly, can be avoided. Perhaps, it's possible to find a spa, reference it, drop the namespace lock and then enter configuration locks. The reason for my inquiry is that from time to time we see cases where ZFS gets into a very bad shape because of a problem with a disk, especially if it's a network disk. The scenario unfolds like this: - there is an outstanding zio that does not complete for a long time, that means that SCL_ZIO is held in the read mode - a vdev event (SPA_ASYNC_REMOVE) occurs and spa_async_thread_vd gets kicked, it acquires a number of config locks in the write mode before getting blocked on SCL_ZIO - this is already quite bad by itself as SCL_STATE is held by spa_async_thread_vd and, thus, spa_sync gets blocked - if one of the paths involving spa_namespace_lock and spa_config_enter gets executed, then spa_namespace_lock ends up locked - that means that practically all ioctl-s will get blocked, because they need to look up a spa first In the end, it is not possible to examine the state of ZFS or to attempt any corrective action. Also, the problem affects all pools, not only the one with the disk problems. I understand that there is not much, if anything at all, that ZFS can do about SCL_ZIO (it's really an issue with an underlying storage stack), but I think that ZFS should try to avoid having a dependency between SCL_ZIO in one pool and global spa_namespace_lock. I will appreciate any thoughts and suggestion on this problem. Thanks! P.S. I also recall a discussion about calling dsl_sync_task() while holding spa_namespace_lock. That can lead to a similar issue. [*] 1. spa_config_enter spa_config_generate spa_open_common spa_get_stats zfs_ioc_pool_stats zfsdev_ioctl 2. spa_config_enter l2arc_dev_get_next l2arc_feed_thread -- Andriy Gapon -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T33dc6c344651ecdc-Ma22fb5d54d38e7ceeda5fb29 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8603 rename zilog's "zl_writer_lock" to "zl_issuer_lock" (#456)
> I doubt I'm going to completely change your mind on this issue, but would you > be able to live with this change? Of, course. I do not have any strong preferences about that. -- 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/456#issuecomment-331498677 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T54c57375fea79b4d-Mdb1529664288dd2af491fb94 Powered by Topicbox: https://topicbox.com
[developer] Re: [zfs] ZIO_IOCTL_PIPELINE excludes ZIO_STAGE_VDEV_IO_DONE
George, we use a different vdev "driver" in FreeBSD, vdev_geom, but thank you for the idea. It seems that I can adapt it to vdev_geom. On 16/09/2017 17:01, George Wilson wrote: > Andriy, > > Assuming this is disk specific, you could look at doing this in the IOCTL > callback -- "vdev_disk_ioctl_done". > > Thanks, > George > > On Fri, Sep 15, 2017 at 11:31 AM Andriy Gapon <mailto:a...@freebsd.org>> wrote: > > > In FreeBSD we create a struct bio object for each I/O request that goes > to a > disk. So, we create bio-s for all vdev zio-s that are to be passed down > including read, write, ioctl (disk cache flush) zio-s. Previously, we > freed > those bio-s in a callback from the I/O subsystem. But since the ABD > change we > had to move that to the io_done stage, because the callback's context is > too > restrictive for abd_return_buf / abd_return_buf_copy (at least for the > current > ABD implementation which is taken as is from illumos). So, currently we > are > leaking bio-s created for ioctl zio-s because the io_done stage is not > called > for them. > > There is probably another way to fix this problem as the ioctl zio-s have > nothing to do with ABD. So, we could free their bio-s in the callback as > we did > before and handle the regular i/o bio-s in the io_done stage as we do > now. This > should work, but at the cost of the less uniform bio handling code. > > So, I wanted to see which of the solutions would be better from the point > of > view of the general zio pipeline architecture. > > Thank you. > > On 14/09/2017 18:48, George Wilson wrote: > > Andriy, > > > > The ZIO_STAGE_VDEV_IO_DONE is not necessary for ZIO_IOCTL_PIPELINE and > that's > > why it was removed. At least in illumos, there is no functional reason > for > > calling it. To add it back would require a small amount of change which > should > > not be a big deal. Can you provide some context around the FreeBSD bug? > > > > Thanks, > > George > > > > > > On Thu, Sep 14, 2017 at 12:45 AM Andriy Gapon <mailto:a...@freebsd.org> > > <mailto:a...@freebsd.org <mailto:a...@freebsd.org>>> wrote: > > > > > > Does anyone know why ZIO_IOCTL_PIPELINE does not include > ZIO_STAGE_VDEV_IO_DONE? > > Or, in other words, why ZIO_IOCTL_PIPELINE actively excludes it by > not > using > > ZIO_VDEV_IO_STAGES? > > > > It seems that ZIO_IOCTL_PIPELINE had ZIO_VDEV_IO_STAGES until this > commit: > > > commit e14bb3258d05c1b1077e2db7cf77088924e56919 > > > Author: Jeff Bonwick > > > Date: Mon Sep 29 18:13:58 2008 -0700 > > > 6754011 SPA 3.0: lock breakup, i/o pipeline refactoring, device > failure > > handling > > > 6667208 zfs/zpool commands on failed pool should not hang > > > 6430480 grabbing config lock as writer during I/O load can take > > excessively long > > > > Of course, the commit message does not have any detailed > explanations > and the > > referenced bugs are not publicly accessible. And it was almost 9 > years ago. > > > > I wonder if the change was because of anything specific to illumos > vdev_disk. > > I think that it would be totally okay to enable > ZIO_STAGE_VDEV_IO_DONE > with > > FreeBSD vdev_geom. In fact, right now ZFS/FreeBSD has a bug because > the done > > stage is not executed. > > > > -- > > Andriy Gapon > > > > ---------- > > illumos-zfs > > Archives: > > > > https://illumos.topicbox.com/groups/zfs/discussions/T7be335e1da154b96-Mea10ec00d17c40849ece338f > > Powered by Topicbox: https://topicbox.com > > > > *illumos-zfs* | Archives > > > > <https://illumos.topicbox.com/groups/zfs/discussions/T7be335e1da154b96-M0cc0a74989d1c00edb4b391a> > > | Powered by Topicbox <https://topicbox.com> > > > -- > Andriy Gapon > -- Andriy Gapon -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T4b8a967c63e22769-M6e632b28aa871ba0b85be0a6 Powered by Topicbox: https://topicbox.com
[developer] Re: [zfs] ZIO_IOCTL_PIPELINE excludes ZIO_STAGE_VDEV_IO_DONE
In FreeBSD we create a struct bio object for each I/O request that goes to a disk. So, we create bio-s for all vdev zio-s that are to be passed down including read, write, ioctl (disk cache flush) zio-s. Previously, we freed those bio-s in a callback from the I/O subsystem. But since the ABD change we had to move that to the io_done stage, because the callback's context is too restrictive for abd_return_buf / abd_return_buf_copy (at least for the current ABD implementation which is taken as is from illumos). So, currently we are leaking bio-s created for ioctl zio-s because the io_done stage is not called for them. There is probably another way to fix this problem as the ioctl zio-s have nothing to do with ABD. So, we could free their bio-s in the callback as we did before and handle the regular i/o bio-s in the io_done stage as we do now. This should work, but at the cost of the less uniform bio handling code. So, I wanted to see which of the solutions would be better from the point of view of the general zio pipeline architecture. Thank you. On 14/09/2017 18:48, George Wilson wrote: > Andriy, > > The ZIO_STAGE_VDEV_IO_DONE is not necessary for ZIO_IOCTL_PIPELINE and that's > why it was removed. At least in illumos, there is no functional reason for > calling it. To add it back would require a small amount of change which should > not be a big deal. Can you provide some context around the FreeBSD bug? > > Thanks, > George > > > On Thu, Sep 14, 2017 at 12:45 AM Andriy Gapon <mailto:a...@freebsd.org>> wrote: > > > Does anyone know why ZIO_IOCTL_PIPELINE does not include > ZIO_STAGE_VDEV_IO_DONE? > Or, in other words, why ZIO_IOCTL_PIPELINE actively excludes it by not > using > ZIO_VDEV_IO_STAGES? > > It seems that ZIO_IOCTL_PIPELINE had ZIO_VDEV_IO_STAGES until this commit: > > commit e14bb3258d05c1b1077e2db7cf77088924e56919 > > Author: Jeff Bonwick > > Date: Mon Sep 29 18:13:58 2008 -0700 > > 6754011 SPA 3.0: lock breakup, i/o pipeline refactoring, device > failure > handling > > 6667208 zfs/zpool commands on failed pool should not hang > > 6430480 grabbing config lock as writer during I/O load can take > excessively long > > Of course, the commit message does not have any detailed explanations and the > referenced bugs are not publicly accessible. And it was almost 9 years ago. > > I wonder if the change was because of anything specific to illumos vdev_disk. > I think that it would be totally okay to enable ZIO_STAGE_VDEV_IO_DONE with > FreeBSD vdev_geom. In fact, right now ZFS/FreeBSD has a bug because the done > stage is not executed. > > -- > Andriy Gapon > > *illumos-zfs* | Archives > <https://illumos.topicbox.com/groups/zfs/discussions/T7be335e1da154b96-M0cc0a74989d1c00edb4b391a> > | Powered by Topicbox <https://topicbox.com> -- Andriy Gapon -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T4b8a967c63e22769-Meab287e30d2674efda496750 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 zfs_zinactive: do not remove the node if the filesystem is readonly (#464)
@ahrens I added additional handling of the unlinked nodes to `re_zget` and updated the comment in `zfs_zinactive`. Could you please take a look? -- 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/464#issuecomment-329812184 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tf5d8079d50248fc8-Mc3d17a67f6de183bcec89516 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 zfs_zinactive: do not remove the node if the filesystem is readonly (#464)
@avg-I pushed 1 commit. d342cf1 update handling of received unlinked files in zfs_rezget -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/464/files/e4779456db23ce331378d4442ecb2185e184214f..d342cf16062d0fff969447a3c31881ca7bfc1cfc -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tf5d8079d50248fc8-M50929254525149e113d77b99 Powered by Topicbox: https://topicbox.com
[developer] ZIO_IOCTL_PIPELINE excludes ZIO_STAGE_VDEV_IO_DONE
Does anyone know why ZIO_IOCTL_PIPELINE does not include ZIO_STAGE_VDEV_IO_DONE? Or, in other words, why ZIO_IOCTL_PIPELINE actively excludes it by not using ZIO_VDEV_IO_STAGES? It seems that ZIO_IOCTL_PIPELINE had ZIO_VDEV_IO_STAGES until this commit: > commit e14bb3258d05c1b1077e2db7cf77088924e56919 > Author: Jeff Bonwick > Date: Mon Sep 29 18:13:58 2008 -0700 >6754011 SPA 3.0: lock breakup, i/o pipeline refactoring, device failure > handling >6667208 zfs/zpool commands on failed pool should not hang >6430480 grabbing config lock as writer during I/O load can take > excessively long Of course, the commit message does not have any detailed explanations and the referenced bugs are not publicly accessible. And it was almost 9 years ago. I wonder if the change was because of anything specific to illumos vdev_disk. I think that it would be totally okay to enable ZIO_STAGE_VDEV_IO_DONE with FreeBSD vdev_geom. In fact, right now ZFS/FreeBSD has a bug because the done stage is not executed. -- Andriy Gapon -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T4b8a967c63e22769-Mda794f9d688b2c7be34b8980 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8648 Fix range locking in ZIL commit codepath (#462)
@ikozhukhov Could you please try to reproduce the problem with the patch applied using a fresh pool? I would like to eliminate a chance that the crash you see is caused by something in the pool from previous experiments. -- 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/462#issuecomment-329241577 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T23be73c52829b560-M52ac2b27f153f5b76cd89989 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 zfs_zinactive: do not remove the node if the filesystem is readonly (#464)
@ahrens The `re_zget` change was an alternative solution that I was considering. I am perfectly okay with it. But you might recall that I also asked about another [admittedly, exotic] scenario and what we should do in it: - open a file - unlink it - set readonly=on - close the file In this scenario `re_zget` is not involved at all, but a very similar `zfs_zinactive` context is created. So, if we go with the `re_zget` solution, then any assertions in `zfs_zinactive` should be pretty flexible. -- 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/464#issuecomment-328894621 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tf5d8079d50248fc8-M7d9e9de5ac6bce7f9eb15a15 Powered by Topicbox: https://topicbox.com
[developer] [openzfs/openzfs] 0000 zfs_zinactive: do not remove the node if the filesystem is readonly (#464)
We normally remove an unlinked node when its last user goes away and the node becomes inactive. However, we should not do that if the filesystem is mounted read-only including the case where it has its readonly property set. The node will remain on the unlinked queue, so it will not be leaked. One particular scenario is when we receive an incremental stream into a mounted read-only filesystem and that stream contains an unlinked file (still on the unlinked queue). If that file is opened before the receive and some time later after the receive it becomes inactive we would remove it and, thus, modify the read-only filesystem. As a result, the filesystem would diverge from its source and further incremental receives would not be possible (without forcing a rollback). You can view, comment on, or merge this pull request online at: https://github.com/openzfs/openzfs/pull/464 -- Commit Summary -- * zfs_zinactive: do not remove the node if the filesystem is read-only -- File Changes -- M usr/src/uts/common/fs/zfs/zfs_znode.c (14) -- Patch Links -- https://github.com/openzfs/openzfs/pull/464.patch https://github.com/openzfs/openzfs/pull/464.diff -- 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/464 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tf5d8079d50248fc8-M642bde7c5b4d84da6ba43de0 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8648 Fix range locking in ZIL commit codepath (#462)
CC @amotin -- 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/462#issuecomment-328561141 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T23be73c52829b560-Ma3549375575729843de2ad53 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8520 lzc_rollback_to should support rolling back to origin (#434)
avg-I commented on this pull request.
> + * See if the snapshot is a snapshot of the filesystem
+* or the snapshot is an origin of the filesystem.
+*/
+ if (snapds->ds_dir == ds->ds_dir ||
+ (dsl_dir_is_clone(ds->ds_dir) &&
+ dsl_dir_phys(ds->ds_dir)->dd_origin_obj ==
+ snapds->ds_object)) {
+ error = SET_ERROR(EEXIST);
+ } else {
+ error = SET_ERROR(ESRCH);
+ }
+ dsl_dataset_rele(snapds, FTAG);
+ dsl_dataset_rele(ds, FTAG);
+ return (error);
+ }
+ dsl_dataset_rele(snapds, FTAG);
}
/* must not have any bookmarks after the most recent snapshot */
Sure!
--
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/434#discussion_r13807
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T5032507ba3c52b88-Mea06d906d664ea5e8b6c1d42
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8520 lzc_rollback_to should support rolling back to origin (#434)
avg-I commented on this pull request.
> + /* Check if the target snapshot exists at all. */
+ error = dsl_dataset_hold(dp, ddra->ddra_tosnap, FTAG, &snapds);
+ if (error != 0) {
+ /* Distinguish between missing dataset and snapshot. */
+ if (error == ENOENT || error == EXDEV)
+ error = SET_ERROR(ESRCH);
+ dsl_dataset_rele(ds, FTAG);
+ return (error);
+ }
+ ASSERT(snapds->ds_is_snapshot);
+
+ /* Check if the snapshot is the latest snapshot indeed. */
+ if (snapds != ds->ds_prev) {
+ /*
+* See if the snapshot is a snapshot of the filesystem
+* or the snapshot is an origin of the filesystem.
Thank you for the suggestion!
--
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/434#discussion_r138098948
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T5032507ba3c52b88-M648a4549862d3c656537048a
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8520 lzc_rollback_to should support rolling back to origin (#434)
avg-I commented on this pull request.
- dsl_dataset_name(ds->ds_prev, namebuf);
- if (strcmp(namebuf, ddra->ddra_tosnap) != 0)
- return (SET_ERROR(EXDEV));
+ /* Check if the target snapshot exists at all. */
+ error = dsl_dataset_hold(dp, ddra->ddra_tosnap, FTAG, &snapds);
+ if (error != 0) {
+ /* Distinguish between missing dataset and snapshot. */
By "dataset" here I meant the parent dataset of the snapshot. If it does not
exists, then we pass up ENOENT, but if the snapshot does not exist, then we
convert ENOENT to ESRCH.
I agree that the comment is confusing. I'll try to write a more verbose one.
--
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/434#discussion_r138097401
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/T5032507ba3c52b88-Mcab916ee46d48183a1b9c654
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 8372 sync and async zio-s share the same task queues (#438)
avg-I commented on this pull request. > @@ -1417,6 +1417,13 @@ zio_taskq_dispatch(zio_t *zio, zio_taskq_type_t q, > boolean_t cutinline) spa_t *spa = zio->io_spa; zio_type_t t = zio->io_type; int flags = (cutinline ? TQ_FRONT : 0); + zio_priority_t p = zio->io_priority; + + ASSERT(q == ZIO_TASKQ_ISSUE || q == ZIO_TASKQ_INTERRUPT); + if (p == ZIO_PRIORITY_SYNC_WRITE) + ASSERT3U(t, ==, ZIO_TYPE_WRITE); Than you for the suggestion. I decided to modify the code to use the more compact form. -- 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/438#discussion_r138091522 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T2b9fdebe0c16de6b-M3530521e3a57d7b898405a42 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 0000 Fix range locking in ZIL commit codepath (#462)
I have a report from a FreeBSD user that 7578 caused a problem for him and that the change in this PR fixes it. His panic was different from what was reported here. He got "zfs: allocating allocated segment" in this stack: ``` (kgdb) backtrace #0 doadump (textdump=1) at pcpu.h:222 #1 0x807c80c6 in kern_reboot (howto=260) at /usr/src/sys/kern/kern_shutdown.c:366 #2 0x807c85a0 in vpanic (fmt=, ap=) at /usr/src/sys/kern/kern_shutdown.c:759 #3 0x807c83d3 in panic (fmt=) at /usr/src/sys/kern/kern_shutdown.c:690 #4 0x81d15192 in vcmn_err (ce=, fmt=0x81b3e673 "zfs: allocating allocated segment(offset=%llu size=%llu)\n", adx=0xfe201b3df810) at /usr/src/sys/cddl/compat/opensolaris/kern/opensolaris_cmn_err.c:58 #5 0x81a843ba in zfs_panic_recover (fmt=) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_misc.c:1595 #6 0x81a6cf8f in range_tree_add (arg=0xf801cd085400, start=51596307456, size=2048) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/range_tree.c:179 #7 0x81a6bb06 in metaslab_free_dva (spa=, dva=, txg=, now=0) at atomic.h:123 #8 0x81a6bbc1 in metaslab_free (spa=0xf8031275d000, bp=0xf80200f08820, txg=8777, now=0) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/metaslab.c:2851 #9 0x81aaf96a in zio_dva_free (zio=) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c:2897 #10 0x81aac6cc in zio_execute (zio=0xf80200f087b0) at /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c:1681 #11 0x8081c377 in taskqueue_run_locked (queue=0xf8024e085700) at /usr/src/sys/kern/subr_taskqueue.c:454 #12 0x8081d518 in taskqueue_thread_loop (arg=) at /usr/src/sys/kern/subr_taskqueue.c:741 ``` His test case was: 1. zfs create -s -V 50T -o logbias=throughput -o volmode=dev -o volblocksize=128k -o compression=lz4 -o dedup=off -o secondarycache=all -o sync=standard zpool/windows 2. exported the LUN via cam layer as an FC target 3. Formated from windows 2012 r2 with 64K blok size NTFS 4. run iometer read test... -- 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/462#issuecomment-328512758 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Tffbfe5cf4d0c0904-M4fce7f961048f0fe11f33108 Powered by Topicbox: https://topicbox.com
