Re: CVS commit: src/sys/fs/tmpfs
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
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
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
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.
Re: CVS commit: src/sys/fs/tmpfs
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
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
On Jan 8, 2014, at 5:11 PM, pedro martelletto 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
"J. Hannken-Illjes" wrote: > On Jan 8, 2014, at 5:11 PM, pedro martelletto 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
On Jan 3, 2014, at 10:18 PM, J. Hannken-Illjes wrote: > On Jan 3, 2014, at 10:13 PM, Mindaugas Rasiukevicius wrote: > >> "Juergen Hannken-Illjes" 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
On Jan 3, 2014, at 10:13 PM, Mindaugas Rasiukevicius wrote: > "Juergen Hannken-Illjes" 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
"Juergen Hannken-Illjes" 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
"J. Hannken-Illjes" 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
On Nov 8, 2013, at 4:44 PM, Mindaugas Rasiukevicius 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
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
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
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
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
14.05.2011, 10:38, "Masao Uebayashi" : > 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
Re: CVS commit: src/sys/fs/tmpfs
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
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
Masao Uebayashi 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
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
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
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 @@
*/
#include
-__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
#include
@@ -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
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
> -__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
> #include
> @@ -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
On Sat, May 14, 2011 at 2:37 AM, Mindaugas Rasiukevicius wrote: > Matt Thomas 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
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; i
Re: CVS commit: src/sys/fs/tmpfs
Matt Thomas 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. 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
In message on Tue, 10 May 2011 07:19:51 +0100 (BST), Iain Hibbert 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 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
Re: CVS commit: src/sys/fs/tmpfs
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 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
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
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
In message <20110509170006.GA15831@marx.bitnet> on Mon, 9 May 2011 20:00:06 +0300, 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 lint(1) support them? > Hopefully not... Me, too. -- Takahiro Kambe
Re: CVS commit: src/sys/fs/tmpfs
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
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
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
On Sun, 8 May 2011 21:21:56 -0700 Matt Thomas 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 :) So can we use "for (int i = 0; ..." ? :p -- NetBSD - Simplicity is prerequisite for reliability
Re: CVS commit: src/sys/fs/tmpfs
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
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
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
Simon Burge 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
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.
Re: CVS commit: src/sys/fs/tmpfs
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
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
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.
Re: CVS commit: src/sys/fs/tmpfs
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
hi, > On Wed Apr 15 2009 at 11:41:26 +, YAMAMOTO Takashi wrote: >> Module Name: src >> Committed By:yamt >> Date:Wed Apr 15 11:41:26 UTC 2009 >> >> Modified Files: >> src/sys/fs/tmpfs: tmpfs_vnops.c >> >> Log Message: >> plug some pnbuf leaks. > > Shouldn't that, theoretically, check for SAVESTART? sure. > (I don't think > there are any users in the tree, though, and I would prefer to see it > consistently go away from all post-lookup vnops). let's wait for dholland cleaning it up. :) YAMAMOTO Takashi > >> @@ -679,6 +679,7 @@ >> vrele(dvp); >> else >> vput(dvp); >> +PNBUF_PUT(cnp->cn_pnbuf); >> >> return error; >> } >> @@ -1061,6 +1062,7 @@ >> /* Release the nodes. */ >> vput(dvp); >> vput(vp); >> +PNBUF_PUT(cnp->cn_pnbuf); >> >> return error; >> } >>
Re: CVS commit: src/sys/fs/tmpfs
On Fri, Apr 10, 2009 at 10:36:38PM +, Andrew Doran wrote: > > 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. > - copy pathname to persistent storage where required (NFS?) Please don't do this until I have time to think about it relative to the cleanups I have planned (and the partially done stuff I have to merge) -- that will be a couple more days from now. (also, this should be on tech-kern) -- David A. Holland dholl...@netbsd.org
Re: CVS commit: src/sys/fs/tmpfs
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*)
Re: CVS commit: src/sys/fs/tmpfs
On Wed Apr 15 2009 at 11:41:26 +, YAMAMOTO Takashi wrote: > Module Name: src > Committed By: yamt > Date: Wed Apr 15 11:41:26 UTC 2009 > > Modified Files: > src/sys/fs/tmpfs: tmpfs_vnops.c > > Log Message: > plug some pnbuf leaks. Shouldn't that, theoretically, check for SAVESTART? (I don't think there are any users in the tree, though, and I would prefer to see it consistently go away from all post-lookup vnops). > @@ -679,6 +679,7 @@ > vrele(dvp); > else > vput(dvp); > + PNBUF_PUT(cnp->cn_pnbuf); > > return error; > } > @@ -1061,6 +1062,7 @@ > /* Release the nodes. */ > vput(dvp); > vput(vp); > + PNBUF_PUT(cnp->cn_pnbuf); > > return error; > } >
Re: CVS commit: src/sys/fs/tmpfs
Andrew Doran writes: > 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? The patch came from christos. He told me to commit it. I would have thought that was obvious from the commit message. Until it was put into place, builds that I did reliably paniced my machine in tmpfs. If the fix is broken, well, please fix the fix or talk to Christos about it. As a side note: my fastest box, a Q9550 (quad core 2.8GHz 12M cache) with 4G of memory and maxvnodes bumped up so that it does virtually no i/o quite reliably dies doing ./build.sh in various places, some of them rather bizarre and hard to debug. A couple of them so far have corresponded to known bugs like the tmpfs issue. Extensive testing, including running other OSes, seems to indicate that the hardware is not at fault. It would be rather nice to get the associated races etc. fixed so the machine would operate reliably. Perry -- Perry E. Metzgerpe...@piermont.com
Re: CVS commit: src/sys/fs/tmpfs
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?
Re: CVS commit: src/sys/fs/tmpfs
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. - copy pathname to persistent storage where required (NFS?)
Re: CVS commit: src/sys/fs/tmpfs
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.
Re: CVS commit: src/sys/fs/tmpfs
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
Re: CVS commit: src/sys/fs/tmpfs
Hello Antti ! 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() Frank Antti Kantee wrote: Module Name:src Committed By: pooka Date: Fri Apr 3 14:47:41 UTC 2009 Modified Files: src/sys/fs/tmpfs: tmpfs_vnops.c Log Message: Fix yet another recent crashy bug in tmpfs rename: since the source dirent is no longer cached in lookup and we do the lookup ourselves in rename, we are most definitely not allowed to assert that it matches the source vnode passed as an argument. In case the source node does not exist or has been replaced, punt with ENOENT. Also, nuke some misleading prehistoric comments which haven't been valid in over a year. Fixes PR kern/41128 by Nicolas Joly To generate a diff of this commit: cvs rdiff -u -r1.54 -r1.55 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.
