Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 5 Dec 2019 15:35:44 +0200
From:      Konstantin Belousov <kostikbel@gmail.com>
To:        Kubilay Kocak <koobs@FreeBSD.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   Re: svn commit: r355407 - head/sys/fs/tmpfs
Message-ID:  <20191205133544.GE2744@kib.kiev.ua>
In-Reply-To: <d8217672-6c1c-e4a0-c4e6-7ea4a8a9721c@FreeBSD.org>
References:  <201912050003.xB503HtP008634@repo.freebsd.org> <d8217672-6c1c-e4a0-c4e6-7ea4a8a9721c@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Dec 05, 2019 at 02:38:07PM +1100, Kubilay Kocak wrote:
> On 5/12/2019 11:03 am, Konstantin Belousov wrote:
> > Author: kib
> > Date: Thu Dec  5 00:03:17 2019
> > New Revision: 355407
> > URL: https://svnweb.freebsd.org/changeset/base/355407
> 
> Could you elaborate on the why/rationale?
> 
> Is there memory wastage/duplication, bug(s), performance or 
> development/maintenance benefit?
Each mount/unmount of tmpfs created and destroyed two zones, as it is
easy to see from the patch.

> 
> The review summary doesnt appear to include this information either
> 
> > Log:
> >    Stop using per-mount tmpfs zones.
> >    
> >    Requested and reviewed by:	jeff
> >    Sponsored by:	The FreeBSD Foundation
> >    MFC after:	1 week
> >    Differential revision:	https://reviews.freebsd.org/D22643
> > 
> > Modified:
> >    head/sys/fs/tmpfs/tmpfs.h
> >    head/sys/fs/tmpfs/tmpfs_subr.c
> >    head/sys/fs/tmpfs/tmpfs_vfsops.c
> > 
> > Modified: head/sys/fs/tmpfs/tmpfs.h
> > ==============================================================================
> > --- head/sys/fs/tmpfs/tmpfs.h	Wed Dec  4 23:24:40 2019	(r355406)
> > +++ head/sys/fs/tmpfs/tmpfs.h	Thu Dec  5 00:03:17 2019	(r355407)
> > @@ -378,10 +378,6 @@ struct tmpfs_mount {
> >   	/* All node lock to protect the node list and tmp_pages_used. */
> >   	struct mtx		tm_allnode_lock;
> >   
> > -	/* Zones used to store file system meta data, per tmpfs mount. */
> > -	uma_zone_t		tm_dirent_pool;
> > -	uma_zone_t		tm_node_pool;
> > -
> >   	/* Read-only status. */
> >   	bool			tm_ronly;
> >   	/* Do not use namecache. */
> > @@ -493,8 +489,9 @@ struct tmpfs_dirent *tmpfs_dir_next(struct tmpfs_node
> >   #endif
> >   
> >   size_t tmpfs_mem_avail(void);
> > -
> >   size_t tmpfs_pages_used(struct tmpfs_mount *tmp);
> > +void tmpfs_subr_init(void);
> > +void tmpfs_subr_uninit(void);
> >   
> >   #endif
> >   
> > 
> > Modified: head/sys/fs/tmpfs/tmpfs_subr.c
> > ==============================================================================
> > --- head/sys/fs/tmpfs/tmpfs_subr.c	Wed Dec  4 23:24:40 2019	(r355406)
> > +++ head/sys/fs/tmpfs/tmpfs_subr.c	Thu Dec  5 00:03:17 2019	(r355407)
> > @@ -72,7 +72,74 @@ SYSCTL_NODE(_vfs, OID_AUTO, tmpfs, CTLFLAG_RW, 0, "tmp
> >   
> >   static long tmpfs_pages_reserved = TMPFS_PAGES_MINRESERVED;
> >   
> > +static uma_zone_t tmpfs_dirent_pool;
> > +static uma_zone_t tmpfs_node_pool;
> > +
> >   static int
> > +tmpfs_node_ctor(void *mem, int size, void *arg, int flags)
> > +{
> > +	struct tmpfs_node *node;
> > +
> > +	node = mem;
> > +	node->tn_gen++;
> > +	node->tn_size = 0;
> > +	node->tn_status = 0;
> > +	node->tn_flags = 0;
> > +	node->tn_links = 0;
> > +	node->tn_vnode = NULL;
> > +	node->tn_vpstate = 0;
> > +	return (0);
> > +}
> > +
> > +static void
> > +tmpfs_node_dtor(void *mem, int size, void *arg)
> > +{
> > +	struct tmpfs_node *node;
> > +
> > +	node = mem;
> > +	node->tn_type = VNON;
> > +}
> > +
> > +static int
> > +tmpfs_node_init(void *mem, int size, int flags)
> > +{
> > +	struct tmpfs_node *node;
> > +
> > +	node = mem;
> > +	node->tn_id = 0;
> > +	mtx_init(&node->tn_interlock, "tmpfsni", NULL, MTX_DEF);
> > +	node->tn_gen = arc4random();
> > +	return (0);
> > +}
> > +
> > +static void
> > +tmpfs_node_fini(void *mem, int size)
> > +{
> > +	struct tmpfs_node *node;
> > +
> > +	node = mem;
> > +	mtx_destroy(&node->tn_interlock);
> > +}
> > +
> > +void
> > +tmpfs_subr_init(void)
> > +{
> > +	tmpfs_dirent_pool = uma_zcreate("TMPFS dirent",
> > +	    sizeof(struct tmpfs_dirent), NULL, NULL, NULL, NULL,
> > +	    UMA_ALIGN_PTR, 0);
> > +	tmpfs_node_pool = uma_zcreate("TMPFS node",
> > +	    sizeof(struct tmpfs_node), tmpfs_node_ctor, tmpfs_node_dtor,
> > +	    tmpfs_node_init, tmpfs_node_fini, UMA_ALIGN_PTR, 0);
> > +}
> > +
> > +void
> > +tmpfs_subr_uninit(void)
> > +{
> > +	uma_zdestroy(tmpfs_node_pool);
> > +	uma_zdestroy(tmpfs_dirent_pool);
> > +}
> > +
> > +static int
> >   sysctl_mem_reserved(SYSCTL_HANDLER_ARGS)
> >   {
> >   	int error;
> > @@ -219,8 +286,7 @@ tmpfs_alloc_node(struct mount *mp, struct tmpfs_mount
> >   	if ((mp->mnt_kern_flag & MNT_RDONLY) != 0)
> >   		return (EROFS);
> >   
> > -	nnode = (struct tmpfs_node *)uma_zalloc_arg(tmp->tm_node_pool, tmp,
> > -	    M_WAITOK);
> > +	nnode = uma_zalloc_arg(tmpfs_node_pool, tmp, M_WAITOK);
> >   
> >   	/* Generic initialization. */
> >   	nnode->tn_type = type;
> > @@ -367,7 +433,7 @@ tmpfs_free_node_locked(struct tmpfs_mount *tmp, struct
> >   		panic("tmpfs_free_node: type %p %d", node, (int)node->tn_type);
> >   	}
> >   
> > -	uma_zfree(tmp->tm_node_pool, node);
> > +	uma_zfree(tmpfs_node_pool, node);
> >   	TMPFS_LOCK(tmp);
> >   	tmpfs_free_tmp(tmp);
> >   	return (true);
> > @@ -434,7 +500,7 @@ tmpfs_alloc_dirent(struct tmpfs_mount *tmp, struct tmp
> >   {
> >   	struct tmpfs_dirent *nde;
> >   
> > -	nde = uma_zalloc(tmp->tm_dirent_pool, M_WAITOK);
> > +	nde = uma_zalloc(tmpfs_dirent_pool, M_WAITOK);
> >   	nde->td_node = node;
> >   	if (name != NULL) {
> >   		nde->ud.td_name = malloc(len, M_TMPFSNAME, M_WAITOK);
> > @@ -470,7 +536,7 @@ tmpfs_free_dirent(struct tmpfs_mount *tmp, struct tmpf
> >   	}
> >   	if (!tmpfs_dirent_duphead(de) && de->ud.td_name != NULL)
> >   		free(de->ud.td_name, M_TMPFSNAME);
> > -	uma_zfree(tmp->tm_dirent_pool, de);
> > +	uma_zfree(tmpfs_dirent_pool, de);
> >   }
> >   
> >   void
> > 
> > Modified: head/sys/fs/tmpfs/tmpfs_vfsops.c
> > ==============================================================================
> > --- head/sys/fs/tmpfs/tmpfs_vfsops.c	Wed Dec  4 23:24:40 2019	(r355406)
> > +++ head/sys/fs/tmpfs/tmpfs_vfsops.c	Thu Dec  5 00:03:17 2019	(r355407)
> > @@ -99,49 +99,6 @@ static const char *tmpfs_updateopts[] = {
> >   	"from", "export", "size", NULL
> >   };
> >   
> > -static int
> > -tmpfs_node_ctor(void *mem, int size, void *arg, int flags)
> > -{
> > -	struct tmpfs_node *node = (struct tmpfs_node *)mem;
> > -
> > -	node->tn_gen++;
> > -	node->tn_size = 0;
> > -	node->tn_status = 0;
> > -	node->tn_flags = 0;
> > -	node->tn_links = 0;
> > -	node->tn_vnode = NULL;
> > -	node->tn_vpstate = 0;
> > -
> > -	return (0);
> > -}
> > -
> > -static void
> > -tmpfs_node_dtor(void *mem, int size, void *arg)
> > -{
> > -	struct tmpfs_node *node = (struct tmpfs_node *)mem;
> > -	node->tn_type = VNON;
> > -}
> > -
> > -static int
> > -tmpfs_node_init(void *mem, int size, int flags)
> > -{
> > -	struct tmpfs_node *node = (struct tmpfs_node *)mem;
> > -	node->tn_id = 0;
> > -
> > -	mtx_init(&node->tn_interlock, "tmpfs node interlock", NULL, MTX_DEF);
> > -	node->tn_gen = arc4random();
> > -
> > -	return (0);
> > -}
> > -
> > -static void
> > -tmpfs_node_fini(void *mem, int size)
> > -{
> > -	struct tmpfs_node *node = (struct tmpfs_node *)mem;
> > -
> > -	mtx_destroy(&node->tn_interlock);
> > -}
> > -
> >   /*
> >    * Handle updates of time from writes to mmaped regions.  Use
> >    * MNT_VNODE_FOREACH_ALL instead of MNT_VNODE_FOREACH_ACTIVE, since
> > @@ -481,12 +438,6 @@ tmpfs_mount(struct mount *mp)
> >   	tmp->tm_pages_max = pages;
> >   	tmp->tm_pages_used = 0;
> >   	new_unrhdr64(&tmp->tm_ino_unr, 2);
> > -	tmp->tm_dirent_pool = uma_zcreate("TMPFS dirent",
> > -	    sizeof(struct tmpfs_dirent), NULL, NULL, NULL, NULL,
> > -	    UMA_ALIGN_PTR, 0);
> > -	tmp->tm_node_pool = uma_zcreate("TMPFS node",
> > -	    sizeof(struct tmpfs_node), tmpfs_node_ctor, tmpfs_node_dtor,
> > -	    tmpfs_node_init, tmpfs_node_fini, UMA_ALIGN_PTR, 0);
> >   	tmp->tm_ronly = (mp->mnt_flag & MNT_RDONLY) != 0;
> >   	tmp->tm_nonc = nonc;
> >   
> > @@ -495,8 +446,6 @@ tmpfs_mount(struct mount *mp)
> >   	    root_mode & ALLPERMS, NULL, NULL, VNOVAL, &root);
> >   
> >   	if (error != 0 || root == NULL) {
> > -		uma_zdestroy(tmp->tm_node_pool);
> > -		uma_zdestroy(tmp->tm_dirent_pool);
> >   		free(tmp, M_TMPFSMNT);
> >   		return (error);
> >   	}
> > @@ -590,9 +539,6 @@ tmpfs_free_tmp(struct tmpfs_mount *tmp)
> >   	}
> >   	TMPFS_UNLOCK(tmp);
> >   
> > -	uma_zdestroy(tmp->tm_dirent_pool);
> > -	uma_zdestroy(tmp->tm_node_pool);
> > -
> >   	mtx_destroy(&tmp->tm_allnode_lock);
> >   	MPASS(tmp->tm_pages_used == 0);
> >   	MPASS(tmp->tm_nodes_inuse == 0);
> > @@ -702,10 +648,23 @@ tmpfs_susp_clean(struct mount *mp __unused)
> >   {
> >   }
> >   
> > +static int
> > +tmpfs_init(struct vfsconf *conf)
> > +{
> > +	tmpfs_subr_init();
> > +	return (0);
> > +}
> > +
> > +static int
> > +tmpfs_uninit(struct vfsconf *conf)
> > +{
> > +	tmpfs_subr_uninit();
> > +	return (0);
> > +}
> > +
> >   /*
> >    * tmpfs vfs operations.
> >    */
> > -
> >   struct vfsops tmpfs_vfsops = {
> >   	.vfs_mount =			tmpfs_mount,
> >   	.vfs_unmount =			tmpfs_unmount,
> > @@ -715,5 +674,7 @@ struct vfsops tmpfs_vfsops = {
> >   	.vfs_fhtovp =			tmpfs_fhtovp,
> >   	.vfs_sync =			tmpfs_sync,
> >   	.vfs_susp_clean =		tmpfs_susp_clean,
> > +	.vfs_init =			tmpfs_init,
> > +	.vfs_uninit =			tmpfs_uninit,
> >   };
> >   VFS_SET(tmpfs_vfsops, tmpfs, VFCF_JAIL);
> > _______________________________________________
> > svn-src-head@freebsd.org mailing list
> > https://lists.freebsd.org/mailman/listinfo/svn-src-head
> > To unsubscribe, send any mail to "svn-src-head-unsubscribe@freebsd.org"
> > 



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