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

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

[Jan Kara]

  Hi,

  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

-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gfs2-2.6.21-rc6-propagate_flags.diff
Type: text/x-patch
Size: 4621 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20070423/d9dceb72/attachment.bin>

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

[Steven Whitehouse]

Hi,

On Mon, 2007-04-23 at 20:41 +0200, Jan Kara wrote:
>   Hi,
> 
>   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?

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
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.

        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,

Steve.

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

[Jan Kara]

  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