[developer] used > 0 || dsl_dir_phys(dd)->dd_used_breakdown[type] >= -used

[Andriy Gapon]

Another problem that seems potentially related to
http://thread.gmane.org/gmane.comp.file-systems.openzfs.devel/2911/focus=2917
but could be somehting different.

So far reproduced only on FreeBSD.

panic: solaris assert: used > 0 ||
dsl_dir_phys(dd)->dd_used_breakdown[type] >= -used, file:
/usr/devel/svn/head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dir.c,
line: 1389
cpuid = 0
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame
0xfe004db89410
vpanic() at vpanic+0x182/frame 0xfe004db89490
panic() at panic+0x43/frame 0xfe004db894f0
assfail() at assfail+0x1a/frame 0xfe004db89500
dsl_dir_diduse_space() at dsl_dir_diduse_space+0x200/frame
0xfe004db89580
dsl_dataset_clone_swap_sync_impl() at
dsl_dataset_clone_swap_sync_impl+0x3f5/frame 0xfe004db89690
dsl_dataset_rollback_sync() at dsl_dataset_rollback_sync+0x11d/frame
0xfe004db897f0
dsl_sync_task_sync() at dsl_sync_task_sync+0xef/frame 0xfe004db89820
dsl_pool_sync() at dsl_pool_sync+0x45b/frame 0xfe004db89890
spa_sync() at spa_sync+0x7c7/frame 0xfe004db89ad0
txg_sync_thread() at txg_sync_thread+0x383/frame 0xfe004db89bb0

After the panic the pool is left in a state where running the same
rollback command (zfs rollback zroot2/test/4@1) results in the same panic.

Some data from the affected datasets:

zroot2/test/4  used  1076224-
zroot2/test/4  referenced51200  -
zroot2/test/4  usedbysnapshots   1024   -
zroot2/test/4  usedbydataset 40960  -
zroot2/test/4  usedbychildren1034240-
zroot2/test/4  originzroot2/test/1@4-

zroot2/test/4@1  used  1024   -
zroot2/test/4@1  referenced50176  -
zroot2/test/4@1  cloneszroot2/test/3/2/2/4-

zroot2/test/1@4  used  24576  -
zroot2/test/1@4  referenced50176  -
zroot2/test/1@4  cloneszroot2/test/4  -

zroot2/test/1  used  318464 -
zroot2/test/1  referenced50176  -
zroot2/test/1  usedbysnapshots   25600  -
zroot2/test/1  usedbydataset 50176  -
zroot2/test/1  usedbychildren242688 -
zroot2/test/1  origin-   -

Using a debugger I determined that zroot2/test/4 has a deadlist of size
40960.  So, in dsl_dataset_clone_swap_sync_impl() dused is calculated as:
dused = 50176 + 0 - (51200 + 40960) = -41984

This is a value that gets passed to
dsl_dir_diduse_space(origin_head->ds_dir, DD_USED_HEAD,
dused, dcomp, duncomp, tx);

And dd_used_breakdown[DD_USED_HEAD] is 40960 there, so the assertion
prevents it from going to the negative territory.

I am not sure how the datasets came to this state.
I can provide any additional data that can be queried from the pool and
from the crash dump.

-- 
Andriy Gapon


---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] spa_namespace_lock vs sync thread

[Andriy Gapon]

George,

thank you very much for the reply.
I've got curious about this while looking into what appears to be a
FreeBSD-specific deadlock:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=203864#c13


On 08/07/2016 17:03, George Wilson wrote:
> Andriy,
> 
> I think this is basically a rule, although I don't think it's stated
> anywhere. We do rely heavily on this locking strategy since there are
> many places that will hold the namespace lock to prevent spa config
> changes but yet wait for a txg to sync.
> 
> Is it honored everywhere? Well, I hope so but there probably hasn't been
> a detailed analysis done to see if there might be places where we do
> something like a spa_open() from some obscure path in the sync thread. I
> would hope that our testing would have discovered that quickly and
> hopefully developers will sufficiently exercise their code to make sure
> they don't violate this.
> 
> Have you seen instances where we deadlock as a result of this?
> 
> Thanks,
> George
> 
> On Fri, Jul 8, 2016 at 9:40 AM, Andriy Gapon  <mailto:a...@freebsd.org>> wrote:
> 
> 
> I see a few places in the code that do the following:
> mutex_enter(&spa_namespace_lock);
> dsl_sync_task(...);
> mutex_exit(&spa_namespace_lock);
> One example is spa_change_guid().
> 
> In my understanding this implies that the sync thread must never acquire
> spa_namespace_lock.  Is this a real rule?  Do we always honor it?
> 
> Thank you!
> --
> Andriy Gapon
> 
> 
> http://www.listbox.com
> 
> 
> *openzfs-developer* | Archives
> <https://www.listbox.com/member/archive/274414/=now>
> <https://www.listbox.com/member/archive/rss/274414/28133750-22ed9730> |
> Modify
> <https://www.listbox.com/member/?&;>
> Your Subscription [Powered by Listbox] <http://www.listbox.com>
> 


-- 
Andriy Gapon


---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

[developer] spa_namespace_lock vs sync thread

[Andriy Gapon]

I see a few places in the code that do the following:
mutex_enter(&spa_namespace_lock);
dsl_sync_task(...);
mutex_exit(&spa_namespace_lock);
One example is spa_change_guid().

In my understanding this implies that the sync thread must never acquire
spa_namespace_lock.  Is this a real rule?  Do we always honor it?

Thank you!
-- 
Andriy Gapon


---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] panic: solaris assert: ba.ba_phys->bt_bytes == 0 (0x400 == 0x0)

[Andriy Gapon]

Igor,

your suggestion was certainly a good one, however I took a path of a
lesser effort and tested my workload on the latest illumos kernel:

panic[cpu3]/thread=ff000bc56c40: assertion failed:
ba.ba_phys->bt_bytes == 0 (0x400 == 0x0), file:
../../common/fs/zfs/bptree.c, line: 293

ff000bc56890 genunix:process_type+164b75 ()
ff000bc56a20 zfs:bptree_iterate+4bf ()
ff000bc56a90 zfs:dsl_scan_sync+17c ()
ff000bc56b50 zfs:spa_sync+2bb ()
ff000bc56c20 zfs:txg_sync_thread+260 ()
ff000bc56c30 unix:thread_start+8 ()

syncing file systems... done
dumping to /dev/zvol/dsk/rpool/dump, offset 65536, content: kernel
dumping:  0:34 100% done
100% done: 339495 pages dumped, dump succeeded
rebooting...

So, if anyone is interested I can provide any requested information from
the crash dump or try your debugging suggestions.

On 22/06/2016 17:45, Igor Kozhukhov wrote:
> based on your changeset number - it is old update:
> https://github.com/illumos/illumos-gate/commit/26455f9efcf9b1e44937d4d86d1ce37b006f25a9
> 6052 decouple lzc_create() from the implementation details
> 
> we have a lot of others changes in illumos tree and i can say - i have
> no panic on my system with gcc48 build - i have tested by zfs tests.
> 
> Maybe, as solution, you can try to merge to latest changes and try to
> check it again?
> i had panic with gcc48 build, but Matt pointed to some delphix update
> and we have upstreamed it and i have no panics any more with full list
> of zfs tests, what availabe on illumos tree.
> 
> best regards,
> -Igor
> 
> 
>> On Jun 22, 2016, at 5:17 PM, Andriy Gapon > <mailto:a...@freebsd.org>> wrote:
>>
>>
>> I am not yet convinced that the problem has anything to do with
>> miscompiled code.  I am using exactly the same optimizations and exactly
>> the same compiler as the official FreeBSD builds.
>>
>> On 22/06/2016 17:03, Igor Kozhukhov wrote:
>>> Hi Andri,
>>>
>>> i have DilOS with gcc-4.8,5 (+ special patches) for illumos builds.
>>> i had some problems with zdb - found it by zfs tests.
>>>
>>> problem has been fixed by disable of optimization :
>>> -fno-aggressive-loop-optimizations
>>>
>>> also, i have added:
>>> -fno-ipa-sra
>>>
>>> but i no remember a story why i have added it ;)
>>> probabbly it was added with another illumos component and new gcc-4.8
>>>
>>> As you know, illumos still is using gcc-4.4.4 and some newer compilers
>>> can produce new issues with older code :)
>>>
>>> I think, you can try to play with your clang optimization flags too.
>>> i have no experience with clang.
>>>
>>> best regards,
>>> -Igor
>>>
>>>
>>>> On Jun 22, 2016, at 4:21 PM, Andriy Gapon >>> <mailto:a...@freebsd.org>
>>>> <mailto:a...@freebsd.org>> wrote:
>>>>
>>>>
>>>> I am getting the following panic using the latest FreeBSD head that is
>>>> synchronized with OpenZFS code as of
>>>> illumos/illumos-gate@26455f9efcf9b1e44937d4d86d1ce37b006f25a9.
>>>>
>>>> panic: solaris assert: ba.ba_phys->bt_bytes == 0 (0x400 == 0x0), file:
>>>> /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/bptree.c,
>>>> line: 292
>>>> cpuid = 1
>>>> KDB: stack backtrace:
>>>> db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame
>>>> 0xfe004db9d310
>>>> vpanic() at vpanic+0x182/frame 0xfe004db9d390
>>>> panic() at panic+0x43/frame 0xfe004db9d3f0
>>>> assfail3() at assfail3+0x2c/frame 0xfe004db9d410
>>>> bptree_iterate() at bptree_iterate+0x35e/frame 0xfe004db9d540
>>>> dsl_scan_sync() at dsl_scan_sync+0x24f/frame 0xfe004db9d890
>>>> spa_sync() at spa_sync+0x897/frame 0xfe004db9dad0
>>>> txg_sync_thread() at txg_sync_thread+0x383/frame 0xfe004db9dbb0
>>>> fork_exit() at fork_exit+0x84/frame 0xfe004db9dbf0
>>>> fork_trampoline() at fork_trampoline+0xe/frame 0xfe004db9dbf0
>>>> --- trap 0, rip = 0, rsp = 0, rbp = 0 ---
>>>>
>>>> I have a crash dump, but unfortunately it's hard to work with it,
>>>> because a lot of useful information got "optimized out" by clang.
>>>>
>>>> I can reproduce the panic using a synthetic workload, but I do not have
>>>> a concise reproduction scenario.  Every time the panic happens bt_bytes
>>>> is 0x400, I haven't seen any other number there.
>>>>
>>>> Does anyone have an i

[developer] metaslab_free_dva: "allocating allocated segment" panic

[Andriy Gapon]

From time to time I am getting another panic in the same environment as
described in the thread titled "panic: solaris assert:
ba.ba_phys->bt_bytes == 0 (0x400 == 0x0)".

Judging from blk_prop value of 0x8009090200070007 the affected block
belongs to ZIL (type 9).

Some info:
Solaris(panic): zfs: allocating allocated segment(offset=3011224576
size=4096)

(kgdb) bt
...
#10 0x80a3d0c3 in panic (fmt=0x81b274c0 "\004") at
/usr/src/sys/kern/kern_shutdown.c:690
#11 0x824a51b2 in vcmn_err (ce=,
fmt=0x8228002a "zfs: allocating allocated segment(offset=%llu
size=%llu)\n",
adx=0xfe004daef980) at
/usr/src/sys/cddl/compat/opensolaris/kern/opensolaris_cmn_err.c:58
#12 0x821aa92d in zfs_panic_recover (fmt=0x12 )
at
/usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa_misc.c:1550
#13 0x82190c75 in range_tree_add (arg=0xf800069ba000,
start=3011224576, size=4096)
at
/usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/range_tree.c:179
#14 0x8218ee15 in metaslab_free_dva (spa=,
dva=, txg=184621, now=0)
at
/usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/metaslab.c:2498
#15 0x8218ef21 in metaslab_free (spa=,
bp=, txg=, now=)
at
/usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/metaslab.c:2618
#16 0x821dd35a in zio_dva_free (zio=) at
/usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c:2620
#17 0x821d90f8 in zio_execute (zio=) at
/usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c:1556
#18 0x80a8dcec in taskqueue_run_locked (queue=) at /usr/src/sys/kern/subr_taskqueue.c:465
#19 0x80a8e838 in taskqueue_thread_loop (arg=) at /usr/src/sys/kern/subr_taskqueue.c:719

(kgdb) p *(zio_t*)$r15
$1 = {io_bookmark = {zb_objset = 0, zb_object = 0, zb_level = 0,
zb_blkid = 0}, io_prop = {zp_checksum = ZIO_CHECKSUM_INHERIT,
zp_compress = ZIO_COMPRESS_INHERIT, zp_type = DMU_OT_NONE, zp_level
= 0 '\0', zp_copies = 0 '\0', zp_dedup = 0, zp_dedup_verify = 0,
zp_nopwrite = 0},
  io_type = ZIO_TYPE_FREE, io_child_type = ZIO_CHILD_LOGICAL, io_cmd =
0, io_priority = ZIO_PRIORITY_NOW, io_reexecute = 0 '\0',
  io_state = 0xf800295b87bd "", io_txg = 184621, io_spa =
0xfe0001746000, io_bp = 0xf800295b87e0, io_bp_override = 0x0,
io_bp_copy = {
blk_dva = 0xf800295b87e0, blk_prop = 9225915215840215047,
blk_pad = 0xf800295b8818, blk_phys_birth = 0, blk_birth = 184606,
blk_fill = 0,
blk_cksum = {zc_word = 0xf800295b8840}}, io_parent_list =
{list_size = 48, list_offset = 16, list_head = {list_next =
0xf80006386d90,
  list_prev = 0xf80006386d90}}, io_child_list = {list_size = 48,
list_offset = 32, list_head = {list_next = 0xf800295b8890,
  list_prev = 0xf800295b8890}}, io_walk_link = 0x0, io_logical =
0xf800295b8770, io_transform_stack = 0x0, io_ready = 0, io_physdone = 0,
  io_done = 0, io_private = 0x0, io_prev_space_delta = 0, io_bp_orig =
{blk_dva = 0xf800295b88e0, blk_prop = 9225915215840215047,
blk_pad = 0xf800295b8918, blk_phys_birth = 0, blk_birth =
184606, blk_fill = 0, blk_cksum = {zc_word = 0xf800295b8940}},
io_data = 0x0,
  io_orig_data = 0x0, io_size = 4096, io_orig_size = 4096, io_vd = 0x0,
io_vsd = 0x0, io_vsd_ops = 0x0, io_offset = 0, io_timestamp = 0,
  io_target_timestamp = 0, io_queue_node = {avl_child =
0xf800295b89b0, avl_pcb = 0}, io_offset_node = {avl_child =
0xf800295b89c8, avl_pcb = 0},
  io_flags = ZIO_FLAG_DONT_QUEUE, io_stage = ZIO_STAGE_DVA_FREE,
io_pipeline = 283, io_orig_flags = ZIO_FLAG_DONT_QUEUE,
  io_orig_stage = ZIO_STAGE_OPEN, io_orig_pipeline = 283, io_error =
0, io_child_error = 0xf800295b89fc, io_children = 0xf800295b8a10,
  io_child_count = 0, io_phys_children = 0, io_parent_count = 1,
io_stall = 0x0, io_gang_leader = 0x0, io_gang_tree = 0x0,
  io_executor = 0xf80006293500, io_waiter = 0x0, io_lock =
{lock_object = {lo_name = 0x8228dd22 "zio->io_lock", lo_flags =
4096,
  lo_data = 0, lo_witness = 0x0}, sx_lock = 1}, io_cv =
{cv_description = 0x8228dd30 "zio->io_cv", cv_waiters = 0},
io_cksum_report = 0x0,
  io_ena = 0, io_tqent = {tqent_task = {ta_link = {stqe_next = 0x0},
ta_pending = 0, ta_priority = 0, ta_func = 0x8211cd50
,
  ta_context = 0xf800295b8ad0}, tqent_func = 0x821d8e10
, tqent_arg = 0xf800295b8770}, io_trim_node = {
avl_child = 0xf800295b8b00, avl_pcb = 0}, io_trim_link =
{list_next = 0x0, list_prev = 0x0}}
-- 
Andriy Gapon


---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] panic: solaris assert: ba.ba_phys->bt_bytes == 0 (0x400 == 0x0)

[Andriy Gapon]

I am not yet convinced that the problem has anything to do with
miscompiled code.  I am using exactly the same optimizations and exactly
the same compiler as the official FreeBSD builds.

On 22/06/2016 17:03, Igor Kozhukhov wrote:
> Hi Andri,
> 
> i have DilOS with gcc-4.8,5 (+ special patches) for illumos builds.
> i had some problems with zdb - found it by zfs tests.
> 
> problem has been fixed by disable of optimization :
> -fno-aggressive-loop-optimizations
> 
> also, i have added:
> -fno-ipa-sra
> 
> but i no remember a story why i have added it ;)
> probabbly it was added with another illumos component and new gcc-4.8
> 
> As you know, illumos still is using gcc-4.4.4 and some newer compilers
> can produce new issues with older code :)
> 
> I think, you can try to play with your clang optimization flags too.
> i have no experience with clang.
> 
> best regards,
> -Igor
> 
> 
>> On Jun 22, 2016, at 4:21 PM, Andriy Gapon > <mailto:a...@freebsd.org>> wrote:
>>
>>
>> I am getting the following panic using the latest FreeBSD head that is
>> synchronized with OpenZFS code as of
>> illumos/illumos-gate@26455f9efcf9b1e44937d4d86d1ce37b006f25a9.
>>
>> panic: solaris assert: ba.ba_phys->bt_bytes == 0 (0x400 == 0x0), file:
>> /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/bptree.c,
>> line: 292
>> cpuid = 1
>> KDB: stack backtrace:
>> db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame
>> 0xfe004db9d310
>> vpanic() at vpanic+0x182/frame 0xfe004db9d390
>> panic() at panic+0x43/frame 0xfe004db9d3f0
>> assfail3() at assfail3+0x2c/frame 0xfe004db9d410
>> bptree_iterate() at bptree_iterate+0x35e/frame 0xfe004db9d540
>> dsl_scan_sync() at dsl_scan_sync+0x24f/frame 0xfe004db9d890
>> spa_sync() at spa_sync+0x897/frame 0xfe004db9dad0
>> txg_sync_thread() at txg_sync_thread+0x383/frame 0xfe004db9dbb0
>> fork_exit() at fork_exit+0x84/frame 0xfe004db9dbf0
>> fork_trampoline() at fork_trampoline+0xe/frame 0xfe004db9dbf0
>> --- trap 0, rip = 0, rsp = 0, rbp = 0 ---
>>
>> I have a crash dump, but unfortunately it's hard to work with it,
>> because a lot of useful information got "optimized out" by clang.
>>
>> I can reproduce the panic using a synthetic workload, but I do not have
>> a concise reproduction scenario.  Every time the panic happens bt_bytes
>> is 0x400, I haven't seen any other number there.
>>
>> Does anyone have an idea what could be causing this?
>> I can try any diagnostic code that might shed more light.
>> Thank you!
>>
>> -- 
>> Andriy Gapon
>>
>>
>> http://www.listbox.com
> 
> *openzfs-developer* | Archives
> <https://www.listbox.com/member/archive/274414/=now>
> <https://www.listbox.com/member/archive/rss/274414/28133750-22ed9730> |
> Modify
> <https://www.listbox.com/member/?&;>
> Your Subscription [Powered by Listbox] <http://www.listbox.com>
> 


-- 
Andriy Gapon


---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

[developer] panic: solaris assert: ba.ba_phys->bt_bytes == 0 (0x400 == 0x0)

[Andriy Gapon]

I am getting the following panic using the latest FreeBSD head that is
synchronized with OpenZFS code as of
illumos/illumos-gate@26455f9efcf9b1e44937d4d86d1ce37b006f25a9.

panic: solaris assert: ba.ba_phys->bt_bytes == 0 (0x400 == 0x0), file:
/usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/bptree.c, line: 292
cpuid = 1
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame
0xfe004db9d310
vpanic() at vpanic+0x182/frame 0xfe004db9d390
panic() at panic+0x43/frame 0xfe004db9d3f0
assfail3() at assfail3+0x2c/frame 0xfe004db9d410
bptree_iterate() at bptree_iterate+0x35e/frame 0xfe004db9d540
dsl_scan_sync() at dsl_scan_sync+0x24f/frame 0xfe004db9d890
spa_sync() at spa_sync+0x897/frame 0xfe004db9dad0
txg_sync_thread() at txg_sync_thread+0x383/frame 0xfe004db9dbb0
fork_exit() at fork_exit+0x84/frame 0xfe004db9dbf0
fork_trampoline() at fork_trampoline+0xe/frame 0xfe004db9dbf0
--- trap 0, rip = 0, rsp = 0, rbp = 0 ---

I have a crash dump, but unfortunately it's hard to work with it,
because a lot of useful information got "optimized out" by clang.

I can reproduce the panic using a synthetic workload, but I do not have
a concise reproduction scenario.  Every time the panic happens bt_bytes
is 0x400, I haven't seen any other number there.

Does anyone have an idea what could be causing this?
I can try any diagnostic code that might shed more light.
Thank you!

-- 
Andriy Gapon


---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [developer] [openzfs/openzfs] 6939 add sysevents to zfs core for commands (#101)

[Andriy Gapon]

On 03/05/2016 19:31, Garrett D'Amore wrote:
> 1. Will sysevents in the core create challenges for portability to other OS?
>  (Matt?  Andriy?  Brian?)

It seems like in FreeBSD we already have a compatibility shim for sysevents, at
least log_sysevent.


-- 
Andriy Gapon



---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

[developer] review request: 5380 call recv_incremental_replication only for recusrive streams

[Andriy Gapon]

https://reviews.csiden.org/r/269/
https://www.illumos.org/issues/5380
Thanks!

-- 
Andriy Gapon


---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

[developer] Re: Review Request 257: libzfs_core: remove the dependency of the interface on sys/fs/zfs.h

[Andriy Gapon]

> On Nov. 10, 2015, 11:29 p.m., Matthew Ahrens wrote:
> > Do you want to open a pull request for this to get the automated testing?
> 
> Andriy Gapon wrote:
> I am in the process of setting up a ZFS test suite environment locally.  
> I'll try to test this change in that environment.
> I can also open a PR if that would be more convenient for the community 
> and you.
> What would you recommend?
> 
> Matthew Ahrens wrote:
> It's easiest for me if you do all the tests and file the RTI on your own 
> :-)
> 
> If you want my help with any of that, then open a pull request.
> 
> Andriy Gapon wrote:
> Okay, that's fair :-)
> ```
> $ /opt/zfs-tests/bin/zfstest -a
> Test: /opt/zfs-tests/tests/functional/acl/cifs/setup (run as root) 
> [00:15] [PASS]
> Test: /opt/zfs-tests/tests/functional/acl/cifs/cifs_attr_001_pos (run as 
> root) [00:16] [PASS]
> Test: /opt/zfs-tests/tests/functional/acl/cifs/cifs_attr_002_pos (run as 
> root) [00:15] [PASS]
> ```
> 
> Matthew Ahrens wrote:
> FYI, I'm still happy to see this go upstream.  LMK if you need help.

Matt, thanks a lot! I was very close to submitting an RTI, but then some things 
changed and now I haven't booted my illumos VM in months.
It would be good if the change gets committed. Unfortunately, I can not 
contribute much towards that at the moment.


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/257/#review870
---


On Nov. 20, 2015, 12:17 p.m., Andriy Gapon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/257/
> ---
> 
> (Updated Nov. 20, 2015, 12:17 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List and Matthew Ahrens.
> 
> 
> Bugs: 6052
> https://www.illumos.org/issues/6052
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> Previously `lzc_create` had a parameter of `dmu_objset_type_t` type that 
> specified what kind of dataset to create.
> Now `lzc_dataset_type` enumeration is used for that purpose.
> At present only a filesystem type and a volume type can be specified.
> `lzc_dataset_type` values are binary compatible with `dmu_objset_type_t` 
> values.
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/sys/fs/zfs.h 569fae20915dc58bebd875fe5f244a82fdc02a9d 
>   usr/src/lib/libzfs_core/common/libzfs_core.h 
> bdd6c951ee496dc1e21a297e7a69b1342aecf79b 
>   usr/src/lib/libzfs/common/libzfs_dataset.c 
> d2f613ca63241bab7b23dfd4b4b1891e0245f660 
>   usr/src/lib/libzfs_core/common/libzfs_core.c 
> 22af0f4a7a9fd8ab15cc7880233ba51274ce87d8 
> 
> Diff: https://reviews.csiden.org/r/257/diff/
> 
> 
> Testing
> ---
> 
> ZFS Test Suite with the following failures:
> ```
> $ egrep 'FAIL|KILL' /var/tmp/test_results/20151116T101807/log
> Test: 
> /opt/zfs-tests/tests/functional/acl/nontrivial/zfs_acl_chmod_inherit_003_pos 
> (run as root) [00:20] [FAIL]
> Test: 
> /opt/zfs-tests/tests/functional/cli_root/zpool_clear/zpool_clear_001_pos (run 
> as root) [01:31] [FAIL]
> Test: 
> /opt/zfs-tests/tests/functional/cli_root/zpool_expand/zpool_expand_001_pos 
> (run as root) [00:14] [FAIL]
> Test: /opt/zfs-tests/tests/functional/cli_user/misc/zpool_add_001_neg (run as 
> avg) [00:01] [FAIL]
> Test: /opt/zfs-tests/tests/functional/cli_user/misc/zpool_create_001_neg (run 
> as avg) [00:00] [FAIL]
> Test: /opt/zfs-tests/tests/functional/mdb/mdb_001_pos (run as root) [00:24] 
> [FAIL]
> Test: /opt/zfs-tests/tests/functional/refreserv/refreserv_004_pos (run as 
> root) [00:00] [FAIL]
> Test: /opt/zfs-tests/tests/functional/rootpool/rootpool_002_neg (run as root) 
> [00:00] [FAIL]
> Test: /opt/zfs-tests/tests/functional/rsend/rsend_008_pos (run as root) 
> [00:01] [FAIL]
> Test: /opt/zfs-tests/tests/functional/rsend/rsend_009_pos (run as root) 
> [00:09] [FAIL]
> Test: /opt/zfs-tests/tests/functional/slog/slog_014_pos (run as root) [00:13] 
> [FAIL]
> Test: /opt/zfs-tests/tests/functional/zvol/zvol_swap/zvol_swap_004_pos (run 
> as root) [00:01] [FAIL]
> ```
> Looks like known failures, at least nothing pointing at `zfs create`
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>




---
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription: 
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com

Re: [OpenZFS Developer] Review Request 141: zfs send -p -I: send properties only for snapshots that are actually sent

[Andriy Gapon]

> On Feb. 15, 2015, 2:29 p.m., Andriy Gapon wrote:
> > usr/src/lib/libzfs/common/libzfs_sendrecv.c, line 628
> > <https://reviews.csiden.org/r/141/diff/1/?file=11927#file11927line628>
> >
> > As @dweeezil correctly noted this simple solution breaks several 
> > usecases.
> > First, zfs recv -F is broken because to implement it the receiving side 
> > needs to know about all snpashots that exist on the sending side.
> > Another bug is that the properties are not included into a full stream 
> > produced with zfs send -p pool/ds@snap.
> > Most likely the original performance issue better be fixed at the 
> > receiving end.

Coming back to this issue after a while.
I think that the biggest problem with my original patch was that it changed the 
content of the backup stream and that required that the receiving side is 
upgraded before it can properly receive the new streams. That's besides the 
unintentional bug. That was a very dangerous change as Tim has correctly 
pointed out.
So now I'm returning to my alternative idea of not changing the stream content 
at all and instead making changes only in the receiving code. That way the 
policy of least astonishment should not be violated.

Looking at the code I see that `recv_incremental_replication()` is called if 
`drr_payloadlen != 0` (and the stream is incremental), that is, if there is a 
serialized nvlist included with the stream begin record.  That is the case even 
if just `-p` option (as opposed to `-R`) is used on the sending side.  But, as 
far as I can see, the job of `recv_incremental_replication` is to handle 
situations like removed or renamed datasets, etc.  None of those things are 
applicable if the stream has been generated with `-p [-i|-I]`. That would be an 
incremental stream for a single dataset just with the snapshot properties 
included.  So, IMO, there is no need to apply all the 
`recv_incremental_replication` machinery in that case.  And it is that function 
that (re-)sets properties of all the already existing snapshots.  Finally, it 
should be noted that `zfs_receive_one()` already has the code to set the 
received properties, if any, for a newly received dataset.


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/141/#review522
---


On Dec. 2, 2014, 2:11 p.m., Justin Gibbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/141/
> ---
> 
> (Updated Dec. 2, 2014, 2:11 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List.
> 
> 
> Bugs: 5380
> https://www.illumos.org/issues/5380
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> ... as opposed to sending properties of all snapshots of the relevant
> filesystem.  The previous behavior results in properties being set on
> all snapshots on the receiving side, which is quite slow.
> 
> Behavior of zfs send -R is not changed.
> 
> Note that send_iterate_fs() now has to iterate snapshots in their
> creation order.
> 
> 
> Diffs
> -
> 
>   usr/src/lib/libzfs/common/libzfs_sendrecv.c 
> c4944438aa2cdcb751ae04a73be52ce9d7b2cef4 
> 
> Diff: https://reviews.csiden.org/r/141/diff/
> 
> 
> Testing
> ---
> 
> Only FreeBSD and ZoL.  None for illumos.
> 
> 
> Thanks,
> 
> Justin Gibbs
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] 6404, 6405 zvol test suite fixes (#27)

[Andriy Gapon]

> @@ -51,7 +51,7 @@ typeset -i min max mem
>  ((min = 2 * 1024 * 1024 * 1024))
>  ((max = 16 * 1024 * 1024 * 1024))
>  
> -for vbs in 512 1024 2048 4096 8192 16384 32768 65536 131072; do
> +for vbs in 8192 16384 32768 65536 131072; do
>   for multiplier in 1 32 16384 131072; do
>   ((volsize = vbs * multiplier))

BTW, do the `new_volsize` checks below actually work?
In my environment adding a volume as a swap device never changes its size, 
volsize always stays the same.

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/27/files#r45337344___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] 6404, 6405 zvol test suite fixes (#27)

[Andriy Gapon]

> @@ -51,7 +51,7 @@ typeset -i min max mem
>  ((min = 2 * 1024 * 1024 * 1024))
>  ((max = 16 * 1024 * 1024 * 1024))
>  
> -for vbs in 512 1024 2048 4096 8192 16384 32768 65536 131072; do
> +for vbs in 8192 16384 32768 65536 131072; do
>   for multiplier in 1 32 16384 131072; do
>   ((volsize = vbs * multiplier))

I solved this problem but skipping an iteration (using `continue`) if `volsize` 
is less than `2 * $(PAGESIZE)`.
At least that's how I interpreted information in the `swap` manual page. That 
seems to work.

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/27/files#r45336886___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 257: libzfs_core: remove the dependency of the interface on sys/fs/zfs.h

[Andriy Gapon]

> On Nov. 10, 2015, 11:29 p.m., Matthew Ahrens wrote:
> > Do you want to open a pull request for this to get the automated testing?
> 
> Andriy Gapon wrote:
> I am in the process of setting up a ZFS test suite environment locally.  
> I'll try to test this change in that environment.
> I can also open a PR if that would be more convenient for the community 
> and you.
> What would you recommend?
> 
> Matthew Ahrens wrote:
> It's easiest for me if you do all the tests and file the RTI on your own 
> :-)
> 
> If you want my help with any of that, then open a pull request.

Okay, that's fair :-)
```
$ /opt/zfs-tests/bin/zfstest -a
Test: /opt/zfs-tests/tests/functional/acl/cifs/setup (run as root) [00:15] 
[PASS]
Test: /opt/zfs-tests/tests/functional/acl/cifs/cifs_attr_001_pos (run as root) 
[00:16] [PASS]
Test: /opt/zfs-tests/tests/functional/acl/cifs/cifs_attr_002_pos (run as root) 
[00:15] [PASS]
```


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/257/#review870
-------


On Nov. 10, 2015, 8:55 p.m., Andriy Gapon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/257/
> ---
> 
> (Updated Nov. 10, 2015, 8:55 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List and Matthew Ahrens.
> 
> 
> Bugs: 6052
> https://www.illumos.org/issues/6052
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> Previously `lzc_create` had a parameter of `dmu_objset_type_t` type that 
> specified what kind of dataset to create.
> Now `lzc_dataset_type` enumeration is used for that purpose.
> At present only a filesystem type and a volume type can be specified.
> `lzc_dataset_type` values are binary compatible with `dmu_objset_type_t` 
> values.
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/sys/fs/zfs.h 569fae20915dc58bebd875fe5f244a82fdc02a9d 
>   usr/src/lib/libzfs_core/common/libzfs_core.h 
> bdd6c951ee496dc1e21a297e7a69b1342aecf79b 
>   usr/src/lib/libzfs/common/libzfs_dataset.c 
> d2f613ca63241bab7b23dfd4b4b1891e0245f660 
>   usr/src/lib/libzfs_core/common/libzfs_core.c 
> 22af0f4a7a9fd8ab15cc7880233ba51274ce87d8 
> 
> Diff: https://reviews.csiden.org/r/257/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 257: libzfs_core: remove the dependency of the interface on sys/fs/zfs.h

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/257/
---

(Updated Nov. 10, 2015, 8:55 p.m.)


Review request for OpenZFS Developer Mailing List and Matthew Ahrens.


Changes
---

`zfs_create`: catch up with the `lzc_create` change


Bugs: 6052
https://www.illumos.org/issues/6052


Repository: illumos-gate


Description
---

Previously `lzc_create` had a parameter of `dmu_objset_type_t` type that 
specified what kind of dataset to create.
Now `lzc_dataset_type` enumeration is used for that purpose.
At present only a filesystem type and a volume type can be specified.
`lzc_dataset_type` values are binary compatible with `dmu_objset_type_t` values.


Diffs (updated)
-

  usr/src/uts/common/sys/fs/zfs.h 569fae20915dc58bebd875fe5f244a82fdc02a9d 
  usr/src/lib/libzfs_core/common/libzfs_core.c 
22af0f4a7a9fd8ab15cc7880233ba51274ce87d8 
  usr/src/lib/libzfs_core/common/libzfs_core.h 
bdd6c951ee496dc1e21a297e7a69b1342aecf79b 
  usr/src/lib/libzfs/common/libzfs_dataset.c 
d2f613ca63241bab7b23dfd4b4b1891e0245f660 

Diff: https://reviews.csiden.org/r/257/diff/


Testing
---


Thanks,

Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] Properly handle updates of variably-sized SA entries. (#24)

[Andriy Gapon]

@ahrens, @behlendorf, I've tested my proposed code on FreeBSD and I am going to 
test it on ZoL.
BTW, the diff against the current ZoL code is not that dramatic:
ClusterHQ/zfs@aef0142ffe7d280ba32a7d97a7f83cf52cd03514
I'd even dare to say that it is trivial / cosmetic.

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/24#issuecomment-155044660___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] [openzfs] 5778 nvpair_type_is_array() does not recognize DATA_TYPE_INT8_ARRAY (#34)

[Andriy Gapon]

Tested using a trivial program.
You can view, comment on, or merge this pull request online at:

  https://github.com/openzfs/openzfs/pull/34

-- Commit Summary --

  * 5778 nvpair_type_is_array() does not recognize DATA_TYPE_INT8_ARRAY

-- File Changes --

M usr/src/common/nvpair/nvpair.c (1)

-- Patch Links --

https://github.com/openzfs/openzfs/pull/34.patch
https://github.com/openzfs/openzfs/pull/34.diff

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/34
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] Properly handle updates of variably-sized SA entries. (#24)

[Andriy Gapon]

Following up to my earlier comment regarding `j++` in the `SA_REMOVE` case.  As 
far as I can that code is not present in the latest ZoL, so that bug must have 
been fixed as a separate ZoL change.

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/24#issuecomment-154712842___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] 6051 lzc_receive: allow the caller to read the begin record (#30)

[Andriy Gapon]

@ahrens So far, the only testing of the new function is via the following 
Python code:
https://github.com/ClusterHQ/pyzfs/commit/340f585d100e4e8cca82e620b0d44e7da314e4cb#diff-34194bfc890283f17239b29a5346b088R2416

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/30#issuecomment-153981314___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] 6051 lzc_receive: allow the caller to read the begin record (#30)

[Andriy Gapon]

> @@ -548,8 +548,9 @@ recv_read(int fd, void *buf, int ilen)
>  }
>  
>  static int
> -lzc_receive_impl(const char *snapname, nvlist_t *props, const char *origin,
> -boolean_t force, boolean_t resumable, int fd)
> +recv_impl(const char *snapname, nvlist_t *props, const char *origin,

I renamed it just to avoid wrapping long lines elsewhere.
My thinking was that because this is a private function it does not have to 
have `lzc_` prefix.

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/30/files#r43806371___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] 6051 lzc_receive: allow the caller to read the begin record (#30)

[Andriy Gapon]

@ahrens I've updated the code.

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/30#issuecomment-153418653___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] Fix gcc warnings (#5)

[Andriy Gapon]

@allanjude if you need any help with that, just let me know - I have OI Hipster 
running on bhyve

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/5#issuecomment-153180402___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] sa_find_sizes() may compute wrong SA header size (#14)

[Andriy Gapon]

This pull request as well #24 present an interesting challenge. In order to 
verify that they do the right thing on illumos it would be nice to reproduce 
the problems that they fix and then to verify that the problems are indeed 
fixed.

But illumos does not support `xattr=sa` (seems to be ZoL-specific).
Nor does illumos seem to support setting ACLs or extended attributes on 
symbolic links.
Even such a simple thing as changing Unix mode of a symbolic link does not 
appear to be supported.

As far as I can tell, all the test cases for the related FreeBSD and ZoL bugs 
and fixes involve one of the above capabilities.  It seems that the bugs in 
question require more than one variable length SAs to be present.  This is 
probably the reason why the bugs haven not affected Solaris and illumos users 
so far.

If anyone has ideas for illumos test scenarios please share.

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/14#issuecomment-153037216___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] Properly handle updates of variably-sized SA entries. (#24)

[Andriy Gapon]

This is what I have in mind:
avg-I/openzfs@61e95060c4cd009e56f3034b0cdc7624ced3217b

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/24#issuecomment-152715803___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] 6051 lzc_receive: allow the caller to read the begin record (#30)

[Andriy Gapon]

> @@ -594,9 +595,14 @@ lzc_receive_impl(const char *snapname, nvlist_t *props, 
> const char *origin,
>   (void) strlcpy(zc.zc_string, origin, sizeof (zc.zc_string));
>  
>   /* zc_begin_record is non-byteswapped BEGIN record */
> - error = recv_read(fd, &zc.zc_begin_record, sizeof (zc.zc_begin_record));
> - if (error != 0)
> - goto out;
> + if (begin_record == NULL) {
> + error = recv_read(fd, &zc.zc_begin_record,
> + sizeof (zc.zc_begin_record));
> + if (error != 0)
> + goto out;
> + } else {
> + zc.zc_begin_record.drr_u.drr_begin = *begin_record;

Will do.

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/30/files#r43569650___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] 5752 dump_nvlist() is not aware of boolean array (#20)

[Andriy Gapon]

@ahrens I would like it to be 
I am juggling commits between multiple repositories with different 
configuration, sometimes that leads to confusions.

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/20#issuecomment-152715681___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] Properly handle updates of variably-sized SA entries. (#24)

[Andriy Gapon]

@trisk As an author of the FreeBSD change I see the following pluses in it.

It improves the description for `sa_modify_attrs`.

The code restructuring allows to increment `length_idx` in only one place under 
a very clear condition. Compare that to the ZoL change where the increment is 
done in two places and one of them, the original, is hidden within the array 
access.

Also, I do not understand why the original code increments `j` in the 
`SA_REMOVE` case and ZoL keeps that logic.  `j` is obviously an index into 
`attr_desc` that we are building. So, it seems that the code would leave a hole 
(an element with all zero fields) in `attr_desc`. Why? Are we writing beyond 
the end of the array in that case?  Are we losing the last attribute if the 
removed attribute is not it?
The FreeBSD code just skips the attribute being removed and does not create any 
hole.
Because of that I was able to assert that `j == attr_count` after the loop.

There are also advantages of the ZoL change.

There is a better comment before the loop.

`SA_REGISTERED_LEN` is assigned to a variable earlier and so the code becomes 
more compact.

The change is smaller and so it is easier to  understand (which does *not* 
imply that the resulting code is easier to read). There is one exception, 
though, in the last "hunk" it is not obvious why `buflen` is replaced with 
`length`. That change is a NOP, I believe.

Having said all of the above, I wouldn't just pick one or the other of the 
changes, but I'd try to fuse them and probably improve on top of them.  
However, I am not sure if the resulting change would be eligible for the fast 
track review via this repository. @ahrens ?
Maybe one of the changes should be imported as-is, indeed, and then further 
improvements could be submitted as a follow-up change...

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/24#issuecomment-152546122___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] [openzfs] 6051 lzc_receive: allow the caller to read the begin record (#30)

[Andriy Gapon]

The begin record contains useful information that the caller might
want to have the access to, so that it can be used to derive values
for 'snapname' and 'origin' parameters.
New lzc_receive_with_header function is added.

Review request on the reviewboard: https://reviews.csiden.org/r/256/
An example of the intended usage: 
ClusterHQ/pyzfs@340f585d100e4e8cca82e620b0d44e7da314e4cb
(see `test_recv_with_header_full`)
You can view, comment on, or merge this pull request online at:

  https://github.com/openzfs/openzfs/pull/30

-- Commit Summary --

  * 6051 lzc_receive: allow the caller to read the begin record

-- File Changes --

M usr/src/lib/libzfs_core/common/libzfs_core.c (40)
M usr/src/lib/libzfs_core/common/libzfs_core.h (7)
M usr/src/lib/libzfs_core/common/mapfile-vers (1)

-- Patch Links --

https://github.com/openzfs/openzfs/pull/30.patch
https://github.com/openzfs/openzfs/pull/30.diff

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/30
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 256: lzc_receive: allow the caller to read the begin record

[Andriy Gapon]

> On Oct. 24, 2015, 8:23 a.m., Matthew Ahrens wrote:
> > Changes look good.  FYI, you will need to explain what testing you did when 
> > filing the RTI.  At a minimum, run the test suite on illumos to exercise 
> > the changes to the pre-existing funcs.  (or open a pull request against 
> > https://github.com/openzfs/openzfs and we will do the regression tests and 
> > RTI for you)

Matt, thanks!  I will do one of those things.  The change has also been 
verified by the test cases included with https://github.com/ClusterHQ/pyzfs.
BTW, could you please updated sysnopsis of the connected issue?  It actually 
describes the previous suggested solution rather than the problem that we are 
trying to solve.


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/256/#review851
---


On Oct. 30, 2015, 12:54 p.m., Andriy Gapon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/256/
> ---
> 
> (Updated Oct. 30, 2015, 12:54 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List and Matthew Ahrens.
> 
> 
> Bugs: 6051
> https://www.illumos.org/issues/6051
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> The begin record contains useful information that the caller might
> want to have the access to, so that it can be used to derive values
> for 'snapname' and 'origin' parameters.
> 
> New lzc_receive_basic function is added as a replacement for the
> previous lzc_receive in the simpler situations.
> 
> 
> Diffs
> -
> 
>   usr/src/lib/libzfs_core/common/mapfile-vers 
> acd4efb07e8b97c093a7380281c96dc589f472c1 
>   usr/src/lib/libzfs_core/common/libzfs_core.c 
> d66f338a78baccaf8675f160b57fb93cb937a7ca 
>   usr/src/lib/libzfs_core/common/libzfs_core.h 
> 5d3a6fda7dcb9f96801d7f23647f410eb51f45bf 
> 
> Diff: https://reviews.csiden.org/r/256/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] 6385 Fix unlocking order in zfs_zget() (#6)

[Andriy Gapon]

@ahrens  yes.

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/6#issuecomment-152092710___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] pyzfs: python wrapper for libzfs_core

[Andriy Gapon]

At ClusterHQ we are developing [possibly yet another] Python wrapper for
libzfs_core.

>From the documentation:
libzfs_core is intended to be a stable interface for programmatic administration
of ZFS. This wrapper provides one-to-one wrappers for libzfs_core API functions,
but the signatures and types are more natural to Python. nvlists are wrapped as
dictionaries or lists depending on their usage. Some parameters have default
values depending on typical use for increased convenience. Enumerations and bit
flags become strings and lists of strings in Python. Errors are reported as
exceptions rather than integer errno-style error codes. The wrapper takes care
to provide one-to-many mapping of the error codes to the exceptions by
interpreting a context in which the error code is produced.

Package documentation: http://pyzfs.readthedocs.org
Package development: https://github.com/ClusterHQ/pyzfs
PyPI package: https://pypi.python.org/pypi/pyzfs

We will appreciate any feedback: bug reports, pull requests, comments,
suggestions, etc.
Thank you for your interest (hopefully) and time.

P.S.
For time being please ignore functions decorated with @_uncommitted.
They cover our libzfs_core extensions that we are submitting for review 
separately.
-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] [openzfs] sa_find_sizes() may compute wrong SA header size (#14)

[Andriy Gapon]

I think that the problem fixed by this change is still present in FreeBSD as 
well as in illumos.

The fix looks good to me.  I think that the problem that it solves is that the 
current code stops adding to `hdrsize` once we get into the trailing 
`blkptr_t`-sized area. So, if we (almost) fill the bonus buffer but do not 
actually spill over, then the calculated header size could be less than it 
should be.

P.S. another problem still present in illumos is the one fixed by the following 
ZoL and FreeBSD commits:
zfsonlinux/zfs@5d862cb0d9a4b6dcc97a88fa0d5a7a717566e5ab
freebsd/freebsd@a7cf665d5f7e2a94346c692418677188cca0356f
I think that ZoL commit message is much clearer while FreeBSD code is a little 
bit nicer :-)
Fusing those two commits together would be even better.

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/14#issuecomment-150617253___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] [openzfs] 5752 dump_nvlist() is not aware of boolean array (#20)

[Andriy Gapon]

Issue: https://www.illumos.org/issues/5752
FreeBSD commit: https://svnweb.freebsd.org/base?view=revision&revision=282121
You can view, comment on, or merge this pull request online at:

  https://github.com/openzfs/openzfs/pull/20

-- Commit Summary --

  * 5752 dump_nvlist() is not aware of boolean array

-- File Changes --

M usr/src/lib/libnvpair/libnvpair.c (11)

-- Patch Links --

https://github.com/openzfs/openzfs/pull/20.patch
https://github.com/openzfs/openzfs/pull/20.diff

---
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/20
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] avl_is_empty(&dn->dn_dbufs) assert in dnode_sync_free

[Andriy Gapon]

Just for the posterity, I was able to reproduce the problem on illumos as well
(running a kernel without Justin's fix):

> ::panicinfo
 cpu0
  thread ff0004819c40
 message assertion failed: avl_is_empty(&dn->dn_dbufs), file:
.../../common/fs/zfs/dnode_sync.c, line: 495

> $C
ff0004819770 vpanic()
ff00048197a0 0xfbdf37d8()
ff00048197e0 dnode_sync_free+0x278(ff01cb3b6528, ff0166429a00)
ff0004819860 dnode_sync+0x68b(ff01cb3b6528, ff0166429a00)
ff00048198b0 dmu_objset_sync_dnodes+0x93(ff014af3d330, 0, 
ff0166429a00)
ff00048199b0 dmu_objset_sync+0x1bc(ff014af3d080, ff014b1ff560,
ff0166429a00)
ff00048199f0 dsl_pool_sync_mos+0x42(ff01495d7e80, ff0166429a00)
ff0004819a80 dsl_pool_sync+0x2fe(ff01495d7e80, 233d6)
ff0004819b50 spa_sync+0x27e(ff014c2c, 233d6)
ff0004819c20 txg_sync_thread+0x260(ff01495d7e80)
ff0004819c30 thread_start+8()

On 28/09/2015 17:25, Andriy Gapon wrote:
> 
> There are several reports from FreeBSD users about getting a panic because of
> avl_is_empty(&dn->dn_dbufs) assertion in dnode_sync_free().  I was also able 
> to
> reproduce the problem with ZFS on Linux 0.6.5. There does not seem to be any
> reports from illumos users.
> 
> I think that the following stack traces demonstrate the problem rather well 
> (the
> stack traces are a little bit unusual as they come from Linux's crash utility,
> but should be legible):
> crash> foreach UN bt
> PID: 703TASK: 88003b8a4440  CPU: 0   COMMAND: "txg_sync"
>  #0 [880039fa3848] __schedule at 8160918d
>  #1 [880039fa38b0] schedule at 816096e9
>  #2 [880039fa38c0] spl_panic at a0012645 [spl]
>  #3 [880039fa3a48] dnode_sync at a062b7cf [zfs]
>  #4 [880039fa3b38] dmu_objset_sync_dnodes at a0612dd7 [zfs]
>  #5 [880039fa3b78] dmu_objset_sync at a06130d5 [zfs]
>  #6 [880039fa3c50] dsl_pool_sync at a0641a8a [zfs]
>  #7 [880039fa3cd0] spa_sync at a0664408 [zfs]
>  #8 [880039fa3da0] txg_sync_thread at a067b970 [zfs]
>  #9 [880039fa3e98] thread_generic_wrapper at a000e18a [spl]
> #10 [880039fa3ec8] kthread at 8109726f
> #11 [880039fa3f50] ret_from_fork at 81614198
> 
> PID: 716TASK: 88003b8a6660  CPU: 0   COMMAND: "trial"
>  #0 [88003c68f738] __schedule at 8160918d
>  #1 [88003c68f7a0] schedule at 816096e9
>  #2 [88003c68f7b0] cv_wait_common at a0014d15 [spl]
>  #3 [88003c68f818] __cv_wait at a0014e65 [spl]
>  #4 [88003c68f828] txg_wait_synced at a067a70f [zfs]
>  #5 [88003c68f868] dsl_sync_task at a064b017 [zfs]
>  #6 [88003c68f928] dsl_destroy_head at a06eee62 [zfs]
>  #7 [88003c68f978] dmu_recv_cleanup_ds at a06194ed [zfs]
>  #8 [88003c68fa98] dmu_recv_stream at a061a992 [zfs]
>  #9 [88003c68fc20] zfs_ioc_recv at a06b1bad [zfs]
> #10 [88003c68fe50] zfsdev_ioctl at a06b3c86 [zfs]
> #11 [88003c68feb8] do_vfs_ioctl at 811d9ca5
> #12 [88003c68ff30] sys_ioctl at 811d9f21
> #13 [88003c68ff80] system_call_fastpath at 81614249
> RIP: 7ff39d5c0257  RSP: 7ff38e5c2008  RFLAGS: 00010206
> RAX: 0010  RBX: 81614249  RCX: 0024
> RDX: 7ff38e5c21d0  RSI: 5a1b  RDI: 0004
> RBP: 7ff38e5c57b0   R8: 342d663438372d62   R9: 636430382d646335
> R10: 643266636131612d  R11: 0246  R12: 0060
> R13: 7ff38e5c3200  R14: 7ff3880080a0  R15: 7ff38e5c21d0
> ORIG_RAX: 0010  CS: 0033  SS: 002b
> 
> PID: 31758  TASK: 88003b332d80  CPU: 0   COMMAND: "dbu_evict"
>  #0 [88003b723ca0] __schedule at 8160918d
>  #1 [88003b723d08] schedule_preempt_disabled at 8160a8d9
>  #2 [88003b723d18] __mutex_lock_slowpath at 81608625
>  #3 [88003b723d78] mutex_lock at 81607a8f
>  #4 [88003b723d90] dbuf_rele at a05fd290 [zfs]
>  #5 [88003b723db0] dmu_buf_rele at a05fe57e [zfs]
>  #6 [88003b723dc0] bpobj_close at a05f78ed [zfs]
>  #7 [88003b723dd8] dsl_deadlist_close at a0636e19 [zfs]
>  #8 [88003b723e10] dsl_dataset_evict at a062d78b [zfs]
>  #9 [88003b723e28] taskq_thread at a000f912 [spl]
> #10 [88003b723ec8] kthread at 8109726f
> #11 [88003b723f50] ret_from_fork at 81614198
> 
> In 100% cases where I hit the assertion it was with DMU_OT_BPOBJ dnodes.
> Justin thinks that the situation is h

Re: [OpenZFS Developer] zfs receive: -u can be ignored sometimes

[Andriy Gapon]

So, should I open a bug report about this?

On 21/10/2015 16:25, Andriy Gapon wrote:
> On 21/10/2015 16:08, Andriy Gapon wrote:
>>
>> It seems that zfs receive -F -u would mount a received filesystem after
>> receiving a full stream if a destination filesystem already existed (and, 
>> thus,
>> got destroyed and re-created) and was mounted.
>> Is this a bug?
>> Or is this behavior actually preferable and only needs to be documented? [*]
>>
>> [*] As it is now, the interaction of -F and receiving a full stream is not
>> documented at all.
>>
> 
> How to reproduce:
> $ zfs create rpool/sandbox
> $ zfs create rpool/sandbox/from
> $ zfs create rpool/sandbox/to
> $ zfs snap rpool/sandbox/from@snap
> $ zfs send rpool/sandbox/from@snap | zfs recv -v -F -u rpool/sandbox/to
> receiving full stream of rpool/sandbox/from@snap into rpool/sandbox/to@snap
> received 41.7KB stream in 1 seconds (41.7KB/sec)
> $ zfs get mounted rpool/sandbox/to
> NAME  PROPERTY  VALUESOURCE
> rpool/tmp/sandbox/to  mounted   yes  -
> 
> This behavior can be more problematic if the mountpoint property changes 
> either
> because it had a non-inherited value or the stream contains properties because
> it has been generated with either -R or -p.
> 


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 256: lzc_receive: allow the caller to read the begin record

[Andriy Gapon]

> On Oct. 10, 2015, 12:27 a.m., Matthew Ahrens wrote:
> > you also need to modify the mapfile

I think I know the answer but just in case: should the new symbol be added 
under the same version `ILLUMOS_0.1`?


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/256/#review825
---


On Oct. 9, 2015, 11:55 a.m., Andriy Gapon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/256/
> ---
> 
> (Updated Oct. 9, 2015, 11:55 a.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List and Matthew Ahrens.
> 
> 
> Bugs: 6051
> https://www.illumos.org/issues/6051
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> The begin record contains useful information that the caller might
> want to have the access to, so that it can be used to derive values
> for 'snapname' and 'origin' parameters.
> 
> New lzc_receive_basic function is added as a replacement for the
> previous lzc_receive in the simpler situations.
> 
> 
> Diffs
> -
> 
>   usr/src/lib/libzfs_core/common/libzfs_core.c 
> 22af0f4a7a9fd8ab15cc7880233ba51274ce87d8 
>   usr/src/lib/libzfs_core/common/libzfs_core.h 
> bdd6c951ee496dc1e21a297e7a69b1342aecf79b 
> 
> Diff: https://reviews.csiden.org/r/256/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 256: lzc_receive: allow the caller to read the begin record

[Andriy Gapon]

> On Oct. 10, 2015, 12:27 a.m., Matthew Ahrens wrote:
> > usr/src/lib/libzfs_core/common/libzfs_core.c, lines 557-559
> > <https://reviews.csiden.org/r/256/diff/1/?file=17800#file17800line557>
> >
> > Are you sure that we want to change the API, vs. adding a new function 
> > with the new signature?  We haven't committed to the lzc interface yet so 
> > we still can change it, but it seems "nice" to retain compatability where 
> > it isn't too gross.
> > 
> > e.g. add a new func like "lzc_receive2" or "lzc_receive_record" that 
> > takes the begin_record, and leave the old lzc_receive with the same 
> > interface.

I probably misunderstood an earlier suggestion of yours.  Indeed, I do not see 
any good reason to break the backward compatibility.
I'll spend a little more time looking for a better name for the new function. 
`lzc_receive2` is pretty non-descriptive. `lzc_receive_record` is better, but 
still could be misinterpreted as "receive a (single) record".  To be honest I 
do not have anything better on my mind right now, but *[having just read 
[this](http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html)]* I hope that 
there is a name that would make the purpose of the new function clear enough 
even without looking at its documentation or even implementation.


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/256/#review825
---


On Oct. 9, 2015, 11:55 a.m., Andriy Gapon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/256/
> ---
> 
> (Updated Oct. 9, 2015, 11:55 a.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List and Matthew Ahrens.
> 
> 
> Bugs: 6051
> https://www.illumos.org/issues/6051
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> The begin record contains useful information that the caller might
> want to have the access to, so that it can be used to derive values
> for 'snapname' and 'origin' parameters.
> 
> New lzc_receive_basic function is added as a replacement for the
> previous lzc_receive in the simpler situations.
> 
> 
> Diffs
> -
> 
>   usr/src/lib/libzfs_core/common/libzfs_core.c 
> 22af0f4a7a9fd8ab15cc7880233ba51274ce87d8 
>   usr/src/lib/libzfs_core/common/libzfs_core.h 
> bdd6c951ee496dc1e21a297e7a69b1342aecf79b 
> 
> Diff: https://reviews.csiden.org/r/256/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] zfs receive: -u can be ignored sometimes

[Andriy Gapon]

On 21/10/2015 16:08, Andriy Gapon wrote:
> 
> It seems that zfs receive -F -u would mount a received filesystem after
> receiving a full stream if a destination filesystem already existed (and, 
> thus,
> got destroyed and re-created) and was mounted.
> Is this a bug?
> Or is this behavior actually preferable and only needs to be documented? [*]
> 
> [*] As it is now, the interaction of -F and receiving a full stream is not
> documented at all.
> 

How to reproduce:
$ zfs create rpool/sandbox
$ zfs create rpool/sandbox/from
$ zfs create rpool/sandbox/to
$ zfs snap rpool/sandbox/from@snap
$ zfs send rpool/sandbox/from@snap | zfs recv -v -F -u rpool/sandbox/to
receiving full stream of rpool/sandbox/from@snap into rpool/sandbox/to@snap
received 41.7KB stream in 1 seconds (41.7KB/sec)
$ zfs get mounted rpool/sandbox/to
NAME  PROPERTY  VALUESOURCE
rpool/tmp/sandbox/to  mounted   yes  -

This behavior can be more problematic if the mountpoint property changes either
because it had a non-inherited value or the stream contains properties because
it has been generated with either -R or -p.

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] zfs receive: -u can be ignored sometimes

[Andriy Gapon]

It seems that zfs receive -F -u would mount a received filesystem after
receiving a full stream if a destination filesystem already existed (and, thus,
got destroyed and re-created) and was mounted.
Is this a bug?
Or is this behavior actually preferable and only needs to be documented? [*]

[*] As it is now, the interaction of -F and receiving a full stream is not
documented at all.

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] Review Request 257: libzfs_core: remove the dependency of the interface on sys/fs/zfs.h

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/257/
---

Review request for OpenZFS Developer Mailing List and Matthew Ahrens.


Bugs: 6052
https://www.illumos.org/issues/6052


Repository: illumos-gate


Description
---

Previously `lzc_create` had a parameter of `dmu_objset_type_t` type that 
specified what kind of dataset to create.
Now `lzc_dataset_type` enumeration is used for that purpose.
At present only a filesystem type and a volume type can be specified.
`lzc_dataset_type` values are binary compatible with `dmu_objset_type_t` values.


Diffs
-

  usr/src/lib/libzfs_core/common/libzfs_core.c 
22af0f4a7a9fd8ab15cc7880233ba51274ce87d8 
  usr/src/lib/libzfs_core/common/libzfs_core.h 
bdd6c951ee496dc1e21a297e7a69b1342aecf79b 

Diff: https://reviews.csiden.org/r/257/diff/


Testing
---


Thanks,

Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] Review Request 256: lzc_receive: allow the caller to read the begin record

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/256/
---

Review request for OpenZFS Developer Mailing List and Matthew Ahrens.


Bugs: 6051
https://www.illumos.org/issues/6051


Repository: illumos-gate


Description
---

The begin record contains useful information that the caller might
want to have the access to, so that it can be used to derive values
for 'snapname' and 'origin' parameters.

New lzc_receive_basic function is added as a replacement for the
previous lzc_receive in the simpler situations.


Diffs
-

  usr/src/lib/libzfs_core/common/libzfs_core.c 
22af0f4a7a9fd8ab15cc7880233ba51274ce87d8 
  usr/src/lib/libzfs_core/common/libzfs_core.h 
bdd6c951ee496dc1e21a297e7a69b1342aecf79b 

Diff: https://reviews.csiden.org/r/256/diff/


Testing
---


Thanks,

Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 255: 6295 metaslab_condense's dbgmsg should include vdev id

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/255/#review812
---

Ship it!


Looks good.

- Andriy Gapon


On Oct. 7, 2015, 7:38 a.m., Matthew Ahrens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/255/
> ---
> 
> (Updated Oct. 7, 2015, 7:38 a.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List and Christopher Siden.
> 
> 
> Bugs: 6295
> https://www.illumos.org/issues/6295
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> 6295 metaslab_condense's dbgmsg should include vdev id
> Reviewed by: George Wilson 
> Reviewed by: Matthew Ahrens 
> 
> Original author: Joe Stein
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/metaslab.c 
> 9ba9fd384141ac553e8ede51a67870a33fabc2c1 
> 
> Diff: https://reviews.csiden.org/r/255/diff/
> 
> 
> Testing
> ---
> 
> http://jenkins.delphix.com/job/zfs-precommit/3142/
> 
> 
> Thanks,
> 
> Matthew Ahrens
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 250: DLPX-37488 exporting a pool while an async destroy is running can leave entries in the deferred tree

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/250/#review811
---

Ship it!


Looks good to me.

- Andriy Gapon


On Oct. 7, 2015, 6:02 a.m., Matthew Ahrens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/250/
> ---
> 
> (Updated Oct. 7, 2015, 6:02 a.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List and Christopher Siden.
> 
> 
> Bugs: 6292
> https://www.illumos.org/issues/6292
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> 6292 exporting a pool while an async destroy is running can leave entries in 
> the deferred tree
> Reviewed by: Paul Dagnelie 
> Reviewed by: Matthew Ahrens 
> 
> Original author: George Wilson
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/dsl_scan.c 
> 6ba5cb6a1c831f681fba16dbc8d25fd8b59b13c9 
> 
> Diff: https://reviews.csiden.org/r/250/diff/
> 
> 
> Testing
> ---
> 
> http://jenkins.delphix.com/job/zfs-precommit/3149/
> 
> 
> Thanks,
> 
> Matthew Ahrens
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 247: 6286 ZFS internal error when set large block on bootfs

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/247/#review810
---

Ship it!


Looks god to me.

- Andriy Gapon


On Oct. 6, 2015, 6:56 p.m., Matthew Ahrens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/247/
> ---
> 
> (Updated Oct. 6, 2015, 6:56 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List and Christopher Siden.
> 
> 
> Bugs: 6286
> https://www.illumos.org/issues/6286
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> 6286 ZFS internal error when set large block on bootfs
> Reviewed by: Paul Dagnelie 
> Reviewed by: George Wilson 
> 
> Original author: Matthew Ahrens
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/zfs_ioctl.c 
> 73cf63ad9a65598bcb33caa50b1be50cee2da02f 
> 
> Diff: https://reviews.csiden.org/r/247/diff/
> 
> 
> Testing
> ---
> 
> http://jenkins.delphix.com/job/zfs-precommit/3132/
> 
> 
> Thanks,
> 
> Matthew Ahrens
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 245: 6267 dn_bonus evicted too early

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/245/#review809
---


This change fixed the issue that I've been seeing with ZoL.
Also, as far as our testing and workloads go, I do not see any regressions.

- Andriy Gapon


On Oct. 6, 2015, 9:39 p.m., Justin Gibbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/245/
> ---
> 
> (Updated Oct. 6, 2015, 9:39 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List and Christopher Siden.
> 
> 
> Bugs: 6267
> https://www.illumos.org/issues/6267
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> ```
> Summary
> ===
> o Fix bonus buffer eviction regression introduced by the fixes for
>   illumos issue 5056.
> 
> o Remove assert that can trigger due to a long standing race between
>   draining references on dbufs and processing of freed dnodes in the
>   syncer. References are guaranteed to drain, and the introduction
>   of db_pending_evict ensures proper dbuf eviction when references
>   finally do drain.
> 
> Details
> ===
> The bonus buffer associated with a dnode is expected to remain resident
> until: the dnode is evicted via dnode_buf_pageout(), the dnode is
> freed in dnode_sync_free(), or the objset containing the dnode is
> evicted via dmu_objset_evict(). However, since bonus buffers (and DMU
> buffers in general) can have draining references when these events occur,
> dbuf_rele_and_unlock() has logic to ensure that once these late references
> are released, affected buffers will be evicted.
> 
> dbuf_rele_and_unlock() currently checks for a dbuf for an evicting
> objset via the os->os_evicting flag, and detects buffers for a freed
> dnode by testing dn->dn_type and dn->dn_free_txg fields. Unfortunately,
> the free'd dnode test can fire prematurely - anytime after the dnode is
> scheduled to be freed via dnode_free() until the free is committed in
> dnode_sync_free(). If all references to the bonus buffer are dropped
> within this window, the bonus buffer will be evicted and code in
> dnode_sync() that relies on the bonus buffer will fail.
> 
> Additionally, the "free'd dnode test" isn't applied to normal buffers
> (buffers that are not the bonus buffer) and there is no mechanism to
> guarantee eviction in the dnode_buf_pageout() case (the dnode is not
> being freed nor is the objset being evicted).
> 
> Replace the two existing deferred eviction mechanisms with a per-dbuf
> flag, db_pending_evict. This is set when explicit eviction is requested
> via either dnode_evict_dbufs() or dnode_evict_bonus(). These actions
> only occur after it is safe for dnode buffers to be evicted (e.g. the
> bonus buffer will not be referenced again).
> 
> uts/common/fs/zfs/sys/dbuf.h:
>   Add comments for boolean fields in dmu_buf_impl_t.
> 
>   Add the db_pending_evict field.
> 
> uts/common/fs/zfs/sys/dbuf.h:
> uts/common/fs/zfs/dbuf.c:
>   Rename db_immediate_evict to db_user_immediate_evict to avoid
>   confusion between dbuf user state eviction and deferred eviction
>   of a dbuf.
> 
> uts/common/fs/zfs/dbuf.c:
>   Consistently use TRUE/FALSE for boolean fields in
>   dmu_buf_impl_t.
> 
>   Simplify pending eviction logic to use the new db_pending_evict
>   flag in all cases.
> 
> uts/common/fs/zfs/dmu_objset.c:
> uts/common/fs/zfs/sys/dmu_objset.h:
>   Remove objset_t's os_evicting field. This same functionality
>   is now provided by db_pending_evict.
> 
> uts/common/fs/zfs/dnode_sync.c:
>   In dnode_evict_dbufs() and dnode_evict_bonus(), mark dbufs
>   with draining references (dbufs that can't be evicted inline)
>   as pending_evict.
> 
>   In dnode_sync_free(), remove ASSERT() that a dnode being free'd
>   has no active dbufs. This is usually the case, but is not
>   guaranteed due to draining references. (e.g. The deadlist for a
>   deleted dataset may still be open if another thread referenced
>   the dataset just before it was freed and the dsl_dataset_t hasn't
>   been released or is still being evicted).
> ```
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/sys/dmu_objset.h 
> 9e98350f001f81a5399b8d8034d9f502d8c771ad 
>   usr/src/uts/common/fs/zfs/dnode_sync.c 
> 0787885229d43e78fdbfd21bd12e75b2e67e9951 
>   usr/src/uts/common/

Re: [OpenZFS Developer] Review Request 240: 2605 want to resume interrupted zfs send

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/240/#review791
---



usr/src/lib/libzfs/common/libzfs_sendrecv.c (line 1460)
<https://reviews.csiden.org/r/240/#comment595>

Since `zfs_send_resume_token_to_nvlist` is a non-trivial function, perhaps 
split the defintion and initialization of `resume_nvl`?


- Andriy Gapon


On Sept. 23, 2015, 6:48 p.m., Matthew Ahrens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/240/
> ---
> 
> (Updated Sept. 23, 2015, 6:48 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List and Christopher Siden.
> 
> 
> Bugs: 2605
> https://www.illumos.org/issues/2605
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> 2605 want to resume interrupted zfs send
> 
> Original author: Matthew Ahrens
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/sys/fs/zfs.h 569fae20915dc58bebd875fe5f244a82fdc02a9d 
>   usr/src/uts/common/fs/zfs/sys/zfs_ioctl.h 
> 1e88212425d967a508dd43263dd0d5778458a857 
>   usr/src/uts/common/fs/zfs/sys/dsl_dataset.h 
> c9cd5890aa7438ef5d7d77f8c26eeb833ac5c39e 
>   usr/src/uts/common/fs/zfs/sys/dmu_traverse.h 
> 544b721e46129a06f5bc3d86ec8e9c3e6d685be1 
>   usr/src/uts/common/fs/zfs/sys/dmu_impl.h 
> cf45e91dba6dd161b430dfc6b4b4fce61989fcce 
>   usr/src/uts/common/fs/zfs/zfs_ioctl.c 
> 73cf63ad9a65598bcb33caa50b1be50cee2da02f 
>   usr/src/uts/common/fs/zfs/dsl_dataset.c 
> 8f9e26bc16838a1565dd3ba6a61beaabd1dff848 
>   usr/src/uts/common/fs/zfs/dmu_send.c 
> a0bf23349aa647f0616b5dac5333fb216a119e62 
>   usr/src/test/zfs-tests/tests/functional/rsend/rsend_022_pos.ksh 
> PRE-CREATION 
>   usr/src/test/zfs-tests/tests/functional/rsend/Makefile 
> fb757b29a365858f2fd82fa972a733b102d78e2c 
>   usr/src/test/zfs-tests/runfiles/delphix.run 
> 0ab2739792b85da61d434482868ba0fbd3002fc0 
>   usr/src/pkg/manifests/system-test-zfstest.mf 
> f9818c15e49b5b7f36c953faa642a8db22729560 
>   usr/src/lib/libzfs_core/common/libzfs_core.c 
> 22af0f4a7a9fd8ab15cc7880233ba51274ce87d8 
>   usr/src/lib/libzfs/common/mapfile-vers 
> c46260717e70351434951e7f4c53e5a44b5d3afb 
>   usr/src/lib/libzfs/common/libzfs_dataset.c 
> d2f613ca63241bab7b23dfd4b4b1891e0245f660 
>   usr/src/lib/libzfs/common/libzfs.h 6cc0ef8114563c167a6af967f9f77568c65b3fb5 
>   usr/src/lib/libzfs/Makefile.com e7d33f3a028c56a6ba6e7e53a3aca51974a33505 
>   usr/src/common/zfs/zfs_prop.c 11abc0d46fa87723de0623402c6cbb55b3582d25 
>   usr/src/uts/common/fs/zfs/sys/dmu_send.h 
> 2442a1f8aab10b86a5eff4d89e02f1f3db4bb4b9 
>   usr/src/uts/common/fs/zfs/dsl_destroy.c 
> c7a623c8291b2f98e7021fa49d5b0b4b22657717 
>   usr/src/uts/common/fs/zfs/dmu_traverse.c 
> 9222b54d624c29eafc57e0ad1e5a9e42bdf50767 
>   usr/src/uts/common/fs/zfs/dmu_objset.c 
> f84ff378c947b913a671dd1790466f940513666c 
>   usr/src/test/zfs-tests/tests/functional/rsend/rsend_024_pos.ksh 
> PRE-CREATION 
>   usr/src/test/zfs-tests/tests/functional/rsend/rsend_021_pos.ksh 
> PRE-CREATION 
>   usr/src/test/zfs-tests/tests/functional/rsend/rsend_020_pos.ksh 
> PRE-CREATION 
>   usr/src/test/zfs-tests/tests/functional/rsend/rsend_019_pos.ksh 
> PRE-CREATION 
>   usr/src/test/zfs-tests/tests/functional/rsend/rsend.kshlib 
> 0a856cfe975b66c7fb19d1714e9ae98be47b5d8b 
>   usr/src/test/zfs-tests/runfiles/openindiana.run 
> f525b7297edc9bdc6e363af96896a3dbf826baec 
>   usr/src/test/zfs-tests/runfiles/omnios.run 
> f19c83f10d4c5b3e26083a224be9f9914d19f8cd 
>   usr/src/test/zfs-tests/include/commands.cfg 
> 36f9457197c37c950d35d42b5129f8ff33a41b5b 
>   usr/src/man/man1m/zfs.1m d5edce0b9b6881cb6065cd488c0a77e86b357596 
>   usr/src/lib/libzfs_core/common/mapfile-vers 
> ebed35cfd03f8abca4c2c510198b99cc7a75a84a 
>   usr/src/lib/libzfs_core/common/libzfs_core.h 
> bdd6c951ee496dc1e21a297e7a69b1342aecf79b 
>   usr/src/lib/libzfs/common/libzfs_sendrecv.c 
> b155fc86996dc2fa61a326b317c05f52967a6ff3 
>   usr/src/lib/libzfs/common/libzfs_mount.c 
> 7625a7d8e93f6690433e309a9d8060bd716e63ef 
>   usr/src/cmd/zstreamdump/zstreamdump.c 
> f6dedc2fa384cb243f7228192e8ee26f9a2e1134 
>   usr/src/cmd/zfs/zfs_main.c 1ed01b404810a01e663eeb24a522f888335b7bde 
>   usr/src/cmd/truss/expound.c 915ec4626b0f2458c531afde4b57161e1e08a036 
> 
> Diff: https://reviews.csiden.org/r/240/diff/
> 
> 
> Testing
> ---
> 
> Test suite including new tests
> 
> 
> Thanks,
> 
> Matthew Ahrens
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] avl_is_empty(&dn->dn_dbufs) assert in dnode_sync_free

[Andriy Gapon]

There are several reports from FreeBSD users about getting a panic because of
avl_is_empty(&dn->dn_dbufs) assertion in dnode_sync_free().  I was also able to
reproduce the problem with ZFS on Linux 0.6.5. There does not seem to be any
reports from illumos users.

I think that the following stack traces demonstrate the problem rather well (the
stack traces are a little bit unusual as they come from Linux's crash utility,
but should be legible):
crash> foreach UN bt
PID: 703TASK: 88003b8a4440  CPU: 0   COMMAND: "txg_sync"
 #0 [880039fa3848] __schedule at 8160918d
 #1 [880039fa38b0] schedule at 816096e9
 #2 [880039fa38c0] spl_panic at a0012645 [spl]
 #3 [880039fa3a48] dnode_sync at a062b7cf [zfs]
 #4 [880039fa3b38] dmu_objset_sync_dnodes at a0612dd7 [zfs]
 #5 [880039fa3b78] dmu_objset_sync at a06130d5 [zfs]
 #6 [880039fa3c50] dsl_pool_sync at a0641a8a [zfs]
 #7 [880039fa3cd0] spa_sync at a0664408 [zfs]
 #8 [880039fa3da0] txg_sync_thread at a067b970 [zfs]
 #9 [880039fa3e98] thread_generic_wrapper at a000e18a [spl]
#10 [880039fa3ec8] kthread at 8109726f
#11 [880039fa3f50] ret_from_fork at 81614198

PID: 716TASK: 88003b8a6660  CPU: 0   COMMAND: "trial"
 #0 [88003c68f738] __schedule at 8160918d
 #1 [88003c68f7a0] schedule at 816096e9
 #2 [88003c68f7b0] cv_wait_common at a0014d15 [spl]
 #3 [88003c68f818] __cv_wait at a0014e65 [spl]
 #4 [88003c68f828] txg_wait_synced at a067a70f [zfs]
 #5 [88003c68f868] dsl_sync_task at a064b017 [zfs]
 #6 [88003c68f928] dsl_destroy_head at a06eee62 [zfs]
 #7 [88003c68f978] dmu_recv_cleanup_ds at a06194ed [zfs]
 #8 [88003c68fa98] dmu_recv_stream at a061a992 [zfs]
 #9 [88003c68fc20] zfs_ioc_recv at a06b1bad [zfs]
#10 [88003c68fe50] zfsdev_ioctl at a06b3c86 [zfs]
#11 [88003c68feb8] do_vfs_ioctl at 811d9ca5
#12 [88003c68ff30] sys_ioctl at 811d9f21
#13 [88003c68ff80] system_call_fastpath at 81614249
RIP: 7ff39d5c0257  RSP: 7ff38e5c2008  RFLAGS: 00010206
RAX: 0010  RBX: 81614249  RCX: 0024
RDX: 7ff38e5c21d0  RSI: 5a1b  RDI: 0004
RBP: 7ff38e5c57b0   R8: 342d663438372d62   R9: 636430382d646335
R10: 643266636131612d  R11: 0246  R12: 0060
R13: 7ff38e5c3200  R14: 7ff3880080a0  R15: 7ff38e5c21d0
ORIG_RAX: 0010  CS: 0033  SS: 002b

PID: 31758  TASK: 88003b332d80  CPU: 0   COMMAND: "dbu_evict"
 #0 [88003b723ca0] __schedule at 8160918d
 #1 [88003b723d08] schedule_preempt_disabled at 8160a8d9
 #2 [88003b723d18] __mutex_lock_slowpath at 81608625
 #3 [88003b723d78] mutex_lock at 81607a8f
 #4 [88003b723d90] dbuf_rele at a05fd290 [zfs]
 #5 [88003b723db0] dmu_buf_rele at a05fe57e [zfs]
 #6 [88003b723dc0] bpobj_close at a05f78ed [zfs]
 #7 [88003b723dd8] dsl_deadlist_close at a0636e19 [zfs]
 #8 [88003b723e10] dsl_dataset_evict at a062d78b [zfs]
 #9 [88003b723e28] taskq_thread at a000f912 [spl]
#10 [88003b723ec8] kthread at 8109726f
#11 [88003b723f50] ret_from_fork at 81614198

In 100% cases where I hit the assertion it was with DMU_OT_BPOBJ dnodes.
Justin thinks that the situation is harmless and the assertion can be removed.
I agree with him.
But on the other hand, I wonder if something could be done in the DSL to avoid
the described situation.
I mean, it seems that bpo_cached_dbuf is a rare (the only?) case where a dbuf
can be held beyond lifetime of  its dnode...

-- 
Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] lzc_receive: add a possibility to take the snapshot name from the stream

[Andriy Gapon]

On 11/08/2015 20:54, Matthew Ahrens wrote:
> On Fri, Aug 7, 2015 at 7:18 AM, Andriy Gapon  <mailto:andriy.ga...@clusterhq.com>> wrote:
> 
> Your proposal seems like a more flexible approach. One thing that bothers 
> me a
> little bit is that every consumer of the new lzc_receive would have to 
> duplicate
> the code for reading / interpreting the BEGIN record.
> 
> Yes, but every consumer might want to interpret the BEGIN record a different 
> way
> (i.e. use the fromsnap name in different ways to determine the name of the
> snapshot to create)
> 
> Possible alternative is to not read the begin record in userland at all 
> and let
> the kernel driver do it. After all, the current lzc_receive just passes 
> the
> record down to the kernel without any interpretation. That would require
> changing the ZFS_IOC_RECV interface, of course.
> 
> But then you'd have to teach the kernel all possible ways that userland might
> want to specify the name to receive based on the fromsnap name in the BEGIN
> record.  Today /sbin/zfs supports at least 4 ways of doing it:
> 
>  - specify full pool/fs@snap
>  - specify pool/fs, get @snap from BEGIN
>  - specify pool/fs -d, append /everything/but/the/poolname@snap from BEGIN
>  - specify pool/fs -e, get /last_elem@snap from BEGIN

Indeed.  So, you have me convinced.

The only potentially unfortunate side effect is that struct drr_begin would
become a part of the API.  We could try to make it opaque to libzfs_core users,
though, by providing a function to read the record from a file descriptor and
then providing a number of accessor functions to extract useful values from the
record.  But not sure if that approach is worth the trouble, it certainly seems
a bit cumbersome.

I'm attaching my current work-in-progress on implementing your suggestion (as I
understood it, possibly incorrectly).  'lzc_receive_basic' is the best name with
which I could come up, but it's probably not the best name.  I had a trouble
picking up a name that would be expressive (of the intent) or descriptive (of
the behavior) enough while still being not very long.

-- 
Andriy Gapon
--- a/usr/src/lib/libzfs_core/common/libzfs_core.h
+++ b/usr/src/lib/libzfs_core/common/libzfs_core.h
@@ -67,6 +67,11 @@ enum lzc_send_flags {
 
 int lzc_send(const char *, const char *, int, enum lzc_send_flags);
-int lzc_receive(const char *, nvlist_t *, const char *, boolean_t, int);
+
+struct drr_begin;
+
+int lzc_receive(const char *, nvlist_t *, const char *, boolean_t, int,
+const struct drr_begin *);
+int lzc_receive_basic(const char *, nvlist_t *, const char *, boolean_t, int);
 int lzc_send_space(const char *, const char *, uint64_t *);
 int lzc_send_progress(const char *, int, uint64_t *);
 
--- a/usr/src/lib/libzfs_core/common/libzfs_core.c
+++ b/usr/src/lib/libzfs_core/common/libzfs_core.c
@@ -625,6 +625,11 @@ recv_read(int fd, void *buf, int ilen)
  * flag will cause the target filesystem to be rolled back or destroyed if
  * necessary to receive.
  *
+ * lzc_receive_basic reads the whole stream while lzc_receive allows the
+ * caller to read the begin record and then to pass it in.  That could be
+ * useful if the caller wants to derive, for example, the snapname or the
+ * origin parameters based on the information contained in the begin record.
+ *
  * Return 0 on success or an errno on failure.
  *
  * Note: this interface does not work on dedup'd streams
@@ -632,7 +637,7 @@ recv_read(int fd, void *buf, int ilen)
  */
 int
 lzc_receive(const char *snapname, nvlist_t *props, const char *origin,
-boolean_t force, int fd)
+boolean_t force, int fd, const struct drr_begin *begin_record)
 {
 	/*
 	 * The receive ioctl is still legacy, so we need to construct our own
@@ -642,7 +647,6 @@ lzc_receive(const char *snapname, nvlist_t *props, const char *origin,
 	char *atp;
 	char *packed = NULL;
 	size_t size;
-	dmu_replay_record_t drr;
 	int error;
 
 	ASSERT3S(g_refcount, >, 0);
@@ -678,10 +682,7 @@ lzc_receive(const char *snapname, nvlist_t *props, const char *origin,
 		(void) strlcpy(zc.zc_string, origin, sizeof (zc.zc_string));
 
 	/* zc_begin_record is non-byteswapped BEGIN record */
-	error = recv_read(fd, &drr, sizeof (drr));
-	if (error != 0)
-		goto out;
-	zc.zc_begin_record = drr.drr_u.drr_begin;
+	zc.zc_begin_record = *begin_record;
 
 	/* zc_cookie is fd to read from */
 	zc.zc_cookie = fd;
@@ -703,6 +704,21 @@ out:
 	return (error);
 }
 
+int
+lzc_receive_basic(const char *snapname, nvlist_t *props, const char *origin,
+boolean_t force, int fd)
+{
+	dmu_replay_record_t drr;
+	int error;
+
+	error = recv_read(fd, &drr, sizeof (drr));
+	if (error == 0) {
+		error = lzc_receive(snapname, props, origin, force, fd,
+		&drr.drr_u.drr_begin);
+	}
+	return (error);
+}
+
 /*
  * Roll back this filesystem or volume to its most rec

Re: [OpenZFS Developer] cache device sharing

[Andriy Gapon]

On 11/09/2015 16:47, George Wilson wrote:
> The ideas was that only one pool could own it so that failover would be
> possible and the cache device would follow that pool. It also would be
> required for persistent l2arc. 
>
> However, it should be possible to create an independent l2arc which never
> fails over but can be added to multiple pools. It would not be a trivial
> implementation but seems doable.

Well, given that the cache device has just a cache of data it can easily follow
its pool by simply discarding the cached data.
Loss of a cache device is harmless even now.

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] cache device sharing

[Andriy Gapon]

On 11/09/2015 16:36, George Wilson wrote:
> I don't think L2ARC can be shared only spare devices can be shared.

But why?
And I do not mean multiple ownership of the cache devices. I mean that if a pool
with a cache device is imported then the cache device could be used by ARC for
the second level caching of data from any pool.

> On Fri, Sep 11, 2015 at 7:23 AM, Andriy Gapon  <mailto:andriy.ga...@clusterhq.com>> wrote:
>
>
> I am curious what was the original reason to not allow sharing of cache
> devices
> (L2ARC) between pools?
> After all, the main ARC is shared for all pools.
> If more control is needed, perhaps we could have a pool property like
> shared-cache-devices (but less verbose) which would tell if data from 
> other
> pools can be cached on this pool's cache devices.
>
> --
> Andriy Gapon
>
> ___
> developer mailing list
> developer@open-zfs.org <mailto:developer@open-zfs.org>
> http://lists.open-zfs.org/mailman/listinfo/developer
>
>


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] cache device sharing

[Andriy Gapon]

I am curious what was the original reason to not allow sharing of cache devices
(L2ARC) between pools?
After all, the main ARC is shared for all pools.
If more control is needed, perhaps we could have a pool property like
shared-cache-devices (but less verbose) which would tell if data from other
pools can be cached on this pool's cache devices.

-- 
Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 232: lzc_receive: add a possibility to take the snapshot name from the stream

[Andriy Gapon]

> On Aug. 4, 2015, 1:14 a.m., Matthew Ahrens wrote:
> > I don't think this is really the direction we want to go with 
> > libzfs_core.c; it increases the complexity of the operation and the 
> > possible error cases.  That said, I understand why you want to do it since 
> > otherwise there's no way to set the name based on the BEGIN record, using 
> > libzfs_core.  Maybe the existing lzc_receive is just the wrong interface, 
> > and it should take the BEGIN record as an argument, forcing the caller to 
> > read and decode it (and decide on the name based on it, if desired).  We 
> > could then provide a convenience routine that works like the current 
> > lzc_receive().
> > 
> > What do you think?

After our converation on the mailing list I am withdrawing this request and I 
will create a new one for the approach that you suggested.


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/232/#review761
-------


On July 15, 2015, 2:36 p.m., Andriy Gapon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/232/
> ---
> 
> (Updated July 15, 2015, 2:36 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List and Matthew Ahrens.
> 
> 
> Bugs: 6051
> https://www.illumos.org/issues/6051
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> lzc_receive: add a possibility to take the snapshot name from the stream
> 
> 
> Diffs
> -
> 
>   usr/src/lib/libzfs_core/common/libzfs_core.c 
> 22af0f4a7a9fd8ab15cc7880233ba51274ce87d8 
> 
> Diff: https://reviews.csiden.org/r/232/diff/
> 
> 
> Testing
> ---
> 
> ZoL, FreeBSD
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 229: account for ashift when choosing buffers to be written to l2arc device

[Andriy Gapon]

> On June 19, 2015, 12:09 a.m., Matthew Ahrens wrote:
> > Looks good to me.  Would like to see review from Saso and George as well.

George, Saso, ping.


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/229/#review754
---


On June 25, 2015, 3:34 p.m., Andriy Gapon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/229/
> ---
> 
> (Updated June 25, 2015, 3:34 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List, Justin Gibbs, George 
> Wilson, Matthew Ahrens, and Saso Kiselkov.
> 
> 
> Bugs: 5219
> https://www.illumos.org/issues/5219
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> **NOTE: this review request supercedes 
> [#112](https://reviews.csiden.org/r/112/).**
> 
> If we don't account for that, then we might end up overwriting disk
> area of buffers that have not been evicted yet, because l2arc_evict
> operates in terms of disk addresses.
> 
> The discrepancy between the write size calculation and the actual increment
> to l2ad_hand was introduced in
> commit 
> [aad02571bc59671aa3103bb070ae365f531b0b62](https://github.com/illumos/illumos-gate/commit/aad02571bc59671aa3103bb070ae365f531b0b62)
> 
> The change that introduced l2ad_hand alignment was almost correct
> as the write size was accumulated as a sum of rounded buffer sizes.
> See commit 
> [e14bb3258d05c1b1077e2db7cf77088924e56919](https://github.com/illumos/illumos-gate/commit/e14bb3258d05c1b1077e2db7cf77088924e56919)
> 
> Also, we now consistently use asize / a_sz for the allocated size and
> psize / p_sz for the physical size.
> The latter accounts for a possible size reduction because of the compression,
> whereas the former accounts for a possible subsequent size expansion
> because of the alignment requirements.
> 
> The code still assumes that either underlying storage subsystems or
> hardware is able to do read-modify-write when an L2ARC buffer size is
> not a multiple of a disk's block size.  This is true for 4KB sector disks
> that provide 512B sector emulation, but may not be true in general.
> In other words, we currently do not have any code to make sure that
> an L2ARC buffer, whether compressed or not, which is used for physical I/O
> has a suitable size.
> 
> Note that currently the cache device utilization is calculated based
> on the physical size, not the allocated size.  The same applies to l2_asize
> kstat. That is wrong, but this commit does not fix that.
> The accounting problem was introduced partially in
> commit 
> [aad02571bc59671aa3103bb070ae365f531b0b62](https://github.com/illumos/illumos-gate/commit/aad02571bc59671aa3103bb070ae365f531b0b62)
> and partially in 
> [3038a2b421b40dc5ac11cd88423696618584f85a](https://github.com/illumos/illumos-gate/commit/3038a2b421b40dc5ac11cd88423696618584f85a)
> (accounting became consistent but in favour of the wrong size).
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/arc.c 09d2e1dd8fdf6648d863d4c036c16b3f7981dbf5 
> 
> Diff: https://reviews.csiden.org/r/229/diff/
> 
> 
> Testing
> ---
> 
> FreeBSD and Linux.
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] lzc_receive: add a possibility to take the snapshot name from the stream

[Andriy Gapon]

Your proposal seems like a more flexible approach. One thing that bothers me a
little bit is that every consumer of the new lzc_receive would have to duplicate
the code for reading / interpreting the BEGIN record.

Possible alternative is to not read the begin record in userland at all and let
the kernel driver do it. After all, the current lzc_receive just passes the
record down to the kernel without any interpretation. That would require
changing the ZFS_IOC_RECV interface, of course.

On 04/08/2015 02:10, Matthew Ahrens wrote:
> I don't think this is really the direction we want to go with libzfs_core; it
> increases the complexity of the operation and the possible error cases. That
> said, I understand why you want to do it since otherwise there's no way to set
> the name based on the BEGIN record, using libzfs_core. Maybe the existing
> lzc_receive is just the wrong interface, and it should take the BEGIN record
> as an argument, forcing the caller to read and decode it (and decide on the
> name based on it, if desired). We could then provide a convenience routine
> that works like the current lzc_receive().
>
> What do you think?
>
>
> --matt
>
> On Fri, Jul 3, 2015 at 10:46 AM, Andriy Gapon  <mailto:andriy.ga...@clusterhq.com>> wrote:
>
>
> Currently lzc_receive() requires that its 'snapname' argument is a
> snapshot name
> (contains '@').  zfs receive allows to specify just a dataset name and
> would try
> to deduce the snapshot name from the stream.  I propose to allow 
> lzc_receive()
> to do the same.  That is quite easy to achieve, it requires only a small
> amount
> of logic, it does not require any additional system calls or any
> additional data
> from the stream.
> Below is a proof of concept patch that seems to work.
>
> --- lib/libzfs_core/common/libzfs_core.c
> +++ lib/libzfs_core/common/libzfs_core.c
> @@ -721,9 +721,8 @@ lzc_receive
> /* zc_name is name of containing filesystem */
> (void) strlcpy(zc.zc_name, snapname, sizeof (zc.zc_name));
> atp = strchr(zc.zc_name, '@');
> -   if (atp == NULL)
> -   return (EINVAL);
> -   *atp = '\0';
> +   if (atp != NULL)
> +   *atp = '\0';
>
> /* if the fs does not exist, try its parent. */
> if (!lzc_exists(zc.zc_name)) {
> @@ -734,9 +733,6 @@ lzc_receive
>
> }
>
> -   /* zc_value is full name of the snapshot to create */
> -   (void) strlcpy(zc.zc_value, snapname, sizeof (zc.zc_value));
> -
> if (props != NULL) {
> /* zc_nvlist_src is props to set */
> packed = fnvlist_pack(props, &size);
> @@ -754,6 +750,20 @@ lzc_receive
> goto out;
> zc.zc_begin_record = drr.drr_u.drr_begin;
>
> +   /* zc_value is full name of the snapshot to create */
> +   (void) strlcpy(zc.zc_value, snapname, sizeof (zc.zc_value));
> +
> +   /* if snapshot name is not provided try to take it from the 
> stream */
> +   atp = strchr(zc.zc_value, '@');
> +   if (atp == NULL) {
> +   atp = strchr(zc.zc_begin_record.drr_toname, '@');
> +   if (atp == NULL)
> +   return (EINVAL);
> +       if (strlen(zc.zc_value) + strlen(atp) >= 
> sizeof(zc.zc_value))
> +   return (ENAMETOOLONG);
> +   strcat(zc.zc_value, atp);
> +   }
> +
>     /* zc_cookie is fd to read from */
> zc.zc_cookie = fd;
>
>
> --
> Andriy Gapon
> ___
> developer mailing list
> developer@open-zfs.org <mailto:developer@open-zfs.org>
> http://lists.open-zfs.org/mailman/listinfo/developer
>
>


-- 
Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] zfs_ioc_snapshot_list_next: ENOENT -> ESRCH

[Andriy Gapon]

Yes, this is my intention.  But I am a little bit worried about breaking
something that might depend on the existing behavior.
So, I guess, that change would require more testing than I typically did.

On 04/08/2015 02:14, Matthew Ahrens wrote:
> I would guess that ENOENT was supposed to mean some different error, but I
> don't see what.  I'd be open to changing this if that's your thinking.
>
> --matt
>
> On Thu, Jul 2, 2015 at 11:02 AM, Andriy Gapon  <mailto:andriy.ga...@clusterhq.com>> wrote:
>
>
> zfs_ioc_snapshot_list_next() converts ENOENT returned from
> dmu_objset_hold(zc->zc_name) to ESRCH.
> So, if zc_name names a dataset that does not exists, then to the userland 
> it
> would instead appear as if the dataset has not snapshots.
> It seems that the code behaves that way from the very start.
>
>     I wonder what is the reason for that?
> Thanks!
> --
> Andriy Gapon
>
>

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] lzc_receive: add a possibility to take the snapshot name from the stream

[Andriy Gapon]

On 03/07/2015 20:46, Andriy Gapon wrote:
> 
> Currently lzc_receive() requires that its 'snapname' argument is a snapshot 
> name
> (contains '@').  zfs receive allows to specify just a dataset name and would 
> try
> to deduce the snapshot name from the stream.  I propose to allow lzc_receive()
> to do the same.  That is quite easy to achieve, it requires only a small 
> amount
> of logic, it does not require any additional system calls or any additional 
> data
> from the stream.

Forgot to add: the benefit is that the new behavior would allow to keep the
snapshot names the same between the sender and receiver at zero cost, without a
need to pass the names out of band.

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] lzc_receive: add a possibility to take the snapshot name from the stream

[Andriy Gapon]

Currently lzc_receive() requires that its 'snapname' argument is a snapshot name
(contains '@').  zfs receive allows to specify just a dataset name and would try
to deduce the snapshot name from the stream.  I propose to allow lzc_receive()
to do the same.  That is quite easy to achieve, it requires only a small amount
of logic, it does not require any additional system calls or any additional data
from the stream.
Below is a proof of concept patch that seems to work.

--- lib/libzfs_core/common/libzfs_core.c
+++ lib/libzfs_core/common/libzfs_core.c
@@ -721,9 +721,8 @@ lzc_receive
/* zc_name is name of containing filesystem */
(void) strlcpy(zc.zc_name, snapname, sizeof (zc.zc_name));
atp = strchr(zc.zc_name, '@');
-   if (atp == NULL)
-   return (EINVAL);
-   *atp = '\0';
+   if (atp != NULL)
+   *atp = '\0';

/* if the fs does not exist, try its parent. */
if (!lzc_exists(zc.zc_name)) {
@@ -734,9 +733,6 @@ lzc_receive

}

-   /* zc_value is full name of the snapshot to create */
-   (void) strlcpy(zc.zc_value, snapname, sizeof (zc.zc_value));
-
if (props != NULL) {
/* zc_nvlist_src is props to set */
packed = fnvlist_pack(props, &size);
@@ -754,6 +750,20 @@ lzc_receive
goto out;
zc.zc_begin_record = drr.drr_u.drr_begin;

+   /* zc_value is full name of the snapshot to create */
+   (void) strlcpy(zc.zc_value, snapname, sizeof (zc.zc_value));
+
+   /* if snapshot name is not provided try to take it from the stream */
+   atp = strchr(zc.zc_value, '@');
+   if (atp == NULL) {
+   atp = strchr(zc.zc_begin_record.drr_toname, '@');
+   if (atp == NULL)
+   return (EINVAL);
+   if (strlen(zc.zc_value) + strlen(atp) >= sizeof(zc.zc_value))
+   return (ENAMETOOLONG);
+   strcat(zc.zc_value, atp);
+   }
+
    /* zc_cookie is fd to read from */
zc.zc_cookie = fd;


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] listing things

[Andriy Gapon]

Currently there are two types of interfaces for listing datasets / snapshots /
holds.  ZFS_IOC_DATASET_LIST_NEXT / ZFS_IOC_SNAPSHOT_LIST_NEXT return strictly a
single entity.  ZFS_IOC_GET_HOLDS returns strictly all (or none, if they don't
fit) holds .

It seems that the both approaches are not ideal from the points of view of the
performance and the memory requirements.

I think that a more balanced and universal approach could be to get entries via
the iteration but with getting more than one entry per an iteration if the
allocated memory allows that.

Just thinking out loud...
-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] zfs_ioc_snapshot_list_next: ENOENT -> ESRCH

[Andriy Gapon]

zfs_ioc_snapshot_list_next() converts ENOENT returned from
dmu_objset_hold(zc->zc_name) to ESRCH.
So, if zc_name names a dataset that does not exists, then to the userland it
would instead appear as if the dataset has not snapshots.
It seems that the code behaves that way from the very start.

I wonder what is the reason for that?
Thanks!
-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] Interesting ZoL commits

[Andriy Gapon]

There are some interesting ZoL commits.

These look like important bug fixes, perhaps someone would be interested in
reviewing them:

dbuf_free_range() overzealously frees dbufs

https://github.com/zfsonlinux/zfs/commit/58806b4cdc32e6f4e4a214cfba3b62a24efb34b7

dmu_objset_userquota_get_ids uses dn_bonus unsafely

https://github.com/zfsonlinux/zfs/commit/5f8e1e850522ee5cd37366427da4b4101a71c8a8

This one also seems to be a useful bugfix:

Relax restriction on zfs_ioc_next_obj() iteration

https://github.com/zfsonlinux/zfs/commit/7290cd3c4ed19fb3f75b8133db2e36afcdd24beb

And I find the following feature to be a nice addition:

Zdb should be able to open the root dataset

https://github.com/zfsonlinux/zfs/commit/b1b85c8772ed28d2c8227e6d32905740817ae2c3

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] Review Request 229: account for ashift when choosing buffers to be written to l2arc device

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/229/
---

Review request for OpenZFS Developer Mailing List, Justin Gibbs, Matthew 
Ahrens, and Saso Kiselkov.


Bugs: 5219
https://www.illumos.org/projects/illumos-gate//issues/5219


Repository: illumos-gate


Description
---

**NOTE: this review request supercedes 
[#112](https://reviews.csiden.org/r/112/).**

If we don't account for that, then we might end up overwriting disk
area of buffers that have not been evicted yet, because l2arc_evict
operates in terms of disk addresses.

The discrepancy between the write size calculation and the actual increment
to l2ad_hand was introduced in
commit 
[aad02571bc59671aa3103bb070ae365f531b0b62](https://github.com/illumos/illumos-gate/commit/aad02571bc59671aa3103bb070ae365f531b0b62)

The change that introduced l2ad_hand alignment was almost correct
as the write size was accumulated as a sum of rounded buffer sizes.
See commit 
[e14bb3258d05c1b1077e2db7cf77088924e56919](https://github.com/illumos/illumos-gate/commit/e14bb3258d05c1b1077e2db7cf77088924e56919)

Also, we now consistently use asize / a_sz for the allocated size and
psize / p_sz for the physical size.
The latter accounts for a possible size reduction because of the compression,
whereas the former accounts for a possible subsequent size expansion
because of the alignment requirements.

The code still assumes that either underlying storage subsystems or
hardware is able to do read-modify-write when an L2ARC buffer size is
not a multiple of a disk's block size.  This is true for 4KB sector disks
that provide 512B sector emulation, but may not be true in general.
In other words, we currently do not have any code to make sure that
an L2ARC buffer, whether compressed or not, which is used for physical I/O
has a suitable size.

Note that currently the cache device utilization is calculated based
on the physical size, not the allocated size.  The same applies to l2_asize
kstat. That is wrong, but this commit does not fix that.
The accounting problem was introduced partially in
commit 
[aad02571bc59671aa3103bb070ae365f531b0b62](https://github.com/illumos/illumos-gate/commit/aad02571bc59671aa3103bb070ae365f531b0b62)
and partially in 
[3038a2b421b40dc5ac11cd88423696618584f85a](https://github.com/illumos/illumos-gate/commit/3038a2b421b40dc5ac11cd88423696618584f85a)
(accounting became consistent but in favour of the wrong size).


Diffs
-

  usr/src/uts/common/fs/zfs/arc.c 09d2e1dd8fdf6648d863d4c036c16b3f7981dbf5 

Diff: https://reviews.csiden.org/r/229/diff/


Testing
---

FreeBSD only.


Thanks,

Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 112: account for ashift when choosing buffers to be written to l2arc device

[Andriy Gapon]

> On April 9, 2015, 4:04 a.m., Bayard Bell wrote:
> > usr/src/uts/common/fs/zfs/arc.c, line 4740
> > 
> >
> > Nit: generally following Matt's general philosophy of helping the 
> > compiler be our friend for safety, if we don't use write_psize until this 
> > loop, might we defer initialization, if not declaration, until this point?

this particular function seems to follow the convention of placing all variable 
declarations before actual code, so I am not if it's really worthwhile to make 
an exception for a single variable


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/112/#review649
---


On Oct. 8, 2014, 4:57 p.m., Justin Gibbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/112/
> ---
> 
> (Updated Oct. 8, 2014, 4:57 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List, Matthew Ahrens and Saso 
> Kiselkov.
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> If we don't account for that, then we might end up overwriting disk
> area of buffers that have not been evicted yet, because l2arc_evict
> operates in terms of disk addresses.
> 
> The discrepancy between the write size calculation and the actual increment
> to l2ad_hand was introduced in
> commit e14bb3258d05c1b1077e2db7cf77088924e56919
> 
> Also, consistently use asize / a_sz for the allocated size, psize / p_sz
> for the physical size.  Where the latter accounts for possible size
> reduction because of compression, whereas the former accounts for possible
> size expansion because of alignment requirements.
> 
> The code still assumes that either underlying storage subsystems or
> hardware is able to do read-modify-write when an L2ARC buffer size is
> not a multiple of a disk's block size.  This is true for 4KB sector disks
> that provide 512B sector emulation, but may not be true in general.
> In other words, we currently do not have any code to make sure that
> an L2ARC buffer, whether compressed or not, which is used for physical I/O
> has a suitable size.
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/arc.c 69d16af3b669458e3937fe0c6a4d91755bc6e2a7 
> 
> Diff: https://reviews.csiden.org/r/112/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Justin Gibbs
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 228: 5984 zfs clone should not mount the clone if canmount == noauto

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/228/
---

(Updated June 7, 2015, 11:27 a.m.)


Review request for OpenZFS Developer Mailing List.


Changes
---

Apparently I forgot to test `zfs clone -o canmount=on` :-\


Bugs: 5984
https://www.illumos.org/projects/illumos-gate//issues/5984


Repository: illumos-gate


Description
---

The change is already implemeted in ZoL.

My main rationale for doing things differently from what was done in ZoL was to 
avoid too deep nesting and the resulting line wrapping ugliness.
Additionally, introduction of `should_auto_mount()` should reduce code 
duplication `zfs_do_create()` and `zfs_do_clone()`.
Lastly, this change adds diagnostic messages for mounting / sharing failures in 
`zfs_do_clone()` similar to those in `zfs_do_create()`.


Diffs (updated)
-

  usr/src/cmd/zfs/zfs_main.c ff04e3a1c6e32918aa6a93aa184942f5511e3bec 

Diff: https://reviews.csiden.org/r/228/diff/


Testing
---

`zfs clone -o canmount=noauto` on FreeBSD.


Thanks,

Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] automatically mount a clone iff canmount == on

[Andriy Gapon]

On 03/06/2015 18:54, Andriy Gapon wrote:
> 
> I found line wrapping that resulted from deep nesting to be quite unpleasing
> aesthetically, so I did a little bit more work than a minimal patch would 
> require:
> https://gist.github.com/avg-I/d38f670b8a86613fa458
> 
> Could you please take a look?
> I can open an issue and create a review request on the reviewboard.

Forgot to add that I also added (copied from zfs_do_create) some diagnostics for
a mount / share failure.

> On 29/05/2015 01:13, Matthew Ahrens wrote:
>> Seems like it will work.  Added a comment to that review:
>>
>> type!=VOLUME and prop_valid_for_type(CANMOUNT) seem like they are essentially
>> checking the same thing (is this a filesystem).  It would be better to make 
>> that
>> check once.  e.g.:
>>
>> if (prop_valid_for_type(CANMOUNT)) {
>>   if (prop_get_init(CANMOUNT) == ON)) {
>> zfs_mount()
>>   }
>> }
>>
>> On Mon, May 25, 2015 at 11:06 AM, Andriy Gapon > <mailto:andriy.ga...@clusterhq.com>> wrote:
>>
>>
>> ZoL has the following change:
>> 
>> https://github.com/FransUrbo/zfs/commit/dd0e0e69f5b1c83bf2895ac00a0b83af77473175
>> https://github.com/zfsonlinux/zfs/issues/2241
>>
>> I think that that change is correct, because creating a clone does not 
>> imply a
>> wish to mount it.
>> What do you think?
>>
>> --
>> Andriy Gapon
>> _______
>> developer mailing list
>> developer@open-zfs.org <mailto:developer@open-zfs.org>
>> http://lists.open-zfs.org/mailman/listinfo/developer
>>
>>
> 
> 


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] automatically mount a clone iff canmount == on

[Andriy Gapon]

I found line wrapping that resulted from deep nesting to be quite unpleasing
aesthetically, so I did a little bit more work than a minimal patch would 
require:
https://gist.github.com/avg-I/d38f670b8a86613fa458

Could you please take a look?
I can open an issue and create a review request on the reviewboard.

On 29/05/2015 01:13, Matthew Ahrens wrote:
> Seems like it will work.  Added a comment to that review:
> 
> type!=VOLUME and prop_valid_for_type(CANMOUNT) seem like they are essentially
> checking the same thing (is this a filesystem).  It would be better to make 
> that
> check once.  e.g.:
> 
> if (prop_valid_for_type(CANMOUNT)) {
>   if (prop_get_init(CANMOUNT) == ON)) {
> zfs_mount()
>   }
> }
> 
> On Mon, May 25, 2015 at 11:06 AM, Andriy Gapon  <mailto:andriy.ga...@clusterhq.com>> wrote:
> 
> 
> ZoL has the following change:
> 
> https://github.com/FransUrbo/zfs/commit/dd0e0e69f5b1c83bf2895ac00a0b83af77473175
> https://github.com/zfsonlinux/zfs/issues/2241
> 
> I think that that change is correct, because creating a clone does not 
> imply a
> wish to mount it.
> What do you think?
> 
> --
> Andriy Gapon
> ___
> developer mailing list
> developer@open-zfs.org <mailto:developer@open-zfs.org>
> http://lists.open-zfs.org/mailman/listinfo/developer
> 
> 


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Temporary in-core DSL Pool metadata snapshots

[Andriy Gapon]

On 29/05/2015 20:06, Matthew Ahrens wrote:
> On Fri, May 29, 2015 at 10:00 AM, Richard Yao  <mailto:richard@clusterhq.com>> wrote:
> 
> It seems that no one is particularly interested in a strict point in time
> version, 
> 
> To be clear, I'm not saying that I'm uninterested in that, just that it seems
> hard/complicated to achieve in combination with other requirements
> (specifically, not needing to have the entire state in memory at once).  (And 
> I
> don't know that anyone else has weighed in.)

Additionally, what good is a consistent snapshot of the children if by the time
you get it any number of changes might have happened.  It's not like we are
preventing the changes from happening while we look at or iterate the list.

> so I'll drop that design constraint, document the issue and hope userland
> programmers relying on the output will actually read the documentation
> carefully so that they do not make the naive assumption that I imagine 
> that
> they will make.
> 
> 
> FWIW, userland programmers already need to be aware of these issues when
> iterating over directory entries (e.g. if a file/directory is concurrently
> renamed, "find" can list it 0, 1, or 2 times).  Concurrent renames are pretty
> difficult to deal with.


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 225: 5946 zfs_ioc_space_snaps: check that firstsnap and lastsnap are snapshots

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/225/
---

(Updated May 27, 2015, 1:43 p.m.)


Review request for OpenZFS Developer Mailing List.


Changes
---

Oops... catch up with dsl_dataset_is_snapshot => ds_is_snapshot.
That change is not in FreeBSD yet.


Bugs: 5945 and 5946
https://www.illumos.org/projects/illumos-gate//issues/5945
https://www.illumos.org/projects/illumos-gate//issues/5946


Repository: illumos-gate


Description
---

5946 zfs_ioc_space_snaps: check that firstsnap and lastsnap are snapshots
otherwise an assertion in dsl_dataset_space_wouldfree() could be triggered.

5945 dmu_send_estimate: verify that fromds is a snapshot
otherwise an assertion in dsl_dataset_is_before() could be triggered.


Diffs (updated)
-

  usr/src/uts/common/fs/zfs/zfs_ioctl.c 
692b49611d2d2085805db5fb3ed258db1a76e207 
  usr/src/uts/common/fs/zfs/dmu_send.c e4abdc325428189825507b76570b9e89c2578868 

Diff: https://reviews.csiden.org/r/225/diff/


Testing
---

ZFS/FreeBSD with DEBUG-enabled ZFS code.


Thanks,

Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] Review Request 225: 5946 zfs_ioc_space_snaps: check that firstsnap and lastsnap are snapshots

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/225/
---

Review request for OpenZFS Developer Mailing List.


Bugs: 5945 and 5946
https://www.illumos.org/projects/illumos-gate//issues/5945
https://www.illumos.org/projects/illumos-gate//issues/5946


Repository: illumos-gate


Description
---

5946 zfs_ioc_space_snaps: check that firstsnap and lastsnap are snapshots
otherwise an assertion in dsl_dataset_space_wouldfree() could be triggered.

5945 dmu_send_estimate: verify that fromds is a snapshot
otherwise an assertion in dsl_dataset_is_before() could be triggered.


Diffs
-

  usr/src/uts/common/fs/zfs/zfs_ioctl.c 
692b49611d2d2085805db5fb3ed258db1a76e207 
  usr/src/uts/common/fs/zfs/dmu_send.c e4abdc325428189825507b76570b9e89c2578868 

Diff: https://reviews.csiden.org/r/225/diff/


Testing
---

ZFS/FreeBSD with DEBUG-enabled ZFS code.


Thanks,

Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] automatically mount a clone iff canmount == on

[Andriy Gapon]

ZoL has the following change:
https://github.com/FransUrbo/zfs/commit/dd0e0e69f5b1c83bf2895ac00a0b83af77473175
https://github.com/zfsonlinux/zfs/issues/2241

I think that that change is correct, because creating a clone does not imply a
wish to mount it.
What do you think?

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] receiving non-incremental stream into existing fs

[Andriy Gapon]

On 06/05/2015 02:29, Matthew Ahrens wrote:
> 
> 
> On Fri, Apr 24, 2015 at 6:29 AM, Andriy Gapon  <mailto:andriy.ga...@clusterhq.com>> wrote:
> 
> 
> BTW, there is another boundary case for zfs_ioc_recv().
> If the desired destination snapshot already exists then EEXIST is always 
> return.
>  On the one hand, this totally makes sense.  On the other hand, if we are 
> doing
> force-receiving and the conflicting local snapshot would be destroyed if 
> it had
> a different name, then maybe we should just destroy it and reuse the name?
> 
> In other words, if the snapshot already exists then:
> - if !force -> EEXIST
> - if force and the snapshot is later than drba_snapobj, then destroy the
> snapshot and proceed to receive the stream
> - otherwise -> EEXIST
> 
> 
> Sure, if you can make that work, the behavior sounds reasonable.

I tried to make this work and it was relatively easy to do in
recv_begin_check_existing_impl().  But dmu_recv_end_check() needs to be patched
up as well, otherwise dsl_dataset_snapshot_check_impl() would fail with EEXIST.
And so far I couldn't figure out a good way to do that.
Not sure if I want to spend much time on this at the moment.

In any case, thank you for the feedback !
-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 220: 5912 full stream can not be force-received into a dataset if it has a snapshot

[Andriy Gapon]

> On May 8, 2015, 5:19 a.m., Matthew Ahrens wrote:
> > I don't see any problem with this.  Did you test it on an old pool that 
> > doesn't have $ORIGIN@$ORIGIN?

In fact, I didn't.  Is there a way to create an older version pool on a modern 
system?  Some hidden option perhaps?


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/220/#review690
---


On May 6, 2015, 3:47 p.m., Andriy Gapon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/220/
> ---
> 
> (Updated May 6, 2015, 3:47 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List, Christopher Siden and 
> Matthew Ahrens.
> 
> 
> Bugs: 5912
> https://www.illumos.org/projects/illumos-gate//issues/5912
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> Fixes for a full stream received into an existing dataset:
> 
> - this should fail early unless the force flag is set
> - EEXIST (dataset exists) seems like a logical error code in the above case
> - if the force flag is set then any local modifications including snapshots
>   should be undone
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/dmu_send.c 
> e4abdc325428189825507b76570b9e89c2578868 
> 
> Diff: https://reviews.csiden.org/r/220/diff/
> 
> 
> Testing
> ---
> 
> OpenZFS / FreeBSD, using lzc_receive()
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] DS_FLAG_NOPROMOTE

[Andriy Gapon]

On 06/05/2015 18:50, Matthew Ahrens wrote:
> After many years, I finally have a potential use case for this, so I'd like to
> keep it in.  I was thinking of proposing this project at a future hackathon
> (This year's OpenZFS DevSummit?  Look for an announcement coming soon :-)  To
> summarize:
> 
> To minimize space used by metadata, and to increase performance, we could
> implement "zfs clone --nopromote". Clones created this way would not be
> promotable (i.e. "zfs promote " would give an error).
> 
> In return, the DSL would not need to track (in the deadlists) blocks that were
> born before the origin.

Sounds cool! Thanks for the heads-up.

> On Wed, May 6, 2015 at 5:27 AM, Andriy Gapon  <mailto:andriy.ga...@clusterhq.com>> wrote:
> 
> 
> DS_FLAG_NOPROMOTE seems to be a read-only flag. That is, it's checked in 
> two
> places, but it is never set.
> Is this some left-over that can be cleaned up?
> 
> --
> Andriy Gapon
> ___
> developer mailing list
> developer@open-zfs.org <mailto:developer@open-zfs.org>
> http://lists.open-zfs.org/mailman/listinfo/developer
> 
> 


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] DS_FLAG_NOPROMOTE

[Andriy Gapon]

DS_FLAG_NOPROMOTE seems to be a read-only flag. That is, it's checked in two
places, but it is never set.
Is this some left-over that can be cleaned up?

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] receiving non-incremental stream into existing fs

[Andriy Gapon]

On 06/05/2015 02:28, Matthew Ahrens wrote:
> That looks good to me, assuming the rest of the code handles it correctly
> (destroying the unneeded snaps - even on pools from before $ORIGIN).  It would
> be great to add a test case for this to the test suite.  EEXIST seems 
> reasonable.
> 

Thank you!
BTW, I have a test case for this scenario in our lzc python wrapper.
Perhaps it would be worthwhile converting some of our tests to ztest test, but
maybe that won't be necessary once we publicly release the code and the tests.

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] receiving non-incremental stream into existing fs

[Andriy Gapon]

[ping]

I would like to get a preliminary review for the change suggested below .
Or should I rather open an issue and submit the change for a review via the
reviewboard?

Thanks!

On 24/04/2015 14:38, Andriy Gapon wrote:
> On 24/04/2015 02:25, Matthew Ahrens wrote:
>> I think that's a bug / not-fully-implemented-feature.  "zfs receive -F" 
>> should
>> be allowed to destroy whatever it takes to do the receive that you requested.
>>  (Also the manpage should be updated to reflect this.)
> 
> 
> To sum up what we've been discussing: does the following patch look like a
> reasonable change:
> 
> --- dmu_send.c
> +++ dmu_send.c
> @@ -982,10 +982,12 @@ recv_begin_check_existing_impl()
> 
>   dsl_dataset_rele(snap, FTAG);
>   } else {
> - /* if full, most recent snapshot must be $ORIGIN */
> - if (ds->ds_phys->ds_prev_snap_txg >= TXG_INITIAL)
> - return (SET_ERROR(ENODEV));
> - drba->drba_snapobj = ds->ds_phys->ds_prev_snap_obj;
> + /* if full, then must be forced */
> + if (!drba->drba_cookie->drc_force)
> + return (SET_ERROR(EEXIST));
> + /* start from $ORIGIN@$ORIGIN, if supported */
> + drba->drba_snapobj = dp->dp_origin_snap != NULL ?
> + dp->dp_origin_snap->ds_object : 0;
>   }
> 
>   return (0);
> 
> I do not have a strong opinion about the return code.  EEXIST makes sense to 
> me,
> but possibly some other code like ENODEV or ETXTBUSY makes even more sense.
> 


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] receiving incremental clone stream without creating clone

[Andriy Gapon]

Perhaps the following would be an unintended, unorthodox use of zfs send + recv,
but I do not immediately see why it shouldn't work (which it doesn't).
Let's assume that we have filesystem fs1 with 2 snapshots, fs1@fromsnap and
fs@bp.  Let's then clone fs@bp to fs2 and then later make another snapshot
fs2@tosnap.  After that we produce a full send stream for fs1@fromsnap and
receive it to a new filesystem dest as dest@fromsnap.  Finally, we produce an
incremental stream fs1@fromsnap -> fs2@tosnap and receive to dest as 
dest@tosnap.

My expectation is that the above should succeed and we should get dest
filesystem equivalent to fs2@tosnap.  After all, if we produced a full stream
for fs2@tosnap and received it to dest2, then that operation would succeed.

But, as far as I can see, the current code insists that an incremental stream
that crossed a branch point on the sending side (that is, its start snapshot is
in one filesystem and its end snapshot is in the filesystem's clone) must create
a new filesystem (a clone) on the receiving side as well.

I wonder where that requirement comes from.
What could go wrong if we allowed the incremental stream with the clone flag to
be received into an existing filesystem (provided that all other things 
matched) ?

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] Review Request 206: 5870 dmu_recv_end_check() leaks origin_head hold if error happens in drc_force branch

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/206/
---

Review request for OpenZFS Developer Mailing List and Christopher Siden.


Bugs: 5870
https://www.illumos.org/projects/illumos-gate//issues/5870


Repository: illumos-gate


Description
---

The leak may happen if !drc_newfs && drc_force and there is an error
iterating through snapshots or any of snapshot checks fails.


Diffs
-

  usr/src/uts/common/fs/zfs/dmu_send.c c4575f8f6dc2453d8483a7e566dafdce2f3f49ff 

Diff: https://reviews.csiden.org/r/206/diff/


Testing
---

FreeBSD-only, using libzfs_core.


Thanks,

Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] receiving non-incremental stream into existing fs

[Andriy Gapon]

BTW, there is another boundary case for zfs_ioc_recv().
If the desired destination snapshot already exists then EEXIST is always return.
 On the one hand, this totally makes sense.  On the other hand, if we are doing
force-receiving and the conflicting local snapshot would be destroyed if it had
a different name, then maybe we should just destroy it and reuse the name?

In other words, if the snapshot already exists then:
- if !force -> EEXIST
- if force and the snapshot is later than drba_snapobj, then destroy the
snapshot and proceed to receive the stream
- otherwise -> EEXIST


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] zfs_rezget() succeeds even when IFTOVT(z_mode) changes

[Andriy Gapon]

It is highly improbable but still quite possible that two objects in different
datasets are created with the same object numbers and in transaction groups with
the same IDs. znodes corresponding to those objects would have the same z_id and
z_gen, but their other attributes may be quite different.
zfs recv -F may replace one of such objects with the other and zfs_rezget()
would be successful in such a case.
As a result file properties recorded in the replaced object's vnode may no
longer match the received object's properties (the znode is refreshed).  One of
the obvious cached properties is the vnode's type recorded in v_type.  Maybe
there are others.

I think that there could quite unexpected consequences if a regular file is
replaced with a directory (or vice versa) and zfs_rezget() is successful.
So, in addition to the existing checks it is better to compare v_type with
(refreshed) z_mode.  Although, I am not sure what would be better to do if those
values do not match: update v_type or fail zfs_rezget(), so that a new
vnode-znode pair could be created.  The latter seems like a safer option.

P.S.
FreeBSD already has the defensive code:
http://bxr.su/FreeBSD/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c#1330

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] z_root change after recv -F

[Andriy Gapon]

On 24/04/2015 03:38, Matthew Ahrens wrote:
> I don't think that we should assume that it can't change.  In practice it 
> might
> always be the same (for now) but I don't think that was the design intent.


I have opened an issue to keep track of this potential problem:
https://www.illumos.org/issues/5867

> On Thu, Apr 23, 2015 at 5:00 AM, Andriy Gapon  
> wrote:
> 
> 
> A semi-theoretical question: can z_root - or, to be correct, ZFS_ROOT_OBJ 
> -
> value change after a filesystem has its content replaced by zfs recv -F 
> of a
> full stream?
> It seems that that case wouldn't be handled correctly if the filesystem is
> mounted and goes through zfs_suspend_fs() + zfs_resume_fs().
> 
> The reason I am asking is that I noticed this:
> $ zdb - pond/var 1
> 
> casesensitivity = 0
> SA_ATTRS = 4
> normalization = 0
> VERSION = 5
> ROOT = 3
> utf8only = 0
> DELETE_QUEUE = 2
> 
> $ zdb - pond/var/crash 1
> 
> ROOT = 4
> utf8only = 0
> casesensitivity = 0
> VERSION = 5
> SA_ATTRS = 2
> SHARES = 7
> DELETE_QUEUE = 3
> normalization = 0
> 
> This may have very well happened because of my playing with the code, but 
> in
> general the root object does not have a fixed ID.
> 
> --
> Andriy Gapon
> ___
> developer mailing list
> developer@open-zfs.org <mailto:developer@open-zfs.org>
> http://lists.open-zfs.org/mailman/listinfo/developer
> 
> 


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] receiving non-incremental stream into existing fs

[Andriy Gapon]

On 24/04/2015 02:25, Matthew Ahrens wrote:
> I think that's a bug / not-fully-implemented-feature.  "zfs receive -F" should
> be allowed to destroy whatever it takes to do the receive that you requested.
>  (Also the manpage should be updated to reflect this.)


To sum up what we've been discussing: does the following patch look like a
reasonable change:

--- dmu_send.c
+++ dmu_send.c
@@ -982,10 +982,12 @@ recv_begin_check_existing_impl()

dsl_dataset_rele(snap, FTAG);
} else {
-   /* if full, most recent snapshot must be $ORIGIN */
-   if (ds->ds_phys->ds_prev_snap_txg >= TXG_INITIAL)
-   return (SET_ERROR(ENODEV));
-   drba->drba_snapobj = ds->ds_phys->ds_prev_snap_obj;
+   /* if full, then must be forced */
+   if (!drba->drba_cookie->drc_force)
+   return (SET_ERROR(EEXIST));
+   /* start from $ORIGIN@$ORIGIN, if supported */
+   drba->drba_snapobj = dp->dp_origin_snap != NULL ?
+   dp->dp_origin_snap->ds_object : 0;
}

return (0);

I do not have a strong opinion about the return code.  EEXIST makes sense to me,
but possibly some other code like ENODEV or ETXTBUSY makes even more sense.

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] receiving non-incremental stream into existing fs

[Andriy Gapon]

On 24/04/2015 00:00, Matthew Ahrens wrote:
> On Thu, Apr 23, 2015 at 1:24 PM, Andriy Gapon  
> wrote:
> It seems that any filesystem is considered modified when compared to 
> $ORIGIN.
> And ds_prev of a filesystem without any real snapshots points to $ORIGIN.
> Or is it $ORIGIN@$ORIGIN actually?
> 
> 
> Yes.  And the filesystems actually *are* modified, compared to the
> $ORIGIN@$ORIGIN.  They have been initialized by the ZPL (or zvol), e.g. to
> contain the root directory.  Whereas the $ORIGIN@$ORIGIN is truly empty from 
> the
> DSL/DMU point of view -- it has no blocks.

This totally makes sense - thank you for the explanation!

I see another curious / not-entirely-expected thing about zfs receive.
If a destination filesystem has been modified since the last received snapshot -
if any, and it also has at least one newer local snapshots, and a stream is
received into that filesystem, and the force flag is true, then:
* if the stream is incremental, then all the newer snapshots are destroyed, the
filesystem is virtually rolled back to the latest received snapshot and then the
stream is applied;
* if the stream is full, then the operation fails despite the force flag.

However, if the destination filesystem is modified but there are no newer
snapshots, then in both cases the operation succeeds: any local modifications
are discarded and the stream is applied.

So, I wonder what a rationale is for not destroying the local snapshots if a
full stream is force-received into the existing and modified and "snapshotted"
filesystem.

Maybe this is a bug and recv_begin_check_existing_impl() needs to take into
account the force flag for the full stream case as well.

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] receiving non-incremental stream into existing fs

[Andriy Gapon]

On 23/04/2015 21:12, Paul Dagnelie wrote:
> I think you're right.  We should modify recv_begin_check_existing_impl to fail
> if fromguid != 0 and force is false.  If we don't, we're guaranteed to fail in
> dsl_dataset_clone_swap_check_impl (unless the dataset has not been modified
> since the origin, I guess. But given that we (almost?) always modify datasets
> when we create them, that doesn't seem likely), and that's definitely 
> suboptimal.

BTW, if I just create a new filesystem with mountpoint=none and never mount it,
never snapshot it, or otherwise touch it; and then when I try to receive a full
stream into that filesystem dsl_dataset_clone_swap_check_impl() still returns
ETXTBUSY.  Apparently dsl_dataset_modified_since_snap() returns true even in
that case.  This is the case for a root filesystem in a pristine pool as well.

It seems that any filesystem is considered modified when compared to $ORIGIN.
And ds_prev of a filesystem without any real snapshots points to $ORIGIN.
Or is it $ORIGIN@$ORIGIN actually?

Finally, it looks that the above is true since ZFS v11 (SPA_VERSION_ORIGIN),
which means that it is always true in practice.

> As for the ENODEV vs EEXIST issue, after we've changed it to require force, it
> will only return ENODEV in the case where tofs exists and has snapshots,
> fromguid == 0, and force is true.  I think that ENODEV is technically correct 
> in
> that case (the latest snapshot of the existing tofs does not map the fromsnap 
> of
> the stream, namely the origin), though I would be fine with changing it to
> EEXIST instead.
> 
> On Wed, Apr 22, 2015 at 5:45 AM, andriy.ga...@clusterhq.com
> <mailto:andriy.ga...@clusterhq.com> wrote:
> 
> I wonder if dmu_recv_begin_check() -> recv_begin_check_existing_impl() 
> should
> fail in the following case:
> - full stream, fromguid == 0
> - tofs already exists
> - tofs does not have any snapshots [*]
> - force is false
> 
> In my opinion there is no way that this could succeed.
> But right now this seem to succeed only to fail later in 
> dmu_recv_end_check()
> (with ETXTBSY). That seems like suboptimal behavior.
> 
> Also, if tofs has any snapshots then dmu_recv_begin_check() fails.  But I 
> do not
> see why the check should succeed even when there are no snapshots.
> Additionally, in this case dmu_recv_begin_check() returns ENODEV, which 
> in this
> context is typically interpreted to mean that a source of an incremental 
> stream
> does not match a latest snapshot of a target filesystem.  That's a little 
> bit
> confusing since we have a full stream.  Maybe it would have better to 
> return
> EEXIST signalling that the target filesystem exists.
> 
>  
> -- 
> Paul Dagnelie


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] z_root change after recv -F

[Andriy Gapon]

A semi-theoretical question: can z_root - or, to be correct, ZFS_ROOT_OBJ -
value change after a filesystem has its content replaced by zfs recv -F of a
full stream?
It seems that that case wouldn't be handled correctly if the filesystem is
mounted and goes through zfs_suspend_fs() + zfs_resume_fs().

The reason I am asking is that I noticed this:
$ zdb - pond/var 1

casesensitivity = 0
SA_ATTRS = 4
normalization = 0
VERSION = 5
ROOT = 3
utf8only = 0
DELETE_QUEUE = 2

$ zdb - pond/var/crash 1

ROOT = 4
utf8only = 0
casesensitivity = 0
VERSION = 5
SA_ATTRS = 2
SHARES = 7
DELETE_QUEUE = 3
normalization = 0

This may have very well happened because of my playing with the code, but in
general the root object does not have a fixed ID.

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] libzfs_core and dmu_objset_type_t

[Andriy Gapon]

Any interest in this topic at all?

On 18/03/2015 18:55, Andriy Gapon wrote:
> I think that we should try to isolate libzfs_core API and ABI as much as
> possible from ZFS implementation details.
> In this respect it seems that there is no good reason to use dmu_objset_type_t
> with lzc_create().  Given that currently we can create only two types of
> datasets, a filesystem or a ZVOL, then a simple boolean parameter would be
> sufficient, but for clarity and extensibility we could use a locally defined
> enumeration like it was done for lzc_send_flags.
> And it seems that libzfs_core.h has to include sys/fs/zfs.h only because of
> dmu_objset_type_t.
>
> What do you think?
>
> Finally, if we go for the change then it would present a challenge of its own,
> because the stable lzc API would have to be modified.  What would be the best
> way to do that?  Or does this concern alone kill the whole proposal?
>
> Thanks!
>


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] receiving non-incremental stream into existing fs

[Andriy Gapon]

I wonder if dmu_recv_begin_check() -> recv_begin_check_existing_impl() should
fail in the following case:
- full stream, fromguid == 0
- tofs already exists
- tofs does not have any snapshots [*]
- force is false

In my opinion there is no way that this could succeed.
But right now this seem to succeed only to fail later in dmu_recv_end_check()
(with ETXTBSY). That seems like suboptimal behavior.

Also, if tofs has any snapshots then dmu_recv_begin_check() fails.  But I do not
see why the check should succeed even when there are no snapshots.
Additionally, in this case dmu_recv_begin_check() returns ENODEV, which in this
context is typically interpreted to mean that a source of an incremental stream
does not match a latest snapshot of a target filesystem.  That's a little bit
confusing since we have a full stream.  Maybe it would have better to return
EEXIST signalling that the target filesystem exists.


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] kernel panic in an edge case of bookmarks creation

[Andriy Gapon]

On 14/04/2015 21:30, Matthew Ahrens wrote:
> First off, a million thanks for writing tests!  I would love to see whatever 
> you
> come up with become part of the test suite.
> 
> Your analysis is correct.  A fix for this was approved last year, I don't know
> why it has not been integrated yet.  I guess I'll do it myself.
> 
> https://reviews.csiden.org/r/143/

Thanks a lot!
This seems like a rather serious problem, because in my environment any
filesystem that gets a bookmark just "disappears", it's not possible to perform
any operation on it.

> On Mon, Apr 13, 2015 at 11:01 PM, Andriy Gapon  <mailto:andriy.ga...@clusterhq.com>> wrote:
> 
> On 14/04/2015 08:04, Andriy Gapon wrote:
> > So, I pass the following bookmarks nvlist to lzc_bookmark():
> > {
> >   "pool/fs#bookmark": "pool/fs@snap",
> >   "pool/fs#another_bookmark": "pool/fs@snap"
> > }
> >
> > That is, I am trying to create in one go two valid bookmarks that point 
> to the
> > same snapshot.
> >
> > This results in the following panic:
> > assert: dsl_dataset_hold(dp, fnvpair_value_string(pair), ((char 
> *)__func__),
> > &snapds) == 0 (0x2 == 0x0), file: .../dsl_bookmark.c, line: ...
> >
> > A stack trace:
> > panic()
> > assfail3()
> > dsl_bookmark_create_sync()
> > dsl_sync_task_sync()
> > dsl_pool_sync()
> > spa_sync()
> > txg_sync_thread()
> >
> > The panic confuses me a lot, because I couldn't figure out how 
> dsl_dataset_hold
> > could return ENOENT.  I only have a guess: I think that the way of how
> > dsl_dataset_zapify() works might cause this.
> 
> After a debugging session I think that I see a problem.  It's not in the
> "zapification" as I guessed, but the "zapification" is a trigger.
> 
> dsl_dataset_hold_obj() has a check for the large blocks support and that 
> check
> has a small but quite nasty bug.  The check leaks an error code in the 
> case the
> support has not been enabled for a dataset:
> 
> if (doi.doi_type == DMU_OTN_ZAP_METADATA) {
> err = zap_contains(mos, dsobj, 
> DS_FIELD_LARGE_BLOCKS);
> if (err == 0)
> ds->ds_large_blocks = B_TRUE;
> else
>     ASSERT3U(err, ==, ENOENT);
> }
> 
> This ENOENT causes dsl_dataset_hold_obj() to fail.
> 
> > Additional note: the panic happens on FreeBSD, but I couldn't reproduce 
> it on Linux.
> 
> Seems like ZoL hasn't imported the large blocks support and so it doesn't 
> have
> the bug.
> 
> --
> Andriy Gapon
> ___
> developer mailing list
> developer@open-zfs.org <mailto:developer@open-zfs.org>
> http://lists.open-zfs.org/mailman/listinfo/developer
> 
> 


-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] kernel panic in an edge case of bookmarks creation

[Andriy Gapon]

On 14/04/2015 08:04, Andriy Gapon wrote:
> So, I pass the following bookmarks nvlist to lzc_bookmark():
> {
>   "pool/fs#bookmark": "pool/fs@snap",
>   "pool/fs#another_bookmark": "pool/fs@snap"
> }
> 
> That is, I am trying to create in one go two valid bookmarks that point to the
> same snapshot.
> 
> This results in the following panic:
> assert: dsl_dataset_hold(dp, fnvpair_value_string(pair), ((char *)__func__),
> &snapds) == 0 (0x2 == 0x0), file: .../dsl_bookmark.c, line: ...
> 
> A stack trace:
> panic()
> assfail3()
> dsl_bookmark_create_sync()
> dsl_sync_task_sync()
> dsl_pool_sync()
> spa_sync()
> txg_sync_thread()
> 
> The panic confuses me a lot, because I couldn't figure out how 
> dsl_dataset_hold
> could return ENOENT.  I only have a guess: I think that the way of how
> dsl_dataset_zapify() works might cause this.

After a debugging session I think that I see a problem.  It's not in the
"zapification" as I guessed, but the "zapification" is a trigger.

dsl_dataset_hold_obj() has a check for the large blocks support and that check
has a small but quite nasty bug.  The check leaks an error code in the case the
support has not been enabled for a dataset:

if (doi.doi_type == DMU_OTN_ZAP_METADATA) {
err = zap_contains(mos, dsobj, DS_FIELD_LARGE_BLOCKS);
if (err == 0)
ds->ds_large_blocks = B_TRUE;
else
ASSERT3U(err, ==, ENOENT);
}

This ENOENT causes dsl_dataset_hold_obj() to fail.

> Additional note: the panic happens on FreeBSD, but I couldn't reproduce it on 
> Linux.

Seems like ZoL hasn't imported the large blocks support and so it doesn't have
the bug.

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] kernel panic in an edge case of bookmarks creation

[Andriy Gapon]

A small introduction first.  I'm currently writing a bunch of test cases for
libzfs_core and I am testing not only for expected behavior of the calls, but
also for various failure modes.

Having said that, I think that the following test case should work even if it
does not make much sense.
So, I pass the following bookmarks nvlist to lzc_bookmark():
{
"pool/fs#bookmark": "pool/fs@snap",
"pool/fs#another_bookmark": "pool/fs@snap"
}

That is, I am trying to create in one go two valid bookmarks that point to the
same snapshot.

This results in the following panic:
assert: dsl_dataset_hold(dp, fnvpair_value_string(pair), ((char *)__func__),
&snapds) == 0 (0x2 == 0x0), file: .../dsl_bookmark.c, line: ...

A stack trace:
panic()
assfail3()
dsl_bookmark_create_sync()
dsl_sync_task_sync()
dsl_pool_sync()
spa_sync()
txg_sync_thread()

The panic confuses me a lot, because I couldn't figure out how dsl_dataset_hold
could return ENOENT.  I only have a guess: I think that the way of how
dsl_dataset_zapify() works might cause this.

Additional note: the panic happens on FreeBSD, but I couldn't reproduce it on 
Linux.

-- 
Andriy Gapon
___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Fw: [REVIEW] 5610 zfs clone from different source and target pools produces coredump

[Andriy Gapon]

Matt,

is it plausible that dsl_dir_hold(pool, "different-pool/fs", ...) could ever be
successful?

On 13/04/2015 20:37, Matthew Ahrens wrote:
> Could you turn the checks in dmu_objset_clone_check() into assertions (plus
> maybe a comment saying where it is checked), rather than just removing them?
>
> --matt
>
> On Mon, Apr 13, 2015 at 4:27 AM, Alexander  <mailto:alexander.ere...@nexenta.com>> wrote:
>
> On 10 Apr 2015 at 19:30:50, Matthew Ahrens (mahr...@delphix.com
> <mailto:mahr...@delphix.com>) wrote:
>> Ship It!
>>
>> Thanks, especially for using the test suite!
>>
>> --matt
>>
>> On Fri, Apr 10, 2015 at 12:57 AM, Alexander > <mailto:alexander.ere...@nexenta.com>> wrote:
>>
>>> http://cr.illumos.org/~webrev/alhazred/5610/
>>> <http://cr.illumos.org/%7Ewebrev/alhazred/5610/>
>>>
>>> Tested with zfs test suite.
>>>
>>>     -- 
>>> Alexander
>>
>>
>
>
> Thanks to Andriy for suggestion,
> I’ve updated webrev in place and re-tested.
>
> -- 
> Alexander
>
>


-- 
Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] libzfs_core and dmu_objset_type_t

[Andriy Gapon]

I think that we should try to isolate libzfs_core API and ABI as much as
possible from ZFS implementation details.
In this respect it seems that there is no good reason to use dmu_objset_type_t
with lzc_create().  Given that currently we can create only two types of
datasets, a filesystem or a ZVOL, then a simple boolean parameter would be
sufficient, but for clarity and extensibility we could use a locally defined
enumeration like it was done for lzc_send_flags.
And it seems that libzfs_core.h has to include sys/fs/zfs.h only because of
dmu_objset_type_t.

What do you think?

Finally, if we go for the change then it would present a challenge of its own,
because the stable lzc API would have to be modified.  What would be the best
way to do that?  Or does this concern alone kill the whole proposal?

Thanks!

-- 
Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 141: zfs send -p -I: send properties only for snapshots that are actually sent

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/141/#review522
---



usr/src/lib/libzfs/common/libzfs_sendrecv.c
<https://reviews.csiden.org/r/141/#comment401>

As @dweeezil correctly noted this simple solution breaks several usecases.
First, zfs recv -F is broken because to implement it the receiving side 
needs to know about all snpashots that exist on the sending side.
Another bug is that the properties are not included into a full stream 
produced with zfs send -p pool/ds@snap.
Most likely the original performance issue better be fixed at the receiving 
end.


- Andriy Gapon


On Dec. 2, 2014, 2:11 p.m., Andriy Gapon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/141/
> ---
> 
> (Updated Dec. 2, 2014, 2:11 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List.
> 
> 
> Bugs: 5380
> https://www.illumos.org/projects/illumos-gate//issues/5380
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> ... as opposed to sending properties of all snapshots of the relevant
> filesystem.  The previous behavior results in properties being set on
> all snapshots on the receiving side, which is quite slow.
> 
> Behavior of zfs send -R is not changed.
> 
> Note that send_iterate_fs() now has to iterate snapshots in their
> creation order.
> 
> 
> Diffs
> -
> 
>   usr/src/lib/libzfs/common/libzfs_sendrecv.c 
> c4944438aa2cdcb751ae04a73be52ce9d7b2cef4 
> 
> Diff: https://reviews.csiden.org/r/141/diff/
> 
> 
> Testing
> ---
> 
> Only FreeBSD and ZoL.  None for illumos.
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 141: zfs send -p -I: send properties only for snapshots that are actually sent

[Andriy Gapon]

> On Dec. 9, 2014, 9:28 p.m., Tim Chase wrote:
> > Please check out an issue related to this in 
> > https://github.com/zfsonlinux/zfs/pull/2907 and my proposed fix.  In short, 
> > the incomplete snapshot list can cause Bad Things™ to happen during "zfs 
> > receive -F".

Thank you for drawing my attention to this problem and your solution. I'll 
investigate it.


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/141/#review393
---


On Dec. 2, 2014, 2:11 p.m., Andriy Gapon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/141/
> ---
> 
> (Updated Dec. 2, 2014, 2:11 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List.
> 
> 
> Bugs: 5380
> https://www.illumos.org/projects/illumos-gate//issues/5380
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> ... as opposed to sending properties of all snapshots of the relevant
> filesystem.  The previous behavior results in properties being set on
> all snapshots on the receiving side, which is quite slow.
> 
> Behavior of zfs send -R is not changed.
> 
> Note that send_iterate_fs() now has to iterate snapshots in their
> creation order.
> 
> 
> Diffs
> -
> 
>   usr/src/lib/libzfs/common/libzfs_sendrecv.c 
> c4944438aa2cdcb751ae04a73be52ce9d7b2cef4 
> 
> Diff: https://reviews.csiden.org/r/141/diff/
> 
> 
> Testing
> ---
> 
> Only FreeBSD and ZoL.  None for illumos.
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 112: account for ashift when choosing buffers to be written to l2arc device

[Andriy Gapon]

> On Nov. 30, 2014, 11:02 p.m., Matthew Ahrens wrote:
> > You will need to test on illumos too, at least some minimal sanity 
> > checking.  Let me know if you need help with that.

I am still not set up for illumos testing :-(
So I must ask for your help again.  Thank you!


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/112/#review378
---


On Oct. 8, 2014, 4:57 p.m., Andriy Gapon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/112/
> ---
> 
> (Updated Oct. 8, 2014, 4:57 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List, Matthew Ahrens and Saso 
> Kiselkov.
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> If we don't account for that, then we might end up overwriting disk
> area of buffers that have not been evicted yet, because l2arc_evict
> operates in terms of disk addresses.
> 
> The discrepancy between the write size calculation and the actual increment
> to l2ad_hand was introduced in
> commit e14bb3258d05c1b1077e2db7cf77088924e56919
> 
> Also, consistently use asize / a_sz for the allocated size, psize / p_sz
> for the physical size.  Where the latter accounts for possible size
> reduction because of compression, whereas the former accounts for possible
> size expansion because of alignment requirements.
> 
> The code still assumes that either underlying storage subsystems or
> hardware is able to do read-modify-write when an L2ARC buffer size is
> not a multiple of a disk's block size.  This is true for 4KB sector disks
> that provide 512B sector emulation, but may not be true in general.
> In other words, we currently do not have any code to make sure that
> an L2ARC buffer, whether compressed or not, which is used for physical I/O
> has a suitable size.
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/arc.c 69d16af3b669458e3937fe0c6a4d91755bc6e2a7 
> 
> Diff: https://reviews.csiden.org/r/112/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andriy Gapon
> 
>

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 128: 5313 Allow I/Os to be aggregated across ZIO priority classes

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/128/
---

(Updated Dec. 2, 2014, 2:22 p.m.)


Review request for OpenZFS Developer Mailing List, Justin Gibbs and George 
Wilson.


Changes
---

The warning should be fixed now.


Bugs: 5313
https://www.illumos.org/projects/illumos-gate//issues/5313


Repository: illumos-gate


Description
---

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/vdev_impl.h:
Add two offset/lba based AVL trees to the vdev queue
object.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zio.h:
Add a second AVL node within each ZIO so that vdev_queue.c
can sort ZIOs by both type and priority.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_queue.c:
Combine reads and writes, irrespecitve of their priorities
into unified, offset sorted, trees.  Selection of the
ZIO to issue is unchanged, but aggregation now uses the
unified tree of the appropriate type so that aggregation
across priority classes is possible.

Original author:Justin T. Gibbs 


Diffs (updated)
-

  usr/src/uts/common/fs/zfs/vdev_queue.c 
f47c4cd1e26dfa635dcf28e4ea3cd0cc8de4e3d1 
  usr/src/uts/common/fs/zfs/sys/zio.h f6cf259bf71a741cfee8e290be93c0600c5c9240 
  usr/src/uts/common/fs/zfs/sys/vdev_impl.h 
6d9bcb17d00fd70d1585b9c2218086ca2239aab6 

Diff: https://reviews.csiden.org/r/128/diff/


Testing
---


Thanks,

Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

[OpenZFS Developer] Review Request 141: zfs send -p -I: send properties only for snapshots that are actually sent

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/141/
---

Review request for OpenZFS Developer Mailing List.


Bugs: 5380
https://www.illumos.org/projects/illumos-gate//issues/5380


Repository: illumos-gate


Description
---

 as opposed to sending properties of all snapshots of the relevant
filesystem.  The previous behavior results in properties being set on
all snapshots on the receiving side, which is quite slow.

Behavior of zfs send -R is not changed.

Note that send_iterate_fs() now has to iterate snapshots in their
creation order.


Diffs
-

  usr/src/lib/libzfs/common/libzfs_sendrecv.c 
c4944438aa2cdcb751ae04a73be52ce9d7b2cef4 

Diff: https://reviews.csiden.org/r/141/diff/


Testing
---

Only FreeBSD and ZoL.  None for illumos.


Thanks,

Andriy Gapon

___
developer mailing list
developer@open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer

Re: [OpenZFS Developer] Review Request 129: [5314] Remove "dbuf phys" db->db_data pointer aliases in ZFS

[Andriy Gapon]

> On Nov. 25, 2014, 1:48 p.m., Andriy Gapon wrote:
> > usr/src/uts/common/fs/zfs/sys/dsl_dir.h, line 87
> > <https://reviews.csiden.org/r/129/diff/3/?file=11663#file11663line87>
> >
> > This change seems to imply that dd_dbuf and dd_pool fields are mutable, 
> > are they?
> 
> Justin Gibbs wrote:
> Do you mean that the original comment implied that dd_pool and dd_dbuf 
> are immutable, since there was no whitespace putting them into a separate 
> block?  They aren't immutable.

My impression is that dd_pool and dd_dbuf values are not changed over a 
lifetime of a dsl_dir_phys object, but quite possibly I could be wrong.


- Andriy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/129/#review363
---


On Nov. 25, 2014, 6:36 p.m., Justin Gibbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/129/
> ---
> 
> (Updated Nov. 25, 2014, 6:36 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List.
> 
> 
> Bugs: 5314
> https://www.illumos.org/projects/illumos-gate//issues/5314
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> Author: Justin T. Gibbs 
> Date:   Tue Nov 11 14:54:48 2014 -0800
> 
> Remove "dbuf phys" db->db_data pointer aliases.
> 
> Use function accessors that cast db->db_data to the appropriate
> "phys" type, removing the need for clients of the dmu buf user
> API to keep properly typed pointer aliases to db->db_data in order
> to conveniently access their data.
> 
> cmd/mdb/common/modules/zfs/Makefile.zfs:
> cmd/mdb/intel/amd64/libzpool/Makefile:
> cmd/mdb/intel/amd64/zfs/Makefile:
> cmd/mdb/intel/ia32/libzpool/Makefile:
> cmd/mdb/intel/ia32/zfs/Makefile:
> cmd/mdb/sparc/v7/libzpool/Makefile:
> cmd/mdb/sparc/v9/libzpool/Makefile:
> cmd/mdb/sparc/v9/zfs/Makefile:
> cmd/zdb/Makefile.com:
> Silence E_STATIC_UNUSED lint warnings.
> 
> cmd/mdb/common/modules/zfs/zfs.c:
> uts/common/fs/zfs/zap_leaf.c:
> In zap_leaf() and zap_leaf_byteswap, now that the pointer alias
> field l_phys has been removed, use the db_data field in an on
> stack dmu_buf_t to point to the leaf's phys data.
> 
> uts/common/fs/zfs/dbuf.c:
> Remove the db_user_data_ptr_ptr field from dbuf and all logic
> to maintain it.
> 
> 
> uts/common/fs/zfs/dbuf.c:
> uts/common/fs/zfs/dnode.c:
> uts/common/fs/zfs/sys/dbuf.h:
> uts/common/fs/zfs/sys/dmu.h:
> uts/common/fs/zfs/dsl_dataset.c:
> uts/common/fs/zfs/dsl_dir.c:
> uts/common/fs/zfs/sa.c:
> uts/common/fs/zfs/zap.c:
> uts/common/fs/zfs/zap_micro.c:
> Modify the DMU buf user API to remove the ability to specify
> a db_data aliasing pointer (db_user_data_ptr_ptr).
> 
> cmd/zdb/zdb.c:
> uts/common/fs/zfs/dmu_diff.c:
> uts/common/fs/zfs/dmu_objset.c:
> uts/common/fs/zfs/dmu_send.c:
> uts/common/fs/zfs/dmu_traverse.c:
> uts/common/fs/zfs/dmu_tx.c:
> uts/common/fs/zfs/dsl_bookmark.c:
> uts/common/fs/zfs/dsl_dataset.c:
> uts/common/fs/zfs/dsl_deadlist.c:
> uts/common/fs/zfs/dsl_deleg.c:
> uts/common/fs/zfs/dsl_destroy.c:
> uts/common/fs/zfs/dsl_dir.c:
> uts/common/fs/zfs/dsl_pool.c:
> uts/common/fs/zfs/dsl_prop.c:
> uts/common/fs/zfs/dsl_scan.c:
> uts/common/fs/zfs/dsl_synctask.c:
> uts/common/fs/zfs/dsl_userhold.c:
> uts/common/fs/zfs/sa.c:
> uts/common/fs/zfs/spa.c:
> uts/common/fs/zfs/spa_history.c:
> uts/common/fs/zfs/zap.c:
> uts/common/fs/zfs/zap_leaf.c:
> uts/common/fs/zfs/zap_micro.c:
> uts/common/fs/zfs/zfs_ioctl.c:
> uts/common/fs/zfs/sys/dsl_dataset.h:
> uts/common/fs/zfs/sys/dsl_dir.h:
> uts/common/fs/zfs/sys/zap_impl.h:
> uts/common/fs/zfs/sys/zap_leaf.h:
> Create and use the new "phys data" accessor functions
> dsl_dir_phys(), dsl_dataset_phys(), zap_m_phys(),
> zap_f_phys(), and zap_leaf_phys().
> 
> uts/common/fs/zfs/sys/dsl_dataset.h:
> uts/common/fs/zfs/sys/dsl_dir.h:
> uts/common/fs/zfs/sys/zap_impl.h:
> uts/common/fs/zfs/sys/zap_leaf.h:
> Remove now unused "phys pointer" aliases to db->db_data
> from clients of the DMU buf user API.
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/sys/zap_leaf.h 
> f6947a72d70e947c4aece66425cb16dd743ecee8 
>   usr/src/uts/common/fs/zfs/sys/dsl_dataset.h 
> ff90f8b439ccf7a137adcf10ba2c018bd9ae30aa 
>   usr/src

Re: [OpenZFS Developer] Review Request 129: [5314] Remove "dbuf phys" db->db_data pointer aliases in ZFS

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/129/#review365
---

Ship it!


There seems to be a small mistake in the commit message /is/to/, but otherwise 
looks good to me.

- Andriy Gapon


On Nov. 21, 2014, 6:38 p.m., Justin Gibbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/129/
> ---
> 
> (Updated Nov. 21, 2014, 6:38 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List.
> 
> 
> Bugs: 5314
> https://www.illumos.org/projects/illumos-gate//issues/5314
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> Author: Justin T. Gibbs 
> Date:   Tue Nov 11 14:54:48 2014 -0800
> 
> Remove "dbuf phys" db->db_data pointer aliases.
> 
> Use function accessors that cast db->db_data to the appropriate
> "phys" type, removing the need for clients of the dmu buf user
> API is keep properly typed pointer aliases to db->db_data in order
> to conveniently access their data.
> 
> cmd/mdb/common/modules/zfs/Makefile.zfs:
> cmd/mdb/intel/amd64/libzpool/Makefile:
> cmd/mdb/intel/amd64/zfs/Makefile:
> cmd/mdb/intel/ia32/libzpool/Makefile:
> cmd/mdb/intel/ia32/zfs/Makefile:
> cmd/mdb/sparc/v7/libzpool/Makefile:
> cmd/mdb/sparc/v9/libzpool/Makefile:
> cmd/mdb/sparc/v9/zfs/Makefile:
> cmd/zdb/Makefile.com:
> Silence E_STATIC_UNUSED lint warnings.
> 
> cmd/mdb/common/modules/zfs/zfs.c:
> uts/common/fs/zfs/zap_leaf.c:
> In zap_leaf() and zap_leaf_byteswap, now that the pointer alias
> field l_phys has been removed, use the db_data field in an on
> stack dmu_buf_t to point to the leaf's phys data.
> 
> uts/common/fs/zfs/dbuf.c:
> Remove the db_user_data_ptr_ptr field from dbuf and all logic
> to maintain it.
> 
> 
> uts/common/fs/zfs/dbuf.c:
> uts/common/fs/zfs/dnode.c:
> uts/common/fs/zfs/sys/dbuf.h:
> uts/common/fs/zfs/sys/dmu.h:
> uts/common/fs/zfs/dsl_dataset.c:
> uts/common/fs/zfs/dsl_dir.c:
> uts/common/fs/zfs/sa.c:
> uts/common/fs/zfs/zap.c:
> uts/common/fs/zfs/zap_micro.c:
> Modify the DMU buf user API to remove the ability to specify
> a db_data aliasing pointer (db_user_data_ptr_ptr).
> 
> cmd/zdb/zdb.c:
> uts/common/fs/zfs/dmu_diff.c:
> uts/common/fs/zfs/dmu_objset.c:
> uts/common/fs/zfs/dmu_send.c:
> uts/common/fs/zfs/dmu_traverse.c:
> uts/common/fs/zfs/dmu_tx.c:
> uts/common/fs/zfs/dsl_bookmark.c:
> uts/common/fs/zfs/dsl_dataset.c:
> uts/common/fs/zfs/dsl_deadlist.c:
> uts/common/fs/zfs/dsl_deleg.c:
> uts/common/fs/zfs/dsl_destroy.c:
> uts/common/fs/zfs/dsl_dir.c:
> uts/common/fs/zfs/dsl_pool.c:
> uts/common/fs/zfs/dsl_prop.c:
> uts/common/fs/zfs/dsl_scan.c:
> uts/common/fs/zfs/dsl_synctask.c:
> uts/common/fs/zfs/dsl_userhold.c:
> uts/common/fs/zfs/sa.c:
> uts/common/fs/zfs/spa.c:
> uts/common/fs/zfs/spa_history.c:
> uts/common/fs/zfs/zap.c:
> uts/common/fs/zfs/zap_leaf.c:
> uts/common/fs/zfs/zap_micro.c:
> uts/common/fs/zfs/zfs_ioctl.c:
> uts/common/fs/zfs/sys/dsl_dataset.h:
> uts/common/fs/zfs/sys/dsl_dir.h:
> uts/common/fs/zfs/sys/zap_impl.h:
> uts/common/fs/zfs/sys/zap_leaf.h:
> Create and use the new "phys data" accessor functions
> dsl_dir_phys(), dsl_dataset_phys(), zap_m_phys(),
> zap_f_phys(), and zap_leaf_phys().
> 
> uts/common/fs/zfs/sys/dsl_dataset.h:
> uts/common/fs/zfs/sys/dsl_dir.h:
> uts/common/fs/zfs/sys/zap_impl.h:
> uts/common/fs/zfs/sys/zap_leaf.h:
> Remove now unused "phys pointer" aliases to db->db_data
> from clients of the DMU buf user API.
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/sys/zap_leaf.h 
> f6947a72d70e947c4aece66425cb16dd743ecee8 
>   usr/src/uts/common/fs/zfs/sys/dsl_dataset.h 
> ff90f8b439ccf7a137adcf10ba2c018bd9ae30aa 
>   usr/src/uts/common/fs/zfs/sys/dbuf.h 
> 8ca8753e5e6d55b13c6d9dde37b3c288feb35066 
>   usr/src/uts/common/fs/zfs/zap_micro.c 
> 59a9f970448a12b1e7de48f2b87c6eef4cc577eb 
>   usr/src/uts/common/fs/zfs/spa_history.c 
> ce64f70b28c39a485abe1848e9f988611bffce51 
>   usr/src/uts/common/fs/zfs/sa.c 8b3963aed9f57332de5b2b551492bae704f1a1ce 
>   usr/src/uts/common/fs/zfs/dsl_synctask.c 
> 8e09347678ddbdaf13f925a149122dd4209624d7 
>   usr/src/uts/common/fs/zfs/dsl_pool.c 
> 1d246b8e688d37b4925d9c22e23bc001e9faa055 
>   usr/src/uts/common/fs/zfs/dsl_

Re: [OpenZFS Developer] Review Request 129: [5314] Remove "dbuf phys" db->db_data pointer aliases in ZFS

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/129/#review364
---


A general question: have you considered using compatibility macros instead of 
the removed fields rather than the accessor functions?  If yes, was that 
considered too dangerous with respect to a potential name clash?

- Andriy Gapon


On Nov. 21, 2014, 6:38 p.m., Justin Gibbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/129/
> ---
> 
> (Updated Nov. 21, 2014, 6:38 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List.
> 
> 
> Bugs: 5314
> https://www.illumos.org/projects/illumos-gate//issues/5314
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> Author: Justin T. Gibbs 
> Date:   Tue Nov 11 14:54:48 2014 -0800
> 
> Remove "dbuf phys" db->db_data pointer aliases.
> 
> Use function accessors that cast db->db_data to the appropriate
> "phys" type, removing the need for clients of the dmu buf user
> API is keep properly typed pointer aliases to db->db_data in order
> to conveniently access their data.
> 
> cmd/mdb/common/modules/zfs/Makefile.zfs:
> cmd/mdb/intel/amd64/libzpool/Makefile:
> cmd/mdb/intel/amd64/zfs/Makefile:
> cmd/mdb/intel/ia32/libzpool/Makefile:
> cmd/mdb/intel/ia32/zfs/Makefile:
> cmd/mdb/sparc/v7/libzpool/Makefile:
> cmd/mdb/sparc/v9/libzpool/Makefile:
> cmd/mdb/sparc/v9/zfs/Makefile:
> cmd/zdb/Makefile.com:
> Silence E_STATIC_UNUSED lint warnings.
> 
> cmd/mdb/common/modules/zfs/zfs.c:
> uts/common/fs/zfs/zap_leaf.c:
> In zap_leaf() and zap_leaf_byteswap, now that the pointer alias
> field l_phys has been removed, use the db_data field in an on
> stack dmu_buf_t to point to the leaf's phys data.
> 
> uts/common/fs/zfs/dbuf.c:
> Remove the db_user_data_ptr_ptr field from dbuf and all logic
> to maintain it.
> 
> 
> uts/common/fs/zfs/dbuf.c:
> uts/common/fs/zfs/dnode.c:
> uts/common/fs/zfs/sys/dbuf.h:
> uts/common/fs/zfs/sys/dmu.h:
> uts/common/fs/zfs/dsl_dataset.c:
> uts/common/fs/zfs/dsl_dir.c:
> uts/common/fs/zfs/sa.c:
> uts/common/fs/zfs/zap.c:
> uts/common/fs/zfs/zap_micro.c:
> Modify the DMU buf user API to remove the ability to specify
> a db_data aliasing pointer (db_user_data_ptr_ptr).
> 
> cmd/zdb/zdb.c:
> uts/common/fs/zfs/dmu_diff.c:
> uts/common/fs/zfs/dmu_objset.c:
> uts/common/fs/zfs/dmu_send.c:
> uts/common/fs/zfs/dmu_traverse.c:
> uts/common/fs/zfs/dmu_tx.c:
> uts/common/fs/zfs/dsl_bookmark.c:
> uts/common/fs/zfs/dsl_dataset.c:
> uts/common/fs/zfs/dsl_deadlist.c:
> uts/common/fs/zfs/dsl_deleg.c:
> uts/common/fs/zfs/dsl_destroy.c:
> uts/common/fs/zfs/dsl_dir.c:
> uts/common/fs/zfs/dsl_pool.c:
> uts/common/fs/zfs/dsl_prop.c:
> uts/common/fs/zfs/dsl_scan.c:
> uts/common/fs/zfs/dsl_synctask.c:
> uts/common/fs/zfs/dsl_userhold.c:
> uts/common/fs/zfs/sa.c:
> uts/common/fs/zfs/spa.c:
> uts/common/fs/zfs/spa_history.c:
> uts/common/fs/zfs/zap.c:
> uts/common/fs/zfs/zap_leaf.c:
> uts/common/fs/zfs/zap_micro.c:
> uts/common/fs/zfs/zfs_ioctl.c:
> uts/common/fs/zfs/sys/dsl_dataset.h:
> uts/common/fs/zfs/sys/dsl_dir.h:
> uts/common/fs/zfs/sys/zap_impl.h:
> uts/common/fs/zfs/sys/zap_leaf.h:
> Create and use the new "phys data" accessor functions
> dsl_dir_phys(), dsl_dataset_phys(), zap_m_phys(),
> zap_f_phys(), and zap_leaf_phys().
> 
> uts/common/fs/zfs/sys/dsl_dataset.h:
> uts/common/fs/zfs/sys/dsl_dir.h:
> uts/common/fs/zfs/sys/zap_impl.h:
> uts/common/fs/zfs/sys/zap_leaf.h:
> Remove now unused "phys pointer" aliases to db->db_data
> from clients of the DMU buf user API.
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/sys/zap_leaf.h 
> f6947a72d70e947c4aece66425cb16dd743ecee8 
>   usr/src/uts/common/fs/zfs/sys/dsl_dataset.h 
> ff90f8b439ccf7a137adcf10ba2c018bd9ae30aa 
>   usr/src/uts/common/fs/zfs/sys/dbuf.h 
> 8ca8753e5e6d55b13c6d9dde37b3c288feb35066 
>   usr/src/uts/common/fs/zfs/zap_micro.c 
> 59a9f970448a12b1e7de48f2b87c6eef4cc577eb 
>   usr/src/uts/common/fs/zfs/spa_history.c 
> ce64f70b28c39a485abe1848e9f988611bffce51 
>   usr/src/uts/common/fs/zfs/sa.c 8b3963aed9f57332de5b2b551492bae704f1a1ce 
>   usr/src/uts/common/fs/zfs/dsl_synctask.c 
> 8e09347678ddbdaf13f925a149122dd4209624d7 
>   usr/src/uts/common/fs/zfs/dsl_p

Re: [OpenZFS Developer] Review Request 129: [5314] Remove "dbuf phys" db->db_data pointer aliases in ZFS

[Andriy Gapon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.csiden.org/r/129/#review363
---



usr/src/uts/common/fs/zfs/sys/dsl_dir.h
<https://reviews.csiden.org/r/129/#comment301>

This change seems to imply that dd_dbuf and dd_pool fields are mutable, are 
they?


- Andriy Gapon


On Nov. 21, 2014, 6:38 p.m., Justin Gibbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.csiden.org/r/129/
> ---
> 
> (Updated Nov. 21, 2014, 6:38 p.m.)
> 
> 
> Review request for OpenZFS Developer Mailing List.
> 
> 
> Bugs: 5314
> https://www.illumos.org/projects/illumos-gate//issues/5314
> 
> 
> Repository: illumos-gate
> 
> 
> Description
> ---
> 
> Author: Justin T. Gibbs 
> Date:   Tue Nov 11 14:54:48 2014 -0800
> 
> Remove "dbuf phys" db->db_data pointer aliases.
> 
> Use function accessors that cast db->db_data to the appropriate
> "phys" type, removing the need for clients of the dmu buf user
> API is keep properly typed pointer aliases to db->db_data in order
> to conveniently access their data.
> 
> cmd/mdb/common/modules/zfs/Makefile.zfs:
> cmd/mdb/intel/amd64/libzpool/Makefile:
> cmd/mdb/intel/amd64/zfs/Makefile:
> cmd/mdb/intel/ia32/libzpool/Makefile:
> cmd/mdb/intel/ia32/zfs/Makefile:
> cmd/mdb/sparc/v7/libzpool/Makefile:
> cmd/mdb/sparc/v9/libzpool/Makefile:
> cmd/mdb/sparc/v9/zfs/Makefile:
> cmd/zdb/Makefile.com:
> Silence E_STATIC_UNUSED lint warnings.
> 
> cmd/mdb/common/modules/zfs/zfs.c:
> uts/common/fs/zfs/zap_leaf.c:
> In zap_leaf() and zap_leaf_byteswap, now that the pointer alias
> field l_phys has been removed, use the db_data field in an on
> stack dmu_buf_t to point to the leaf's phys data.
> 
> uts/common/fs/zfs/dbuf.c:
> Remove the db_user_data_ptr_ptr field from dbuf and all logic
> to maintain it.
> 
> 
> uts/common/fs/zfs/dbuf.c:
> uts/common/fs/zfs/dnode.c:
> uts/common/fs/zfs/sys/dbuf.h:
> uts/common/fs/zfs/sys/dmu.h:
> uts/common/fs/zfs/dsl_dataset.c:
> uts/common/fs/zfs/dsl_dir.c:
> uts/common/fs/zfs/sa.c:
> uts/common/fs/zfs/zap.c:
> uts/common/fs/zfs/zap_micro.c:
> Modify the DMU buf user API to remove the ability to specify
> a db_data aliasing pointer (db_user_data_ptr_ptr).
> 
> cmd/zdb/zdb.c:
> uts/common/fs/zfs/dmu_diff.c:
> uts/common/fs/zfs/dmu_objset.c:
> uts/common/fs/zfs/dmu_send.c:
> uts/common/fs/zfs/dmu_traverse.c:
> uts/common/fs/zfs/dmu_tx.c:
> uts/common/fs/zfs/dsl_bookmark.c:
> uts/common/fs/zfs/dsl_dataset.c:
> uts/common/fs/zfs/dsl_deadlist.c:
> uts/common/fs/zfs/dsl_deleg.c:
> uts/common/fs/zfs/dsl_destroy.c:
> uts/common/fs/zfs/dsl_dir.c:
> uts/common/fs/zfs/dsl_pool.c:
> uts/common/fs/zfs/dsl_prop.c:
> uts/common/fs/zfs/dsl_scan.c:
> uts/common/fs/zfs/dsl_synctask.c:
> uts/common/fs/zfs/dsl_userhold.c:
> uts/common/fs/zfs/sa.c:
> uts/common/fs/zfs/spa.c:
> uts/common/fs/zfs/spa_history.c:
> uts/common/fs/zfs/zap.c:
> uts/common/fs/zfs/zap_leaf.c:
> uts/common/fs/zfs/zap_micro.c:
> uts/common/fs/zfs/zfs_ioctl.c:
> uts/common/fs/zfs/sys/dsl_dataset.h:
> uts/common/fs/zfs/sys/dsl_dir.h:
> uts/common/fs/zfs/sys/zap_impl.h:
> uts/common/fs/zfs/sys/zap_leaf.h:
> Create and use the new "phys data" accessor functions
> dsl_dir_phys(), dsl_dataset_phys(), zap_m_phys(),
> zap_f_phys(), and zap_leaf_phys().
> 
> uts/common/fs/zfs/sys/dsl_dataset.h:
> uts/common/fs/zfs/sys/dsl_dir.h:
> uts/common/fs/zfs/sys/zap_impl.h:
> uts/common/fs/zfs/sys/zap_leaf.h:
> Remove now unused "phys pointer" aliases to db->db_data
> from clients of the DMU buf user API.
> 
> 
> Diffs
> -
> 
>   usr/src/uts/common/fs/zfs/sys/zap_leaf.h 
> f6947a72d70e947c4aece66425cb16dd743ecee8 
>   usr/src/uts/common/fs/zfs/sys/dsl_dataset.h 
> ff90f8b439ccf7a137adcf10ba2c018bd9ae30aa 
>   usr/src/uts/common/fs/zfs/sys/dbuf.h 
> 8ca8753e5e6d55b13c6d9dde37b3c288feb35066 
>   usr/src/uts/common/fs/zfs/zap_micro.c 
> 59a9f970448a12b1e7de48f2b87c6eef4cc577eb 
>   usr/src/uts/common/fs/zfs/spa_history.c 
> ce64f70b28c39a485abe1848e9f988611bffce51 
>   usr/src/uts/common/fs/zfs/sa.c 8b3963aed9f57332de5b2b551492bae704f1a1ce 
>   usr/src/uts/common/fs/zfs/dsl_synctask.c 
> 8e09347678ddbdaf13f925a149122dd4209624d7 
>   usr/src/uts/common/fs/zfs/dsl_pool.c