All of lore.kernel.org
 help / color / mirror / Atom feed
* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
@ 2007-02-07  4:51 Wendy Cheng
  2007-02-07 10:49 ` Steven Whitehouse
  0 siblings, 1 reply; 14+ messages in thread
From: Wendy Cheng @ 2007-02-07  4:51 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Lookup failure found in '07 NFS connectathon. The nfsd is in the middle
of readdirplus procedure call where it builds the file handles
associated with the directory. GFS2 lookup code has been expected the
first 64 bit of gfs2 inode number (formal inode number) but
do_filldir_main() wrongly passes on-disk address inode number (no_addr).
This patch fixes this.

Signed-off-by: S. Wendy Cheng <wcheng@redhat.com>

dir.c |    3 ++-
1 files changed, 2 insertions(+), 1 deletion(-)

--- linux-git/fs/gfs2/dir.c	2007-02-06 01:07:26.000000000 -0500
+++ linux/fs/gfs2/dir.c	2007-02-06 20:50:16.000000000 -0500
@@ -1241,8 +1241,9 @@ static int do_filldir_main(struct gfs2_i
 
 		error = filldir(opaque, (const char *)(dent + 1),
 				be16_to_cpu(dent->de_name_len),
-				off, be64_to_cpu(dent->de_inum.no_addr),
+				off, be64_to_cpu(dent->de_inum.no_formal_ino),
 				be16_to_cpu(dent->de_type));
+		
 		if (error)
 			return 1;
 




^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-07  4:51 [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main Wendy Cheng
@ 2007-02-07 10:49 ` Steven Whitehouse
  2007-02-07 22:35   ` Wendy Cheng
  0 siblings, 1 reply; 14+ messages in thread
From: Steven Whitehouse @ 2007-02-07 10:49 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Hi,

I'm not sure that this is the right fix. no_formal_ino is used only as a
hash value in the lookup, the "real" lookup is always via no_addr. So we
can alter that hash if required to just use no_addr, but I'd rather not
have to return no_formal_ino here since we have no way to map that back
to the disk location of the inode in general. In fact given a maximum
sized GFS2 filesystem and enough nodes to fill it with inodes in a
reasonable time (yes, I know thats a lot and a long time!), its not even
certain to be unique,

Steve.

On Tue, 2007-02-06 at 23:51 -0500, Wendy Cheng wrote:
> Lookup failure found in '07 NFS connectathon. The nfsd is in the middle
> of readdirplus procedure call where it builds the file handles
> associated with the directory. GFS2 lookup code has been expected the
> first 64 bit of gfs2 inode number (formal inode number) but
> do_filldir_main() wrongly passes on-disk address inode number (no_addr).
> This patch fixes this.
> 
> Signed-off-by: S. Wendy Cheng <wcheng@redhat.com>
> 
> dir.c |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletion(-)
> 
> --- linux-git/fs/gfs2/dir.c	2007-02-06 01:07:26.000000000 -0500
> +++ linux/fs/gfs2/dir.c	2007-02-06 20:50:16.000000000 -0500
> @@ -1241,8 +1241,9 @@ static int do_filldir_main(struct gfs2_i
>  
>  		error = filldir(opaque, (const char *)(dent + 1),
>  				be16_to_cpu(dent->de_name_len),
> -				off, be64_to_cpu(dent->de_inum.no_addr),
> +				off, be64_to_cpu(dent->de_inum.no_formal_ino),
>  				be16_to_cpu(dent->de_type));
> +		
>  		if (error)
>  			return 1;
>  
> 
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-07 10:49 ` Steven Whitehouse
@ 2007-02-07 22:35   ` Wendy Cheng
  2007-02-07 23:37     ` Wendy Cheng
  2007-02-08  9:42     ` Steven Whitehouse
  0 siblings, 2 replies; 14+ messages in thread
From: Wendy Cheng @ 2007-02-07 22:35 UTC (permalink / raw)
  To: cluster-devel.redhat.com

On Wed, 2007-02-07 at 10:49 +0000, Steven Whitehouse wrote:
> Hi,
> 
> I'm not sure that this is the right fix. no_formal_ino is used only as a
> hash value in the lookup, the "real" lookup is always via no_addr. So we
> can alter that hash if required to just use no_addr, but I'd rather not
> have to return no_formal_ino here since we have no way to map that back
> to the disk location of the inode in general. In fact given a maximum
> sized GFS2 filesystem and enough nodes to fill it with inodes in a
> reasonable time (yes, I know thats a lot and a long time!), its not even
> certain to be unique,
> 

NFS won't run well without this patch - I originally included this into
an "if command == nfsd" clause. Later checking into Ken's description
(in cluster-list at redhat.com) about this issue, I think his intention
here was that any ino returned to user space should use no_formal_ino.
And this is exactly what the filldir should be doing (by returning
no_formal_ino back to caller). 

Another option is temporarily disabling this feature and make this works
as GFS1 (with these two inode numbers identical) ? 

I like the current patch though. 

-- Wendy



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-07 22:35   ` Wendy Cheng
@ 2007-02-07 23:37     ` Wendy Cheng
  2007-02-08  9:42     ` Steven Whitehouse
  1 sibling, 0 replies; 14+ messages in thread
From: Wendy Cheng @ 2007-02-07 23:37 UTC (permalink / raw)
  To: cluster-devel.redhat.com

On Wed, 2007-02-07 at 17:35 -0500, Wendy Cheng wrote:
> On Wed, 2007-02-07 at 10:49 +0000, Steven Whitehouse wrote:
> > Hi,
> > 
> > I'm not sure that this is the right fix. no_formal_ino is used only as a
> > hash value in the lookup, the "real" lookup is always via no_addr. So we
> > can alter that hash if required to just use no_addr, but I'd rather not
> > have to return no_formal_ino here since we have no way to map that back
> > to the disk location of the inode in general. In fact given a maximum
> > sized GFS2 filesystem and enough nodes to fill it with inodes in a
> > reasonable time (yes, I know thats a lot and a long time!), its not even
> > certain to be unique,
> > 
> 
> NFS won't run well without this patch - I originally included this into
> an "if command == nfsd" clause. Later checking into Ken's description
> (in cluster-list at redhat.com) about this issue, I think his intention
> here was that any ino returned to user space should use no_formal_ino.
> And this is exactly what the filldir should be doing (by returning
> no_formal_ino back to caller). 
> 
> Another option is temporarily disabling this feature and make this works
> as GFS1 (with these two inode numbers identical) ? 


To make it clear .. we need to either change the original design or I
think we should take this patch - since

1. The contents of filldir are intended to get passed to user mode - and
that should be the formal inode number
2. NFS can't run without this change.

-- Wendy



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-07 22:35   ` Wendy Cheng
  2007-02-07 23:37     ` Wendy Cheng
@ 2007-02-08  9:42     ` Steven Whitehouse
  2007-02-24  3:56       ` Wendy Cheng
  1 sibling, 1 reply; 14+ messages in thread
From: Steven Whitehouse @ 2007-02-08  9:42 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Hi,

On Wed, 2007-02-07 at 17:35 -0500, Wendy Cheng wrote:
> On Wed, 2007-02-07 at 10:49 +0000, Steven Whitehouse wrote:
> > Hi,
> > 
> > I'm not sure that this is the right fix. no_formal_ino is used only as a
> > hash value in the lookup, the "real" lookup is always via no_addr. So we
> > can alter that hash if required to just use no_addr, but I'd rather not
> > have to return no_formal_ino here since we have no way to map that back
> > to the disk location of the inode in general. In fact given a maximum
> > sized GFS2 filesystem and enough nodes to fill it with inodes in a
> > reasonable time (yes, I know thats a lot and a long time!), its not even
> > certain to be unique,
> > 
> 
> NFS won't run well without this patch - I originally included this into
> an "if command == nfsd" clause. Later checking into Ken's description
> (in cluster-list at redhat.com) about this issue, I think his intention
> here was that any ino returned to user space should use no_formal_ino.
> And this is exactly what the filldir should be doing (by returning
> no_formal_ino back to caller). 
> 
I'd rather not use no_formal_ino at all if possible. It is not, in
general, unique for any fs which has seen a lot of activity and there is
also no way to look up an inode from its no_formal_ino alone without
potentially searching the entire fs. To my mind an inode number should
be guaranteed to be unique for the lifetime of the inode and also be
useable as a reference by which the inode can be looked up (whether
thats directly by making it identical to the disk block number or
indirectly by using a lookup table, for example), no_formal_ino doesn't
appear to satisfy either condition where as no_addr does.

If there was an index to map no_formal_ino to a disk block, then I'd
agree, but I'd much prefer that we use no_addr in the mean time. I
suspect that all you need to do is to alter gfs2_ilookup() and
gfs2_iget() to use no_addr rather than no_formal_ino as the hash value.

Since that hash is only used internally, there should be no problem in
altering it and I think that will result in the same thing.

What I've not been able to work out is how your scheme can work in the
case that the inode is not already in cache, since filldir doesn't then
appear to get passed the no_addr at all.

> Another option is temporarily disabling this feature and make this works
> as GFS1 (with these two inode numbers identical) ? 
> 
> I like the current patch though. 
> 
> -- Wendy
Well one other option is to use the inode version number that was added recently
to each inode. The idea of no_formal_ino is that its not really an inode
number at all in the traditional sense, but just a way to ensure that an
inode isn't being accessed from a stale filehandle.

I added the inode version number with the intention that we'd be able to
use it instead of no_formal_ino, eventually, with NFS. One reason is
that because the max sized filesystem is 2^64 blocks and the
no_formal_ino is also 64 bits in size its quite likely to correlate with
the size of the fs and thus generate identical numbers once its wrapped
for similar disk offsets. Thats also true for any smaller fs which is a
power of two in size I think, but with a large number of rewrites of the
fs required in each case.

With the version number scheme, the version numbers are unique to a
particular resource group so we are getting 2^64/2^32 (for a max sized
resource group) = 2^32 (average) reallocations of each block within the
rgrp as an inode before the counter will wrap,

Steve.




^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-08  9:42     ` Steven Whitehouse
@ 2007-02-24  3:56       ` Wendy Cheng
  2007-02-26 15:45         ` Steven Whitehouse
  0 siblings, 1 reply; 14+ messages in thread
From: Wendy Cheng @ 2007-02-24  3:56 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Steve,

I gave this issue some more thoughts - would like to suggest we take 
this patch (at least for now) since it aligns with current code base.

The no_formal_ino is apparently intented to get returned back to user 
space due to its unique-ness (and we have to trust pick_formal_ino() 
does its job right). With normal readdir system call, after the inode 
number is sent to user space, there is no route (I've checked) for it to 
come back to kernel. So the only user that would use these filldir ino 
inside the kernel is NFSD. NFSD calls gfs2_ilookup() that requires 
no_formal_ino.

If you look further... The current lookup code actually uses 
no_formal_ino, not no_addr. The two main "gate" routines that controls 
ino-to-inode conversion are:

* gfs2_ilookup() (used by NFS route)
* gfs2_inode_lookup() (used by VFS that calls gfs2_lookup()).

Both use no_formal_ino - gfs2_inode_lookup() logic hides this behind the 
little wrapper "gfs2_iget()". Since current VFS side's lookup has been 
working fine, this no_formal_ino idea apparently is working. So let's 
make NFSD side work the *same* way. I'm convinced this patch does a 
right thing.

I don't dispute using generation number may not be a bad idea and may 
perform better. However, if we really look into the details, it is not 
easy for current code structure. Since we have something already 
working, it is not wise to mess this code layout at this moment.

Make sense ?

-- Wendy




^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-24  3:56       ` Wendy Cheng
@ 2007-02-26 15:45         ` Steven Whitehouse
  2007-02-26 16:11           ` Jonathan E Brassow
  0 siblings, 1 reply; 14+ messages in thread
From: Steven Whitehouse @ 2007-02-26 15:45 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Hi,

On Fri, 2007-02-23 at 22:56 -0500, Wendy Cheng wrote:
> Steve,
> 
> I gave this issue some more thoughts - would like to suggest we take 
> this patch (at least for now) since it aligns with current code base.
> 
> The no_formal_ino is apparently intented to get returned back to user 
> space due to its unique-ness (and we have to trust pick_formal_ino() 
> does its job right). With normal readdir system call, after the inode 
> number is sent to user space, there is no route (I've checked) for it to 
> come back to kernel. So the only user that would use these filldir ino 
> inside the kernel is NFSD. NFSD calls gfs2_ilookup() that requires 
> no_formal_ino.
> 
no_formal_ino is not ever guaranteed to be unique which is why I want to
keep it away from userspace. I don't really want to have to change that
in order to fix NFS.

As you say there is no route for the inode number returned via readdir
to come back to the kernel, but it should match the inode number
returned by stat and I don't see that we should change either of them
just to get NFS to work when userspace is perfectly ok as it is.

> If you look further... The current lookup code actually uses 
> no_formal_ino, not no_addr. The two main "gate" routines that controls 
> ino-to-inode conversion are:
> 
> * gfs2_ilookup() (used by NFS route)
> * gfs2_inode_lookup() (used by VFS that calls gfs2_lookup()).
> 
Neither of them need to use no_formal_ino at all. The lookup is using
no_addr as there is no other index which makes sense. I know that
no_formal_ino is used as part of the hash, but it shouldn't be really
and its trivial to change that. You can't swap to just using
no_formal_ino on its own as there is no mapping to find its on-disk
location from no_formal_ino.

> Both use no_formal_ino - gfs2_inode_lookup() logic hides this behind the 
> little wrapper "gfs2_iget()". Since current VFS side's lookup has been 
> working fine, this no_formal_ino idea apparently is working. So let's 
> make NFSD side work the *same* way. I'm convinced this patch does a 
> right thing.
> 
> I don't dispute using generation number may not be a bad idea and may 
> perform better. However, if we really look into the details, it is not 
> easy for current code structure. Since we have something already 
> working, it is not wise to mess this code layout at this moment.
> 
> Make sense ?
> 
> -- Wendy
> 
I'm afraid that I'm still not convinced on that. It seems that ext3 uses
the method that I'm proposing in that it looks up the inode by inode
number and then only afterwards checks the generation number. It also
has the advantage of separating the NFS interface from the rest of the
filesystem. That seems to me to be a lot of the problem that we are
looking at here in that the VFS's lookup function wants to look up by
no_addr and (currently) NFS is asking for something slightly different,

Steve.




^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-26 15:45         ` Steven Whitehouse
@ 2007-02-26 16:11           ` Jonathan E Brassow
  2007-02-26 17:16             ` Steven Whitehouse
  0 siblings, 1 reply; 14+ messages in thread
From: Jonathan E Brassow @ 2007-02-26 16:11 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Please note, I haven't been following current GFS2 development...

On Feb 26, 2007, at 9:45 AM, Steven Whitehouse wrote:

> Hi,
>
> On Fri, 2007-02-23 at 22:56 -0500, Wendy Cheng wrote:
>> Steve,
>>
>> I gave this issue some more thoughts - would like to suggest we take
>> this patch (at least for now) since it aligns with current code base.
>>
>> The no_formal_ino is apparently intented to get returned back to user
>> space due to its unique-ness (and we have to trust pick_formal_ino()
>> does its job right). With normal readdir system call, after the inode
>> number is sent to user space, there is no route (I've checked) for it 
>> to
>> come back to kernel. So the only user that would use these filldir ino
>> inside the kernel is NFSD. NFSD calls gfs2_ilookup() that requires
>> no_formal_ino.
>>
> no_formal_ino is not ever guaranteed to be unique which is why I want 
> to
> keep it away from userspace. I don't really want to have to change that
> in order to fix NFS.

Why is it not unique?  (from Ken's doc on GFS2:  "If the filesystem 
allocated 1 billion inodes per second, it would take over 500 years for 
roll-over.")

> As you say there is no route for the inode number returned via readdir
> to come back to the kernel, but it should match the inode number
> returned by stat and I don't see that we should change either of them
> just to get NFS to work when userspace is perfectly ok as it is.

There were alot of things done to make NFS work properly with GFS - 
many of them through great pain, because GFS is a clustered FS.

>> If you look further... The current lookup code actually uses
>> no_formal_ino, not no_addr. The two main "gate" routines that controls
>> ino-to-inode conversion are:
>>
>> * gfs2_ilookup() (used by NFS route)
>> * gfs2_inode_lookup() (used by VFS that calls gfs2_lookup()).
>>
> Neither of them need to use no_formal_ino at all. The lookup is using
> no_addr as there is no other index which makes sense. I know that
> no_formal_ino is used as part of the hash, but it shouldn't be really
> and its trivial to change that. You can't swap to just using
> no_formal_ino on its own as there is no mapping to find its on-disk
> location from no_formal_ino.
>
>> Both use no_formal_ino - gfs2_inode_lookup() logic hides this behind 
>> the
>> little wrapper "gfs2_iget()". Since current VFS side's lookup has been
>> working fine, this no_formal_ino idea apparently is working. So let's
>> make NFSD side work the *same* way. I'm convinced this patch does a
>> right thing.
>>
>> I don't dispute using generation number may not be a bad idea and may
>> perform better. However, if we really look into the details, it is not
>> easy for current code structure. Since we have something already
>> working, it is not wise to mess this code layout at this moment.
>>
>> Make sense ?
>>
>> -- Wendy
>>
> I'm afraid that I'm still not convinced on that. It seems that ext3 
> uses
> the method that I'm proposing in that it looks up the inode by inode
> number and then only afterwards checks the generation number. It also
> has the advantage of separating the NFS interface from the rest of the
> filesystem. That seems to me to be a lot of the problem that we are
> looking at here in that the VFS's lookup function wants to look up by
> no_addr and (currently) NFS is asking for something slightly different,
>

How do you plan on handling inode migration when using NFS?  I believe 
that was one of the central reasons for having no_formal_inode.  There 
may be other reasons as well, as I am not familiar with the code.

  brassow



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-26 16:11           ` Jonathan E Brassow
@ 2007-02-26 17:16             ` Steven Whitehouse
  2007-02-27 20:04               ` Wendy Cheng
  0 siblings, 1 reply; 14+ messages in thread
From: Steven Whitehouse @ 2007-02-26 17:16 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Hi,

On Mon, 2007-02-26 at 10:11 -0600, Jonathan E Brassow wrote:
> Please note, I haven't been following current GFS2 development...
> 
> On Feb 26, 2007, at 9:45 AM, Steven Whitehouse wrote:
> 
> > Hi,
> >
> > On Fri, 2007-02-23 at 22:56 -0500, Wendy Cheng wrote:
> >> Steve,
> >>
> >> I gave this issue some more thoughts - would like to suggest we take
> >> this patch (at least for now) since it aligns with current code base.
> >>
> >> The no_formal_ino is apparently intented to get returned back to user
> >> space due to its unique-ness (and we have to trust pick_formal_ino()
> >> does its job right). With normal readdir system call, after the inode
> >> number is sent to user space, there is no route (I've checked) for it 
> >> to
> >> come back to kernel. So the only user that would use these filldir ino
> >> inside the kernel is NFSD. NFSD calls gfs2_ilookup() that requires
> >> no_formal_ino.
> >>
> > no_formal_ino is not ever guaranteed to be unique which is why I want 
> > to
> > keep it away from userspace. I don't really want to have to change that
> > in order to fix NFS.
>
> Why is it not unique?  (from Ken's doc on GFS2:  "If the filesystem 
> allocated 1 billion inodes per second, it would take over 500 years for 
> roll-over.")
> 
Granted it will take a long time, but it does rather look a bit like
another "640k will be enough for anyone" type prediction. The main
problem here being that when it happens there will be no way to tell
that it has happened.

> > As you say there is no route for the inode number returned via readdir
> > to come back to the kernel, but it should match the inode number
> > returned by stat and I don't see that we should change either of them
> > just to get NFS to work when userspace is perfectly ok as it is.
> 
> There were alot of things done to make NFS work properly with GFS - 
> many of them through great pain, because GFS is a clustered FS.
> 
> >> If you look further... The current lookup code actually uses
> >> no_formal_ino, not no_addr. The two main "gate" routines that controls
> >> ino-to-inode conversion are:
> >>
> >> * gfs2_ilookup() (used by NFS route)
> >> * gfs2_inode_lookup() (used by VFS that calls gfs2_lookup()).
> >>
> > Neither of them need to use no_formal_ino at all. The lookup is using
> > no_addr as there is no other index which makes sense. I know that
> > no_formal_ino is used as part of the hash, but it shouldn't be really
> > and its trivial to change that. You can't swap to just using
> > no_formal_ino on its own as there is no mapping to find its on-disk
> > location from no_formal_ino.
> >
> >> Both use no_formal_ino - gfs2_inode_lookup() logic hides this behind 
> >> the
> >> little wrapper "gfs2_iget()". Since current VFS side's lookup has been
> >> working fine, this no_formal_ino idea apparently is working. So let's
> >> make NFSD side work the *same* way. I'm convinced this patch does a
> >> right thing.
> >>
> >> I don't dispute using generation number may not be a bad idea and may
> >> perform better. However, if we really look into the details, it is not
> >> easy for current code structure. Since we have something already
> >> working, it is not wise to mess this code layout at this moment.
> >>
> >> Make sense ?
> >>
> >> -- Wendy
> >>
> > I'm afraid that I'm still not convinced on that. It seems that ext3 
> > uses
> > the method that I'm proposing in that it looks up the inode by inode
> > number and then only afterwards checks the generation number. It also
> > has the advantage of separating the NFS interface from the rest of the
> > filesystem. That seems to me to be a lot of the problem that we are
> > looking at here in that the VFS's lookup function wants to look up by
> > no_addr and (currently) NFS is asking for something slightly different,
> >
> 
> How do you plan on handling inode migration when using NFS?  I believe 
> that was one of the central reasons for having no_formal_inode.  There 
> may be other reasons as well, as I am not familiar with the code.
> 
>   brassow
> 
The problem with no_formal_ino is that it doesn't allow inode migration
since there is no index in which its possible to lookup no_formal_ino
and find out the on-disk location of the inode. If we had such an index
then I'd be suggesting using no_formal_ino and not no_addr as our inode
number.

There are other issues as well. The glock number is determined by
no_addr (thats not something I've changed, btw) and in order to migrate
an inode it really needs to be kept under the same lock, otherwise all
kinds of odd things might happen.

So it seemed easier to use the no_addr as the inode's unique number and
try to keep everything consistent for now. This should then allow the
addition of some kind of indirection layer later on, to allow migration.

In the mean time no_formal_ino will still exist, its just that it won't
be used as the lookup key, so theres no on-disk format change to
consider here, all the same fields will continue to have the same
values, so there should be no problem to use it again if required at a
later date,

Steve.




^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-26 17:16             ` Steven Whitehouse
@ 2007-02-27 20:04               ` Wendy Cheng
  2007-02-28  9:03                 ` Steven Whitehouse
  0 siblings, 1 reply; 14+ messages in thread
From: Wendy Cheng @ 2007-02-27 20:04 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Steven Whitehouse wrote:
> In the mean time no_formal_ino will still exist, its just that it won't
> be used as the lookup key, so theres no on-disk format change to
> consider here, all the same fields will continue to have the same
> values, so there should be no problem to use it again if required at a
> later date,
>
>   
ok, as long as we don't rush into removing the code, this is an 
acceptable plan. We can discuss the issue further when we meet 
face-to-face next month. In the mean time, will go ahead to make lookup 
code consistent by tentatively putting no_formal_ino aside. Without the 
changes, half of the current NFS lookups would fail. We need to have 
something working right now.

I also invite Kent Baxley (one of our TAMs) here to help out with the 
issue. I recalled he mentioned sometime ago that the "create" table in 
GFS2-mySQL benchmark was noticeably slow - not sure whether 
pick_formal_ino() plays any role there. Will give him two gfs2.ko to 
re-run the benchmark and see what we get.

-- Wendy




^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-27 20:04               ` Wendy Cheng
@ 2007-02-28  9:03                 ` Steven Whitehouse
  2007-02-28 16:24                   ` Wendy Cheng
  0 siblings, 1 reply; 14+ messages in thread
From: Steven Whitehouse @ 2007-02-28  9:03 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Hi,

On Tue, 2007-02-27 at 15:04 -0500, Wendy Cheng wrote:
> Steven Whitehouse wrote:
> > In the mean time no_formal_ino will still exist, its just that it won't
> > be used as the lookup key, so theres no on-disk format change to
> > consider here, all the same fields will continue to have the same
> > values, so there should be no problem to use it again if required at a
> > later date,
> >
> >   
> ok, as long as we don't rush into removing the code, this is an 
> acceptable plan. We can discuss the issue further when we meet 
> face-to-face next month. In the mean time, will go ahead to make lookup 
> code consistent by tentatively putting no_formal_ino aside. Without the 
> changes, half of the current NFS lookups would fail. We need to have 
> something working right now.
> 
Yes, agreed.

> I also invite Kent Baxley (one of our TAMs) here to help out with the 
> issue. I recalled he mentioned sometime ago that the "create" table in 
> GFS2-mySQL benchmark was noticeably slow - not sure whether 
> pick_formal_ino() plays any role there. Will give him two gfs2.ko to 
> re-run the benchmark and see what we get.
> 
> -- Wendy
> 
Probably it doesn't make much difference. I did some tests a while back when trying
to work out why file creates are slow and I discovered that removing the
pick_formal_ino code made no noticeable difference to the create speed,
so the bottleneck appears to be elsewhere,

Steve.




^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-28  9:03                 ` Steven Whitehouse
@ 2007-02-28 16:24                   ` Wendy Cheng
  2007-02-28 16:26                     ` Wendy Cheng
  2007-02-28 16:46                     ` Steven Whitehouse
  0 siblings, 2 replies; 14+ messages in thread
From: Wendy Cheng @ 2007-02-28 16:24 UTC (permalink / raw)
  To: cluster-devel.redhat.com

ok, the following is the minimum changes to get NFSD going before we 
settle down this issue .. would appreciate this in the tree so other NFS 
related works can get done in parallel.

-- Wendy



-------------- next part --------------
A non-text attachment was scrubbed...
Name: gfs2_inum.patch
Type: text/x-patch
Size: 789 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20070228/689c764c/attachment.bin>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-28 16:24                   ` Wendy Cheng
@ 2007-02-28 16:26                     ` Wendy Cheng
  2007-02-28 16:46                     ` Steven Whitehouse
  1 sibling, 0 replies; 14+ messages in thread
From: Wendy Cheng @ 2007-02-28 16:26 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Wendy Cheng wrote:
> ok, the following is the minimum changes to get NFSD going before we 
> settle down this issue .. would appreciate this in the tree so other 
> NFS related works can get done in parallel.
>
Should have said this passes cthon04 test suites this morning... Wendy
>
>
>
> ------------------------------------------------------------------------
>
>  Signed-off-by: S. Wendy Cheng <wcheng@redhat.com>
>
>  inode.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> --- linux-feb-17/fs/gfs2/inode.c	2007-02-17 20:07:16.000000000 -0500
> +++ up-kernel/fs/gfs2/inode.c	2007-02-28 11:34:26.000000000 -0500
> @@ -61,13 +61,13 @@ static int iget_set(struct inode *inode,
>  
>  struct inode *gfs2_ilookup(struct super_block *sb, struct gfs2_inum_host *inum)
>  {
> -	return ilookup5(sb, (unsigned long)inum->no_formal_ino,
> +	return ilookup5(sb, (unsigned long)inum->no_addr,
>  			iget_test, inum);
>  }
>  
>  static struct inode *gfs2_iget(struct super_block *sb, struct gfs2_inum_host *inum)
>  {
> -	return iget5_locked(sb, (unsigned long)inum->no_formal_ino,
> +	return iget5_locked(sb, (unsigned long)inum->no_addr,
>  		     iget_test, iget_set, inum);
>  }
>  
>   



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main
  2007-02-28 16:24                   ` Wendy Cheng
  2007-02-28 16:26                     ` Wendy Cheng
@ 2007-02-28 16:46                     ` Steven Whitehouse
  1 sibling, 0 replies; 14+ messages in thread
From: Steven Whitehouse @ 2007-02-28 16:46 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Hi,

Now in the git tree. Thanks,

Steve.

On Wed, 2007-02-28 at 11:24 -0500, Wendy Cheng wrote:
> ok, the following is the minimum changes to get NFSD going before we 
> settle down this issue .. would appreciate this in the tree so other NFS 
> related works can get done in parallel.
> 
> -- Wendy
> 
> 
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2007-02-28 16:46 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-02-07  4:51 [Cluster-devel] [GFS2 Patch] pass formal ino in do_filldir_main Wendy Cheng
2007-02-07 10:49 ` Steven Whitehouse
2007-02-07 22:35   ` Wendy Cheng
2007-02-07 23:37     ` Wendy Cheng
2007-02-08  9:42     ` Steven Whitehouse
2007-02-24  3:56       ` Wendy Cheng
2007-02-26 15:45         ` Steven Whitehouse
2007-02-26 16:11           ` Jonathan E Brassow
2007-02-26 17:16             ` Steven Whitehouse
2007-02-27 20:04               ` Wendy Cheng
2007-02-28  9:03                 ` Steven Whitehouse
2007-02-28 16:24                   ` Wendy Cheng
2007-02-28 16:26                     ` Wendy Cheng
2007-02-28 16:46                     ` Steven Whitehouse

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.