Prefaulting for i/o buffers

[Konstantin Belousov]

On Mon, Feb 06, 2012 at 09:58:14AM +0100, Pawel Jakub Dawidek wrote:
> On Mon, Feb 06, 2012 at 12:12:59AM +0200, Konstantin Belousov wrote:
> > http://people.freebsd.org/~kib/misc/vm1.9.patch
> 
> --- a/sys/fs/nfsclient/nfs_clbio.c
> +++ b/sys/fs/nfsclient/nfs_clbio.c
> @@ -820,7 +820,10 @@ do_sync:
>  			t_uio->uio_segflg = UIO_SYSSPACE;
>  			t_uio->uio_rw = UIO_WRITE;
>  			t_uio->uio_td = td;
> -			bcopy(uiop->uio_iov->iov_base, t_iov->iov_base, size);
> +			error = copyin(uiop->uio_iov->iov_base,
> +			    t_iov->iov_base, size);
> +			if (error != 0)
> +				goto err_free;
> 
> Good catch. It makes me wonder if uiop will always point at userland
> buffer. What if we eg. AUDIT subsystem writing from within the kernel to
> a file stored on NFS file system?
Hm, I committed to the wrong branch.

Anyway, speaking about this (unrelated) change, I thought that directio
path cannot happen for UIO_SYSSPACE. But since both you and Rick think
that handling UIO_SYSSPACE there makes sense, so be it.

> 
> +	if (lock->rl_currdep == TAILQ_FIRST(&lock->rl_waiters) &&
> +	    lock->rl_currdep != NULL)
> +		lock->rl_currdep = TAILQ_NEXT(lock->rl_currdep, rl_q_link);
> +	for (entry = lock->rl_currdep; entry;
> 
> Minor style inconsistency. Two lines earlier you compare pointer with
> NULL, which is nice, but here you treat pointer as boolean.
Fixed.

> 
> +void
> +rangelock_unlock(struct rangelock *lock, void *cookie)
> +{
> +	struct rl_q_entry *entry;
> +
> +	MPASS(lock != NULL && cookie != NULL);
> +
> +	entry = cookie;
> +	sleepq_lock(lock);
> +	rangelock_unlock_locked(lock, entry);
> +}
> 
> You could drop using addtional 'entry' variable and just pass 'cookie'
> directly. Although current code is self-explaining what 'cookie'
> actually is, so I kinda like it as-is.
Dropped entry.

> 
> +/*
> + * Add the lock request to the queue of the pending requests for
> + * rangelock.  Sleeps until the request can be granted.
> 
> s/request/lock/ ?
No, I think request is more precise there.

> 
> @@ -216,6 +218,7 @@ thread_fini(void *mem, int size)
>  
>  	td = (struct thread *)mem;
>  	EVENTHANDLER_INVOKE(thread_fini, td);
> +	rlqentry_free(td->td_rlqe);
> 
> Not sure if it was intended or not, but td_rlqe can be NULL now, so it
> might be worth checking that. It is not a big deal, as uma_zfree() can handle
> NULL pointers though.
Yes, it is intended. As you noted yourself, uma_zfree() handles NULL.
For typical thread, td_rlqe is not NULL, so it is slightly better to not
check for NULL.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20120206/5c4c3fa4/attachment.pgp