Re: [review] sendfile / sendfile_sync refactoring

Hi,

It's definitely a work in progress, as you've observed.

On 28 November 2013 00:20, Konstantin Belousov <kostikbel_at_gmail.com> wrote:
> 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.

Yup. I don't like having it in here either. I may create a new header
file specifically for this.

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

Yeah, ignore that bit. I'm working on adding code for that.

>
> 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().

I plan on doing that soon.

> 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.

It's just precursor work to add support for other SF_* flags -
specifically, a new kqueue notification flag for completion.

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

The main motivation for moving the sendfile_sync setup and teardown
out of do_sendfile() is so do_sendfile() can keep a very little/light
idea of the behaviour of sendfile_sync. I don't mind keeping the code
in there.

Thanks for your feedback. I'll post an updated diff when I've fleshed
out some more of this.



-a