Date: Mon, 6 May 2013 20:16:11 +0200 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: Konstantin Belousov <kib@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r250027 - head/sys/kern Message-ID: <20130506181610.GA1390@garage.freebsd.pl> In-Reply-To: <201304281912.r3SJC9bL030636@svn.freebsd.org> References: <201304281912.r3SJC9bL030636@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--AqsLC8rIMeq19msA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 >=20 > 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. > =20 > 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. > =20 > Reviewed by: alc > Tested by: pho, bf (previous version) > MFC after: 1 week >=20 > Modified: > head/sys/kern/uipc_syscalls.c >=20 > Modified: head/sys/kern/uipc_syscalls.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- 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=20 > struct mbuf *m =3D NULL; > struct sf_buf *sf; > struct vm_page *pg; > + struct vattr va; > off_t off, xfsize, fsbytes =3D 0, sbytes =3D 0, rem =3D 0; > int error, hdrlen =3D 0, mnw =3D 0; > + int bsize; > struct sendfile_sync *sfs =3D NULL; > =20 > /* > @@ -2102,6 +2104,16 @@ retry_space: > */ > space -=3D hdrlen; > =20 > + error =3D vn_lock(vp, LK_SHARED); > + if (error !=3D 0) > + goto done; > + error =3D VOP_GETATTR(vp, &va, td->td_ucred); > + if (error !=3D 0) { > + VOP_UNLOCK(vp, 0); > + goto done; > + } > + bsize =3D 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; > =20 > - 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 =3D (uap->nbytes - fsbytes - loopbytes); > else > - rem =3D obj->un_pager.vnp.vnp_size - > + rem =3D va.va_size - > uap->offset - fsbytes - loopbytes; > xfsize =3D omin(PAGE_SIZE - pgoff, rem); > xfsize =3D omin(space - loopbytes, xfsize); > if (xfsize <=3D 0) { > - VM_OBJECT_WUNLOCK(obj); > done =3D 1; /* all data sent */ > break; > } > @@ -2136,7 +2146,6 @@ retry_space: > * Let the outer loop figure out how to handle it. > */ > if (space <=3D loopbytes) { > - VM_OBJECT_WUNLOCK(obj); > done =3D 0; > break; > } > @@ -2146,6 +2155,7 @@ retry_space: > * if not found or wait and loop if busy. > */ > pindex =3D OFF_TO_IDX(off); > + VM_OBJECT_WLOCK(obj); > pg =3D vm_page_grab(obj, pindex, VM_ALLOC_NOBUSY | > VM_ALLOC_NORMAL | VM_ALLOC_WIRED | VM_ALLOC_RETRY); > =20 > @@ -2163,7 +2173,6 @@ retry_space: > else if (uap->flags & SF_NODISKIO) > error =3D EBUSY; > else { > - int bsize; > ssize_t resid; > =20 > /* > @@ -2175,13 +2184,6 @@ retry_space: > =20 > /* > * Get the page from backing store. > - */ > - error =3D vn_lock(vp, LK_SHARED); > - if (error !=3D 0) > - goto after_read; > - bsize =3D 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: > } > } > =20 > + VOP_UNLOCK(vp, 0); > + > /* Add the buffer chain to the socket buffer. */ > if (m !=3D NULL) { > int mlen, err; --=20 Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://mobter.com --AqsLC8rIMeq19msA Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (FreeBSD) iEYEARECAAYFAlGH82oACgkQForvXbEpPzSldQCdGnw/MPq5gVdX94Y31lxwqoKW iXAAoIgOhuoEX1ZaUU9AYyZIYYKX6ef0 =Yuzy -----END PGP SIGNATURE----- --AqsLC8rIMeq19msA--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130506181610.GA1390>