Re: [developer] Proposal: Platform-specific ZFS properties

2019-02-26 Thread George Wilson
ertainly that is the case > for RackTop. > > > > Automatic conversion (or simply using a single shared value) only makes > sense when there is broad agreement about the property contents. Absent > that, namespace separation, **without** naïve attempts to convert, is > probably

[developer] Proposal: Platform-specific ZFS properties

2019-02-25 Thread George Wilson
A couple of months ago we discussed the issues surrounding zfs properties which have platform specific implementations. The example given was “sharenfs” which has platform-specific syntax and platform-specific implementations. Having platform-specific syntax for these types of properties means

[developer] Re: [openzfs/openzfs] 9751 Allocation throttling misplacing ditto blocks (#688)

2018-10-04 Thread George Wilson
grwilson commented on this pull request. > @@ -1039,6 +1039,13 @@ metaslab_group_allocatable(metaslab_group_t *mg, > metaslab_group_t *rotor, if (mg->mg_no_free_space) return (B_FALSE); + /* +* Relax allocation throttling

[developer] Re: [openzfs/openzfs] 9738 Fix third block copy allocations, broken at 9112. (#687)

2018-10-01 Thread George Wilson
grwilson 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/687#pullrequestreview-160374250 -- openzfs:

[developer] Re: [openzfs/openzfs] 9102 zfs should be able to initialize storage devices (#586)

2018-04-26 Thread George Wilson
@citrus-it in the past we've seen storage arrays that have zeroed out blocks behind the scenes leading to ZFS reported checksums. We wanted to have a default value that would not be confused with that type of corruption. We did make this a global parameter (`zfs_initialize_value`) so that it

[developer] Re: [openzfs/openzfs] 9102 zfs should be able to initialize storage devices (#586)

2018-03-10 Thread George Wilson
You could go through and `dd` to every disk before adding it to the pool and that is the common practice used by many. The problem that this is trying to solve is to be able to avoid the delay of having to wait for every disk to be pre-warmed before using it. Also, people often forget to "warm"

[developer] [openzfs/openzfs] 9102 zfs should be able to initialize storage devices (#586)

2018-03-09 Thread George Wilson
Reviewed by: John Wren Kennedy Reviewed by: Matthew Ahrens Reviewed by: Pavel Zakharov Reviewed by: Prakash Surya PROBLEM The first access to a block incurs a performance penalty on some

[developer] Re: [openzfs/openzfs] arc_loan_compressed_buf() can increment arc_loaned_bytes by the wrong value (#558)

2018-02-23 Thread George Wilson
grwilson 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/558#pullrequestreview-99102002 -- openzfs-developer

[developer] Re: [openzfs/openzfs] arc_loan_compressed_buf() can increment arc_loaned_bytes by the wrong value (#558)

2018-02-23 Thread George Wilson
grwilson requested changes on this pull request. This change looks good, can you also change `arc_loan_buf` to mimic this change. I think we should always rely on `arc_buf_size` to determine the correct value to pass to `arc_loaned_bytes_update` -- You are receiving this because you are

[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)

2018-02-12 Thread George Wilson
@grwilson pushed 1 commit. 1d7e2b0 all bits -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/505/files/445e55128ca0a1cf9b0a7013d996edd50da3a0f4..1d7e2b0d0cb2416d9838f0bd25fc174f31c4be09

[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)

2018-02-10 Thread George Wilson
@grwilson pushed 1 commit. 445e551 predefine child bits -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/505/files/b1342b226af1aa8a39cbcbe3b59a9270765c8ef2..445e55128ca0a1cf9b0a7013d996edd50da3a0f4

[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)

2018-02-09 Thread George Wilson
One of the guiding principles for zfs is simple administration and it seems like we're exposing way too many knobs to the administrator. These knobs expose the internals of the product. For example, if I want to clear the labels, why wouldn't I just run `zpool labelclear` and have the command

[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)

2018-02-08 Thread George Wilson
@youzhongyang @avg-I can you take another look to make sure I've address all of your concerns? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/505#issuecomment-364239237

[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)

2018-02-08 Thread George Wilson
Your comments are considered and I will be pushing a new commit with the updated logic. I've tried several variations to the suggestions to find which looks the best and has the least amount of impact. The suggested `ZIO_CHILD_BIT` option looks the best so I've gone with that option. -- You

[developer] Re: [openzfs/openzfs] 7584 Improve 'zpool labelclear' command (#424)

2018-02-07 Thread George Wilson
grwilson commented on this pull request. I have concerns about the ultimate goal for this command. It's unclear if we really want to "wipe" or just "forget" about this device. > @@ -709,8 +751,12 @@ zpool_do_labelclear(int argc, char **argv) } if (zpool_read_label(fd, ) != 0)

[developer] Re: [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)

2017-12-26 Thread George Wilson
grwilson commented on this pull request. > @@ -209,12 +209,13 @@ enum zio_flag { ZIO_FLAG_CANFAIL) enum zio_child { - ZIO_CHILD_VDEV = 0, - ZIO_CHILD_GANG, - ZIO_CHILD_DDT, - ZIO_CHILD_LOGICAL, - ZIO_CHILD_TYPES + ZIO_CHILD_VDEV = 1 << 0,

[developer] [openzfs/openzfs] 8857 zio_remove_child() panic due to already destroyed parent zio (#505)

2017-12-16 Thread George Wilson
PROBLEM It's possible for a parent zio to complete even though it has children which have not completed. This can result in the following panic: > $C ff01809128c0 vpanic() ff01809128e0 mutex_panic+0x58(fb94c904, ff597dde7f80) ff0180912950

[developer] Re: [openzfs/openzfs] fixed arc_cksum_is_equal() that doesn't take into account ABD-logic (#498)

2017-11-22 Thread George Wilson
grwilson commented on this pull request. > @@ -1567,7 +1569,7 @@ arc_cksum_is_equal(arc_buf_hdr_t *hdr, zio_t *zio) bzero((char *)cbuf + csize, HDR_GET_PSIZE(hdr) - csize); BTW, this wasn't part of your change but this also looks broken. I believe that we should be

[developer] Re: [openzfs/openzfs] fixed arc_cksum_is_equal() that doesn't take into account ABD-logic (#498)

2017-11-22 Thread George Wilson
grwilson approved this pull request. Changes look good. Make sure you open up an illumos bug for this this. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub:

Re: [developer] read-only pool, remove vdev

2017-10-17 Thread George Wilson
Andriy, I'm not familiar with spa_async_thread_vd() (I don't believe that is upstreamed yet) so I can't comment on what is the correct thing to do there. As for spa_async_thread(), I think we can ignore all tasks when the pool is read-only. You'll probably need to update the call to vdev_probe()

[developer] Re: [zfs] ZIO_IOCTL_PIPELINE excludes ZIO_STAGE_VDEV_IO_DONE

2017-09-16 Thread George Wilson
This > should work, but at the cost of the less uniform bio handling code. > > So, I wanted to see which of the solutions would be better from the point > of > view of the general zio pipeline architecture. > > Thank you. > > On 14/09/2017 18:48, George Wilson wr

[developer] Re: [zfs] ZIO_IOCTL_PIPELINE excludes ZIO_STAGE_VDEV_IO_DONE

2017-09-14 Thread George Wilson
Andriy, The ZIO_STAGE_VDEV_IO_DONE is not necessary for ZIO_IOCTL_PIPELINE and that's why it was removed. At least in illumos, there is no functional reason for calling it. To add it back would require a small amount of change which should not be a big deal. Can you provide some context around

[developer] Re: [openzfs/openzfs] 8473 scrub does not detect errors on active spares (#422)

2017-08-25 Thread George Wilson
grwilson 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/422#pullrequestreview-58790150 -- openzfs-developer

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

2017-08-25 Thread George Wilson
grwilson 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/409#pullrequestreview-58659280 -- openzfs-developer

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

2017-08-23 Thread George Wilson
grwilson commented on this pull request. There are lots of good comments in the pull request that aren't referenced in the actual code. It would be good to incorporate some of these as comments. /* * All forms of zfs create (create, mkdir, mkxattrdir, symlink) *

[developer] Re: [openzfs/openzfs] 7910 l2arc_write_buffers() may write beyond target_sz (#310)

2017-06-12 Thread George Wilson
grwilson 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/310#pullrequestreview-43429326 -- openzfs-developer

[developer] Re: [openzfs/openzfs] 7910 l2arc_write_buffers() may write beyond target_sz (#310)

2017-06-12 Thread George Wilson
grwilson commented on this pull request. > ARCSTAT_INCR(arcstat_l2_size, write_sz); - ARCSTAT_INCR(arcstat_l2_asize, write_asize); - vdev_space_update(dev->l2ad_vdev, write_asize, 0, 0); + ARCSTAT_INCR(arcstat_l2_asize, write_psize); I agree. These variables and their