Re: [RFC PATCH 1/8] share/private/slave a subtree

[Ram]

On Fri, 2005-07-08 at 12:49, Miklos Szeredi wrote:
> > The reason why I implemented that way, is to less confuse the user and
> > provide more flexibility. With my implementation, we have the ability
> > to share any part of the tree, without bothering if it is a mountpoint
> > or not. The side effect of this operation is, it ends up creating 
> > a vfsmount if the dentry is not a mountpoint.
> > 
> > so when a user says
> >   mount --make-shared /tmp/abc
> > the tree under /tmp/abc becomes shared. 
> > With your suggestion either the user will get -EINVAL or the tree
> > under / will become shared. The second behavior will be really
> > confusing.
> 
> You are right.
> 
> > I am ok with -EINVAL. 
> 
> I think it should be this then.  These operations are similar to a
> remount (they don't actually mount something, just change some
> property of a mount).  Remount returns -EINVAL if not performed on the
> root of a mount.
> 
> > Also there is another reason why I used this behavior. Lets say /mnt
> > is a mountpoint and Say a user does
> > mount make-shared /mnt 
> > 
> > and then does 
> > mount --bind /mnt/abc  /mnt1
> > 
> > NOTE: we need propogation to be set up between /mnt/abc and /mnt1 and
> > propogation can only be set up for vfsmounts.  In this case /mnt/abc 
> > is not a mountpoint. I have two choices, either return -EINVAL
> > or create a vfsmount at that point. But -EINVAL is not consistent
> > with standard --bind behavior. So I chose the later behavior.
> > 
> > Now that we anyway need this behavior while doing bind mounts from
> > shared trees, I kept the same behavior for --make-shared.
> 
> Well, the mount program can easily implement this behavior if wanted,
> just by doing the 'bind dir dir' and then doing 'make-shared dir'.
> 
> The other way round (disabling the automatic 'bind dir dir') is much
> more difficult.

Ok. will make it -EINVAL. It was not clear in Al Viro's RFC what the
behavior should be.

> 
> > > Some notes (maybe outside the code) explaining the mechanism of the
> > > propagations would be nice.  Without these it's hard to understand the
> > > design decisions behind such an implementation.
> > 
> > Ok. I will make a small writeup on the mechanism.
> 
> That will help, thanks.

A small writeup is enclosed. Caution its too complex. :) 

RP

Sorry if reading it as a attachment is difficult. my mailer does
not allow me to inline properly. will try mutt next time.
Pnode traversal implementation.


This write-up explains the motivation and reason behind the implementation
of pnode_next() and pnode_traverse() functionality.

Section 1 explains the operations  involved during a mount in a shared subtree.
Section 2 explains the operations  involved during a umount in a shared subtree.
Section 3 explains the operations  involved to check of umount_busy in
shared subtree.
Section 4 explains the operations  involved in making a overlay mount in
a shared subtree. (make_mounted operation)
Section 5 explains the operations  involved in removing a overlay mount in
a shared subtree. (make_umounted operation)

And finally section 6 explains the motivation behind the pnode_next()
and pnode_traverse().

Caution: head can spin as you try to understand the detail. :)


Section 1. mount:

to begin with we have a the following mount tree 

 root
  / /  \  \ \
 /  t1  t2 \  \ 
   t0   t3 \
t4

note: 
t0, t1, t2, t3, t4 all contain mounts.
t1 t2 t3 are the slave of t0. 
t4 is the slave of t2.
t4 and t3 is marked as shared.

The corresponding propagation tree will be:

p0
  /   \
 p1   p2
 / 
 p3


***
  p0 contains the mount t0, and contains the slave mount t1
  p1 contains the mount t2
  p3 contains the mount t4
  p2 contains the mount t3

  NOTE: you may need to look at this multiple time as you try to
understand the various scenarios.
***


now if we mount something under the mount t0, the same has
be mounted under all the other mounts (t1,t2,t3,and t4) and
the new propagation tree for all these child mounts should
look identical to the one of their parents.

say I mounted something under /t0/abc the new mount tree will
look like:

 root
  / /  \  \ \
 /  t1  t2 \  \ 
   t0   /   /   t3 \
   /   c1  c2   /   t4
  c0   c3\
   

Re: share/private/slave a subtree - define vs enum

[Roman Zippel]

Hi,

On Mon, 11 Jul 2005, Horst von Brand wrote:

> > I don't generally disagree with that, I just think that defines are not 
> > part of that list.
> 
> Covered in "bad coding style" and "hard to read code", at least.

Somehow I missed the last lkml debate about where simple defines where a 
problem.

> > Look, it's great that you do reviews, but please keep in mind it's the 
> > author who has to work with code and he has to be primarily happy with, 
> > so you don't have to point out every minor issue.
> 
> Wrong. The author has to work with the code, but there are much more people
> that have to read it now and fix it in the future. It doesn't make sense
> having everybody using their own indentation style, variable naming scheme,
> and ways of defining constants.

I didn't say this, I said "minor issues". Please read more carefully.

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Horst von Brand]

Roman Zippel <[EMAIL PROTECTED]> wrote:
> On Sun, 10 Jul 2005, Pekka Enberg wrote:

[...]

> > Would you please be so kind to define your criteria for things that
> > "need fixing" so we could see if can reach some sort of an agreement on
> > this. My list is roughly as follows:
> > 
> >   - Erroneous use of kernel API
> >   - Bad coding style
> >   - Layering violations
> >   - Duplicate code
> >   - Hard to read code

> I don't generally disagree with that, I just think that defines are not 
> part of that list.

Covered in "bad coding style" and "hard to read code", at least.

> Look, it's great that you do reviews, but please keep in mind it's the 
> author who has to work with code and he has to be primarily happy with, 
> so you don't have to point out every minor issue.

Wrong. The author has to work with the code, but there are much more people
that have to read it now and fix it in the future. It doesn't make sense
having everybody using their own indentation style, variable naming scheme,
and ways of defining constants. For better or worse, #define /is/ the
standard idiom (in kernel and elsewhere) to define constants in C. See also
, particularly
comandment 8. And consider also the /spirit/ of Documentation/CodingStyle.

> Although it also differs between core and driver code, we don't have to be 
> that strict with driver code as longs as it "looks" ok and is otherwise 
> correct. The requirements for core kernel code are higher, but even here 
> defines are a well accepted language construct.

I'd rather ask for the higher requirements of authors of new code... not
demand, just ask.
-- 
Dr. Horst H. von Brand   User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria  +56 32 654239
Casilla 110-V, Valparaiso, ChileFax:  +56 32 797513
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Horst von Brand]

Vojtech Pavlik <[EMAIL PROTECTED]> wrote:
> On Sun, Jul 10, 2005 at 09:21:42PM +0300, Pekka Enberg wrote:
> > Hmm. So we disagree on that issue as well. I think the point of review
> > is to improve code and help others conform with the existing coding
> > style which is why I find it strange that you're suggesting me to limit
> > my comments to a subset you agree with.
> > 
> > Would you please be so kind to define your criteria for things that
> > "need fixing" so we could see if can reach some sort of an agreement on
> > this. My list is roughly as follows:
> > 
> >   - Erroneous use of kernel API
> >   - Bad coding style
> >   - Layering violations
> >   - Duplicate code
> >   - Hard to read code

> The reason people post their patches for review is to get good feedback
> on them. The problems you list above are mostly nitpicks. They must be
> fixed before inclusion of the patch, but only make sense to start fixing
> once the patch does a reasonable change.

If the coding style is an obstacle to understanding, it has to be fixed if
there is going to be any kind of review. Besides, nitpicks being simple to
address, they could as well be fixed while at it. Perhaps that way the
author learns to do it right, and less nitpicks are left in later additions
and fixes.
-- 
Dr. Horst H. von Brand   User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria  +56 32 654239
Casilla 110-V, Valparaiso, ChileFax:  +56 32 797513
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Pekka J Enberg]

Hi Roman, 


Roman Zippel writes:
I don't generally disagree with that, I just think that defines are not 
part of that list.


They're in Documentation/CodingStyle (see Chapter 11). 


Roman Zippel writes:
Look, it's great that you do reviews, but please keep in mind it's the 
author who has to work with code and he has to be primarily happy with, 
so you don't have to point out every minor issue.


I completely agree with you that the author must be happy with the code. I 
also think Vojtech has a good point about reviewing for the most important 
things first and worrying about nitpicks later. 

I'll try to find a better balance for reviews. Thanks Roman. 

 Pekka 


-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Pavel Machek]

Hi!

> >>enums in C are (de?)promoted to integral types under most conditions, so 
> >>the type-checking is useless.
> >
> >
> >It's a warning in gcc afaik and spare should complain as well.
> 
> Check again.

Check sparse with -Wbitwise and enum properly marked as bitwise...

Pavel
-- 
teflon -- maybe it is a trademark, but it should not be.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Vojtech Pavlik]

On Sun, Jul 10, 2005 at 09:21:42PM +0300, Pekka Enberg wrote:

> Hmm. So we disagree on that issue as well. I think the point of review
> is to improve code and help others conform with the existing coding
> style which is why I find it strange that you're suggesting me to limit
> my comments to a subset you agree with.
> 
> Would you please be so kind to define your criteria for things that
> "need fixing" so we could see if can reach some sort of an agreement on
> this. My list is roughly as follows:
> 
>   - Erroneous use of kernel API
>   - Bad coding style
>   - Layering violations
>   - Duplicate code
>   - Hard to read code
 
The reason people post their patches for review is to get good feedback
on them. The problems you list above are mostly nitpicks. They must be
fixed before inclusion of the patch, but only make sense to start fixing
once the patch does a reasonable change.

Often patches have deeper problems (like "this won't ever work", "there
is a nice race hidden in there", "why do we need this part at all"), and
spotting those is much more valuable for both the sumbitter and the
progress of development.

Obviously, it's much harder to do that than to comment on a misplaced
brace.

It's an utter waste of effort to force a first time patch author to fix
all the style issues in his patch, just to see it rejected by the
maintainer because it is fundamentally wrong later.

Just something to consider.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Roman Zippel]

Hi,

On Sun, 10 Jul 2005, Pekka Enberg wrote:

> > The point of a review is to comment on things that _need_ fixing. Less 
> > experienced hackers take this a requirement for their drivers to be 
> > included.
> 
> Hmm. So we disagree on that issue as well. I think the point of review
> is to improve code and help others conform with the existing coding
> style which is why I find it strange that you're suggesting me to limit
> my comments to a subset you agree with.
> 
> Would you please be so kind to define your criteria for things that
> "need fixing" so we could see if can reach some sort of an agreement on
> this. My list is roughly as follows:
> 
>   - Erroneous use of kernel API
>   - Bad coding style
>   - Layering violations
>   - Duplicate code
>   - Hard to read code

I don't generally disagree with that, I just think that defines are not 
part of that list.
Look, it's great that you do reviews, but please keep in mind it's the 
author who has to work with code and he has to be primarily happy with, 
so you don't have to point out every minor issue.
Although it also differs between core and driver code, we don't have to be 
that strict with driver code as longs as it "looks" ok and is otherwise 
correct. The requirements for core kernel code are higher, but even here 
defines are a well accepted language construct.

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[randy_dunlap]

On Sun, 10 Jul 2005 21:21:42 +0300 Pekka Enberg wrote:

| Hi Roman,
| 
| At some point in time, I wrote:
| > > Roman, it is not as if I get to decide for the patch submitters. I
| > > comment on any issues _I_ have with the patch and the authors fix
| > > whatever they want (or what the maintainers ask for).
| 
| On Fri, 2005-07-08 at 21:59 +0200, Roman Zippel wrote:
| > The point of a review is to comment on things that _need_ fixing. Less 
| > experienced hackers take this a requirement for their drivers to be 
| > included.
| 
| Hmm. So we disagree on that issue as well. I think the point of review
| is to improve code and help others conform with the existing coding
| style which is why I find it strange that you're suggesting me to limit
| my comments to a subset you agree with.
| 
| Would you please be so kind to define your criteria for things that
| "need fixing" so we could see if can reach some sort of an agreement on
| this. My list is roughly as follows:
| 
|   - Erroneous use of kernel API
|   - Bad coding style
|   - Layering violations
|   - Duplicate code
|   - Hard to read code

Those are all good points IMO, but there is little (or no)
concensus on "bad coding style" or "hard to read code".

Maybe it would make more sense to make some suggested changes to
Documentation/CodingStyle (in the form of a patch) and see what
kinds of reactions (support) it gets.

---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Pekka Enberg]

Hi Roman,

At some point in time, I wrote:
> > Roman, it is not as if I get to decide for the patch submitters. I
> > comment on any issues _I_ have with the patch and the authors fix
> > whatever they want (or what the maintainers ask for).

On Fri, 2005-07-08 at 21:59 +0200, Roman Zippel wrote:
> The point of a review is to comment on things that _need_ fixing. Less 
> experienced hackers take this a requirement for their drivers to be 
> included.

Hmm. So we disagree on that issue as well. I think the point of review
is to improve code and help others conform with the existing coding
style which is why I find it strange that you're suggesting me to limit
my comments to a subset you agree with.

Would you please be so kind to define your criteria for things that
"need fixing" so we could see if can reach some sort of an agreement on
this. My list is roughly as follows:

  - Erroneous use of kernel API
  - Bad coding style
  - Layering violations
  - Duplicate code
  - Hard to read code

Pekka

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Denis Vlasenko]

On Friday 08 July 2005 19:57, Roman Zippel wrote:
> Hi,
> 
> On Fri, 8 Jul 2005, Bryan Henderson wrote:
> 
> > I wasn't aware anyone preferred defines to enums for declaring enumerated 
> > data types.
> 
> If it's really enumerated data types, that's fine, but this example was 
> about bitfield masks.
> 
> > Isn't the only argument for defines, "that's what I'm used to."?
> 
> defines are not just used for constants and there is _nothing_ wrong with 
> using defines for constants.

Apart from the wart that defines know zilch about scopes.
Thus completely fine looking code will give you atrociously
obscure compiler message. Real world example for userspace:

# cat t.c
#include 
#include 

void f() {
char errno[] = "hello world";
puts(errno);
}

# gcc -c t.c
t.c: In function `f':
t.c:5: error: function `__errno_location' is initialized like a variable
t.c:5: error: conflicting types for '__errno_location'
/usr/include/bits/errno.h:38: error: previous declaration of '__errno_location' 
was here
--
vda

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Pekka Enberg]

Hi,

On Fri, 2005-07-08 at 21:11 +0200, Roman Zippel wrote:
> So it basically comes down to personal preference, if the original uses 
> defines and it works fine, I don't really see a good enough reason to 
> change it to enums, so please leave the decision to author.

(And I don't see a good enough reason to use #defines when you don't
 absolutely have to. This is what we disagree on.)

Roman, it is not as if I get to decide for the patch submitters. I
comment on any issues _I_ have with the patch and the authors fix
whatever they want (or what the maintainers ask for).

As I disagree with the part about enums being a personal preference, I
will continue to comment on them in the future. If patch authors wish to
ignore them (or any of my comments for that matter), that's ok with me.

P.S. Working code is not enough for the kernel. It must be maintainable
as well.

Pekka

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Bryan Henderson]

>I don't see how the following is tortured: 
>
>enum {
>   PNODE_MEMBER_VFS  = 0x01,
>   PNODE_SLAVE_VFS   = 0x02
>}; 

Only because it's using a facility that's supposed to be for enumerated 
types for something that isn't.  If it were a true enumerated type, the 
codes for the enumerations (0x01, 0x02) would be quite arbitrary, whereas 
here they must fundamentally be integers whose pure binary cipher has 
exactly one 1 bit (because, as I understand it, these are used as bitmasks 
somewhere).

I can see that this paradigm has practical advantages over using macros 
(or a middle ground - integer constants), but only as a byproduct of what 
the construct is really for.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Mike Waychison]

Wichert Akkerman wrote:

Previously Mike Waychison wrote:

enums in C are (de?)promoted to integral types under most conditions, so 
the type-checking is useless.



It's a warning in gcc afaik and spare should complain as well.



Check again.

You must be thinking of another language.

Show me how you can get warnings out of this:

enum foo { FOO, };


static enum foo doit(enum foo bar) {
int i = bar;
return i;
}


int main(void)
{
int i;
i = doit(0);
return 0;
}

Mike Waychison
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Ram]

On Fri, 2005-07-08 at 12:11, Roman Zippel wrote:
> Hi,
> 
> On Fri, 8 Jul 2005, Pekka J Enberg wrote:
> 
> > I don't see how the following is tortured: 
> > enum {
> >   PNODE_MEMBER_VFS  = 0x01,
> >   PNODE_SLAVE_VFS   = 0x02
> > }; 
> > In fact, I think it is more natural. An almost identical example even 
> > appears
> > in K&R. 
> 
> So it basically comes down to personal preference, if the original uses 
> defines and it works fine, I don't really see a good enough reason to 
> change it to enums, so please leave the decision to author.

Ok. I will change to enums whereever I define new categories of
#defines. And leave the #defines for already existing category as is.

RP


-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Roman Zippel]

Hi,

On Fri, 8 Jul 2005, Pekka Enberg wrote:

> On Fri, 2005-07-08 at 21:11 +0200, Roman Zippel wrote:
> > So it basically comes down to personal preference, if the original uses 
> > defines and it works fine, I don't really see a good enough reason to 
> > change it to enums, so please leave the decision to author.
> 
> (And I don't see a good enough reason to use #defines when you don't
>  absolutely have to. This is what we disagree on.)

"use" != "change".
If an author already uses defines, that's fine and in most cases there is 
no reason to change it.

> Roman, it is not as if I get to decide for the patch submitters. I
> comment on any issues _I_ have with the patch and the authors fix
> whatever they want (or what the maintainers ask for).

The point of a review is to comment on things that _need_ fixing. Less 
experienced hackers take this a requirement for their drivers to be 
included.

> P.S. Working code is not enough for the kernel. It must be maintainable
> as well.

defines are perfectly maintainable.

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/8] share/private/slave a subtree

[Miklos Szeredi]

> The reason why I implemented that way, is to less confuse the user and
> provide more flexibility. With my implementation, we have the ability
> to share any part of the tree, without bothering if it is a mountpoint
> or not. The side effect of this operation is, it ends up creating 
> a vfsmount if the dentry is not a mountpoint.
> 
> so when a user says
>   mount --make-shared /tmp/abc
> the tree under /tmp/abc becomes shared. 
> With your suggestion either the user will get -EINVAL or the tree
> under / will become shared. The second behavior will be really
> confusing.

You are right.

> I am ok with -EINVAL. 

I think it should be this then.  These operations are similar to a
remount (they don't actually mount something, just change some
property of a mount).  Remount returns -EINVAL if not performed on the
root of a mount.

> Also there is another reason why I used this behavior. Lets say /mnt
> is a mountpoint and Say a user does
>   mount make-shared /mnt 
> 
> and then does 
> mount --bind /mnt/abc  /mnt1
> 
> NOTE: we need propogation to be set up between /mnt/abc and /mnt1 and
> propogation can only be set up for vfsmounts.  In this case /mnt/abc 
> is not a mountpoint. I have two choices, either return -EINVAL
> or create a vfsmount at that point. But -EINVAL is not consistent
> with standard --bind behavior. So I chose the later behavior.
> 
> Now that we anyway need this behavior while doing bind mounts from
> shared trees, I kept the same behavior for --make-shared.

Well, the mount program can easily implement this behavior if wanted,
just by doing the 'bind dir dir' and then doing 'make-shared dir'.

The other way round (disabling the automatic 'bind dir dir') is much
more difficult.

> > Some notes (maybe outside the code) explaining the mechanism of the
> > propagations would be nice.  Without these it's hard to understand the
> > design decisions behind such an implementation.
> 
> Ok. I will make a small writeup on the mechanism.

That will help, thanks.

Miklos



-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Roman Zippel]

Hi,

On Fri, 8 Jul 2005, Pekka J Enberg wrote:

> I don't see how the following is tortured: 
> enum {
>   PNODE_MEMBER_VFS  = 0x01,
>   PNODE_SLAVE_VFS   = 0x02
> }; 
> In fact, I think it is more natural. An almost identical example even appears
> in K&R. 

So it basically comes down to personal preference, if the original uses 
defines and it works fine, I don't really see a good enough reason to 
change it to enums, so please leave the decision to author.

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Pekka J Enberg]

Roman Zippel writes:
> If it's really enumerated data types, that's fine, but this example was 
> about bitfield masks.


Bryan Henderson writes:
Ah.  In that case, enum is a pretty tortured way to declare it, though it 
does have the practical advantages over define that have been mentioned 
because the syntax is more rigorous.


I think the bit we're talking about is this: 


Index: 2.6.12/fs/pnode.c
===
--- /dev/null
+++ 2.6.12/fs/pnode.c
@@ -0,0 +1,362 @@
+
+#define PNODE_MEMBER_VFS  0x01
+#define PNODE_SLAVE_VFS   0x02


I don't see how the following is tortured: 


enum {
  PNODE_MEMBER_VFS  = 0x01,
  PNODE_SLAVE_VFS   = 0x02
}; 

In fact, I think it is more natural. An almost identical example even 
appears in K&R. 

  Pekka 


-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Wichert Akkerman]

Previously Mike Waychison wrote:
> enums in C are (de?)promoted to integral types under most conditions, so 
> the type-checking is useless.

It's a warning in gcc afaik and spare should complain as well.

Wichert.

-- 
Wichert Akkerman <[EMAIL PROTECTED]>It is simple to make things.
http://www.wiggy.net/   It is hard to make things simple.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Mike Waychison]

Wichert Akkerman wrote:

Previously Bryan Henderson wrote:

Two advantages of the enum declaration that haven't been mentioned yet, 
that help me significantly:



There is another one: with enums the compiler checks types for you.



enums in C are (de?)promoted to integral types under most conditions, so 
the type-checking is useless.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Wichert Akkerman]

Previously Bryan Henderson wrote:
> Two advantages of the enum declaration that haven't been mentioned yet, 
> that help me significantly:

There is another one: with enums the compiler checks types for you.

Wichert.

-- 
Wichert Akkerman <[EMAIL PROTECTED]>It is simple to make things.
http://www.wiggy.net/   It is hard to make things simple.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/8] share/private/slave a subtree

[Ram]

On Fri, 2005-07-08 at 09:51, Miklos Szeredi wrote:
> > > > + * recursively change the type of the mountpoint.
> > > > + */
> > > > +static int do_change_type(struct nameidata *nd, int flag)
> > > > +{
> > > > +   struct vfsmount *m, *mnt;
> > > > +   struct vfspnode *old_pnode = NULL;
> > > > +   int err;
> > > > +
> > > > +   if (!(flag & MS_SHARED) && !(flag & MS_PRIVATE)
> > > > +   && !(flag & MS_SLAVE))
> > > > +   return -EINVAL;
> > > > +
> > > > +   if ((err = _do_make_mounted(nd, &mnt)))
> > > > +   return err;
> > > 
> > > 
> > > Why does this opertation do any mounting?  If it's a type change, it
> > > should just change the type of something already mounted, no?
> > 
> > apart from changing types, it has to create a new vfsmount if one
> > does not exist at that point. 
> 
> Why?
> 
> I think it would be more logical to either
> 
>   - return -EINVAL if it's not a mountpoint
> 
>   - change the type of the mount even if it's not a mountpoint
> 
> Is there some reason the user can't do the 'bind dir dir' manually if
> needed?

The reason why I implemented that way, is to less confuse the user and
provide more flexibility. With my implementation, we have the ability
to share any part of the tree, without bothering if it is a mountpoint
or not. The side effect of this operation is, it ends up creating 
a vfsmount if the dentry is not a mountpoint.

so when a user says
  mount --make-shared /tmp/abc
the tree under /tmp/abc becomes shared. 
With your suggestion either the user will get -EINVAL or the tree
under / will become shared. The second behavior will be really
confusing. I am ok with -EINVAL. 


Also there is another reason why I used this behavior. Lets say /mnt
is a mountpoint and Say a user does
mount make-shared /mnt 

and then does 
mount --bind /mnt/abc  /mnt1

NOTE: we need propogation to be set up between /mnt/abc and /mnt1 and
propogation can only be set up for vfsmounts.  In this case /mnt/abc 
is not a mountpoint. I have two choices, either return -EINVAL
or create a vfsmount at that point. But -EINVAL is not consistent
with standard --bind behavior. So I chose the later behavior.

Now that we anyway need this behavior while doing bind mounts from
shared trees, I kept the same behavior for --make-shared.


> 
> 
> > > > +/*
> > > > + * Walk the pnode tree for each pnode encountered.  A given pnode in 
> > > > the tree
> > > > + * can be returned a minimum of 2 times.  First time the pnode is 
> > > > encountered,
> > > > + * it is returned with the flag PNODE_DOWN. Everytime the pnode is 
> > > > encountered
> > > > + * after having traversed through each of its children, it is returned 
> > > > with the
> > > > + * flag PNODE_MID.  And finally when the pnode is encountered after 
> > > > having
> > > > + * walked all of its children, it is returned with the flag PNODE_UP.
> > > > + *
> > > > + * @context: provides context on the state of the last walk in the 
> > > > pnode
> > > > + * tree.
> > > > + */
> > > > +static int inline
> > > > +pnode_next(struct pcontext *context)
> > > > +{
> > > 
> > > Is such a generic traversal function really needed?  Why?
> > 
> > Yes. I found it useful to write generic code without having to worry
> > about the details of the traversal being duplicated everywhere.  Do you
> > have better suggestion? This function is the equivalent of next_mnt()
> > for traversing pnode trees.
> 
> I'm just wondering, why do you need to return 2/3 times per node.  Are
> all these traversal points needed by propagation operations?  Why?

yes. It becomes highly necessary when we do mounts in shared
vfsmounts.  The mount has to be propogated to all the slave pnodes
as well as the member and slave mounts.  And in the processs of
propogation a new pnode propogation tree has to be created that
sets up the propogation for all the new child mounts.

The construction of the new child propogation tree during the process of
traversal of the parent's propogation tree, necessitates the need
for encountering the same pnode multiple times in different contexts.

Moreover I tried to abstract the propogation tree traversal as much
as possible so that all kinds of mount operations just have to
concentrate on the core operation instead of the details of the
traversal. pnode_opt.patch has most of these abstraction. 


> Some notes (maybe outside the code) explaining the mechanism of the
> propagations would be nice.  Without these it's hard to understand the
> design decisions behind such an implementation.

Ok. I will make a small writeup on the mechanism.


> 
> > > 
> > > > +struct vfsmount *
> > > > +pnode_make_mounted(struct vfspnode *pnode, struct vfsmount *mnt, 
> > > > struct dentry *dentry)
> > > > +{
> > > 
> > > Again a header comment would be nice, on what exactly this function
> > > does.  Also the implementation is really cryptic, but I can't even
> > > start 

Re: share/private/slave a subtree - define vs enum

[Bryan Henderson]

>If it's really enumerated data types, that's fine, but this example was 
>about bitfield masks.

Ah.  In that case, enum is a pretty tortured way to declare it, though it 
does have the practical advantages over define that have been mentioned 
because the syntax is more rigorous.

The proper way to do bitfield masks is usually C bit field declarations, 
but I understand that tradition works even more strongly against using 
those than against using enum to declare enumerated types.

>there is _nothing_ wrong with using defines for constants.

I disagree with that; I find practical and, more importantly, 
philosophical reasons not to use defines for constants.  I'm sure you've 
heard the arguments; I just didn't want to let that statement go 
uncontested.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Roman Zippel]

Hi,

On Fri, 8 Jul 2005, Bryan Henderson wrote:

> I wasn't aware anyone preferred defines to enums for declaring enumerated 
> data types.

If it's really enumerated data types, that's fine, but this example was 
about bitfield masks.

> Isn't the only argument for defines, "that's what I'm used to."?

defines are not just used for constants and there is _nothing_ wrong with 
using defines for constants.

> The macro language is one the most hated parts of the C language; it makes 
> sense to try to avoid it as a general rule.

Nevertheless it's part of the language, it's used all over the kernel and 
suddenly starting to mix different types of definitions, makes things 
only worse. I prefer consistency here over any minor advantages enums 
might have.

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/8] share/private/slave a subtree

[Miklos Szeredi]

> > > + * recursively change the type of the mountpoint.
> > > + */
> > > +static int do_change_type(struct nameidata *nd, int flag)
> > > +{
> > > + struct vfsmount *m, *mnt;
> > > + struct vfspnode *old_pnode = NULL;
> > > + int err;
> > > +
> > > + if (!(flag & MS_SHARED) && !(flag & MS_PRIVATE)
> > > + && !(flag & MS_SLAVE))
> > > + return -EINVAL;
> > > +
> > > + if ((err = _do_make_mounted(nd, &mnt)))
> > > + return err;
> > 
> > 
> > Why does this opertation do any mounting?  If it's a type change, it
> > should just change the type of something already mounted, no?
> 
> apart from changing types, it has to create a new vfsmount if one
> does not exist at that point. 

Why?

I think it would be more logical to either

  - return -EINVAL if it's not a mountpoint

  - change the type of the mount even if it's not a mountpoint

Is there some reason the user can't do the 'bind dir dir' manually if
needed?


> > > +/*
> > > + * Walk the pnode tree for each pnode encountered.  A given pnode in the 
> > > tree
> > > + * can be returned a minimum of 2 times.  First time the pnode is 
> > > encountered,
> > > + * it is returned with the flag PNODE_DOWN. Everytime the pnode is 
> > > encountered
> > > + * after having traversed through each of its children, it is returned 
> > > with the
> > > + * flag PNODE_MID.  And finally when the pnode is encountered after 
> > > having
> > > + * walked all of its children, it is returned with the flag PNODE_UP.
> > > + *
> > > + * @context: provides context on the state of the last walk in the pnode
> > > + *   tree.
> > > + */
> > > +static int inline
> > > +pnode_next(struct pcontext *context)
> > > +{
> > 
> > Is such a generic traversal function really needed?  Why?
> 
> Yes. I found it useful to write generic code without having to worry
> about the details of the traversal being duplicated everywhere.  Do you
> have better suggestion? This function is the equivalent of next_mnt()
> for traversing pnode trees.

I'm just wondering, why do you need to return 2/3 times per node.  Are
all these traversal points needed by propagation operations?  Why?

Some notes (maybe outside the code) explaining the mechanism of the
propagations would be nice.  Without these it's hard to understand the
design decisions behind such an implementation.

> > 
> > > +struct vfsmount *
> > > +pnode_make_mounted(struct vfspnode *pnode, struct vfsmount *mnt, struct 
> > > dentry *dentry)
> > > +{
> > 
> > Again a header comment would be nice, on what exactly this function
> > does.  Also the implementation is really cryptic, but I can't even
> > start to decipher without knowing what it's supposed to do.
> 
> yes. this function takes care of traversing the propogation tree and
> creating a child vfsmount for dentries in each vfsmount encountered.
> In other words it calls do_make_mounted() for each vfsmount that belongs
> to the propogation tree.

So it just does the propagated 'bind dir dir'?

Why not use the generic propagated bind routine (defined in a later
patch I presume) for this? 

Miklos
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree - define vs enum

[Bryan Henderson]

I wasn't aware anyone preferred defines to enums for declaring enumerated 
data types.  The practical advantages of enums are slight, but as far as I 
know, the practical advantages of defines are zero.  Isn't the only 
argument for defines, "that's what I'm used to."?

Two advantages of the enum declaration that haven't been mentioned yet, 
that help me significantly:

- if you have a typo in a define, it can be really hard to interpret the 
compiler error messages.  The same typo in an enum gets a pointed error 
message referring to the line that has the typo.

- Gcc warns you if a switch statement doesn't handle every case.  I often 
add an enumeration and Gcc lets me know where I forgot to consider it.

The macro language is one the most hated parts of the C language; it makes 
sense to try to avoid it as a general rule.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/8] share/private/slave a subtree

[Ram]

On Fri, 2005-07-08 at 04:17, Pekka Enberg wrote:
> On 7/8/05, Ram <[EMAIL PROTECTED]> wrote:
> > This patch adds the shared/private/slave support for VFS trees.
> 
> Inlining the patches to email would be greatly appreciated. Here are
> some comments.
> 
> > +int
> > +_do_make_mounted(struct nameidata *nd, struct vfsmount **mnt)
> 
> Use two underscores to follow naming conventions.

I have renamed this function as make_mounted() in the 2nd patch.
Sure, will follow the convention.

> 
> > Index: 2.6.12/fs/pnode.c
> > ===
> > --- /dev/null
> > +++ 2.6.12/fs/pnode.c
> > @@ -0,0 +1,362 @@
> > +
> > +#define PNODE_MEMBER_VFS  0x01
> > +#define PNODE_SLAVE_VFS   0x02
> 
> Enums, please.

ok.

> 
> > +
> > +static kmem_cache_t * pnode_cachep;
> > +
> > +/* spinlock for pnode related operations */
> > + __cacheline_aligned_in_smp DEFINE_SPINLOCK(vfspnode_lock);
> > +
> > +
> > +static void
> > +pnode_init_fn(void *data, kmem_cache_t *cachep, unsigned long flags)
> > +{
> > +   struct vfspnode *pnode = (struct vfspnode *)data;
> 
> Redundant cast.

ok.

> 
> > +   INIT_LIST_HEAD(&pnode->pnode_vfs);
> > +   INIT_LIST_HEAD(&pnode->pnode_slavevfs);
> > +   INIT_LIST_HEAD(&pnode->pnode_slavepnode);
> > +   INIT_LIST_HEAD(&pnode->pnode_peer_slave);
> > +   pnode->pnode_master = NULL;
> > +   pnode->pnode_flags = 0;
> > +   atomic_set(&pnode->pnode_count,0);
> > +}
> > +
> > +void __init
> > +pnode_init(unsigned long mempages)
> > +{
> > +   pnode_cachep = kmem_cache_create("pnode_cache",
> > +   sizeof(struct vfspnode), 0,
> > +   SLAB_HWCACHE_ALIGN|SLAB_PANIC, pnode_init_fn, NULL);
> > +}
> > +
> > +
> > +struct vfspnode *
> > +pnode_alloc(void)
> > +{
> > +   struct vfspnode *pnode =  (struct vfspnode *)kmem_cache_alloc(
> > +   pnode_cachep, GFP_KERNEL);
> 
> Redundant cast.

ok.

> 
> > +struct inoutdata {
> 
> Wants a better name.

ok. something like propogation_data? or pdata?

> 
> > +   void *my_data; /* produced and consumed by me */
> > +   void *in_data; /* produced by master, consumed by slave */
> > +   void *out_data; /* produced by slave, comsume by master */
> > +};
> > +
> > +struct pcontext {
> > +   struct vfspnode *start;
> > +   int flag;
> > +   int traversal;
> > +   int level;
> > +   struct vfspnode *master_pnode;
> > +   struct vfspnode *pnode;
> > +   struct vfspnode *slave_pnode;
> > +};
> > +
> > +
> > +#define PNODE_UP 1
> > +#define PNODE_DOWN 2
> > +#define PNODE_MID 3
> 
> Enums, please.

ok. I will incorporate all the rest of the comments.  There are lots of
places as noted by you I need to following the coding style. I will.

Thanks,
RP
 

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/8] share/private/slave a subtree

[Ram]

On Fri, 2005-07-08 at 07:32, Miklos Szeredi wrote:
> > This patch adds the shared/private/slave support for VFS trees.
> 
> [...]
> 
> > -struct vfsmount *lookup_mnt(struct vfsmount *mnt, struct dentry *dentry)
> > +struct vfsmount *lookup_mnt(struct vfsmount *mnt, struct dentry *dentry, 
> > struct dentry *root)
> >  {
> 
> How about changing it to inline and calling it __lookup_mnt_root(),
> and calling it from lookup_mnt() (which could keep the old signature)
> and lookup_mnt_root().  That way the compiler can optimize away the
> root check for the plain lookup_mnt() case, and there's no need to
> modify callers of lookup_mnt().
> 
> [...]


ok. 

> 
> >  
> > +struct vfsmount *do_make_mounted(struct vfsmount *mnt, struct dentry 
> > *dentry)
> > +{
> 
> What does this function do?  Can we have a header comment?

This function does the equivalent of 'mount --bind dir dir'
It  creates a new child vfsmount at that dentry, and moves
the children vfsmounts below that dentry, under the newly created child
vfsmount.  There is a header comment for that function. But it got
into the 2nd patch.

> 
> > +int
> > +_do_make_mounted(struct nameidata *nd, struct vfsmount **mnt)
> > +{
> 
> Ditto.

This function returns immeditely if a vfsmount already exists at the
dentry. Otherwise it creates a vfsmount at the specified dentry, and if
the dentry belongs to shared vfsmount it ensures the same
operation is done on all peer vfsmounts to which propogation
is set.


> 
> > +/*
> > + * recursively change the type of the mountpoint.
> > + */
> > +static int do_change_type(struct nameidata *nd, int flag)
> > +{
> > +   struct vfsmount *m, *mnt;
> > +   struct vfspnode *old_pnode = NULL;
> > +   int err;
> > +
> > +   if (!(flag & MS_SHARED) && !(flag & MS_PRIVATE)
> > +   && !(flag & MS_SLAVE))
> > +   return -EINVAL;
> > +
> > +   if ((err = _do_make_mounted(nd, &mnt)))
> > +   return err;
> 
> 
> Why does this opertation do any mounting?  If it's a type change, it
> should just change the type of something already mounted, no?

apart from changing types, it has to create a new vfsmount if one
does not exist at that point. 

> 
> > +   case MS_SHARED:
> > +   /*
> > +* if the mount is already a slave mount,
> > +* allocated a new pnode and make it
> > +* a slave pnode of the original pnode.
> > +*/
> > +   if (IS_MNT_SLAVE(m)) {
> > +   old_pnode = m->mnt_pnode;
> > +   pnode_del_slave_mnt(m);
> > +   }
> > +   if(!IS_MNT_SHARED(m)) {
> > +   m->mnt_pnode = pnode_alloc();
> > +   if(!m->mnt_pnode) {
> > +   pnode_add_slave_mnt(old_pnode, m);
> > +   err = -ENOMEM;
> > +   break;
> > +   }
> > +   pnode_add_member_mnt(m->mnt_pnode, m);
> > +   }
> > +   if(old_pnode) {
> > +   pnode_add_slave_pnode(old_pnode,
> > +   m->mnt_pnode);
> > +   }
> > +   SET_MNT_SHARED(m);
> > +   break;
> > +
> > +   case MS_SLAVE:
> > +   if (IS_MNT_SLAVE(m)) {
> > +   break;
> > +   }
> > +   /*
> > +* only shared mounts can
> > +* be made slave
> > +*/
> > +   if (!IS_MNT_SHARED(m)) {
> > +   err = -EINVAL;
> > +   break;
> > +   }
> > +   old_pnode = m->mnt_pnode;
> > +   pnode_del_member_mnt(m);
> > +   pnode_add_slave_mnt(old_pnode, m);
> > +   SET_MNT_SLAVE(m);
> > +   break;
> > +
> > +   case MS_PRIVATE:
> > +   if(m->mnt_pnode)
> > +   pnode_disassociate_mnt(m);
> > +   SET_MNT_PRIVATE(m);
> > +   break;
> > +
> 
> Can this be split into three functions?

yes. will do.

> 
> [...]
> 
> > +/*
> > + * Walk the pnode tree for each pnode encountered.  A given pnode in the 
> > tree
> > + * can be returned a minimum of 2 times.  First time the pnode is 
> > encountered,
> > + * it is returned with the flag PNODE_DOWN. Everytime the pnode is 
> > encountered
> > + * after having traversed through each of its children, it is returned 
> > with the
> > + * flag PNODE_MID.  And finally when the pnode is encountered after having
> > + * walked all of its children, it is returned with the flag PNODE_UP.
> > + *
> > + * @context: provides context on the state of the last walk in the pnode
> > + * tree

Re: share/private/slave a subtree

[Pekka Enberg]

On Fri, 2005-07-08 at 15:34 +0200, Roman Zippel wrote:
> Are the advantages big enough to actively discourage defines? It's nice 
> that you do reviews, but please leave some room for personal preferences. 
> If the code is correct and perfectly readable, it doesn't matter whether 
> to use defines or enums. Unless you also intent to also debug and work 
> with that code, why don't leave the decision to the author?

I think the advantages are big enough. Also, in my experience, it is
usually not a conscious decision by the author. But if you and other
developers think my enum pushing is too much, I can tone it down :).

Pekka

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/8] share/private/slave a subtree

[Miklos Szeredi]

> This patch adds the shared/private/slave support for VFS trees.

[...]

> -struct vfsmount *lookup_mnt(struct vfsmount *mnt, struct dentry *dentry)
> +struct vfsmount *lookup_mnt(struct vfsmount *mnt, struct dentry *dentry, 
> struct dentry *root)
>  {

How about changing it to inline and calling it __lookup_mnt_root(),
and calling it from lookup_mnt() (which could keep the old signature)
and lookup_mnt_root().  That way the compiler can optimize away the
root check for the plain lookup_mnt() case, and there's no need to
modify callers of lookup_mnt().

[...]

>  
> +struct vfsmount *do_make_mounted(struct vfsmount *mnt, struct dentry *dentry)
> +{

What does this function do?  Can we have a header comment?

> +int
> +_do_make_mounted(struct nameidata *nd, struct vfsmount **mnt)
> +{

Ditto.

> +/*
> + * recursively change the type of the mountpoint.
> + */
> +static int do_change_type(struct nameidata *nd, int flag)
> +{
> + struct vfsmount *m, *mnt;
> + struct vfspnode *old_pnode = NULL;
> + int err;
> +
> + if (!(flag & MS_SHARED) && !(flag & MS_PRIVATE)
> + && !(flag & MS_SLAVE))
> + return -EINVAL;
> +
> + if ((err = _do_make_mounted(nd, &mnt)))
> + return err;


Why does this opertation do any mounting?  If it's a type change, it
should just change the type of something already mounted, no?

> + case MS_SHARED:
> + /*
> +  * if the mount is already a slave mount,
> +  * allocated a new pnode and make it
> +  * a slave pnode of the original pnode.
> +  */
> + if (IS_MNT_SLAVE(m)) {
> + old_pnode = m->mnt_pnode;
> + pnode_del_slave_mnt(m);
> + }
> + if(!IS_MNT_SHARED(m)) {
> + m->mnt_pnode = pnode_alloc();
> + if(!m->mnt_pnode) {
> + pnode_add_slave_mnt(old_pnode, m);
> + err = -ENOMEM;
> + break;
> + }
> + pnode_add_member_mnt(m->mnt_pnode, m);
> + }
> + if(old_pnode) {
> + pnode_add_slave_pnode(old_pnode,
> + m->mnt_pnode);
> + }
> + SET_MNT_SHARED(m);
> + break;
> +
> + case MS_SLAVE:
> + if (IS_MNT_SLAVE(m)) {
> + break;
> + }
> + /*
> +  * only shared mounts can
> +  * be made slave
> +  */
> + if (!IS_MNT_SHARED(m)) {
> + err = -EINVAL;
> + break;
> + }
> + old_pnode = m->mnt_pnode;
> + pnode_del_member_mnt(m);
> + pnode_add_slave_mnt(old_pnode, m);
> + SET_MNT_SLAVE(m);
> + break;
> +
> + case MS_PRIVATE:
> + if(m->mnt_pnode)
> + pnode_disassociate_mnt(m);
> + SET_MNT_PRIVATE(m);
> + break;
> +

Can this be split into three functions?

[...]

> +/*
> + * Walk the pnode tree for each pnode encountered.  A given pnode in the tree
> + * can be returned a minimum of 2 times.  First time the pnode is 
> encountered,
> + * it is returned with the flag PNODE_DOWN. Everytime the pnode is 
> encountered
> + * after having traversed through each of its children, it is returned with 
> the
> + * flag PNODE_MID.  And finally when the pnode is encountered after having
> + * walked all of its children, it is returned with the flag PNODE_UP.
> + *
> + * @context: provides context on the state of the last walk in the pnode
> + *   tree.
> + */
> +static int inline
> +pnode_next(struct pcontext *context)
> +{

Is such a generic traversal function really needed?  Why?

[...]

> +struct vfsmount *
> +pnode_make_mounted(struct vfspnode *pnode, struct vfsmount *mnt, struct 
> dentry *dentry)
> +{

Again a header comment would be nice, on what exactly this function
does.  Also the implementation is really cryptic, but I can't even
start to decipher without knowing what it's supposed to do.

[...]

> +static inline struct vfspnode *
> +get_pnode_n(struct vfspnode *pnode, size_t n)
> +{

Seems to be unused throughout the patch series

> + struct list_head mnt_pnode_mntlist;/* and going through their
> +pnode's vfsmount */
> + struct vfspnode *mnt_pnode; /* and going through their
> +pnode's vfsmount *

Re: share/private/slave a subtree

[Roman Zippel]

Hi,

On Fri, 8 Jul 2005, Pekka J Enberg wrote:

> > You can't do that with defines?
> 
> Sure you can but have you ever tried to figure out where a group of #define
> enumerations end?

Comments? Newlines?

> Enums are a natural language construct for grouping related
> constants so why not use it? 

So are defines.

> Bottom line, there are few advantages to using enums rather than #defines
> which is why they are IMHO preferred for new code. 

Are the advantages big enough to actively discourage defines? It's nice 
that you do reviews, but please leave some room for personal preferences. 
If the code is correct and perfectly readable, it doesn't matter whether 
to use defines or enums. Unless you also intent to also debug and work 
with that code, why don't leave the decision to the author?

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree

[Pekka J Enberg]

On Fri, 8 Jul 2005, Pekka J Enberg wrote:

> Hey, I just review patches. I don't get to set requirements. There's a reason
> why enums are preferred though. They define a proper name for the constant.


Roman Zippel writes:

Who prefers that?


Well, me, at least. I can't speak for others. 


On Fri, 8 Jul 2005, Pekka J Enberg wrote:

> It's far to easy to mess up with #defines.


Roman Zippel writes:

Rather unlikely with such simple masks.


Redefining a constant with #define by an accident is easy. Introducing 
duplicate constants is equally easy (see radeon headers for an example). 


On Fri, 8 Jul 2005, Pekka J Enberg wrote:

> They also document the code intent
> much better as you can group related constants together.


Roman Zippel writes:

You can't do that with defines?


Sure you can but have you ever tried to figure out where a group of #define 
enumerations end? Enums are a natural language construct for grouping 
related constants so why not use it? 

Bottom line, there are few advantages to using enums rather than #defines 
which is why they are IMHO preferred for new code. 

   Pekka 


-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree

[Roman Zippel]

Hi,

On Fri, 8 Jul 2005, Pekka J Enberg wrote:

> On Fri, 8 Jul 2005, Pekka Enberg wrote:
> > > > +#define PNODE_MEMBER_VFS  0x01
> > > > +#define PNODE_SLAVE_VFS   0x02
> > > > Enums, please.
> 
> Roman Zippel writes:
> > Is this becoming a requirement now? I personally would rather leave that to
> > personal preference...
> 
> Hey, I just review patches. I don't get to set requirements. There's a reason
> why enums are preferred though. They define a proper name for the constant.

Who prefers that?

> It's far to easy to mess up with #defines.

Rather unlikely with such simple masks.

> They also document the code intent
> much better as you can group related constants together. 

You can't do that with defines?

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: share/private/slave a subtree

[Pekka J Enberg]

On Fri, 8 Jul 2005, Pekka Enberg wrote:

> > +#define PNODE_MEMBER_VFS  0x01
> > +#define PNODE_SLAVE_VFS   0x02
> 
> Enums, please.


Roman Zippel writes:
Is this becoming a requirement now? I personally would rather leave that 
to personal preference...


Hey, I just review patches. I don't get to set requirements. There's a 
reason why enums are preferred though. They define a proper name for the 
constant. It's far to easy to mess up with #defines. They also document the 
code intent much better as you can group related constants together. 


Pekka
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/8] share/private/slave a subtree

[Roman Zippel]

Hi,

On Fri, 8 Jul 2005, Pekka Enberg wrote:

> > +#define PNODE_MEMBER_VFS  0x01
> > +#define PNODE_SLAVE_VFS   0x02
> 
> Enums, please.

Is this becoming a requirement now? I personally would rather leave that 
to personal preference...

bye, Roman
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/8] share/private/slave a subtree

[Pekka Enberg]

On 7/8/05, Ram <[EMAIL PROTECTED]> wrote:
> This patch adds the shared/private/slave support for VFS trees.

Inlining the patches to email would be greatly appreciated. Here are
some comments.

> +int
> +_do_make_mounted(struct nameidata *nd, struct vfsmount **mnt)

Use two underscores to follow naming conventions.

> Index: 2.6.12/fs/pnode.c
> ===
> --- /dev/null
> +++ 2.6.12/fs/pnode.c
> @@ -0,0 +1,362 @@
> +
> +#define PNODE_MEMBER_VFS  0x01
> +#define PNODE_SLAVE_VFS   0x02

Enums, please.

> +
> +static kmem_cache_t * pnode_cachep;
> +
> +/* spinlock for pnode related operations */
> + __cacheline_aligned_in_smp DEFINE_SPINLOCK(vfspnode_lock);
> +
> +
> +static void
> +pnode_init_fn(void *data, kmem_cache_t *cachep, unsigned long flags)
> +{
> + struct vfspnode *pnode = (struct vfspnode *)data;

Redundant cast.

> + INIT_LIST_HEAD(&pnode->pnode_vfs);
> + INIT_LIST_HEAD(&pnode->pnode_slavevfs);
> + INIT_LIST_HEAD(&pnode->pnode_slavepnode);
> + INIT_LIST_HEAD(&pnode->pnode_peer_slave);
> + pnode->pnode_master = NULL;
> + pnode->pnode_flags = 0;
> + atomic_set(&pnode->pnode_count,0);
> +}
> +
> +void __init
> +pnode_init(unsigned long mempages)
> +{
> + pnode_cachep = kmem_cache_create("pnode_cache",
> +   sizeof(struct vfspnode), 0,
> +   SLAB_HWCACHE_ALIGN|SLAB_PANIC, pnode_init_fn, NULL);
> +}
> +
> +
> +struct vfspnode *
> +pnode_alloc(void)
> +{
> + struct vfspnode *pnode =  (struct vfspnode *)kmem_cache_alloc(
> + pnode_cachep, GFP_KERNEL);

Redundant cast.

> +struct inoutdata {

Wants a better name.

> + void *my_data; /* produced and consumed by me */
> + void *in_data; /* produced by master, consumed by slave */
> + void *out_data; /* produced by slave, comsume by master */
> +};
> +
> +struct pcontext {
> + struct vfspnode *start;
> + int flag;
> + int traversal;
> + int level;
> + struct vfspnode *master_pnode;
> + struct vfspnode *pnode;
> + struct vfspnode *slave_pnode;
> +};
> +
> +
> +#define PNODE_UP 1
> +#define PNODE_DOWN 2
> +#define PNODE_MID 3

Enums, please.

> +
> +/*
> + * Walk the pnode tree for each pnode encountered.  A given pnode in the tree
> + * can be returned a minimum of 2 times.  First time the pnode is 
> encountered,
> + * it is returned with the flag PNODE_DOWN. Everytime the pnode is 
> encountered
> + * after having traversed through each of its children, it is returned with 
> the
> + * flag PNODE_MID.  And finally when the pnode is encountered after having
> + * walked all of its children, it is returned with the flag PNODE_UP.
> + *
> + * @context: provides context on the state of the last walk in the pnode
> + *   tree.
> + */
> +static int inline
> +pnode_next(struct pcontext *context)

Rather large function to be an inline.

> +{
> + int traversal = context->traversal;
> + int ret=0;
> + struct vfspnode *pnode = context->pnode,
> + *slave_pnode=context->slave_pnode,
> + *master_pnode=context->master_pnode;

Add a separate declaration for each variable. The above is hard to read.

> + struct list_head *next;
> +
> + spin_lock(&vfspnode_lock);
> + /*
> +  * the first traversal will be on the root pnode
> +  * with flag PNODE_DOWN
> +  */
> + if (!pnode) {
> + context->pnode = get_pnode(context->start);
> + context->master_pnode = NULL;
> + context->traversal = PNODE_DOWN;
> + context->slave_pnode = NULL;
> + context->level = 0;
> + ret = 1;
> + goto out;
> + }
> +
> + /*
> +  * if the last traversal was PNODE_UP, than the current
> +  * traversal is PNODE_MID on the master pnode.
> +  */
> + if (traversal == PNODE_UP) {
> + if (!master_pnode) {
> + /* DONE. return */
> + put_pnode(pnode);
> + ret = 0;

Using goto out and dropping the else branch would make this more readable.

> + } else {
> + context->traversal = PNODE_MID;
> + context->level--;
> + context->pnode = master_pnode;
> + put_pnode(slave_pnode);
> + context->slave_pnode = pnode;
> + context->master_pnode = (master_pnode ?
> + master_pnode->pnode_master : NULL);
> + ret = 1;
> + }
> + } else {
> + if(traversal == PNODE_MID) {

Missing space before parenthesis.

> + next = slave_pnode->pnode_peer_slave.next;
> + } else {
> + next = pnode->pnode_slavepnode.next;
> + }

Please drop the extra braces.

> + put_pnode(slave_pnode);
> + cont

[RFC PATCH 1/8] share/private/slave a subtree

[Ram]

This patch adds the shared/private/slave support for VFS trees.

RP
This patch adds the shared/private/slave support for VFS trees.

Signed by Ram Pai ([EMAIL PROTECTED])

 fs/Makefile|2 
 fs/dcache.c|2 
 fs/namei.c |4 
 fs/namespace.c |  170 
 fs/pnode.c |  335 +
 include/linux/dcache.h |2 
 include/linux/fs.h |5 
 include/linux/mount.h  |   48 ++-
 include/linux/pnode.h  |   82 +++
 9 files changed, 641 insertions(+), 9 deletions(-)

Index: 2.6.12/fs/namespace.c
===
--- 2.6.12.orig/fs/namespace.c
+++ 2.6.12/fs/namespace.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -62,6 +63,7 @@ struct vfsmount *alloc_vfsmnt(const char
 		INIT_LIST_HEAD(&mnt->mnt_mounts);
 		INIT_LIST_HEAD(&mnt->mnt_list);
 		INIT_LIST_HEAD(&mnt->mnt_fslink);
+		INIT_LIST_HEAD(&mnt->mnt_pnode_mntlist);
 		if (name) {
 			int size = strlen(name)+1;
 			char *newname = kmalloc(size, GFP_KERNEL);
@@ -84,7 +86,7 @@ void free_vfsmnt(struct vfsmount *mnt)
  * Now, lookup_mnt increments the ref count before returning
  * the vfsmount struct.
  */
-struct vfsmount *lookup_mnt(struct vfsmount *mnt, struct dentry *dentry)
+struct vfsmount *lookup_mnt(struct vfsmount *mnt, struct dentry *dentry, struct dentry *root)
 {
 	struct list_head * head = mount_hashtable + hash(mnt, dentry);
 	struct list_head * tmp = head;
@@ -97,7 +99,8 @@ struct vfsmount *lookup_mnt(struct vfsmo
 		if (tmp == head)
 			break;
 		p = list_entry(tmp, struct vfsmount, mnt_hash);
-		if (p->mnt_parent == mnt && p->mnt_mountpoint == dentry) {
+		if (p->mnt_parent == mnt && p->mnt_mountpoint == dentry &&
+(root == NULL || p->mnt_root == root )) {
 			found = mntget(p);
 			break;
 		}
@@ -119,6 +122,7 @@ static void detach_mnt(struct vfsmount *
 	mnt->mnt_mountpoint = mnt->mnt_root;
 	list_del_init(&mnt->mnt_child);
 	list_del_init(&mnt->mnt_hash);
+	list_del_init(&mnt->mnt_pnode_mntlist);
 	old_nd->dentry->d_mounted--;
 }
 
@@ -161,6 +165,7 @@ clone_mnt(struct vfsmount *old, struct d
 		mnt->mnt_mountpoint = mnt->mnt_root;
 		mnt->mnt_parent = mnt;
 		mnt->mnt_namespace = old->mnt_namespace;
+		mnt->mnt_pnode = get_pnode(old->mnt_pnode);
 
 		/* stick the duplicate mount on the same expiry list
 		 * as the original if that was on one */
@@ -176,6 +181,7 @@ void __mntput(struct vfsmount *mnt)
 {
 	struct super_block *sb = mnt->mnt_sb;
 	dput(mnt->mnt_root);
+	put_pnode(mnt->mnt_pnode);
 	free_vfsmnt(mnt);
 	deactivate_super(sb);
 }
@@ -615,6 +621,148 @@ out_unlock:
 	return err;
 }
 
+struct vfsmount *do_make_mounted(struct vfsmount *mnt, struct dentry *dentry)
+{
+	struct vfsmount *child_mnt, *next;
+	struct nameidata nd;
+	struct vfsmount *newmnt = clone_mnt(mnt, dentry);
+	LIST_HEAD(move);
+
+	if (newmnt) {
+		/* put in code for mount expiry */
+		/* TOBEDONE */
+
+		/*
+		 * walk through the mount list of mnt and move
+		 * them under the new mount
+		 */
+		spin_lock(&vfsmount_lock);
+		list_for_each_entry_safe(child_mnt, next,
+&mnt->mnt_mounts, mnt_child) {
+
+			if(child_mnt->mnt_mountpoint == dentry)
+continue;
+
+			if(!is_subdir(child_mnt->mnt_mountpoint, dentry))
+continue;
+
+			detach_mnt(child_mnt, &nd);
+			nd.mnt = newmnt;
+			attach_mnt(child_mnt, &nd);
+		}
+
+		nd.mnt = mnt;
+		nd.dentry = dentry;
+		attach_mnt(newmnt, &nd);
+ 		spin_unlock(&vfsmount_lock);
+ 	}
+	return newmnt;
+}
+
+int
+_do_make_mounted(struct nameidata *nd, struct vfsmount **mnt)
+{
+	struct vfsmount *parent_mnt;
+	struct dentry *parent_dentry;
+	int err = mount_is_safe(nd);
+	if (err)
+		return err;
+	parent_dentry = nd->dentry;
+	parent_mnt = nd->mnt;
+ 	/*
+	 * check if dentry already has a vfsmount
+	 * if it does not, create a vfsmount there.
+	 * which means you need to propogate it
+	 * across all vfsmounts.
+ 	 */
+	if(parent_dentry == parent_mnt->mnt_root) {
+		*mnt = parent_mnt;
+	} else {
+		*mnt = IS_MNT_SHARED(parent_mnt) ?
+			 pnode_make_mounted(parent_mnt->mnt_pnode,
+	 parent_mnt, parent_dentry) :
+			 do_make_mounted(parent_mnt, parent_dentry);
+		if (!*mnt)
+			err = -ENOMEM;
+ 	}
+	return err;
+}
+
+/*
+ * recursively change the type of the mountpoint.
+ */
+static int do_change_type(struct nameidata *nd, int flag)
+{
+	struct vfsmount *m, *mnt;
+	struct vfspnode *old_pnode = NULL;
+	int err;
+
+	if (!(flag & MS_SHARED) && !(flag & MS_PRIVATE)
+			&& !(flag & MS_SLAVE))
+		return -EINVAL;
+
+	if ((err = _do_make_mounted(nd, &mnt)))
+		return err;
+
+	spin_lock(&vfsmount_lock);
+	for (m = mnt; m; m = next_mnt(m, mnt)) {
+		switch (flag) {
+		case MS_SHARED:
+			/*
+			 * if the mount is already a slave mount,
+			 * allocated a new pnode and make it
+			 * a slave pnode of the original pnode.
+			 */
+			if (IS_MNT_SLAVE(m)) {
+old_pnode = m->mnt_pnode;
+pnode_del_slave_