Re: Pull tmpfs fix from NetBSD

[Marc Espie]

On Fri, Dec 11, 2015 at 10:05:14AM -0700, Bob Beck wrote:
> 
> 
> 
> On Fri, Dec 11, 2015 at 01:09:30AM -0500, Michael McConville wrote:
> > Here's the PR:
> > 
> > https://gnats.netbsd.org/50381
> > 
> > And the commit:
> > 
> > https://marc.info/?l=netbsd-source-changes=144694603617544=2
> > 
> > We have very few local changes to tmpfs and we share the
> > KASSERT(de->td_node == NULL), so I think this applies to us.
> > 
> > Thoughts? ok?
> > 
> > 
> > Index: sys/tmpfs/tmpfs_subr.c
> > ===
> > RCS file: /cvsroot/src/sys/fs/tmpfs/tmpfs_subr.c,v
> > retrieving revision 1.96.4.1
> > retrieving revision 1.96.4.1.2.1
> > diff -u -p -r1.96.4.1 -r1.96.4.1.2.1
> > --- sys/fs/tmpfs/tmpfs_subr.c   22 Dec 2014 02:05:08 -  1.96.4.1
> > +++ sys/fs/tmpfs/tmpfs_subr.c   8 Nov 2015 01:27:10 -   
> > 1.96.4.1.2.1
> > @@ -451,6 +451,7 @@ tmpfs_alloc_dirent(tmpfs_mount_t *tmp, c
> > nde->td_namelen = len;
> > memcpy(nde->td_name, name, len);
> > nde->td_seq = TMPFS_DIRSEQ_NONE;
> > +   nde->td_node = NULL; /* for asserts */
> >  
> > *de = nde;
> > return 0;
> > 
> 
> Well, that diff won't apply directly, and in my opinion this is a safter way, 
> considering
> the tmpfs code, to accomplish the same thing.
> 
> Index: tmpfs/tmpfs_mem.c
> ===
> RCS file: /cvs/src/sys/tmpfs/tmpfs_mem.c,v
> retrieving revision 1.7
> diff -u -p -u -p -r1.7 tmpfs_mem.c
> --- tmpfs/tmpfs_mem.c 14 Mar 2015 03:38:52 -  1.7
> +++ tmpfs/tmpfs_mem.c 11 Dec 2015 17:01:06 -
> @@ -151,7 +151,7 @@ tmpfs_dirent_get(struct tmpfs_mount *mp)
>   if (!tmpfs_mem_incr(mp, sizeof(struct tmpfs_dirent))) {
>   return NULL;
>   }
> - return pool_get(_dirent_pool, PR_WAITOK);
> + return pool_get(_dirent_pool, PR_ZERO|PR_WAITOK);
>  }
Looks good to me. Don't see any point in any comment or extra code.

Re: Pull tmpfs fix from NetBSD

[Bob Beck]

On Fri, Dec 11, 2015 at 01:09:30AM -0500, Michael McConville wrote:
> Here's the PR:
> 
> https://gnats.netbsd.org/50381
> 
> And the commit:
> 
> https://marc.info/?l=netbsd-source-changes=144694603617544=2
> 
> We have very few local changes to tmpfs and we share the
> KASSERT(de->td_node == NULL), so I think this applies to us.
> 
> Thoughts? ok?
> 
> 
> Index: sys/tmpfs/tmpfs_subr.c
> ===
> RCS file: /cvsroot/src/sys/fs/tmpfs/tmpfs_subr.c,v
> retrieving revision 1.96.4.1
> retrieving revision 1.96.4.1.2.1
> diff -u -p -r1.96.4.1 -r1.96.4.1.2.1
> --- sys/fs/tmpfs/tmpfs_subr.c 22 Dec 2014 02:05:08 -  1.96.4.1
> +++ sys/fs/tmpfs/tmpfs_subr.c 8 Nov 2015 01:27:10 -   1.96.4.1.2.1
> @@ -451,6 +451,7 @@ tmpfs_alloc_dirent(tmpfs_mount_t *tmp, c
>   nde->td_namelen = len;
>   memcpy(nde->td_name, name, len);
>   nde->td_seq = TMPFS_DIRSEQ_NONE;
> + nde->td_node = NULL; /* for asserts */
>  
>   *de = nde;
>   return 0;
> 

Well, that diff won't apply directly, and in my opinion this is a safter way, 
considering
the tmpfs code, to accomplish the same thing.

Index: tmpfs/tmpfs_mem.c
===
RCS file: /cvs/src/sys/tmpfs/tmpfs_mem.c,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 tmpfs_mem.c
--- tmpfs/tmpfs_mem.c   14 Mar 2015 03:38:52 -  1.7
+++ tmpfs/tmpfs_mem.c   11 Dec 2015 17:01:06 -
@@ -151,7 +151,7 @@ tmpfs_dirent_get(struct tmpfs_mount *mp)
if (!tmpfs_mem_incr(mp, sizeof(struct tmpfs_dirent))) {
return NULL;
}
-   return pool_get(_dirent_pool, PR_WAITOK);
+   return pool_get(_dirent_pool, PR_ZERO|PR_WAITOK);
 }
 
 void

Re: Pull tmpfs fix from NetBSD

[Michael McConville]

Bob Beck wrote:
> Stability before performance.  Tmpfs does not have the former yet.

ok mmcc@ for your PR_ZERO diff, as long as there's a comment added about
the performance impact and the potential to back out in the future.

I think it'd still be worthwhile to add the NULL assignment from NetBSD.
It'd keep us synced and prevent us from relying on everything being
zeroed.

Re: Pull tmpfs fix from NetBSD

[Stefan Sperling]

On Fri, Dec 11, 2015 at 04:47:16PM -0500, Michael McConville wrote:
> Stefan Sperling wrote:
> > On Fri, Dec 11, 2015 at 04:05:49PM -0500, Michael McConville wrote:
> > > Bob Beck wrote:
> > > > Stability before performance.  Tmpfs does not have the former yet.
> > > 
> > > ok mmcc@ for your PR_ZERO diff, as long as there's a comment added
> > > about the performance impact and the potential to back out in the
> > > future.
> > 
> > I don't see the point of such a comment.
> > Would you want such a comment everywhere PR_ZERO is used? What about
> > calloc? Every committed change has the potential to be backed out for
> > some reason.
> 
> This just seems like a potential long-term, irreversible divergence.
> Specifically, people may start making changes that rely on it or forego
> NetBSD bugfixes related to it. IIUC, we pull tmpfs pretty directly from
> NetBSD (where it originated), so I was trying to avoid that. Upon
> further reflection, I don't know whether these sorts of changes have
> already happened, or to what extent people care.

I don't think anyone can rely on comments you're proposing long term.
IMO, just fix the code, and move on. Leave it to those who come after
you to read the code you left behind properly. Yes, that includes
reading code across function calls and verifying interdependencies.

Re: Pull tmpfs fix from NetBSD

[Theo de Raadt]

> That said, using M_ZERO does sound like a safety improvement. However,
> that also looks like a big struct (in the process of getting an actual
> number). Thoughts on the performance impact?

No performance impact at all.

until the next time an uninitialized field occurs in there!

then maybe kaboom.  As in, crash kernel.  Is a crash a performance impact?
I think so..

Re: Pull tmpfs fix from NetBSD

[Michael McConville]

Stefan Sperling wrote:
> On Fri, Dec 11, 2015 at 04:05:49PM -0500, Michael McConville wrote:
> > Bob Beck wrote:
> > > Stability before performance.  Tmpfs does not have the former yet.
> > 
> > ok mmcc@ for your PR_ZERO diff, as long as there's a comment added
> > about the performance impact and the potential to back out in the
> > future.
> 
> I don't see the point of such a comment.
> Would you want such a comment everywhere PR_ZERO is used? What about
> calloc? Every committed change has the potential to be backed out for
> some reason.

This just seems like a potential long-term, irreversible divergence.
Specifically, people may start making changes that rely on it or forego
NetBSD bugfixes related to it. IIUC, we pull tmpfs pretty directly from
NetBSD (where it originated), so I was trying to avoid that. Upon
further reflection, I don't know whether these sorts of changes have
already happened, or to what extent people care.

Re: Pull tmpfs fix from NetBSD

[Stefan Sperling]

On Fri, Dec 11, 2015 at 04:05:49PM -0500, Michael McConville wrote:
> Bob Beck wrote:
> > Stability before performance.  Tmpfs does not have the former yet.
> 
> ok mmcc@ for your PR_ZERO diff, as long as there's a comment added about
> the performance impact and the potential to back out in the future.

I don't see the point of such a comment.
Would you want such a comment everywhere PR_ZERO is used? What about calloc?
Every committed change has the potential to be backed out for some reason.

Re: Pull tmpfs fix from NetBSD

[Bob Beck]

tmpfs actrually already must diverege from netbsd. we can not just
blithly accept changes from there.. our kernel midlayers are very
different.

tmpfs actually does not work very well right now.



On Fri, Dec 11, 2015 at 2:47 PM, Michael McConville  wrote:
> Stefan Sperling wrote:
>> On Fri, Dec 11, 2015 at 04:05:49PM -0500, Michael McConville wrote:
>> > Bob Beck wrote:
>> > > Stability before performance.  Tmpfs does not have the former yet.
>> >
>> > ok mmcc@ for your PR_ZERO diff, as long as there's a comment added
>> > about the performance impact and the potential to back out in the
>> > future.
>>
>> I don't see the point of such a comment.
>> Would you want such a comment everywhere PR_ZERO is used? What about
>> calloc? Every committed change has the potential to be backed out for
>> some reason.
>
> This just seems like a potential long-term, irreversible divergence.
> Specifically, people may start making changes that rely on it or forego
> NetBSD bugfixes related to it. IIUC, we pull tmpfs pretty directly from
> NetBSD (where it originated), so I was trying to avoid that. Upon
> further reflection, I don't know whether these sorts of changes have
> already happened, or to what extent people care.

Re: Pull tmpfs fix from NetBSD

[Michael McConville]

Bob Beck wrote:
> On Fri, Dec 11, 2015 at 01:09:30AM -0500, Michael McConville wrote:
> > Here's the PR:
> > 
> > https://gnats.netbsd.org/50381
> > 
> > And the commit:
> > 
> > https://marc.info/?l=netbsd-source-changes=144694603617544=2
> > 
> > We have very few local changes to tmpfs and we share the
> > KASSERT(de->td_node == NULL), so I think this applies to us.
> > 
> > Thoughts? ok?
> > 
> > 
> > Index: sys/tmpfs/tmpfs_subr.c
> > ===
> > RCS file: /cvsroot/src/sys/fs/tmpfs/tmpfs_subr.c,v
> > retrieving revision 1.96.4.1
> > retrieving revision 1.96.4.1.2.1
> > diff -u -p -r1.96.4.1 -r1.96.4.1.2.1
> > --- sys/fs/tmpfs/tmpfs_subr.c   22 Dec 2014 02:05:08 -  1.96.4.1
> > +++ sys/fs/tmpfs/tmpfs_subr.c   8 Nov 2015 01:27:10 -   
> > 1.96.4.1.2.1
> > @@ -451,6 +451,7 @@ tmpfs_alloc_dirent(tmpfs_mount_t *tmp, c
> > nde->td_namelen = len;
> > memcpy(nde->td_name, name, len);
> > nde->td_seq = TMPFS_DIRSEQ_NONE;
> > +   nde->td_node = NULL; /* for asserts */
> >  
> > *de = nde;
> > return 0;
> > 
> 
> Well, that diff won't apply directly, and in my opinion this is a
> safter way, considering the tmpfs code, to accomplish the same thing.

It applies directly, unless I'm missing something. I just forgot to
share the OpenBSD diff, so the file path is different (drop the fs dir).

That said, using M_ZERO does sound like a safety improvement. However,
that also looks like a big struct (in the process of getting an actual
number). Thoughts on the performance impact?

Re: Pull tmpfs fix from NetBSD

[Bob Beck]

Stability before performance.  Tmpfs does not have the former yet.


On Fri, Dec 11, 2015 at 12:59 PM, Michael McConville  wrote:
> Bob Beck wrote:
>> On Fri, Dec 11, 2015 at 01:09:30AM -0500, Michael McConville wrote:
>> > Here's the PR:
>> >
>> > https://gnats.netbsd.org/50381
>> >
>> > And the commit:
>> >
>> > https://marc.info/?l=netbsd-source-changes=144694603617544=2
>> >
>> > We have very few local changes to tmpfs and we share the
>> > KASSERT(de->td_node == NULL), so I think this applies to us.
>> >
>> > Thoughts? ok?
>> >
>> >
>> > Index: sys/tmpfs/tmpfs_subr.c
>> > ===
>> > RCS file: /cvsroot/src/sys/fs/tmpfs/tmpfs_subr.c,v
>> > retrieving revision 1.96.4.1
>> > retrieving revision 1.96.4.1.2.1
>> > diff -u -p -r1.96.4.1 -r1.96.4.1.2.1
>> > --- sys/fs/tmpfs/tmpfs_subr.c   22 Dec 2014 02:05:08 -  
>> > 1.96.4.1
>> > +++ sys/fs/tmpfs/tmpfs_subr.c   8 Nov 2015 01:27:10 -   
>> > 1.96.4.1.2.1
>> > @@ -451,6 +451,7 @@ tmpfs_alloc_dirent(tmpfs_mount_t *tmp, c
>> > nde->td_namelen = len;
>> > memcpy(nde->td_name, name, len);
>> > nde->td_seq = TMPFS_DIRSEQ_NONE;
>> > +   nde->td_node = NULL; /* for asserts */
>> >
>> > *de = nde;
>> > return 0;
>> >
>>
>> Well, that diff won't apply directly, and in my opinion this is a
>> safter way, considering the tmpfs code, to accomplish the same thing.
>
> It applies directly, unless I'm missing something. I just forgot to
> share the OpenBSD diff, so the file path is different (drop the fs dir).
>
> That said, using M_ZERO does sound like a safety improvement. However,
> that also looks like a big struct (in the process of getting an actual
> number). Thoughts on the performance impact?
>