[PATCH] Btrfs: do not change inode flags in rename

[PATCH] Btrfs: do not change inode flags in rename

[Liu Bo]

Before we forced to change a file's NOCOW and COMPRESS flag due to
the parent directory's, but this ends up a bad idea, because it
confuses end users a lot about file's NOCOW status, eg. if someone
change a file to NOCOW via 'chattr' and then rename it in the current
directory which is without NOCOW attribute, the file will lose the
NOCOW flag silently.

This diables 'change flags in rename', so from now on we'll only
inherit flags from the parent directory on creation stage while in
other places we can use 'chattr' to set NOCOW or COMPRESS flags.

Reported-by: Marios Titas <redneb8888@gmail.com>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/inode.c |   25 -------------------------
 1 files changed, 0 insertions(+), 25 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d9984fa..383a7d8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7325,29 +7325,6 @@ static int btrfs_getattr(struct vfsmount *mnt,
 	return 0;
 }
 
-/*
- * If a file is moved, it will inherit the cow and compression flags of the new
- * directory.
- */
-static void fixup_inode_flags(struct inode *dir, struct inode *inode)
-{
-	struct btrfs_inode *b_dir = BTRFS_I(dir);
-	struct btrfs_inode *b_inode = BTRFS_I(inode);
-
-	if (b_dir->flags & BTRFS_INODE_NODATACOW)
-		b_inode->flags |= BTRFS_INODE_NODATACOW;
-	else
-		b_inode->flags &= ~BTRFS_INODE_NODATACOW;
-
-	if (b_dir->flags & BTRFS_INODE_COMPRESS) {
-		b_inode->flags |= BTRFS_INODE_COMPRESS;
-		b_inode->flags &= ~BTRFS_INODE_NOCOMPRESS;
-	} else {
-		b_inode->flags &= ~(BTRFS_INODE_COMPRESS |
-				    BTRFS_INODE_NOCOMPRESS);
-	}
-}
-
 static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 			   struct inode *new_dir, struct dentry *new_dentry)
 {
@@ -7513,8 +7490,6 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		}
 	}
 
-	fixup_inode_flags(new_dir, old_inode);
-
 	ret = btrfs_add_link(trans, new_dir, old_inode,
 			     new_dentry->d_name.name,
 			     new_dentry->d_name.len, 0, index);
-- 
1.7.7.6

Re: [PATCH] Btrfs: do not change inode flags in rename

[David Sterba]

On Mon, Feb 25, 2013 at 12:04:42PM +0800, Liu Bo wrote:
> Before we forced to change a file's NOCOW and COMPRESS flag due to
> the parent directory's, but this ends up a bad idea, because it
> confuses end users a lot about file's NOCOW status, eg. if someone
> change a file to NOCOW via 'chattr' and then rename it in the current
> directory which is without NOCOW attribute, the file will lose the
> NOCOW flag silently.
> 
> This diables 'change flags in rename', so from now on we'll only
> inherit flags from the parent directory on creation stage while in
> other places we can use 'chattr' to set NOCOW or COMPRESS flags.
> 
> Reported-by: Marios Titas <redneb8888@gmail.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Thank you!
Reviewed-by: David Sterba <dsterba@suse.cz>

Re: [PATCH] Btrfs: do not change inode flags in rename

[Liu Bo]

(add Miao to CC)
On Mon, Feb 25, 2013 at 12:04:42PM +0800, Liu Bo wrote:
> Before we forced to change a file's NOCOW and COMPRESS flag due to
> the parent directory's, but this ends up a bad idea, because it
> confuses end users a lot about file's NOCOW status, eg. if someone
> change a file to NOCOW via 'chattr' and then rename it in the current
> directory which is without NOCOW attribute, the file will lose the
> NOCOW flag silently.
> 
> This diables 'change flags in rename', so from now on we'll only
> inherit flags from the parent directory on creation stage while in
> other places we can use 'chattr' to set NOCOW or COMPRESS flags.
> 
> Reported-by: Marios Titas <redneb8888@gmail.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/inode.c |   25 -------------------------
>  1 files changed, 0 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d9984fa..383a7d8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7325,29 +7325,6 @@ static int btrfs_getattr(struct vfsmount *mnt,
>  	return 0;
>  }
>  
> -/*
> - * If a file is moved, it will inherit the cow and compression flags of the new
> - * directory.
> - */
> -static void fixup_inode_flags(struct inode *dir, struct inode *inode)
> -{
> -	struct btrfs_inode *b_dir = BTRFS_I(dir);
> -	struct btrfs_inode *b_inode = BTRFS_I(inode);
> -
> -	if (b_dir->flags & BTRFS_INODE_NODATACOW)
> -		b_inode->flags |= BTRFS_INODE_NODATACOW;
> -	else
> -		b_inode->flags &= ~BTRFS_INODE_NODATACOW;
> -
> -	if (b_dir->flags & BTRFS_INODE_COMPRESS) {
> -		b_inode->flags |= BTRFS_INODE_COMPRESS;
> -		b_inode->flags &= ~BTRFS_INODE_NOCOMPRESS;
> -	} else {
> -		b_inode->flags &= ~(BTRFS_INODE_COMPRESS |
> -				    BTRFS_INODE_NOCOMPRESS);
> -	}
> -}
> -
>  static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  			   struct inode *new_dir, struct dentry *new_dentry)
>  {
> @@ -7513,8 +7490,6 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  		}
>  	}
>  
> -	fixup_inode_flags(new_dir, old_inode);
> -
>  	ret = btrfs_add_link(trans, new_dir, old_inode,
>  			     new_dentry->d_name.name,
>  			     new_dentry->d_name.len, 0, index);
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: do not change inode flags in rename

[Josef Bacik]

On Sun, Feb 24, 2013 at 09:04:42PM -0700, Liu Bo wrote:
> Before we forced to change a file's NOCOW and COMPRESS flag due to
> the parent directory's, but this ends up a bad idea, because it
> confuses end users a lot about file's NOCOW status, eg. if someone
> change a file to NOCOW via 'chattr' and then rename it in the current
> directory which is without NOCOW attribute, the file will lose the
> NOCOW flag silently.
> 
> This diables 'change flags in rename', so from now on we'll only
> inherit flags from the parent directory on creation stage while in
> other places we can use 'chattr' to set NOCOW or COMPRESS flags.
>

I'm of the mind we definitely shouldn't drop flags we've set previously, but I
think we should also inherit any flags we have set on the directory, so if we
move a file into a NOCOW directory we should inherit the flag.  I'm not married
to the idea, but it seems to make the most sense to me.  Thanks,

Josef 

Re: [PATCH] Btrfs: do not change inode flags in rename

[Liu Bo]

On Mon, Feb 25, 2013 at 01:56:47PM -0500, Josef Bacik wrote:
> On Sun, Feb 24, 2013 at 09:04:42PM -0700, Liu Bo wrote:
> > Before we forced to change a file's NOCOW and COMPRESS flag due to
> > the parent directory's, but this ends up a bad idea, because it
> > confuses end users a lot about file's NOCOW status, eg. if someone
> > change a file to NOCOW via 'chattr' and then rename it in the current
> > directory which is without NOCOW attribute, the file will lose the
> > NOCOW flag silently.
> > 
> > This diables 'change flags in rename', so from now on we'll only
> > inherit flags from the parent directory on creation stage while in
> > other places we can use 'chattr' to set NOCOW or COMPRESS flags.
> >
> 
> I'm of the mind we definitely shouldn't drop flags we've set previously, but I
> think we should also inherit any flags we have set on the directory, so if we
> move a file into a NOCOW directory we should inherit the flag.  I'm not married
> to the idea, but it seems to make the most sense to me.  Thanks,
> 
> Josef 

Hi Josef,

(Said in another thread)
I'm ok with either one, but...
from some reports on the list, end users are more likely to control, use chattr
files by themselves, inheriting flags via moving a file to a new directory is
indeed not very welcomed.

So for practical use, I assume that it's fairly enough to inherit flags only on
creation?

thanks,
liubo

Re: [PATCH] Btrfs: do not change inode flags in rename

[David Sterba]

On Tue, Feb 26, 2013 at 08:11:17AM +0800, Liu Bo wrote:
> On Mon, Feb 25, 2013 at 01:56:47PM -0500, Josef Bacik wrote:
> > On Sun, Feb 24, 2013 at 09:04:42PM -0700, Liu Bo wrote:
> > > Before we forced to change a file's NOCOW and COMPRESS flag due to
> > > the parent directory's, but this ends up a bad idea, because it
> > > confuses end users a lot about file's NOCOW status, eg. if someone
> > > change a file to NOCOW via 'chattr' and then rename it in the current
> > > directory which is without NOCOW attribute, the file will lose the
> > > NOCOW flag silently.
> > > 
> > > This diables 'change flags in rename', so from now on we'll only
> > > inherit flags from the parent directory on creation stage while in
> > > other places we can use 'chattr' to set NOCOW or COMPRESS flags.
> > >
> > 
> > I'm of the mind we definitely shouldn't drop flags we've set previously, but I
> > think we should also inherit any flags we have set on the directory, so if we
> > move a file into a NOCOW directory we should inherit the flag.  I'm not married
> > to the idea, but it seems to make the most sense to me.  Thanks,
> > 
> (Said in another thread)
> I'm ok with either one, but...
> from some reports on the list, end users are more likely to control, use chattr
> files by themselves, inheriting flags via moving a file to a new directory is
> indeed not very welcomed.
> 
> So for practical use, I assume that it's fairly enough to inherit flags only on
> creation?

I still haven't figured out in what cases the silent flag inheritance
(for a non-empty) file would help and the user would be happy that it
works like this.

david

Re: [PATCH] Btrfs: do not change inode flags in rename

[Josef Bacik]

On Tue, Feb 26, 2013 at 05:33:00AM -0700, David Sterba wrote:
> On Tue, Feb 26, 2013 at 08:11:17AM +0800, Liu Bo wrote:
> > On Mon, Feb 25, 2013 at 01:56:47PM -0500, Josef Bacik wrote:
> > > On Sun, Feb 24, 2013 at 09:04:42PM -0700, Liu Bo wrote:
> > > > Before we forced to change a file's NOCOW and COMPRESS flag due to
> > > > the parent directory's, but this ends up a bad idea, because it
> > > > confuses end users a lot about file's NOCOW status, eg. if someone
> > > > change a file to NOCOW via 'chattr' and then rename it in the current
> > > > directory which is without NOCOW attribute, the file will lose the
> > > > NOCOW flag silently.
> > > > 
> > > > This diables 'change flags in rename', so from now on we'll only
> > > > inherit flags from the parent directory on creation stage while in
> > > > other places we can use 'chattr' to set NOCOW or COMPRESS flags.
> > > >
> > > 
> > > I'm of the mind we definitely shouldn't drop flags we've set previously, but I
> > > think we should also inherit any flags we have set on the directory, so if we
> > > move a file into a NOCOW directory we should inherit the flag.  I'm not married
> > > to the idea, but it seems to make the most sense to me.  Thanks,
> > > 
> > (Said in another thread)
> > I'm ok with either one, but...
> > from some reports on the list, end users are more likely to control, use chattr
> > files by themselves, inheriting flags via moving a file to a new directory is
> > indeed not very welcomed.
> > 
> > So for practical use, I assume that it's fairly enough to inherit flags only on
> > creation?
> 
> I still haven't figured out in what cases the silent flag inheritance
> (for a non-empty) file would help and the user would be happy that it
> works like this.
> 

K, I'll take this as it is then and drop my previous patch.  Thanks,

Josef