[Cluster-devel] [GFS2 PATCH]

[Cluster-devel] [GFS2 PATCH]

[Bob Peterson]

Hi,

This patch fixes a problem whereby gfs2_grow was failing and causing GFS2
to assert. The problem was that when GFS2's fallocate operation tried to
acquire an "allocation" it made sure the rindex was up to date, and if not,
it called gfs2_rindex_update. However, if the file being fallocated was
the rindex itself, it was already locked at that point. By calling
gfs2_rindex_update at an earlier point in time, we bring rindex up to date
and thereby avoid trying to lock it when the "allocation" is acquired.

Regards,

Bob Peterson
Red Hat File Systems

Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
--
 fs/gfs2/file.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 310f2fb..52db4c4 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -774,6 +774,11 @@ static long gfs2_fallocate(struct file *file, int mode, loff_t offset,
 	if (bytes == 0)
 		bytes = sdp->sd_sb.sb_bsize;
 
+	error = gfs2_rindex_update(sdp);
+	if (error) {
+		fs_warn(sdp, "rindex update returns %d\n", error);
+		return error;
+	}
 	gfs2_holder_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &ip->i_gh);
 	error = gfs2_glock_nq(&ip->i_gh);
 	if (unlikely(error))

[Cluster-devel] [GFS2 PATCH]

[Steven Whitehouse]

Hi,

If we are going to do this, then perhaps we should consider reading in
the rindex on mount? That way it will always be uptodate, and we can
refuse to mount if the rindex is damaged which is probably cleaner than
doing it after the event.

The only concern is the time taken to mount large filesystems. Having
said that the rindex should be contiguous on disk in most cases, so it
should be a fairly fast operation. Worth considering, anyway I think,

Steve.

On Fri, 2012-02-17 at 09:15 -0500, Bob Peterson wrote:
> Hi,
> 
> This patch fixes a problem whereby gfs2_grow was failing and causing GFS2
> to assert. The problem was that when GFS2's fallocate operation tried to
> acquire an "allocation" it made sure the rindex was up to date, and if not,
> it called gfs2_rindex_update. However, if the file being fallocated was
> the rindex itself, it was already locked at that point. By calling
> gfs2_rindex_update at an earlier point in time, we bring rindex up to date
> and thereby avoid trying to lock it when the "allocation" is acquired.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
> --
>  fs/gfs2/file.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index 310f2fb..52db4c4 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -774,6 +774,11 @@ static long gfs2_fallocate(struct file *file, int mode, loff_t offset,
>  	if (bytes == 0)
>  		bytes = sdp->sd_sb.sb_bsize;
>  
> +	error = gfs2_rindex_update(sdp);
> +	if (error) {
> +		fs_warn(sdp, "rindex update returns %d\n", error);
> +		return error;
> +	}
>  	gfs2_holder_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &ip->i_gh);
>  	error = gfs2_glock_nq(&ip->i_gh);
>  	if (unlikely(error))
> 

[Cluster-devel] [GFS2 PATCH]

[Bob Peterson]

----- Original Message -----
| Hi,
| 
| If we are going to do this, then perhaps we should consider reading
| in
| the rindex on mount? That way it will always be uptodate, and we can
| refuse to mount if the rindex is damaged which is probably cleaner
| than
| doing it after the event.
| 
| The only concern is the time taken to mount large filesystems. Having
| said that the rindex should be contiguous on disk in most cases, so
| it
| should be a fairly fast operation. Worth considering, anyway I think,
| 
| Steve.

Hi,

That's not a bad idea, and we should consider it for a future enhancement.
However, I think these checks still need to be here because there are
other ways the rindex can get out of date and need to be re-read after
mount. For example, if there was another intermediate gfs2_grow done on a
different node.

BTW, I assume you saw my other patch from yesterday regarding gfs2_unlink, right?

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [GFS2 PATCH]

[Steven Whitehouse]

Hi,

On Fri, 2012-02-17 at 09:34 -0500, Bob Peterson wrote:
> ----- Original Message -----
> | Hi,
> | 
> | If we are going to do this, then perhaps we should consider reading
> | in
> | the rindex on mount? That way it will always be uptodate, and we can
> | refuse to mount if the rindex is damaged which is probably cleaner
> | than
> | doing it after the event.
> | 
> | The only concern is the time taken to mount large filesystems. Having
> | said that the rindex should be contiguous on disk in most cases, so
> | it
> | should be a fairly fast operation. Worth considering, anyway I think,
> | 
> | Steve.
> 
> Hi,
> 
> That's not a bad idea, and we should consider it for a future enhancement.
> However, I think these checks still need to be here because there are
> other ways the rindex can get out of date and need to be re-read after
> mount. For example, if there was another intermediate gfs2_grow done on a
> different node.
> 
There is no locking here to prevent the rindex from becoming out of date
again, immediately after this call, so that might still be picked up by
he lower level code and result in another problem if I'm reading the
original bug report correctly.

An alternative way to resolve this is just to check whether the rindex
is already locked before trying to lock it again at the lower level. Or,
I guess, we could even do both.

> BTW, I assume you saw my other patch from yesterday regarding gfs2_unlink, right?
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems

Yes, I was just about to take a look at it - I've been a little waylaid
with other things in the last couple of days,

Steve.

[Cluster-devel] [GFS2 PATCH]

[Steven Whitehouse]

Hi,

On Fri, 2012-02-17 at 09:34 -0500, Bob Peterson wrote:
> ----- Original Message -----
> | Hi,
> | 
> | If we are going to do this, then perhaps we should consider reading
> | in
> | the rindex on mount? That way it will always be uptodate, and we can
> | refuse to mount if the rindex is damaged which is probably cleaner
> | than
> | doing it after the event.
> | 
> | The only concern is the time taken to mount large filesystems. Having
> | said that the rindex should be contiguous on disk in most cases, so
> | it
> | should be a fairly fast operation. Worth considering, anyway I think,
> | 
> | Steve.
> 
> Hi,
> 
> That's not a bad idea, and we should consider it for a future enhancement.
> However, I think these checks still need to be here because there are
> other ways the rindex can get out of date and need to be re-read after
> mount. For example, if there was another intermediate gfs2_grow done on a
> different node.
> 
> BTW, I assume you saw my other patch from yesterday regarding gfs2_unlink, right?
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems

Both patches now pushed to the -nmw tree, but I'd like to see a more
comprehensive fix for this in due course,

Steve.

[Cluster-devel] [GFS2 PATCH]

[Bob Peterson]

Hi,

I noticed that the latest change to i_height got rid of the
value from the inode dump.  This patch adds it back.

Regards,

Bob Peterson
Red Hat GFS

Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
--
 fs/gfs2/inode.c       |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 084c741..11c5ce4 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) Sistina Software, Inc.  1997-2003 All rights reserved.
- * Copyright (C) 2004-2006 Red Hat, Inc.  All rights reserved.
+ * Copyright (C) 2004-2008 Red Hat, Inc.  All rights reserved.
  *
  * This copyrighted material is made available to anyone wishing to use,
  * modify, copy, or redistribute it subject to the terms and conditions
@@ -1435,6 +1435,7 @@ void gfs2_dinode_print(const struct gfs2_inode *ip)
 	printk(KERN_INFO "  di_goal_data = %llu\n",
 	       (unsigned long long)di->di_goal_data);
 	printk(KERN_INFO "  di_flags = 0x%.8X\n", di->di_flags);
+	printk(KERN_INFO "  i_height = %u\n", ip->i_height);
 	printk(KERN_INFO "  di_depth = %u\n", di->di_depth);
 	printk(KERN_INFO "  di_entries = %u\n", di->di_entries);
 	printk(KERN_INFO "  di_eattr = %llu\n",

[Cluster-devel] [GFS2 PATCH]

[Steven Whitehouse]

Hi,

Now in the -nmw tree. Thanks,

Steve.

On Mon, 2008-01-28 at 11:15 -0600, Bob Peterson wrote:
> Hi,
> 
> I noticed that the latest change to i_height got rid of the
> value from the inode dump.  This patch adds it back.
> 
> Regards,
> 
> Bob Peterson
> Red Hat GFS
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
> --
>  fs/gfs2/inode.c       |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 084c741..11c5ce4 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -1,6 +1,6 @@
>  /*
>   * Copyright (C) Sistina Software, Inc.  1997-2003 All rights reserved.
> - * Copyright (C) 2004-2006 Red Hat, Inc.  All rights reserved.
> + * Copyright (C) 2004-2008 Red Hat, Inc.  All rights reserved.
>   *
>   * This copyrighted material is made available to anyone wishing to use,
>   * modify, copy, or redistribute it subject to the terms and conditions
> @@ -1435,6 +1435,7 @@ void gfs2_dinode_print(const struct gfs2_inode *ip)
>  	printk(KERN_INFO "  di_goal_data = %llu\n",
>  	       (unsigned long long)di->di_goal_data);
>  	printk(KERN_INFO "  di_flags = 0x%.8X\n", di->di_flags);
> +	printk(KERN_INFO "  i_height = %u\n", ip->i_height);
>  	printk(KERN_INFO "  di_depth = %u\n", di->di_depth);
>  	printk(KERN_INFO "  di_entries = %u\n", di->di_entries);
>  	printk(KERN_INFO "  di_eattr = %llu\n",
> 
>