Date: Mon, 6 Feb 2012 11:54:15 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Pawel Jakub Dawidek <pjd@freebsd.org> Cc: arch@freebsd.org Subject: Re: Prefaulting for i/o buffers Message-ID: <20120206095415.GM3283@deviant.kiev.zoral.com.ua> In-Reply-To: <20120206085814.GB1324@garage.freebsd.pl> References: <20120203193719.GB3283@deviant.kiev.zoral.com.ua> <20120205114345.GE30033@garage.freebsd.pl> <20120205164729.GH3283@deviant.kiev.zoral.com.ua> <20120205192045.GH30033@garage.freebsd.pl> <20120205221259.GL3283@deviant.kiev.zoral.com.ua> <20120206085814.GB1324@garage.freebsd.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
--T4pX28sySFCT/oEO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 >=20 > --- a/sys/fs/nfsclient/nfs_clbio.c > +++ b/sys/fs/nfsclient/nfs_clbio.c > @@ -820,7 +820,10 @@ do_sync: > t_uio->uio_segflg =3D UIO_SYSSPACE; > t_uio->uio_rw =3D UIO_WRITE; > t_uio->uio_td =3D td; > - bcopy(uiop->uio_iov->iov_base, t_iov->iov_base, size); > + error =3D copyin(uiop->uio_iov->iov_base, > + t_iov->iov_base, size); > + if (error !=3D 0) > + goto err_free; >=20 > 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. >=20 > + if (lock->rl_currdep =3D=3D TAILQ_FIRST(&lock->rl_waiters) && > + lock->rl_currdep !=3D NULL) > + lock->rl_currdep =3D TAILQ_NEXT(lock->rl_currdep, rl_q_link); > + for (entry =3D lock->rl_currdep; entry; >=20 > Minor style inconsistency. Two lines earlier you compare pointer with > NULL, which is nice, but here you treat pointer as boolean. Fixed. >=20 > +void > +rangelock_unlock(struct rangelock *lock, void *cookie) > +{ > + struct rl_q_entry *entry; > + > + MPASS(lock !=3D NULL && cookie !=3D NULL); > + > + entry =3D cookie; > + sleepq_lock(lock); > + rangelock_unlock_locked(lock, entry); > +} >=20 > 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. >=20 > +/* > + * Add the lock request to the queue of the pending requests for > + * rangelock. Sleeps until the request can be granted. >=20 > s/request/lock/ ? No, I think request is more precise there. >=20 > @@ -216,6 +218,7 @@ thread_fini(void *mem, int size) > =20 > td =3D (struct thread *)mem; > EVENTHANDLER_INVOKE(thread_fini, td); > + rlqentry_free(td->td_rlqe); >=20 > 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 ha= ndle > 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. --T4pX28sySFCT/oEO Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAk8vo0cACgkQC3+MBN1Mb4h1DQCfefzc3n6Z3mj0Ia2dp8hk4RMR 2YwAoOWHv8a6dXehFKAGpB7glFSIV2oI =JAkA -----END PGP SIGNATURE----- --T4pX28sySFCT/oEO--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120206095415.GM3283>