[Cluster-devel] Re: [PATCH] Copy i_flags to gfs2 inode flags on write

[Jan Kara <jack@suse.cz>]

  Hi,

On Tue 24-04-07 11:05:12, Steven Whitehouse wrote:
> On Mon, 2007-04-23 at 20:41 +0200, Jan Kara wrote:
> >   attached is a patch to copy i_flags to GFS2 specific i_di_di_flags. The
> > patch is compile-tested only. Steven, does it look OK to you?
> > 
> > 								Honza
> > 
> I can't see why we need this at the moment... what other interface is
> there aside from the ioctl() for setting these flags?
  Hm, VFS quota subsystem sets flags on quota files. Given that GFS2
doesn't use VFS quota, you are right that currently there's noone who
could change the flags in GFS2 inode. But still I'd find it more correct to
store the flags, especially if it does not cost you much. So do you agree
with the idea of the patch?

> Also:
> 
>         diff -rupX /home/jack/.kerndiffexclude linux-2.6.21-rc6-4-ocfs2_flags_update/fs/gfs2/ops_file.c linux-2.6.21-rc6-5-gfs2_flags_update/fs/gfs2/ops_file.c
>         ---
>         linux-2.6.21-rc6-4-ocfs2_flags_update/fs/gfs2/ops_file.c    2007-04-10 17:09:55.000000000 +0200
>         +++
>         linux-2.6.21-rc6-5-gfs2_flags_update/fs/gfs2/ops_file.c     2007-04-23 20:10:55.000000000 +0200
>         @@ -207,6 +207,7 @@ static int gfs2_get_flags(struct file *f
>                 if (error)
>                         return error;
>          
>         +       gfs2_get_inode_flags(ip);
>                 fsflags = fsflags_cvt(gfs2_to_fsflags,
>         ip->i_di.di_flags);
> 
> Why do you call gfs2_get_inode_flags() here? There is no reason that the
  I guess what I have written above explains it...

> gfs2 flags will not match the vfs inode flags at this point that I can
> see (its a bug if they don't match). It is not valid to change the flags
> while only holding a read lock and when outside of a transaction anyway.
  Oh, right. So just merging GFS2 flags with VFS inode flags to some
temporary variable should fix the problem.
  
>         diff -rupX /home/jack/.kerndiffexclude linux-2.6.21-rc6-4-ocfs2_flags_update/fs/gfs2/ops_file.h linux-2.6.21-rc6-5-gfs2_flags_update/fs/gfs2/ops_file.h
>         ---
>         linux-2.6.21-rc6-4-ocfs2_flags_update/fs/gfs2/ops_file.h    2007-02-07 12:03:23.000000000 +0100
>         +++
>         linux-2.6.21-rc6-5-gfs2_flags_update/fs/gfs2/ops_file.h     2007-04-23 20:11:43.000000000 +0200
>         @@ -18,6 +18,7 @@ extern int gfs2_internal_read(struct gfs
>                                       struct file_ra_state *ra_state,
>                                       char *buf, loff_t *pos, unsigned
>         size);
>          extern void gfs2_set_inode_flags(struct inode *inode);
>         +extern void gfs2_get_inode_flags(struct gfs2_inode *inode);
> 
> Please use *ip for struct gfs2_inode and *inode for struct inode in
> common with the rest of the code as it helps avoid confusion,
  Ups, I swear I wanted to ;). But somehow "inode" crept in...

									Honza

-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs