I forgot to add, even with D7179, it seems still be a good idea to ensure that we are not trying to rename a zvol which is still opened somewhere.

I was not aware of D7179, thanks to pointing it. I tried it in some simple case and it seems to work pretty well.
Moving zfsvfs_update_fromname() and zvol_rename_minors() from txg_sync to the ioctl part was something I considered but I was unsure of the result because of the part of the rename still in txg_sync but after a second look it seems fine.
About the race, maybe adding a way to lock a specific zvol by adding something in zvol_state (from zvol.c) and holding the lock on the specific zvol during a zfs_ioc_rename could be enough to fix the issue ? so we won't have concurrent rename on the same zvol ? or am I missing another race here ?
My feeling is that we have now a second obvious deadlock on zfs rename in stable/11 and it would be better to use your patch now with (or maybe even without) a lock on the zvol than what we have now.

In D13088#273183, @allanjude wrote:

if (zpool_read_label(fd, &config) != 0 && errno == ENOMEM) {
        (void) no_memory(hdl);
        return (-1);
}

would be:

if (zpool_read_label(fd, &config) == -1) {
        (void) no_memory(hdl);
        return (-1);
}

Maybe instead:

if (zpool_read_label(fd, &config) != 0 && errno == ENOMEM) {
        (void) no_memory(hdl);
        /* After calling no_memory(), we can reset errno */
        errno = 0;
        return (-1);
}

Thanks for your comments Allan and Andriy.
Concerning the errno set to 12 (ENOMEM), it's effectively set by the ioctl ZFS_IOC_POOL_STATS which is first called with a too small nvlist.
I'm attaching the conditional watchpoint where it happens.

In D13093#272417, @imp wrote:

In D13093#272254, @nikita_elyzion.net wrote:

Now that you point it, yes I missed that part!
It will be difficult for me to test it, do you want that I path ata_da.c anyway ?

The 4k quirk is in serious need of being revisited because we shouldn't have to do this except for a tiny tiny fraction of drives.

Now that you point it, yes I missed that part!
It will be difficult for me to test it, do you want that I path ata_da.c anyway ?

I took some time to dig more in the code of arc_adjust()/arc_shrink()/arc_kmem_reap_now() and the pagedaemon/vmmeter and the suggestion of avg seems good.
freemem is actually #define freemem vm_cnt.v_free_count (maybe we can use another stats from vmmeter/pagedaemon to replace the needfree by another define ?)
I have also improved my test to reproduce more easily the issue and removing the needfree variable seems good.

In D12181#254016, @trasz wrote:

I like the idea - this would allow us to say "no more than one initiator at a time", greatly simplifying host standby (cluster) setups.

That said, from my understanding there's a race condition - if two initiators try to connect at the same time, they will both make it through the login phase, since each ctld(8) child doesn't know about the other. I believe the check should happen in the kernel, during handoff. The iSCSI spec already contains a mechanism for the target to request logout (or just drop the connection) using the Asynchronous Message PDU; I'd expect initiators to be able to handle it gracefully, as it's mandatory part of the spec - although I've never tried that myself.

I'd also suggest a different name; other ctl.conf parameters don't use underscores, and mostly avoid contractions. How about "connections-max"?

And yeah, the man page :-)

Updating the diff, I forgot to push a typo fix from a repo.

In D12163#252767, @avg wrote:

Can you try to experiment with removing needfree altogether?
I mean both the code that sets it (in arc_lowmwem and in arc_reclaim_thread) and the code that uses it (in arc_available_memory and in arc_reclaim_thread).
FreeBSD needfree does not appear to be very useful.

In D12144#252284, @manu wrote:

In D12144#252281, @bapt wrote:

Looks good to me, but I will wait for people with more internal knowledge than I have about ZFS to step in anyway

Same for me.

Nikita: Could you provide numbers ? (i.e.: when does one need to change the default value)

In D12144#252121, @asomers wrote:

However you generated this DR, Phabricator didn't get enough context info. The best way to start a code review is to use php5-arcanist according to the instructions at https://wiki.freebsd.org/Phabricator . But if you must use the web interface, remember to use "-D99999" when creating the diff to ensure that maximal context is available.

The 'q' in {q:led/locate} is necessary for the json output at least, without that we have : "led":locate instead of "led":"locate"

Fixed bapt and allanjude comments.
I'm keeping .Dd as is because bapt seems to prefer to update it at commit time.

updating with jhb comments