svn commit: r250027 - head/sys/kern

[Pawel Jakub Dawidek]

On Sun, Apr 28, 2013 at 07:12:09PM +0000, Konstantin Belousov wrote:
> Author: kib
> Date: Sun Apr 28 19:12:09 2013
> New Revision: 250027
> URL: http://svnweb.freebsd.org/changeset/base/250027
> 
> Log:
>   Eliminate the layering violation in the kern_sendfile().  When quering
>   the file size, use VOP_GETATTR() instead of accessing vnode vm_object
>   un_pager.vnp.vnp_size.

Doesn't it add extra I/O to query file system about file's attributes?
If it does, does it matter here?

>   Take the shared vnode lock earlier to cover the added VOP_GETATTR()
>   call and, as consequence, the whole internal sendfile loop.  Reduce vm
>   object lock scope to not protect the local calculations.
>   
>   Note that this is the last misuse of the vnp_size in the tree, the
>   others were removed from the ELF image activator by r230246.
>   
>   Reviewed by:	alc
>   Tested by:	pho, bf (previous version)
>   MFC after:	1 week
> 
> Modified:
>   head/sys/kern/uipc_syscalls.c
> 
> Modified: head/sys/kern/uipc_syscalls.c
> ==============================================================================
> --- head/sys/kern/uipc_syscalls.c	Sun Apr 28 18:40:55 2013	(r250026)
> +++ head/sys/kern/uipc_syscalls.c	Sun Apr 28 19:12:09 2013	(r250027)
> @@ -1902,8 +1902,10 @@ kern_sendfile(struct thread *td, struct 
>  	struct mbuf *m = NULL;
>  	struct sf_buf *sf;
>  	struct vm_page *pg;
> +	struct vattr va;
>  	off_t off, xfsize, fsbytes = 0, sbytes = 0, rem = 0;
>  	int error, hdrlen = 0, mnw = 0;
> +	int bsize;
>  	struct sendfile_sync *sfs = NULL;
>  
>  	/*
> @@ -2102,6 +2104,16 @@ retry_space:
>  		 */
>  		space -= hdrlen;
>  
> +		error = vn_lock(vp, LK_SHARED);
> +		if (error != 0)
> +			goto done;
> +		error = VOP_GETATTR(vp, &va, td->td_ucred);
> +		if (error != 0) {
> +			VOP_UNLOCK(vp, 0);
> +			goto done;
> +		}
> +		bsize = vp->v_mount->mnt_stat.f_iosize;
> +
>  		/*
>  		 * Loop and construct maximum sized mbuf chain to be bulk
>  		 * dumped into socket buffer.
> @@ -2111,7 +2123,6 @@ retry_space:
>  			vm_offset_t pgoff;
>  			struct mbuf *m0;
>  
> -			VM_OBJECT_WLOCK(obj);
>  			/*
>  			 * Calculate the amount to transfer.
>  			 * Not to exceed a page, the EOF,
> @@ -2121,12 +2132,11 @@ retry_space:
>  			if (uap->nbytes)
>  				rem = (uap->nbytes - fsbytes - loopbytes);
>  			else
> -				rem = obj->un_pager.vnp.vnp_size -
> +				rem = va.va_size -
>  				    uap->offset - fsbytes - loopbytes;
>  			xfsize = omin(PAGE_SIZE - pgoff, rem);
>  			xfsize = omin(space - loopbytes, xfsize);
>  			if (xfsize <= 0) {
> -				VM_OBJECT_WUNLOCK(obj);
>  				done = 1;		/* all data sent */
>  				break;
>  			}
> @@ -2136,7 +2146,6 @@ retry_space:
>  			 * Let the outer loop figure out how to handle it.
>  			 */
>  			if (space <= loopbytes) {
> -				VM_OBJECT_WUNLOCK(obj);
>  				done = 0;
>  				break;
>  			}
> @@ -2146,6 +2155,7 @@ retry_space:
>  			 * if not found or wait and loop if busy.
>  			 */
>  			pindex = OFF_TO_IDX(off);
> +			VM_OBJECT_WLOCK(obj);
>  			pg = vm_page_grab(obj, pindex, VM_ALLOC_NOBUSY |
>  			    VM_ALLOC_NORMAL | VM_ALLOC_WIRED | VM_ALLOC_RETRY);
>  
> @@ -2163,7 +2173,6 @@ retry_space:
>  			else if (uap->flags & SF_NODISKIO)
>  				error = EBUSY;
>  			else {
> -				int bsize;
>  				ssize_t resid;
>  
>  				/*
> @@ -2175,13 +2184,6 @@ retry_space:
>  
>  				/*
>  				 * Get the page from backing store.
> -				 */
> -				error = vn_lock(vp, LK_SHARED);
> -				if (error != 0)
> -					goto after_read;
> -				bsize = vp->v_mount->mnt_stat.f_iosize;
> -
> -				/*
>  				 * XXXMAC: Because we don't have fp->f_cred
>  				 * here, we pass in NOCRED.  This is probably
>  				 * wrong, but is consistent with our original
> @@ -2191,8 +2193,6 @@ retry_space:
>  				    trunc_page(off), UIO_NOCOPY, IO_NODELOCKED |
>  				    IO_VMIO | ((MAXBSIZE / bsize) << IO_SEQSHIFT),
>  				    td->td_ucred, NOCRED, &resid, td);
> -				VOP_UNLOCK(vp, 0);
> -			after_read:
>  				VM_OBJECT_WLOCK(obj);
>  				vm_page_io_finish(pg);
>  				if (!error)
> @@ -2281,6 +2281,8 @@ retry_space:
>  			}
>  		}
>  
> +		VOP_UNLOCK(vp, 0);
> +
>  		/* Add the buffer chain to the socket buffer. */
>  		if (m != NULL) {
>  			int mlen, err;

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://mobter.com
-------------- 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/svn-src-head/attachments/20130506/d538d1d0/attachment.sig>