Date: Sat, 11 Jul 2015 15:48:48 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-fs@freebsd.org Subject: Re: [PATCH 2/2] Create a dedicated function for ensuring that cdir and rdir are populated. Message-ID: <20150711124848.GG2080@kib.kiev.ua> In-Reply-To: <1436569684-3939-3-git-send-email-mjguzik@gmail.com> References: <1436569684-3939-1-git-send-email-mjguzik@gmail.com> <1436569684-3939-3-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Jul 11, 2015 at 01:08:04AM +0200, Mateusz Guzik wrote:
> From: Mateusz Guzik <mjg@freebsd.org>
>
> Previously several places were doing it on its own, partially
> incorrectly (e.g. without the filedesc locked) or even actively harmful
> by assigning rootvnode without vreling it or populating jdir.
>
> This functionality should not exist and will be garbage collected after
> all callers are properly reviewed.
Why do you think that this code 'should not exist' ? The code comes due
to the need of some kernel processes to do file i/o, but also from the
desire to avoid having all kernel processes reference some vnode, even
if not needed. How do you propose to make it possible to use lookups
etc from a kernel process ?
Regardless of the note above, the patch looks fine, it is the good cleanup.
> ---
> sys/cam/ctl/ctl_backend_block.c | 13 +------------
> sys/cddl/compat/opensolaris/kern/opensolaris_kobj.c | 13 +------------
> sys/cddl/compat/opensolaris/sys/vnode.h | 13 +------------
> sys/compat/ndis/subr_ndis.c | 5 +----
> sys/dev/xen/blkback/blkback.c | 13 +------------
> sys/kern/kern_descrip.c | 19 +++++++++++++++++++
> sys/kern/subr_firmware.c | 13 +------------
> sys/sys/filedesc.h | 1 +
> 8 files changed, 26 insertions(+), 64 deletions(-)
>
> diff --git a/sys/cam/ctl/ctl_backend_block.c b/sys/cam/ctl/ctl_backend_block.c
> index c56023b..8ea52aa 100644
> --- a/sys/cam/ctl/ctl_backend_block.c
> +++ b/sys/cam/ctl/ctl_backend_block.c
> @@ -2123,18 +2123,7 @@ ctl_be_block_open(struct ctl_be_block_softc *softc,
> return (1);
> }
>
> - if (!curthread->td_proc->p_fd->fd_cdir) {
> - curthread->td_proc->p_fd->fd_cdir = rootvnode;
> - VREF(rootvnode);
> - }
> - if (!curthread->td_proc->p_fd->fd_rdir) {
> - curthread->td_proc->p_fd->fd_rdir = rootvnode;
> - VREF(rootvnode);
> - }
> - if (!curthread->td_proc->p_fd->fd_jdir) {
> - curthread->td_proc->p_fd->fd_jdir = rootvnode;
> - VREF(rootvnode);
> - }
> + pwd_ensure_dirs();
>
> again:
> NDINIT(&nd, LOOKUP, FOLLOW, UIO_SYSSPACE, be_lun->dev_path, curthread);
> diff --git a/sys/cddl/compat/opensolaris/kern/opensolaris_kobj.c b/sys/cddl/compat/opensolaris/kern/opensolaris_kobj.c
> index 9ff798a..52d695b 100644
> --- a/sys/cddl/compat/opensolaris/kern/opensolaris_kobj.c
> +++ b/sys/cddl/compat/opensolaris/kern/opensolaris_kobj.c
> @@ -67,21 +67,10 @@ static void *
> kobj_open_file_vnode(const char *file)
> {
> struct thread *td = curthread;
> - struct filedesc *fd;
> struct nameidata nd;
> int error, flags;
>
> - fd = td->td_proc->p_fd;
> - FILEDESC_XLOCK(fd);
> - if (fd->fd_rdir == NULL) {
> - fd->fd_rdir = rootvnode;
> - vref(fd->fd_rdir);
> - }
> - if (fd->fd_cdir == NULL) {
> - fd->fd_cdir = rootvnode;
> - vref(fd->fd_cdir);
> - }
> - FILEDESC_XUNLOCK(fd);
> + pwd_ensure_dirs();
>
> flags = FREAD | O_NOFOLLOW;
> NDINIT(&nd, LOOKUP, 0, UIO_SYSSPACE, file, td);
> diff --git a/sys/cddl/compat/opensolaris/sys/vnode.h b/sys/cddl/compat/opensolaris/sys/vnode.h
> index 22256cf..d7bc7f7 100644
> --- a/sys/cddl/compat/opensolaris/sys/vnode.h
> +++ b/sys/cddl/compat/opensolaris/sys/vnode.h
> @@ -162,7 +162,6 @@ vn_openat(char *pnamep, enum uio_seg seg, int filemode, int createmode,
> int fd)
> {
> struct thread *td = curthread;
> - struct filedesc *fdc;
> struct nameidata nd;
> int error, operation;
>
> @@ -179,17 +178,7 @@ vn_openat(char *pnamep, enum uio_seg seg, int filemode, int createmode,
> }
> ASSERT(umask == 0);
>
> - fdc = td->td_proc->p_fd;
> - FILEDESC_XLOCK(fdc);
> - if (fdc->fd_rdir == NULL) {
> - fdc->fd_rdir = rootvnode;
> - vref(fdc->fd_rdir);
> - }
> - if (fdc->fd_cdir == NULL) {
> - fdc->fd_cdir = rootvnode;
> - vref(fdc->fd_rdir);
> - }
> - FILEDESC_XUNLOCK(fdc);
> + pwd_ensure_dirs();
>
> if (startvp != NULL)
> vref(startvp);
> diff --git a/sys/compat/ndis/subr_ndis.c b/sys/compat/ndis/subr_ndis.c
> index f3ba700..ac26a2e 100644
> --- a/sys/compat/ndis/subr_ndis.c
> +++ b/sys/compat/ndis/subr_ndis.c
> @@ -2817,10 +2817,7 @@ NdisOpenFile(status, filehandle, filelength, filename, highestaddr)
>
> /* Some threads don't have a current working directory. */
>
> - if (td->td_proc->p_fd->fd_rdir == NULL)
> - td->td_proc->p_fd->fd_rdir = rootvnode;
> - if (td->td_proc->p_fd->fd_cdir == NULL)
> - td->td_proc->p_fd->fd_cdir = rootvnode;
> + pwd_ensure_dirs();
>
> NDINIT(&nd, LOOKUP, FOLLOW, UIO_SYSSPACE, path, td);
>
> diff --git a/sys/dev/xen/blkback/blkback.c b/sys/dev/xen/blkback/blkback.c
> index 459271e..f266ffd 100644
> --- a/sys/dev/xen/blkback/blkback.c
> +++ b/sys/dev/xen/blkback/blkback.c
> @@ -2692,18 +2692,7 @@ xbb_open_backend(struct xbb_softc *xbb)
> if ((xbb->flags & XBBF_READ_ONLY) == 0)
> flags |= FWRITE;
>
> - if (!curthread->td_proc->p_fd->fd_cdir) {
> - curthread->td_proc->p_fd->fd_cdir = rootvnode;
> - VREF(rootvnode);
> - }
> - if (!curthread->td_proc->p_fd->fd_rdir) {
> - curthread->td_proc->p_fd->fd_rdir = rootvnode;
> - VREF(rootvnode);
> - }
> - if (!curthread->td_proc->p_fd->fd_jdir) {
> - curthread->td_proc->p_fd->fd_jdir = rootvnode;
> - VREF(rootvnode);
> - }
> + pwd_ensure_dirs();
>
> again:
> NDINIT(&nd, LOOKUP, FOLLOW, UIO_SYSSPACE, xbb->dev_name, curthread);
> diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
> index 37381ee..dea9d35 100644
> --- a/sys/kern/kern_descrip.c
> +++ b/sys/kern/kern_descrip.c
> @@ -68,6 +68,7 @@ __FBSDID("$FreeBSD$");
> #include <sys/sbuf.h>
> #include <sys/signalvar.h>
> #include <sys/socketvar.h>
> +#include <sys/kdb.h>
> #include <sys/stat.h>
> #include <sys/sx.h>
> #include <sys/syscallsubr.h>
> @@ -308,6 +309,24 @@ fdfree(struct filedesc *fdp, int fd)
> #endif
> }
>
> +void
> +pwd_ensure_dirs(void)
> +{
> + struct filedesc *fdp;
> +
> + fdp = curproc->p_fd;
> + FILEDESC_XLOCK(fdp);
> + if (fdp->fd_cdir == NULL) {
> + fdp->fd_cdir = rootvnode;
> + VREF(rootvnode);
> + }
> + if (fdp->fd_rdir == NULL) {
> + fdp->fd_rdir = rootvnode;
> + VREF(rootvnode);
> + }
> + FILEDESC_XUNLOCK(fdp);
> +}
> +
> /*
> * System calls on descriptors.
> */
> diff --git a/sys/kern/subr_firmware.c b/sys/kern/subr_firmware.c
> index 20ab76e..172d719 100644
> --- a/sys/kern/subr_firmware.c
> +++ b/sys/kern/subr_firmware.c
> @@ -383,19 +383,8 @@ firmware_put(const struct firmware *p, int flags)
> static void
> set_rootvnode(void *arg, int npending)
> {
> - struct thread *td = curthread;
> - struct proc *p = td->td_proc;
>
> - FILEDESC_XLOCK(p->p_fd);
> - if (p->p_fd->fd_cdir == NULL) {
> - p->p_fd->fd_cdir = rootvnode;
> - VREF(rootvnode);
> - }
> - if (p->p_fd->fd_rdir == NULL) {
> - p->p_fd->fd_rdir = rootvnode;
> - VREF(rootvnode);
> - }
> - FILEDESC_XUNLOCK(p->p_fd);
> + pwd_ensure_dirs();
>
> free(arg, M_TEMP);
> }
> diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h
> index e569a3b..727a098 100644
> --- a/sys/sys/filedesc.h
> +++ b/sys/sys/filedesc.h
> @@ -208,6 +208,7 @@ fd_modified(struct filedesc *fdp, int fd, seq_t seq)
> /* cdir/rdir/jdir manipulation functions. */
> void pwd_chdir(struct thread *td, struct vnode *vp);
> int pwd_chroot(struct thread *td, struct vnode *vp);
> +void pwd_ensure_dirs(void);
>
> #endif /* _KERNEL */
>
> --
> 2.4.5
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150711124848.GG2080>