Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 09 May 2013 14:11:44 +0200
From:      Goran Lowkrantz <goran.lowkrantz@ismobile.com>
To:        Konstantin Belousov <kostikbel@gmail.com>
Cc:        pho@freebsd.org, freebsd-stable@freebsd.org
Subject:   Re: Nullfs leaks i-nodes
Message-ID:  <BFD5A40BC867D1DA2AAEFF16@[10.255.253.2]>
In-Reply-To: <20130508091317.GJ3047@kib.kiev.ua>
References:  <B799E3B928B18B9E6C68F912@[172.16.2.62]> <20130508091317.GJ3047@kib.kiev.ua>

next in thread | previous in thread | raw e-mail | index | archive | help
--On Wednesday, 08 May, 2013 12:13 PM +0300 Konstantin Belousov 
<kostikbel@gmail.com> wrote:

> On Tue, May 07, 2013 at 08:30:06AM +0200, G??ran L??wkrantz wrote:
>> I created a PR, kern/178238, on this but would like to know if anyone
>> has  any ideas or patches?
>>
>> Have updated the system where I see this to FreeBSD 9.1-STABLE #0
>> r250229  and still have the problem.
>
> The patch below should fix the issue for you, at least it did so in my
> limited testing.
>
> What is does:
> 1. When inactivating a nullfs vnode, check if the lower vnode is
>    unlinked, and reclaim upper vnode if so. [This fixes your case].
>
> 2. Besides a callback to the upper filesystems for the lower vnode
>    reclaimation, it also calls the upper fs for lower vnode unlink.
>    This allows nullfs to purge cached vnodes for the unlinked lower.
>    [This fixes an opposite case, when the vnode is removed from the
>    lower mount, but upper aliases prevent the vnode from being
>    recycled].
>
> 3. Fix a wart which existed from the introduction of the nullfs caching,
>    do not unlock lower vnode in the nullfs_reclaim_lowervp().  It should
>    be completely innocent, but now it is also formally safe.
>
> 4. Fix vnode reference leak in nullfs_reclaim_lowervp().
>
> Please note that the patch is basically not tested, I only verified your
> scenario and a mirror of it as described in item 2.
>
> diff --git a/sys/fs/nullfs/null.h b/sys/fs/nullfs/null.h
> index 4f37020..a624be6 100644
> --- a/sys/fs/nullfs/null.h
> +++ b/sys/fs/nullfs/null.h
> @@ -53,8 +53,11 @@ struct null_node {
>  	LIST_ENTRY(null_node)	null_hash;	/* Hash list */
>  	struct vnode	        *null_lowervp;	/* VREFed once */
>  	struct vnode		*null_vnode;	/* Back pointer */
> +	u_int			null_flags;
>  };
>
> +#define	NULLV_NOUNLOCK	0x0001
> +
>  #define	MOUNTTONULLMOUNT(mp) ((struct null_mount *)((mp)->mnt_data))
>  #define	VTONULL(vp) ((struct null_node *)(vp)->v_data)
>  #define	NULLTOV(xp) ((xp)->null_vnode)
> diff --git a/sys/fs/nullfs/null_vfsops.c b/sys/fs/nullfs/null_vfsops.c
> index 02932bd..544c358 100644
> --- a/sys/fs/nullfs/null_vfsops.c
> +++ b/sys/fs/nullfs/null_vfsops.c
> @@ -65,7 +65,6 @@ static vfs_statfs_t	nullfs_statfs;
>  static vfs_unmount_t	nullfs_unmount;
>  static vfs_vget_t	nullfs_vget;
>  static vfs_extattrctl_t	nullfs_extattrctl;
> -static vfs_reclaim_lowervp_t nullfs_reclaim_lowervp;
>
>  /*
>   * Mount null layer
> @@ -391,8 +390,22 @@ nullfs_reclaim_lowervp(struct mount *mp, struct
> vnode *lowervp)  	vp = null_hashget(mp, lowervp);
>  	if (vp == NULL)
>  		return;
> +	VTONULL(vp)->null_flags |= NULLV_NOUNLOCK;
>  	vgone(vp);
> -	vn_lock(lowervp, LK_EXCLUSIVE | LK_RETRY);
> +	vput(vp);
> +}
> +
> +static void
> +nullfs_unlink_lowervp(struct mount *mp, struct vnode *lowervp)
> +{
> +	struct vnode *vp;
> +
> +	vp = null_hashget(mp, lowervp);
> +	if (vp == NULL || vp->v_usecount > 1)
> +		return;
> +	VTONULL(vp)->null_flags |= NULLV_NOUNLOCK;
> +	vgone(vp);
> +	vput(vp);
>  }
>
>  static struct vfsops null_vfsops = {
> @@ -408,6 +421,7 @@ static struct vfsops null_vfsops = {
>  	.vfs_unmount =		nullfs_unmount,
>  	.vfs_vget =		nullfs_vget,
>  	.vfs_reclaim_lowervp =	nullfs_reclaim_lowervp,
> +	.vfs_unlink_lowervp =	nullfs_unlink_lowervp,
>  };
>
>  VFS_SET(null_vfsops, nullfs, VFCF_LOOPBACK | VFCF_JAIL);
> diff --git a/sys/fs/nullfs/null_vnops.c b/sys/fs/nullfs/null_vnops.c
> index f59865f..3c41124 100644
> --- a/sys/fs/nullfs/null_vnops.c
> +++ b/sys/fs/nullfs/null_vnops.c
> @@ -692,18 +692,21 @@ null_unlock(struct vop_unlock_args *ap)
>  static int
>  null_inactive(struct vop_inactive_args *ap __unused)
>  {
> -	struct vnode *vp;
> +	struct vnode *vp, *lvp;
>  	struct mount *mp;
>  	struct null_mount *xmp;
>
>  	vp = ap->a_vp;
> +	lvp = NULLVPTOLOWERVP(vp);
>  	mp = vp->v_mount;
>  	xmp = MOUNTTONULLMOUNT(mp);
> -	if ((xmp->nullm_flags & NULLM_CACHE) == 0) {
> +	if ((xmp->nullm_flags & NULLM_CACHE) == 0 ||
> +	    (lvp->v_vflag & VV_NOSYNC) != 0) {
>  		/*
>  		 * If this is the last reference and caching of the
> -		 * nullfs vnodes is not enabled, then free up the
> -		 * vnode so as not to tie up the lower vnodes.
> +		 * nullfs vnodes is not enabled, or the lower vnode is
> +		 * deleted, then free up the vnode so as not to tie up
> +		 * the lower vnodes.
>  		 */
>  		vp->v_object = NULL;
>  		vrecycle(vp);
> @@ -748,7 +751,10 @@ null_reclaim(struct vop_reclaim_args *ap)
>  	 */
>  	if (vp->v_writecount > 0)
>  		VOP_ADD_WRITECOUNT(lowervp, -1);
> -	vput(lowervp);
> +	if ((xp->null_flags & NULLV_NOUNLOCK) != 0)
> +		vunref(lowervp);
> +	else
> +		vput(lowervp);
>  	free(xp, M_NULLFSNODE);
>
>  	return (0);
> diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c
> index de118f7..988a114 100644
> --- a/sys/kern/vfs_subr.c
> +++ b/sys/kern/vfs_subr.c
> @@ -2700,19 +2700,20 @@ vgone(struct vnode *vp)
>  }
>
>  static void
> -vgonel_reclaim_lowervp_vfs(struct mount *mp __unused,
> +notify_lowervp_vfs_dummy(struct mount *mp __unused,
>      struct vnode *lowervp __unused)
>  {
>  }
>
>  /*
> - * Notify upper mounts about reclaimed vnode.
> + * Notify upper mounts about reclaimed or unlinked vnode.
>   */
> -static void
> -vgonel_reclaim_lowervp(struct vnode *vp)
> +void
> +vfs_notify_upper(struct vnode *vp, int event)
>  {
>  	static struct vfsops vgonel_vfsops = {
> -		.vfs_reclaim_lowervp = vgonel_reclaim_lowervp_vfs
> +		.vfs_reclaim_lowervp = notify_lowervp_vfs_dummy,
> +		.vfs_unlink_lowervp = notify_lowervp_vfs_dummy,
>  	};
>  	struct mount *mp, *ump, *mmp;
>
> @@ -2736,7 +2737,17 @@ vgonel_reclaim_lowervp(struct vnode *vp)
>  		}
>  		TAILQ_INSERT_AFTER(&mp->mnt_uppers, ump, mmp, mnt_upper_link);
>  		MNT_IUNLOCK(mp);
> -		VFS_RECLAIM_LOWERVP(ump, vp);
> +		switch (event) {
> +		case VFS_NOTIFY_UPPER_RECLAIM:
> +			VFS_RECLAIM_LOWERVP(ump, vp);
> +			break;
> +		case VFS_NOTIFY_UPPER_UNLINK:
> +			VFS_UNLINK_LOWERVP(ump, vp);
> +			break;
> +		default:
> +			KASSERT(0, ("invalid event %d", event));
> +			break;
> +		}
>  		MNT_ILOCK(mp);
>  		ump = TAILQ_NEXT(mmp, mnt_upper_link);
>  		TAILQ_REMOVE(&mp->mnt_uppers, mmp, mnt_upper_link);
> @@ -2783,7 +2794,7 @@ vgonel(struct vnode *vp)
>  	active = vp->v_usecount;
>  	oweinact = (vp->v_iflag & VI_OWEINACT);
>  	VI_UNLOCK(vp);
> -	vgonel_reclaim_lowervp(vp);
> +	vfs_notify_upper(vp, VFS_NOTIFY_UPPER_RECLAIM);
>
>  	/*
>  	 * Clean out any buffers associated with the vnode.
> diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
> index 29cb7bd..a004ea0 100644
> --- a/sys/kern/vfs_syscalls.c
> +++ b/sys/kern/vfs_syscalls.c
> @@ -1846,6 +1846,7 @@ restart:
>  		if (error)
>  			goto out;
>  #endif
> +		vfs_notify_upper(vp, VFS_NOTIFY_UPPER_UNLINK);
>  		error = VOP_REMOVE(nd.ni_dvp, vp, &nd.ni_cnd);
>  #ifdef MAC
>  out:
> @@ -3825,6 +3826,7 @@ restart:
>  			return (error);
>  		goto restart;
>  	}
> +	vfs_notify_upper(vp, VFS_NOTIFY_UPPER_UNLINK);
>  	error = VOP_RMDIR(nd.ni_dvp, nd.ni_vp, &nd.ni_cnd);
>  	vn_finished_write(mp);
>  out:
> diff --git a/sys/sys/mount.h b/sys/sys/mount.h
> index a9c86f6..a953dae 100644
> --- a/sys/sys/mount.h
> +++ b/sys/sys/mount.h
> @@ -608,7 +608,7 @@ typedef	int vfs_mount_t(struct mount *mp);
>  typedef int vfs_sysctl_t(struct mount *mp, fsctlop_t op,
>  		    struct sysctl_req *req);
>  typedef void vfs_susp_clean_t(struct mount *mp);
> -typedef void vfs_reclaim_lowervp_t(struct mount *mp, struct vnode
> *lowervp); +typedef void vfs_notify_lowervp_t(struct mount *mp, struct
> vnode *lowervp);
>  struct vfsops {
>  	vfs_mount_t		*vfs_mount;
> @@ -626,7 +626,8 @@ struct vfsops {
>  	vfs_extattrctl_t	*vfs_extattrctl;
>  	vfs_sysctl_t		*vfs_sysctl;
>  	vfs_susp_clean_t	*vfs_susp_clean;
> -	vfs_reclaim_lowervp_t	*vfs_reclaim_lowervp;
> +	vfs_notify_lowervp_t	*vfs_reclaim_lowervp;
> +	vfs_notify_lowervp_t	*vfs_unlink_lowervp;
>  };
>
>  vfs_statfs_t	__vfs_statfs;
> @@ -747,6 +748,14 @@ vfs_statfs_t	__vfs_statfs;
>  	}								\
>  } while (0)
>
> +#define	VFS_UNLINK_LOWERVP(MP, VP) do {					\
> +	if (*(MP)->mnt_op->vfs_unlink_lowervp != NULL) {		\
> +		VFS_PROLOGUE(MP);					\
> +		(*(MP)->mnt_op->vfs_unlink_lowervp)((MP), (VP));	\
> +		VFS_EPILOGUE(MP);					\
> +	}								\
> +} while (0)
> +
>  #define VFS_KNOTE_LOCKED(vp, hint) do					\
>  {									\
>  	if (((vp)->v_vflag & VV_NOKNOTE) == 0)				\
> @@ -759,6 +768,9 @@ vfs_statfs_t	__vfs_statfs;
>  		VN_KNOTE((vp), (hint), 0);				\
>  } while (0)
>
> +#define	VFS_NOTIFY_UPPER_RECLAIM	1
> +#define	VFS_NOTIFY_UPPER_UNLINK		2
> +
>  #include <sys/module.h>
>
>  /*
> @@ -840,6 +852,7 @@ int	vfs_modevent(module_t, int, void *);
>  void	vfs_mount_error(struct mount *, const char *, ...);
>  void	vfs_mountroot(void);			/* mount our root filesystem */
>  void	vfs_mountedfrom(struct mount *, const char *from);
> +void	vfs_notify_upper(struct vnode *, int);
>  void	vfs_oexport_conv(const struct oexport_args *oexp,
>  	    struct export_args *exp);
>  void	vfs_ref(struct mount *);
I assume this is CURRENT? Tried on STABLE but got this:
cc1: warnings being treated as errors
/usr/src/sys/kern/vfs_subr.c: In function 'vfs_notify_upper':
/usr/src/sys/kern/vfs_subr.c:2801: warning: implicit declaration of 
function 'VFS_PROLOGUE'
/usr/src/sys/kern/vfs_subr.c:2801: warning: nested extern declaration of 
'VFS_PROLOGUE' [-Wnested-externs]
/usr/src/sys/kern/vfs_subr.c:2801: warning: implicit declaration of 
function 'VFS_EPILOGUE'
/usr/src/sys/kern/vfs_subr.c:2801: warning: nested extern declaration of 
'VFS_EPILOGUE' [-Wnested-externs]
*** [vfs_subr.o] Error code 1

/glz






Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BFD5A40BC867D1DA2AAEFF16>