We shouldn't make the code more complex and less readable just to optimize for some rare edge cases. If someone disables block cloning, they are most likely not too concerned with performance and trying to avoid an extra vnode lock at the expense of code readability is, in my opinion, the wrong approach.

Thank you for the fix!

In D38814#884116, @mjg wrote:

I think this is a step backwards and it is not what I suggested.

For example now any fs which does not provide it's own primitive will take a trip through returning EOPNOTSUPP when it can be avoided.

I suggested everyone handling as much as they can and otherwise punting to vn_generic_copy_file_range or a differently named fallback with the same result.

To further illustrate the win in nullfs case, consider the following with 2 nullfs mount points:

1. we land in null_copy_file_range or whatever
2. both target vnodes get "unwrapped"
3. the vop gets invoked in vnode1 with both as arguments and we are done here

With the proposed patch it will be instead:

1. we land in null_copy_file_range or whatever
2. both target vnodes get found + refed
3. the vop returns EOPNOTSUPP
4. the routine unrefs vnodes and returns
5. ... now vn_generic_copy_file_range will keep calling VOP_READ and VOP_WRITE while looking up + refing (and unrefing) vnodes for each call

In D38814#883415, @asomers wrote:

LGTM. But is there any file system that you expect to support cross-mountpoint copy_file_range ?

In D38803#883156, @mjg wrote:

My grep fails to find any existing custom routines, I'm guessing one for zfs will be showing up later. I think this also means there is no worry of any backwards-incompatible breakage.

Use larger context for the diff to include the entire function.

After discussion with mav@, add ASCQs for PARAMETERS CHANGED and MODE PARAMETERS CHANGED, similar to Linux and Windows. Also add a comment describing those values.

In D21807#476635, @mav wrote:

In D21807#476430, @pjd wrote:

I see what you mean. Others implement this the same way:
Linux: https://github.com/torvalds/linux/blob/master/drivers/scsi/virtio_scsi.c#L304
Windows: https://github.com/virtio-win/kvm-guest-drivers-windows/blob/master/vioscsi/vioscsi.c#L1621

I see. Though they both handle 3 statuses, not just one.

In D21807#476253, @mav wrote:

In D21807#476160, @pjd wrote:

What do you mean by 'too strict parsing of ASC/ASCQ'?

I mean that considering the codes are passed and they are called ASC/ASCQ in specification, there potentially may be other ones too, for which CAM may have own more specific handling ideas, and I believe it is not a SIM driver duty to handle details of SCSI protocol. I don't object your implementation, just thinking out loud whether it could it be done better/otherwise.

In D21807#476017, @mav wrote:

This virtio API looks ugly. They conform themselves that device should also report normal UNIT ATTENTION with the same status, which is already handled by CAM DA driver. If we assume that device does generate UNIT ATTENTION, then we should be able to make cam send TUR request to see it by using AC_SCSI_AEN async. Alternatively, it is possible to simulate UNIT ATTENTION receive by using AC_UNIT_ATTENTION with fake CCB, including ASC/ASCQ received from virtio API. Your method should also work, but what I don\t very like it too strict parsing ASC/ASCQ and sending not exactly correct async.

In D19854#426191, @ngie wrote:

In D19854#426189, @pjd wrote:

Do you mind elaborating why cleaning up like that is wrong? The reason I did it was to avoid creating multiple md devices. Here we only have 8 iterations of the loop, but I could easly imagine tests were I'd have hundreds and cleaning as I go would be a good thing then, IMHO. But maybe I'm missing something obvious here, so if you could explain that would be great. Thanks.

PS. The patch is fine.

Long story short is that it was doubling up the cleanup process.

It would allocate the device in body function, clean it up in the body function, then try to clean up the same device in the cleanup function, failing because it didn't exist in a state that could be deallocated/detached.

Do you mind elaborating why cleaning up like that is wrong? The reason I did it was to avoid creating multiple md devices. Here we only have 8 iterations of the loop, but I could easly imagine tests were I'd have hundreds and cleaning as I go would be a good thing then, IMHO. But maybe I'm missing something obvious here, so if you could explain that would be great. Thanks.

Change looks good, my only suggestion would be to use maybe NAME2ADDR and ADDR2NAME, so the intentions are obvious.

Revert style changes.
Remove #ifdef INET checks from radlib.h, it prevents some consumers from compiling.
Implement some of oshogbo suggestions.

• Allow for optional compilation without IPv4 or IPv6 support.
• Make it possible to use IPv6 addresses in configuration file.
• Call getaddrinfo(3) with flags set to AI_ADDRCONFIG.

You are probably talking about this one: https://markmail.org/message/e3zkukh7oggutrvw
I've tried it, but it is very out-dated and has some bugs. It also keeps server on a TAILQ, which is nice, but I don't think it should be combined. The nice thing about that patch was to add multiple servers when a hostname resolves to multiple IP addresses, which I also implemented.

I agree with delphij that we should not introduce lower limit.

Looks good, but I agree that hastctl should use libmd too in that case.

Could you please explain why you implemented that in the kernel and not in userland geli command?

In D4712#124230, @cem wrote:

I agree with kib@. It would be nice not to have separate userspace tools here.

In fact we already have encrypted GEOM providers (g_eli, ...). All we lack is a way to dump core through them—maybe? I'm not even sure we lack that.

In D4712#99936, @kib wrote:

I looked over the patch, it looks fine to me.

Still, I have a question. You choice was to modify the dump code, which resulted in spreading the patch over the MI dump and all MD minidump code. Semi-obvious alternate approach is to introduce a geom which would write the generated encrypted symmetric key into the first block and write data block N into encrypted block N+1 of the underlying provider. Then you do not have to touch the kernel dump code at all.

Is it possible technically ?

Hi Pascal.

It looks good overall, but please be sure you run GELI regression tests. Thanks!

Thanks John, both good suggestions.