Re: [PATCH] Btrfs: update inode flags when renaming
On Mon, Feb 25, 2013 at 12:23:03PM +0800, Miao Xie wrote: Onmon, 25 Feb 2013 11:50:01 +0800, Liu Bo wrote: On Fri, Feb 22, 2013 at 11:04:40PM +0100, David Sterba wrote: On Fri, Feb 22, 2013 at 05:34:47PM +0800, Miao Xie wrote: Onfri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote: On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote: Sorry, but the bug persists even with the above patch. touch test chattr +C test lsattr test mv test test2 lsattr test2 In the above scenario test2 will not have the C flag. What do you expect? IMO it's right that test2 does not have the C flag. No, it's not right. For the users, they expect the C flag is not lost because they just do a rename operation. but fixup_inode_flags() re-sets the flags by the parent directory's flag. I think we should inherit the flags from the parent just when we create a new file/directory, in the other cases, just give a option to the users. How do you think about? I agree with that. The COW status of a file should not be changed at all when renamed. The typical users are database files and vm images, losing the NOCOW flag just from moving here and back is quite unexpected. david Yeah, I agree to remove this bad 'change in rename', will send a patch to address it. I think we can add a mount option, if the option is set, when we move a file to a new directory, or create a new file, we will inherit the flags of the parent. If not set, we inherit the flags only when create a new file. How do you think about it? I'd rather see mount options being used for filesystem-wide (or subvolume-wide) things, and renaming files is a local operation for which a finer grained control via chattr seems more fit. david -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: update inode flags when renaming
On Fri, Feb 22, 2013 at 11:04:40PM +0100, David Sterba wrote: On Fri, Feb 22, 2013 at 05:34:47PM +0800, Miao Xie wrote: On fri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote: On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote: Sorry, but the bug persists even with the above patch. touch test chattr +C test lsattr test mv test test2 lsattr test2 In the above scenario test2 will not have the C flag. What do you expect? IMO it's right that test2 does not have the C flag. No, it's not right. For the users, they expect the C flag is not lost because they just do a rename operation. but fixup_inode_flags() re-sets the flags by the parent directory's flag. I think we should inherit the flags from the parent just when we create a new file/directory, in the other cases, just give a option to the users. How do you think about? I agree with that. The COW status of a file should not be changed at all when renamed. The typical users are database files and vm images, losing the NOCOW flag just from moving here and back is quite unexpected. david Yeah, I agree to remove this bad 'change in rename', will send a patch to address it. thanks, liubo -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: update inode flags when renaming
On mon, 25 Feb 2013 11:50:01 +0800, Liu Bo wrote: On Fri, Feb 22, 2013 at 11:04:40PM +0100, David Sterba wrote: On Fri, Feb 22, 2013 at 05:34:47PM +0800, Miao Xie wrote: On fri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote: On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote: Sorry, but the bug persists even with the above patch. touch test chattr +C test lsattr test mv test test2 lsattr test2 In the above scenario test2 will not have the C flag. What do you expect? IMO it's right that test2 does not have the C flag. No, it's not right. For the users, they expect the C flag is not lost because they just do a rename operation. but fixup_inode_flags() re-sets the flags by the parent directory's flag. I think we should inherit the flags from the parent just when we create a new file/directory, in the other cases, just give a option to the users. How do you think about? I agree with that. The COW status of a file should not be changed at all when renamed. The typical users are database files and vm images, losing the NOCOW flag just from moving here and back is quite unexpected. david Yeah, I agree to remove this bad 'change in rename', will send a patch to address it. I think we can add a mount option, if the option is set, when we move a file to a new directory, or create a new file, we will inherit the flags of the parent. If not set, we inherit the flags only when create a new file. How do you think about it? Thanks Miao thanks, liubo -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: update inode flags when renaming
On Mon, Feb 25, 2013 at 12:23:03PM +0800, Miao Xie wrote: Onmon, 25 Feb 2013 11:50:01 +0800, Liu Bo wrote: On Fri, Feb 22, 2013 at 11:04:40PM +0100, David Sterba wrote: On Fri, Feb 22, 2013 at 05:34:47PM +0800, Miao Xie wrote: Onfri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote: On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote: Sorry, but the bug persists even with the above patch. touch test chattr +C test lsattr test mv test test2 lsattr test2 In the above scenario test2 will not have the C flag. What do you expect? IMO it's right that test2 does not have the C flag. No, it's not right. For the users, they expect the C flag is not lost because they just do a rename operation. but fixup_inode_flags() re-sets the flags by the parent directory's flag. I think we should inherit the flags from the parent just when we create a new file/directory, in the other cases, just give a option to the users. How do you think about? I agree with that. The COW status of a file should not be changed at all when renamed. The typical users are database files and vm images, losing the NOCOW flag just from moving here and back is quite unexpected. david Yeah, I agree to remove this bad 'change in rename', will send a patch to address it. I think we can add a mount option, if the option is set, when we move a file to a new directory, or create a new file, we will inherit the flags of the parent. If not set, we inherit the flags only when create a new file. How do you think about it? I'm ok with the option, 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 -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: update inode flags when renaming
Sorry, but the bug persists even with the above patch.
touch test
chattr +C test
lsattr test
mv test test2
lsattr test2
In the above scenario test2 will not have the C flag.
On Fri, Feb 22, 2013 at 3:11 AM, Liu Bo bo.li@oracle.com wrote:
A user reported some weird behaviours,
if we move a file with the noCow flag to a directory without the
noCow flag, the file is now without the flag, but after remount,
we'll find the file's noCow flag comes back.
This is because we missed a proper inode update after inheriting
parent directory's flags,
Reported-by: Marios Titas redneb8...@gmail.com
Signed-off-by: Liu Bo bo.li@oracle.com
---
fs/btrfs/inode.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d9984fa..d2e3352 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7478,8 +7478,6 @@ static int btrfs_rename(struct inode *old_dir, struct
dentry *old_dentry,
old_dentry-d_inode,
old_dentry-d_name.name,
old_dentry-d_name.len);
- if (!ret)
- ret = btrfs_update_inode(trans, root, old_inode);
}
if (ret) {
btrfs_abort_transaction(trans, root, ret);
@@ -7514,6 +7512,11 @@ static int btrfs_rename(struct inode *old_dir, struct
dentry *old_dentry,
}
fixup_inode_flags(new_dir, old_inode);
+ ret = btrfs_update_inode(trans, root, old_inode);
+ if (ret) {
+ btrfs_abort_transaction(trans, root, ret);
+ goto out_fail;
+ }
ret = btrfs_add_link(trans, new_dir, old_inode,
new_dentry-d_name.name,
--
1.7.7.6
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: update inode flags when renaming
Wouldn't though inheriting create all sorts of problems? For instance
check the example that I give in my other responese [1].
[1] http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg22396.html
On Fri, Feb 22, 2013 at 4:34 AM, Miao Xie mi...@cn.fujitsu.com wrote:
On fri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote:
On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
Sorry, but the bug persists even with the above patch.
touch test
chattr +C test
lsattr test
mv test test2
lsattr test2
In the above scenario test2 will not have the C flag.
What do you expect? IMO it's right that test2 does not have the C flag.
No, it's not right.
For the users, they expect the C flag is not lost because they just do
a rename operation. but fixup_inode_flags() re-sets the flags by the
parent directory's flag.
I think we should inherit the flags from the parent just when we create
a new file/directory, in the other cases, just give a option to the users.
How do you think about?
Thanks
Miao
This patch ensure that we get the same result after we remount, no more
the C flag coming back :)
thanks,
liubo
On Fri, Feb 22, 2013 at 3:11 AM, Liu Bo bo.li@oracle.com wrote:
A user reported some weird behaviours,
if we move a file with the noCow flag to a directory without the
noCow flag, the file is now without the flag, but after remount,
we'll find the file's noCow flag comes back.
This is because we missed a proper inode update after inheriting
parent directory's flags,
Reported-by: Marios Titas redneb8...@gmail.com
Signed-off-by: Liu Bo bo.li@oracle.com
---
fs/btrfs/inode.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d9984fa..d2e3352 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7478,8 +7478,6 @@ static int btrfs_rename(struct inode *old_dir,
struct dentry *old_dentry,
old_dentry-d_inode,
old_dentry-d_name.name,
old_dentry-d_name.len);
- if (!ret)
- ret = btrfs_update_inode(trans, root, old_inode);
}
if (ret) {
btrfs_abort_transaction(trans, root, ret);
@@ -7514,6 +7512,11 @@ static int btrfs_rename(struct inode *old_dir,
struct dentry *old_dentry,
}
fixup_inode_flags(new_dir, old_inode);
+ ret = btrfs_update_inode(trans, root, old_inode);
+ if (ret) {
+ btrfs_abort_transaction(trans, root, ret);
+ goto out_fail;
+ }
ret = btrfs_add_link(trans, new_dir, old_inode,
new_dentry-d_name.name,
--
1.7.7.6
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: update inode flags when renaming
On Fri, Feb 22, 2013 at 04:10:37AM -0500, Marios Titas wrote:
You are right, your patch does improve the situation a bit. But it
still does not address the main issue. To illustrate that, consider
the following scenario:
Sorry for so much confusion for users.
Please let me explain the following senario,
touch test
chattr +C test
head -c 1048576 /dev/zero test
mv test test2
now test2 lost the C flag because it was renamed. But the data in
test2 was written before it lost the C flag and so the extents do not
have checksums!
Also try to clone it with BTRFS_IOC_CLONE. It fails as if it had the C flag:
cp --reflink test2 test3
We don't clone a file when the src(test2) file has NODATASUM and the dest(test3)
file does not have NODATASUM, or vice versa. This ensures our checksum's valid.
Here,
* test2 does has NODATASUM because test has NODATASUM, while
* test3 is a new created file, and we're not with '-o nodatasum' or
'-o nodatacow' mount options or we don't chattr test3,
so test3 does not have NODATASUM flags set.
So 'cp' ends up 'INVALID'.
OTOH, if you try to clone over a file with NODATACOW then it works:
touch test3
chattr +C test3
cp --reflink test2 test3
Now test3 is with NODATACOW, so the above 'cp' works.
so the file is in an incosistent state: it sometimes behaves as if it
had the NODATACOW flag and sometimes as if it didn't.
The C flag refers to NODATACOW, this NODATACOW is used to tell btrfs
if we write the file's data on COW mode.
So the failure of 'clone' does not equal to the file is NODATACOW.
Feel free to correct me.
thanks,
liubo
Thanks
On Fri, Feb 22, 2013 at 3:40 AM, Liu Bo bo.li@oracle.com wrote:
On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
Sorry, but the bug persists even with the above patch.
touch test
chattr +C test
lsattr test
mv test test2
lsattr test2
In the above scenario test2 will not have the C flag.
What do you expect? IMO it's right that test2 does not have the C flag.
This patch ensure that we get the same result after we remount, no more
the C flag coming back :)
thanks,
liubo
On Fri, Feb 22, 2013 at 3:11 AM, Liu Bo bo.li@oracle.com wrote:
A user reported some weird behaviours,
if we move a file with the noCow flag to a directory without the
noCow flag, the file is now without the flag, but after remount,
we'll find the file's noCow flag comes back.
This is because we missed a proper inode update after inheriting
parent directory's flags,
Reported-by: Marios Titas redneb8...@gmail.com
Signed-off-by: Liu Bo bo.li@oracle.com
---
fs/btrfs/inode.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d9984fa..d2e3352 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7478,8 +7478,6 @@ static int btrfs_rename(struct inode *old_dir,
struct dentry *old_dentry,
old_dentry-d_inode,
old_dentry-d_name.name,
old_dentry-d_name.len);
- if (!ret)
- ret = btrfs_update_inode(trans, root, old_inode);
}
if (ret) {
btrfs_abort_transaction(trans, root, ret);
@@ -7514,6 +7512,11 @@ static int btrfs_rename(struct inode *old_dir,
struct dentry *old_dentry,
}
fixup_inode_flags(new_dir, old_inode);
+ ret = btrfs_update_inode(trans, root, old_inode);
+ if (ret) {
+ btrfs_abort_transaction(trans, root, ret);
+ goto out_fail;
+ }
ret = btrfs_add_link(trans, new_dir, old_inode,
new_dentry-d_name.name,
--
1.7.7.6
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: update inode flags when renaming
On Fri, Feb 22, 2013 at 05:34:47PM +0800, Miao Xie wrote: Onfri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote: On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote: Sorry, but the bug persists even with the above patch. touch test chattr +C test lsattr test mv test test2 lsattr test2 In the above scenario test2 will not have the C flag. What do you expect? IMO it's right that test2 does not have the C flag. No, it's not right. For the users, they expect the C flag is not lost because they just do a rename operation. but fixup_inode_flags() re-sets the flags by the parent directory's flag. I think we should inherit the flags from the parent just when we create a new file/directory, in the other cases, just give a option to the users. How do you think about? I agree with that. The COW status of a file should not be changed at all when renamed. The typical users are database files and vm images, losing the NOCOW flag just from moving here and back is quite unexpected. david -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: update inode flags when renaming
On Fri, Feb 22, 2013 at 04:19:27PM -0500, Marios Titas wrote: I think that many end users will find all this very confusing. They will never expect that renaming a file will cause it to suddenly lose one flag (NODATACOW) while preserving the other (NODATASUM). Especially since they cannot explicitly control the NODATASUM flag on a per file basis. I think that renaming a file should preserve all flags no matter if it's done in the same directory or not. Just like it preserves permissions, ownership and inode number. I agree. For completeness, the other inherited flags/attributes are compression statuses. Silently changing them on remove may be wrong in case the file gains the 'never try to compress' flag after some clever heuristic (which we do not have yet) decides so. So I think inheriting the flags from the parent on rename is not a good idea either. Interestingly enough, files don't lose any of the two flags if instead of renaming you link and then unlink the original. Link does not take the same codepath as new file or rename. A new directory entry is created and link count increased. david -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
