Lustre devel - Mar 2007 - struct nameidata

Hi,

I have a question to the vfs_intent-2.6.12.patch.

In the vanilla kernel struct nameidata looks like this:

struct nameidata {
	struct dentry	*dentry;
	struct vfsmount *mnt;
	struct qstr	last;
	unsigned int	flags;
	int		last_type;
	unsigned	depth;
	char *saved_names[MAX_NESTED_LINKS + 1];

	/* Intent data */
	union {
		struct open_intent open;
	} intent;
};

The union intent looks to me as if it was planned to add for each call a 
struct {call}_intent. But since I''m rather new to kernel coding I might
be
wrong.

Now in vfs_intent-2.6.12.patch this union is incompatibly replaced by struct 
lookup_intent

 
@@ -21,10 +55,7 @@
        unsigned        depth;
        char *saved_names[MAX_NESTED_LINKS + 1];

-       /* Intent data */
-       union {
-               struct open_intent open;
-       } intent;
+       struct lookup_intent intent;
 };

 /*


So each other filesystem using union open will break to compile. I guess that 
as of 2.6.12 no other filesystem broke, but with 2.6.20, at least two other 
filesystem them won''t compile anymore. So we either have the option to
fix
those filesystems or to fix lustre. 
Personally I would tend to keep in kernel filesystems the way they are and to 
fix lustre. But maybe there is a reason why the patch was done the way it 
presently is?

Fixing lustre:
I already don''t understand why struct lookup_intent is actually 
called "lookup_intent". Is it really for lookup purposes only? But why
does
it have a member "int it_create_mode" then? 
To me struct lookup_intent looks like as if one wanted to avoid the struct 
{call}_intent for each fs operation wrote a more general struct 
lookup_intent. 
I also don''t understand why it was done incompatibly with union intent.
I''m
mean, it was to expect that some day other filesystems will begin to use the 
member(s) of this union?


Thanks in advance,
Bernd



-- 
Bernd Schubert
Q-Leap Networks GmbH / transtec AG / ttec

In message <200703211649.29292.bs@q-leap.de>,Bernd Schubert
writes:
>The union intent looks to me as if it was planned to add for each call a 
>struct {call}_intent. But since I''m rather new to kernel coding I
might be
>wrong.
it seems likely this is the intent.  it mirrors the per filesystem bit
that is in struct inode.  its not clear to me how useful this would
be though because it would seem that other union member would need to
share the atleast the same initial contents as open_intent since vfs
layer wants to set these elements regardless.
>Now in vfs_intent-2.6.12.patch this union is incompatibly replaced by struct
>lookup_intent
>
> 
>@@ -21,10 +55,7 @@
>        unsigned        depth;
>        char *saved_names[MAX_NESTED_LINKS + 1];
>
>-       /* Intent data */
>-       union {
>-               struct open_intent open;
>-       } intent;
>+       struct lookup_intent intent;
> };
>
> /*
>
>
>So each other filesystem using union open will break to compile. I guess
that
>as of 2.6.12 no other filesystem broke, but with 2.6.20, at least two other 
>filesystem them won''t compile anymore. So we either have the option
to fix
>those filesystems or to fix lustre. 
when confronted with this same dilemma, i choose to keep struct nameidata
as it was, add a 

	#define lookup_intent open_intent

and just modify to open_intent to have the extra lustre bits.  the patches
to lustre were pretty small (smaller than fixing the kernel).  mostly it
was changing ->intent to ->intent.open.

On Wednesday 21 March 2007 20:54:15 chas williams - CONTRACTOR
wrote:
> In message <200703211649.29292.bs@q-leap.de>,Bernd Schubert writes:
> >So each other filesystem using union open will break to compile. I
guess
> > that as of 2.6.12 no other filesystem broke, but with 2.6.20, at least
> > two other filesystem them won''t compile anymore. So we either
have the
> > option to fix those filesystems or to fix lustre.
>
> when confronted with this same dilemma, i choose to keep struct nameidata
> as it was, add a
>
> 	#define lookup_intent open_intent
>
> and just modify to open_intent to have the extra lustre bits.  the patches
> to lustre were pretty small (smaller than fixing the kernel).  mostly it
> was changing ->intent to ->intent.open.
Thanks, we did it similarily now. That way almost no lustre patches to other 
file systems are required and IMHO clusterfs should itegrate those patches 
into future lustre releases.

-- 
Bernd Schubert
Q-Leap Networks GmbH / transtec AG / ttec