tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: Move the vnode lock into file systems
On Sat, Jun 26, 2010 at 10:39:27AM +0200, Juergen Hannken-Illjes wrote:
> The vnode lock operations currently work on a rw lock located inside the
> vnode. I propose to move this lock into the file system node.
>
> This place is more logical as we lock a file system node and not a vnode.
> This becomes clear if we think of a file system where one file system node
> is attached to more than one vnode. Ptyfs allowing multiple mounts is such
> a candidate.
>
> A diff implemeting this for ufs, lfs and ext2fs is attached.
>
> Comments or objections anyone?
I like this idea. Any reason this cannot live in genfs_node?
Thinking out loud here, but doing this would help us eliminate deadfs and
make forced unmount safe again. With another field in genfs_node (say
'bool gone') to indicate that VOP_REVOKE has taken place, we could return
failure at the lock point for revoked vnodes. We'd still need to allow
VOP_INACTIVE and VOP_RECLAIM to take place on the vnode though (possibly
also VOP_CLOSE??) - haven't thought it through fully.
Have you given thought to removing the lock function from vget()? That
is, change it only to acquire references and deal with XLOCK?
Thanks!
> --
> Juergen Hannken-Illjes - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig
> (Germany)
>
> Index: sys/ufs/ufs/inode.h
> ===================================================================
> RCS file: /cvsroot/src/sys/ufs/ufs/inode.h,v
> retrieving revision 1.56
> diff -p -u -4 -r1.56 inode.h
> --- sys/ufs/ufs/inode.h 22 Feb 2009 20:28:07 -0000 1.56
> +++ sys/ufs/ufs/inode.h 25 Jun 2010 20:43:24 -0000
> @@ -91,9 +91,9 @@ struct inode {
> #define i_fs inode_u.fs
> #define i_lfs inode_u.lfs
> #define i_e2fs inode_u.e2fs
>
> - void *i_unused1; /* Unused. */
> + krwlock_t i_lock; /* Inode lock. */
> struct dquot *i_dquot[MAXQUOTAS]; /* Dquot structures. */
> u_quad_t i_modrev; /* Revision level for NFS lease. */
> struct lockf *i_lockf;/* Head of byte-level lock list. */
>
> Index: sys/ufs/ufs/ufs_extern.h
> ===================================================================
> RCS file: /cvsroot/src/sys/ufs/ufs/ufs_extern.h,v
> retrieving revision 1.62
> diff -p -u -4 -r1.62 ufs_extern.h
> --- sys/ufs/ufs/ufs_extern.h 13 Sep 2009 05:17:37 -0000 1.62
> +++ sys/ufs/ufs/ufs_extern.h 25 Jun 2010 20:43:24 -0000
> @@ -67,11 +67,11 @@ int ufs_create(void *);
> int ufs_getattr(void *);
> int ufs_inactive(void *);
> #define ufs_fcntl genfs_fcntl
> #define ufs_ioctl genfs_enoioctl
> -#define ufs_islocked genfs_islocked
> +int ufs_islocked(void *);
> int ufs_link(void *);
> -#define ufs_lock genfs_lock
> +int ufs_lock(void *);
> int ufs_lookup(void *);
> int ufs_mkdir(void *);
> int ufs_mknod(void *);
> #define ufs_mmap genfs_mmap
> @@ -88,9 +88,9 @@ int ufs_rmdir(void *);
> #define ufs_poll genfs_poll
> int ufs_setattr(void *);
> int ufs_strategy(void *);
> int ufs_symlink(void *);
> -#define ufs_unlock genfs_unlock
> +int ufs_unlock(void *);
> int ufs_whiteout(void *);
> int ufsspec_close(void *);
> int ufsspec_read(void *);
> int ufsspec_write(void *);
> Index: sys/ufs/ufs/ufs_ihash.c
> ===================================================================
> RCS file: /cvsroot/src/sys/ufs/ufs/ufs_ihash.c,v
> retrieving revision 1.28
> diff -p -u -4 -r1.28 ufs_ihash.c
> --- sys/ufs/ufs/ufs_ihash.c 5 Nov 2009 08:18:02 -0000 1.28
> +++ sys/ufs/ufs/ufs_ihash.c 25 Jun 2010 20:43:24 -0000
> @@ -170,9 +170,10 @@ ufs_ihashins(struct inode *ip)
>
> KASSERT(mutex_owned(&ufs_hashlock));
>
> /* lock the inode, then put it on the appropriate hash list */
> - vlockmgr(&ip->i_vnode->v_lock, LK_EXCLUSIVE);
> + rw_init(&ip->i_lock);
> + rw_enter(&ip->i_lock, RW_WRITER);
>
> mutex_enter(&ufs_ihash_lock);
> ipp = &ihashtbl[INOHASH(ip->i_dev, ip->i_number)];
> LIST_INSERT_HEAD(ipp, ip, i_hash);
> @@ -187,5 +188,6 @@ ufs_ihashrem(struct inode *ip)
> {
> mutex_enter(&ufs_ihash_lock);
> LIST_REMOVE(ip, i_hash);
> mutex_exit(&ufs_ihash_lock);
> + rw_destroy(&ip->i_lock);
> }
> Index: sys/ufs/ufs/ufs_vnops.c
> ===================================================================
> RCS file: /cvsroot/src/sys/ufs/ufs/ufs_vnops.c,v
> retrieving revision 1.183
> diff -p -u -4 -r1.183 ufs_vnops.c
> --- sys/ufs/ufs/ufs_vnops.c 24 Jun 2010 13:03:20 -0000 1.183
> +++ sys/ufs/ufs/ufs_vnops.c 25 Jun 2010 20:43:25 -0000
> @@ -2346,4 +2346,66 @@ ufs_gop_markupdate(struct vnode *vp, int
>
> ip->i_flag |= mask;
> }
> }
> +
> +/*
> + * Lock the node.
> + */
> +int
> +ufs_lock(void *v)
> +{
> + struct vop_lock_args /* {
> + struct vnode *a_vp;
> + int a_flags;
> + } */ *ap = v;
> + struct inode *ip = VTOI(ap->a_vp);
> + int flags = ap->a_flags;
> + krw_t op;
> +
> + KASSERT((flags & ~(LK_EXCLUSIVE | LK_SHARED | LK_NOWAIT)) == 0);
> +
> + op = ((flags & LK_EXCLUSIVE) != 0 ? RW_WRITER : RW_READER);
> +
> + if ((flags & LK_NOWAIT) != 0)
> + return (rw_tryenter(&ip->i_lock, op) ? 0 : EBUSY);
> +
> + rw_enter(&ip->i_lock, op);
> +
> + return 0;
> +}
> +
> +/*
> + * Unlock the node.
> + */
> +int
> +ufs_unlock(void *v)
> +{
> + struct vop_unlock_args /* {
> + struct vnode *a_vp;
> + } */ *ap = v;
> + struct inode *ip = VTOI(ap->a_vp);
> +
> + rw_exit(&ip->i_lock);
> +
> + return 0;
> +}
> +
> +/*
> + * Return whether or not the node is locked.
> + */
> +int
> +ufs_islocked(void *v)
> +{
> + struct vop_islocked_args /* {
> + struct vnode *a_vp;
> + } */ *ap = v;
> + struct inode *ip = VTOI(ap->a_vp);
> +
> + if (rw_write_held(&ip->i_lock))
> + return LK_EXCLUSIVE;
> +
> + if (rw_read_held(&ip->i_lock))
> + return LK_SHARED;
> +
> + return 0;
> +}
> Index: sys/ufs/lfs/lfs_syscalls.c
> ===================================================================
> RCS file: /cvsroot/src/sys/ufs/lfs/lfs_syscalls.c,v
> retrieving revision 1.137
> diff -p -u -4 -r1.137 lfs_syscalls.c
> --- sys/ufs/lfs/lfs_syscalls.c 24 Jun 2010 13:03:19 -0000 1.137
> +++ sys/ufs/lfs/lfs_syscalls.c 25 Jun 2010 20:43:24 -0000
> @@ -1136,12 +1136,12 @@ lfs_fastvget(struct mount *mp, ino_t ino
> error = copyin(dinp, ip->i_din.ffs1_din, sizeof (struct
> ufs1_dinode));
> if (error) {
> DLOG((DLOG_CLEAN, "lfs_fastvget: dinode copyin failed"
> " for ino %d\n", ino));
> + /* Unlock and discard unneeded inode. */
> + rw_exit(&ip->i_lock);
> ufs_ihashrem(ip);
>
> - /* Unlock and discard unneeded inode. */
> - vlockmgr(&vp->v_lock, LK_RELEASE);
> lfs_vunref(vp);
> *vpp = NULL;
> return (error);
> }
> @@ -1158,13 +1158,13 @@ lfs_fastvget(struct mount *mp, ino_t ino
> /*
> * The inode does not contain anything useful, so it
> * would be misleading to leave it on its hash chain.
> * Iput() will return it to the free list.
> + * Unlock and discard unneeded inode.
> */
> + rw_exit(&ip->i_lock);
> ufs_ihashrem(ip);
>
> - /* Unlock and discard unneeded inode. */
> - vlockmgr(&vp->v_lock, LK_RELEASE);
> lfs_vunref(vp);
> brelse(bp, 0);
> *vpp = NULL;
> return (error);
Home |
Main Index |
Thread Index |
Old Index