[Cluster-devel] [GFS2] Fix problems relating to execution of files on GFS2

[Cluster-devel] [GFS2] Fix problems relating to execution of files on GFS2

[Steven Whitehouse]

Hi,

This patch fixes a couple of problems which affected the execution of files on GFS2.
The first is that there was a corner case where inodes were not always uptodate at
the point at which permissions checks were being carried out, this was resulting in
refusal of execute permission, but only on the first lookup, subsequent requests
worked correctly. The second was a problem relating to incorrect updating of file sizes
which was introduced with the write_begin/end code for GFS2 a little while back.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>

diff --git a/fs/gfs2/ops_address.c b/fs/gfs2/ops_address.c
index e16ad81..37406a3 100644
--- a/fs/gfs2/ops_address.c
+++ b/fs/gfs2/ops_address.c
@@ -848,14 +848,11 @@ static int gfs2_write_end(struct file *file, struct address_space *mapping,
 
 	ret = generic_write_end(file, mapping, pos, len, copied, page, fsdata);
 
-	if (likely(ret >= 0)) {
-		copied = ret;
-		if  ((pos + copied) > inode->i_size) {
-			di = (struct gfs2_dinode *)dibh->b_data;
-			ip->i_di.di_size = inode->i_size;
-			di->di_size = cpu_to_be64(inode->i_size);
-			mark_inode_dirty(inode);
-		}
+	if (likely(ret >= 0) && (inode->i_size > ip->i_di.di_size)) {
+		di = (struct gfs2_dinode *)dibh->b_data;
+		ip->i_di.di_size = inode->i_size;
+		di->di_size = cpu_to_be64(inode->i_size);
+		mark_inode_dirty(inode);
 	}
 
 	if (inode == sdp->sd_rindex)
diff --git a/fs/gfs2/ops_inode.c b/fs/gfs2/ops_inode.c
index 291f0c7..8386ab3 100644
--- a/fs/gfs2/ops_inode.c
+++ b/fs/gfs2/ops_inode.c
@@ -113,8 +113,18 @@ static struct dentry *gfs2_lookup(struct inode *dir, struct dentry *dentry,
 	if (inode && IS_ERR(inode))
 		return ERR_PTR(PTR_ERR(inode));
 
-	if (inode)
+	if (inode) {
+		struct gfs2_glock *gl = GFS2_I(inode)->i_gl;
+		struct gfs2_holder gh;
+		int error;
+		error = gfs2_glock_nq_init(gl, LM_ST_SHARED, LM_FLAG_ANY, &gh);
+		if (error) {
+			iput(inode);
+			return ERR_PTR(error);
+		}
+		gfs2_glock_dq_uninit(&gh);
 		return d_splice_alias(inode, dentry);
+	}
 	d_add(dentry, inode);
 
 	return NULL;

[Cluster-devel] [GFS2] Fix problems relating to execution of files on GFS2

[Wendy Cheng]

Steven Whitehouse wrote:
> --- a/fs/gfs2/ops_inode.c
> +++ b/fs/gfs2/ops_inode.c
> @@ -113,8 +113,18 @@ static struct dentry *gfs2_lookup(struct inode *dir, struct dentry *dentry,
>  	if (inode && IS_ERR(inode))
>  		return ERR_PTR(PTR_ERR(inode));
>  
> -	if (inode)
> +	if (inode) {
> +		struct gfs2_glock *gl = GFS2_I(inode)->i_gl;
> +		struct gfs2_holder gh;
> +		int error;
> +		error = gfs2_glock_nq_init(gl, LM_ST_SHARED, LM_FLAG_ANY, &gh);
>   

ok, so this shared glock is now added *back*.  As I recall, its removal 
about one year ago had caused great grief in NFS portion of logic - had 
to do a tedious work to make NFS work due to this change. Now, the logic 
is reverted - has performance impact been measured (since it is a disk 
read) ?

-- Wendy
> +		if (error) {
> +			iput(inode);
> +			return ERR_PTR(error);
> +		}
> +		gfs2_glock_dq_uninit(&gh);
>  		return d_splice_alias(inode, dentry);
> +	}
>  	d_add(dentry, inode);
>  
>  	return NULL;
>
>
>   

[Cluster-devel] [GFS2] Fix problems relating to execution of files on GFS2

[Steven Whitehouse]

Hi,

On Mon, 2008-01-07 at 13:11 -0500, Wendy Cheng wrote:
> Steven Whitehouse wrote:
> > --- a/fs/gfs2/ops_inode.c
> > +++ b/fs/gfs2/ops_inode.c
> > @@ -113,8 +113,18 @@ static struct dentry *gfs2_lookup(struct inode *dir, struct dentry *dentry,
> >  	if (inode && IS_ERR(inode))
> >  		return ERR_PTR(PTR_ERR(inode));
> >  
> > -	if (inode)
> > +	if (inode) {
> > +		struct gfs2_glock *gl = GFS2_I(inode)->i_gl;
> > +		struct gfs2_holder gh;
> > +		int error;
> > +		error = gfs2_glock_nq_init(gl, LM_ST_SHARED, LM_FLAG_ANY, &gh);
> >   
> 
> ok, so this shared glock is now added *back*.  As I recall, its removal 
> about one year ago had caused great grief in NFS portion of logic - had 
> to do a tedious work to make NFS work due to this change. Now, the logic 
> is reverted - has performance impact been measured (since it is a disk 
> read) ?
> 
> -- Wendy

Well yes and no. Its not in the same place as before, its only in the
VFS lookup code and not in the GFS2 internal lookup code, and I really
didn't want to have to add it back here if it could at all be avoided,
but since there is no dcache revalidate in the initial lookup case it
seems we have no choice now :(

Some tests with postmark show no measurable difference in performance so
far, so maybe its not so bad. We still need to get to the point where we
can use lookup intents and solve the problem properly,

Steve.