On Wed, Sep 29, 2021 at 11:25 AM Qu Wenruo [off-list ref] wrote:

On 2021/9/29 17:59, Filipe Manana wrote:
[...]

quoted

quoted

quoted

Normally we should only exit if errno is not EINTR, and retry
(continue) on the EINTR case.

For this, I'm a little concerned.

EINTR means the operation is interrupted (by signal).
In that case, doesn't it mean the user want to stop the receive?

There will be plenty of opportunities to be interrupted and still be responsive.
But ok, you can leave it as it is.

quoted

[...]

quoted

quoted

quoted

What if we have thousands of clone operations?
Is there any rate limited print() in progs like there is for kernel?

Unfortunately we don't yet have.

But the good news (that I didn't catch at time of writing) is, we now
have global verbose/quite switch, so that we can easily hide those
warning by default and only output such warning for verbose run.

The problem with this is that it will possibly hide future bugs with
the kernel sending incorrect clone operations.

Having this fallback-to-read-write behaviour by default would hide
some bugs we had in the past on the kernel side, and unless users
start running receive with the verbose switch, we won't know about it.
Even if they do run with the verbose switch, some might not notice the
warnings at all, and for those who notice it they might not bother to
report the warnings since the receive succeeded and they didn't find
any data corruption/loss.

Or we might start receiving reports about send/receive doing less
cloning/extent sharing than before, and we won't easily know that the
receive side has fallen back to this read-write behaviour due to some
bug on kernel.

That's why making the behaviour explicit through a new command line
flag would cause less surprises and make it harder to miss regressions
on the kernel. And add some documentation explaining on which cases
the flag is useful.

This sounds indeed better, handling both cases well.

I'll try to add a new option to receive, maybe something called
--clone-fallback.

Any advice on the option name would be very helpful.

--clone-fallback

Sounds fine to me. It's short enough and gives some clue what it's about.
It's not completely self-explanatory, but I don't have a better
suggestion, and I think any name will always require an explanation of
what it does and when it's useful in the documentation (--help, man
page).

Thanks.

Thanks,
Qu

quoted

Thanks.

quoted

Thanks,
Qu

quoted

That's one reason why my proposal had in mind an optional flag to
trigger this behaviour.

Thanks for doing it, I was planning on doing something similar soon.

quoted

+               ret = buffered_copy(clone_fd, rctx->write_fd, clone_offset,
+                                   len, offset);
          }

   out:
--
2.33.0

-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”