Re: [review] sendfile / sendfile_sync refactoring

On Wed, Nov 27, 2013 at 11:36:44AM -0800, Adrian Chadd wrote:
> Hi,
> 
> Here's part #2 in my sendfile refactoring. This is a little more
> intrusive than the first patch.
> 
> http://people.freebsd.org/~adrian/netflix/20131126-sendfile-sync-refactor-2.diff
> 
sf_buf.h is for sf buffers, and not for sendfile stuff.  Please do not
pollute this header.  The changes you have to make to if_ti.c illustrate
my point.

The sys/event.h change of adding new kevent type is useless now and 
has no relation to the rest of the patch.

Patch has too many XXX notes for its triviality, some of which are
baseless, e.g. the comment for struct sendfile_sync forward declaration
in sys/file.h.

You extracted all sfs accesses from the sendfile implementation, except
the one in sf_buf_mext().  This is weird, please move the code from
sf_buf_mext() into static function and put it near sf_sync_ref().

While moving the code into sf_sync_syscall_wait(), please use the
opportunity and replace the if (sfs->count != 0) with the while ()
cv_wait(); loop, to avoid spurious wakeups.

I do not see any use for sfs->flags. Also, I do not see any use
for passing the flags masked with SF_SYNC, which is always set, to
sf_sync_alloc().  If the flags are going to be useful later, it should
be added when needed later.

The sf_sync_* stuff in the compat32 code just duplicates the main
syscall.  It should be done in the common code.

> My aim here is to move the sendfile_sync stuff out of uipc_syscalls.c
> and out of sendfile-only code and into something that can be reused
> elsewhere or replaced later on. I've created methods for all the
> sendfile_sync stuff, I've moved it out of the do_sendfile() loop so
> do_sendfile() doesn't specifically implement the completion behaviour.

As is, the patch is sincere nop. Modulo the comments above, I do not
object against it.