From owner-freebsd-fs@freebsd.org Sat Jul 11 12:48:55 2015 Return-Path: Delivered-To: freebsd-fs@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id BF9E83ADF for ; Sat, 11 Jul 2015 12:48:55 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 655BA11C0 for ; Sat, 11 Jul 2015 12:48:55 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id t6BCmmR3045277 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Sat, 11 Jul 2015 15:48:49 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua t6BCmmR3045277 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id t6BCmmGr045276; Sat, 11 Jul 2015 15:48:48 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Sat, 11 Jul 2015 15:48:48 +0300 From: Konstantin Belousov To: Mateusz Guzik 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> References: <1436569684-3939-1-git-send-email-mjguzik@gmail.com> <1436569684-3939-3-git-send-email-mjguzik@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436569684-3939-3-git-send-email-mjguzik@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 11 Jul 2015 12:48:55 -0000 On Sat, Jul 11, 2015 at 01:08:04AM +0200, Mateusz Guzik wrote: > From: Mateusz Guzik > > 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 > #include > #include > +#include > #include > #include > #include > @@ -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