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
pgpSQh_8BCWdl.pgp
Description: PGP signature
