Re: CVS commit: src/sys/fs/tmpfs

2020-05-19 Thread Andrew Doran
On Tue, May 19, 2020 at 11:09:07PM +, Andrew Doran wrote:

> vnode locks are not held during getpages/putpages.

^ for fault handling, anyway.  for read/write they are held by the caller to
ubc_uiomove().

Andrew


Re: CVS commit: src/sys/fs/tmpfs

2020-05-19 Thread Andrew Doran
On Sun, May 17, 2020 at 11:49:52PM +, m...@netbsd.org wrote:

> On Sun, May 17, 2020 at 09:47:50PM +, m...@netbsd.org wrote:
> > On Sun, May 17, 2020 at 07:39:15PM +, Andrew Doran wrote:
> > > Module Name:  src
> > > Committed By: ad
> > > Date: Sun May 17 19:39:15 UTC 2020
> > > 
> > > Modified Files:
> > >   src/sys/fs/tmpfs: tmpfs.h tmpfs_subr.c tmpfs_vnops.c
> > > 
> > > Log Message:
> > > PR kern/55268: tmpfs is slow
> > > 
> > > tmpfs_getpages(): ...implement lazy update of atime/mtime.
> > > 
> > 
> > I'm confused about how this makes sense. Can you elaborate?
> > Presumably RAM is as fast as other RAM.
> 
> riastradh responded to this elsewhere: avoid atomic ops

Right, also to:

- avoid taking a mutex to serialize the update to the time fields in the
  tmpfs node.  vnode locks are not held during getpages/putpages.  the
  vmobjlock is held here and often read-held so there can be lots of
  concurrent access.

- avoid doing a writeback on the tmpfs_node, which if it's heavily shared
  (consider for example ld.so or libc.so) involves lots of cache coherency
  overhead.

- avoid querying the clock.

Andrew


Re: CVS commit: src/sys/fs/tmpfs

2020-05-17 Thread maya
On Sun, May 17, 2020 at 09:47:50PM +, m...@netbsd.org wrote:
> On Sun, May 17, 2020 at 07:39:15PM +, Andrew Doran wrote:
> > Module Name:src
> > Committed By:   ad
> > Date:   Sun May 17 19:39:15 UTC 2020
> > 
> > Modified Files:
> > src/sys/fs/tmpfs: tmpfs.h tmpfs_subr.c tmpfs_vnops.c
> > 
> > Log Message:
> > PR kern/55268: tmpfs is slow
> > 
> > tmpfs_getpages(): ...implement lazy update of atime/mtime.
> > 
> 
> I'm confused about how this makes sense. Can you elaborate?
> Presumably RAM is as fast as other RAM.

riastradh responded to this elsewhere: avoid atomic ops

thanks


Re: CVS commit: src/sys/fs/tmpfs

2020-05-17 Thread maya
On Sun, May 17, 2020 at 07:39:15PM +, Andrew Doran wrote:
> Module Name:  src
> Committed By: ad
> Date: Sun May 17 19:39:15 UTC 2020
> 
> Modified Files:
>   src/sys/fs/tmpfs: tmpfs.h tmpfs_subr.c tmpfs_vnops.c
> 
> Log Message:
> PR kern/55268: tmpfs is slow
> 
> tmpfs_getpages(): ...implement lazy update of atime/mtime.
> 

I'm confused about how this makes sense. Can you elaborate?
Presumably RAM is as fast as other RAM.


CVS commit: src/sys/fs/tmpfs

2019-10-04 Thread matthew green
Module Name:src
Committed By:   mrg
Date:   Fri Oct  4 12:34:40 UTC 2019

Modified Files:
src/sys/fs/tmpfs: tmpfs_vfsops.c

Log Message:
remove an always false check and its' "This can never happen?" comment.


To generate a diff of this commit:
cvs rdiff -u -r1.74 -r1.75 src/sys/fs/tmpfs/tmpfs_vfsops.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/fs/tmpfs/tmpfs_vfsops.c
diff -u src/sys/fs/tmpfs/tmpfs_vfsops.c:1.74 src/sys/fs/tmpfs/tmpfs_vfsops.c:1.75
--- src/sys/fs/tmpfs/tmpfs_vfsops.c:1.74	Tue Jan  1 10:06:54 2019
+++ src/sys/fs/tmpfs/tmpfs_vfsops.c	Fri Oct  4 12:34:40 2019
@@ -1,4 +1,4 @@
-/*	$NetBSD: tmpfs_vfsops.c,v 1.74 2019/01/01 10:06:54 hannken Exp $	*/
+/*	$NetBSD: tmpfs_vfsops.c,v 1.75 2019/10/04 12:34:40 mrg Exp $	*/
 
 /*
  * Copyright (c) 2005, 2006, 2007 The NetBSD Foundation, Inc.
@@ -42,7 +42,7 @@
  */
 
 #include 
-__KERNEL_RCSID(0, "$NetBSD: tmpfs_vfsops.c,v 1.74 2019/01/01 10:06:54 hannken Exp $");
+__KERNEL_RCSID(0, "$NetBSD: tmpfs_vfsops.c,v 1.75 2019/10/04 12:34:40 mrg Exp $");
 
 #include 
 #include 
@@ -132,10 +132,6 @@ tmpfs_mount(struct mount *mp, const char
 	if (args->ta_root_uid == VNOVAL || args->ta_root_gid == VNOVAL)
 		return EINVAL;
 
-	/* This can never happen? */
-	if ((args->ta_root_mode & ALLPERMS) == VNOVAL)
-		return EINVAL;
-
 	/* Get the memory usage limit for this file-system. */
 	if (args->ta_size_max < PAGE_SIZE) {
 		memlimit = UINT64_MAX;



CVS commit: src/sys/fs/tmpfs

2019-10-04 Thread matthew green
Module Name:src
Committed By:   mrg
Date:   Fri Oct  4 12:34:40 UTC 2019

Modified Files:
src/sys/fs/tmpfs: tmpfs_vfsops.c

Log Message:
remove an always false check and its' "This can never happen?" comment.


To generate a diff of this commit:
cvs rdiff -u -r1.74 -r1.75 src/sys/fs/tmpfs/tmpfs_vfsops.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.



CVS commit: src/sys/fs/tmpfs

2019-07-13 Thread Maxime Villard
Module Name:src
Committed By:   maxv
Date:   Sun Jul 14 05:58:44 UTC 2019

Modified Files:
src/sys/fs/tmpfs: tmpfs_rename.c

Log Message:
Fix uninitialized variable: if 'tvp' is NULL, '*tdep' is not initialized.
This could have caused the KASSERT to wrongfully fire.

ok riastradh@


To generate a diff of this commit:
cvs rdiff -u -r1.8 -r1.9 src/sys/fs/tmpfs/tmpfs_rename.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.



CVS commit: src/sys/fs/tmpfs

2019-07-13 Thread Maxime Villard
Module Name:src
Committed By:   maxv
Date:   Sun Jul 14 05:58:44 UTC 2019

Modified Files:
src/sys/fs/tmpfs: tmpfs_rename.c

Log Message:
Fix uninitialized variable: if 'tvp' is NULL, '*tdep' is not initialized.
This could have caused the KASSERT to wrongfully fire.

ok riastradh@


To generate a diff of this commit:
cvs rdiff -u -r1.8 -r1.9 src/sys/fs/tmpfs/tmpfs_rename.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/fs/tmpfs/tmpfs_rename.c
diff -u src/sys/fs/tmpfs/tmpfs_rename.c:1.8 src/sys/fs/tmpfs/tmpfs_rename.c:1.9
--- src/sys/fs/tmpfs/tmpfs_rename.c:1.8	Mon Jul  6 10:24:59 2015
+++ src/sys/fs/tmpfs/tmpfs_rename.c	Sun Jul 14 05:58:44 2019
@@ -1,4 +1,4 @@
-/*	$NetBSD: tmpfs_rename.c,v 1.8 2015/07/06 10:24:59 wiz Exp $	*/
+/*	$NetBSD: tmpfs_rename.c,v 1.9 2019/07/14 05:58:44 maxv Exp $	*/
 
 /*-
  * Copyright (c) 2012 The NetBSD Foundation, Inc.
@@ -34,7 +34,7 @@
  */
 
 #include 
-__KERNEL_RCSID(0, "$NetBSD: tmpfs_rename.c,v 1.8 2015/07/06 10:24:59 wiz Exp $");
+__KERNEL_RCSID(0, "$NetBSD: tmpfs_rename.c,v 1.9 2019/07/14 05:58:44 maxv Exp $");
 
 #include 
 #include 
@@ -282,7 +282,7 @@ tmpfs_gro_rename(struct mount *mp, kauth
 	KASSERT(tcnp != NULL);
 	KASSERT(tdep != NULL);
 	KASSERT(fdep != tdep);
-	KASSERT((*fdep) != (*tdep));
+	KASSERT((tvp == NULL) || (*fdep) != (*tdep));
 	KASSERT((*fdep) != NULL);
 	KASSERT((*fdep)->td_node == VP_TO_TMPFS_NODE(fvp));
 	KASSERT((tvp == NULL) || ((*tdep) != NULL));



CVS commit: src/sys/fs/tmpfs

2019-07-13 Thread Maxime Villard
Module Name:src
Committed By:   maxv
Date:   Sat Jul 13 14:24:37 UTC 2019

Modified Files:
src/sys/fs/tmpfs: tmpfs_mem.c

Log Message:
Remove the roundups, they are incorrect and cause memcmp to wrongfully fail
because of uninitialized bytes at the end of the buffers.

ok rmind@


To generate a diff of this commit:
cvs rdiff -u -r1.9 -r1.10 src/sys/fs/tmpfs/tmpfs_mem.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.



CVS commit: src/sys/fs/tmpfs

2019-07-13 Thread Maxime Villard
Module Name:src
Committed By:   maxv
Date:   Sat Jul 13 14:24:37 UTC 2019

Modified Files:
src/sys/fs/tmpfs: tmpfs_mem.c

Log Message:
Remove the roundups, they are incorrect and cause memcmp to wrongfully fail
because of uninitialized bytes at the end of the buffers.

ok rmind@


To generate a diff of this commit:
cvs rdiff -u -r1.9 -r1.10 src/sys/fs/tmpfs/tmpfs_mem.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/fs/tmpfs/tmpfs_mem.c
diff -u src/sys/fs/tmpfs/tmpfs_mem.c:1.9 src/sys/fs/tmpfs/tmpfs_mem.c:1.10
--- src/sys/fs/tmpfs/tmpfs_mem.c:1.9	Mon Aug 22 23:07:36 2016
+++ src/sys/fs/tmpfs/tmpfs_mem.c	Sat Jul 13 14:24:37 2019
@@ -1,4 +1,4 @@
-/*	$NetBSD: tmpfs_mem.c,v 1.9 2016/08/22 23:07:36 skrll Exp $	*/
+/*	$NetBSD: tmpfs_mem.c,v 1.10 2019/07/13 14:24:37 maxv Exp $	*/
 
 /*
  * Copyright (c) 2010, 2011 The NetBSD Foundation, Inc.
@@ -35,7 +35,7 @@
  */
 
 #include 
-__KERNEL_RCSID(0, "$NetBSD: tmpfs_mem.c,v 1.9 2016/08/22 23:07:36 skrll Exp $");
+__KERNEL_RCSID(0, "$NetBSD: tmpfs_mem.c,v 1.10 2019/07/13 14:24:37 maxv Exp $");
 
 #include 
 #include 
@@ -234,8 +234,8 @@ tmpfs_strname_free(struct tmpfs_mount *m
 bool
 tmpfs_strname_neqlen(struct componentname *fcnp, struct componentname *tcnp)
 {
-	const size_t fln = roundup2(fcnp->cn_namelen, TMPFS_NAME_QUANTUM);
-	const size_t tln = roundup2(tcnp->cn_namelen, TMPFS_NAME_QUANTUM);
+	const size_t fln = fcnp->cn_namelen;
+	const size_t tln = tcnp->cn_namelen;
 
 	return (fln != tln) || memcmp(fcnp->cn_nameptr, tcnp->cn_nameptr, fln);
 }



Re: CVS commit: src/sys/fs/tmpfs

2017-01-11 Thread Joerg Sonnenberger
On Wed, Jan 11, 2017 at 09:12:33PM +, David Holland wrote:
> On Wed, Jan 11, 2017 at 12:12:33PM +, Joerg Sonnenberger wrote:
>  > Modified Files:
>  >src/sys/fs/tmpfs: tmpfs_vnops.c
>  > 
>  > Log Message:
>  > Remove RO check in tmpfs_putpages for now, the syncer doesn't like the
>  > error code.
> 
> Either removing it is wrong or it should be changed to KASSERT :-)

So the problem is that the syncer will unconditionally call putpages
e.g. on umount. It might need a two stage approach for dealing with
dirty mmapped pages to implement properly, but for the use cases of
read-only tmpfs I have (and likely others), it doesn't really matter.
E.g. if you want to build multiple build chroots without paying for the
extreme locking penalty of nullfs.

Joerg


Re: CVS commit: src/sys/fs/tmpfs

2017-01-11 Thread David Holland
On Wed, Jan 11, 2017 at 12:12:33PM +, Joerg Sonnenberger wrote:
 > Modified Files:
 >  src/sys/fs/tmpfs: tmpfs_vnops.c
 > 
 > Log Message:
 > Remove RO check in tmpfs_putpages for now, the syncer doesn't like the
 > error code.

Either removing it is wrong or it should be changed to KASSERT :-)

-- 
David A. Holland
dholl...@netbsd.org


Re: CVS commit: src/sys/fs/tmpfs

2014-01-08 Thread Mindaugas Rasiukevicius
J. Hannken-Illjes hann...@eis.cs.tu-bs.de wrote:
 On Jan 8, 2014, at 5:11 PM, pedro martelletto pe...@netbsd.org wrote:
 
  Module Name:src
  Committed By:   pedro
  Date:   Wed Jan  8 16:11:04 UTC 2014
  
  Modified Files:
  src/sys/fs/tmpfs: tmpfs_subr.c
  
  Log Message:
  Allocate direntp on the stack in tmpfs_dir_getdents(), thus saving
  calls to kmem_zalloc() and kmem_free(); OK rmind@. From OpenBSD.
 
 Is it really a good idea to allocate 528 bytes on the kernel stack?
 File systems nest and already use much stack space.

It is harmless in this case since we get a few or more pages for the stack.

 Looks better to use a pool_cache.

It is worth to create a separate pool_cache(9) only if the allocations can
potentially be very intensive.

-- 
Mindaugas


Re: CVS commit: src/sys/fs/tmpfs

2014-01-08 Thread J. Hannken-Illjes
On Jan 8, 2014, at 5:11 PM, pedro martelletto pe...@netbsd.org wrote:

 Module Name:  src
 Committed By: pedro
 Date: Wed Jan  8 16:11:04 UTC 2014
 
 Modified Files:
   src/sys/fs/tmpfs: tmpfs_subr.c
 
 Log Message:
 Allocate direntp on the stack in tmpfs_dir_getdents(), thus saving
 calls to kmem_zalloc() and kmem_free(); OK rmind@. From OpenBSD.


Is it really a good idea to allocate 528 bytes on the kernel stack?
File systems nest and already use much stack space.
Looks better to use a pool_cache.

--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)



Re: CVS commit: src/sys/fs/tmpfs

2014-01-04 Thread J. Hannken-Illjes
On Jan 3, 2014, at 10:18 PM, J. Hannken-Illjes hann...@eis.cs.tu-bs.de wrote:

 On Jan 3, 2014, at 10:13 PM, Mindaugas Rasiukevicius rm...@netbsd.org wrote:
 
 Juergen Hannken-Illjes hann...@netbsd.org wrote:
 Module Name:src
 Committed By:   hannken
 Date:   Fri Jan  3 09:53:12 UTC 2014
 
 Modified Files:
 src/sys/fs/tmpfs: tmpfs_subr.c tmpfs_vnops.c
 
 Log Message:
 Fix a race where thread1 runs VOP_REMOVE() and gets preempted in
 tmpfs_reclaim() before the call to tmpfs_free_node().  Thread2
 runs VFS_FHTOVP() and gets a new vnode attached to the node thread1
 is about to destroy.
 
 Change tmpfs_alloc_node() to always assign non-zero generation number
 and tmpfs_inactive() to set the generation number of unlinked nodes
 to zero.
 
 Can you explain how does this help?  It still seems racy to me.
 
 Please describe the race in more detail.  Tmpfs_fhtovp() will fail
 as soon as an unlinked tmpfs node drops its last vnode reference.

Ok -- got it.  We check the generation number too early in tmpfs_fhtovp().

Should be fixed with tmpfs_vfsops.c Rev. 1.55

--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)



Re: CVS commit: src/sys/fs/tmpfs

2014-01-03 Thread Mindaugas Rasiukevicius
Juergen Hannken-Illjes hann...@netbsd.org wrote:
 Module Name:  src
 Committed By: hannken
 Date: Fri Jan  3 09:53:12 UTC 2014
 
 Modified Files:
   src/sys/fs/tmpfs: tmpfs_subr.c tmpfs_vnops.c
 
 Log Message:
 Fix a race where thread1 runs VOP_REMOVE() and gets preempted in
 tmpfs_reclaim() before the call to tmpfs_free_node().  Thread2
 runs VFS_FHTOVP() and gets a new vnode attached to the node thread1
 is about to destroy.
 
 Change tmpfs_alloc_node() to always assign non-zero generation number
 and tmpfs_inactive() to set the generation number of unlinked nodes
 to zero.

Can you explain how does this help?  It still seems racy to me.
Why not just check for tn_links == 0 in tmpfs_fhtovp()?

-- 
Mindaugas


Re: CVS commit: src/sys/fs/tmpfs

2014-01-03 Thread J. Hannken-Illjes
On Jan 3, 2014, at 10:13 PM, Mindaugas Rasiukevicius rm...@netbsd.org wrote:

 Juergen Hannken-Illjes hann...@netbsd.org wrote:
 Module Name: src
 Committed By:hannken
 Date:Fri Jan  3 09:53:12 UTC 2014
 
 Modified Files:
  src/sys/fs/tmpfs: tmpfs_subr.c tmpfs_vnops.c
 
 Log Message:
 Fix a race where thread1 runs VOP_REMOVE() and gets preempted in
 tmpfs_reclaim() before the call to tmpfs_free_node().  Thread2
 runs VFS_FHTOVP() and gets a new vnode attached to the node thread1
 is about to destroy.
 
 Change tmpfs_alloc_node() to always assign non-zero generation number
 and tmpfs_inactive() to set the generation number of unlinked nodes
 to zero.
 
 Can you explain how does this help?  It still seems racy to me.

Please describe the race in more detail.  Tmpfs_fhtovp() will fail
as soon as an unlinked tmpfs node drops its last vnode reference.

 Why not just check for tn_links == 0 in tmpfs_fhtovp()?

Because it is ok as long as the corresponding vnode is open/referenced.

--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)



Re: CVS commit: src/sys/fs/tmpfs

2013-11-10 Thread J. Hannken-Illjes

On Nov 8, 2013, at 4:44 PM, Mindaugas Rasiukevicius rm...@netbsd.org wrote:

 Module Name:  src
 Committed By: rmind
 Date: Fri Nov  8 15:44:23 UTC 2013
 
 Modified Files:
   src/sys/fs/tmpfs: tmpfs.h tmpfs_rename.c tmpfs_subr.c tmpfs_vfsops.c
   tmpfs_vnops.c
 
 Log Message:
 tmpfs: replace the broken tmpfs_dircookie() logic which uses the node
 address truncated to 31 bits (required for 32-bit readdir compatibility,
 e.g. linux32).  Instead, assign 2^31 range using the following logic:
 - The first half of the 2^31 is assigned incrementally (the fast path).
 - When exceeded, use the second half of 2^31, but manage with vmem(9).
 
 It will require 2 billion files per-directory to trigger vmem(9) usage.
 Also, while here, add some fixes for tmpfs_unmount().
 
 Should fix PR/47739, PR/47480, PR/46088 and PR/41068.
 Thanks to wiz@ for stress testing.

The tests fs/vfs/t_union/tmpfs_basic and fs/vfs/t_union/tmpfs_whiteout
start failing after this commit.

--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)



Re: CVS commit: src/sys/fs/tmpfs

2013-11-10 Thread Mindaugas Rasiukevicius
J. Hannken-Illjes hann...@eis.cs.tu-bs.de wrote:
  Module Name:src
  Committed By:   rmind
  Date:   Fri Nov  8 15:44:23 UTC 2013
  
 ...
 
 The tests fs/vfs/t_union/tmpfs_basic and fs/vfs/t_union/tmpfs_whiteout
 start failing after this commit.
 

Fixed.

-- 
Mindaugas


Re: CVS commit: src/sys/fs/tmpfs

2011-08-18 Thread Taylor R Campbell
Forgot to add that this also fixes a space leak in tmpfs_rename,
introduced a couple months ago, which nobody reported as far as I
know.  The leak sometimes caused tmpfs_renamerace_dirs to fail with
ENOSPC.  The problem was that renaming a directory over an empty
directory would fail to decrement the target's link count enough, so
that the target would never be released.


Re: CVS commit: src/sys/fs/tmpfs

2011-08-18 Thread David Holland
On Thu, Aug 18, 2011 at 09:51:50PM +, Taylor R Campbell wrote:
  Forgot to add that this also fixes a space leak in tmpfs_rename,
  introduced a couple months ago, which nobody reported as far as I
  know.  The leak sometimes caused tmpfs_renamerace_dirs to fail with
  ENOSPC.  The problem was that renaming a directory over an empty
  directory would fail to decrement the target's link count enough, so
  that the target would never be released.

There was a thread somewhere a while back (in tech-kern I think) about
refcount leaks in tmpfs. As I recall what changed a couple months ago
was a semi-compensating error somewhere else... or maybe that was
earlier.

-- 
David A. Holland
dholl...@netbsd.org


Re: CVS commit: src/sys/fs/tmpfs

2011-05-15 Thread Matt Thomas

On May 14, 2011, at 1:43 PM, Martin Husemann wrote:

 On Sat, May 14, 2011 at 10:34:05AM +0200, Marc Balmer wrote:
 What is the current state of C99 vs. older Cs?  Do all arches /
 compilers we have support C99?
 
 We have lost the playstation2 port, because we don't have a working C99
 compiler for it (so a -current kernel can not be compiled).

No, we lost the playstation2 port because it required a special compiler
and needs lots of one-off changes in the mips code.


Re: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread David Holland
On Fri, May 13, 2011 at 06:37:53PM +0100, Mindaugas Rasiukevicius wrote:
  Generally - C99 is encouraged.  However, I disagree that variables
  should be declared in the middle of context (for a minimum scope),
  unless there is a *clear* benefit.  Otherwise, it makes code harder
  to read, especially if code fragment is long and/or complex.

The i in for (int i=0; in; i++) isn't in the middle of a context;
it's at the beginning of the for loop's context.

so it should really be considered as a different case from other
mid-block decls.

-- 
David A. Holland
dholl...@netbsd.org


Re: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread Masao Uebayashi
On Sat, May 14, 2011 at 2:37 AM, Mindaugas Rasiukevicius
rm...@netbsd.org wrote:
 Matt Thomas m...@3am-software.com wrote:

 On May 7, 2011, at 5:03 PM, Christos Zoulas wrote:

  Module Name:        src
  Committed By:       christos
  Date:               Sun May  8 00:03:35 UTC 2011
 
  Modified Files:
      src/sys/fs/tmpfs: tmpfs_vnops.c
 
  Log Message:
  no c99 please.

 The kernel explicitly allows C99 and actually C99 is encouraged.
 So that should reverted :)

 Generally - C99 is encouraged.  However, I disagree that variables
 should be declared in the middle of context (for a minimum scope),
 unless there is a *clear* benefit.  Otherwise, it makes code harder
 to read, especially if code fragment is long and/or complex.

I disagree.  If variables are declared in the middle of context, those
variables have narrower contexts.  Narrowing context is always a win
IMO.

I'd like to hear the benefit not doing this (== the old convention).


 Benefits could be, e.g. use of const or limitation of the variable
 scope for performance sensitive code, also avoiding of #ifdefs, etc.

 In this case, I used for (int i = 0; ...) because the loop was in
 the beginning of context and #ifdef DEBUG-only.

 --
 Mindaugas



Re: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread Marc Balmer
Am 10.05.11 02:34, schrieb Matt Thomas:
 Module Name:  src
 Committed By: matt
 Date: Tue May 10 00:34:26 UTC 2011
 
 Modified Files:
   src/sys/fs/tmpfs: tmpfs_vnops.c
 
 Log Message:
 yes, more C99 please (back out previous change).

After this committ/back-out/back-out-pf-the-back-out fest I have a
technical question:

What is the current state of C99 vs. older Cs?  Do all arches /
compilers we have support C99?  I assume gcc, llvm/clang are safe, but
what about pcc wrt C99?

I'd like a short clarification here, since this might influence my
coding...  tnx.

 
 
 To generate a diff of this commit:
 cvs rdiff -u -r1.79 -r1.80 src/sys/fs/tmpfs/tmpfs_vnops.c
 
 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.
 
 
 
 
 Modified files:
 
 Index: src/sys/fs/tmpfs/tmpfs_vnops.c
 diff -u src/sys/fs/tmpfs/tmpfs_vnops.c:1.79 
 src/sys/fs/tmpfs/tmpfs_vnops.c:1.80
 --- src/sys/fs/tmpfs/tmpfs_vnops.c:1.79   Sun May  8 00:03:35 2011
 +++ src/sys/fs/tmpfs/tmpfs_vnops.cTue May 10 00:34:26 2011
 @@ -1,4 +1,4 @@
 -/*   $NetBSD: tmpfs_vnops.c,v 1.79 2011/05/08 00:03:35 christos Exp $
 */
 +/*   $NetBSD: tmpfs_vnops.c,v 1.80 2011/05/10 00:34:26 matt Exp $*/
  
  /*
   * Copyright (c) 2005, 2006, 2007 The NetBSD Foundation, Inc.
 @@ -35,7 +35,7 @@
   */
  
  #include sys/cdefs.h
 -__KERNEL_RCSID(0, $NetBSD: tmpfs_vnops.c,v 1.79 2011/05/08 00:03:35 
 christos Exp $);
 +__KERNEL_RCSID(0, $NetBSD: tmpfs_vnops.c,v 1.80 2011/05/10 00:34:26 matt 
 Exp $);
  
  #include sys/param.h
  #include sys/dirent.h
 @@ -1466,8 +1466,7 @@
  
  #if defined(DEBUG)
   if (!error  pgs) {
 - int i;
 - for (i = 0; i  npages; i++) {
 + for (int i = 0; i  npages; i++) {
   KASSERT(pgs[i] != NULL);
   }
   }
 



Re: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread Anders Magnusson

On 05/14/2011 10:34 AM, Marc Balmer wrote:

Am 10.05.11 02:34, schrieb Matt Thomas:

Module Name:src
Committed By:   matt
Date:   Tue May 10 00:34:26 UTC 2011

Modified Files:
src/sys/fs/tmpfs: tmpfs_vnops.c

Log Message:
yes, more C99 please (back out previous change).

After this committ/back-out/back-out-pf-the-back-out fest I have a
technical question:

What is the current state of C99 vs. older Cs?  Do all arches /
compilers we have support C99?  I assume gcc, llvm/clang are safe, but
what about pcc wrt C99?
pcc should be C99 compliant. If something do not work as expected, I'll 
fix it.


-- Ragge


I'd like a short clarification here, since this might influence my
coding...  tnx.



To generate a diff of this commit:
cvs rdiff -u -r1.79 -r1.80 src/sys/fs/tmpfs/tmpfs_vnops.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.




Modified files:

Index: src/sys/fs/tmpfs/tmpfs_vnops.c
diff -u src/sys/fs/tmpfs/tmpfs_vnops.c:1.79 src/sys/fs/tmpfs/tmpfs_vnops.c:1.80
--- src/sys/fs/tmpfs/tmpfs_vnops.c:1.79 Sun May  8 00:03:35 2011
+++ src/sys/fs/tmpfs/tmpfs_vnops.c  Tue May 10 00:34:26 2011
@@ -1,4 +1,4 @@
-/* $NetBSD: tmpfs_vnops.c,v 1.79 2011/05/08 00:03:35 christos Exp $
*/
+/* $NetBSD: tmpfs_vnops.c,v 1.80 2011/05/10 00:34:26 matt Exp $*/

  /*
   * Copyright (c) 2005, 2006, 2007 The NetBSD Foundation, Inc.
@@ -35,7 +35,7 @@
   */

  #includesys/cdefs.h
-__KERNEL_RCSID(0, $NetBSD: tmpfs_vnops.c,v 1.79 2011/05/08 00:03:35 christos Exp 
$);
+__KERNEL_RCSID(0, $NetBSD: tmpfs_vnops.c,v 1.80 2011/05/10 00:34:26 matt Exp 
$);

  #includesys/param.h
  #includesys/dirent.h
@@ -1466,8 +1466,7 @@

  #if defined(DEBUG)
if (!error  pgs) {
-   int i;
-   for (i = 0; i  npages; i++) {
+   for (int i = 0; i  npages; i++) {
KASSERT(pgs[i] != NULL);
}
}





Re: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread Marc Balmer
Am 14.05.11 10:45, schrieb Anders Magnusson:
[...]

 What is the current state of C99 vs. older Cs?  Do all arches /
 compilers we have support C99?  I assume gcc, llvm/clang are safe, but
 what about pcc wrt C99?
 pcc should be C99 compliant. If something do not work as expected, I'll
 fix it.

cool!  thanks for the update.

 
 -- Ragge

[...]


Re: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread Iain Hibbert
On Sat, 14 May 2011, Marc Balmer wrote:

 What is the current state of C99 vs. older Cs?  Do all arches /
 compilers we have support C99?  I assume gcc, llvm/clang are safe, but
 what about pcc wrt C99?

 I'd like a short clarification here, since this might influence my
 coding...  tnx.

pcc is a C99 compiler (with some gcc compatibility) which is still under
development, though C99 feature support is complete.

pcc is capable of building large parts of userland (I am running with
/bin, /sbin and /usr/bin currently, and am going to install /usr/sbin
soon), plus i386 kernels though there are still bugs to track down (eg no
system crash but a build.sh failed, I think due to some corrupted files..)

I'm thinking that though we have some support for C99 in tree, the
'official' position is that llvm/clang and pcc are not yet supported (eg
there has been no such announcement of support, llvm/clang source is not
yet in tree and the in-tree pcc is a year out of date).

So IMO, apart from style issues (which it would be nice to update the
share/misc/style document with), it should be safe to use any C99 features
and, excepting some of the build tools which may be needed for
bootstrapping, I don't think its useful to restrict ourselves to an older
standard..

iain


Re: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread Mindaugas Rasiukevicius
Masao Uebayashi uebay...@gmail.com wrote:
  The kernel explicitly allows C99 and actually C99 is encouraged.
  So that should reverted :)
 
  Generally - C99 is encouraged.  However, I disagree that variables
  should be declared in the middle of context (for a minimum scope),
  unless there is a *clear* benefit.  Otherwise, it makes code harder
  to read, especially if code fragment is long and/or complex.
 
 I disagree.  If variables are declared in the middle of context, those
 variables have narrower contexts.  Narrowing context is always a win
 IMO.
 
 I'd like to hear the benefit not doing this (== the old convention).

Benefit is code readability.  It is easier to track the variables when
they are defined and initialised in the beginning of context.

If code is longer and/or complex - it likely has loops or conditional
statements, and variables can be defined in the beginning of *their*
context (basically, after { bracket).  This is very encouraged.

For other cases, compilers do a good job anyway (if you do not believe
me - check with objdump) and there is no need to hurt code readability.  

 
  Benefits could be, e.g. use of const or limitation of the variable
  scope for performance sensitive code, also avoiding of #ifdefs, etc.
 
  In this case, I used for (int i = 0; ...) because the loop was in
  the beginning of context and #ifdef DEBUG-only.
 
  --
  Mindaugas
 

-- 
Mindaugas


Re: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread Christos Zoulas
On May 14, 12:00pm, rm...@netbsd.org (Mindaugas Rasiukevicius) wrote:
-- Subject: Re: CVS commit: src/sys/fs/tmpfs

| Benefit is code readability.  It is easier to track the variables when
| they are defined and initialised in the beginning of context.
| 
| If code is longer and/or complex - it likely has loops or conditional
| statements, and variables can be defined in the beginning of *their*
| context (basically, after { bracket).  This is very encouraged.
| 
| For other cases, compilers do a good job anyway (if you do not believe
| me - check with objdump) and there is no need to hurt code readability.  

This whole discussion misses the point. The reason I changed the
code back to regular c from c99 is because the code did not compile.
This happened because rump did not pass -std=gnu99 in the compiler
flags. Now I did 'for (size_t i = 0;' in sys/crypto and the regression
tests failed. If we are going to be compiling the kernel in c99
mode, then I suggest that we do the same for userland instead of
turning it on for userland piecemeal.

christos


Re: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread David Holland
On Sat, May 14, 2011 at 12:07:20PM -0400, Christos Zoulas wrote:
  If we are going to be compiling the kernel in c99 mode, then I
  suggest that we do the same for userland instead of turning it on
  for userland piecemeal.

+1

is there anything we expect to break?

-- 
David A. Holland
dholl...@netbsd.org


Re: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread Alexander Nasonov
14.05.2011, 10:38, Masao Uebayashi uebay...@gmail.com:
 I disagree.  If variables are declared in the middle of context, those
 variables have narrower contexts.  Narrowing context is always a win
 IMO.

That's true only if you don't use gotos. Otherwise, you might jump past
an initialization point with obvious consequences. If I remember correctly,
a compiler reports an error only for variable length arrays in this case.
-- 
Alex


Change the subject (defaulting to c99 in userland) was: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread Bernd Ernesti
On Sat, May 14, 2011 at 04:45:07PM +, David Holland wrote:
 On Sat, May 14, 2011 at 12:07:20PM -0400, Christos Zoulas wrote:
   If we are going to be compiling the kernel in c99 mode, then I
   suggest that we do the same for userland instead of turning it on
   for userland piecemeal.
 
 +1
 
 is there anything we expect to break?

It would be nice if this thread was not just on source-changes-d.

Bernd



Re: CVS commit: src/sys/fs/tmpfs

2011-05-14 Thread Martin Husemann
On Sat, May 14, 2011 at 10:34:05AM +0200, Marc Balmer wrote:
 What is the current state of C99 vs. older Cs?  Do all arches /
 compilers we have support C99?

We have lost the playstation2 port, because we don't have a working C99
compiler for it (so a -current kernel can not be compiled).

Martin


Re: CVS commit: src/sys/fs/tmpfs

2011-05-10 Thread Iain Hibbert
On Tue, 10 May 2011, Takahiro Kambe wrote:

 In message 20110509170006.GA15831@marx.bitnet
   on Mon, 9 May 2011 20:00:06 +0300,
   Jukka Ruohonen jruoho...@iki.fi wrote:
  On Mon, May 09, 2011 at 06:50:08PM +0200, Adam Hoka wrote:
  So can we use for (int i = 0; ... ? :p
 lint(1) support them?

Yes, lint(1) knows about it, see lint(7):

  325 variable declaration in for loop

and lint -S (use C9X mode) disables this warning..

iain


Re: CVS commit: src/sys/fs/tmpfs

2011-05-10 Thread Takahiro Kambe
In message alpine.neb.2.00.1105100716550.29...@galant.ukfsn.org
on Tue, 10 May 2011 07:19:51 +0100 (BST),
Iain Hibbert plu...@rya-online.net wrote:
 On Tue, 10 May 2011, Takahiro Kambe wrote:
 
 In message 20110509170006.GA15831@marx.bitnet
  on Mon, 9 May 2011 20:00:06 +0300,
  Jukka Ruohonen jruoho...@iki.fi wrote:
  On Mon, May 09, 2011 at 06:50:08PM +0200, Adam Hoka wrote:
  So can we use for (int i = 0; ... ? :p
 lint(1) support them?
 
 Yes, lint(1) knows about it, see lint(7):
 
   325 variable declaration in for loop
Wow, thanks much!

-- 
Takahiro Kambe t...@back-street.net


Re: CVS commit: src/sys/fs/tmpfs

2011-05-09 Thread Christos Zoulas
On May 8,  9:21pm, m...@3am-software.com (Matt Thomas) wrote:
-- Subject: Re: CVS commit: src/sys/fs/tmpfs

| The kernel explicitly allows C99 and actually C99 is encouraged.
| So that should reverted :)

Test it. Build a DEBUG kernel and when it works, you can or I will revert it.
In this case it thinks that i is used out of the loop.

christos


Re: CVS commit: src/sys/fs/tmpfs

2011-05-09 Thread Jukka Ruohonen
On Mon, May 09, 2011 at 06:50:08PM +0200, Adam Hoka wrote:
 So can we use for (int i = 0; ... ? :p

Hopefully not...

- Jukka.


Re: CVS commit: src/sys/fs/tmpfs

2011-05-09 Thread Matt Thomas

On May 9, 2011, at 5:25 AM, Christos Zoulas wrote:

 On May 8,  9:21pm, m...@3am-software.com (Matt Thomas) wrote:
 -- Subject: Re: CVS commit: src/sys/fs/tmpfs
 
 | The kernel explicitly allows C99 and actually C99 is encouraged.
 | So that should reverted :)
 
 Test it. Build a DEBUG kernel and when it works, you can or I will revert it.

Been doing DEBUG builds with tmpfs for a long time with no problems.

 In this case it thinks that i is used out of the loop.

Where?  After the for loop, it returns.

Re: CVS commit: src/sys/fs/tmpfs

2011-05-09 Thread Christos Zoulas
On May 9, 10:23am, m...@3am-software.com (Matt Thomas) wrote:
-- Subject: Re: CVS commit: src/sys/fs/tmpfs

| 
| On May 9, 2011, at 5:25 AM, Christos Zoulas wrote:
| 
|  On May 8,  9:21pm, m...@3am-software.com (Matt Thomas) wrote:
|  -- Subject: Re: CVS commit: src/sys/fs/tmpfs
| =20
|  | The kernel explicitly allows C99 and actually C99 is encouraged.
|  | So that should reverted :)
| =20
|  Test it. Build a DEBUG kernel and when it works, you can or I will revert=
|  it.
| 
| Been doing DEBUG builds with tmpfs for a long time with no problems.
| 
|  In this case it thinks that i is used out of the loop.
| 
| Where?  After the for loop, it returns.

Well, just add back the commented out #CPPFLAGS+=-DDEBUG in
/usr/src/sys/rump/Makefile.rump and build again. My MKDEBUG
build causes that to happen too, and so it fails.

christos



Re: CVS commit: src/sys/fs/tmpfs

2011-05-09 Thread Takahiro Kambe
In message 20110509170006.GA15831@marx.bitnet
on Mon, 9 May 2011 20:00:06 +0300,
Jukka Ruohonen jruoho...@iki.fi wrote:
 On Mon, May 09, 2011 at 06:50:08PM +0200, Adam Hoka wrote:
 So can we use for (int i = 0; ... ? :p
lint(1) support them?

 Hopefully not...
Me, too.

-- 
Takahiro Kambe t...@back-street.net


Re: CVS commit: src/sys/fs/tmpfs

2011-05-09 Thread Matt Thomas

On May 9, 2011, at 4:35 PM, Christos Zoulas wrote:

 | Been doing DEBUG builds with tmpfs for a long time with no problems.
 | 
 |  In this case it thinks that i is used out of the loop.
 | 
 | Where?  After the for loop, it returns.
 
 Well, just add back the commented out #CPPFLAGS+=-DDEBUG in
 /usr/src/sys/rump/Makefile.rump and build again. My MKDEBUG
 build causes that to happen too, and so it fails.

Oh.  rump.  Rump incorrectly didn't use -std=gnu99.  That has now been fixed.


Re: CVS commit: src/sys/fs/tmpfs

2011-05-09 Thread Matt Thomas

On May 9, 2011, at 10:00 AM, Jukka Ruohonen wrote:

 On Mon, May 09, 2011 at 06:50:08PM +0200, Adam Hoka wrote:
 So can we use for (int i = 0; ... ? :p
 
 Hopefully not...

For the kernel, for (int ...  is allowed.




Re: CVS commit: src/sys/fs/tmpfs

2011-05-08 Thread Matt Thomas

On May 7, 2011, at 5:03 PM, Christos Zoulas wrote:

 Module Name:  src
 Committed By: christos
 Date: Sun May  8 00:03:35 UTC 2011
 
 Modified Files:
   src/sys/fs/tmpfs: tmpfs_vnops.c
 
 Log Message:
 no c99 please.

The kernel explicitly allows C99 and actually C99 is encouraged.
So that should reverted :)


Re: CVS commit: src/sys/fs/tmpfs

2009-11-11 Thread Mindaugas Rasiukevicius
Simon Burge sim...@netbsd.org wrote:
  
  Simplify tmpfs_itimes() and use vfs_timestamp().  [ ... ]
 
 Was changing from getnanotime() to effectively nanotime() (via
 vfs_timestamp()) deliberate?  The original intention of using
 getnanotime() for filesystem timestamps was that having a perfect
 timestamp was less important than the time taken to obtain the
 timestamp itself.

Yes, deliberate.  It is consistent with other *_itimes() routines.  I doubt
that such performance difference matters in this place.  However, if we want
to move to getnanotime(), then it should be changed in vfs_timestamp().

-- 
Mindaugas


Re: CVS commit: src/sys/fs/tmpfs

2009-11-11 Thread YAMAMOTO Takashi
hi,

 Mindaugas Rasiukevicius wrote:
 
 Module Name: src
 Committed By:rmind
 Date:Wed Nov 11 09:59:42 UTC 2009
 
 Modified Files:
 
  src/sys/fs/tmpfs: tmpfs_subr.c
 
 Log Message:
 
 Simplify tmpfs_itimes() and use vfs_timestamp().  [ ... ]
 
 Was changing from getnanotime() to effectively nanotime() (via
 vfs_timestamp()) deliberate?  The original intention of using
 getnanotime() for filesystem timestamps was that having a perfect
 timestamp was less important than the time taken to obtain the
 timestamp itself.
 
 Cheers,
 Simon.

perfect timestamp is important for nfs.

freebsd has a knob to switch getnanotime/nanotime for vfs_timestamp
for the tradeoff.

YAMAMOTO Takashi


Re: CVS commit: src/sys/fs/tmpfs

2009-04-28 Thread Antti Kantee
On Tue Apr 28 2009 at 00:29:05 +, YAMAMOTO Takashi wrote:
  Hmm, does this work correctly if you find the component via the
  cache_lookup() path?
  
  Ok, I dug into this a little.  Short answer: no, but ...
  
  It seems that cache_lookup() always returns false if MAKEENTRY is not set.
  However, it first does the lookup and removes the entry.  Does anyone
  know why it then returns false and forces a relookup?  Now in the
  case of tmpfs we always get 1 cache lookup and 2 full lookups for each
  remove/rename operation.
 
 because it's what ufs expects. :-)

Oh right, it wants the offset.

 i introduced cache_lookup_raw for nfs, which doesn't want these behaviours.
 i wanted to replace cache_lookup but some people prefered the current one.

Based on a quick look it seems like tmpfs could've switched to
cache_lookup_raw() when directory caching was removed.


Re: CVS commit: src/sys/fs/tmpfs

2009-04-27 Thread YAMAMOTO Takashi
hi,

 On Fri Apr 24 2009 at 18:00:45 +0300, Antti Kantee wrote:
 On Sat Apr 11 2009 at 20:42:59 +, Andrew Doran wrote:
  On Sat, Apr 11, 2009 at 12:21:57AM +, Perry E. Metzger wrote:
  
   Modified Files:
src/sys/fs/tmpfs: tmpfs_vnops.c
   
   Log Message:
   SAVENAME was not set for rename and delete as required
   
   Patch from christos, fixes pr 41183
  
  Now it leaks pathname buffers.
  
  Who reviewed this change and who okayed it?
 
 Hmm, does this work correctly if you find the component via the
 cache_lookup() path?
 
 Ok, I dug into this a little.  Short answer: no, but ...
 
 It seems that cache_lookup() always returns false if MAKEENTRY is not set.
 However, it first does the lookup and removes the entry.  Does anyone
 know why it then returns false and forces a relookup?  Now in the
 case of tmpfs we always get 1 cache lookup and 2 full lookups for each
 remove/rename operation.

because it's what ufs expects. :-)

i introduced cache_lookup_raw for nfs, which doesn't want these behaviours.
i wanted to replace cache_lookup but some people prefered the current one.

YAMAMOTO Takashi


Re: CVS commit: src/sys/fs/tmpfs

2009-04-24 Thread Antti Kantee
On Sat Apr 11 2009 at 20:42:59 +, Andrew Doran wrote:
 On Sat, Apr 11, 2009 at 12:21:57AM +, Perry E. Metzger wrote:
 
  Modified Files:
  src/sys/fs/tmpfs: tmpfs_vnops.c
  
  Log Message:
  SAVENAME was not set for rename and delete as required
  
  Patch from christos, fixes pr 41183
 
 Now it leaks pathname buffers.
 
 Who reviewed this change and who okayed it?

Hmm, does this work correctly if you find the component via the
cache_lookup() path?


Re: CVS commit: src/sys/fs/tmpfs

2009-04-15 Thread Antti Kantee
On Fri Apr 10 2009 at 22:36:38 +, Andrew Doran wrote:
 On Sat, Apr 11, 2009 at 01:32:09AM +0300, Antti Kantee wrote:
 
  On Fri Apr 10 2009 at 21:34:10 +, Andrew Doran wrote:
   On Fri, Apr 10, 2009 at 06:57:45PM +0200, Frank Kardel wrote:
   
It may be related: I am now seeing a tmpfs uvm_fault():

hand copied bt:
uvm_fault()
tmpfs_do_detach()
tmpfs_remove()
VOP_REMOVE()
do_sys_unlink()
syscall()
   
   It may be this: http://gnats.netbsd.org/41183
  
  Might be.  I filed a PR for that ages ago and had forgotten all about
  it by now.  See kern/38188.
 
 On the face of it what do you think of:
 
 - preserve pnbuf across entirety of operations that use it
 - retire SAVENAME, VOP_ABORTOP()
 - encapsulate operations with namei_init(), fini() or whatnot.

I thought the general agreement was to move to deferring final lookup
until the actual operation.

I don't see the difference between namei_fini() and ABORTOP, although
maybe the first is more explicit and less confusing for people not
familiar with vfs.

 - copy pathname to persistent storage where required (NFS?)

Or make pnbuf an object with a reference counter?  But is that optimizing
for the worst case?

The two questions I'm always too tired to answer to myself are:
1) how does this work with nfs(d)
2) how does this work with layered file systems
   (ok, i peeked at unionfs.  unionfs has a creative abortop...  *punt*)