[developer] [openzfs/openzfs] Race in ZFS parallel mount (#700)

[Andy Fiddaman]

There is a race condition in the ZFS parallel mount code which shows up if you 
have zoned datasets with the same mountpoint as those in the global zone. The 
code makes the incorrect assumption that mount points are globally unique. This 
results in mount failures during boot or pool import.

Take the following set of filesystems which have been sorted by mountpoint by 
the existing code - mountpoints followed by the filesystem in brackets. The 
letters at the left are my annotations.

With this set of filesystems, `zfs_foreach_mountpoint()` will create tasks 
_a-h_, task _g_ will create _A-D_ and _D_ will create _s-z_. The problem is 
that, for example, _t_ can run before _B_.

```
a   / (rpool/ROOT/r151024l)
b   / (rpool/ROOT/r151028.pre2)
c   / (rpool/ROOT/r151026.l1tf)
d   /data (data/zone/build/export)
e   /data (data/zone/reci/export)
f   /data (data)
g   /data (data/zone/ns1/export)
 A  /data/sendmail (data/zone/build/export/sendmail)
 B  /data/sendmail (data/sendmail)
 C  /data/sendmail (data/zone/ns1/export/sendmail)
 D  /data/sendmail (data/zone/reci/export/sendmail)
  s /data/sendmail/clientmqueue (data/zone/reci/export/sendmail/clientmqueue)
  t /data/sendmail/clientmqueue (data/sendmail/clientmqueue)
  u /data/sendmail/clientmqueue (data/zone/ns1/export/sendmail/clientmqueue)
  v /data/sendmail/clientmqueue (data/zone/build/export/sendmail/clientmqueue)
  w /data/sendmail/mqueue (data/zone/ns1/export/sendmail/mqueue)
  x /data/sendmail/mqueue (data/zone/build/export/sendmail/mqueue)
  y /data/sendmail/mqueue (data/sendmail/mqueue)
  z /data/sendmail/mqueue (data/zone/reci/export/sendmail/mqueue)
h   /home (data/home)
 Z  /home/af (data/home/af)
```

The fix I've gone for at the moment is to change the sort so that filesystems 
with the `zoned` attribute are sorted to the bottom. In the global zone, that 
results in the expected sorted list of filesystems and has the additional 
benefit that we can stop creating tasks once we see a zoned filesystem in the 
list. In a non-global zone, only the delegated filesystems are seen so the list 
is just traversed as normal.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Race in ZFS parallel mount

-- File Changes --

M usr/src/lib/libzfs/common/libzfs_mount.c (34)

-- Patch Links --

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/700

--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Tc23e72f4569fbbf8-M53cdab155df6b0ea9e5647e7
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

[developer] Re: [openzfs/openzfs] 9880 Race in ZFS parallel mount (#700)

[Jason King]

jasonbking requested changes on this pull request.

Just two cstyle nits, otherwise looks good.

> @@ -1166,6 +1176,14 @@ mountpoint_cmp(const void *arga, const void *argb)
const char *a = mounta;
const char *b = mountb;
boolean_t gota, gotb;
+   uint64_t zoneda, zonedb;
+
+   zoneda = zfs_prop_get_int(za, ZFS_PROP_ZONED);
+   zonedb = zfs_prop_get_int(zb, ZFS_PROP_ZONED);
+   if (zoneda && !zonedb)
+   return 1;

Cstyle nit -- `return (1);`

> @@ -1166,6 +1176,14 @@ mountpoint_cmp(const void *arga, const void *argb)
const char *a = mounta;
const char *b = mountb;
boolean_t gota, gotb;
+   uint64_t zoneda, zonedb;
+
+   zoneda = zfs_prop_get_int(za, ZFS_PROP_ZONED);
+   zonedb = zfs_prop_get_int(zb, ZFS_PROP_ZONED);
+   if (zoneda && !zonedb)
+   return 1;
+   if (!zoneda && zonedb)
+   return -1;

Similar to above -- `return (-1);`

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/700#pullrequestreview-163384948
--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Teaa4e2ae04d2f095-M7b573b4f0fbbfb87c093ff63
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

[developer] Re: [openzfs/openzfs] 9880 Race in ZFS parallel mount (#700)

[Andy Fiddaman]

citrus-it commented on this pull request.



> @@ -1166,6 +1176,14 @@ mountpoint_cmp(const void *arga, const void *argb)
const char *a = mounta;
const char *b = mountb;
boolean_t gota, gotb;
+   uint64_t zoneda, zonedb;
+
+   zoneda = zfs_prop_get_int(za, ZFS_PROP_ZONED);
+   zonedb = zfs_prop_get_int(zb, ZFS_PROP_ZONED);
+   if (zoneda && !zonedb)
+   return 1;

Thanks - always forget that strange convention. I've just run pbchk and these 
were the only two things reported - fixed and pushed.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/700#discussion_r224104654
--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Teaa4e2ae04d2f095-Mb03c98189b300660f0a18a25
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

[developer] Re: [openzfs/openzfs] 9880 Race in ZFS parallel mount (#700)

[Jason King]

jasonbking approved this pull request.





-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/700#pullrequestreview-163395125
--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Teaa4e2ae04d2f095-M789d501e7ae3f306f73cd1b8
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

[developer] Re: [openzfs/openzfs] 9880 Race in ZFS parallel mount (#700)

[Matthew Ahrens]

@sebroy could you take a look if you have time?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/700#issuecomment-428634013
--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Teaa4e2ae04d2f095-Mabd2a6fe95251dedbdfaa4dc
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

[developer] ZFS TRIM code review

[Jerry Jelinek]

I have taken the Nexenta TRIM work that was originally out for code review
last year and updated it to work with the latest version of ZFS. The code
passes the new trim tests and I have done some additional manual testing
with DTrace to verify that the trim requests pass down into the scsi driver
and that the proper unmapping is taking place. We have not run this in
production yet. I currently have a code review up at:

https://cr.joyent.us/#/c/4929/

Anyone with a github account should be able to set themselves up to comment
on this CR, or you can just send me feedback in email. I know this is a
large review, but it has already gotten a lot of feedback last year when
Nexenta put it out, and hopefully people will be able to take another look
so we can get this integrated.

The following is a summary of the changes I made from the original 2017
Nexenta work to bring it up to date with the latest ZFS code.

The names of many of the AVL trees were changed with commit 86714001. Here
is the mapping between what was in the Nexenta code and what the new names
are:
ms_tree-> ms_allocatable
ms_freeingtree  -> ms_freeing
ms_freedtree-> ms_freed
ms_defertree-> ms_defer
ms_size_tree-> ms_allocatable_by_size

usr/src/uts/common/fs/zfs/metaslab.c
range_tree_create() calls had the wrong number of args, lock parameter no
longer used.

metaslab_trim_remove changes; in orig diff it was called in:
metaslab_load range_tree_walk defertree name changed to
defer
metaslab_block_alloc  ok
metaslab_claim_dvathis has heavy modifications. now
  -> metaslab_claim_impl ->
metaslab_claim_concrete
  I added the metaslab_trim_remove in
metaslab_claim_concrete

In orig patch, had spa_get_auto_trim() and metaslab_trim_add() calls in
metaslab_free_dva()
metaslab_free_dva has heavy modifications; now the call path looks like:
-> metaslab_free_impl -> metaslab_free_concrete

I don't think the spa_get_auto_trim() and metaslab_trim_add() should be in
metaslab_free_concrete() though, due to new checkpoint handling.

In the Nexenta patch, metaslab_free calls metaslab_free_dva but in
metaslab_free for the current code, it calls metaslab_unalloc_dva for the
"now" case - this matches the "now" case for the patched
metaslab_trim_add() code.  metaslab_unalloc_dva doesn't exist in the patch,
but this looks like the right place to add the spa_get_auto_trim() and
metaslab_trim_add() at end.

In metaslab_check_free(), I did not include the debugging code which
depended on the debug changes we omitted from the driver changes we've
already committed.

usr/src/uts/common/fs/zfs/range_tree.c
332 Removed invalid assert.

usr/src/uts/common/fs/zfs/spa.c
spa_load_impl - changes here added to spa_ld_get_props instead
spa_load_impl - added 3rd arg to spa_dir_prop calls
Fixed spa_event_notify parameters throughout.

usr/src/uts/common/fs/zfs/spa_misc.c
Some minor change, but nothing substantial.

usr/src/uts/common/fs/zfs/vdev_raidz.c
Fixed up vdev_raidz_map_alloc.
Added correct ops initialization.
In vdev_raidz_map_free(), fixed code to use the new abd_free() behavior
which was added in commit 4ee0199ec (due to backout and reimpl). I also
removed the size variable which is no longer used, but wasn't cleaned up
from this commit.

usr/src/uts/common/fs/zfs/vdev_indirect.c
Was not included in Nexenta patch. Added correct ops initialization.

usr/src/uts/common/fs/zfs/zio.c
Removed dfl_ck_func related debug code.

usr/src/lib/libzpool
Fixed Makefile.com
KERNEL_OBJS was including taskq.o which didn't build right in user-land,
but was unnecessary so removed it.

zpool.1m
Fixed all of the new format errors highlighted during the build.

Thanks,
Jerry

--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Tce91d507e14ab14a-M5f620ce6ea23ba58411413ed
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

Re: [developer] ZFS TRIM code review

[Igor Kozhukhov]

Hi Jerry,

thanks you very much!

do you have changes on illumos-joyent tree for cherry-pick ?

-Igor


> On Oct 10, 2018, at 7:26 PM, Jerry Jelinek  wrote:
> 
> I have taken the Nexenta TRIM work that was originally out for code review 
> last year and updated it to work with the latest version of ZFS. The code 
> passes the new trim tests and I have done some additional manual testing with 
> DTrace to verify that the trim requests pass down into the scsi driver and 
> that the proper unmapping is taking place. We have not run this in production 
> yet. I currently have a code review up at:
> 
> https://cr.joyent.us/#/c/4929/ 
> 
> Anyone with a github account should be able to set themselves up to comment 
> on this CR, or you can just send me feedback in email. I know this is a large 
> review, but it has already gotten a lot of feedback last year when Nexenta 
> put it out, and hopefully people will be able to take another look so we can 
> get this integrated.
> 
> The following is a summary of the changes I made from the original 2017 
> Nexenta work to bring it up to date with the latest ZFS code.
> 
> The names of many of the AVL trees were changed with commit 86714001. Here is 
> the mapping between what was in the Nexenta code and what the new names are:
> ms_tree-> ms_allocatable
> ms_freeingtree  -> ms_freeing
> ms_freedtree-> ms_freed
> ms_defertree-> ms_defer
> ms_size_tree-> ms_allocatable_by_size
> 
> usr/src/uts/common/fs/zfs/metaslab.c
> range_tree_create() calls had the wrong number of args, lock parameter no 
> longer used.
> 
> metaslab_trim_remove changes; in orig diff it was called in:
> metaslab_load range_tree_walk defertree name changed to 
> defer
> metaslab_block_alloc  ok
> metaslab_claim_dvathis has heavy modifications. now
>   -> metaslab_claim_impl -> 
> metaslab_claim_concrete
>   I added the metaslab_trim_remove in 
> metaslab_claim_concrete
> 
> In orig patch, had spa_get_auto_trim() and metaslab_trim_add() calls in 
> metaslab_free_dva()
> metaslab_free_dva has heavy modifications; now the call path looks like:
> -> metaslab_free_impl -> metaslab_free_concrete
> 
> I don't think the spa_get_auto_trim() and metaslab_trim_add() should be in 
> metaslab_free_concrete() though, due to new checkpoint handling.
> 
> In the Nexenta patch, metaslab_free calls metaslab_free_dva but in 
> metaslab_free for the current code, it calls metaslab_unalloc_dva for the 
> "now" case - this matches the "now" case for the patched metaslab_trim_add() 
> code.  metaslab_unalloc_dva doesn't exist in the patch, but this looks like 
> the right place to add the spa_get_auto_trim() and metaslab_trim_add() at end.
> 
> In metaslab_check_free(), I did not include the debugging code which depended 
> on the debug changes we omitted from the driver changes we've already 
> committed.
> 
> usr/src/uts/common/fs/zfs/range_tree.c
> 332 Removed invalid assert.
> 
> usr/src/uts/common/fs/zfs/spa.c
> spa_load_impl - changes here added to spa_ld_get_props instead
> spa_load_impl - added 3rd arg to spa_dir_prop calls
> Fixed spa_event_notify parameters throughout.
> 
> usr/src/uts/common/fs/zfs/spa_misc.c
> Some minor change, but nothing substantial.
> 
> usr/src/uts/common/fs/zfs/vdev_raidz.c
> Fixed up vdev_raidz_map_alloc.
> Added correct ops initialization.
> In vdev_raidz_map_free(), fixed code to use the new abd_free() behavior which 
> was added in commit 4ee0199ec (due to backout and reimpl). I also removed the 
> size variable which is no longer used, but wasn't cleaned up from this commit.
> 
> usr/src/uts/common/fs/zfs/vdev_indirect.c
> Was not included in Nexenta patch. Added correct ops initialization.
> 
> usr/src/uts/common/fs/zfs/zio.c
> Removed dfl_ck_func related debug code.
> 
> usr/src/lib/libzpool
> Fixed Makefile.com
> KERNEL_OBJS was including taskq.o which didn't build right in user-land, but 
> was unnecessary so removed it.
> 
> zpool.1m
> Fixed all of the new format errors highlighted during the build.
> 
> Thanks,
> Jerry
> 
> illumos  / illumos-developer / see 
> discussions  + participants 
>  + delivery options 
> Permalink 
> 

--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Tce91d507e14ab14a-M7c2699f6e9536d04070abf4b
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

[developer] Re: [openzfs/openzfs] Trying again with sequential scan -- this is simply (#648)

[Matthew Ahrens]

This is another version of porting the FreeBSD version of this to illumos: 
https://www.illumos.org/rb/r/1226/

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/648#issuecomment-428646240
--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/T6e124cbb2c8562a8-Mb7e01847cd8580d207af96a2
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

[developer] Re: [openzfs/openzfs] Trying again with sequential scan -- this is simply (#648)

[tsoome]

> This is another version of porting the FreeBSD version of this to illumos: 
> https://www.illumos.org/rb/r/1226/

Note it does depend on 2 other commits:

https://www.illumos.org/rb/r/1224/
and
https://www.illumos.org/rb/r/1225/

The bundle itself seems to be stable enough to run resilver on mirror, raidz 
and scrubs. The only down side is that I did skip some ztest scripts, but it 
should be easy task to bring them up.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/648#issuecomment-428651357
--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/T6e124cbb2c8562a8-Mc193709ab008f26c4487f97c
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

[developer] Re: [openzfs/openzfs] 8423 Implement large_dnode pool feature (#409)

[Matthew Ahrens]

Updated version of this is here: https://www.illumos.org/rb/r/1224/

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/409#issuecomment-428652593
--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Tfefe6318017dd4c7-Mc1827d96b013b4335f0785c0
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

[developer] Re: [openzfs/openzfs] 6363 Add UNMAP/TRIM functionality to ZFS and Illumos (#172)

[Matthew Ahrens]

There's an updated version of this at https://cr.joyent.us/#/c/4929, but it 
doesn't currently seem to include all the changes from this PR.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/172#issuecomment-428668366
--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Ta7c0d14ddf82209a-M947f539c150a86f06c8d72d7
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

Re: [developer] ZFS TRIM code review

[Matthew Ahrens]

Thanks for pushing this forward, Jerry.  I have a few very high level
questions / points:

It looks like the changes that Saso made in response to the code review
feedback that I gave in April-May 2017 are not part of your proposed
changes.  I'd like to see those included.
https://github.com/openzfs/openzfs/pull/172/commits

Do you have any performance results to share, even preliminary?  A lot of
the complexity of this change is due to performance concerns (e.g.
consolidating trim segments across multiple TXG's).  It would be good to
verify that this complexity is worth it.

--matt

On Wed, Oct 10, 2018 at 9:27 AM Jerry Jelinek 
wrote:

> I have taken the Nexenta TRIM work that was originally out for code review
> last year and updated it to work with the latest version of ZFS. The code
> passes the new trim tests and I have done some additional manual testing
> with DTrace to verify that the trim requests pass down into the scsi driver
> and that the proper unmapping is taking place. We have not run this in
> production yet. I currently have a code review up at:
>
> https://cr.joyent.us/#/c/4929/
>
> Anyone with a github account should be able to set themselves up to
> comment on this CR, or you can just send me feedback in email. I know this
> is a large review, but it has already gotten a lot of feedback last year
> when Nexenta put it out, and hopefully people will be able to take another
> look so we can get this integrated.
>
> The following is a summary of the changes I made from the original 2017
> Nexenta work to bring it up to date with the latest ZFS code.
>
> The names of many of the AVL trees were changed with commit 86714001. Here
> is the mapping between what was in the Nexenta code and what the new names
> are:
> ms_tree-> ms_allocatable
> ms_freeingtree  -> ms_freeing
> ms_freedtree-> ms_freed
> ms_defertree-> ms_defer
> ms_size_tree-> ms_allocatable_by_size
>
> usr/src/uts/common/fs/zfs/metaslab.c
> range_tree_create() calls had the wrong number of args, lock parameter no
> longer used.
>
> metaslab_trim_remove changes; in orig diff it was called in:
> metaslab_load range_tree_walk defertree name changed
> to defer
> metaslab_block_alloc  ok
> metaslab_claim_dvathis has heavy modifications. now
>   -> metaslab_claim_impl ->
> metaslab_claim_concrete
>   I added the metaslab_trim_remove in
> metaslab_claim_concrete
>
> In orig patch, had spa_get_auto_trim() and metaslab_trim_add() calls in
> metaslab_free_dva()
> metaslab_free_dva has heavy modifications; now the call path looks
> like:
> -> metaslab_free_impl -> metaslab_free_concrete
>
> I don't think the spa_get_auto_trim() and metaslab_trim_add() should be in
> metaslab_free_concrete() though, due to new checkpoint handling.
>
> In the Nexenta patch, metaslab_free calls metaslab_free_dva but in
> metaslab_free for the current code, it calls metaslab_unalloc_dva for the
> "now" case - this matches the "now" case for the patched
> metaslab_trim_add() code.  metaslab_unalloc_dva doesn't exist in the patch,
> but this looks like the right place to add the spa_get_auto_trim() and
> metaslab_trim_add() at end.
>
> In metaslab_check_free(), I did not include the debugging code which
> depended on the debug changes we omitted from the driver changes we've
> already committed.
>
> usr/src/uts/common/fs/zfs/range_tree.c
> 332 Removed invalid assert.
>
> usr/src/uts/common/fs/zfs/spa.c
> spa_load_impl - changes here added to spa_ld_get_props instead
> spa_load_impl - added 3rd arg to spa_dir_prop calls
> Fixed spa_event_notify parameters throughout.
>
> usr/src/uts/common/fs/zfs/spa_misc.c
> Some minor change, but nothing substantial.
>
> usr/src/uts/common/fs/zfs/vdev_raidz.c
> Fixed up vdev_raidz_map_alloc.
> Added correct ops initialization.
> In vdev_raidz_map_free(), fixed code to use the new abd_free() behavior
> which was added in commit 4ee0199ec (due to backout and reimpl). I also
> removed the size variable which is no longer used, but wasn't cleaned up
> from this commit.
>
> usr/src/uts/common/fs/zfs/vdev_indirect.c
> Was not included in Nexenta patch. Added correct ops initialization.
>
> usr/src/uts/common/fs/zfs/zio.c
> Removed dfl_ck_func related debug code.
>
> usr/src/lib/libzpool
> Fixed Makefile.com
> KERNEL_OBJS was including taskq.o which didn't build right in user-land,
> but was unnecessary so removed it.
>
> zpool.1m
> Fixed all of the new format errors highlighted during the build.
>
> Thanks,
> Jerry
>
> *illumos * / illumos-developer / see
> discussions  + participants
>  + delivery options
>  Permalink
> 
>

[developer] Re: [openzfs/openzfs] 6363 Add UNMAP/TRIM functionality to ZFS and Illumos (#172)

[Jerry Jelinek]

The changes at https://cr.joyent.us/#/c/4929 do not include everything from the 
original PR. The driver changes to support unmap were split out into a separate 
commit and those have already integrated into illumos as commit 047c81d31. The 
current set of changes is the rest of the changes for TRIM itself.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/172#issuecomment-428683260
--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Ta7c0d14ddf82209a-M087be63ec3beaeef9cab0100
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

Re: [developer] ZFS TRIM code review

[Jerry Jelinek]

Igor,

These changes haven't integrated into illumos-joyent yet, so there is no
patch to cherry pick, except from the CR itself.

Thanks,
Jerry


On Wed, Oct 10, 2018 at 10:29 AM, Igor Kozhukhov  wrote:

> Hi Jerry,
>
> thanks you very much!
>
> do you have changes on illumos-joyent tree for cherry-pick ?
>
> -Igor
>
>
> On Oct 10, 2018, at 7:26 PM, Jerry Jelinek 
> wrote:
>
> I have taken the Nexenta TRIM work that was originally out for code review
> last year and updated it to work with the latest version of ZFS. The code
> passes the new trim tests and I have done some additional manual testing
> with DTrace to verify that the trim requests pass down into the scsi driver
> and that the proper unmapping is taking place. We have not run this in
> production yet. I currently have a code review up at:
>
> https://cr.joyent.us/#/c/4929/
>
> Anyone with a github account should be able to set themselves up to
> comment on this CR, or you can just send me feedback in email. I know this
> is a large review, but it has already gotten a lot of feedback last year
> when Nexenta put it out, and hopefully people will be able to take another
> look so we can get this integrated.
>
> The following is a summary of the changes I made from the original 2017
> Nexenta work to bring it up to date with the latest ZFS code.
>
> The names of many of the AVL trees were changed with commit 86714001. Here
> is the mapping between what was in the Nexenta code and what the new names
> are:
> ms_tree-> ms_allocatable
> ms_freeingtree  -> ms_freeing
> ms_freedtree-> ms_freed
> ms_defertree-> ms_defer
> ms_size_tree-> ms_allocatable_by_size
>
> usr/src/uts/common/fs/zfs/metaslab.c
> range_tree_create() calls had the wrong number of args, lock parameter no
> longer used.
>
> metaslab_trim_remove changes; in orig diff it was called in:
> metaslab_load range_tree_walk defertree name changed
> to defer
> metaslab_block_alloc  ok
> metaslab_claim_dvathis has heavy modifications. now
>   -> metaslab_claim_impl ->
> metaslab_claim_concrete
>   I added the metaslab_trim_remove in
> metaslab_claim_concrete
>
> In orig patch, had spa_get_auto_trim() and metaslab_trim_add() calls in
> metaslab_free_dva()
> metaslab_free_dva has heavy modifications; now the call path looks
> like:
> -> metaslab_free_impl -> metaslab_free_concrete
>
> I don't think the spa_get_auto_trim() and metaslab_trim_add() should be in
> metaslab_free_concrete() though, due to new checkpoint handling.
>
> In the Nexenta patch, metaslab_free calls metaslab_free_dva but in
> metaslab_free for the current code, it calls metaslab_unalloc_dva for the
> "now" case - this matches the "now" case for the patched
> metaslab_trim_add() code.  metaslab_unalloc_dva doesn't exist in the patch,
> but this looks like the right place to add the spa_get_auto_trim() and
> metaslab_trim_add() at end.
>
> In metaslab_check_free(), I did not include the debugging code which
> depended on the debug changes we omitted from the driver changes we've
> already committed.
>
> usr/src/uts/common/fs/zfs/range_tree.c
> 332 Removed invalid assert.
>
> usr/src/uts/common/fs/zfs/spa.c
> spa_load_impl - changes here added to spa_ld_get_props instead
> spa_load_impl - added 3rd arg to spa_dir_prop calls
> Fixed spa_event_notify parameters throughout.
>
> usr/src/uts/common/fs/zfs/spa_misc.c
> Some minor change, but nothing substantial.
>
> usr/src/uts/common/fs/zfs/vdev_raidz.c
> Fixed up vdev_raidz_map_alloc.
> Added correct ops initialization.
> In vdev_raidz_map_free(), fixed code to use the new abd_free() behavior
> which was added in commit 4ee0199ec (due to backout and reimpl). I also
> removed the size variable which is no longer used, but wasn't cleaned up
> from this commit.
>
> usr/src/uts/common/fs/zfs/vdev_indirect.c
> Was not included in Nexenta patch. Added correct ops initialization.
>
> usr/src/uts/common/fs/zfs/zio.c
> Removed dfl_ck_func related debug code.
>
> usr/src/lib/libzpool
> Fixed Makefile.com
> KERNEL_OBJS was including taskq.o which didn't build right in user-land,
> but was unnecessary so removed it.
>
> zpool.1m
> Fixed all of the new format errors highlighted during the build.
>
> Thanks,
> Jerry
>
>
> *openzfs * / openzfs-developer / see
> discussions  + participants
>  + delivery options
>  Permalink
> 
>

--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Tce91d507e14ab14a-Mc7c56f4df73ce49b7cd4ee66
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

[developer] Re: [openzfs/openzfs] 9847 leaking dd_clones (DMU_OT_DSL_CLONES) objects (#696)

[Brian Behlendorf]

behlendorf approved this pull request.

The improved leak detection is nice!

> @@ -4430,6 +4499,161 @@ verify_checkpoint(spa_t *spa)
return (error);
 }
 
+/* ARGSUSED */
+static void
+mos_leaks_cb(void *arg, uint64_t start, uint64_t size)
+{
+   for (uint64_t i = start; i < size; i++) {
+   (void) printf("MOS object %llu referenced but not allocated\n",
+   (long long)i);

nit: `u_longlong_t` is used for this throughout `zdb` to match the format 
specifier.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/696#pullrequestreview-163507349
--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/Te6b30b1c1b31d15c-M35b069d24ef78abc7120b319
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription

Re: [developer] ZFS TRIM code review

[Jerry Jelinek]

Matt,

I started from what I thought was the final Nexenta patch, but I guess that
did not include your feedback. Sorry about that, I'll go back and pull that
in. We (Joyent) don't have any performance data ourselves yet. I'll see if
Nexenta has any data they could share.

Thanks,
Jerry


On Wed, Oct 10, 2018 at 12:05 PM, Matthew Ahrens 
wrote:

> Thanks for pushing this forward, Jerry.  I have a few very high level
> questions / points:
>
> It looks like the changes that Saso made in response to the code review
> feedback that I gave in April-May 2017 are not part of your proposed
> changes.  I'd like to see those included. https://github.com/
> openzfs/openzfs/pull/172/commits
>
> Do you have any performance results to share, even preliminary?  A lot of
> the complexity of this change is due to performance concerns (e.g.
> consolidating trim segments across multiple TXG's).  It would be good to
> verify that this complexity is worth it.
>
> --matt
>
> On Wed, Oct 10, 2018 at 9:27 AM Jerry Jelinek 
> wrote:
>
>> I have taken the Nexenta TRIM work that was originally out for code
>> review last year and updated it to work with the latest version of ZFS. The
>> code passes the new trim tests and I have done some additional manual
>> testing with DTrace to verify that the trim requests pass down into the
>> scsi driver and that the proper unmapping is taking place. We have not run
>> this in production yet. I currently have a code review up at:
>>
>> https://cr.joyent.us/#/c/4929/
>>
>> Anyone with a github account should be able to set themselves up to
>> comment on this CR, or you can just send me feedback in email. I know this
>> is a large review, but it has already gotten a lot of feedback last year
>> when Nexenta put it out, and hopefully people will be able to take another
>> look so we can get this integrated.
>>
>> The following is a summary of the changes I made from the original 2017
>> Nexenta work to bring it up to date with the latest ZFS code.
>>
>> The names of many of the AVL trees were changed with commit 86714001.
>> Here is the mapping between what was in the Nexenta code and what the new
>> names are:
>> ms_tree-> ms_allocatable
>> ms_freeingtree  -> ms_freeing
>> ms_freedtree-> ms_freed
>> ms_defertree-> ms_defer
>> ms_size_tree-> ms_allocatable_by_size
>>
>> usr/src/uts/common/fs/zfs/metaslab.c
>> range_tree_create() calls had the wrong number of args, lock parameter no
>> longer used.
>>
>> metaslab_trim_remove changes; in orig diff it was called in:
>> metaslab_load range_tree_walk defertree name changed
>> to defer
>> metaslab_block_alloc  ok
>> metaslab_claim_dvathis has heavy modifications. now
>>   -> metaslab_claim_impl ->
>> metaslab_claim_concrete
>>   I added the metaslab_trim_remove in
>> metaslab_claim_concrete
>>
>> In orig patch, had spa_get_auto_trim() and metaslab_trim_add() calls in
>> metaslab_free_dva()
>> metaslab_free_dva has heavy modifications; now the call path looks
>> like:
>> -> metaslab_free_impl -> metaslab_free_concrete
>>
>> I don't think the spa_get_auto_trim() and metaslab_trim_add() should be
>> in metaslab_free_concrete() though, due to new checkpoint handling.
>>
>> In the Nexenta patch, metaslab_free calls metaslab_free_dva but in
>> metaslab_free for the current code, it calls metaslab_unalloc_dva for the
>> "now" case - this matches the "now" case for the patched
>> metaslab_trim_add() code.  metaslab_unalloc_dva doesn't exist in the patch,
>> but this looks like the right place to add the spa_get_auto_trim() and
>> metaslab_trim_add() at end.
>>
>> In metaslab_check_free(), I did not include the debugging code which
>> depended on the debug changes we omitted from the driver changes we've
>> already committed.
>>
>> usr/src/uts/common/fs/zfs/range_tree.c
>> 332 Removed invalid assert.
>>
>> usr/src/uts/common/fs/zfs/spa.c
>> spa_load_impl - changes here added to spa_ld_get_props instead
>> spa_load_impl - added 3rd arg to spa_dir_prop calls
>> Fixed spa_event_notify parameters throughout.
>>
>> usr/src/uts/common/fs/zfs/spa_misc.c
>> Some minor change, but nothing substantial.
>>
>> usr/src/uts/common/fs/zfs/vdev_raidz.c
>> Fixed up vdev_raidz_map_alloc.
>> Added correct ops initialization.
>> In vdev_raidz_map_free(), fixed code to use the new abd_free() behavior
>> which was added in commit 4ee0199ec (due to backout and reimpl). I also
>> removed the size variable which is no longer used, but wasn't cleaned up
>> from this commit.
>>
>> usr/src/uts/common/fs/zfs/vdev_indirect.c
>> Was not included in Nexenta patch. Added correct ops initialization.
>>
>> usr/src/uts/common/fs/zfs/zio.c
>> Removed dfl_ck_func related debug code.
>>
>> usr/src/lib/libzpool
>> Fixed Makefile.com
>> KERNEL_OBJS was including taskq.o which didn't build right in user-land,
>> but was unnecessary so removed it.
>>
>> zpool.1m
>> Fixed all of

[developer] Re: [openzfs/openzfs] 9466 add JSON output support to channel programs (#619)

[Prakash Surya]

Closed #619 via 62d2a09b3a37b22b70bfec78da162ca55224c072.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/619#event-1896758749
--
openzfs: openzfs-developer
Permalink: 
https://openzfs.topicbox.com/groups/developer/T0465226805877059-M343432dded6222a560f05665
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription